fix(aws): add validateAwsRegion to all AWS route schemas to prevent SSRF#4250
fix(aws): add validateAwsRegion to all AWS route schemas to prevent SSRF#4250waleedlatif1 merged 4 commits intostagingfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Updates Reviewed by Cursor Bugbot for commit 1b97c15. Configure here. |
Greptile SummaryThis PR correctly applies Confidence Score: 4/5Safe to merge for the 61 covered routes, but the SSRF vulnerability persists in 31 other AWS routes — follow-up is required before the fix can be considered complete. The regex fix and all 61 route updates are correct and well-tested. A concrete, confirmed P1 security gap remains: S3, RDS, Secrets Manager, SQS, Textract, Athena, and CloudFormation routes still have the original unvalidated region field. The PR title and description claim completeness so the omission is unexpected rather than a deliberate deferral. apps/sim/app/api/tools/s3/, apps/sim/app/api/tools/rds/, apps/sim/app/api/tools/secrets_manager/, apps/sim/app/api/tools/sqs/, apps/sim/app/api/tools/textract/, apps/sim/app/api/tools/athena/, apps/sim/app/api/tools/cloudformation/*
|
| Filename | Overview |
|---|---|
| apps/sim/lib/core/security/input-validation.ts | Regex updated to add eu-isoe and mx prefixes, and alternation reordered (longer prefixes first) — logic is correct and covers all 41 known AWS regions |
| apps/sim/lib/core/security/input-validation.test.ts | New test cases added for us-iso-west-1, mx-central-1, and eu-isoe-west-1 — all are correct and cover the newly added regex branches |
| apps/sim/app/api/tools/cloudwatch/describe-alarms/route.ts | Representative of all 61 routes updated in this PR — validateAwsRegion .refine() applied correctly with a static message object |
| apps/sim/app/api/tools/s3/put-object/route.ts | Still uses bare region: z.string() — region is passed to S3Client and also string-interpolated into the response URL, leaving the same SSRF vector unpatched |
| apps/sim/app/api/tools/rds/query/route.ts | Still uses bare region: z.string() passed directly to createRdsClient — same SSRF vector as the routes fixed in this PR |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming POST request] --> B[checkInternalAuth]
B -->|fail| C[401 Unauthorized]
B -->|pass| D[Zod schema parse]
D --> E{region field}
E -->|61 patched routes: validateAwsRegion refine| F[Regex check passes]
E -->|31 unpatched routes: bare z.string| G[Any string accepted]
F --> H[AWS SDK Client constructed with validated region]
G --> I[AWS SDK Client constructed with arbitrary region]
I --> J[SSRF: SDK resolves attacker-controlled endpoint]
H --> K[AWS API call succeeds safely]
Comments Outside Diff (1)
-
apps/sim/app/api/tools/s3/put-object/route.ts, line 19 (link)SSRF fix incomplete — S3, RDS, Secrets Manager, SQS, Textract, Athena, and CloudFormation routes still unprotected
The PR title states "add
validateAwsRegionto all AWS route schemas" but 31 AWS routes were omitted. All of them accept a bareregion: z.string()that is passed directly to the AWS SDK (and in S3's case, also interpolated into the response URL on line 121:`https://${bucketName}.s3.${region}.amazonaws.com/...`). Affected services and representative files:- S3 —
s3/copy-object,s3/delete-object,s3/list-objects,s3/put-object - RDS —
rds/delete,rds/execute,rds/insert,rds/introspect,rds/query,rds/update - Secrets Manager —
secrets_manager/create-secret,secrets_manager/delete-secret,secrets_manager/get-secret,secrets_manager/list-secrets,secrets_manager/update-secret - SQS —
sqs/send - Textract —
textract/parse - Athena — 7 routes
- CloudFormation — 7 routes
The same
.refine((v) => validateAwsRegion(v).isValid, { message: ... })pattern used in this PR should be applied to all of these. - S3 —
Reviews (2): Last reviewed commit: "fix(aws): eliminate double validateAwsRe..." | Re-trigger Greptile
…nation order - Replace double-call .refine() pattern with single-call + static message across all 61 AWS routes - Reorder regex alternation to put longer prefixes first (eu-isoe before eu, us-isob/us-iso/us-gov before us) for engine-agnostic correctness
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1b97c15. Configure here.
Summary
Type of Change
Testing
Checklist