fix(apiform): strip path components from upload filenames - #120
Conversation
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
ff7e2ed to
8095fae
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8095fae62c
ℹ️ 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".
| } | ||
|
|
||
| func multipartBaseName(name string) string { | ||
| return path.Base(strings.ReplaceAll(name, `\`, "/")) |
There was a problem hiding this comment.
When Name() comes from a real POSIX file whose basename contains a backslash, this unconditional replacement treats a legal filename character as a directory separator. For example, /tmp/invoice\2026.pdf previously produced filename="invoice\\2026.pdf" but now produces filename="2026.pdf", changing the filename observed by the API; only interpret backslashes as separators for recognizably Windows paths or according to the reader's native path semantics.
AGENTS.md reference: AGENTS.md:L38-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 47daacf. Windows-style paths are now normalized only when they are recognizably Windows paths; legal backslashes in POSIX basenames are preserved. Added the /tmp/invoice\\2026.pdf regression. Focused apiform test passes.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47daacf87e
ℹ️ 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".
| if len(name) < 2 || name[1] != ':' { | ||
| return false |
There was a problem hiding this comment.
On Windows, a reader such as an *os.File opened with a relative path like reports\report.pdf retains that relative name, but this predicate returns false because it has neither a drive prefix nor a UNC prefix. path.Base then treats the backslash as an ordinary character, so the multipart header still sends filename="reports\\report.pdf"; single-backslash rooted paths have the same problem. Account for native Windows path semantics so these common paths are reduced to report.pdf.
AGENTS.md reference: AGENTS.md:L38-L43
Useful? React with 👍 / 👎.
saioai
left a comment
saioai
left a comment
There was a problem hiding this comment.
i checked 47daacf. the absolute Windows case works and the POSIX backslash case is preserved, but reports\report.pdf and \reports\report.pdf still appear with path components in the raw multipart filename. i reproduced both in an isolated test. please handle native Windows relative and rooted paths, add regression coverage for them, and run CI on an updated branch. i can't approve this yet.
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
|
Fixed the remaining native Windows path cases in 610b070. Relative and rooted backslash paths now use Windows separator semantics on Windows, while POSIX filenames containing a literal backslash remain unchanged. Added regressions for both Windows cases plus the POSIX distinction. Validation: go test ./internal/apiform, go test ./internal/..., go test ./... -run ^$, go mod verify, and ./scripts/lint all pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 610b0706b7
ℹ️ 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".
| if goos == "windows" || isWindowsPath(name) { | ||
| name = strings.ReplaceAll(name, `\`, "/") | ||
| } | ||
| return path.Base(name) |
There was a problem hiding this comment.
When Name() returns a valid drive-relative Windows path such as C:report.pdf, replacing backslashes changes nothing and path.Base returns C:report.pdf, whereas Windows basename semantics remove the volume and return report.pdf. This can occur for a native Windows file opened relative to a drive's current directory, so the multipart filename still exposes a path prefix; strip the recognized drive volume before applying the portable basename logic and add this case to the Windows table.
AGENTS.md reference: AGENTS.md:L38-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 15b731d. Recognized drive-letter paths now strip the volume prefix before portable basename handling, so C:report.pdf emits report.pdf; the Windows helper table now covers this case. The focused and repository offline checks pass.
| want string | ||
| }{ | ||
| {name: "posix", readerName: "/home/alice/reports/report.pdf", want: "report.pdf"}, | ||
| {name: "posix basename with backslash", readerName: `/tmp/invoice\2026.pdf`, want: `invoice\2026.pdf`}, |
There was a problem hiding this comment.
On a Windows test run, Marshal uses runtime.GOOS == "windows", so this reader name is normalized to /tmp/invoice/2026.pdf and correctly emits 2026.pdf; the unconditional invoice\2026.pdf expectation therefore makes this package test fail on every Windows environment. Exercise the POSIX-literal behavior only through multipartBaseNameForOS(..., "linux"), or otherwise condition this integration case on the host OS.
AGENTS.md reference: AGENTS.md:L62-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 15b731d. The host-dependent POSIX backslash case is no longer exercised through Marshal; the explicit multipartBaseNameForOS(..., "linux") regression retains that behavior without failing Windows runs. All offline checks pass.
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
Castiron custom code✅ No new custom-code files detected. 5 mixed files remain; 0 existing customizations changed. Compared 5 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 35252665772 --repo openai/openai-cli \
--name castiron-custom-code-35252665772-1 --dir /tmp/castiron-custom-code-35252665772-1
git apply --stat /tmp/castiron-custom-code-35252665772-1/custom-code.patch
cat /tmp/castiron-custom-code-35252665772-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin f984eb1df5620cdd5dc2b8a03d892ed38cc9d015 50dfede0a1ebdb5b99cdb049bd809b117561a884
python3 scripts/castiron/custom_code_report.py report \
--base f984eb1df5620cdd5dc2b8a03d892ed38cc9d015 \
--head 50dfede0a1ebdb5b99cdb049bd809b117561a884 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-50dfede0a1eb
cat /tmp/castiron-custom-code-50dfede0a1eb/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
saioai
left a comment
saioai
left a comment
There was a problem hiding this comment.
approving. the filename fallback now strips windows path components, including relative and drive-relative paths. explicit filenames stay unchanged, and the regression tests pass.
|
Thanks @saioai for the re-review and merge. Appreciate you checking the remaining native Windows path cases. |
Automated Release PR --- ## [1.16.0](openai/openai-cli@v1.15.0...v1.16.0) (2026-09-22) ### Features * **api:** add external storage and safety case commands ([openai#229](openai#229)) ([c30c961](openai@c30c961)) * **api:** add prompt-cache prewarming ([openai#196](openai#196)) ([9032f58](openai@9032f58)) * **api:** add response transformation hooks ([openai#223](openai#223)) ([8de34a7](openai@8de34a7)) * **api:** add webhook endpoint management ([openai#201](openai#201)) ([dea465a](openai@dea465a)) * **cli:** support custom request headers ([openai#190](openai#190)) ([07e3ed9](openai@07e3ed9)) ### Bug Fixes * **apiform:** preserve float32 precision in comma arrays ([openai#78](openai#78)) ([774612f](openai@774612f)) * **apiform:** strip path components from upload filenames ([openai#120](openai#120)) ([a92e143](openai@a92e143)) * **apiform:** support primitive pointers in comma arrays ([openai#119](openai#119)) ([9d146cd](openai@9d146cd)) * **apiquery:** preserve narrow numeric parameters ([openai#76](openai#76)) ([b152780](openai@b152780)) * **apiquery:** reject complex elements in comma arrays ([openai#118](openai#118)) ([46faf2c](openai@46faf2c)) * **apiquery:** reject non-string map keys ([openai#88](openai#88)) ([687a097](openai@687a097)) * **autocomplete:** omit hidden flags from suggestions ([openai#122](openai#122)) ([137d59b](openai@137d59b)) * **cmd:** include backslash in path detection for @ file references ([openai#38](openai#38)) ([f984eb1](openai@f984eb1)) * **debug:** redact sensitive response headers ([openai#32](openai#32)) ([ee62f85](openai@ee62f85)) * **explore:** avoid panic when printing an empty result set ([openai#68](openai#68)) ([f74838c](openai@f74838c)) * install Linux package binaries under /usr/bin ([openai#34](openai#34)) ([ac4e7cb](openai@ac4e7cb)) * **jsonview:** avoid width underflow in static string rendering ([openai#117](openai#117)) ([cbdf265](openai@cbdf265)) * **jsonview:** preserve literal object keys in pretty output and explorer ([openai#143](openai#143)) ([25664bf](openai@25664bf)) * omit redirect destinations from multipart upload errors ([openai#189](openai#189)) ([642d511](openai@642d511)) * **output:** stop pagination at max items ([openai#43](openai#43)) ([552840e](openai@552840e)) * **requestflag:** preserve JSON numbers written in exponent form ([openai#195](openai#195)) ([20db45e](openai@20db45e)) ### Chores * **api:** clarify Live SIP call help ([openai#193](openai#193)) ([79435c4](openai@79435c4)) * **api:** document MCP connector deprecation ([openai#202](openai#202)) ([1d4e76c](openai@1d4e76c)) * **api:** update image request examples ([openai#197](openai#197)) ([4b75e9b](openai@4b75e9b)) * **deps:** bump the codeql group across 1 directory with 2 updates ([openai#161](openai#161)) ([97734f3](openai@97734f3)) * **deps:** bump the codeql group across 1 directory with 2 updates ([openai#224](openai#224)) ([0713595](openai@0713595)) * **deps:** bump the go-minor-and-patch group across 1 directory with 2 updates ([openai#144](openai#144)) ([aa1158d](openai@aa1158d)) * **deps:** update openai-go to v3.61.0 ([openai#184](openai#184)) ([de52b2e](openai@de52b2e)) * **deps:** update openai-go to v3.63.0 ([openai#198](openai#198)) ([3f2c883](openai@3f2c883)) * **deps:** update openai-go to v3.63.1 ([openai#204](openai#204)) ([eb415e5](openai@eb415e5)) * **deps:** update openai-go to v3.64.0 ([openai#206](openai#206)) ([0169bff](openai@0169bff)) * **deps:** update openai-go to v3.64.2 ([openai#225](openai#225)) ([a7859e5](openai@a7859e5)) ### Documentation * correct bootstrap dependency check description ([openai#181](openai#181)) ([4aa657c](openai@4aa657c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: openai-sdks[bot] <284451331+openai-sdks[bot]@users.noreply.github.com> Co-authored-by: saioai <vguvvala@openai.com>
3 participants
Summary
Ensure multipart uploads send only the basename of reader-provided file paths, regardless of whether the path uses POSIX or Windows separators.
Problem
internal/apiform.encodeReaderderives filenames from readers exposingName()withpath.Base, which only treats/as a separator. A Windows-style name such asC:\dir\report.pdfcan therefore remain unchanged when processed on a non-Windows host or supplied by a cross-platform/custom reader, placing directory components in the multipartfilename=parameter instead of justreport.pdf.Fix
Normalize backslash separators before taking the path basename. POSIX paths keep their existing behavior, while Windows-style paths reduce to the same basename representation.
The change is limited to the
Name()fallback. Readers that explicitly implementFilename()retain control of the filename they provide.Regression coverage
Added table-driven multipart tests covering POSIX and Windows-style paths. Both must produce
filename="report.pdf", and the serialized body must not contain directory components.Validation
The branch is based directly on current upstream
main(a7719136b8ed401b0c51a05553a5e4f720150307) and contains one DCO-signed commit touching only the multipart encoder plus focused regression coverage. Full repository test execution is left to CI.Risk
Low. Only the fallback filename derived from a reader's
Name()changes, and only by removing directory components. File contents, content type handling, explicitFilename()implementations, field names, and non-file multipart fields are unchanged.