fix(sdk): redact standalone keys and structured credentials - #322
fix(sdk): redact standalone keys and structured credentials#322mldangelo-oai wants to merge 5 commits into
Conversation
|
@codex review |
|
@codex review |
![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: dd61f0df2c
ℹ️ 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".
| ); | ||
| return redactQuotedCredentialValues(withoutPrivateKeys) | ||
| .replaceAll( | ||
| /(\b(?:authorization|auth)\b(?:\\?["'])?\s*[:=]\s*)([A-Za-z][A-Za-z0-9._~-]{0,63})((?:\s|%20|\+)+)(?=[A-Za-z_][A-Za-z0-9_-]{0,64}\s*=)[^\r\n]+/giu, |
There was a problem hiding this comment.
username* before redacting Digest headers
When a valid Digest header starts with the extended username* parameter, this lookahead rejects the *, so the structured-header replacement is skipped. The following generic replacement then redacts only the first token; for example, Authorization: Digest username*=UTF-8''user, response="RSECRET" retains response="RSECRET" in the emitted error. Accept the full auth-param token syntax so the entire structured credential is redacted.
Useful? React with 👍 / 👎.
| ); | ||
| return redactQuotedCredentialValues(withoutPrivateKeys) | ||
| .replaceAll( | ||
| /(\b(?:authorization|auth)\b(?:\\?["'])?\s*[:=]\s*)([A-Za-z][A-Za-z0-9._~-]{0,63})((?:\s|%20|\+)+)(?=[A-Za-z_][A-Za-z0-9_-]{0,64}\s*=)[^\r\n]+/giu, |
There was a problem hiding this comment.
This new structured-credential matcher only accepts the exact field names authorization and auth, although the subsequent credential matchers deliberately support names such as client_authorization_value and auth_token. When those fields contain parameterized credentials, the fallback redacts only the scheme or first value; for example, client_authorization_value=Digest username="example", response="LEAK_SECRET" leaves response="LEAK_SECRET" visible. Apply the structured-header handling to the same credential-field name forms supported by the other matchers.
Useful? React with 👍 / 👎.
| ) | ||
| .replaceAll( | ||
| /(\b[A-Za-z0-9_-]{0,64}(?:api[_-]?key|access[_-]?key(?:[_-]?id)?|private[_-]?key|authorization|auth|token|secret|credential|signature|sig|password|passwd)(?:[_-][A-Za-z0-9_-]{1,64}|(?:value|data|token|secret|credential|password|header|field|id|key)[A-Za-z0-9_-]{0,48})?\b(?:\\?["'])?\s*[:=]\s*(?:\\?["'])?)(?!\[redacted\]|[A-Za-z][A-Za-z0-9._~-]{0,63}(?:\s|%20|\+)+\[redacted\])[^\s"',;}&\\\]]+/giu, | ||
| /(\b[A-Za-z0-9_-]{0,64}(?:api[_-]?keys?|access[_-]?key(?:[_-]?id)?|private[_-]?key|authorization|auth|token|secret|credentials?|signature|sig|password|passwd)(?:[_-][A-Za-z0-9_-]{1,64}|(?:value|data|token|secret|credential|password|header|field|id|key)[A-Za-z0-9_-]{0,48})?\b(?:\\?["'])?\s*[:=]\s*(?:\\?["'])?)(?!\[redacted\]|[A-Za-z][A-Za-z0-9._~-]{0,63}(?:\s|%20|\+)+\[redacted\])[^\s"',;}&\\\]]+/giu, |
There was a problem hiding this comment.
Plural fields commonly hold arrays or objects, but the unquoted replacement stops at quotes, commas, and closing brackets. Consequently, inputs such as credentials=[alice:SECRET_ONE,bob:SECRET_TWO] become credentials=[redacted],bob:SECRET_TWO], and JSON such as {"credentials":["SECRET_ONE","SECRET_TWO"]} retains both secrets. The newly recognized plural fields need collection-aware or conservatively bounded redaction rather than redacting only the first fragment.
Useful? React with 👍 / 👎.
|
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: 552c382ae4
ℹ️ 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".
| ); | ||
| return redactQuotedCredentialValues(withoutPrivateKeys) | ||
| .replaceAll( | ||
| /(\b(?:authorization|auth)\b(?:\\?["'])?\s*[:=]\s*)([A-Za-z][A-Za-z0-9._~-]{0,63})((?:\s|%20|\+)+)[A-Za-z_][A-Za-z0-9_-]{0,64}\s*=\s*(?:"[^"]*"|'[^']*'|[^\s,;]+)(?:\s*,\s*[A-Za-z_][A-Za-z0-9_-]{0,64}\s*=\s*(?:"[^"]*"|'[^']*'|[^\s,;]+))*/giu, |
There was a problem hiding this comment.
When a valid quoted authorization parameter contains an escaped delimiter, such as Authorization: Digest username="a\"b", response="SECRET", the "[^"]*" branch treats the escaped quote as the end of the value. Redaction therefore stops early and returns the remainder—including the following Digest response or signature—verbatim. Continue through quoted-pairs to the actual unescaped closing delimiter before replacing the complete parameter list.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8b9589bc3
ℹ️ 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".
|
|
||
| function redactCredentialCollections(message: string): string { | ||
| const assignment = | ||
| /(\b[A-Za-z0-9_-]{0,64}(?:credentials|api[_-]?keys)\b(?:\\*["'])?\s*[:=]\s*)([\[{])/giu; |
There was a problem hiding this comment.
The new collection helper still matches only names ending exactly in credentials or apiKeys, while the subsequent credential matchers explicitly recognize suffixed forms such as credentials_value and api_keys_data. For an input like credentials_value=["SECRET_ONE","SECRET_TWO"], the fallback replaces only [ and emits both secrets as credentials_value=[redacted]"SECRET_ONE","SECRET_TWO"]; extend this matcher to the same suffix forms before parsing the collection.
Useful? React with 👍 / 👎.
|
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf34227cb2
ℹ️ 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".
| redactCredentialCollections(withoutPrivateKeys), | ||
| ) | ||
| .replaceAll( | ||
| /(\b[A-Za-z0-9_-]{0,64}(?:authorization|auth)(?:[_-][A-Za-z0-9_-]{1,64}|(?:value|data|token|secret|credential|password|header|field|id|key)[A-Za-z0-9_-]{0,48})?\b(?:\\?["'])?\s*[:=]\s*)([A-Za-z][A-Za-z0-9.+~-]{0,63})((?:\s|%20|\+)+)[A-Za-z_*][A-Za-z0-9_.*-]{0,64}\s*=\s*(?:"(?:\\.|[^"\\])*"|'(?:\\.|[^'\\])*'|[^\s,;]+)(?:\s*,\s*[A-Za-z_*][A-Za-z0-9_.*-]{0,64}\s*=\s*(?:"(?:\\.|[^"\\])*"|'(?:\\.|[^'\\])*'|[^\s,;]+))*/giu, |
There was a problem hiding this comment.
The updated matcher now handles username*, but it still requires the first auth-param name to match [A-Za-z_*][A-Za-z0-9_.*-]* rather than the full HTTP token syntax. When an extension scheme uses a valid parameter beginning with a digit or another permitted punctuation character, Authorization: Custom 0nonce=visible, response=SYNTHETIC_SECRET falls through and becomes Authorization: Custom [redacted], response=SYNTHETIC_SECRET, exposing the later credential. Match the complete token syntax for parameter names (and auth schemes) before applying the fallback.
Useful? React with 👍 / 👎.
| "$1[redacted]", | ||
| ) | ||
| .replaceAll( | ||
| /-----BEGIN [A-Z0-9 ]*PRIVATE KEY-----[\s\S]*?(?:-----END [A-Z0-9 ]*PRIVATE KEY-----|$)/giu, |
There was a problem hiding this comment.
When an error contains a standalone ASCII-armored OpenPGP private key, its standard -----BEGIN PGP PRIVATE KEY BLOCK----- label does not match because this expression requires the label to end immediately after PRIVATE KEY. The entire block, including its secret payload, is therefore returned unchanged by redactedErrorMessage; include the PRIVATE KEY BLOCK armor form in the standalone-key redaction.
Useful? React with 👍 / 👎.
|
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2d786be76
ℹ️ 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".
| redactCredentialCollections(withoutPrivateKeys), | ||
| ) | ||
| .replaceAll( | ||
| /(\b[A-Za-z0-9_-]{0,64}(?:authorization|auth)(?:[_-][A-Za-z0-9_-]{1,64}|(?:value|data|token|secret|credential|password|header|field|id|key)[A-Za-z0-9_-]{0,48})?\b(?:\\?["'])?\s*[:=]\s*)([A-Za-z0-9][A-Za-z0-9.+~-]{0,63})((?:\s|%20|\+)+)[!#$%&'*+.^_`|~0-9A-Za-z-]{1,65}\s*=\s*(?:"(?:\\.|[^"\\])*"|'(?:\\.|[^'\\])*'|[^\s,;]+)(?:\s*,\s*[!#$%&'*+.^_`|~0-9A-Za-z-]{1,65}\s*=\s*(?:"(?:\\.|[^"\\])*"|'(?:\\.|[^'\\])*'|[^\s,;]+))*/giu, |
There was a problem hiding this comment.
The updated matcher now accepts additional token punctuation, but the new {1,65} ceiling still rejects valid HTTP auth-param names longer than 65 characters. In that case the generic fallback redacts only the first fragment; for example, Authorization: Digest <66-character-name>=foo, response=SECRET becomes Authorization: Digest [redacted], response=SECRET. Remove the arbitrary token-length limit so later structured credential parameters cannot leak.
Useful? React with 👍 / 👎.
| if (quote !== undefined) { | ||
| if (character === "\\") position += 1; | ||
| else if (character === quote) quote = undefined; | ||
| } else if (character === '"' || character === "'") { | ||
| quote = character; |
There was a problem hiding this comment.
When an error contains a JSON-stringified object with a credential collection, the quotes inside the encoded collection are backslash-escaped, but this scanner treats the first escaped quote as an opening structural quote. It therefore never recognizes the closing bracket and discards the remainder of the message; for example, {\"credentials\":[\"SECRET\"],\"safe\":\"visible\"} is reduced to {\"credentials\":"[redacted]", losing both valid JSON structure and the safe diagnostic fields. Account for escaped delimiters outside quote mode, including nested JSON-stringification levels.
Useful? React with 👍 / 👎.
|
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2d786be76
ℹ️ 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".
| function redactQuotedCredentialValues(message: string): string { | ||
| const assignment = | ||
| /(\b[A-Za-z0-9_-]{0,64}(?:api[_-]?key|access[_-]?key(?:[_-]?id)?|private[_-]?key|authorization|auth|token|secret|credential|signature|sig|password|passwd)(?:[_-][A-Za-z0-9_-]{1,64}|(?:value|data|token|secret|credential|password|header|field|id|key)[A-Za-z0-9_-]{0,48})?\b(?:\\*["'])?\s*[:=]\s*)(\\*)(["'])/giu; | ||
| /(\b[A-Za-z0-9_-]{0,64}(?:api[_-]?keys?|access[_-]?key(?:[_-]?id)?|private[_-]?key|authorization|auth|token|secret|credentials?|signature|sig|password|passwd)(?:[_-][A-Za-z0-9_-]{1,64}|(?:value|data|token|secret|credential|password|header|field|id|key)[A-Za-z0-9_-]{0,48})?\b(?:\\*["'])?\s*[:=]\s*)(\\*)(["'])/giu; |
There was a problem hiding this comment.
When a newly supported quoted plural field ends in a literal backslash, its valid JSON representation has two backslashes before the closing quote, but the delimiter scanner only accepts a closing quote preceded by exactly openingSlashes backslashes. Thus {"credentials":"SYNTHETIC_SECRET\\\\","safe":"visible"} becomes the invalid {"credentials":"[redacted]"safe":"visible"}, while the equivalent plain-text assignment can discard the entire safe suffix. Determine whether the quote is escaped from the backslash parity rather than requiring an exact count.
Useful? React with 👍 / 👎.
|
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. |
ianw-oai
left a comment
ianw-oai
left a comment
There was a problem hiding this comment.
we should just remove this feature
2 participants
Summary
Verification
Companion monorepo backport: https://github.com/openai/openai/pull/1262861