Skip to content
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.

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

Security fixes #130

Merged
merged 6 commits into from
May 28, 2024
Merged

Security fixes #130

merged 6 commits into from
May 28, 2024

Conversation

akohler
Copy link
Member

@akohler akohler commented May 24, 2024

Implements SYS-1612 and SYS-1621, with other minor changes. Tagged as version v1.1.7 for deployment.

This PR primarily fixes two bugs:

  1. Prevents master file downloads by anonymous users. The generic static serve view used with media/ paths is now wrapped by a custom serve_media_files view, which requires login. Otherwise, anyone who could figure out a URL for a master file could download it from anywhere. There also are tests covering this now.
  2. Enforces correct authorization for the delete_file view. The check for group membership has been added to the view itself, and a PermissionDenied exception is raised with HTTP 403 (and message logged) if violated. This protects against the unlikely, but possible, case where a valid but unauthorized user could figure out the URL to delete a file via the view, and called that URL directly.

Other minor changes:

  • Updated various packages for security fixes
  • Refactored some tests to remove duplicate code, fixed some copy/paste code, and simplify use of Django's test client for web requests
  • Tweaked sorting of files in upload_file to include file name as tertiary sort, after sequence and file code. This fixes a (visual) problem where submasters for PDF/XML files could be listed before masters, since there's no special submaster file codes for those.

Testing

(Re)start system to pick up changes from requirements.txt.
There are 107 tests, all should pass.

For extra manual testing / confirmation

  • File deletion
    • Log in as a user not in the "Oral History Staff" group (or temporarily remove it from your user).
    • Find a MediaFile.id, then go to http://127.0.0.1:8000/delete_file/that_id .
    • You should get a "403 Forbidden" page (and the file should not get deleted). The logs should show a PermissionDenied exception, along with something like
      WARNING 2024-05-24 12:16:16,204 oh_staff_ui.views views Unauthorized attempt to delete oh_static/pdf/submasters/fake-ce54fea54d-2-submaster.pdf (25475) by akohler
  • Master file access
    • Find the URL for a valid master file you've ingested locally, preferably one you can view via browser like PDF or XML.
    • Confirm you can view it via browser, when logged in to the application.
    • Try to retrieve it from outside the application, either another browser (not logged in) or via curl.
    • Anonymous browser: should redirect to the login screen
    • curl: should give you a 302 response, showing Location redirect to the login screen
$ curl -I http://127.0.0.1:8000/media/oh_masters/pdf/masters/fake-ce54fea54d-2-master.pdf
HTTP/1.1 302 Found
Date: Fri, 24 May 2024 19:19:27 GMT
Server: WSGIServer/0.2 CPython/3.11.5
Content-Type: text/html; charset=utf-8
Location: /accounts/login/?next=/media/oh_masters/pdf/masters/fake-ce54fea54d-2-master.pdf
X-Frame-Options: DENY
Vary: Cookie
X-Content-Type-Options: nosniff
Referrer-Policy: same-origin
Cross-Origin-Opener-Policy: same-origin

@akohler akohler requested a review from ztucker4 May 24, 2024 19:28
Copy link
Contributor

@ztucker4 ztucker4 left a comment

Choose a reason for hiding this comment

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

This looks good. I can no longer view files without being logged in, and can no longer delete files without being logged in and in the correct user group. The file sorting change also looks good, and thanks for the code cleanup in tests.py! Merging now.

@ztucker4 ztucker4 merged commit 8dbf4a8 into main May 28, 2024
@ztucker4 ztucker4 deleted the security_fixes branch May 28, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants