fix(apiquery): reject non-string map keys - #88
Conversation
|
@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: c628bb98a7
ℹ️ 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".
| mapKey := iter.Key() | ||
| if mapKey.Kind() != reflect.String { | ||
| return nil, fmt.Errorf("apiquery: cannot encode a map with a non-string key") |
There was a problem hiding this comment.
Because this check only runs for entries returned by MapRange, Marshal(map[int]string{}) and Marshal(map[int]string(nil)) still succeed with empty query values even though their map key type is unsupported. This makes rejection depend on whether the map happens to contain data; inspect value.Type().Key().Kind() before starting the iteration so empty and populated non-string-keyed maps follow the same contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1b44518. Map key type is now validated before iteration, so populated, empty, and nil non-string-key maps are rejected consistently. Focused apiquery tests pass.
There was a problem hiding this comment.
Fixed in 1b44518. The encoder now validates the map key type before iterating, so empty and nil non-string-key maps fail consistently with populated maps. Added regressions for both empty and nil cases. go test ./internal/apiquery 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. |
saioai
left a comment
saioai
left a comment
There was a problem hiding this comment.
URL query parameters need text keys. Previously, a map with integer keys could produce a meaningless key; the PR returns a clear error instead. It also rejects empty and nil integer-key maps consistently. I ran the PR’s regression tests on main: all three integer-key cases failed as expected. with pr, also the package race tests, internal tests, repository test compilation, build, vet, module verification, diff check passed
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 34912128026 --repo openai/openai-cli \
--name castiron-custom-code-34912128026-1 --dir /tmp/castiron-custom-code-34912128026-1
git apply --stat /tmp/castiron-custom-code-34912128026-1/custom-code.patch
cat /tmp/castiron-custom-code-34912128026-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin de52b2eddf77d9b755c7ae7c3d8888d5e818d272 5c1dd7cd6f32479f53e94911a3c3067ac2f81c3b
python3 scripts/castiron/custom_code_report.py report \
--base de52b2eddf77d9b755c7ae7c3d8888d5e818d272 \
--head 5c1dd7cd6f32479f53e94911a3c3067ac2f81c3b --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-5c1dd7cd6f32
cat /tmp/castiron-custom-code-5c1dd7cd6f32/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
|
Thanks for the review, validation, and merge. I appreciate the checks around non-string, empty, and nil map-key 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
Fail clearly when URL-query encoding receives a map whose keys are not strings, instead of serializing reflection placeholder text into parameter names.
Fixes #87.
Problem
internal/apiquery.encodeMapcurrently assumes every map key is a string and calls:without checking the key kind.
For a non-string
reflect.Value,String()is not a conversion of the underlying key. It returns reflection's diagnostic representation. A value such as:can therefore proceed into request encoding with a malformed implementation-specific query key instead of failing at the serialization boundary.
Root cause
The map encoder relies on a string-key invariant that it never validates.
The sibling multipart/form encoder already rejects non-string map keys explicitly, so the two request encoders currently disagree on the same unsupported input shape.
Fix
Inspect each map key before using it:
String-key behavior and nested query formatting remain unchanged.
Regression coverage
Added focused tests proving:
map[int]string{1: "one"}returns a clear error and no query values;map[string]stringcontinues to encode normally.The invalid-map case exercises the public
Marshalpath rather than calling the internal encoder directly.Validation
The branch is based directly on upstream
mainatd082a010f7c6cacf407d8a1581446a7857f9f1bband is not behind it.Production diff: 5 additions and 1 deletion in
internal/apiquery/encoder.go, plus one focused regression file.Full repository validation is left to the repository's GitHub Actions checks.
Risk
Low. The only newly rejected inputs are map shapes that cannot preserve their key semantics in URL query parameter names. Existing string-key maps, arrays, primitives, and nesting formats are unchanged.