autoreview: secret-scanner false positives on identifier and member-expression values (e.g. `apiSecret: ctx.cloudinary.apiSecret`)

[jrans]

Summary

secret_text_risk in skills/autoreview/scripts/autoreview (as of 8184e2b) refuses review bundles containing ordinary TypeScript code shapes that reference credentials rather than containing them. Because the refusal is a hard SystemExit with no allowlist or override, a single such line makes --mode branch unusable for the whole PR.

False-positive shapes

All of these are flagged by the assignment analyzer (SECRET_ASSIGNMENT_PATTERN + secret_text_risk):

// 1. Bare identifier value of 12+ characters (a reference, not a literal)
const signingPhrase = "test-signing-phrase"; // test fixture
signCloudinaryUploadParams({ apiSecret: signingPhrase, ... });

// 2. Member expression whose root is outside the hardcoded whitelist
return { apiKey: ctx.cloudinary.apiKey, ... };          // `ctx` not in UNQUOTED_SECRET_REFERENCE_PATTERNS
cloudinary: { apiSecret: environment.cloudinaryApiSecret } // `environment` not whitelisted (though `env` is)

Meanwhile apiKey: env.CLOUDINARY_API_KEY passes, since env is in the whitelist — the same code with a context object named ctx or environment is refused.

Why these can't leak secrets

• A pure identifier ([A-Za-z_$][A-Za-z0-9_$]*) is a reference; the current rule already accepts identifiers under 12 characters, so the length cutoff only penalizes descriptive names (signingPhrase, sessionToken) without excluding any literal secret material — literals with only identifier characters and no digits/symbols at 12+ chars are overwhelmingly variable names in practice.
• A dotted member expression (a.b.c) likewise cannot be a secret; the root-whitelist approach means any codebase whose config/context object isn't named one of the ~20 blessed roots gets refused.

Suggestions (either would fix our case)

1. Treat any pure identifier or member expression ([A-Za-z_$][A-Za-z0-9_$]*(\.[A-Za-z_$][A-Za-z0-9_$]*)*) as a safe reference in the assignment analyzer, regardless of length — keeping the existing checks for quoted strings, URI credentials, and the concrete token-format patterns.
2. Or: provide an escape hatch (repo-level config or CLI flag) so branch review can proceed with flagged lines redacted from the bundle instead of refusing outright.

Context

Hit in CI on a PR adding Cloudinary upload signing — refusing to include secret-like content in review bundle; clean or redact branch diff before running autoreview with ~30 flagged lines, all of the shapes above. We worked around it by destructuring to shorthand properties and shortening a fixture name under 12 characters, but that's regex-appeasement rather than a sustainable pattern.

[clawsweeper]

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Reviewed July 30, 2026, 2:00 PM ET / 18:00 UTC.

Summary
Keep this issue open. Current main supports parser-verified JavaScript/TypeScript source references, but still limits unquoted member-expression roots to a fixed allowlist, so the reported ctx and environment forms remain outside the accepted path. The merged work in #92 partially addressed the problem; the narrower proposal in #102 closed unmerged and is not a safe canonical replacement.

Reproducibility: no. live reproduction was run in this read-only review, but the current scanner source provides a high-confidence source reproduction: ctx and environment are absent from the accepted member-expression roots while env is present.

Ways to help us reproduce this

• Add a screenshot or short recording showing the behavior.
• Add expected vs actual behavior.
• Include redacted logs or terminal output.

Maintainer decision needed

Question	Recommendation
Should verified JavaScript/TypeScript identifier and non-computed member-expression references be accepted regardless of their root name, or should autoreview retain a curated root allowlist?	Accept verified source references: Recommended: use the existing parser path to accept only syntactically verified identifiers and non-computed member expressions in JavaScript/TypeScript, regardless of root name.

Why: Both choices affect the fail-closed secret-scanning boundary: a broader syntax rule removes false positives, while a root allowlist limits what source forms may enter review bundles.

Next step
A focused patch is plausible, but a maintainer must choose the permanent accepted-reference grammar at this secret-scanning boundary before automation should implement it.

Security
Needs attention: The issue exposes a real review-workflow false positive at a secret-scanning boundary, so any fix must preserve dialect-scoped parser verification and fail-closed handling of literal credentials.

Review details

Best possible solution:

Define a narrow parser-backed JavaScript/TypeScript rule that accepts syntactically verified identifier and non-computed member-expression references regardless of root name, while retaining strict rejection for literals, URLs, token formats, computed expressions, and all non-source files.

Do we have a high-confidence way to reproduce the issue?

No live reproduction was run in this read-only review, but the current scanner source provides a high-confidence source reproduction: ctx and environment are absent from the accepted member-expression roots while env is present.

Is this the best way to solve the issue?

Unclear until maintainers choose the allowed source-reference grammar. A parser-backed syntax rule is safer and less brittle than adding provider- or project-specific root names, provided it remains limited to verified JavaScript/TypeScript source.

AGENTS.md: found and applied where relevant.

Remaining risk / open question:

• Accepting arbitrary text that merely resembles a member expression could weaken the fail-closed secret boundary if the parser/dialect guard is broadened beyond verified JavaScript and TypeScript source.
• A configurable bypass or redaction-only continuation would alter the review trust model and needs an explicit maintainer policy decision before implementation.

Codex review notes: model internal, reasoning high; reviewed against fe588b1a6267.

Label changes

Label changes:

• add issue-rating: 🦞 diamond lobster: Current issue advisory state selects this label.
• add clawsweeper:source-repro: Current issue advisory state selects this label.
• remove issue-rating: 🐚 platinum hermit: Current issue advisory state no longer selects this label.
• remove clawsweeper:needs-live-repro: Current issue advisory state no longer selects this label.

Label justifications:

• P2: This blocks ordinary branch-review workflows for affected source shapes but does not indicate a current credential disclosure or runtime outage.
• impact:security: The reported behavior is in autoreview’s credential-detection boundary, where an overly broad fix could admit sensitive data into review bundles.

Evidence reviewed

Security concerns:

• [medium] Keep source-reference acceptance parser-scoped — skills/autoreview/scripts/autoreview:3219
  A root-independent rule is safe only after the value is verified as a JavaScript/TypeScript identifier or non-computed member expression; applying a textual regex broadly could admit secret-like data from non-source files or unsupported syntax.
  Confidence: 0.93

What I checked:

• Current scanner retains a root allowlist: The current secret_text_risk implementation accepts JavaScript/TypeScript member references through UNQUOTED_SECRET_REFERENCE_PATTERNS, whose listed roots include env but not the reported ctx or environment; the source-backed path therefore still supports the reported false-positive mechanism. (skills/autoreview/scripts/autoreview:3219, fe588b1a6267)
• Merged partial fix: Merged PR 92 introduced parser-backed recognition of verified JavaScript and TypeScript credential references and is the provenance for the current source-reference handling, but it did not establish acceptance of arbitrary object roots. (skills/autoreview/scripts/autoreview:2612, 8d37aeaae5e1)
• Unmerged follow-up is not canonical: PR 102 proposed extra provider roots, including data and providerConfig, but it closed unmerged with a dirty merge state. It does not resolve the remaining grammar decision for ctx and environment. (7d0cd1953c01)
• Security-boundary history: Recent merged hardening work kept autoreview fail-closed around ambiguous review inputs, making the accepted-reference grammar a security-boundary decision rather than a safe mechanical allowlist expansion. (skills/autoreview/scripts/autoreview:2612)

Likely related people:

• steipete: Authored and merged PR 92, which introduced the current parser-verified source-reference path that this issue needs to extend or refine. (role: introduced parser-backed source-reference behavior; confidence: high; commits: 8d37aeaae5e1; files: skills/autoreview/scripts/autoreview, skills/autoreview/tests/test_autoreview_hardening.py)
• vincentkoc: Authored the recent autoreview fail-closed hardening work referenced by this issue’s surrounding history, so they are likely familiar with the intended secret-scanning boundary. (role: recent security-boundary contributor; confidence: medium; files: skills/autoreview/scripts/autoreview, skills/autoreview/tests/test_autoreview_hardening.py)

How this review workflow works

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.