fix(ssestream): preserve decoder parameter value case - #852
Conversation
|
@codex review |
|
Additional focused validation after opening the PR: I copied the exact key-normalization helpers from this head into an isolated stdlib-only Go module, ran |
|
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. |
![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: 593ecdc12c
ℹ️ 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".
|
@codex review |
|
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: 82aeabfca7
ℹ️ 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".
|
@codex review |
|
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: dcb689026e
ℹ️ 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".
|
@codex review Current head: |
|
@codex review |
|
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: 136594ca39
ℹ️ 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".
|
@codex review |
|
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: 4cbccd1a71
ℹ️ 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".
|
Addressed the latest P2 in |
|
@codex review Current head: |
|
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: d57e93c928
ℹ️ 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".
|
@codex review Current head: |
|
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. |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cfe9c5488
ℹ️ 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 _, parsedParams, err := mime.ParseMediaType(contentType); err == nil { | ||
| externalBodyAccessType = strings.ToLower(parsedParams["access-type"]) |
There was a problem hiding this comment.
When access-type uses a valid RFC 2231 charset unsupported by Go's mime.ParseMediaType, such as access-type*=ISO-8859-1''FTP, the parser omits the decoded parameter and leaves externalBodyAccessType empty. A registration ending in mode=IMAGE therefore no longer matches an equivalent lowercase response ending in mode=image, even though FTP's mode value is case-insensitive; lookup instead falls back to the bare or default decoder. Derive the logical access type without relying on mime.ParseMediaType's US-ASCII/UTF-8-only extended-value decoder.
AGENTS.md reference: AGENTS.md:L71-L73
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 940739a852
ℹ️ 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 err != nil { | ||
| return "" | ||
| } | ||
| if accessType := standardExternalBodyAccessType(parsedParams["access-type"]); accessType != "" { |
There was a problem hiding this comment.
When an encoded continuation has a malformed section zero but a later section equals a standard access type—for example, access-type*0*=UTF-8''%ZZ; access-type*1*=FTP—Go's mime.ParseMediaType discards the failed first segment but returns FTP from the second, so this branch classifies the malformed value as FTP. The normalizer then folds mode=IMAGE and mode=image, allowing a malformed response to select or overwrite the wrong parameter-specific decoder even though extension/undetermined access types intentionally keep mode case-sensitive. Validate that every continuation segment decoded successfully before accepting the assembled access type, and cover malformed section-zero input.
AGENTS.md reference: AGENTS.md:L35-L40
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71e5e79f8d
ℹ️ 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 hasMetadata { | ||
| if firstQuote := strings.IndexByte(core, '\''); firstQuote >= 0 { | ||
| secondQuote := strings.IndexByte(core[firstQuote+1:], '\'') | ||
| if secondQuote < 0 { | ||
| return false |
There was a problem hiding this comment.
Fresh evidence after the earlier malformed-continuation fix is that hasMetadata only validates the prefix when an apostrophe happens to exist, so access-type*0*=BROKEN; access-type*1*=FTP is accepted. Go's MIME parser then discards the undecodable section zero, assembles FTP from section one, and this code folds mode=IMAGE with mode=image, allowing malformed registrations to collide or select the parameter-specific decoder. Require section zero and unsuffixed encoded values to contain a nonempty charset'language' prefix before using their assembled access type.
AGENTS.md reference: AGENTS.md:L35-L40
Useful? React with 👍 / 👎.
| switch strings.TrimSpace(mediaType) { | ||
| case "message/external-body": |
There was a problem hiding this comment.
For application/pkcs7-mime, the standardized smime-type alternatives such as signed-data and enveloped-data are case-insensitive, but this table omits them. A decoder registered as application/pkcs7-mime; smime-type=SIGNED-DATA previously matched a lowercase response because registration folded the entire key; it now retains the uppercase value and falls through to the bare or default decoder. Add media-type-specific normalization for smime-type so equivalent S/MIME responses preserve decoder routing.
AGENTS.md reference: AGENTS.md:L71-L73
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 187200c061
ℹ️ 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".
| firstQuote := strings.IndexByte(core, '\'') | ||
| if firstQuote <= 0 { | ||
| return false | ||
| } | ||
| secondQuote := strings.IndexByte(core[firstQuote+1:], '\'') | ||
| if secondQuote < 0 { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Fresh evidence beyond the existing empty/missing-prefix cases is that a quoted value such as access-type*="BAD CHAR''FTP" passes this validation because only the apostrophe positions are checked. replaceExtendedParameterCharset then erases the malformed charset, mime.ParseMediaType yields FTP, and mode=IMAGE is folded with mode=image, allowing malformed input to select or overwrite a parameter-specific decoder instead of preserving case-sensitive routing. Validate the charset metadata syntax before substituting it.
AGENTS.md reference: AGENTS.md:L37-L40
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4b362333a
ℹ️ 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".
| rewritten.WriteString("US-ASCII") | ||
| rewritten.WriteString(core[firstQuote:]) |
There was a problem hiding this comment.
Fresh evidence beyond the earlier ISO-8859-1 fix is a valid non-ASCII-compatible charset such as access-type*=UTF-16BE''%00F%00T%00P: replacing its label with US-ASCII while retaining the encoded bytes makes mime.ParseMediaType produce a NUL-interleaved value rather than FTP. standardExternalBodyAccessType therefore rejects it, so a registration with mode=IMAGE no longer matches an equivalent response with mode=image, although the previous registration path folded both keys. Decode or transcode the declared charset before classifying the access type instead of merely substituting its label.
AGENTS.md reference: AGENTS.md:L71-L73
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd8129cd5c
ℹ️ 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".
| case isExternalBodyAccessType && hasExternalBodyAccessType: | ||
| valueStart, valueEnd := trimOWSBounds(value) | ||
| normalized.WriteString(value[:valueStart]) | ||
| normalized.WriteString(externalBodyAccessType) |
There was a problem hiding this comment.
When an external-body response contains a successfully decoded access-type with delimiters, this writes those delimiters directly into the flat key. For example, access-type*=UTF-8''FTP%3Bmode%3DIMAGE and the distinct access-type*=UTF-8''FTP;mode=image both normalize to access-type*=ftp;mode=image, so attacker-controlled metadata can select or overwrite the wrong parameter-specific decoder. Preserve an escaped, unambiguous representation or substitute only validated standard access-type tokens, and add a public-entrypoint collision regression.
AGENTS.md reference: AGENTS.md:L35-L40
Useful? React with 👍 / 👎.
| switch c { | ||
| case '!', '#', '$', '%', '&', '+', '-', '^', '_', '`', '~': | ||
| continue | ||
| default: | ||
| return false |
There was a problem hiding this comment.
A valid RFC 2231 value using the registered US-ASCII alias ANSI_X3.4-1968 is rejected here solely because its charset name contains a period, before ianaindex can resolve it. Consequently, a registration such as message/external-body; access-type*=ANSI_X3.4-1968''FTP; mode=IMAGE no longer matches the equivalent lowercase response, whereas the parent implementation folded both keys. Accept valid registered aliases—or delegate validation to the charset lookup—so the decoded standard access type can still control mode normalization.
AGENTS.md reference: AGENTS.md:L71-L73
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08f6e9203a
ℹ️ 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".
| case "multipart/encrypted": | ||
| return strings.EqualFold(name, "protocol") | ||
| case "multipart/signed": | ||
| return strings.EqualFold(name, "protocol") |
There was a problem hiding this comment.
Fresh evidence after the earlier fix is that the final reviewed blob again returns true only for protocol, so the reported micalg correction is absent. RFC 1847 defines micalg values as case-insensitive; consequently, registering multipart/signed; micalg=PGP-SHA256 no longer matches a response containing micalg=pgp-sha256, although the parent implementation lowercased both keys, and routing falls back to the bare/default decoder. Include micalg in this branch and retain a registration-to-lookup regression test.
AGENTS.md reference: AGENTS.md:L71-L73
Useful? React with 👍 / 👎.
1 participant
Summary
Fix parameter-specific stream decoder registration so MIME parameter values that are case-sensitive are not lowercased when the decoder key is stored.
This is a follow-up to #790. That PR intentionally changed response lookup to lowercase only the media-type prefix while preserving the parameter tail, but
RegisterDecoderstill lowercases the entire registration string. The two sides are therefore asymmetric for registrations containing case-sensitive parameter values.Reproduction
Registering:
currently stores the decoder under:
while response lookup preserves
V1. As a result, the intended.../V1representation misses the registered decoder, while the distinct.../v1representation can select it instead.Fix
Use one decoder-key normalization path for registration and exact lookup. The normalizer follows MIME's default rule that parameter values are case-sensitive unless that media type and parameter define otherwise:
profilevalues;charsetvalues;message/external-body,text/plain,text/csv,multipart/related,multipart/signed(protocolandmicalg),multipart/encrypted, andmultipart/report;The bare-media fallback still uses
mime.ParseMediaTypeas before, so unsupported extended parameters cannot silently collapse into the bare decoder key.Generation ownership
packages/ssestream/ssestream.gois generated-owned scaffolding, but this public repository does not contain the Castiron template or a per-file generator input that an external contributor can update.CONTRIBUTING.mdexplicitly states that manual SDK modifications persist between generations.The merged predecessor #790 received the same Castiron-ownership review. Its maintainer documented the established persistent runtime-patch path for this file, resolved that review, and the final head received human approval. This follow-up intentionally uses that same repository path without altering
.castiron.stats.ymlor other generation metadata. Propagating the behavior across generated SDKs requires an internal Castiron change by maintainers and is separate from preserving correct routing inopenai-go.Regression coverage
Coverage includes:
profile=.../V1versus.../v1routing;charsetvalues;message/external-bodyaccess-type,permission, andmode;text/plainformatanddelsp, with contextual negative controls;multipart/relatedtype,multipart/signedprotocolandmicalg,multipart/encryptedprotocol,multipart/reportreport-type, andtext/csvheader;*=text;The existing #790 regression cases remain in place.
Validation
Current head:
e8b7a35379956aa60dae137bceacaccfb9f63feb.packages/ssestream/ssestream.goandpackages/ssestream/content_type_case_test.go;go test ./packages/ssestream: pass;go test -race ./packages/ssestream: pass;go vet ./packages/ssestream: pass;go fmt ./packages/ssestream: clean;git diff --check: pass;Hosted Go 1.26 job blocker
The hosted Go 1.26 job has failed in three separate attempts before executing any root, examples, external-consumer, or changed-package test. Every attempt stops in the unrelated
Verify mock-server installation integritystep with:The following test steps are then skipped. The feature diff does not touch
scripts/test-mock,scripts/steady, workflows, or dependency metadata, and the changed package passes locally under Go 1.26.6. This red check is therefore recorded as a repeated hosted-runner/mock-installation blocker rather than a product-test failure.