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. |
2 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.