diff --git a/.changeset/category-severity-no-opt-in.md b/.changeset/category-severity-no-opt-in.md new file mode 100644 index 0000000000..3d297383fa --- /dev/null +++ b/.changeset/category-severity-no-opt-in.md @@ -0,0 +1,5 @@ +--- +"react-doctor": patch +--- + +Fix category-level severities silently force-enabling opt-out rules. A config that only re-stamps category severities (e.g. `categories: { "Maintainability": "warn" }`) no longer activates `defaultEnabled: false` rules such as `forbid-component-props`, `react-in-jsx-scope`, `no-danger`, or `design-no-redundant-size-axes` in that category — enabling an opt-out rule now requires pinning the rule itself (or a legacy alias key) to `"warn"`/`"error"` under `rules`, matching the documented contract. Category severities still re-stamp the severity of already-enabled rules, and `react-doctor rules` now previews the same behavior. diff --git a/.changeset/cli-audit-fixes.md b/.changeset/cli-audit-fixes.md new file mode 100644 index 0000000000..c9ba005890 --- /dev/null +++ b/.changeset/cli-audit-fixes.md @@ -0,0 +1,13 @@ +--- +"react-doctor": patch +--- + +CLI audit fixes: + +- Windows agent hooks no longer report false findings on every edit (cmd.exe's exit 9009 falls through, the local bin is probed as the runnable `.cmd` shim, 16 MiB output buffer, guarded output read). +- Legacy `.sh` agent hooks (≤0.5.8) are upgraded to the current Node hook by a once-per-repo migration on your next interactive scan (and on re-install) instead of scanning every edit twice; the cleanup is anchored to the exact legacy install paths and never touches unrelated hook groups in your settings. +- `ci upgrade --pr` restores the workflow file and explains an already-open React Doctor PR instead of silently claiming success; `ci config` bails to the apply-by-hand snippet on YAML syntax errors instead of crashing. +- The action-pin migration only rewrites `millionco/react-doctor` refs (in any owner casing) — a fork's `@main` is no longer rewritten to a tag that may not exist on the fork. +- Baseline and `--staged` scans resolve `config.plugins` from the real config directory, so custom-plugin findings are no longer mislabeled as newly introduced. +- A workspace module's `noScore: true` survives workspace scans, and the multi-project share prompt honors each project's merged `noScore`/`share` — any opted-out project now suppresses the aggregate share link. +- Degraded baseline results are no longer cached, and older binaries treat a newer CLI state schema as read-only (reads never rewrite the state file). diff --git a/.changeset/fix-1013-audit-followups.md b/.changeset/fix-1013-audit-followups.md new file mode 100644 index 0000000000..7a76f62a7d --- /dev/null +++ b/.changeset/fix-1013-audit-followups.md @@ -0,0 +1,22 @@ +--- +"oxlint-plugin-react-doctor": patch +"eslint-plugin-react-doctor": patch +--- + +fix(rules): close three follow-up gaps in the 20-day audit fixes + +- **Comment stripper**: `isRegexLiteralStart` now uses a Unicode-aware + identifier class, so a division after a non-ASCII identifier (`café / total`, + `合計 / 個数`) is no longer misread as a regex literal — which had blanked + real code up to the next slash and let `/* … */` comment bodies escape + stripping across the pattern-based security-scan rules. +- **`server-auth-actions`**: the cache/navigation exemption now requires the + callee to resolve to _any_ import rather than specifically `next/cache` / + `next/navigation`. A module-local `const revalidatePath = …` (a privileged + shadow) is still flagged, but a revalidation-only action importing through a + common re-export barrel (`import { revalidatePath } from "@/lib/cache"`) is no + longer a false positive. +- **`rn-no-raw-text`**: fragment piercing now sees through named + `` / `` (via the existing `isJsxFragmentElement` + helper), not only the shorthand `<>`, so children forwarded through a named + fragment into a host are classified the same as the shorthand form. diff --git a/.changeset/fix-979-ink-tui-rule-false-positives.md b/.changeset/fix-979-ink-tui-rule-false-positives.md new file mode 100644 index 0000000000..ac52840467 --- /dev/null +++ b/.changeset/fix-979-ink-tui-rule-false-positives.md @@ -0,0 +1,13 @@ +--- +"oxlint-plugin-react-doctor": patch +"eslint-plugin-react-doctor": patch +"react-doctor": patch +--- + +Fix four false positives found by React Doctor reviewing real, idiomatic React code (the Ink TUI in #979): + +- `no-derived-state` no longer flags state accumulators — a `setState` inside an effect whose functional updater computes the new value from its own parameter (`setKeys((previous) => new Set(previous).add(key))`, `setTotal((prev) => prev + count)`, `setItems((prev) => [...prev, item])`). Accumulated history is by definition not derivable from the current props/state. The spread-only object merge (`setForm((prev) => ({ ...prev, field: }))`) still reports. +- `no-array-index-as-key` no longer flags positional rendering of string fragments (characters, lines, tokens): `[...str]` and `Array.from(str)` where the source is provably a string (literal, template, `String()` call, or a binding/prop typed `string` in the same file), plus any `str.split(...)` receiver (only strings have `.split`, so no proof is needed) — including a local binding initialized from one (`const parts = line.split(" "); parts.map(...)`). Fragment position is the stable identity there — nothing reorders, filters, or carries per-item state. Data lists still report. +- `prefer-useReducer` now requires an actual co-update signal instead of merely counting `useState` calls: it reports only when the threshold number of distinct setters are called together as sibling statements of one handler/effect block. Independent state updated from separate handlers or separate keyboard-handler branches stays quiet, and the message no longer claims each `useState` "can trigger a separate render" (wrong since React 18 automatic batching) — it now explains the real rationale: state that changes together is easier to keep consistent as a single reducer action. +- `jsx-no-jsx-as-prop` only claims what it can prove: when the receiving component is not resolvable in the current file (imported), the message uses conditional wording ("If this child is memoized, …") instead of asserting a memo bailout that may not exist. Same-file components provably wrapped in `memo()` (or MobX `observer()`) keep the assertive message; provably plain function components already stayed quiet. +- `lazy()` / `React.lazy()` components are no longer treated as memoized — `lazy` defers loading but does not skip re-renders. `jsx-no-jsx-as-prop` now uses the conditional wording for them, and the memoised-consumer-gated rules (`jsx-no-new-object-as-prop`, `jsx-no-new-array-as-prop`, `jsx-no-new-function-as-prop`, `prefer-stable-empty-fallback`) no longer report fresh-reference props passed to a `lazy()` component, matching their premise of a provably defeated memo bailout. diff --git a/.changeset/fix-core-20-day-audit.md b/.changeset/fix-core-20-day-audit.md new file mode 100644 index 0000000000..2ff33e5985 --- /dev/null +++ b/.changeset/fix-core-20-day-audit.md @@ -0,0 +1,6 @@ +--- +"react-doctor": patch +"@react-doctor/core": patch +--- + +Core-engine reliability and security fixes from the 20-day audit. The lint binary-split retry budget is now scoped per batch and anchored at the first failure, so one pathological batch no longer starves the rest of the scan's recovery (and drop reasons name the limit that fired). `REACT_DOCTOR_SUPPLY_CHAIN_TIMEOUT_MS` can now raise the supply-chain budget instead of only lowering it. A corrupt per-file lint cache no longer fails every warm scan until hand-deleted, and the cache now busts when the oxlint child runs a different Node than the CLI (nvm fallback). The `/tmp` fallback cache directory is scoped per user so another local user can't pre-create and poison it, and the auto-detected default branch is validated before reaching git argv. The spawn argv guard is platform-sized (Windows 24k chars, macOS 800k, other POSIX 1.5M), so large `--scope lines` diffs no longer silently degrade to file scope on Linux/macOS. A security-scan I/O failure now skips that pass instead of failing the whole scan, and is reported on the run's telemetry as `securityScan.failed`. Note: the per-file lint cache is invalidated once on upgrade (its ruleset-hash separators changed). diff --git a/.changeset/fix-react-builtins-false-positives.md b/.changeset/fix-react-builtins-false-positives.md new file mode 100644 index 0000000000..c18b7fcb9f --- /dev/null +++ b/.changeset/fix-react-builtins-false-positives.md @@ -0,0 +1,22 @@ +--- +"oxlint-plugin-react-doctor": patch +--- + +fix(react-builtins): eliminate false positives across builtin DOM/JSX rules + +Harden the react-builtins rules against false positives on real-world code: + +- `button-has-type`, `iframe-missing-sandbox`, `checked-requires-onchange-or-readonly`: a JSX or `createElement` spread (`{...props}`) can forward the "missing" attribute at runtime, so these rules no longer report an attribute they cannot see — except an `';`, + }); + expect(findings.length).toBeGreaterThan(0); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/clickjacking-redirect-risk.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/clickjacking-redirect-risk.ts index 44e3cf7f61..2eb6c08e8a 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/clickjacking-redirect-risk.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/clickjacking-redirect-risk.ts @@ -14,13 +14,16 @@ export const clickjackingRedirectRisk = defineRule({ // identifier/property between `redirect(` and the keyword is caller input. // `(?!\s*(?:await\s+)?[\w$]*(?:safe|valid|sanitiz|allowlist|whitelist)` // skips targets already passed through a safe-redirect helper. - // The iframe branch requires URL-param shapes (`redirect=`), not the bare - // word `redirect`, which JSX comments inside the tag commonly mention. + // The iframe branch requires URL-param shapes (`redirect=`, query-style + // `role=` — `[?&]role=`, including the `&`-escaped form), not the bare + // word `redirect` or an ARIA `role="..."` attribute — the 700-char window + // bleeds past the tag, so a bare `role=` matched the accessible-iframe / + // iframe-inside-a-modal idioms. scan: scanByPattern({ shouldScan: (file) => isProductionSourcePath(file.relativePath) || isConfigOrCiPath(file.relativePath), pattern: - /\bredirect\s*\((?!\s*(?:await\s+)?[\w$]*(?:safe|valid|sanitiz|allowlist|whitelist)[\w$]*\s*\()[^)'"`\n]*\b(?:searchParams\.get|nextUrl\.searchParams|returnTo|callbackUrl|continue|next)\b| { }); expect(findings).toHaveLength(0); }); + + it("stays silent on spawn with a fixed command and request input in the argv array (no shell)", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/git.ts", + content: `spawn("git", ["log", req.query.branch]);\nspawnSync("ls", ["-la", req.query.dir]);\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("still flags spawn when the command itself is request input", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/run.ts", + content: `spawn(req.query.cmd, args);\n`, + }); + expect(findings).toHaveLength(1); + }); + + it("still flags spawn when a shell is explicitly enabled with request input in the call", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/run.ts", + content: `spawn("sh", ["-c", command], { shell: true, env: req.body });\n`, + }); + expect(findings).toHaveLength(1); + }); + + it("flags spawn of a shell binary running request input via -c", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/run.ts", + content: `spawn("sh", ["-c", req.query.cmd]);\n`, + }); + expect(findings).toHaveLength(1); + }); + + it("flags spawnSync of a shell binary running request input via -c", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/run.ts", + content: `spawnSync("bash", ["-c", req.body.script]);\n`, + }); + expect(findings).toHaveLength(1); + }); + + it("stays silent on a shell binary running a static -c command", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/tasks.ts", + content: `spawn("sh", ["-c", "git status"]);\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent on zero-taint spawn with { shell: true } and a static argv", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/tasks.ts", + content: `spawn("ls", ["-la"], { shell: true });\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent on zero-taint exec with { shell: true }", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/tasks.ts", + content: `execSync("git status", { shell: true });\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("flags spawn of a template-literal tainted command", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/run.ts", + content: "spawn(`${req.query.cmd}`);\n", + }); + expect(findings).toHaveLength(1); + }); + + it("flags spawn of a concatenated tainted command with { shell: true }", () => { + const findings = runScanRule(commandExecutionInputRisk, { + relativePath: "src/server/run.ts", + content: `spawn("tar -xf " + req.query.file, { shell: true });\n`, + }); + expect(findings).toHaveLength(1); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/command-execution-input-risk.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/command-execution-input-risk.ts index 4c840c0246..3cfe197f32 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/command-execution-input-risk.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/command-execution-input-risk.ts @@ -3,12 +3,49 @@ import { isDevToolingPath } from "./utils/is-dev-tooling-path.js"; import { isProductionScriptSourcePath } from "./utils/is-production-script-source-path.js"; import { scanByPattern } from "./utils/scan-by-pattern.js"; +const REQUEST_TAINT_SOURCE = String.raw`(?:req\.|request\.|params\.|query\.|body\.|searchParams|\$_(?:GET|POST|REQUEST))`; + // `(? isProductionScriptSourcePath(file.relativePath) && !isDevToolingPath(file.relativePath), - pattern: COMMAND_EXECUTION_INPUT_RISK_PATTERN, + pattern: COMMAND_EXECUTION_INPUT_RISK_PATTERNS, message: "Command execution appears to include request, query, body, or shell-interpolated input.", }), diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/dangerous-html-sink.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/dangerous-html-sink.regressions.test.ts index 81f3123eac..f3ab97b056 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/dangerous-html-sink.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/dangerous-html-sink.regressions.test.ts @@ -11,6 +11,22 @@ describe("security-scan/dangerous-html-sink — regressions", () => { expect(findings).toHaveLength(0); }); + it("stays silent on a string literal containing an escaped quote", () => { + const findings = runScanRule(dangerousHtmlSink, { + relativePath: "src/components/notice.ts", + content: `const showNotice = () => {\n noticeElement.innerHTML = "It\\"s static content";\n};\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent on a single-quoted literal containing a double quote", () => { + const findings = runScanRule(dangerousHtmlSink, { + relativePath: "src/components/notice.ts", + content: `const showNotice = () => {\n noticeElement.innerHTML = 'content';\n};\n`, + }); + expect(findings).toHaveLength(0); + }); + it("stays silent when the value is sanitized at the sink", () => { const findings = runScanRule(dangerousHtmlSink, { relativePath: "src/components/rich-text.tsx", @@ -590,6 +606,14 @@ describe("security-scan/dangerous-html-sink — regressions", () => { expect(findings).toHaveLength(1); }); + it("stays silent when every concat operand re-serializes existing DOM (cast-wrapped outerHTML)", () => { + const findings = runScanRule(dangerousHtmlSink, { + relativePath: "src/components/compose-node.ts", + content: `container.innerHTML = (icon as SVGElement).outerHTML + (label as HTMLSpanElement).outerHTML;\n`, + }); + expect(findings).toHaveLength(0); + }); + it("stays silent on mermaid-rendered SVG assigned from mermaid.render (tldraw shape)", () => { const findings = runScanRule(dangerousHtmlSink, { relativePath: "src/createMermaidDiagram.ts", diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/dangerous-html-sink.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/dangerous-html-sink.ts index 294993eefc..79c101b5dd 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/dangerous-html-sink.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/dangerous-html-sink.ts @@ -28,8 +28,11 @@ const HTML_TAINT_PATTERN = // literal/constant exemptions: without tolerating it the value never matches, // the scan window bleeds into the next statement, and the taint check fires on // unrelated tokens there (e.g. a following `content` variable). +// Escape-aware bodies (`"It\"s"`, `'a "b"'`) so a quote of the other kind — or +// an escaped one — inside the literal doesn't end the match early and drop the +// exemption. const STRING_LITERAL_VALUE_PATTERN = - /^(?:["'][^"']*["']|`[^`$]*`)\s*(?:\/\/[^\n]*)?\s*(?:[;,})\n]|$)/; + /^(?:"(?:\\.|[^"\\\n])*"|'(?:\\.|[^'\\\n])*'|`[^`$]*`)\s*(?:\/\/[^\n]*)?\s*(?:[;,})\n]|$)/; const MODULE_CONSTANT_VALUE_PATTERN = /^[A-Z][A-Z0-9_]*\s*(?:\/\/[^\n]*)?\s*(?:[;,})\n]|$)/; @@ -201,6 +204,55 @@ const isInertParseTarget = (target: string, fileContent: string): boolean => { return scratchReadPattern.test(fileContent); }; +// Split a value expression on top-level `+` operators, ignoring `+` inside +// parentheses, brackets, braces, or string/template literals. +const splitTopLevelByPlus = (text: string): string[] => { + const parts: string[] = []; + let current = ""; + let depth = 0; + let openQuote: string | null = null; + for (let index = 0; index < text.length; index += 1) { + const character = text[index]; + if (openQuote !== null) { + current += character; + if (character === openQuote && text[index - 1] !== "\\") openQuote = null; + continue; + } + if (character === '"' || character === "'" || character === "`") { + openQuote = character; + current += character; + continue; + } + if (character === "(" || character === "[" || character === "{") depth += 1; + else if (character === ")" || character === "]" || character === "}") depth -= 1; + if (character === "+" && depth === 0 && text[index - 1] !== "+" && text[index + 1] !== "+") { + parts.push(current); + current = ""; + continue; + } + current += character; + } + parts.push(current); + return parts; +}; + +// `a.innerHTML = (icon as SVGElement).outerHTML + (text as HTMLSpanElement).outerHTML` +// re-serializes already-rendered DOM on BOTH sides of the concat — no fresh +// input is spliced in — so it is no more dangerous than a single DOM read. +const isAllOperandsDomContentConcat = (valueExpression: string): boolean => { + const body = valueExpression.replace(/[;}]\s*$/, "").trim(); + if (!body.includes("+")) return false; + const operands = splitTopLevelByPlus(body) + .map((operand) => operand.trim()) + .filter((operand) => operand.length > 0); + if (operands.length < 2) return false; + return operands.every((operand) => { + const withoutCast = operand.replace(/\(\s*([\w$]+(?:\??\.[\w$]+)*)\s+as\s+[^)]*\)/g, "$1"); + if (!DOM_CONTENT_SOURCE_VALUE_PATTERN.test(withoutCast)) return false; + return !HTML_TAINT_PATTERN.test(withoutCast.replace(DOM_CONTENT_SOURCE_VALUE_PATTERN, "")); + }); +}; + export const dangerousHtmlSink = defineRule({ id: "dangerous-html-sink", title: "HTML injection sink with dynamic content", @@ -253,6 +305,7 @@ export const dangerousHtmlSink = defineRule({ const afterDomRead = valueExpression.replace(DOM_CONTENT_SOURCE_VALUE_PATTERN, ""); if (!HTML_TAINT_PATTERN.test(afterDomRead)) continue; } + if (isAllOperandsDomContentConcat(valueExpression)) continue; const longValueTail = HTML_VALUE_START_PATTERN.exec( lines.slice(lineIndex, lineIndex + 1 + STATIC_TEMPLATE_LOOKAHEAD_LINES).join("\n"), diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-client-owned-authz-field.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-client-owned-authz-field.regressions.test.ts index 62b935e834..fcc17e51b6 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-client-owned-authz-field.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-client-owned-authz-field.regressions.test.ts @@ -18,4 +18,33 @@ describe("security-scan/firebase-client-owned-authz-field — regressions", () = }); expect(findings).toHaveLength(0); }); + + // FP wave 4: a benign write (`{ displayName }`) followed by an UNRELATED + // later statement that merely reads `.role` must not fire — the authz + // field must live inside the write call's own statement. + it("stays silent when .role belongs to a separate later statement", () => { + const findings = runScanRule(firebaseClientOwnedAuthzField, { + relativePath: "src/components/profile.tsx", + content: `import { setDoc, doc } from "firebase/firestore";\nexport const saveName = (uid, name) => setDoc(doc(db, "users", uid), { displayName: name });\nexport const useRole = () => useContext(AuthContext).role;\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("still flags a role field inside the write call's own object", () => { + const findings = runScanRule(firebaseClientOwnedAuthzField, { + relativePath: "src/components/profile.tsx", + content: `import { setDoc, doc } from "firebase/firestore";\nexport const save = (uid) => setDoc(doc(db, "users", uid), { displayName: "x", role: "admin" });\n`, + }); + expect(findings.length).toBeGreaterThan(0); + }); + + // FN wave 5: a `;` inside a string literal within the write's own args must + // not truncate the statement window before the authz field. + it("flags a role field after a semicolon inside a string in the write's own args", () => { + const findings = runScanRule(firebaseClientOwnedAuthzField, { + relativePath: "src/components/profile.tsx", + content: `import { setDoc, doc } from "firebase/firestore";\nexport const save = (uid) => setDoc(doc(db, "users", uid), { note: "a;b", role: "admin" });\n`, + }); + expect(findings.length).toBeGreaterThan(0); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-client-owned-authz-field.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-client-owned-authz-field.ts index ecc2fea0eb..c795aa4d6f 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-client-owned-authz-field.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-client-owned-authz-field.ts @@ -19,8 +19,14 @@ export const firebaseClientOwnedAuthzField = defineRule({ isClientSourcePath(file.relativePath) && (CLIENT_DATABASE_EVIDENCE_PATTERN.test(file.content) || CLIENT_DATABASE_EVIDENCE_PATTERN.test(file.relativePath)), + // The trailing window is quoted-string-aware: it cannot cross a bare `;` + // statement boundary into an UNRELATED later statement — the authz field + // must appear inside the write call's own statement — but a `;` INSIDE a + // string literal in the write's own args (`{ note: "a;b", role: … }`) + // does not truncate it. `setDoc(doc(db, "users", uid), { displayName })` + // followed by a separate `useRole = ….role` still does not fire. pattern: - /(?:\b(?:setDoc|updateDoc|addDoc)\s*\(|(?:\b(?:firebase|firestore|getFirestore)\b|\bcollection\s*\(|\.collection\s*\()[\s\S]{0,500}\.(?:set|update|add)\s*\()[\s\S]{0,700}\b(?:ownerId|ownerID|creatorId|creatorID|providerId|providerID|orgId|orgID|tenantId|tenantID|workspaceId|workspaceID|ghostOrg|role|roles|isAdmin)\b/i, + /(?:\b(?:setDoc|updateDoc|addDoc)\s*\(|(?:\b(?:firebase|firestore|getFirestore)\b|\bcollection\s*\(|\.collection\s*\()[\s\S]{0,500}\.(?:set|update|add)\s*\()(?:[^;'"`]|'[^'\n]*'|"[^"\n]*"|`[^`]*`){0,700}\b(?:ownerId|ownerID|creatorId|creatorID|providerId|providerID|orgId|orgID|tenantId|tenantID|workspaceId|workspaceID|ghostOrg|role|roles|isAdmin)\b/i, message: "Client code writes an ownership, tenant, or role field that should be server-owned and immutable.", }), diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-permissive-rules.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-permissive-rules.regressions.test.ts new file mode 100644 index 0000000000..010de79f66 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/firebase-permissive-rules.regressions.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runScanRule } from "../../../test-utils/run-scan-rule.js"; +import { firebasePermissiveRules } from "./firebase-permissive-rules.js"; + +describe("security-scan/firebase-permissive-rules — regressions", () => { + // FP wave 4: a cautionary commented-out `allow … if true` is never + // executed; comments must be stripped from `.rules` files before scanning. + it("stays silent when the permissive rule is inside a comment", () => { + const findings = runScanRule(firebasePermissiveRules, { + relativePath: "firestore.rules", + content: `// counter-example, never do this: allow read, write: if true;\nmatch /users/{uid} {\n allow read, write: if request.auth.uid == uid;\n}`, + }); + expect(findings).toHaveLength(0); + }); + + it("still flags an uncommented permissive rule", () => { + const findings = runScanRule(firebasePermissiveRules, { + relativePath: "firestore.rules", + content: `match /users/{uid} {\n allow read, write: if true;\n}`, + }); + expect(findings.length).toBeGreaterThan(0); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/insecure-crypto-risk.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/insecure-crypto-risk.regressions.test.ts index c77f91c1a1..76606749a5 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/insecure-crypto-risk.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/insecure-crypto-risk.regressions.test.ts @@ -36,6 +36,14 @@ describe("security-scan/insecure-crypto-risk — regressions", () => { expect(findings).toHaveLength(0); }); + it("stays silent on weak-crypto mentions that live only in comments", () => { + const findings = runScanRule(insecureCryptoRisk, { + relativePath: "src/server/auth.ts", + content: `// TODO: stop hashing the password with md5(value)\n/* legacy DES cipher removed in v2 — see encrypt.ts */\nexport const hashPassword = (password: string) => argon2.hash(password);\n`, + }); + expect(findings).toHaveLength(0); + }); + it("flags md5 hashing of password material", () => { const findings = runScanRule(insecureCryptoRisk, { relativePath: "src/server/auth.ts", diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/insecure-crypto-risk.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/insecure-crypto-risk.ts index dc6ad70e42..9ee4203aab 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/insecure-crypto-risk.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/insecure-crypto-risk.ts @@ -2,6 +2,7 @@ import { DEMO_CONTEXT_PATTERN } from "../../constants/security-scan.js"; import { defineRule } from "../../utils/define-rule.js"; import type { ScanFinding } from "../../utils/file-scan.js"; import { getLocationAtIndex } from "./utils/get-location-at-index.js"; +import { getScannableContent } from "./utils/scan-by-pattern.js"; import { isProductionSourcePath } from "./utils/is-production-source-path.js"; const WEAK_HASH_PATTERN = /createHash\s*\(\s*["'](?:md5|sha1)["']|\bmd5\s*\(/gi; @@ -22,8 +23,12 @@ const WEAK_CIPHER_NAME_PATTERN = /\b(?:DES|RC4|Blowfish)\b/; const CIPHER_CONTEXT_PATTERN = /\b(?:cipher|decipher|encrypt|decrypt|crypto)\b/i; +// `{0,100}` (not `*`) before the `signature` literal: the unbounded run is +// O(n²) over any long identifier-shaped blob (a hex constant, a low-entropy +// data URI), which measurably hangs the scan on large generated files — +// real identifiers never approach 100 chars. const UNSAFE_SIGNATURE_COMPARISON_PATTERN = - /[A-Za-z_$][\w$.]*signature[\w$]*(?:\([^)]*\))?\s*(?:===?|!==?)\s*[A-Za-z_$][\w$.]*(?:\([^)]*\))?|[A-Za-z_$][\w$.]*(?:\([^)]*\))?\s*(?:===?|!==?)\s*[A-Za-z_$][\w$.]*signature[\w$]*(?:\([^)]*\))?/i; + /[A-Za-z_$][\w$.]{0,100}signature[\w$]*(?:\([^)]*\))?\s*(?:===?|!==?)\s*[A-Za-z_$][\w$.]*(?:\([^)]*\))?|[A-Za-z_$][\w$.]{0,100}(?:\([^)]*\))?\s*(?:===?|!==?)\s*[A-Za-z_$][\w$.]{0,100}signature[\w$]*(?:\([^)]*\))?/i; // `signature !== PluginSignatureStatus.valid` compares enum/status members and // `signatureMethod === SIGNATURE_METHOD_RSA_SHA1` compares against a module @@ -120,23 +125,28 @@ export const insecureCryptoRisk = defineRule({ // not within the 250-char window around the hash call. if (PROTOCOL_MANDATED_HASH_CONTEXT_PATTERN.test(file.relativePath)) return []; + // Match against comment-stripped content (positions preserved) — migration + // notes and doc comments are exactly where `md5` / `DES` prose concentrates, + // and a `// TODO: stop hashing the password with md5(value)` must not fire. + const content = getScannableContent(file); + let matchIndex = findMatchIndexNearContext( - file.content, + content, WEAK_HASH_PATTERN, SECURITY_CONTEXT_PATTERN, PROTOCOL_MANDATED_HASH_CONTEXT_PATTERN, ); - if (matchIndex < 0) matchIndex = file.content.search(WEAK_CIPHER_ALGORITHM_PATTERN); - if (matchIndex < 0) matchIndex = file.content.search(DEPRECATED_CIPHER_API_PATTERN); - if (matchIndex < 0 && CIPHER_CONTEXT_PATTERN.test(file.content)) { - matchIndex = file.content.search(WEAK_CIPHER_NAME_PATTERN); + if (matchIndex < 0) matchIndex = content.search(WEAK_CIPHER_ALGORITHM_PATTERN); + if (matchIndex < 0) matchIndex = content.search(DEPRECATED_CIPHER_API_PATTERN); + if (matchIndex < 0 && CIPHER_CONTEXT_PATTERN.test(content)) { + matchIndex = content.search(WEAK_CIPHER_NAME_PATTERN); } if ( matchIndex < 0 && - !TIMING_SAFE_COMPARISON_PATTERN.test(file.content) && + !TIMING_SAFE_COMPARISON_PATTERN.test(content) && !CLIENT_COMPONENT_FILE_PATTERN.test(file.relativePath) ) { - const comparisonMatch = UNSAFE_SIGNATURE_COMPARISON_PATTERN.exec(file.content); + const comparisonMatch = UNSAFE_SIGNATURE_COMPARISON_PATTERN.exec(content); if ( comparisonMatch !== null && !ENUM_MEMBER_COMPARAND_PATTERN.test(comparisonMatch[0]) && @@ -148,7 +158,7 @@ export const insecureCryptoRisk = defineRule({ } if (matchIndex < 0) { matchIndex = findRandomCallIndexWithSameLineContext( - file.content, + content, MATH_RANDOM_CALL_PATTERN, SECURITY_RANDOM_CONTEXT_PATTERN, UI_NONCE_CONTEXT_PATTERN, @@ -156,7 +166,7 @@ export const insecureCryptoRisk = defineRule({ } if (matchIndex < 0) return []; - const location = getLocationAtIndex(file.content, matchIndex); + const location = getLocationAtIndex(content, matchIndex); const finding: ScanFinding = { message: "Code uses weak hashes, deprecated ciphers, timing-unsafe comparisons, or Math.random in a security-shaped context.", diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/package-metadata-secret.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/package-metadata-secret.regressions.test.ts new file mode 100644 index 0000000000..1b6afba5d6 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/package-metadata-secret.regressions.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runScanRule } from "../../../test-utils/run-scan-rule.js"; +import { packageMetadataSecret } from "./package-metadata-secret.js"; + +const SUPABASE_SERVICE_ROLE_JWT = + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJzdXBhYmFzZSIsInJvbGUiOiJzZXJ2aWNlX3JvbGUiLCJpYXQiOjE2NDF9.M2NzY4NDU3NjQ1Njc4OTAxMjM0NTY3ODkwMTIzNDU2Nzg5MDEy"; + +describe("security-scan/package-metadata-secret — regressions", () => { + // FP wave 4: the bare word `service_role` (a Supabase role name) in a + // helper package's metadata is not a leaked secret value. + it("stays silent on the word service_role in package metadata", () => { + const findings = runScanRule(packageMetadataSecret, { + relativePath: "package.json", + content: `{"name":"supabase-service-role-helpers","description":"Utilities for the Supabase service_role key on the server.","keywords":["supabase","service_role","rls"]}`, + }); + expect(findings).toHaveLength(0); + }); + + it("still flags a real high-entropy secret value in package metadata", () => { + const findings = runScanRule(packageMetadataSecret, { + relativePath: "package.json", + content: `{"name":"x","config":{"db":"postgres://dbuser:r3alL0ngPwd0rdValue@db.prod.example.com/app"}}`, + }); + expect(findings.length).toBeGreaterThan(0); + }); + + // FN wave (PR #993): dropping the bare `service_role` keyword must not + // drop the *credential* — a service_role JWT value is caught by + // JWT_LITERAL_VALUE_PATTERN. + it("flags a leaked service_role JWT credential in package.json config", () => { + const findings = runScanRule(packageMetadataSecret, { + relativePath: "package.json", + content: `{"name":"x","config":{"service_role":"${SUPABASE_SERVICE_ROLE_JWT}"}}`, + }); + expect(findings.length).toBeGreaterThan(0); + }); + + it("flags SUPABASE_SERVICE_ROLE_KEY name with JWT value in package.json", () => { + const findings = runScanRule(packageMetadataSecret, { + relativePath: "package.json", + content: `{"name":"x","config":{"SUPABASE_SERVICE_ROLE_KEY":"${SUPABASE_SERVICE_ROLE_JWT}"}}`, + }); + expect(findings.length).toBeGreaterThan(0); + }); + + it("still fires on sb_secret_ style new-format supabase secret", () => { + const findings = runScanRule(packageMetadataSecret, { + relativePath: "package.json", + content: `{"name":"x","config":{"key":"sb_secret_abcdefghijklmnopqrstuvwx"}}`, + }); + expect(findings.length).toBeGreaterThan(0); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/package-metadata-secret.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/package-metadata-secret.ts index 89c728e66c..14ad4dee76 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/package-metadata-secret.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/package-metadata-secret.ts @@ -1,8 +1,20 @@ -import { SECRET_VALUE_PATTERNS } from "../../constants/security.js"; +import { JWT_LITERAL_VALUE_PATTERN, SECRET_VALUE_PATTERNS } from "../../constants/security.js"; import { defineRule } from "../../utils/define-rule.js"; import { findSuspiciousPublicEnvSecretNamePattern } from "./utils/find-suspicious-public-env-secret-name.js"; import { getMatchLocation } from "./utils/get-match-location.js"; +// The bare keyword `service_role` (a Supabase role *name*) is a legitimate +// word in a helper package's `description`/`keywords` — not a leaked secret. +// A genuine service_role *credential* is a JWT (`eyJ…`) or `sb_secret_…`; +// `sb_secret_` is caught by SECRET_VALUE_PATTERNS, and the JWT value is +// caught by JWT_LITERAL_VALUE_PATTERN below (scoped to package metadata — +// unlike source files, where anon-key JWTs are legitimate, ANY JWT literal +// committed into package.json is a leak). +const PACKAGE_METADATA_VALUE_PATTERNS = [ + ...SECRET_VALUE_PATTERNS.filter((pattern) => !pattern.test("service_role")), + JWT_LITERAL_VALUE_PATTERN, +]; + export const packageMetadataSecret = defineRule({ id: "package-metadata-secret", title: "Secret-like package metadata", @@ -13,7 +25,7 @@ export const packageMetadataSecret = defineRule({ if (!file.relativePath.endsWith("package.json")) return []; const pattern = findSuspiciousPublicEnvSecretNamePattern(file.content) ?? - SECRET_VALUE_PATTERNS.find((candidate) => candidate.test(file.content)); + PACKAGE_METADATA_VALUE_PATTERNS.find((candidate) => candidate.test(file.content)); if (pattern === undefined) return []; const location = getMatchLocation(file.content, pattern); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/path-traversal-risk.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/path-traversal-risk.regressions.test.ts index 4bace9bed9..7f8b9f362c 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/path-traversal-risk.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/path-traversal-risk.regressions.test.ts @@ -26,4 +26,39 @@ describe("security-scan/path-traversal-risk — regressions", () => { }); expect(findings).toHaveLength(0); }); + + it("stays silent when request input is sanitized through path.basename()", () => { + const findings = runScanRule(pathTraversalRisk, { + relativePath: "src/server/files.ts", + content: `const p = path.join(UPLOAD_DIR, path.basename(req.params.file));\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("still flags request input joined without a sanitizer", () => { + const findings = runScanRule(pathTraversalRisk, { + relativePath: "src/server/files.ts", + content: `const p = path.join(UPLOAD_DIR, req.params.file);\n`, + }); + expect(findings).toHaveLength(1); + }); + + // FP wave 4: a static literal path segment whose filename happens to be + // spelled like a taint accessor (`public/body.html`, `${dir}/query.sql`) + // is preceded by `/` or a backtick — never a real request read. + it("stays silent on a static path segment after a slash", () => { + const findings = runScanRule(pathTraversalRisk, { + relativePath: "src/server/handler.ts", + content: `fs.readFileSync(path.join(__dirname, "public/body.html"));\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent on a template literal suffix after a slash", () => { + const findings = runScanRule(pathTraversalRisk, { + relativePath: "src/server/handler.ts", + content: "readFile(`${dir}/query.sql`);\n", + }); + expect(findings).toHaveLength(0); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/path-traversal-risk.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/path-traversal-risk.ts index 7d53e332dc..fd3c0d0376 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/path-traversal-risk.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/path-traversal-risk.ts @@ -3,10 +3,16 @@ import { isDevToolingPath } from "./utils/is-dev-tooling-path.js"; import { isProductionSourcePath } from "./utils/is-production-source-path.js"; import { scanByPattern } from "./utils/scan-by-pattern.js"; -// `(? { }); expect(findings).toHaveLength(0); }); + + it("stays silent when event.data is bound to a local before the origin guard returns", () => { + const findings = runScanRule(postmessageOriginRisk, { + relativePath: "src/widget.ts", + content: `window.addEventListener("message", (event) => {\n const data = event.data;\n if (event.origin !== window.location.origin) return;\n handleCommand(data);\n});\n`, + }); + expect(findings).toHaveLength(0); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/postmessage-origin-risk.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/postmessage-origin-risk.ts index 2eaa93d794..26c08555c8 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/postmessage-origin-risk.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/postmessage-origin-risk.ts @@ -16,6 +16,13 @@ const POSTMESSAGE_ORIGIN_CHECK_PATTERN = /origin(?!al)|\.source\s*[!=]==?/i; const MESSAGE_DATA_READ_PATTERN = /\b(?:event|e|evt|msg|message)\.data\b/; +// True when the text immediately before the first `event.data` is a +// `const`/`let`/`var` declaration's initializer — i.e. the data is being READ +// INTO A LOCAL (`const data = event.data`) rather than used directly. The +// local isn't consumed until later, so an origin guard placed anywhere after +// the binding (the read-then-guard-then-use idiom) still protects the use. +const MESSAGE_DATA_BINDING_PATTERN = /\b(?:const|let|var)\s+[^;=]*=\s*$/; + // MessagePort/Worker/BroadcastChannel/EventSource/WebSocket message events // are same-application or server-stream channels; window-origin checks // neither exist nor apply there. `self.onmessage` is the worker-global @@ -82,7 +89,13 @@ export const postmessageOriginRisk = defineRule({ const messageDataIndex = nodeText.search(MESSAGE_DATA_READ_PATTERN); if (messageDataIndex < 0) return; const originCheckIndex = nodeText.search(POSTMESSAGE_ORIGIN_CHECK_PATTERN); - if (originCheckIndex >= 0 && originCheckIndex < messageDataIndex) return; + if (originCheckIndex >= 0) { + // When the data is bound to a local first, the guard protects the + // later use regardless of textual order (read-then-guard-then-use). + // When the data is used directly, the guard must precede that use. + if (MESSAGE_DATA_BINDING_PATTERN.test(nodeText.slice(0, messageDataIndex))) return; + if (originCheckIndex < messageDataIndex) return; + } const location = getLocationAtIndex(file.content, getNodeStartIndex(node)); findings.push({ diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/raw-sql-injection-risk.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/raw-sql-injection-risk.regressions.test.ts index 00089b022b..545769e869 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/raw-sql-injection-risk.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/raw-sql-injection-risk.regressions.test.ts @@ -61,4 +61,120 @@ describe("security-scan/raw-sql-injection-risk — regressions", () => { }); expect(findings).toHaveLength(0); }); + + it("stays silent when concat output is wrapped in connection.escape()", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'connection.query("SELECT * FROM users WHERE id = " + connection.escape(id));\n', + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent when concat output is wrapped in connection.escapeId()", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'connection.query("SELECT * FROM t ORDER BY " + connection.escapeId(col));\n', + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent when concat output is wrapped in SqlString.escape()", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'pool.query("SELECT * FROM t WHERE name = " + SqlString.escape(name));\n', + }); + expect(findings).toHaveLength(0); + }); + + it("flags concat with raw req.body input", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'connection.query("SELECT * FROM users WHERE id = " + req.body.id);\n', + }); + expect(findings).toHaveLength(1); + }); + + it("flags concat with a bare variable", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'connection.query("SELECT * FROM users WHERE id = " + userId);\n', + }); + expect(findings).toHaveLength(1); + }); + + it("flags concat wrapped in escapeHtml (HTML escaping is not SQL-safe)", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'connection.query("SELECT * FROM users WHERE name = " + utils.escapeHtml(v));\n', + }); + expect(findings).toHaveLength(1); + }); + + it("flags a raw tainted operand after an escaped first operand", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: + 'connection.query("SELECT * FROM users WHERE id = " + connection.escape(id) + " AND role = " + req.body.role);\n', + }); + expect(findings).toHaveLength(1); + }); + + it("flags concat wrapped in lodash _.escape (HTML escape, not SQL-safe)", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: + 'connection.query("SELECT * FROM users WHERE name = \'" + _.escape(req.query.name) + "\'");\n', + }); + expect(findings).toHaveLength(1); + }); + + it("flags concat wrapped in validator.escape (HTML escape, not SQL-safe)", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: + 'connection.query("SELECT * FROM users WHERE name = \'" + validator.escape(req.query.name) + "\'");\n', + }); + expect(findings).toHaveLength(1); + }); + + it("stays silent when concat output is wrapped in client.escapeLiteral()", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'client.query("SELECT * FROM users WHERE name = " + client.escapeLiteral(name));\n', + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent when concat output is wrapped in client.escapeIdentifier()", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'client.query("SELECT * FROM t ORDER BY " + client.escapeIdentifier(col));\n', + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent with a newline between + and the escaper (multi-line concat)", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: + 'connection.query(\n "SELECT * FROM users WHERE id = " +\n connection.escape(id)\n);\n', + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent when the escaped operand is parenthesized", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'connection.query("SELECT * FROM users WHERE id = " + (connection.escape(id)));\n', + }); + expect(findings).toHaveLength(0); + }); + + it("flags escape used as a property access (not a call)", () => { + const findings = runScanRule(rawSqlInjectionRisk, { + relativePath: "src/server/users.ts", + content: 'connection.query("SELECT * FROM users WHERE id = " + obj.escape);\n', + }); + expect(findings).toHaveLength(1); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/raw-sql-injection-risk.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/raw-sql-injection-risk.ts index d5f11a4b9a..189320b3e4 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/raw-sql-injection-risk.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/raw-sql-injection-risk.ts @@ -2,6 +2,23 @@ import { defineRule } from "../../utils/define-rule.js"; import { isProductionScriptSourcePath } from "./utils/is-production-script-source-path.js"; import { scanByPattern } from "./utils/scan-by-pattern.js"; +const SQL_STRING_LITERAL_OPERAND = /(?:"[^"\n]{0,200}"|'[^'\n]{0,200}'|`[^`$]{0,200}`)/.source; +// SQL-safe escaper output: `.escapeId()`/`.escapeLiteral()`/`.escapeIdentifier()` +// (mysqljs/sqlstring + node-postgres) on any receiver, or bare `.escape()` only +// when the receiver's last segment is SQL-shaped — so HTML escapers like +// `_.escape()`, `validator.escape()`, and `utils.escapeHtml()` are NOT safe. +const SQL_ESCAPER_CALL = + /(?:[\w$]+(?:\.[\w$]+)*\.escape(?:Id|Literal|Identifier)|(?:[\w$]+\.)*(?:connection|conn|client|pool|db|mysql|sqlstring|knex)\.escape)\s*\([^()]{0,200}\)/ + .source; +const SAFE_SQL_CONCAT_OPERAND = `\\s*\\(?\\s*(?:${SQL_STRING_LITERAL_OPERAND}|${SQL_ESCAPER_CALL})\\s*\\)?`; +// Walk every `+`-joined operand after the leading literal and fire on the +// first unsafe one, so a single escaped operand cannot launder a later raw +// concatenation in the same query. +const QUERY_CONCAT_WITH_UNSAFE_OPERAND = new RegExp( + `\\.query\\s*\\(\\s*${SQL_STRING_LITERAL_OPERAND}(?:\\s*\\+${SAFE_SQL_CONCAT_OPERAND})*\\s*\\+(?!${SAFE_SQL_CONCAT_OPERAND})`, + "i", +); + // `Prisma.raw("AND ")` (pure literal) and `whereRaw("col = {p: String}", {p})` // (driver-side binding) are parameterized usage, not string-built SQL — the // escape hatch only matters when the argument is dynamic. The `${` check @@ -12,7 +29,7 @@ const RAW_SQL_RISK_PATTERNS = [ /\bPrisma\.raw\s*\((?!\s*(?:["'][^"'\n]*["']\s*[,)]|`[^`$]*`))/, /\bsql\.\s*(?:raw|unsafe)\s*\((?!\s*(?:["'][^"'\n]*["']\s*[,)]|`[^`$]*`))/, /\b(?:client|pool|conn)\.query\s*\(\s*['"`]\s*(?:SELECT|INSERT|UPDATE|DELETE)\b[^)]{0,400}\$\{(?!\s*[\w$.]*(?:sanitiz|escape|quote)[\w$]*\s*\()/i, - /\.query\s*\(\s*['"`][^'"`]{0,200}['"`]\s*\+/, + QUERY_CONCAT_WITH_UNSAFE_OPERAND, /\.(?:where|orderBy|having)Raw\s*\((?!\s*(?:["'][^"'\n]*["']\s*[,)]|`[^`$]*`))/, /\bcursor\.execute\s*\(\s*f['"]/, /\bcursor\.execute\s*\(\s*(?:"[^"]{0,400}"|'[^']{0,400}')\s*(?:%|\.format\s*\(|\+)/, diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/secret-in-fallback.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/secret-in-fallback.regressions.test.ts new file mode 100644 index 0000000000..b9ecf861a2 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/secret-in-fallback.regressions.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runScanRule } from "../../../test-utils/run-scan-rule.js"; +import { secretInFallback } from "./secret-in-fallback.js"; + +describe("security-scan/secret-in-fallback — regressions", () => { + it("stays silent on NEXT_PUBLIC_* tokens (public-by-design, inlined into the bundle)", () => { + const findings = runScanRule(secretInFallback, { + relativePath: "src/lib/map.ts", + content: `const token = process.env.NEXT_PUBLIC_MAPBOX_TOKEN ?? "pk.eyJ1IjoiZXhhbXBsZSJ9";\nconst key = process.env.NEXT_PUBLIC_API_KEY ?? "abcdef123456";\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent on a *_PUBLISHABLE_KEY mid-name keyword", () => { + const findings = runScanRule(secretInFallback, { + relativePath: "src/lib/stripe.ts", + content: `const k = process.env.STRIPE_PUBLISHABLE_KEY ?? "pk_test_abcdef123456";\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("still flags a genuine secret env var with a hardcoded fallback", () => { + const findings = runScanRule(secretInFallback, { + relativePath: "src/lib/stripe.ts", + content: `const k = process.env.STRIPE_SECRET_KEY ?? "sk_live_abcdef123456";\n`, + }); + expect(findings).toHaveLength(1); + }); + + // FP wave 4: a purely numeric default is a duration/size config, never a + // credential — even when the env name carries `TOKEN`/`SECRET`. + it("stays silent on a numeric duration fallback", () => { + const findings = runScanRule(secretInFallback, { + relativePath: "src/config.ts", + content: `const t = process.env.SESSION_TOKEN_TIMEOUT ?? "18000000";\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("still flags a non-numeric secret fallback", () => { + const findings = runScanRule(secretInFallback, { + relativePath: "src/config.ts", + content: `const k = process.env.STRIPE_SECRET_KEY ?? "sk_live_realsecretvalue12345";\n`, + }); + expect(findings.length).toBeGreaterThan(0); + }); + + // Bugbot: a PUBLIC segment mid-name is not public-by-design — only a + // leading framework public prefix or a publishable/anon key convention is. + it("flags a mid-name PUBLIC segment with a secret fallback", () => { + const findings = runScanRule(secretInFallback, { + relativePath: "src/lib/webhooks.ts", + content: `const s = process.env.INTERNAL_PUBLIC_WEBHOOK_SECRET ?? "whsec_realvalue123456";\n`, + }); + expect(findings).toHaveLength(1); + }); + + it("flags a public-prefixed name that still ends in _SECRET", () => { + const findings = runScanRule(secretInFallback, { + relativePath: "src/lib/webhooks.ts", + content: `const s = process.env.NEXT_PUBLIC_WEBHOOK_SECRET ?? "whsec_abc123def456";\n`, + }); + expect(findings).toHaveLength(1); + }); + + it("stays silent on a NEXT_PUBLIC_* URL fallback", () => { + const findings = runScanRule(secretInFallback, { + relativePath: "src/lib/api.ts", + content: `const u = process.env.NEXT_PUBLIC_API_URL ?? "https://api.example.com";\n`, + }); + expect(findings).toHaveLength(0); + }); + + it("stays silent on a SUPABASE_ANON_KEY fallback (client-safe by convention)", () => { + const findings = runScanRule(secretInFallback, { + relativePath: "src/lib/supabase.ts", + content: `const k = process.env.SUPABASE_ANON_KEY ?? "eyJhbGciOiJIUzI1NiJ9.anonpayload";\n`, + }); + expect(findings).toHaveLength(0); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/secret-in-fallback.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/secret-in-fallback.ts index dfb48916b9..f5bcd7b5e9 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/secret-in-fallback.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/secret-in-fallback.ts @@ -5,15 +5,25 @@ import { scanByPattern } from "./utils/scan-by-pattern.js"; // A secret-shaped env var with a hardcoded string fallback // (`process.env.STRIPE_SECRET_KEY ?? ""`). Two bugs at once: the // literal is a committed secret, and the app silently uses it (fails open) -// when the env var is unset. The env-name lookahead skips intentionally-public -// vars (PUBLIC/PUBLISHABLE/ANON). The trailing negative lookbehind skips names -// that only REFERENCE a secret rather than hold one — `_HEADER`/`_NAME`/`_ID`/ +// when the env var is unset. The env-name lookahead skips only names that are +// public BY CONSTRUCTION: a leading framework public prefix (`NEXT_PUBLIC_`/ +// `EXPO_PUBLIC_`/`GATSBY_PUBLIC_`/`NUXT_PUBLIC_`/`REACT_APP_PUBLIC_`/ +// `VITE_PUBLIC_`/bare `PUBLIC_`) or a publishable/anon key naming convention +// (`…PUBLISHABLE_KEY`/`…ANON_KEY`). A PUBLIC segment elsewhere in the name +// (`INTERNAL_PUBLIC_WEBHOOK_SECRET`) does not make the value public, and a +// name ending in `_SECRET`/`_PRIVATE_KEY`/`_PASSWORD`/`_PASSWD` is never +// exempt even under a public prefix — that fallback is a committed secret +// either way. The trailing negative lookbehind skips names that only +// REFERENCE a secret rather than hold one — `_HEADER`/`_NAME`/`_ID`/ // `_ENDPOINT`/`_URL`/… suffixes (e.g. `AUTH_TOKEN_HEADER`, `AWS_ACCESS_KEY_ID`, // `TOKEN_ENDPOINT`), whose values are header names, key ids, or URLs, not // secrets. The value lookahead skips placeholder defaults and URL values so // only substantive secret literals flag. +// `(?![0-9]+["'`])` skips a purely numeric default — a duration/size config +// like `SESSION_TOKEN_TIMEOUT ?? "18000000"` (a millisecond count) is never a +// credential, even though the name carries `TOKEN`. const HARDCODED_SECRET_FALLBACK_PATTERN = - /\bprocess\.env\.(?![A-Z0-9_]*(?:PUBLIC|PUBLISHABLE|ANON)\b)[A-Z][A-Z0-9_]*(?:SECRET|TOKEN|PASSWORD|PASSWD|PRIVATE_KEY|API_?KEY|APIKEY|ACCESS_KEY|CLIENT_SECRET|CREDENTIAL|SIGNING_KEY|ENCRYPTION_KEY|WEBHOOK_SECRET|SERVICE_ROLE)[A-Z0-9_]*(? { + // FP wave 4: a decorative SVG filter applied to a sibling AFTER the iframe + // never targets the iframe, so it stays exempt. Ancestor filters BEFORE the + // iframe are the actual clickjacking primitive and still fire. + it("stays silent when the filter styles a sibling element after the iframe", () => { + const findings = runScanRule(svgFilterClickjackingRisk, { + relativePath: "src/embed.tsx", + content: `const A = () => (<>