Skip to content

refactor(review): extract shared secret-detection module (#4608) #4678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 10, 2026
Merged

refactor(review): extract shared secret-detection module (#4608) #4678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion scripts/check-engine-parity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,18 +89,62 @@ export const SHARES_MEANINGFUL_FILE_MARKERS = Object.freeze([
"diffFilePriority(path) < 4",
] as const);

/** `review-enrichment/src/analyzers/secret-scan.ts` (REES) is a genuinely separate, deliberately WIDER
* advisory copy (deploys standalone on Railway with its own tsconfig/build/test pipeline — see that file's
* own header) of the shared hard-block primitives now in `src/review/secret-patterns.ts` (#4608). Unlike
* the other named pairs above, REES is NOT meant to converge toward byte-identical — a full-file comparison
* would immediately false-fail on REES's 80+ extra rules — so this pair's markers cover only the narrow,
* explicitly shared subset (the isPlaceholderSecretValue algorithm + the kind names both sides agree on). */
export const SECRET_DETECTION_TWIN_PAIR: NamedTwinPair = Object.freeze({
area: "secret-detection",
hostRelative: "src/review/secret-patterns.ts",
engineRelative: "review-enrichment/src/analyzers/secret-scan.ts",
hostFileName: "secret-patterns.ts",
engineFileName: "secret-scan.ts",
});

// isPlaceholderSecretValue's signature + full body (one marker per line, so a merely-reformatted-but-
// equivalent body doesn't false-fail) plus the HARD_SECRET_KINDS name literals that are EXACT string matches
// against REES's `kind` values today. `private_key_block`/`aws_access_key` are deliberately EXCLUDED: REES
// names the same two concepts `private_key`/`aws_access_key_id` (a pre-existing, out-of-scope naming
// divergence) — including them here would false-fail this check on the very PR that introduces it.
export const SECRET_DETECTION_MARKERS = Object.freeze([
"function isPlaceholderSecretValue(value: string): boolean {",
"if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true;",
"if (new Set(value.toLowerCase()).size <= 2) return true;",
"if (LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN.test(value)) return true;",
"if (ALL_LOWERCASE_SEGMENTS_PATTERN.test(value) && SELF_NAMING_FIXTURE_SUFFIX_PATTERN.test(value)) return true;",
"return hasLongSequentialRun(value);",
'"github_token"',
'"github_pat"',
'"slack_token"',
'"google_api_key"',
'"gitlab_token"',
'"npm_token"',
'"stripe_secret_key"',
'"sendgrid_key"',
'"huggingface_token"',
'"voyage_api_key"',
'"firecrawl_api_key"',
'"jwt"',
'"generic_secret_assignment"',
] as const);

/** Every explicitly named twin pair, checked for core-marker presence in `runEngineParityChecks` — the
* same escape hatch #4518 built for `GATE_DECISION_TWIN_PAIR`, generalized (#4605) so a function-level or
* nested-directory duplicate can be added here without inventing a new mechanism. `GATE_DECISION_TWIN_PAIR`
* additionally gets the co-edit-or-version-bump enforcement (`checkGateDecisionVersionBump`) since its two
* sides are deliberately maintained as structurally divergent implementations; the other pairs here are
* meant to stay much closer to byte-identical, so presence-check plus a content marker on the specific
* historically-drifted value (see `DIFF_FILE_PRIORITY_MARKERS`) is the proportionate guard for now. */
* historically-drifted value (see `DIFF_FILE_PRIORITY_MARKERS`) is the proportionate guard for now —
* `SECRET_DETECTION_TWIN_PAIR` is the exception (see its own doc comment): its two sides are expected to
* diverge everywhere EXCEPT the explicitly shared marker subset. */
export const NAMED_TWIN_PAIRS: ReadonlyArray<{ pair: NamedTwinPair; markers: readonly string[] }> = Object.freeze([
{ pair: GATE_DECISION_TWIN_PAIR, markers: GATE_DECISION_CORE_MARKERS },
{ pair: SAFE_URL_TWIN_PAIR, markers: SAFE_URL_MARKERS },
{ pair: DIFF_FILE_PRIORITY_TWIN_PAIR, markers: DIFF_FILE_PRIORITY_MARKERS },
{ pair: SHARES_MEANINGFUL_FILE_TWIN_PAIR, markers: SHARES_MEANINGFUL_FILE_MARKERS },
{ pair: SECRET_DETECTION_TWIN_PAIR, markers: SECRET_DETECTION_MARKERS },
]);
const ENGINE_SRC_ROOT = "packages/gittensory-engine/src";
const HOST_SRC_ROOT = "src";
Expand Down
140 changes: 15 additions & 125 deletions src/review/content-lane/security-scan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// Deterministic security/abuse scan for content submissions (content-lane primitive).
//
// SELF-CONTAINED NATIVE PORT (reviewbot→gittensory convergence). Byte-faithful to reviewbot's
// src/agents/awesome-claude/security-scan.ts + the shared core/secrets-scan.ts (inlined here so the
// module is self-contained). PURE — data in, data out, no I/O.
// src/agents/awesome-claude/security-scan.ts + the shared core/secrets-scan.ts. PURE — data in, data out,
// no I/O.
//
// Design principle (learned via adversarial review): the gate AUTO-CLOSES at high confidence with NO
// human queue, so a false-positive close PERMANENTLY rejects a legitimate submission — the worst
Expand All @@ -11,104 +11,19 @@
// (pipe-to-shell installers, prompt-injection prose, "exfil-looking" code) is indistinguishable at
// the regex level from legitimate documentation or defensive-security tooling — so it routes to
// MANUAL (a human decides), never an auto-close.

// ── Inlined secret-pattern scanner (reviewbot core/secrets-scan.ts) ───────────────────────────
const SECRET_PATTERNS: Array<{ name: string; re: RegExp }> = [
{ name: "github_token", re: /\bgh[pousr]_[A-Za-z0-9]{20,}\b/ },
{ name: "github_pat", re: /\bgithub_pat_[A-Za-z0-9_]{20,}\b/ },
{ name: "private_key_block", re: /-----BEGIN(?: RSA| EC| OPENSSH| PGP| DSA)? PRIVATE KEY-----/ },
{ name: "aws_access_key", re: /\bAKIA[0-9A-Z]{16}\b/ },
{ name: "slack_token", re: /\bxox[baprs]-[A-Za-z0-9-]{10,}\b/ },
{ name: "google_api_key", re: /\bAIza[0-9A-Za-z_-]{35}\b/ },
{ name: "gitlab_token", re: /\bglpat-[0-9A-Za-z_-]{20}(?![0-9A-Za-z_-])/ },
{ name: "npm_token", re: /\bnpm_[A-Za-z0-9]{36}\b/ },
// Stripe live secret / restricted keys: `sk_live_` / `rk_live_` + >=24 base62.
{ name: "stripe_secret_key", re: /\b(?:sk|rk)_live_[0-9A-Za-z]{24,}\b/ },
// SendGrid API key: `SG.` + 22-char id + `.` + 43-char secret (base64url).
{ name: "sendgrid_key", re: /\bSG\.[A-Za-z0-9_-]{22}\.[A-Za-z0-9_-]{43}(?![A-Za-z0-9_-])/ },
// Hugging Face user access token: `hf_` + 34 base62 chars.
{ name: "huggingface_token", re: /\bhf_[A-Za-z0-9]{34}\b/ },
// Voyage AI API key: `pa-` (platform) or `al-` (MongoDB Atlas) + base62 body.
{ name: "voyage_api_key", re: /\b(?:pa|al)-[A-Za-z0-9]{20,}(?![A-Za-z0-9_-])/ },
// Firecrawl API key: `fc-` + base62 body (alnum only; reject hyphen-continued identifiers).
{ name: "firecrawl_api_key", re: /\bfc-[A-Za-z0-9]{16,}(?![A-Za-z0-9_-])/ },
{ name: "jwt", re: /\beyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\b/ },
{ name: "seed_or_mnemonic", re: /\b(?:seed phrase|mnemonic)\b/i },
{ name: "bittensor_key", re: /\b(?:hot|cold)key\b\s*[:=]/i },
];

// Deliberately NOT in SECRET_PATTERNS above: unlike the format-specific patterns (a real GitHub token/AWS key
// ALWAYS matches its exact character format, so a bare .test() is precise enough), a keyword-plus-quoted-value
// SHAPE also matches plenty of non-secrets -- a Zod schema field (`password: z.string()`), a TypeScript type
// declaration, or a placeholder value ("xxx", "your-api-key-here", "<REDACTED>"). Captured so each match's
// VALUE can be checked against isPlaceholderSecretValue before counting as a hit; the value itself is never
// returned from this module (only the kind name), preserving the existing never-echo-the-secret guarantee.
const GENERIC_SECRET_ASSIGNMENT_PATTERN =
/(?:api[_-]?key|secret|token|password|passwd|access[_-]?key|client[_-]?secret)["']?\s*[:=]\s*["']([A-Za-z0-9+/=_-]{16,})["']/gi;

const PLACEHOLDER_VALUE_PATTERN = /placeholder|change[_-]?me|your[_-]|<[^>]*>|\bexample\b|redacted|dummy|\bsample\b|\btodo\b|\bfixme\b|\binsert\b|replace[_-]?me|\bfake\b/i;

// A string with NO repeated characters (e.g. "abcdefghijklmnop123") has HIGH Shannon entropy by raw
// character-frequency counting, but is obviously not a real secret -- entropy alone only measures frequency,
// not ORDER, so a keyboard-sequential/alphabetical run slips past a pure distinct-character-count check. Detect
// the longest run of consecutive ascending or descending character codes (e.g. "abcdefg" or "9876543") and
// treat a long one as a human-constructed test value, not a randomly generated credential -- real API
// keys/tokens essentially never contain a 6+ character monotonic run.
const MIN_SEQUENTIAL_RUN_LENGTH = 6;
function hasLongSequentialRun(value: string): boolean {
let ascendingRun = 1;
let descendingRun = 1;
for (let i = 1; i < value.length; i += 1) {
const diff = value.charCodeAt(i) - value.charCodeAt(i - 1);
ascendingRun = diff === 1 ? ascendingRun + 1 : 1;
descendingRun = diff === -1 ? descendingRun + 1 : 1;
if (ascendingRun >= MIN_SEQUENTIAL_RUN_LENGTH || descendingRun >= MIN_SEQUENTIAL_RUN_LENGTH) return true;
}
return false;
}

// Lowercase hyphenated mock names are fixtures; mixed-case/digit-bearing values containing "mock" remain
// plausible credentials and must still be reported by the generic assignment scanner.
const LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN = /^(?:[a-z]+-)*mock(?:-[a-z]+)*$/;

// All-lowercase-letters value check, shared by the self-naming-suffix exclusion below.
const ALL_LOWERCASE_SEGMENTS_PATTERN = /^[a-z]+(?:[-_][a-z]+)*$/;

// #4579-followup (metagraphed/gittensory#4524 "token = default-session-token"/"beta-session-token",
// awesome-claude#4758 "embedded_secret: unsafe_install_or_secret" -- both confirmed live, no real secret
// present): a value whose OWN last hyphen/underscore-separated segment is itself one of the same secret-shaped
// trigger words reads as a NAME for a concept ("this is a kind of token/secret"), not an opaque credential --
// a real generated token/key value never ends by literally restating what kind of thing it is. Deliberately
// NARROWER than "any multi-segment lowercase phrase": a Diceware-style passphrase like
// "alpha-bravo-charlie-delta" doesn't end in a trigger word, so it still correctly flags -- only values that
// self-identify as a token/secret/key/password NAME are excluded.
const SELF_NAMING_FIXTURE_SUFFIX_PATTERN = /[-_](?:token|secret|key|password|passwd)$/i;

/** True for an obvious non-secret filler value: a known placeholder phrase, a string built from at most 2
* distinct characters (e.g. "xxxxxxxxxxxxxxxx", "----------------"), a lowercase-hyphenated mock/fixture name
* (e.g. "mock-response-value"), a long monotonic character-code run (e.g. "abcdefghijklmnop123"), or a
* lowercase identifier whose own last segment self-names as a secret kind (e.g. "default-session-token",
* "unsafe_install_or_secret") — real high-entropy secrets never look like any of these. */
function isPlaceholderSecretValue(value: string): boolean {
if (PLACEHOLDER_VALUE_PATTERN.test(value)) return true;
if (new Set(value.toLowerCase()).size <= 2) return true;
if (LOWERCASE_HYPHENATED_MOCK_FIXTURE_PATTERN.test(value)) return true;
if (ALL_LOWERCASE_SEGMENTS_PATTERN.test(value) && SELF_NAMING_FIXTURE_SUFFIX_PATTERN.test(value)) return true;
return hasLongSequentialRun(value);
}

function hasGenericSecretAssignment(text: string): boolean {
// No zero-length-match / lastIndex-stall guard needed: the pattern's captured value alone requires 16+
// characters, so every match is well over 16 characters long and lastIndex always advances past match.index.
GENERIC_SECRET_ASSIGNMENT_PATTERN.lastIndex = 0;
let match: RegExpExecArray | null;
while ((match = GENERIC_SECRET_ASSIGNMENT_PATTERN.exec(text)) !== null) {
// The pattern's sole capturing group is mandatory (not `?`/`*`-wrapped), so it is always present
// whenever the overall match succeeds -- non-null by construction, not a runtime branch.
if (!isPlaceholderSecretValue(match[1]!)) return true;
}
return false;
}
//
// #4608: the format-specific patterns + placeholder-value heuristics used to be inlined here (a second,
// independent copy of src/review/secrets-scan.ts's primitives). Both files live under src/, same build,
// same deploy — no deploy-independence reason to hand-duplicate them; that duplication already caused two
// independent, currently-live drifts (see #4587/#4604). Now imported from the shared ../secret-patterns.

import {
GENERIC_SECRET_ASSIGNMENT_PATTERN,
HARD_SECRET_KINDS,
hasGenericSecretAssignment,
isPlaceholderSecretValue,
SECRET_PATTERNS,
} from "../secret-patterns";

export interface SecretScanResult {
found: boolean;
Expand All @@ -135,31 +50,6 @@ export interface SecurityFinding {
// for the pipe-to-shell install check (manual-flag) and the first-party grounding relaxation.
export const EXECUTABLE_CATEGORIES = new Set(["skills", "agents", "commands", "hooks", "mcp", "statuslines"]);

// Concrete credential formats only — NOT the weak heuristics (seed phrase / hot|coldkey) that would
// false-positive on legitimate Bittensor content. #2553: google_api_key/jwt are as format-precise as the
// original five (near-zero false-positive risk), and generic_secret_assignment already excludes
// placeholder/type-declaration/schema-shaped matches (see isPlaceholderSecretValue) before the kind is ever
// produced, so all three are safe unconditional hard blockers. voyage_api_key/firecrawl_api_key (#4604) are
// equally format-precise — keeping this gate in parity with the PR-diff gate it mirrors (safety.ts's
// HARD_SECRET_KINDS / secrets-scan.ts).
const HARD_SECRET_KINDS = new Set([
"github_token",
"github_pat",
"private_key_block",
"aws_access_key",
"slack_token",
"google_api_key",
"gitlab_token",
"npm_token",
"stripe_secret_key",
"sendgrid_key",
"huggingface_token",
"voyage_api_key",
"firecrawl_api_key",
"jwt",
"generic_secret_assignment",
]);

// A literal pipe-to-shell install. Common in legitimate installers (uv/rustup/deno/nvm), so this is a
// MANUAL flag for a human, never an auto-close.
const PIPED_INSTALL_RE = /\b(?:curl|wget)\b[^\n|]*\|\s*(?:sudo\s+)?(?:sh|bash|zsh|fish|python3?|node)\b/i;
Expand Down
31 changes: 6 additions & 25 deletions src/review/safety.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,38 +7,19 @@

import type { AdvisoryFinding } from "../types";
import { neutralizePromptInjection, safeReviewTitle } from "./prompt-injection";
import { HARD_SECRET_KINDS } from "./secret-patterns";
import { scanDiffForSecretsWithLocations } from "./secrets-scan";

// Concrete credential formats only — NOT the weak heuristics (`seed_or_mnemonic` / `bittensor_key`) that
// false-positive on legitimate config/workflow content. A `coldkey:` / `hotkey =` line or the word
// "mnemonic" in a .toml, .github/workflows/**, or wrangler/workers config is NOT a leaked credential, but it
// matches those two patterns — on these Bittensor repos that wrongly hard-blocked owner config/workflow PRs
// (RC6: #1505/#1495/#1485). A real-format token IS a leak regardless of the file it lives in, so we keep the
// concrete formats as hard blockers and ignore only the ambiguous heuristics. This mirrors the same gate the
// content lane already uses (src/review/content-lane/security-scan.ts).
//
// #2553: widened to match review-enrichment/src/analyzers/secret-scan.ts's richer, higher-recall rule set.
// google_api_key/jwt are as format-precise as the original five (near-zero false-positive risk).
// generic_secret_assignment is the one keyword-shaped pattern here — secrets-scan.ts already excludes
// placeholder/type-declaration/schema-shaped matches (see isPlaceholderSecretValue there) before this kind
// is ever produced, so it is safe to treat as an unconditional hard blocker like the rest.
const HARD_SECRET_KINDS = new Set([
"github_token",
"github_pat",
"private_key_block",
"aws_access_key",
"slack_token",
"google_api_key",
"gitlab_token",
"npm_token",
"stripe_secret_key",
"sendgrid_key",
"huggingface_token",
"voyage_api_key",
"firecrawl_api_key",
"jwt",
"generic_secret_assignment",
]);
// concrete formats as hard blockers and ignore only the ambiguous heuristics. HARD_SECRET_KINDS is shared
// (#4608) with the same gate the content lane uses (src/review/content-lane/security-scan.ts) via
// ./secret-patterns — google_api_key/jwt/generic_secret_assignment (#2553) and voyage_api_key/
// firecrawl_api_key (#4604) are as format-precise as the original five, so all are safe unconditional hard
// blockers; see that module's header for the full reasoning.

/** True when the safety scan is enabled. Flag-OFF (default) → every helper below is a no-op pass-through. */
export function isSafetyEnabled(env: {
Expand Down
Loading