fix: restrict rendering PDFs from other origins #51
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a security fix to prevent rendering PDF textbooks from external origins. The implementation checks if URLs start with https:// and, if so, blocks them from being displayed in the PDF viewer by emptying the viewer_params and current_url variables. This prevents potential security issues with untrusted external content.
Key changes:
- Added origin checks to block external
https://URLs in PDF textbook rendering - Modified template to conditionally render iframe based on presence of
fileparameter - External URLs now result in an empty iframe instead of loading external content
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| lms/djangoapps/staticbook/views.py | Added security checks to detect and block external URLs (starting with https://) from being rendered in PDF viewer, clearing viewer_params and current_url for external URLs |
| lms/templates/static_pdfbook.html | Added conditional rendering of iframe based on whether file parameter exists in viewer_params, displaying empty iframe for blocked external URLs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Faraz32123
force-pushed
the
fix/restrict_entertaining_other_origin_PDFs
branch
from
c8d9637 to
5cf2abd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lms/djangoapps/staticbook/tests.py
Outdated
| self.assertContains(response, "Chapter 2 for PDF") | ||
| self.assertContains(response, "file={}".format(PDF_BOOK['chapters'][1]['url'])) | ||
| self.assertNotContains(response, "file={}".format(PDF_BOOK['chapters'][1]['url'])) | ||
| self.assertContains(response, "page=17") |
There was a problem hiding this comment.
This test is inconsistent with the security fix. Since PDF_BOOK uses external URLs (starting with 'https://'), the page parameter should NOT appear in the response according to the new security logic. The test should use assertNotContains instead:
self.assertNotContains(response, "page=17")This is consistent with the other test changes where external URLs are blocked.
| self.assertContains(response, "page=17") | |
| self.assertNotContains(response, "page=17") |
| current_url = current_chapter['url'] | ||
| if not current_url.startswith('https://'): | ||
| viewer_params = '&file=' | ||
| viewer_params += current_url |
There was a problem hiding this comment.
Logic issue: When chapters exist and the current chapter URL is external (starts with 'https://'), the code overwrites any previous viewer_params that might have been set from the textbook URL at lines 95-97. This means if a textbook has a non-external base URL but external chapter URLs, the base URL's viewer_params will be lost.
Additionally, the check at line 110 should also check for 'http://' URLs (not just 'https://') to properly block all external origins. Consider using current_url.startswith(('http://', 'https://')) for more comprehensive coverage.
lms/djangoapps/staticbook/views.py
Outdated
| textbook['url'] = remap_static_url(textbook['url'], course) | ||
| viewer_params += textbook['url'] | ||
| current_url = textbook['url'] | ||
| if not current_url.startswith('https://'): |
There was a problem hiding this comment.
Security: The check only blocks URLs starting with 'https://' but doesn't block 'http://' URLs. External origins could use insecure HTTP URLs to bypass this restriction. Consider checking for both protocols:
if not current_url.startswith(('http://', 'https://')):
viv-helix
requested review from
djoseph-apphelix,
papphelix and
pganesh-apphelix
viv-helix
force-pushed
the
fix/restrict_entertaining_other_origin_PDFs
branch
from
d0095af to
8b51cf5
Compare
Ticket: TNL2-452
Before:
Screen.Recording.2025-11-28.at.4.12.46.PM.mov
After:
Screen.Recording.2025-11-28.at.4.15.35.PM.mov