fix(tickets): bind download/stream ticket resource_path to the request path#454
fix(tickets): bind download/stream ticket resource_path to the request path#454brandonrc wants to merge 1 commit into
Conversation
…t path
The backend ticket middleware compares the bound resource_path against
request.uri().path() by byte equality at consume time, so the minter must
send the exact absolute path the later request carries.
createDownloadTicket sent `${repoKey}/${artifactPath}` and createStreamTicket
sent `migration/${jobId}`. Both were non-absolute (rejected with
"resource_path must start with '/'") and would not have byte-matched the real
request path even if made absolute, so UI artifact downloads and migration
progress streaming failed.
Bind to the actual request paths:
- /api/v1/repositories/{repoKey}/download/{artifactPath}
- /api/v1/migrations/{jobId}/stream
Add regression tests asserting the resource_path argument for both call sites.
Fixes #453
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Accessibility review (WCAG 2.2 AA) — download/stream ticket resource_path fix.
This change is confined to API client logic (createDownloadTicket / createStreamTicket now bind the absolute resource_path) plus regression tests. No UI surface, no new controls, labels, dialogs, tables, or status messages. Nothing to assess against WCAG here.
One adjacent observation, not introduced by this PR: ticket-creation failures surface through the existing reject path; ensure wherever these tickets are consumed (download/stream trigger UI) a failed ticket is announced to AT via a live region rather than a silent no-op. Out of scope for this PR.
No accessibility concerns with this change.
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Frontend correctness review. This is the correct fix. I verified the byte-equality contract holds on both sides:
createDownloadTicketnow bindsresource_pathto/api/v1/repositories/${repoKey}/download/${artifactPath}, which is exactly whatgetDownloadUrlreturns (also unencoded). The browser download request inrepo-detail-content.tsx#handleDownloadisgetDownloadUrl(...) + "?ticket=...", and the backend comparesrequest.uri().path()(path only, query stripped), so the bound path matches the consumed path. Correct.createStreamTicketnow binds/api/v1/migrations/${jobId}/stream, which matches the URLcreateProgressStreambuilds before attaching the ticket as a query param. Correct.
The two regression tests pin the exact request shape, including asserting getDownloadUrl produces the same string the ticket binds to, which is the right way to lock this so a future change to one path forces a change to the other.
No blockers, no suggestions of substance.
💭 Encoding consistency (pre-existing, not introduced here)
Neither getDownloadUrl nor the new bound resource_path percent-encode artifactPath, so a path with a space or # could mismatch what the browser actually sends in request.uri().path() (the browser will encode the href). This is a pre-existing concern that predates this PR and the fix deliberately keeps the two sides identical, so it does not regress anything. Worth a follow-up issue to encode consistently on both the URL builder and the ticket binding, but out of scope here.
Touches src/lib/api/artifacts.ts / artifacts.test.ts, shared with #447 and #451. Different functions, so expect at most a mechanical test-file conflict. Flag merge order.
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Security/API review (comment-only).
Verified against the backend ticket middleware in backend/src/api/middleware/auth.rs:
ticket_path_allowed(line 743) doesSome(p) => p == request_path, an exact byte-equality check.request_pathisrequest.uri().path()(line 859), i.e. the full path including the/api/v1/...prefix, with the query string (?ticket=) already stripped.
The change is correct. The new resource_path (/api/v1/repositories/${repoKey}/download/${artifactPath} and /api/v1/migrations/${jobId}/stream) now matches exactly what the consuming request carries. The previous values (${repoKey}/${artifactPath}, migration/${jobId}) could never byte-match request.uri().path(), so tickets were effectively always rejected. This is a real fix, not a loosening of the check.
Confirmed the consume path is identical in repo-detail-content.tsx:182-186: getDownloadUrl(repoKey, artifact.path) builds the href and createDownloadTicket(repoKey, artifact.path) mints the ticket from the same raw interpolation, so both stay byte-identical.
No over-broad binding: the bound path is fully-qualified to a single resource, so one ticket authenticates exactly one path (the tightest possible binding). No SSRF/open-redirect: the value is a server-relative path with no host/scheme component, and repoKey/path originate from the artifact list rather than free-form user input at mint time.
Minor (non-blocking, pre-existing): neither side percent-encodes artifactPath, so a path containing ? or # would truncate identically on both sides. Not introduced by this PR and not reachable with valid artifact paths. Consider normalising encoding in getDownloadUrl in a follow-up so the mint and consume paths share one helper.
No blocking issues.
|
Shipped in v1.2.0. The changes from this PR were folded into the release integration branch #466, which was squash-merged to main and tagged as v1.2.0. The code is already on main, so closing this as superseded by #466 rather than merging it (re-applying would just duplicate what already shipped). No work lost. See #466 for the consolidated record. |
1 participant
Summary
UI artifact downloads and migration progress streaming are broken because the web client binds download tickets to a
resource_paththe backend cannot match.The backend binds a download ticket to an exact request path and, at consume time, compares
bound_path == request.uri().path()by byte equality (backend/src/api/middleware/auth.rs). The minter is responsible for sending the exact absolute path the later request will carry. Two call sites sent the wrong value:artifactsApi.createDownloadTicket`${repoKey}/${artifactPath}``/api/v1/repositories/${repoKey}/download/${artifactPath}`migrationApi.createStreamTicket`migration/${jobId}``/api/v1/migrations/${jobId}/stream`Each was wrong twice: not absolute (the
resource_path must start with '/'rejection @flopma hit), and not the full request path, so even an absolute form would have been consumed against a mismatching path and silently burned.No backend change is needed; the backend validator and consumer middleware behave as designed.
Fixes #453
Test Checklist
Added regression tests asserting the exact
resource_pathpassed to the SDK for both call sites (src/lib/api/__tests__/artifacts.test.ts,src/lib/api/__tests__/migration.test.ts). Ran the affected suites locally: 73 passed. ESLint clean on changed files.UI Changes
This is a client-side request-payload fix in the API layer. No rendered UI changed, though it restores the download and migration-stream flows the UI depends on.