Fix URL signing api for URLs containing special characters - #12435
Fix URL signing api for URLs containing special characters#12435ErykKul wants to merge 37 commits into
Conversation
ErykKul
added
Size: 3
|
coverage: 25.402% (+0.2%) from 25.242% — fix-url-signing-special-characters into develop |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
pdurbin
added
the
Status: Needs Input
|
@ErykKul just a reminder that we'd like your input and code freeze for 6.12 is next Thursday! 🥶 |
|
We think only @ErykKul is actively using this but we'd like to get it into 6.12. |
|
@pdurbin I am on it. Trying to wrap it up. Not ready yet, I will try to have it ready by tomorrow evening. |
…eserved params instead of stripping, constant-time signature check #12435
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…, forging without the server secret fails #12435
This comment has been minimized.
This comment has been minimized.
…chronized, no per-node cache), 400s instead of 500s for smuggled or manifest reserved params, constructor injection, coverage tests #12435
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…h, including bulk PUT #12435
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
@qqmyers Ready for another look. The signing/validation fix itself is unchanged since your last pass; the rework around it, per our discussion: Signing secret is now a DB setting (:ApiSigningSecret), auto-generated on first use. I dropped the JVM option entirely: there's no good way to generate a secret and keep it synced across the deployment shapes people actually run (docker compose, Kubernetes, hosted Payara). Admins can still store their own; a minimum length of 32 is enforced on every write path (single PUT, localized, bulk). DELETE rotates it. Upgrading invalidates outstanding signed URLs (release note covers it). With the secret always present, all the "no secret configured" fallback paths are gone. Reserved query parameters are now rejected, never silently stripped — requestSignedUrl and tool-manifest allowedApiCalls templates return a 400 naming the parameter (note: a pre-existing manifest that misused one worked silently in 6.10 and will 400 at launch after upgrade). One behavior change to ratify: requestSignedUrl with an explicit but unknown user now returns 400 instead of silently signing with the calling superuser's token (a typo used to mint a full-privilege URL). Omitting user still defaults to the caller, as already documented: I wondered whether that default should be more explicit (or required), but that felt out of scope for this PR. Also while in there: constant-time signature comparison, and the secret is redacted from the action log. |
…in the settings test #12435
…ecial-characters # Conflicts: # src/main/java/edu/harvard/iq/dataverse/api/Access.java
|
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
7 participants
What this PR does / why we need it:
Bug fix; URL signing for URLs containing special characters broken in 6.10, this PR fixes the introduced bug.
Which issue(s) this PR closes:
Issue not created, bug fixed directly here.
Special notes for your reviewer:
Added tests to prevent regression. Also, API requires now the signing secret to be set, otherwise it will not work.
Suggestions on how to test this:
No special testing needed, the unit tests should be sufficient.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No
Is there a release notes update needed for this change?:
Yes, included.
Additional documentation:
No