Update ghs_ redaction regex for new stateless token format#35063
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
ghs_ redaction regex for new stateless token format
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. No action needed: PR #35063 is already merged. Reviewed the ghs_ regex simplification — change is a reasonable simplification that trades a complex alternation for a single greedy character class with a 36-char minimum. No blocking correctness or security issues identified; over-redaction risk is acceptable for a secret-redaction tool. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35063 does not have the 'implementation' label and has 0 new lines in default business logic directories (src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/), which is well below the 100-line threshold. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.3M
| @@ -190,8 +190,9 @@ describe("redact_secrets.cjs", () => { | |||
|
|
|||
There was a problem hiding this comment.
[/tdd] Stale test name: "JWT-like" no longer reflects the new unified format — consider renaming to something like "should redact boundary-length GitHub Server-to-Server Token (ghs_)".
💡 Suggested rename
it("should redact boundary-length GitHub Server-to-Server Token (ghs_)", async () => {The JWT-segment structure was intentional in the old test (it verified the dot-separated 3-segment shape). Since the token format is now just ghs_[A-Za-z0-9._-]{36,}, the "JWT-like" label is misleading and may confuse future readers about what invariant is actually being asserted.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
i️ Note on test inflation (6:1 ratio)The test file grew at a 6:1 ratio vs the production file (+24 vs +4 lines), which exceeds the 2:1 threshold and triggered a 10-point deduction. However, in this context the inflation is expected and appropriate: a single regex change warrants multiple test cases covering distinct token formats (classic, long JWT-like, boundary-length, and dash-containing). No remediation needed. 📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
ghs_installation tokens now use a stateless, variable-length format and may include.,_, and-. The existing pattern was still split between legacy fixed-length matching and JWT-like segmentation logic, which could miss valid new-format tokens.Regex update (secret redaction core)
ghs_built-in pattern inactions/setup/js/redact_secrets.cjswith a single format-aligned matcher:[0-9A-Za-z._-]{36,}Test alignment + boundary coverage
ghs_expectations inactions/setup/js/redact_secrets.test.cjsto match the new canonical pattern.ghs_tokens,