Skip to content

fix: enhance PDF viewer security by removing file parameters and validating URLs #37934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
papphelix wants to merge 3 commits into
base: master
Choose a base branch
from
+202 −122
Open

fix: enhance PDF viewer security by removing file parameters and validating URLs #37934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
55e1f91
fix: enhance PDF viewer security by removing file parameters and vali…
viv-helix Jan 13, 2026
6a7f831
fix: remove arbitrary file parameter from PDF viewer
papphelix Jan 23, 2026
ffd8e6b
Merge branch 'master' into fix/pdf-arbitary-file-param
papphelix Jan 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions common/static/js/vendor/pdfjs/viewer.js
View file
Open in desktop
Copy link
Contributor

@feanil feanil Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a vendored file so we don't want to make changes to it to ease transition to newer versions of the vendored library.

Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

'use strict';

var DEFAULT_URL = 'compressed.tracemonkey-pldi-09.pdf';
var DEFAULT_URL = '';
var DEFAULT_SCALE_DELTA = 1.1;
var MIN_SCALE = 0.25;
var MAX_SCALE = 10.0;
Expand Down Expand Up @@ -1291,7 +1291,7 @@ var PDFFindController = (function PDFFindControllerClosure() {
if (this.pagesToSearch < 0) {
// No point in wrapping again, there were no matches.
this.updateMatch(false);
// while matches were not found, searching for a page
// while matches were not found, searching for a page
// with matches should nevertheless halt.
return true;
}
Expand Down Expand Up @@ -1324,7 +1324,7 @@ var PDFFindController = (function PDFFindControllerClosure() {
offset.matchIdx = null;

this.pagesToSearch--;

if (offset.pageIdx >= numPages || offset.pageIdx < 0) {
offset.pageIdx = (previous ? numPages - 1 : 0);
offset.wrapped = true;
Expand All @@ -1335,7 +1335,7 @@ var PDFFindController = (function PDFFindControllerClosure() {
var state = FindStates.FIND_NOTFOUND;
var wrapped = this.offset.wrapped;
this.offset.wrapped = false;

if (found) {
var previousPage = this.selected.pageIdx;
this.selected.pageIdx = this.offset.pageIdx;
Expand All @@ -1346,7 +1346,7 @@ var PDFFindController = (function PDFFindControllerClosure() {
this.updatePage(previousPage);
}
}

this.updateUIState(state, this.state.findPrevious);
if (this.selected.pageIdx !== -1) {
this.updatePage(this.selected.pageIdx);
Expand Down Expand Up @@ -2708,7 +2708,7 @@ var DocumentProperties = {

parseDate: function documentPropertiesParseDate(inputDate) {
// This is implemented according to the PDF specification (see
// http://www.gnupdf.org/Date for an overview), but note that
// http://www.gnupdf.org/Date for an overview), but note that
// Adobe Reader doesn't handle changing the date to universal time
// and doesn't use the user's time zone (they're effectively ignoring
// the HH' and mm' parts of the date string).
Expand Down
44 changes: 28 additions & 16 deletions lms/djangoapps/staticbook/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,25 +129,32 @@ def test_book(self):
url = self.make_url('pdf_book', book_index=0)
response = self.client.get(url)
self.assertContains(response, "Chapter 1 for PDF")
self.assertNotContains(response, "options.chapterNum =")
self.assertNotContains(response, "page=")
# Verify file parameter is not present (security fix)
self.assertNotContains(response, "file=")
# Verify postMessage infrastructure is in place
self.assertContains(response, "request_pdf_url")
self.assertContains(response, "pdf_url_response")

def test_book_chapter(self):
# We can access a book at a particular chapter.
self.make_course(pdf_textbooks=[PDF_BOOK])
url = self.make_url('pdf_book', book_index=0, chapter=2)
response = self.client.get(url)
self.assertContains(response, "Chapter 2 for PDF")
self.assertContains(response, "file={}".format(PDF_BOOK['chapters'][1]['url']))
self.assertNotContains(response, "page=")
# Verify file parameter is not present anywhere (security fix)
self.assertNotContains(response, "file=")
# Verify postMessage infrastructure is in place
self.assertContains(response, "request_pdf_url")

def test_book_page(self):
# We can access a book at a particular page.
self.make_course(pdf_textbooks=[PDF_BOOK])
url = self.make_url('pdf_book', book_index=0, page=17)
response = self.client.get(url)
self.assertContains(response, "Chapter 1 for PDF")
self.assertNotContains(response, "options.chapterNum =")
# Verify file parameter is not present (security fix)
self.assertNotContains(response, "file=")
# Page parameter is still used in viewer_params
self.assertContains(response, "page=17")

def test_book_chapter_page(self):
Expand All @@ -156,7 +163,9 @@ def test_book_chapter_page(self):
url = self.make_url('pdf_book', book_index=0, chapter=2, page=17)
response = self.client.get(url)
self.assertContains(response, "Chapter 2 for PDF")
self.assertContains(response, "file={}".format(PDF_BOOK['chapters'][1]['url']))
# Verify file parameter is not present (security fix)
self.assertNotContains(response, "file=")
# Page parameter is still used in viewer_params
self.assertContains(response, "page=17")

def test_bad_book_id(self):
Expand Down Expand Up @@ -202,29 +211,32 @@ def test_chapter_page_xss(self):

def test_static_url_map_contentstore(self):
"""
This ensure static URL mapping is happening properly for
a course that uses the contentstore
This ensure static URL mapping is happening properly for
a course that uses the contentstore.
URLs are remapped in backend but not exposed via file parameter (security fix).
"""
self.make_course(pdf_textbooks=[PORTABLE_PDF_BOOK])
url = self.make_url('pdf_book', book_index=0, chapter=1)
response = self.client.get(url)
self.assertNotContains(response, 'file={}'.format(PORTABLE_PDF_BOOK['chapters'][0]['url']))
self.assertContains(response, 'file=/asset-v1:{0.org}+{0.course}+{0.run}+type@asset+block/{1}'.format(
# Verify file parameter is not present in response (security fix)
self.assertNotContains(response, 'file=')
# Verify the chapter URL is in the sidebar for postMessage communication
self.assertContains(response, '/asset-v1:{0.org}+{0.course}+{0.run}+type@asset+block/{1}'.format(
self.course.location,
PORTABLE_PDF_BOOK['chapters'][0]['url'].replace('/static/', '')))

def test_static_url_map_static_asset_path(self):
"""
Like above, but used when the course has set a static_asset_path
Like above, but used when the course has set a static_asset_path.
URLs are remapped in backend but not exposed via file parameter (security fix).
"""
self.make_course(pdf_textbooks=[PORTABLE_PDF_BOOK], static_asset_path='awesomesauce')
url = self.make_url('pdf_book', book_index=0, chapter=1)
response = self.client.get(url)
self.assertNotContains(response, 'file={}'.format(PORTABLE_PDF_BOOK['chapters'][0]['url']))
self.assertNotContains(response, 'file=/c4x/{0.org}/{0.course}/asset/{1}'.format(
self.course.location,
PORTABLE_PDF_BOOK['chapters'][0]['url'].replace('/static/', '')))
self.assertContains(response, 'file=/static/awesomesauce/{}'.format(
# Verify file parameter is not present anywhere (security fix)
self.assertNotContains(response, 'file=')
# Verify the remapped URL is in the sidebar for postMessage communication
self.assertContains(response, '/static/awesomesauce/{}'.format(
PORTABLE_PDF_BOOK['chapters'][0]['url'].replace('/static/', '')))

def test_invalid_chapter_id(self):
Expand Down
5 changes: 4 additions & 1 deletion lms/djangoapps/staticbook/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def pdf_index(request, course_id, book_index, chapter=None, page=None):
raise Http404(f"Invalid book index value: {book_index}")
textbook = course.pdf_textbooks[book_index]

viewer_params = '&file='
viewer_params = ''
current_url = ''

if 'url' in textbook:
Expand All @@ -99,6 +99,9 @@ def pdf_index(request, course_id, book_index, chapter=None, page=None):
if 'chapters' in textbook:
for entry in textbook['chapters']:
entry['url'] = remap_static_url(entry['url'], course)
# Security: Validate chapter URL doesn't contain dangerous schemes
if entry['url'].lower().startswith(('javascript:', 'data:', 'vbscript:', 'file:')):
entry['url'] = '' # Sanitize dangerous URLs
Copy link
Contributor

@feanil feanil Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to atleast log a message in the case where the URL might be malicious so that operators can notice and potentially react to the issue. The message probably needs to contain the malicious URL and the course it's in in order to respond to the issue.

if chapter is not None and int(chapter) <= (len(textbook['chapters'])):
current_chapter = textbook['chapters'][int(chapter) - 1]
else:
Expand Down
180 changes: 107 additions & 73 deletions lms/templates/pdf_viewer.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<%page expression_filter="h"/>
<!DOCTYPE html>
<!DOCTYPE html>
<%namespace name='static' file='static_content.html'/>
<%!
from openedx.core.djangolib.js_utils import (
Expand Down Expand Up @@ -46,10 +46,44 @@
PDFJS.workerSrc = "${static.url('js/vendor/pdfjs/pdf.worker.js') | n, js_escaped_string}";
PDFJS.disableWorker = true;
PDFJS.cMapUrl = "${static.url('css/vendor/pdfjs/cmaps/') | n, js_escaped_string}";
PDF_URL = '${current_url | n, js_escaped_string}';

var PDF_URL = '${current_url | n, js_escaped_string}';
Copy link
Contributor

@feanil feanil Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what the goal of this function is? It looks like previously we were just letting the pdf.js library load the PDF from the PDF_URL, what is the pupose of all this exctar loading logic?


if (window.parent !== window) {
window.parent.postMessage({type: 'request_pdf_url'}, '*');

function handlePdfUrlResponse(event) {
if (event.data && event.data.type === 'pdf_url_response') {
PDF_URL = event.data.url;

if (PDFViewerApplication.open) {
PDFViewerApplication.open(PDF_URL);
PDFViewerApplication.mouseScroll(0);

setTimeout(function() {
if (PDFViewerApplication.pdfDocument) {
if (event.data.title) document.getElementById('titleField').textContent = event.data.title;
if (event.data.author) document.getElementById('authorField').textContent = event.data.author;
if (event.data.subject) document.getElementById('subjectField').textContent = event.data.subject;
if (event.data.keywords) document.getElementById('keywordsField').textContent = event.data.keywords;
document.getElementById('creatorField').textContent = 'edX Platform';
}
}, 500);
}
} else if (event.data && event.data.type === 'chapter_change') {
window.parent.postMessage({type: 'request_pdf_url'}, '*');
}
}

window.addEventListener('message', handlePdfUrlResponse);
}

document.addEventListener('DOMContentLoaded', function () {
PDFViewerApplication && PDFViewerApplication.open(PDF_URL);
});
</script>

<script ${static.url('js/vendor/pdfjs/debugger.js')}></script>
<script type="text/javascript" src="${static.url('js/vendor/pdfjs/debugger.js')}"></script>

<%static:js group='main_vendor'/>
<%static:js group='application'/>
Expand Down Expand Up @@ -347,77 +381,77 @@

</div> <!-- outerContainer -->
<div id="printContainer"></div>
<div id="mozPrintCallback-shim" hidden>
<style scoped>
#mozPrintCallback-shim {
position: fixed;
top: 0;
left: 0;
height: 100%;
width: 100%;
z-index: 9999999;

display: block;
text-align: center;
background-color: rgba(0, 0, 0, 0.5);
}
#mozPrintCallback-shim[hidden] {
display: none;
}
@media print {
#mozPrintCallback-shim {
display: none;
}
}

#mozPrintCallback-shim .mozPrintCallback-dialog-box {
display: inline-block;
margin: -50px auto 0;
position: relative;
top: 45%;
left: 0;
min-width: 220px;
max-width: 400px;

padding: 9px;

border: 1px solid hsla(0, 0%, 0%, .5);
border-radius: 2px;
box-shadow: 0 1px 4px rgba(0, 0, 0, 0.3);

background-color: #474747;

color: hsl(0, 0%, 85%);
font-size: 16px;
line-height: 20px;
}
#mozPrintCallback-shim .progress-row {
clear: both;
padding: 1em 0;
}
#mozPrintCallback-shim progress {
width: 100%;
}
#mozPrintCallback-shim .relative-progress {
clear: both;
float: right;
}
#mozPrintCallback-shim .progress-actions {
clear: both;
}
</style>
<div class="mozPrintCallback-dialog-box">
<!-- TODO: Localise the following strings -->
Preparing document for printing...
<div class="progress-row">
<progress value="0" max="100"></progress>
<span class="relative-progress">0%</span>
</div>
<div class="progress-actions">
<input type="button" value="Cancel" class="mozPrintCallback-cancel">
<div id="mozPrintCallback-shim" hidden>
Copy link
Contributor

@feanil feanil Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's a formatting cleanup independent of this change, can you make this change in a separate PR to make the delta smaller and easier to review?

<style scoped>
#mozPrintCallback-shim {
position: fixed;
top: 0;
left: 0;
height: 100%;
width: 100%;
z-index: 9999999;

display: block;
text-align: center;
background-color: rgba(0, 0, 0, 0.5);
}
#mozPrintCallback-shim[hidden] {
display: none;
}
@media print {
#mozPrintCallback-shim {
display: none;
}
}

#mozPrintCallback-shim .mozPrintCallback-dialog-box {
display: inline-block;
margin: -50px auto 0;
position: relative;
top: 45%;
left: 0;
min-width: 220px;
max-width: 400px;

padding: 9px;

border: 1px solid hsla(0, 0%, 0%, .5);
border-radius: 2px;
box-shadow: 0 1px 4px rgba(0, 0, 0, 0.3);

background-color: #474747;

color: hsl(0, 0%, 85%);
font-size: 16px;
line-height: 20px;
}
#mozPrintCallback-shim .progress-row {
clear: both;
padding: 1em 0;
}
#mozPrintCallback-shim progress {
width: 100%;
}
#mozPrintCallback-shim .relative-progress {
clear: both;
float: right;
}
#mozPrintCallback-shim .progress-actions {
clear: both;
}
</style>
<div class="mozPrintCallback-dialog-box">
<!-- TODO: Localise the following strings -->
Preparing document for printing...
<div class="progress-row">
<progress value="0" max="100"></progress>
<span class="relative-progress">0%</span>
</div>
<div class="progress-actions">
<input type="button" value="Cancel" class="mozPrintCallback-cancel">
</div>
</div>
</div>
</div>
</div>
<script type="text/javascript" src="${static.url('js/vendor/pdfjs/viewer.js')}"></script>
<script type="text/javascript" src="${static.url('js/pdf-analytics.js')}"></script>
</body>
Expand Down
Loading
Loading