fix: enhance PDF viewer security by removing file parameters and validating URLs#37934
fix: enhance PDF viewer security by removing file parameters and validating URLs#37934papphelix wants to merge 3 commits intoopenedx:masterfrom
Conversation
feanil
left a comment
feanil
left a comment
There was a problem hiding this comment.
@papphelix I'm not super familiar with the JS for this page, can you add more comments and explainations for this code and why it's needed.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| 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}'; |
There was a problem hiding this comment.
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?
| </div> | ||
| <div class="progress-actions"> | ||
| <input type="button" value="Cancel" class="mozPrintCallback-cancel"> | ||
| <div id="mozPrintCallback-shim" hidden> |
There was a problem hiding this comment.
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?
| tabindex="-1" | ||
| seamless></iframe> | ||
| </div> | ||
| <iframe |
There was a problem hiding this comment.
This formatting change seems unnecessary to this PR and can also be in a separate PR if you feel strongly about making it.
3 participants
This pull request is to fix the file param in URL which is creating security issue by sending arbitrary data in query param which can put cms under risk of different attacks like
Issue Screenshots:
Fixes done:
pdf-file-param-removal.mov