feat(s3): add presigned url operation using AWS Signature V4 impl #20727
Conversation
|
|
n8n-assistant
bot
added
community
|
Hey @ahmed-musallam, Thank you for your contribution. We appreciate the time and effort you’ve taken to submit this pull request. Before we can proceed, please ensure the following: Regarding new nodes: If your node integrates with an AI service that you own or represent, please email [email protected] and we will be happy to discuss the best approach. About review timelines: Thank you again for contributing to n8n. |
ahmed-musallam
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| // Use virtual-hosted-style | ||
| host = `${bucketName}.s3.${regionName}.amazonaws.com`; | ||
| canonicalUri = `/${fileKey}`; | ||
| } |
There was a problem hiding this comment.
Bug: Presigned URL Generation Fails with Special Characters
The generatePresignedUrl function constructs the canonical URI without URI-encoding the bucketName and fileKey path segments. AWS Signature V4 requires path segments to be encoded, which causes signature mismatches and invalid presigned URLs when these names contain special characters.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 3 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="packages/nodes-base/nodes/Aws/S3/V2/GenericFunctions.ts">
<violation number="1" location="packages/nodes-base/nodes/Aws/S3/V2/GenericFunctions.ts:165">
Rule violated: **Prefer Typeguards over Type casting**
`const regionName` narrows with `as string`, violating the "Prefer Typeguards over Type casting" rule. Please replace the assertion with an explicit type check or annotation that avoids `as` so incorrect values aren’t silently coerced.
(Based on your team's feedback about verifying downstream resolutions before flagging casts.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/nodes-base/nodes/Aws/S3/V2/GenericFunctions.ts:174">
Percent-encode the key segment in the path-style canonical URI to avoid signature mismatches for keys containing reserved characters.</violation>
<violation number="3" location="packages/nodes-base/nodes/Aws/S3/V2/GenericFunctions.ts:178">
Canonical URI must be percent-encoded before signing; otherwise keys with spaces or reserved characters produce invalid presigned URLs.</violation>
<violation number="4" location="packages/nodes-base/nodes/Aws/S3/V2/GenericFunctions.ts:206">
Rule violated: **Prefer Typeguards over Type casting**
Assigning `value` with `as string` breaks the "Prefer Typeguards over Type casting" rule and can mask non-string query parameters. Please guard or convert the value instead of asserting.
(Based on your team's feedback about verifying downstream resolutions before flagging casts.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/nodes-base/nodes/Aws/S3/V2/AwsS3V2.node.ts">
<violation number="1" location="packages/nodes-base/nodes/Aws/S3/V2/AwsS3V2.node.ts:702">
Rule violated: **Prefer Typeguards over Type casting**
The new presigned-URL branch calls generatePresignedUrl with `region as string`, which violates the guideline against using `as` for narrowing because the preceding code never verifies LocationConstraint._ is actually a string. Please add a proper type guard (e.g., ensure the field exists and is a string) before invoking the helper instead of casting.
(Based on your team's feedback about verifying the surrounding lines before flagging an `as` cast; I confirmed there’s no guard later in this block.) [FEEDBACK_USED]</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| // Add additional query parameters | ||
| Object.entries(query).forEach(([key, value]) => { | ||
| queryParams[key] = value as string; |
There was a problem hiding this comment.
Prompt for AI agents
~~~ Address the following comment on packages/nodes-base/nodes/Aws/S3/V2/GenericFunctions.ts at line 206: Assigning `value` with `as string` breaks the "Prefer Typeguards over Type casting" rule and can mask non-string query parameters. Please guard or convert the value instead of asserting. (Based on your team's feedback about verifying downstream resolutions before flagging casts.) @@ -131,3 +132,104 @@ export async function awsApiRequestRESTAllItems( + + // Add additional query parameters + Object.entries(query).forEach(([key, value]) => { + queryParams[key] = value as string; + }); + ~~~
|
|
||
| const accessKeyId = `${credentials.accessKeyId}`.trim(); | ||
| const secretAccessKey = `${credentials.secretAccessKey}`.trim(); | ||
| const regionName = (region || credentials.region || 'us-east-1') as string; |
There was a problem hiding this comment.
Prompt for AI agents
~~~ Address the following comment on packages/nodes-base/nodes/Aws/S3/V2/GenericFunctions.ts at line 165: `const regionName` narrows with `as string`, violating the "Prefer Typeguards over Type casting" rule. Please replace the assertion with an explicit type check or annotation that avoids `as` so incorrect values aren’t silently coerced. (Based on your team's feedback about verifying downstream resolutions before flagging casts.) @@ -131,3 +132,104 @@ export async function awsApiRequestRESTAllItems( + + const accessKeyId = `${credentials.accessKeyId}`.trim(); + const secretAccessKey = `${credentials.secretAccessKey}`.trim(); + const regionName = (region || credentials.region || 'us-east-1') as string; + + // Determine the endpoint ~~~
| fileKey, | ||
| expirationTime, | ||
| presignedQuery, | ||
| region as string, |
There was a problem hiding this comment.
Prompt for AI agents
~~~ Address the following comment on packages/nodes-base/nodes/Aws/S3/V2/AwsS3V2.node.ts at line 702: The new presigned-URL branch calls generatePresignedUrl with `region as string`, which violates the guideline against using `as` for narrowing because the preceding code never verifies LocationConstraint._ is actually a string. Please add a proper type guard (e.g., ensure the field exists and is a string) before invoking the helper instead of casting. (Based on your team's feedback about verifying the surrounding lines before flagging an `as` cast; I confirmed there’s no guard later in this block.) @@ -659,6 +663,59 @@ export class AwsS3V2 implements INodeType { + fileKey, + expirationTime, + presignedQuery, + region as string, + ); + ~~~
| if (bucketName.includes('.')) { | ||
| // Bucket name contains dots, use path-style | ||
| host = `s3.${regionName}.amazonaws.com`; | ||
| canonicalUri = `/${bucketName}/${fileKey}`; |
There was a problem hiding this comment.
Percent-encode the key segment in the path-style canonical URI to avoid signature mismatches for keys containing reserved characters.
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Aws/S3/V2/GenericFunctions.ts at line 174:
<comment>Percent-encode the key segment in the path-style canonical URI to avoid signature mismatches for keys containing reserved characters.</comment>
<file context>
@@ -131,3 +132,104 @@ export async function awsApiRequestRESTAllItems(
+ if (bucketName.includes('.')) {
+ // Bucket name contains dots, use path-style
+ host = `s3.${regionName}.amazonaws.com`;
+ canonicalUri = `/${bucketName}/${fileKey}`;
+ } else {
+ // Use virtual-hosted-style
</file context>
| canonicalUri = `/${bucketName}/${fileKey}`; | |
| canonicalUri = `/${bucketName}/${encodeURIComponent(fileKey).replace(/%2F/g, '/')}`; |
| } else { | ||
| // Use virtual-hosted-style | ||
| host = `${bucketName}.s3.${regionName}.amazonaws.com`; | ||
| canonicalUri = `/${fileKey}`; |
There was a problem hiding this comment.
Canonical URI must be percent-encoded before signing; otherwise keys with spaces or reserved characters produce invalid presigned URLs.
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Aws/S3/V2/GenericFunctions.ts at line 178:
<comment>Canonical URI must be percent-encoded before signing; otherwise keys with spaces or reserved characters produce invalid presigned URLs.</comment>
<file context>
@@ -131,3 +132,104 @@ export async function awsApiRequestRESTAllItems(
+ } else {
+ // Use virtual-hosted-style
+ host = `${bucketName}.s3.${regionName}.amazonaws.com`;
+ canonicalUri = `/${fileKey}`;
+ }
+
</file context>
| canonicalUri = `/${fileKey}`; | |
| canonicalUri = `/${encodeURIComponent(fileKey).replace(/%2F/g, '/')}`; |
2 participants
Summary
This PR adds a new
Get presigned URL for a fileoperation to the AWS S3 Base Node.It was very useful in my workflows for APIs that depend on presigned URLs and did not allow me to upload files directly.
Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)