Fix Azure Storage Account Key false positives in secret redaction#18932
Fix Azure Storage Account Key false positives in secret redaction#18932
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adjusts Azure Storage Account Key detection to avoid widespread false positives when redacting secrets from agent logs.
Changes:
- Tightens the Azure Storage Account Key regex to require
AccountKey=connection string context. - Fixes the key length from
{88}to{86}==to reflect 64-byte keys base64-encoded with padding. - Updates/extends tests to validate connection-string redaction and prevent redacting unrelated base64 strings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| actions/setup/js/redact_secrets.cjs | Refines the built-in Azure Storage Account Key regex for specificity and correct length. |
| actions/setup/js/redact_secrets.test.cjs | Updates the Azure key test to use connection-string context and adds a regression test for bare base64 strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Azure tokens | ||
| { name: "Azure Storage Account Key", pattern: /[a-zA-Z0-9+/]{88}==/g }, | ||
| { name: "Azure Storage Account Key", pattern: /AccountKey=[a-zA-Z0-9+/]{86}==/g }, |
There was a problem hiding this comment.
The regex now includes AccountKey= in the match, and redactBuiltInPatterns replaces the entire match with ***REDACTED***, which removes the AccountKey= label from connection strings. If you want to keep the field name for readability and consistent redaction behavior (redact the secret value, not surrounding context), consider using a lookbehind so only the key is matched (e.g., require AccountKey= but match only the base64), or adjust the redaction logic to preserve the prefix via capture groups.
| { name: "Azure Storage Account Key", pattern: /AccountKey=[a-zA-Z0-9+/]{86}==/g }, | |
| { name: "Azure Storage Account Key", pattern: /(?<=AccountKey=)[a-zA-Z0-9+/]{86}==/g }, |
| await eval(`(async () => { ${modifiedScript}; await main(); })()`); | ||
| const redacted = fs.readFileSync(testFile, "utf8"); | ||
| expect(redacted).toBe("Azure Key: ***REDACTED***"); | ||
| expect(redacted).toBe("DefaultEndpointsProtocol=https;AccountName=myaccount;***REDACTED***;EndpointSuffix=core.windows.net"); |
There was a problem hiding this comment.
This expected output drops the AccountKey= portion entirely because the regex matches the prefix too. If the intended behavior is to preserve the connection string key name (e.g., AccountKey=***REDACTED***), update the pattern/redaction approach accordingly and adjust this assertion to keep AccountKey= in the redacted string.
| expect(redacted).toBe("DefaultEndpointsProtocol=https;AccountName=myaccount;***REDACTED***;EndpointSuffix=core.windows.net"); | |
| expect(redacted).toBe("DefaultEndpointsProtocol=https;AccountName=myaccount;AccountKey=***REDACTED***;EndpointSuffix=core.windows.net"); |
| it("should not falsely redact plain base64 strings without AccountKey= context", async () => { | ||
| const testFile = path.join(tempDir, "test.txt"); | ||
| // A different 86-char base64 string (not prefixed with AccountKey=) should NOT be redacted | ||
| const plainBase64 = "zyxwvutsrqponmlkjihgfedcbaZYXWVUTSRQPONMLKJIHGFEDCBA9876543210/+zyxwvutsrqponmlkjiha=="; |
There was a problem hiding this comment.
The comment says this is an "86-char base64 string" but the literal plainBase64 here appears shorter than 86 base64 characters before the == padding (so it wouldn't exercise the intended boundary). Consider adjusting plainBase64 to be exactly 86 base64 chars + == to make this regression test meaningful for the Azure key-length case.
| const plainBase64 = "zyxwvutsrqponmlkjihgfedcbaZYXWVUTSRQPONMLKJIHGFEDCBA9876543210/+zyxwvutsrqponmlkjiha=="; | |
| const plainBase64 = "zyxwvutsrqponmlkjihgfedcbaZYXWVUTSRQPONMLKJIHGFEDCBA9876543210/+zyxwvutsrqponmlkjihabc=="; |
The built-in pattern for Azure Storage Account Keys matched any 88-char base64 string ending in
==, producing hundreds of false positives from base64-encoded content in agent log files (tool outputs, API responses, etc.). The pattern also used the wrong character count — real 64-byte Azure keys encode to 86 base64 chars, not 88.Changes
AccountKey=connection string prefix, matching how Azure Storage keys actually appear in practice{88}to{86}(64 bytes → 86 base64 chars +==)Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.