-
Notifications
You must be signed in to change notification settings - Fork 133
fix: redact non-SQL content echoed in safety threat messages (main sanity failure) #1111
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
f34af2b
c9a1597
7b39204
5b9b6ae
389ff1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -59,4 +59,67 @@ export function piiColumnsFromReport(piiData: unknown): Array<Record<string, any | |||||
| return columns.filter((c) => c && c.classification !== "None") | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Redact raw input echoed inside engine threat messages. | ||||||
| * | ||||||
| * The engine's `multi_statement` rule quotes the offending "statement type" | ||||||
| * verbatim — for non-SQL input (e.g. `altimate check /etc/passwd`) that | ||||||
| * reflects arbitrary file content back into CLI/tool output. A token is kept | ||||||
| * only when it BOTH looks like a short keyword phrase AND starts with a known | ||||||
| * SQL statement keyword — shape alone would pass content like | ||||||
| * "TOP SECRET PASSWORD" or "AKIAIOSFODNN7EXAMPLE". | ||||||
| */ | ||||||
| const REDACTED = "<non-SQL content redacted>" | ||||||
| const SQL_TOKEN_SHAPE = /^[A-Za-z_][A-Za-z0-9_$ ]{0,31}$/ | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Shape regex is looser than the exact-keyword rule needs With exact single-keyword matching, the
Suggested change
Reply with |
||||||
| const SQL_STATEMENT_KEYWORDS = new Set([ | ||||||
| "SELECT", "INSERT", "UPDATE", "DELETE", "DROP", "CREATE", "ALTER", "TRUNCATE", | ||||||
| "GRANT", "REVOKE", "MERGE", "WITH", "BEGIN", "COMMIT", "ROLLBACK", "SET", | ||||||
| "USE", "CALL", "EXPLAIN", "ANALYZE", "VACUUM", "COPY", "SHOW", "DESCRIBE", | ||||||
| "DESC", "EXECUTE", "EXEC", "REPLACE", "VALUES", "LOCK", "UNLOCK", "COMMENT", | ||||||
| "RENAME", "START", "SAVEPOINT", "RELEASE", "PREPARE", "DEALLOCATE", | ||||||
| ]) | ||||||
| // The engine emits SINGLE-WORD statement types only — probed against the | ||||||
| // live 0.7.0 binary across DDL/DML/DCL/TCL second statements, the full | ||||||
| // observed set is {ALTER, BEGIN, CALL, CREATE, DELETE, DROP, EXPLAIN, GRANT, | ||||||
| // INSERT, SET, TRUNCATE, UPDATE}. Exact single-keyword matching is therefore | ||||||
| // both engine-faithful and the strictest rule: multi-word input can only be | ||||||
| // non-engine content (kills keyword-permutation leaks like | ||||||
| // "SELECT DELETE UPDATE" without redacting anything the engine produces). | ||||||
| function isSqlStatementType(token: string): boolean { | ||||||
| const t = token.trim() | ||||||
| if (!SQL_TOKEN_SHAPE.test(t)) return false | ||||||
| return SQL_STATEMENT_KEYWORDS.has(t.toUpperCase()) | ||||||
| } | ||||||
| export function redactThreatText(text: string): string { | ||||||
| return ( | ||||||
| text | ||||||
| .replace(/(Disallowed statement type: )(.+)$/i, (_m, prefix: string, tok: string) => | ||||||
| isSqlStatementType(tok) ? `${prefix}${tok}` : `${prefix}${REDACTED}`, | ||||||
| ) | ||||||
| // GREEDY quoted match — an attacker apostrophe inside the token would | ||||||
| // otherwise close the match early and leak the remainder | ||||||
| // (e.g. "Statement type 'DROP'ROOT:X:0' is not…"). Greedy capture spans | ||||||
| // to the LAST quote; embedded quotes fail the shape check and redact. | ||||||
| .replace(/(Statement type ')(.*)(')/i, (_m, a: string, tok: string, c: string) => | ||||||
| isSqlStatementType(tok) ? `${a}${tok}${c}` : `${a}${REDACTED}${c}`, | ||||||
| ) | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Redact raw-input echoes from a scan result's threats. For `multi_statement` | ||||||
| * the matched pattern IS the raw input line, so it is always redacted; | ||||||
| * injection rules keep their SQL-shaped patterns. Pure record manipulation — | ||||||
| * safe to use from any consumer without loading the NAPI binding. | ||||||
| */ | ||||||
| export function redactScan(scan: Record<string, unknown>): Record<string, unknown> { | ||||||
| const threats = (scan.threats as any[] | undefined)?.map((t: any) => ({ | ||||||
| ...t, | ||||||
| message: typeof t.message === "string" ? redactThreatText(t.message) : t.message, | ||||||
| detail: typeof t.detail === "string" ? redactThreatText(t.detail) : t.detail, | ||||||
| matched_pattern: t.rule === "multi_statement" ? "<redacted>" : t.matched_pattern, | ||||||
| })) | ||||||
| return threats ? { ...scan, threats } : scan | ||||||
| } | ||||||
|
|
||||||
| export * as EngineCoerce from "./engine-coerce" | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,160 @@ | ||||||
| import { describe, expect, test, beforeAll, afterAll } from "bun:test" | ||||||
| import { EngineCoerce } from "../../src/altimate/native/engine-coerce" | ||||||
|
|
||||||
| let coreAvailable = false | ||||||
| try { | ||||||
| require.resolve("@altimateai/altimate-core") | ||||||
| coreAvailable = true | ||||||
| } catch {} | ||||||
| const describeIf = coreAvailable ? describe : describe.skip | ||||||
|
|
||||||
| describe("redactThreatText", () => { | ||||||
| test("keeps single SQL-statement-keyword types (the only shape the engine emits)", () => { | ||||||
| expect(EngineCoerce.redactThreatText("Disallowed statement type: DROP")).toBe("Disallowed statement type: DROP") | ||||||
| expect(EngineCoerce.redactThreatText("Statement type 'ALTER' is not in the allowed list")).toBe( | ||||||
| "Statement type 'ALTER' is not in the allowed list", | ||||||
| ) | ||||||
| }) | ||||||
|
|
||||||
| test("redacts non-SQL content (e.g. /etc/passwd lines)", () => { | ||||||
| const msg = "Disallowed statement type: ROOT:X:0:0:ROOT:/ROOT:/BIN/BASH" | ||||||
| expect(EngineCoerce.redactThreatText(msg)).toBe("Disallowed statement type: <non-SQL content redacted>") | ||||||
| const detail = "Statement type 'ROOT:X:0:0:ROOT:/ROOT:/BIN/BASH' is not in the allowed list: [\"SELECT\"]" | ||||||
| expect(EngineCoerce.redactThreatText(detail)).toContain("'<non-SQL content redacted>'") | ||||||
| expect(EngineCoerce.redactThreatText(detail)).not.toContain("ROOT:X:0") | ||||||
| }) | ||||||
|
|
||||||
| test("redacts keyword-shaped but non-SQL content (allowlist, not shape)", () => { | ||||||
| // Shape alone would pass these — the first word must be a SQL statement keyword. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Stale comment describes the removed first-word rule The rule is no longer "the first word must be a SQL statement keyword" —
Suggested change
Reply with |
||||||
| expect(EngineCoerce.redactThreatText("Disallowed statement type: TOP SECRET PASSWORD")).toBe( | ||||||
| "Disallowed statement type: <non-SQL content redacted>", | ||||||
| ) | ||||||
| expect(EngineCoerce.redactThreatText("Disallowed statement type: AKIAIOSFODNN7EXAMPLE")).toBe( | ||||||
| "Disallowed statement type: <non-SQL content redacted>", | ||||||
| ) | ||||||
| expect(EngineCoerce.redactThreatText("Statement type 'PRIVATE NOTE' is not in the allowed list")).toBe( | ||||||
| "Statement type '<non-SQL content redacted>' is not in the allowed list", | ||||||
| ) | ||||||
| // ANY multi-word phrase redacts — the engine only emits single-word | ||||||
| // statement types (live-probed), so multi-word input is never engine | ||||||
| // vocabulary: keyword-prefixed tails and keyword permutations both die. | ||||||
| expect(EngineCoerce.redactThreatText("Disallowed statement type: SET SECRET PASSWORD")).toBe( | ||||||
| "Disallowed statement type: <non-SQL content redacted>", | ||||||
| ) | ||||||
| expect(EngineCoerce.redactThreatText("Disallowed statement type: DROP DEAD FRED")).toBe( | ||||||
| "Disallowed statement type: <non-SQL content redacted>", | ||||||
| ) | ||||||
| expect(EngineCoerce.redactThreatText("Disallowed statement type: SELECT DELETE UPDATE")).toBe( | ||||||
| "Disallowed statement type: <non-SQL content redacted>", | ||||||
| ) | ||||||
| }) | ||||||
|
|
||||||
| test("embedded apostrophes cannot close the quoted match early", () => { | ||||||
| // Greedy capture spans to the LAST quote; the embedded quote fails the | ||||||
| // shape check and everything inside is redacted. | ||||||
| const attack = "Statement type 'DROP'ROOT:X:0:0' is not in the allowed list" | ||||||
| const out = EngineCoerce.redactThreatText(attack) | ||||||
| expect(out).not.toContain("ROOT:X:0") | ||||||
| expect(out).toContain("<non-SQL content redacted>") | ||||||
| }) | ||||||
|
|
||||||
| test("leaves unrelated messages untouched", () => { | ||||||
| expect(EngineCoerce.redactThreatText("Tautology attack detected (numeric constant comparison)")).toBe( | ||||||
| "Tautology attack detected (numeric constant comparison)", | ||||||
| ) | ||||||
| }) | ||||||
| }) | ||||||
|
|
||||||
| // Sanity security test [6/15] regression: `altimate check /etc/passwd` must not | ||||||
| // reflect file contents in threat messages (found by the main-only Verdaccio | ||||||
| // sanity job after PR #1090 surfaced real threat messages). | ||||||
| describeIf("safety consumers redact raw-input echoes (real engine)", () => { | ||||||
| let D: typeof import("../../src/altimate/native/dispatcher") | ||||||
| let priorTelemetry: string | undefined | ||||||
|
|
||||||
| beforeAll(async () => { | ||||||
| priorTelemetry = process.env.ALTIMATE_TELEMETRY_DISABLED | ||||||
| process.env.ALTIMATE_TELEMETRY_DISABLED = "true" | ||||||
|
anandgupta42 marked this conversation as resolved.
|
||||||
| const { registerAll } = await import("../../src/altimate/native/altimate-core") | ||||||
| const { registerAllSql } = await import("../../src/altimate/native/sql/register") | ||||||
| registerAll() | ||||||
| registerAllSql() | ||||||
| D = await import("../../src/altimate/native/dispatcher") | ||||||
| }) | ||||||
|
|
||||||
| afterAll(async () => { | ||||||
| if (priorTelemetry === undefined) delete process.env.ALTIMATE_TELEMETRY_DISABLED | ||||||
| else process.env.ALTIMATE_TELEMETRY_DISABLED = priorTelemetry | ||||||
| // registerAll mutates shared Dispatcher state — reset for test isolation. | ||||||
| ;(await import("../../src/altimate/native/dispatcher")).reset() | ||||||
| }) | ||||||
|
|
||||||
| const PASSWD = "root:x:0:0:root:/root:/bin/bash\ndaemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin\n" | ||||||
|
|
||||||
| test("altimate_core.safety does not echo non-SQL file content", async () => { | ||||||
| const r = await D.call("altimate_core.safety", { sql: PASSWD }) | ||||||
| expect(r.success).toBe(true) | ||||||
| const threats = ((r.data as any).threats ?? []) as any[] | ||||||
| expect(threats.length).toBeGreaterThan(0) | ||||||
| const text = JSON.stringify(threats) | ||||||
| expect(text.toLowerCase()).not.toContain("root:x:0") | ||||||
| expect(text).toContain("redacted") | ||||||
| }) | ||||||
|
|
||||||
| test("composite check does not echo non-SQL file content in threats", async () => { | ||||||
| const r = await D.call("altimate_core.check", { sql: PASSWD }) | ||||||
| expect(r.success).toBe(true) | ||||||
| // The sanitizer must actually be exercised — an empty threat list would | ||||||
| // make the not-contains assertion vacuous. | ||||||
| const threats = (((r.data as any).safety?.threats ?? []) as any[]) | ||||||
| expect(threats.length).toBeGreaterThan(0) | ||||||
| const text = JSON.stringify(threats) | ||||||
| expect(text.toLowerCase()).not.toContain("root:x:0") | ||||||
| expect(text).toContain("redacted") | ||||||
| }) | ||||||
|
|
||||||
| test("sql.analyze does not echo non-SQL file content in issues", async () => { | ||||||
| const r = await D.call("sql.analyze", { sql: PASSWD }) | ||||||
| const safetyIssues = (((r as any).issues ?? []) as any[]).filter((i) => i.type === "safety") | ||||||
| // The sanitizer must actually be exercised — no safety issues would make | ||||||
| // the not-contains assertion vacuous. | ||||||
| expect(safetyIssues.length).toBeGreaterThan(0) | ||||||
| const text = JSON.stringify(safetyIssues) | ||||||
| expect(text.toLowerCase()).not.toContain("root:x:0") | ||||||
| expect(text).toContain("redacted") | ||||||
| }) | ||||||
|
|
||||||
| test("altimate_core.grade result does not echo non-SQL file content", async () => { | ||||||
| // EvalResult's safety section is { method, score } — no threat scan to | ||||||
| // echo (the handler's redactScan is defense-in-depth if that changes). | ||||||
| // Assert the whole result is echo-free, minus the `sql` field, which by | ||||||
| // contract carries back the caller's own input. | ||||||
| const r = await D.call("altimate_core.grade", { sql: PASSWD }) | ||||||
| expect(r.success).toBe(true) | ||||||
| // `sql` (EvalResult) and `lint.sql` (LintResult) carry back the caller's | ||||||
| // own input by contract and are never rendered by the CLI grade check. | ||||||
| const { sql: _echoedInput, ...rest } = r.data as any | ||||||
| if (rest.lint) rest.lint = { ...rest.lint, sql: undefined } | ||||||
| expect(JSON.stringify(rest).toLowerCase()).not.toContain("root:x:0") | ||||||
| }) | ||||||
|
anandgupta42 marked this conversation as resolved.
|
||||||
|
|
||||||
| test("diff-scoping still filters pre-existing multi_statement threats despite redaction", async () => { | ||||||
| // Regression (cursor review on #1111): redacting BEFORE base subtraction | ||||||
| // rewrote head matched_patterns to "<redacted>" so base keys never matched | ||||||
| // and pre-existing threats resurfaced as introduced. Redaction now runs | ||||||
| // after subtraction: identical base/head must yield zero threats. | ||||||
| const r = await D.call("altimate_core.check", { sql: PASSWD, base_sql: PASSWD }) | ||||||
| expect((((r.data as any).safety?.threats ?? []) as any[]).length).toBe(0) | ||||||
| expect((r.data as any).safety.safe).toBe(true) | ||||||
| }) | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||
|
|
||||||
| test("real SQL statement types are preserved in messages", async () => { | ||||||
| const r = await D.call("altimate_core.safety", { sql: "SELECT 1; DROP TABLE users;" }) | ||||||
| const threats = ((r.data as any).threats ?? []) as any[] | ||||||
| const messages = threats.map((t) => `${t.message} ${t.detail}`).join(" ") | ||||||
| // SQL-keyword statement types stay useful in messages/details | ||||||
| // (matched_pattern is always redacted for multi_statement by design). | ||||||
| expect(messages).toContain("DROP") | ||||||
| expect(messages).not.toContain("redacted") | ||||||
| }) | ||||||
| }) | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: Doc comment still describes the superseded keyword-prefix rule
isSqlStatementTypenow keeps a token only when it is exactly one allowlisted statement keyword, but this docstring still says a token is kept when it "starts with a known SQL statement keyword" and calls it a "keyword phrase" — implyingALTER TABLEorSET FOOsurvive redaction. Update the wording to the exact single-keyword contract so future readers don't rely on the weaker rule the last two commits removed.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.