fix: LSDV-5337: Pre-signed file proxy url clashing with already html encoded values causing errors in signature #4447
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…encoded values causing errors in signature
❌ Deploy Preview for label-studio-docs-new-theme failed.
|
❌ Deploy Preview for heartex-docs failed.
|
…adding test to cover fallback scenario
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #4447 +/- ##
===========================================
- Coverage 75.57% 75.57% -0.01%
===========================================
Files 156 156
Lines 12213 12219 +6
===========================================
+ Hits 9230 9234 +4
- Misses 2983 2985 +2 ☔ View full report in Codecov by Sentry. |
triklozoid
approved these changes
Jun 28, 2023
wesleylima
approved these changes
Jun 28, 2023
AndrejOros
approved these changes
Jun 29, 2023
wesleylima
pushed a commit
that referenced
this pull request
Aug 11, 2023
…encoded values causing errors in signature (#4447) * fix: LSDV-5337: Pre-signed file proxy url clashing with already html encoded values causing errors in signature * Update base_models.py * updating tests to account for change in assertions of url structure, adding test to cover fallback scenario * Adding a test to outline resolution and support of the upper limits of file uris in cloud storage * removing any vendor specifics, just keep the characters * don't need this in the stubs as it was triggering false positives on security
shayantabatabaee
pushed a commit
to shayantabatabaee/label-studio
that referenced
this pull request
Sep 19, 2023
…encoded values causing errors in signature (HumanSignal#4447) * fix: LSDV-5337: Pre-signed file proxy url clashing with already html encoded values causing errors in signature * Update base_models.py * updating tests to account for change in assertions of url structure, adding test to cover fallback scenario * Adding a test to outline resolution and support of the upper limits of file uris in cloud storage * removing any vendor specifics, just keep the characters * don't need this in the stubs as it was triggering false positives on security
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR fulfills these requirements
[fix|feat|ci|chore|doc]: TICKET-ID: Short description of change madeex.fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errorsChange has impacts in these area(s)
(check all that apply)
Describe the reason for change
In some cases the values inside of the pre-signed storage uris could become double encoded for a portion of the string (either sourced this way, or parsed). This would then invalidate the signature when calculating the final presigned url, or possibly the resultant URL entirely.
What does this fix?
Switch from using a url encode/decode on the fileuri query param, and instead use a urlsafe base64 encode/decode. This way we can support all the various values which could be supported in the external storage(s) URI and not limit or break user functionality when using the presigned url options. There is also provisions for a fallback if the base64 fails, it will operate with unquote on the string as was before, which will serve to automatically migrate usage while it is inflight and local user caches invalidate.
What libraries were added/updated?
N/A.
Does this change affect performance?
No.
Does this change affect security?
No.
What feature flags were used to cover this change?
None.
Does this PR introduce a breaking change?
(check only one)
What level of testing was included in the change?
(check all that apply)
Which logical domain(s) does this change affect?
ImportStorage, PresignedStorageData