Repository navigation
Conversation
c36f0b2 to
2d11d1b
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed 2d11d1b8fe72e47b22c28bef9ff652952cec818e. I found no new P0–P2 issue in this change.
Both new alternatives match github_pat_ followed by at least 20 ASCII letters, digits or underscores, with word boundaries (packages/core/src/redaction.ts:74 and packages/core/src/display-redaction.ts:103). Their fine-grained PAT behavior agrees; the pre-existing differences between classic-token families are unchanged. GitHub's credential reference confirms the github_pat_ prefix. These patterns are defensive masking, not token-validity checks: a sufficiently long non-secret identifier with that prefix is also hidden. I did not establish an official fixed-length guarantee or test a live credential.
After a fresh dependency install and the normal build:test, the three affected suites passed all 41 tests, the complete core suite passed all 934 tests, and the complete UI suite passed all 733 tests. An additional synthetic probe passed 3,262 assertions covering the 19/20-character threshold, mixed-case bodies and underscores, surrounding delimiters, bare and keyed values, MCP free-form arguments, character-by-character rendering and bounded streaming recovery. Removing the two new alternatives makes the synthetic bare PAT pass through unchanged again.
There is no schema or wire-protocol change. The local merge with main 9a32d423b is clean and retains main's epoch 217. CI for this exact head is now running (run 38139463822); it has not finished, so I cannot report it as green. I did not run the full monorepo suite, a live Copilot account, external MCP integration or interactive PTY/screenshot checks.
Both `redactSecrets` and the display redactor covered every classic GitHub token family (`ghp_`, `gho_`, `ghu_`, `ghs_`, `ghr_`) but not fine-grained PATs (`github_pat_...`) - the format GitHub has issued by default since 2022, and one of the token families Maka imports as a GitHub Copilot credential (`isSupportedGitHubCopilotAccountToken` accepts `gho_`, `ghu_`, and `github_pat_`; classic `ghp_` is refused). A bare printed fine-grained PAT therefore passed both redactors unmasked: it reached the model and the transcript (the PTY screen collector suppresses a line only when redaction changes it), was shown in the quiet panel/TUI, and was stored in the diagnostic log, so shared transcripts, screenshots, or diagnostics bundles leaked a live repository credential. The display redactor also kept leaking keyed lines such as `env` output (`GITHUB_TOKEN=github_pat_...`) that the core redactor's sensitive-key assignment rule already masked. Add the `github_pat_` alternative to both pattern lists. The display pattern keeps its reversible streaming contract: the token body's `_` is already inside the default continuation set. Fixes apache#6061 Generated-by: Codex
2d11d1b to
c895e0f
Compare
|
CI note: the I force-pushed the identical tree (same diff) to re-trigger |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I compared c895e0fba4282ed5beb781d60d3ec5a17dd285bf with my previous review at 2d11d1b8fe72e47b22c28bef9ff652952cec818e. The single-commit patches are identical in git range-diff; both redactors and all three changed test files are unchanged. I found no new P0–P2 issue in this update.
The behavior remains the additional github_pat_ alternative in packages/core/src/redaction.ts:74 and packages/core/src/display-redaction.ts:103, with a 20-character minimum body and word boundaries. MCP free-form secret detection inherits the core redactor. This is a masking heuristic, not validation of a live token; sufficiently long non-secret identifiers with that prefix are also masked.
On this exact head, after a fresh dependency install and normal test build, I reran the 41 focused tests, all 934 core tests, all 733 UI tests, and 3,262 assertions covering synthetic lengths, delimiters, keyed/bare values, MCP scrubbing, character-by-character streaming and bounded recovery. All passed. Removing the two PAT alternatives in an isolated negative control restores the original bare-token leak.
The merge with main 0a8e29d2ed6a2703052ecd961ee4e3044c58af13 is conflict-free and makes no protocol change; the merged tree passes the epoch guard at 217. GitHub reports this head mergeable, but CI run 38148445125 is still running at this check, so I am not claiming all checks have passed. I did not exercise real credentials, a live Copilot account, external MCP services, PTY interaction, or the full monorepo test suite.
|
Thanks for the contribution! Merging now. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: no blocking findings in the automated review of this head, green CI, and no conflicts.
Summary
Users whose session output prints a fine-grained GitHub PAT (
github_pat_...) now get that token masked everywhere Maka renders or stores text - explicitly including GitHub Copilot subscription users, becausegithub_pat_is one of the three token families Maka imports as a Copilot credential (isSupportedGitHubCopilotAccountTokenacceptsgho_,ghu_, andgithub_pat_; classicghp_is refused) - where before the quiet panel/TUI, the model/transcript path, and diagnostics exposed it in plain text, so sharing logs or a screenshot could leak a live repository credential.The problem:
redactSecrets(packages/core/src/redaction.ts) and the display redactor (packages/core/src/display-redaction.ts) enumerated the classic GitHub familiesghp_,gho_,ghu_,ghs_, andghr_, but not fine-grained PATs, so a bare printed PAT passed both unmasked. The display redactor also leaked keyed lines such asenvoutput (GITHUB_TOKEN=github_pat_...) that the core redactor's sensitive-key assignment rule already masked. The PTY screen collector suppresses a line only when redaction changes it (packages/runtime/src/pty-screen-collector.ts:501), the diagnostic log stores the redacted text (packages/core/src/diagnostic-log.ts:58), and MCP secret scrubbing usesisPatternSecretas the only signal for free-form positions (packages/core/src/mcp-secrets.ts:93).The change: one alternative added to each pattern list (
github_pat_[0-9A-Za-z_]{20,}), plus regression tests in the three suites that cover these paths. The display pattern's reversible streaming contract is unchanged; the token body's_is already inside the default continuation set.Fixes #6061
Verification
Minimal repro with fake bare tokens (no key prefix), run against the built
@maka/core(script importspackages/core/dist/redaction.js,display-redaction.js, andmcp-secrets.js).Before (only the two pattern lines reverted):
After:
Shape coverage around the same change (fake token, core vs. display redactor with only the two pattern lines reverted vs. this branch):
Checks actually run locally (Node 24.21.0, Windows 11):
node --testonredaction.test.js,display-redaction.test.js,mcp-secrets.test.js: with the two pattern lines reverted and the new tests in place, the four new assertions fail (4 of 41); with the change, 41/41 pass. This is the "tests fail without the change" evidence.npm --workspace @maka/core run test:dist: 934/934 pass.npm --workspace @maka/ui run test:dist: 733/733 pass (the display redactor is re-exported through@maka/ui).npm run build: exit 0.npm run lint: exit 0.npm run format:check: exit 0.npm run typecheck: exit 0.npx knip --workspace apps/desktop: exit 0.Not run: the full monorepo
npm testand the@maka/evalsuite, which need a live Runtime Host/desktop environment this machine cannot host; the changed package's own suite is green.AI use
Tool(s) and scope: Codex (OpenAI) drafted the pattern additions, the regression tests, and the reproduction under my direction; I reviewed the cited code paths, ran every check above, and own the change.
Checklist
Does this PR entail a change in behavior?