Repository navigation
fix(security): prevent streaming upload filename path disclosure - #2386
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8fd78be7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Overview
This aligns branded streaming uploads with buffered uploads by stripping directory components before multipart-header construction. The default path now covers mixed/nested bodies and preserves lazy streaming, but the Skills path-preserving mode still admits unsafe path shapes and the exported filename documentation no longer matches wire behavior.
Confirmed findings
-
[Medium] Skills uploads preserve absolute and traversal-bearing paths
- Location: src/internal/uploads.ts:462-464
- Both public Skills methods hardcode
stripFilenames: falsefor directory upload.normalizeFilenamePathonly converts backslashes, so absolute POSIX paths, drive/UNC/device paths, and..segments are still transmitted. This retains local-path disclosure and gives a relative directory-upload field unsafe names. - Centralize a preserved-relative-path validator for buffered and streaming paths: keep safe nested relative names, but reject leading
/, drive/UNC/device forms, and parent segments. Add public Skills regressions for both upload modes.
-
[Low] Exported
toStreamingFilefilename documentation is now inaccurate- Location: src/internal/uploads.ts:39-54
- The comments say
nameis the filename sent in multipart, but default endpoints now send only its basename while Skills preserves a normalized relative path. - Document
nameas the logical/source name from which the wire filename is derived, including default directory stripping and the Skills exception.
All exact-head checks passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c29bd16875
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cd609684e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Independent two-pass re-review confirms the prior path-preservation and documentation issues are fixed. Buffered and streaming Skills uploads reject absolute, Windows drive and UNC, and traversal paths while preserving safe relative structure and lazy streaming. Validated inputs are snapshotted, threads are resolved, and substantive CI is green. No actionable findings.
Summary
toStreamingFile()uploads by default, matching existing bufferedFilebehavior and preventing private local paths from appearing in multipart filenames.Validation
client.files.create,client.images.edit,client.audio.transcriptions.create,client.skills.create, andclient.skills.versions.create../scripts/lintpnpm exec tsc --project tsconfig.json --noEmit./scripts/builddist/src.node --experimental-strip-types scripts/test-packed-package.tsnode --experimental-strip-types scripts/check-node-version-policy.tsOnly two handwritten upload tests and their owning handwritten upload implementation were changed; no generated endpoints or package metadata changed.