fix: Prevent double-encoded presigned urls from s3 #336
Conversation
|
🚀 Thanks for opening this pull request! |
|
Warning Rate limit exceeded@AdrianCurtin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated getFileLocation to use an unencoded file key (bucketPrefix + original filename) when generating presigned URLs to avoid double encoding. The presigned GetObjectCommand now uses this new key. Non-presigned URL logic and other flows remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getFileLocation
participant S3Presigner as S3 Presigner
participant S3 as S3
Caller->>getFileLocation: request file location
alt presigned URLs enabled
getFileLocation->>getFileLocation: build presignedFileKey (prefix + raw filename)
getFileLocation->>S3Presigner: sign GetObjectCommand(presignedFileKey)
S3Presigner->>S3: GetObjectCommand(presignedFileKey)
S3-->>S3Presigner: signed request
S3Presigner-->>getFileLocation: presigned URL
getFileLocation-->>Caller: presigned URL
else presigned URLs disabled
getFileLocation-->>Caller: non-presigned URL
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
AdrianCurtin
changed the title
AdrianCurtin
changed the title
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
index.js (4)
231-233: Nit: Clarify naming to reflect semantics (raw/unencoded key)Consider renaming
presignedFileKeyto something that conveys the key is unencoded, e.g.rawFileKeyorunencodedFileKey, for future maintainability.Apply this localized rename:
- const presignedFileKey = `${this._bucketPrefix}${filename}`; + const rawFileKey = `${this._bucketPrefix}${filename}`;Note: See companion change on Line 236 to update the reference.
231-236: Nit: Limit scope by computing the raw key only when presigning is enabledYou can avoid an always-on allocation by moving the raw/unencoded key computation inside the
if (this._presignedUrl)block, since it’s only used there.
236-236: Follow-up: Align variable rename in presign paramsIf you adopt the naming nit, update the params reference accordingly.
- const params = { Bucket: this._bucket, Key: presignedFileKey }; + const params = { Bucket: this._bucket, Key: rawFileKey };
231-239: Verify custom baseUrl flows don’t change the canonical path used for signingWhen
_presignedUrlis true and a custom_baseUrlis provided,buildDirectAccessUrlreconstructs the URL by combiningbaseUrl+baseUrlFileKeyand appending the query from the presigned URL. This remains valid only if the path portion exactly matches the canonical URI used during signing (after encoding). Given your switch to using the unencoded key for signing, this should still match becausefileName/fileKeyare encoded in the same way. However, configs with_baseUrlDirectand any path rewriting (e.g., CDN origin path or removed bucket prefix) can break signatures.Please validate these cases:
- Filenames containing spaces, plus signs, percent signs, and unicode (e.g., "a b+%c—µ.png").
- Nested paths in filenames (e.g., "folder/sub/na me.png").
- Configurations with
_baseUrlset, both_baseUrlDirect = trueandfalse.- Non-empty
_bucketPrefix, especially when_baseUrlDirect = true.If any of these alter the path compared to the one used for signing, the signature will be invalid.
I can draft a small set of unit tests (or a minimal Node script) to assert that the constructed URL path exactly equals the presigned URL path for the above cases. Want me to provide that?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
index.js(1 hunks)
🔇 Additional comments (2)
index.js (2)
231-233: Correct fix to avoid double-encoding during presignUsing the original, unencoded filename for the presigned S3 GetObjectCommand key is the right approach. The AWS presigner handles URL encoding; passing an already-encoded key led to double-encoding. This aligns with the PR objective and should resolve #335.
236-236: Update presign params to use the raw/unencoded keySwitching the presign GetObjectCommand to the unencoded key is correct and matches S3 expectations.
AdrianCurtin
force-pushed
the
fix-presign-double-encoding
branch
from
8483cde to
ac4037e
Compare
… s3 urls for presigning (s3 will encode the filename when it performs GetObjectCommand, but filename is already encoded and will be double encoded!)
AdrianCurtin
force-pushed
the
fix-presign-double-encoding
branch
from
ac4037e to
2c922b8
Compare
AdrianCurtin
changed the title
AdrianCurtin
changed the title
se the unencoded file name instead of the encoded one to submit s3 urls for presigning
(s3 will encode the filename when it performs GetObjectCommand, but filename is already encoded and will be double encoded!)
This fix doesn't rely on any other PRs to work
Addresses #335
Summary by CodeRabbit