fix(uploads): authorize internal file URLs before download#5049
Conversation
downloadFileFromUrl treated any URL containing /api/files/serve/ as trusted-internal and read the object straight from storage by key with no access check, while every other resolution path in the file calls verifyFileAccess. Reachable during workflow execution via file[] inputs (type: 'url'), letting an authenticated user read arbitrary storage objects across tenants by supplying a storage key. Thread the caller's userId into downloadFileFromUrl and run verifyFileAccess(key, userId, undefined, context, false) on the resolved key before downloadFile; fail closed when no userId is present. Update all callers (execution file inputs, tool file outputs, KB ingestion); webhook and chat inputs already thread userId via processExecutionFiles.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview
Callers (execution file inputs, tool file outputs, KB document/OCR/parser paths) pass Reviewed by Cursor Bugbot for commit bb1d384. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR closes a cross-tenant file access vulnerability where
Confidence Score: 4/5The authorization gap is correctly closed and all affected call sites are updated; the remaining notes are about defensive hardening, not functional breakage. The fix is well-scoped and correct across all four files. The two P2 notes in file-utils.server.ts are about defensive hardening — the isInternalFileUrl substring match and userId-check ordering — neither of which blocks the primary security goal of the PR. apps/sim/lib/uploads/utils/file-utils.server.ts — the isInternalFileUrl substring match and the userId-check ordering are both worth a second look. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Caller (execution / KB / tool)
participant DL as downloadFileFromUrl
participant Auth as verifyFileAccess
participant Store as Storage Service
participant DNS as DNS/SSRF Guard + secureFetch
Caller->>DL: "downloadFileFromUrl(url, { userId })"
alt isInternalFileUrl(url)
DL->>DL: parseInternalFileUrl - key, context
alt userId is undefined
DL-->>Caller: throw Access denied
end
DL->>Auth: verifyFileAccess(key, userId, context)
alt access denied
Auth-->>DL: false
DL-->>Caller: throw insufficient permissions
end
Auth-->>DL: true
DL->>Store: downloadFile(key, context)
Store-->>DL: Buffer
else external URL
DL->>DNS: validateUrlWithDNS(url)
DNS-->>DL: isValid + resolvedIP
DL->>DNS: secureFetchWithPinnedIP(url, resolvedIP)
DNS-->>DL: Response
DL->>DL: readResponseToBufferWithLimit
end
DL-->>Caller: Buffer
Reviews (1): Last reviewed commit: "fix(uploads): authorize internal file UR..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
bugbot run Added a |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 41ae711. Configure here.
Cursor Bugbot flagged a context-spoofing bypass: downloadFileFromUrl resolved context via parseInternalFileUrl, which honors a caller-controlled ?context= query param. An attacker could label a private storage key with a world-readable context (profile-pictures/og-images/workspace-logos) so verifyFileAccess short-circuits to granted while downloadFile still reads the private object. Infer context from the key only (inferContextFromKey), mirroring how /api/files/serve resolves it; ignore the query param. Also move the userId guard ahead of key resolution so auth failures surface first.
|
bugbot run Latest commits address the context-spoofing finding (context now derived from key only) and move rationale into TSDoc. Please re-review HEAD. |
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 a4bba94. Configure here.
isInternalFileUrl matched the /api/files/serve/ substring anywhere in the string, so a crafted URL could carry it in a query string or fragment and skip DNS/SSRF validation. Match it in the path component only. The raw path is checked without URL normalization on purpose: the files parse route relies on traversal sequences surviving this check (an absolute https://host/api/files/serve/../.. URL must classify as internal so the '..' check rejects it, rather than being normalized to /etc/... and waved through as external). Host is intentionally not gated — cross-tenant reads are prevented at the storage sink by verifyFileAccess, and host-allowlisting would break self-hosted/multi-domain deployments. Adds unit tests.
|
bugbot run Folded in the isInternalFileUrl hardening (Greptile P2): the marker is now matched in the URL path only, closing the query-string/fragment bypass. Kept it non-normalizing and host-agnostic on purpose so path-traversal protection in the parse route still applies and self-hosted/multi-domain deploys aren't broken; cross-tenant reads remain gated by verifyFileAccess at the sink. Added unit tests. |
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 9f47c14. Configure here.
|
bugbot run |
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 bb1d384. Configure here.
2 participants
Summary
downloadFileFromUrltreated any URL containing/api/files/serve/as trusted-internal and read the object straight from storage by key, skipping theverifyFileAccesscheck every other resolution path in the file runs.file[]inputs (type: 'url'), so an authenticated user could read storage objects across tenants by supplying a storage key.userIdintodownloadFileFromUrland runverifyFileAccess(key, userId, undefined, context, false)on the resolved key before reading bytes; fail closed when nouserIdis present.userIdviaprocessExecutionFiles./api/files/serveroute already enforces.Type of Change
Testing
Tested manually. Typecheck, Biome,
check:api-validation, and existing upload/KB tests pass.Checklist