fix issue 62892: record/file match on secret and one-time url - #1934
ivis-kondo wants to merge 2 commits into
Conversation
…e urls - Add an identity check between the issued secret/one-time URL record (record_id, file_name) and the item/file actually being requested. - Fix the four management-operation handlers: each caught its own abort(403)/abort(404) in a broad `except Exception` and turned it into a 500 response. - Fix is_private_index() to correctly reject an item when it belongs to multiple indexes that are all non-public. - Add and update unit tests accordingly.
Reviewer's GuideThe PR prevents secret and one-time URL reuse against another record or file, fixes management handlers that converted intended 403/404 responses into 500s, corrects multi-index privacy evaluation, and adds comprehensive regression tests. Sequence diagram for validating secret and one-time URL targetssequenceDiagram
participant Client
participant DownloadView
participant Utils
participant URLRecord
Client->>DownloadView: Request record_id and file_name with token
DownloadView->>Utils: validate_url_download(record, filename, token)
Utils->>Utils: convert_token_into_obj(token, is_secret_url)
Utils->>Utils: ensure_url_record_matches(url_obj, record_id, file_name)
alt record and file match
Utils-->>DownloadView: Valid URL
DownloadView-->>Client: Allow download
else target mismatch or invalid URL record
Utils-->>DownloadView: False and unavailable message
DownloadView-->>Client: Reject download
end
Sequence diagram for safe secret and one-time URL managementsequenceDiagram
actor User
participant ManagementView
participant URLRecord
participant Utils
User->>ManagementView: Copy or delete URL
ManagementView->>Utils: can_manage_secret_url() or can_manage_onetime_url()
alt insufficient permissions
ManagementView-->>User: 403 Forbidden
else permitted
ManagementView->>URLRecord: get_by_id(url_id)
ManagementView->>Utils: ensure_url_record_matches(url_record, pid, file_name)
alt missing or mismatched URL record
ManagementView-->>User: 404 Not Found
else matching URL record
ManagementView->>URLRecord: create_download_url() or delete_logically()
ManagementView-->>User: Complete operation
end
end
Flow diagram for multi-index privacy evaluationflowchart TD
A[Record] --> B{path exists?}
B -- No --> C[Private]
B -- Yes --> D["Indexes.is_public_state_and_not_in_future(path)"]
D -- True --> E[Not private]
D -- False --> C
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. This changes the authorization boundary for secret and one-time download URLs: a defect in the record/file matching check could expose files through a valid token issued for another target, or incorrectly block access across all such URLs. Reverting removes the new check, but any files exposed while the faulty behavior was live would not be recovered by the revert.
Summary by Sourcery
Enforce target matching for issued download URLs and preserve correct authorization and not-found responses across URL management operations.
Bug Fixes:
Tests: