From baccfd9c2fb57cfbbe74d34899d1c634cb8b7e02 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Thu, 15 Oct 2026 16:41:09 +0700 Subject: [PATCH 1/8] feat(contracts): share the kit proposal forbidden-key policy Add skillx.kit-proposal-policy/v1 with the forbidden-key lists, key normalization, the refusing content rules and the local path system allowlist, plus shared vectors that SkillX and AgentKit both test against. Refs #84. --- .../src/fixtures/kit-proposal-policy-v1.json | 116 +++ .../kit-proposal-policy-vectors-v1.json | 710 ++++++++++++++++++ packages/contracts/src/personal-kit/index.ts | 4 +- .../src/personal-kit/proposal-policy.test.ts | 60 ++ .../src/personal-kit/proposal-policy.ts | 87 +++ 5 files changed, 976 insertions(+), 1 deletion(-) create mode 100644 packages/contracts/src/fixtures/kit-proposal-policy-v1.json create mode 100644 packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json create mode 100644 packages/contracts/src/personal-kit/proposal-policy.test.ts create mode 100644 packages/contracts/src/personal-kit/proposal-policy.ts diff --git a/packages/contracts/src/fixtures/kit-proposal-policy-v1.json b/packages/contracts/src/fixtures/kit-proposal-policy-v1.json new file mode 100644 index 0000000..7dae5a5 --- /dev/null +++ b/packages/contracts/src/fixtures/kit-proposal-policy-v1.json @@ -0,0 +1,116 @@ +{ + "schema": "skillx.kit-proposal-policy/v1", + "envelope": "skillx.kit-proposal/v1", + "forbiddenKeys": { + "description": "A key anywhere in a proposal is normalized, then refused when it equals an exact name or contains a stem. A key longer than maxKeyLength (Unicode code points of the key as received) or not printable ASCII after normalization cannot be checked and is refused too. A key whose value is a number is exempt (an evidence signal such as sessions: 3 cannot carry session content). String values outside the file-content fields that parse as a JSON object or array are checked as nested documents.", + "normalization": [ + "percent-decode %XX escapes until nothing changes (at most 3 rounds), each escape to the code unit of its byte", + "Unicode NFKC", + "lowercase", + "refuse when any character is outside printable ASCII (U+0020 to U+007E)", + "remove every character outside [a-z0-9]" + ], + "maxKeyLength": 64, + "exact": [ + "chat", + "chats", + "cwd", + "dotenv", + "env", + "log", + "logs", + "message", + "messages", + "pwd", + "raw", + "session", + "sessions" + ], + "contains": [ + "accesskey", + "apikey", + "conversation", + "credential", + "environment", + "envvar", + "history", + "homedir", + "passphrase", + "passwd", + "password", + "privatekey", + "prompt", + "secret", + "sessionid", + "sessionlog", + "stderr", + "stdout", + "token", + "toolcall", + "tooloutput", + "toolresult", + "transcript", + "workdir" + ], + "contentFields": [ + "contentBase64", + "patch" + ], + "refusal": { + "code": "proposal.forbidden_field", + "reports": "the key path only, never the value" + } + }, + "content": { + "description": "What a change introduces (added lines of a changed file, a new file whole, the evidence summary) is refused on any rule below; findings are danger:: and never carry the matched text. Regular expressions are implementation detail; the vectors are the contract.", + "refusing": [ + "binary-content", + "byte-order-mark", + "local-path", + "secret-bearer", + "secret-dotenv", + "secret-jwt", + "secret-password", + "secret-private-key", + "secret-url-credentials" + ], + "localPathSystemAllowlist": { + "roots": [ + "bin", + "sbin", + "etc", + "proc" + ], + "usr": [ + "bin", + "include", + "lib", + "lib32", + "lib64", + "libexec", + "sbin", + "share", + "src" + ], + "usrLocal": [ + "bin", + "cellar", + "etc", + "include", + "lib", + "lib64", + "libexec", + "man", + "opt", + "sbin", + "share" + ], + "dev": [ + "null", + "stderr", + "stdin", + "stdout" + ] + } + } +} diff --git a/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json b/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json new file mode 100644 index 0000000..d98c290 --- /dev/null +++ b/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json @@ -0,0 +1,710 @@ +{ + "schema": "skillx.kit-proposal-policy-vectors/v1", + "policy": "skillx.kit-proposal-policy/v1", + "description": "Shared test vectors. keys: run the forbidden-key check over value (the root path is proposal); refusedAt is the first refused key path in document order, or null. secrets.parts and multilinePasswords.*.parts are joined with no separator before use.", + "keys": [ + { + "name": "a benign envelope with every known field", + "value": { + "schema": "skillx.kit-proposal/v1", + "fromRevisionId": "pkr_00000000-0000-4000-8000-000000000000", + "skill": { + "identity": "@alpha/demo", + "base": { + "package": "@alpha/demo", + "version": "1.0.0", + "releaseDigest": "sha256:0000000000000000000000000000000000000000000000000000000000000000" + }, + "overlay": { + "schema": "agentkit.skill-overlay/v1", + "baseTreeDigest": "sha256:0000000000000000000000000000000000000000000000000000000000000000", + "ops": [ + { + "op": "patch", + "path": "SKILL.md", + "expectSha256": "0000000000000000000000000000000000000000000000000000000000000000", + "patch": "{\"prompt\": \"file content is scanned by the content rules\"}" + }, + { + "op": "add", + "path": "notes.md", + "contentBase64": "bm90ZXMK", + "mode": "100644" + } + ] + }, + "compositionDigest": "sha256:0000000000000000000000000000000000000000000000000000000000000000" + }, + "declaredPermissions": [], + "evidence": { + "summary": "prefer small steps", + "signals": { + "sessions": 3, + "tokens": 1200, + "tool_calls": 4 + } + }, + "visibility": "private" + }, + "refusedAt": null + }, + { + "name": "top-level transcript", + "value": { + "transcript": "x" + }, + "refusedAt": "proposal.transcript" + }, + { + "name": "upper case", + "value": { + "TRANSCRIPT": "x" + }, + "refusedAt": "proposal.TRANSCRIPT" + }, + { + "name": "camel case under skill", + "value": { + "skill": { + "systemPrompt": "x" + } + }, + "refusedAt": "proposal.skill.systemPrompt" + }, + { + "name": "kebab case", + "value": { + "session-log": "x" + }, + "refusedAt": "proposal.session-log" + }, + { + "name": "snake case", + "value": { + "api_key": "x" + }, + "refusedAt": "proposal.api_key" + }, + { + "name": "dotted", + "value": { + "api.key": "x" + }, + "refusedAt": "proposal.api.key" + }, + { + "name": "spaced title case", + "value": { + "Tool Calls": [] + }, + "refusedAt": "proposal.Tool Calls" + }, + { + "name": "stem inside a longer key", + "value": { + "rawTranscript": "x" + }, + "refusedAt": "proposal.rawTranscript" + }, + { + "name": "stem with a suffix", + "value": { + "sessionLogs": [] + }, + "refusedAt": "proposal.sessionLogs" + }, + { + "name": "vendor secret name", + "value": { + "AWS_SECRET_ACCESS_KEY": "x" + }, + "refusedAt": "proposal.AWS_SECRET_ACCESS_KEY" + }, + { + "name": "password stem", + "value": { + "dbPassword": "x" + }, + "refusedAt": "proposal.dbPassword" + }, + { + "name": "history stem", + "value": { + "conversationHistory": [] + }, + "refusedAt": "proposal.conversationHistory" + }, + { + "name": "exact env", + "value": { + "evidence": { + "env": { + "HOME": "x" + } + } + }, + "refusedAt": "proposal.evidence.env" + }, + { + "name": "exact cwd in an array", + "value": { + "evidence": { + "items": [ + { + "ok": 1 + }, + { + "cwd": "x" + } + ] + } + }, + "refusedAt": "proposal.evidence.items[1].cwd" + }, + { + "name": "deeply nested", + "value": { + "a": { + "b": { + "c": { + "d": { + "e": { + "messages": [] + } + } + } + } + } + }, + "refusedAt": "proposal.a.b.c.d.e.messages" + }, + { + "name": "percent-encoded", + "value": { + "%74ranscript": "x" + }, + "refusedAt": "proposal.%74ranscript" + }, + { + "name": "double percent-encoded", + "value": { + "%2574oken": "x" + }, + "refusedAt": "proposal.%2574oken" + }, + { + "name": "fullwidth letters (NFKC)", + "value": { + "ｔｒａｎｓｃｒｉｐｔ": "x" + }, + "refusedAt": "proposal.ｔｒａｎｓｃｒｉｐｔ" + }, + { + "name": "zero-width character", + "value": { + "pro​mpt": "x" + }, + "refusedAt": "proposal.pro​mpt" + }, + { + "name": "Cyrillic homoglyph", + "value": { + "рrompt": "x" + }, + "refusedAt": "proposal.рrompt" + }, + { + "name": "any non-ASCII key", + "value": { + "café": "x" + }, + "refusedAt": "proposal.café" + }, + { + "name": "key longer than the limit", + "value": { + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa": "x" + }, + "refusedAt": "proposal.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + }, + { + "name": "key at the limit", + "value": { + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa": "x" + }, + "refusedAt": null + }, + { + "name": "numeric value is exempt", + "value": { + "evidence": { + "signals": { + "sessions": 3, + "prompt_count": 2 + } + } + }, + "refusedAt": null + }, + { + "name": "string value is not exempt", + "value": { + "evidence": { + "signals": { + "sessions": "3" + } + } + }, + "refusedAt": "proposal.evidence.signals.sessions" + }, + { + "name": "boolean value is not exempt", + "value": { + "env": true + }, + "refusedAt": "proposal.env" + }, + { + "name": "null value is not exempt", + "value": { + "secret": null + }, + "refusedAt": "proposal.secret" + }, + { + "name": "embedded JSON object in a string", + "value": { + "evidence": { + "summary": "{\"transcript\": \"x\"}" + } + }, + "refusedAt": "proposal.evidence.summary#json.transcript" + }, + { + "name": "embedded JSON array in a string", + "value": { + "evidence": { + "summary": " [{\"chat\": []}]" + } + }, + "refusedAt": "proposal.evidence.summary#json[0].chat" + }, + { + "name": "embedded JSON with an escaped key", + "value": { + "evidence": { + "summary": "{\"\\u0074oken\": \"x\"}" + } + }, + "refusedAt": "proposal.evidence.summary#json.token" + }, + { + "name": "text that is not JSON", + "value": { + "evidence": { + "summary": "{not json" + } + }, + "refusedAt": null + }, + { + "name": "file content fields are not parsed", + "value": { + "ops": [ + { + "patch": "{\"transcript\": \"x\"}", + "contentBase64": "[]" + } + ] + }, + "refusedAt": null + }, + { + "name": "first refusal in document order", + "value": { + "a": { + "prompt": "x" + }, + "b": { + "secret": "y" + } + }, + "refusedAt": "proposal.a.prompt" + } + ], + "localPaths": { + "refuse": [ + "\"cwd\": \"C:\\\\Users\\\\kai\\\\repo\"", + "D:\\www\\project\\notes.md", + "d:/data/file", + "share \\\\srv01\\public\\x", + "\"\\\\\\\\srv01\\\\share\\\\x\"", + "file:///home/kai/x", + "file://host/share", + "/mnt/c/Users/kai", + ">/home/kai/log", + "/dev/null", + "read /dev/stdin, write /dev/stdout and /dev/stderr", + "/USR/BIN/env", + "https://example.com/var/www/x?y=%2Fz", + "dates 12/31/2025 and 1/2", + "run /ak:plan now", + "-I/usr/include", + "-L/usr/local/lib", + "/usr/lib/node_modules/x", + "/usr/share/doc/x", + "/usr/local", + "/usr/local/opt/node", + "https://example.com/usr/home/x", + "vscode://ms-python.python", + "git+https://github.com/a/b", + "ssh://git@github.com/org/repo", + "foo-bar/-draft/x", + "a -b", + "a/b//c", + "////////////////////////////////////////", + "postgres://db.internal/app" + ] + }, + "secrets": [ + { + "parts": [ + "password=", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "db_passwd: ", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "\"password\": \"", + "hunter", + "22", + "\"" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "Authorization: Bearer ", + "a1a1a1a1a1a1a1a1a1a1a1a1" + ], + "rules": [ + "secret-bearer" + ] + }, + { + "parts": [ + "postgres://admin:", + "s3cretpw", + "@db.internal/x" + ], + "rules": [ + "secret-url-credentials" + ] + }, + { + "parts": [ + "x-postgres://admin:", + "s3cretpw", + "@db.internal/x" + ], + "rules": [ + "secret-url-credentials" + ] + }, + { + "parts": [ + "token ", + "eyJ", + "hbGciOiJIUzI1NiJ9", + ".eyJ", + "zdWIiOiIxIn0", + ".c2ln" + ], + "rules": [ + "secret-jwt" + ] + }, + { + "parts": [ + "auth-", + "eyJ", + "hbGciOiJIUzI1NiJ9", + ".eyJ", + "zdWIiOiIxIn0", + ".c2ln" + ], + "rules": [ + "secret-jwt" + ] + }, + { + "parts": [ + "-----BEGIN ", + "OPENSSH PRIVATE KEY-----" + ], + "rules": [ + "secret-private-key" + ] + }, + { + "parts": [ + "OPENAI_API_KEY=", + "x9x9x9x9x9x9x9x9x9x9" + ], + "rules": [ + "secret-dotenv" + ] + }, + { + "parts": [ + "export DB_PASSWORD=", + "qqqqqqqqqqqq" + ], + "rules": [ + "secret-dotenv", + "secret-password" + ] + }, + { + "parts": [ + "password: " + ], + "rules": [] + }, + { + "parts": [ + "password=${PASSWORD}" + ], + "rules": [] + }, + { + "parts": [ + "Bearer " + ], + "rules": [] + }, + { + "parts": [ + "https://example.com/a@b" + ], + "rules": [] + }, + { + "parts": [ + "API_KEY=" + ], + "rules": [] + }, + { + "parts": [ + "the password field" + ], + "rules": [] + }, + { + "parts": [ + "AUTHOR_NAME=Jonathan" + ], + "rules": [] + } + ], + "multilinePasswords": { + "refuse": [ + { + "parts": [ + "db:\n password:\n ", + "hunter", + "22", + "\n" + ] + }, + { + "parts": [ + "password: |\n ", + "hunter", + "22", + "\n" + ] + }, + { + "parts": [ + "password: >-\n ", + "hunter", + "22", + "\n more\n" + ] + }, + { + "parts": [ + "PASSWORD:\n\n ", + "hunter", + "22", + "\n" + ] + }, + { + "parts": [ + "users:\n - password:\n ", + "hunter", + "22", + "\n" + ] + }, + { + "parts": [ + "users:\n - name: a\n password:\n ", + "hunter", + "22", + "\n" + ] + }, + { + "parts": [ + "\"password\":\n ", + "hunter", + "22", + "\n" + ] + }, + { + "parts": [ + "'db_password': |\n ", + "hunter", + "22", + "\n" + ] + }, + { + "parts": [ + " - \"password\":\n ", + "hunter", + "22", + "\n" + ] + } + ], + "allow": [ + { + "parts": [ + "password: |\n \n" + ] + }, + { + "parts": [ + "password:\n ${DB_PASSWORD}\n" + ] + }, + { + "parts": [ + "password:\nnext: 1\n" + ] + }, + { + "parts": [ + "password: |\n ab\n" + ] + }, + { + "parts": [ + "- password:\n- next\n" + ] + } + ] + } +} diff --git a/packages/contracts/src/personal-kit/index.ts b/packages/contracts/src/personal-kit/index.ts index 1d2885b..00ddd7a 100644 --- a/packages/contracts/src/personal-kit/index.ts +++ b/packages/contracts/src/personal-kit/index.ts @@ -12,4 +12,6 @@ export type { TreeFacts } from "./frontmatter-facts"; export { overlayBetween, rebaseOverlay } from "./rebase"; export type { RebaseFinding, RebaseResult } from "./rebase"; export * from "./kit-document"; -export * from "./kit-lock"; \ No newline at end of file +export * from "./kit-lock"; +export * from "./proposal-policy"; + diff --git a/packages/contracts/src/personal-kit/proposal-policy.test.ts b/packages/contracts/src/personal-kit/proposal-policy.test.ts new file mode 100644 index 0000000..f050275 --- /dev/null +++ b/packages/contracts/src/personal-kit/proposal-policy.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; +import vectors from "../fixtures/kit-proposal-policy-vectors-v1.json"; +import { KIT_PROPOSAL_POLICY, KIT_PROPOSAL_POLICY_SCHEMA, findForbiddenProposalKey, isForbiddenProposalKey, normalizeProposalKey } from "./proposal-policy"; + +/** + * The shared `skillx.kit-proposal-policy/v1` rules and their vectors, the + * contract AgentKit tests against. The content vectors (paths, secrets) run + * against the web detectors in `apps/web/tests/personal-kits`. + */ +const sortedUnique = (list: string[]) => [...new Set(list)].sort(); + +describe("kit proposal policy file", () => { + it("names its schema and the envelope it governs", () => { + expect(KIT_PROPOSAL_POLICY.schema).toBe(KIT_PROPOSAL_POLICY_SCHEMA); + expect(KIT_PROPOSAL_POLICY.envelope).toBe("skillx.kit-proposal/v1"); + expect(vectors.policy).toBe(KIT_PROPOSAL_POLICY_SCHEMA); + }); + + it("keeps every list sorted, unique and already normalized", () => { + const { exact, contains } = KIT_PROPOSAL_POLICY.forbiddenKeys; + for (const list of [exact, contains, KIT_PROPOSAL_POLICY.content.refusing]) expect(list).toEqual(sortedUnique(list)); + for (const name of [...exact, ...contains]) expect(normalizeProposalKey(name)).toBe(name); + }); + + it("never refuses a field of the envelope itself", () => { + const fields = ["schema", "fromRevisionId", "skill", "identity", "base", "package", "version", "releaseDigest", "overlay", "baseTreeDigest", "ops", "op", "path", "contentBase64", "mode", "expectSha256", "patch", "compositionDigest", "declaredPermissions", "evidence", "summary", "signals", "visibility"]; + expect(fields.filter(isForbiddenProposalKey)).toEqual([]); + }); +}); + +describe("forbidden key vectors", () => { + it.each(vectors.keys.map((v) => [v.name, v] as const))("%s", (_name, v) => { + const found = findForbiddenProposalKey(v.value, 32); + expect(found === null ? null : found.path).toBe(v.refusedAt); + if (found !== null) expect(found.reason).toBe("forbidden"); + }); + + it("normalizes case, separators, encodings and NFKC forms to one name", () => { + for (const key of ["systemPrompt", "SYSTEM_PROMPT", "system-prompt", "system.prompt", "System Prompt", "%73ystemPrompt", "%2573ystemPrompt", "ｓｙｓｔｅｍＰｒｏｍｐｔ"]) { + expect(normalizeProposalKey(key)).toBe("systemprompt"); + } + expect(normalizeProposalKey("pro​mpt")).toBeNull(); + expect(normalizeProposalKey("%E2%80%8Bprompt")).toBeNull(); + expect(normalizeProposalKey("x".repeat(65))).toBeNull(); + // The length counts code points, so 64 astral characters are within the limit (and refused as non-ASCII). + expect([..."😀".repeat(64)].length).toBe(64); + expect(normalizeProposalKey("😀".repeat(64))).toBeNull(); + }); + + it("bounds the nesting, also through embedded JSON", () => { + let deep: unknown = {}; + for (let i = 0; i < 40; i++) deep = { a: deep }; + expect(findForbiddenProposalKey(deep, 32)).toMatchObject({ reason: "too_deep" }); + let text = "{}"; + // Each level is an object holding the previous level as a JSON string (escaping doubles per level). + for (let i = 0; i < 12; i++) text = JSON.stringify({ a: text }); + expect(findForbiddenProposalKey({ summary: text }, 32)).toBeNull(); + expect(findForbiddenProposalKey({ summary: text }, 8)).toMatchObject({ reason: "too_deep" }); + }); +}); diff --git a/packages/contracts/src/personal-kit/proposal-policy.ts b/packages/contracts/src/personal-kit/proposal-policy.ts new file mode 100644 index 0000000..ff596f0 --- /dev/null +++ b/packages/contracts/src/personal-kit/proposal-policy.ts @@ -0,0 +1,87 @@ +/** + * `skillx.kit-proposal-policy/v1`: the forbidden-key rules of a + * `skillx.kit-proposal/v1` envelope, shared with AgentKit, which refuses the + * same keys locally before it uploads a proposal. The single source is + * `fixtures/kit-proposal-policy-v1.json`; both sides test against + * `fixtures/kit-proposal-policy-vectors-v1.json`. This module only applies + * the JSON rules, so a list change is a JSON change. + */ +import policy from "../fixtures/kit-proposal-policy-v1.json"; + +export const KIT_PROPOSAL_POLICY_SCHEMA = "skillx.kit-proposal-policy/v1"; +export const KIT_PROPOSAL_POLICY = policy; + +const EXACT = new Set(policy.forbiddenKeys.exact); +const CONTAINS = policy.forbiddenKeys.contains; +const CONTENT_FIELDS = new Set(policy.forbiddenKeys.contentFields); +const MAX_KEY_LENGTH = policy.forbiddenKeys.maxKeyLength; + +/** Decodes `%XX` escapes (each to the code unit of its byte) until nothing changes, at most 3 rounds. */ +function percentDecode(key: string): string { + let out = key; + for (let i = 0; i < 3; i++) { + const next = out.replace(/%([0-9a-f]{2})/gi, (_, h: string) => String.fromCharCode(parseInt(h, 16))); + if (next === out) break; + out = next; + } + return out; +} + +/** + * The comparable form of a key (`[a-z0-9]*`), or null when the key cannot be + * checked: longer than the limit, or not printable ASCII once decoded and + * NFKC-folded (zero-width characters, homoglyphs, any other script). + */ +export function normalizeProposalKey(key: string): string | null { + if ([...key].length > MAX_KEY_LENGTH) return null; + const folded = percentDecode(key).normalize("NFKC").toLowerCase(); + if (!/^[\x20-\x7e]*$/.test(folded)) return null; + return folded.replace(/[^a-z0-9]/g, ""); +} + +/** True when a key is refused whatever its (non-numeric) value. */ +export function isForbiddenProposalKey(key: string): boolean { + const normal = normalizeProposalKey(key); + return normal === null || EXACT.has(normal) || CONTAINS.some((stem) => normal.includes(stem)); +} + +export type ProposalKeyFinding = { reason: "forbidden"; path: string } | { reason: "too_deep"; path: string }; + +/** Parses a string that holds a JSON object or array, or returns undefined. */ +function embeddedJson(text: string): unknown { + const t = text.trimStart(); + if (!t.startsWith("{") && !t.startsWith("[")) return undefined; + try { + return JSON.parse(t) as unknown; + } catch { + return undefined; + } +} + +/** + * The first refused key of `value` in document order (its path, never its + * value), or the first place it nests deeper than `maxDepth`; null when the + * document passes. A key with a number value is exempt. A string outside the + * file-content fields that holds a JSON object or array is checked as a + * nested document at `#json`. + */ +export function findForbiddenProposalKey(value: unknown, maxDepth: number, at = "proposal", depth = 0): ProposalKeyFinding | null { + if (typeof value === "object" && value !== null && depth >= maxDepth) return { reason: "too_deep", path: at }; + if (Array.isArray(value)) { + for (let i = 0; i < value.length; i++) { + const found = findForbiddenProposalKey(value[i], maxDepth, `${at}[${i}]`, depth + 1); + if (found !== null) return found; + } + return null; + } + if (typeof value !== "object" || value === null) return null; + for (const [key, child] of Object.entries(value)) { + const path = `${at}.${key}`; + if (typeof child === "number") continue; + if (isForbiddenProposalKey(key)) return { reason: "forbidden", path }; + const nested = typeof child === "string" && !CONTENT_FIELDS.has(key) ? embeddedJson(child) : undefined; + const found = nested === undefined ? findForbiddenProposalKey(child, maxDepth, path, depth + 1) : findForbiddenProposalKey(nested, maxDepth, `${path}#json`, depth + 1); + if (found !== null) return found; + } + return null; +} From 2c397d523b7b8b9e0ed3a97a360fc211ce0582b3 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Thu, 15 Oct 2026 16:41:10 +0700 Subject: [PATCH 2/8] fix(kits): harden proposal scanning and inherit the activation gate Make every proposal detector linear in its input, refuse flag-glued paths, editor file links, non-ASCII segments, /usr/home and /usr/local/, find YAML password list items and quoted keys, and require a web session to activate rebase or resolution drafts that carry a session-derived revision forward. Refs #86. --- .../app/lib/personal-kits/kit-lifecycle.ts | 30 +++- .../lib/personal-kits/proposal-detectors.ts | 144 +++++++----------- .../lib/personal-kits/proposal-envelope.ts | 39 +++-- .../personal-kits/proposal-path-detector.ts | 119 +++++++++++++++ .../kit-proposal-activation.test.ts | 93 +++++++++++ .../proposal-detector-cost.test.ts | 57 +++++++ .../personal-kits/proposal-detectors.test.ts | 140 ++++------------- docs/personal-kits-api.md | 51 +++++-- 8 files changed, 439 insertions(+), 234 deletions(-) create mode 100644 apps/web/app/lib/personal-kits/proposal-path-detector.ts create mode 100644 apps/web/tests/personal-kits/kit-proposal-activation.test.ts create mode 100644 apps/web/tests/personal-kits/proposal-detector-cost.test.ts diff --git a/apps/web/app/lib/personal-kits/kit-lifecycle.ts b/apps/web/app/lib/personal-kits/kit-lifecycle.ts index c903d23..e64aa4e 100644 --- a/apps/web/app/lib/personal-kits/kit-lifecycle.ts +++ b/apps/web/app/lib/personal-kits/kit-lifecycle.ts @@ -5,7 +5,8 @@ * moves `effective_revision_id` under the generation guard; nothing becomes * effective implicitly. A proposal (`proposed`) must be reviewed first, and * a session-derived revision activates only on a private kit and only from - * an interactive web session. A quarantined revision anywhere in the chain + * an interactive web session, and so does a rebase or conflict-resolution + * draft that carries one forward. A quarantined revision anywhere in the chain * fails the lock with `kit.locked_incompatible`. * Rejection is final for that revision; a proposal is rejected only through * its web-session review, never through the generic reject. The lock is derived on read from the @@ -62,6 +63,29 @@ function requireApproval(raw: unknown, rev: RevisionRow): void { if (digest !== rev.revision_digest) throw conflict("kit.approval_required", "approval.revisionDigest must name the exact revision being activated"); } +/** + * True for a session-derived revision and for a rebase or conflict-resolution + * revision that carries one forward (through any chain of rebases and + * resolutions): it inherits the web-session activation gate. A manual + * revision is a document its author submits whole, so the chain stops there. + */ +async function derivesFromSession(db: D1Database, kitId: string, rev: RevisionRow): Promise { + if (rev.origin === "session-derived") return true; + if (rev.origin !== "rebase" && rev.origin !== "resolution") return false; + const found = await db + .prepare( + `WITH RECURSIVE chain(id, origin, parent) AS ( + SELECT id, origin, parent_revision_id FROM personal_kit_revisions WHERE id = ? AND kit_id = ? + UNION + SELECT r.id, r.origin, r.parent_revision_id FROM personal_kit_revisions r JOIN chain c ON r.id = c.parent + WHERE r.kit_id = ? AND c.origin IN ('rebase', 'resolution')) + SELECT 1 AS found FROM chain WHERE origin = 'session-derived' LIMIT 1`, + ) + .bind(rev.id, kitId, kitId) + .first<{ found: number }>(); + return found !== null; +} + /** `POST .../activate` and `POST .../rollback` (rollback: a revision that was effective before). */ export async function activateRevision(caller: KitCaller, kitName: string, body: Record, generation: number, rollback: boolean): Promise { const kit = await writableKit(caller.env.DB, kitName, caller.userId); @@ -71,8 +95,8 @@ export async function activateRevision(caller: KitCaller, kitName: string, body: if (rev.state === "proposed") throw conflict("kit.review_required", "a proposed revision must be reviewed before activation"); if (!ACTIVATABLE.has(rev.state)) throw conflict("kit.revision_unusable", `the revision is ${rev.state}`); const sessionDerived = rev.origin === "session-derived"; - if (sessionDerived) { - if (rev.state !== "reviewed") throw conflict("kit.review_required", "a session-derived revision must be reviewed"); + if (sessionDerived && rev.state !== "reviewed") throw conflict("kit.review_required", "a session-derived revision must be reviewed"); + if (await derivesFromSession(caller.env.DB, kit.id, rev)) { requireInteractive(caller); // Activation would make the kit private; a public kit is forked to a private one instead. if (kit.visibility !== "private") throw conflict("proposal.private_kit_required", "a session-derived revision activates on a private kit only; fork this kit to a private kit"); diff --git a/apps/web/app/lib/personal-kits/proposal-detectors.ts b/apps/web/app/lib/personal-kits/proposal-detectors.ts index f5153a3..9251704 100644 --- a/apps/web/app/lib/personal-kits/proposal-detectors.ts +++ b/apps/web/app/lib/personal-kits/proposal-detectors.ts @@ -1,118 +1,90 @@ /** - * Text detectors for session-derived content. Every absolute local path is - * refused, in plain, JSON-escaped (`\/`, `\\`) and percent-encoded (`%2F`, - * `%5C`, `%3A`) form, case-insensitively: a drive-letter path, a UNC share - * (`\\host\share` or `//host/share`), a `file:` or `smb:` URL, a bare user, - * temp, mount or volume root (`/home`, `/Users`, `/tmp`, ...) and any POSIX - * path of two or more segments. Only well-known system paths stay allowed: - * `/usr/`, `/bin/`, `/sbin/`, `/etc/`, `/proc/` and `/dev/null`, `/dev/stdin`, - * `/dev/stdout`, `/dev/stderr`. Web URLs (`https://host/path`) are not paths. - * Generic credentials (password assignments, also a YAML `password:` key - * whose value sits on the next lines, Bearer tokens, URLs with `user:pass@`, - * JWTs, private key blocks and dotenv-style secret keys) complement the - * vendor token rules of the release scanner. Detectors return rule names - * only, never the matched text. + * Text detectors for session-derived content. Absolute local paths are + * refused by `proposal-path-detector`. Generic credentials (password + * assignments, also a YAML `password:` key, list item or quoted key whose + * value sits on the next lines, Bearer tokens, URLs with `user:pass@`, JWTs, + * private key blocks and dotenv-style secret keys) complement the vendor + * token rules of the release scanner. Detectors return rule names only, + * never the matched text. Every pattern runs in time linear in the text + * (bounded scheme names, one start per token or line), so hostile input + * cannot exhaust the Worker CPU. The shared vectors are in + * `@skillx/contracts` (`kit-proposal-policy-vectors-v1.json`). */ +export { hasLocalPath } from "./proposal-path-detector"; -/** Not part of a path or word: what may precede an absolute path. */ -const START = String.raw`(?`, `${...}`, `$VAR`, `%VAR%`, `***`). */ +const VALUE = String.raw`["']?(?![<$%{*])[^\s"'<>]`; +/** `KEY=value` at the start of a line; the key is tested against `SECRET_KEY` on its own. */ +const DOTENV_LINE = new RegExp(String.raw`(?:^|\n)[ \t]*(?:export[ \t]+)?([A-Za-z0-9_]+)[ \t]*=[ \t]*${VALUE}{8,}`, "g"); -const LOCAL_PATHS: ReadonlyArray = [ - // Drive letter: C:\, C:/, C:\\ (JSON-escaped), with or without a following segment. - new RegExp(String.raw`${START}[A-Za-z]:${SEP}`), - // UNC share (\\host\share), also escaped (\\\\host\\share). - new RegExp(String.raw`(? = [ + ["secret-password", new RegExp(String.raw`(?`]*/gi; - -/** Decodes percent-encoded separators and colons (also double-encoded, `%252F`). */ -function decodeSeparators(text: string): string { - let out = text; - for (let i = 0; i < 3; i++) { - const next = out.replace(/%25(2f|5c|3a)/gi, "%$1").replace(/%2f/gi, "/").replace(/%5c/gi, "\\").replace(/%3a/gi, ":"); - if (next === out) break; - out = next; +/** + * A JWT: `eyJ` at a word boundary, at least four base64url characters, a + * dot, `eyJ` and four more, and another dot. Checked per token and part so + * a long run of `eyJ-eyJ-...` is read once. + */ +function hasJwt(text: string): boolean { + for (const [token] of text.matchAll(/[A-Za-z0-9_.-]+/g)) { + if (!token.includes("eyJ")) continue; + const parts = token.split("."); + for (let i = 0; i + 2 < parts.length; i++) { + const start = /(?:^|-)eyJ/.exec(parts[i]!); + const head = start === null ? -1 : start.index + start[0].length; + if (head >= 0 && parts[i]!.length - head >= 4 && parts[i + 1]!.startsWith("eyJ") && parts[i + 1]!.length >= 7) return true; + } } - return out; -} - -function allowedSystemPath(path: string, first: string, second: string | undefined, after: string): boolean { - const root = first.toLowerCase(); - // `/usr/bin/../../home/...` climbs out of the system root. - const whole = path + /^[A-Za-z0-9._~+@$%\\/-]*/.exec(after)![0]; - if (/(?:^|[\\/])\.\.(?:[\\/]|$)/.test(whole)) return false; - if (SYSTEM_ROOTS.has(root)) return true; - // `/dev/null` and the standard streams, but not a path below them. - return root === "dev" && second !== undefined && SYSTEM_DEVICES.has(second.toLowerCase()) && !/^\\*\//.test(after); + return false; } -/** True when `text` names an absolute local path. */ -export function hasLocalPath(text: string): boolean { - const decoded = decodeSeparators(text); - if (LOCAL_PATHS.some((re) => re.test(decoded))) return true; - const withoutUrls = decoded.replace(WEB_URL, " "); - for (const m of withoutUrls.matchAll(POSIX_PATH)) { - if (!allowedSystemPath(m[0], m[1]!, m[2], withoutUrls.slice(m.index! + m[0].length))) return true; - } +function hasDotenvSecret(text: string): boolean { + for (const m of text.matchAll(DOTENV_LINE)) if (SECRET_KEY.test(m[1]!)) return true; return false; } -const SECRET_KEY = "[A-Za-z0-9_]*(?:SECRET|TOKEN|PASSWORD|PASSWD|PWD|API_?KEY|ACCESS_?KEY|PRIVATE_?KEY|CLIENT_?SECRET|CREDENTIALS?)[A-Za-z0-9_]*"; -/** A value that is not an obvious placeholder (`<...>`, `${...}`, `$VAR`, `%VAR%`, `***`). */ -const VALUE = String.raw`["']?(?![<$%{*])[^\s"'<>]`; - -const GENERIC_SECRETS: ReadonlyArray<[rule: string, re: RegExp]> = [ - ["secret-password", new RegExp(String.raw`(? re.test(text)).map(([rule]) => rule).sort(); + const rules = LINE_RULES.filter(([, re]) => re.test(text)).map(([rule]) => rule); + if (hasJwt(text)) rules.push("secret-jwt"); + if (hasDotenvSecret(text)) rules.push("secret-dotenv"); + return rules.sort(); } /** - * A YAML password key whose value sits on the following, more indented lines - * (a plain indented block or a `|` / `>` block scalar). Group 2 is the block. + * A YAML password key (also a list item `- password:` or a quoted key + * `"password":`) whose value sits on the following, more indented lines (a + * plain indented block or a `|` / `>` block scalar). Group 2 is the block. */ const YAML_PASSWORD_BLOCK = new RegExp( - String.raw`(?:^|\n)([ \t]*)[A-Za-z0-9_-]*(?:password|passwd|passphrase|pwd)["']?[ \t]*:[ \t]*(?:[|>][-+0-9]*[ \t]*)?(?:#[^\n]*)?\n((?:[ \t]*\n)*\1[ \t]+\S[^\n]*(?:\n\1[ \t]+[^\n]*|\n[ \t]*(?=\n))*)`, + String.raw`(?:^|\n)([ \t]*)(?:-[ \t]+)*["']?[A-Za-z0-9_-]*(?:password|passwd|passphrase|pwd)["']?[ \t]*:[ \t]*(?:[|>][-+0-9]*[ \t]*)?(?:#[^\n]*)?\n((?:[ \t]*\n)*\1[ \t]+\S[^\n]*(?:\n\1[ \t]+[^\n]*|\n[ \t]*(?=\n))*)`, "gi", ); /** An obvious placeholder value (`<...>`, `${...}`, `$VAR`, `%VAR%`, `***`), optionally quoted. */ const PLACEHOLDER = /^["']?[<$%{*]/; +/** The password blocks of `text` that hold a real value. */ +function passwordBlocks(text: string): string[] { + const blocks: string[] = []; + for (const m of text.matchAll(YAML_PASSWORD_BLOCK)) { + const value = m[2]!.trim(); + if (value.length >= 4 && !PLACEHOLDER.test(value)) blocks.push(m[0].replace(/^\n/, "")); + } + return blocks; +} + /** * True when `after` holds a YAML password block that `before` does not: the * line-based diff alone would miss a new value under a key the base already has. */ export function hasMultilinePassword(before: string | null, after: string): boolean { - for (const m of after.matchAll(YAML_PASSWORD_BLOCK)) { - const value = m[2]!.trim(); - if (value.length < 4 || PLACEHOLDER.test(value)) continue; - if (before === null || !before.includes(m[0].replace(/^\n/, ""))) return true; - } - return false; + const kept = new Set(before === null ? [] : passwordBlocks(before)); + return passwordBlocks(after).some((block) => !kept.has(block)); } /** True for bytes that open with a UTF-8, UTF-16 or UTF-32 byte order mark. */ diff --git a/apps/web/app/lib/personal-kits/proposal-envelope.ts b/apps/web/app/lib/personal-kits/proposal-envelope.ts index 168083c..488d032 100644 --- a/apps/web/app/lib/personal-kits/proposal-envelope.ts +++ b/apps/web/app/lib/personal-kits/proposal-envelope.ts @@ -7,10 +7,14 @@ * * Raw session data is never accepted: a key that names a transcript, * prompt, message list, environment, secret, working directory or history - * anywhere in the proposal is refused with `proposal.forbidden_field` - * naming the key path, never its value. Unknown fields are refused too. + * anywhere in the proposal (in any case, separator or encoding, also inside + * a string that holds JSON) is refused with `proposal.forbidden_field` + * naming the key path, never its value; so is a key that cannot be checked + * (non-ASCII or overlong). The rules are the shared, versioned + * `skillx.kit-proposal-policy/v1` that AgentKit applies before upload. + * Unknown fields are refused too. */ -import { validateOverlay } from "@skillx/contracts"; +import { findForbiddenProposalKey, validateOverlay } from "@skillx/contracts"; import type { KitSkillEntry, SkillOverlay } from "@skillx/contracts"; import { releaseError } from "~/lib/releases/release-http"; import { MAX_JSON_DEPTH, requireId } from "./kit-http"; @@ -25,29 +29,20 @@ export interface KitProposal { evidence: { summary: string; signals: Record } | null; } -const FORBIDDEN_KEYS = new Set([ - "transcript", "transcripts", "session", "sessions", "sessionid", "sessionlog", "prompt", "prompts", "systemprompt", - "messages", "conversation", "chat", "env", "environment", "envvars", "secret", "secrets", "token", "tokens", "apikey", - "credentials", "password", "cwd", "workdir", "homedir", "history", "toolcalls", "tooloutput", "stdout", "stderr", "logs", "raw", -]); - const isRecord = (v: unknown): v is Record => typeof v === "object" && v !== null && !Array.isArray(v); export const proposalError = (code: string, message: string) => releaseError(422, code, message); const invalid = (message: string) => proposalError("proposal.invalid", message); -/** Refuses raw session data by key name at any depth; reports the key path only. */ -export function assertNoForbiddenKeys(value: unknown, at = "proposal", depth = 0): void { - if (typeof value === "object" && value !== null && depth >= MAX_JSON_DEPTH) throw invalid(`${at} nests deeper than ${MAX_JSON_DEPTH} levels`); - if (Array.isArray(value)) return value.forEach((v, i) => assertNoForbiddenKeys(v, `${at}[${i}]`, depth + 1)); - if (!isRecord(value)) return; - for (const [key, child] of Object.entries(value)) { - const path = `${at}.${key}`; - // A number (an evidence signal such as `sessions: 3`) cannot carry session content. - if (typeof child !== "number" && FORBIDDEN_KEYS.has(key.toLowerCase().replace(/[-_]/g, ""))) { - throw proposalError("proposal.forbidden_field", `${path} is not accepted: proposals carry reviewed overlays only, never raw session data`); - } - assertNoForbiddenKeys(child, path, depth + 1); - } +/** + * Refuses raw session data by key name at any depth, with the shared + * `skillx.kit-proposal-policy/v1` rules (see `@skillx/contracts`); reports + * the key path only. + */ +export function assertNoForbiddenKeys(value: unknown): void { + const found = findForbiddenProposalKey(value, MAX_JSON_DEPTH); + if (found === null) return; + if (found.reason === "too_deep") throw invalid(`${found.path} nests deeper than ${MAX_JSON_DEPTH} levels`); + throw proposalError("proposal.forbidden_field", `${found.path} is not accepted: proposals carry reviewed overlays only, never raw session data`); } function only(raw: Record, at: string, fields: string[]): void { diff --git a/apps/web/app/lib/personal-kits/proposal-path-detector.ts b/apps/web/app/lib/personal-kits/proposal-path-detector.ts new file mode 100644 index 0000000..22b6965 --- /dev/null +++ b/apps/web/app/lib/personal-kits/proposal-path-detector.ts @@ -0,0 +1,119 @@ +/** + * Absolute local path detector for session-derived content. Every absolute + * local path is refused, in plain, JSON-escaped (`\/`, `\\`) and + * percent-encoded (`%2F`, `%5C`, `%3A`) form, case-insensitively: a + * drive-letter path, a UNC share (`\\host\share` or `//host/share`), a + * `file:` or `smb:` URL, an editor link to a file (`vscode://file/...`), a + * bare user, temp, mount or volume root (`/home`, `/Users`, `/tmp`, ...), + * a path glued to a compiler flag (`-I/opt/x`) and any POSIX path of two or + * more segments, in any script (`/Usuários/...`). Only well-known system + * paths stay allowed: `/bin/`, `/sbin/`, `/etc/`, `/proc/`, the system + * directories of `/usr/` and `/usr/local/` (not `/usr/home` or + * `/usr/local/`), and `/dev/null`, `/dev/stdin`, `/dev/stdout`, + * `/dev/stderr`. URLs of network schemes (`https://host/path`) are not paths. + * + * Every pattern runs in time linear in the text: separator runs are + * collapsed first, scheme names are bounded and each path run is read once, + * so a long run of `/`, `\` or `a-a-a-...` cannot exhaust the Worker CPU. + */ +import { KIT_PROPOSAL_POLICY } from "@skillx/contracts"; + +/** Not part of a path or word: what may precede an absolute path. */ +const START = String.raw`(? = [ + // Drive letter: C:\, C:/, C:\\ (JSON-escaped), with or without a following segment. + new RegExp(String.raw`${START}[A-Za-z]:${SEP}`, "u"), + // UNC share (\\host\share), also escaped (\\\\host\\share). + new RegExp(String.raw`(?`]*/gi; +/** A short flag glued to a path (`-I/opt/x`, `-isystem/opt/x`), after a word boundary of a command line. */ +const GLUED_FLAG = /(? text.replace(/[\\/]{9,}/g, (run) => run.slice(0, 4) + run.slice(-4)); + +function allowedSystemPath(segments: Array): boolean { + const [root, second, third] = segments.map((s) => s?.toLowerCase()); + if (SYSTEM.roots.has(root!)) return true; + if (root === "usr") { + if (second === undefined || second === "") return true; + if (second !== "local") return SYSTEM.usr.has(second); + return third === undefined || third === "" || SYSTEM.usrLocal.has(third); + } + // `/dev/null` and the standard streams, but not a path below them. + return root === "dev" && second !== undefined && SYSTEM.dev.has(second) && third === undefined; +} + +/** True when `text` names an absolute local path. */ +export function hasLocalPath(text: string): boolean { + const decoded = collapseSeparators(decodeSeparators(text)).replace(GLUED_FLAG, "$1 "); + if (LOCAL_PATHS.some((re) => re.test(decoded))) return true; + const plain = decoded.replace(WEB_URL, " "); + let runEnd = -1; + let lastDotDot = -1; + for (const m of plain.matchAll(POSIX_PATH)) { + const start = m.index!; + if (start >= runEnd) { + // A new path run: read it once and remember its last `..` segment. + PATH_RUN.lastIndex = start; + PATH_RUN.exec(plain); + runEnd = PATH_RUN.lastIndex; + lastDotDot = -1; + for (const d of plain.slice(start, runEnd).matchAll(DOT_DOT)) lastDotDot = start + d.index!; + } + // `/usr/bin/../../home/...` climbs out of the system root. + if (lastDotDot >= start) return true; + SEGMENTS.lastIndex = start; + const segments = SEGMENTS.exec(plain)!; + if (!allowedSystemPath([segments[1], segments[2], segments[3]])) return true; + } + return false; +} diff --git a/apps/web/tests/personal-kits/kit-proposal-activation.test.ts b/apps/web/tests/personal-kits/kit-proposal-activation.test.ts new file mode 100644 index 0000000..1bc3b09 --- /dev/null +++ b/apps/web/tests/personal-kits/kit-proposal-activation.test.ts @@ -0,0 +1,93 @@ +/** + * A rebase or conflict-resolution draft that carries a reviewed + * session-derived revision forward inherits its activation gate: only a + * web session activates it, never the API key that submitted the proposal. + * Drafts that descend from manual revisions keep activating with a key. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { BODY, createKitHarness, skillText } from "./kit-harness"; +import type { KitHarness } from "./kit-harness"; + +const session = vi.hoisted(() => ({ user: null as { id: string } | null })); +vi.mock("~/lib/auth/session-helpers", () => ({ + getSession: async () => (session.user === null ? null : { user: session.user }), +})); + +type Rev = { id: string; digest: string }; +let k: KitHarness; +let manual: Rev; +const WEB = { key: null }; +const MINE = { "SKILL.md": skillText(BODY.replace("two\n", "two (mine)\n")), "notes.md": "my notes\n" }; +const LEARNED = { ...MINE, "notes.md": "my notes\nprefer small steps\n" }; +const v2 = (from: string, to: string) => ({ "SKILL.md": skillText(BODY.replace(from, to)) }); + +beforeEach(async () => { + session.user = { id: "user-a" }; + k = await createKitHarness(); + await k.publish("@alpha/demo", "1.0.0", { "SKILL.md": skillText(BODY) }); + manual = (await k.call("revisions", { expectedGeneration: 0, idempotencyKey: k.idem(), document: k.doc([await k.entry("@alpha/demo", "1.0.0", MINE)]) })).body.revision; +}); + +/** A reviewed session-derived revision of the kit; returns it and the kit generation after the review. */ +async function reviewedProposal(): Promise<{ rev: Rev; generation: number }> { + const e = await k.entry("@alpha/demo", "1.0.0", LEARNED); + const proposal = { schema: "skillx.kit-proposal/v1", fromRevisionId: manual.id, skill: { identity: e.identity, base: e.base, overlay: e.overlay, compositionDigest: e.compositionDigest }, declaredPermissions: [], visibility: "private" }; + const rev = (await k.call("proposals", { expectedGeneration: 1, idempotencyKey: k.idem(), proposal })).body.revision as Rev; + const review = await k.call(`revisions/${rev.id}/review`, { expectedGeneration: 2, decision: "approve", approval: { revisionDigest: rev.digest, permissions: [], visibility: "private", consent: true } }, WEB); + expect(review.status).toBe(200); + return { rev, generation: review.body.generation }; +} + +const rebase = (fromRevisionId: string, version: string, expectedGeneration: number) => + k.call("rebase", { expectedGeneration, idempotencyKey: k.idem(), fromRevisionId, updates: [{ package: "@alpha/demo", version }] }); + +/** Activation by API key is refused with the interactive gate; a web session then activates. */ +async function expectWebOnly(rev: Rev, generation: number) { + const body = { expectedGeneration: generation, revisionId: rev.id, approval: { revisionDigest: rev.digest } }; + const byKey = await k.call("activate", body); + expect(byKey.status).toBe(403); + expect(byKey.body.error.code).toBe("kit.interactive_approval_required"); + const byWeb = await k.call("activate", body, WEB); + expect(byWeb.status).toBe(200); + const stored = k.h.d1.raw.prepare("SELECT approval_json FROM personal_kit_revisions WHERE id = ?").get(rev.id) as { approval_json: string }; + expect(JSON.parse(stored.approval_json)).toMatchObject({ credential: "web-session" }); +} + +describe("activation gate of revisions derived from a proposal", () => { + it("gates a rebase draft of a reviewed proposal, also through a chain of rebases", async () => { + const { rev, generation } = await reviewedProposal(); + await k.publish("@alpha/demo", "2.0.0", v2("eight\n", "eight (v2)\n")); + const first = await rebase(rev.id, "2.0.0", generation); + expect(first.body.revision).toMatchObject({ state: "draft", origin: "rebase", parentRevisionId: rev.id }); + await k.publish("@alpha/demo", "3.0.0", v2("seven\n", "seven (v3)\n")); + const second = await rebase(first.body.revision.id, "3.0.0", first.body.generation); + expect(second.body.revision).toMatchObject({ origin: "rebase", parentRevisionId: first.body.revision.id }); + await expectWebOnly(second.body.revision, second.body.generation); + }); + + it("gates a conflict resolution of a reviewed proposal", async () => { + const { rev, generation } = await reviewedProposal(); + await k.publish("@alpha/demo", "2.0.0", v2("two\n", "two (v2)\n")); + const clash = await rebase(rev.id, "2.0.0", generation); + expect(clash.body.status).toBe("needs-review"); + const merged = await k.overlayTo("@alpha/demo", "2.0.0", { ...LEARNED, "SKILL.md": skillText(BODY.replace("two\n", "two (mine, v2)\n")) }); + const res = await k.call(`conflicts/${clash.body.conflictId}/resolve`, { + expectedGeneration: clash.body.generation, + idempotencyKey: k.idem(), + approvedBy: "user", + decisions: [{ identity: "@alpha/demo", choice: "edit-overlay", ...merged }], + }); + expect(res.status).toBe(201); + expect(res.body.revision).toMatchObject({ origin: "resolution", parentRevisionId: rev.id }); + await expectWebOnly(res.body.revision, res.body.generation); + }); + + it("keeps API-key activation for drafts that descend from manual revisions", async () => { + await k.publish("@alpha/demo", "2.0.0", v2("eight\n", "eight (v2)\n")); + const res = await rebase(manual.id, "2.0.0", 1); + expect(res.status).toBe(201); + const draft = res.body.revision as Rev; + const act = await k.call("activate", { expectedGeneration: res.body.generation, revisionId: draft.id, approval: { revisionDigest: draft.digest } }); + expect(act.status).toBe(200); + }); +}); diff --git a/apps/web/tests/personal-kits/proposal-detector-cost.test.ts b/apps/web/tests/personal-kits/proposal-detector-cost.test.ts new file mode 100644 index 0000000..f3463c8 --- /dev/null +++ b/apps/web/tests/personal-kits/proposal-detector-cost.test.ts @@ -0,0 +1,57 @@ +/** + * The proposal detectors run in time linear in the text: a request body of + * about 1 MiB built to make a backtracking pattern quadratic (long separator + * runs, `a-a-a-...` scheme runs, `eyJ-eyJ-...`, repeated keys) must scan in + * well under the Worker CPU limit. The bound is generous for slow runners; + * a quadratic pattern takes minutes on these inputs. + */ +import { describe, expect, it } from "vitest"; +import { genericSecrets, hasLocalPath, hasMultilinePassword } from "~/lib/personal-kits/proposal-detectors"; + +const SIZE = 1024 * 1024; +const fill = (unit: string) => unit.repeat(Math.ceil(SIZE / unit.length)).slice(0, SIZE); +const BOUND_MS = 1500; + +const HOSTILE: Record = { + "slashes": fill("/"), + "backslashes": fill("\\"), + "JSON-escaped slashes": fill("\\/"), + "percent-encoded slashes": fill("%2F"), + "double-encoded slashes": fill("%252F"), + "scheme-like dashes": fill("a-"), + "scheme-like pluses": fill("a+"), + "drive-like colons": fill("a:"), + "JWT-like heads": fill("eyJ-"), + "dollar segments": fill("/usr/$"), + "allowed system paths": fill(" /usr/bin/x"), + "climbing segments": fill("/a/.."), + "editor links": fill("x://file"), + "glued flags": fill(" -I"), + "flag-like dashes": " -" + fill("a"), + "one long segment": " /" + fill("a"), + "non-ASCII segments": fill("/é"), + "UNC-like runs": fill("\\\\ab"), + "secret key words": fill("TOKEN"), + "password words": fill("password"), + "password assignments": fill("password: "), + "credential URLs": fill("x://a:"), + "YAML password keys": fill("password:\n"), + "YAML list password keys": fill("- password:\n \n"), + "private key heads": fill("-----BEGIN A "), +}; + +describe("detector cost on hostile input", () => { + it.each(Object.entries(HOSTILE))("scans %s in linear time", (_name, text) => { + const started = performance.now(); + hasLocalPath(text); + genericSecrets(text); + hasMultilinePassword(text, text); + expect(performance.now() - started).toBeLessThan(BOUND_MS); + }); + + it("still finds a path or secret at the end of a hostile run", () => { + expect(hasLocalPath(fill("/") + "home/kai")).toBe(true); + expect(hasLocalPath(fill("a-") + " /srv/x/y")).toBe(true); + expect(genericSecrets(fill("eyJ-") + " " + ["eyJ", "hbGciOiJIUzI1NiJ9", ".eyJ", "zdWIiOiIxIn0", ".c2ln"].join(""))).toEqual(["secret-jwt"]); + }); +}); diff --git a/apps/web/tests/personal-kits/proposal-detectors.test.ts b/apps/web/tests/personal-kits/proposal-detectors.test.ts index 3b0fcbc..f4fe39a 100644 --- a/apps/web/tests/personal-kits/proposal-detectors.test.ts +++ b/apps/web/tests/personal-kits/proposal-detectors.test.ts @@ -1,130 +1,52 @@ import { describe, expect, it } from "vitest"; +import vectors from "../../../../packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json"; import { addedLines, genericSecrets, hasByteOrderMark, hasLocalPath, hasMultilinePassword } from "~/lib/personal-kits/proposal-detectors"; import { sharedSkills } from "~/lib/personal-kits/kit-verify"; import { assertNoForbiddenKeys } from "~/lib/personal-kits/proposal-envelope"; -describe("absolute local path detector", () => { - it.each([ - String.raw`"cwd": "C:\\Users\\kai\\repo"`, - String.raw`D:\www\project\notes.md`, - "d:/data/file", - String.raw`share \\srv01\public\x`, - String.raw`"\\\\srv01\\share\\x"`, - "file:///home/kai/x", - "file://host/share", - "/mnt/c/Users/kai", - ">/home/kai/log", - " expect(hasLocalPath(text)).toBe(true)); +/** The shared `skillx.kit-proposal-policy/v1` content vectors (AgentKit tests against the same file). */ +const joined = (v: { parts: string[] }) => v.parts.join(""); - it.each([ - "#!/usr/bin/env node", - "https://example.com/Users/guide", - "src/tmp/file", - "./tmp/x", - "~/notes", - "docs/home/page", - "at 12:30 today", - "http://localhost:3000/api", - String.raw`"\\n\\t"`, - "ratio a:b", - "/homepage and /tmpl", - // The system allowlist, in any case. - "#!/bin/sh", - "/usr/local/bin/tool", - "/sbin/ifconfig", - "edit /etc/hosts", - "/proc/self/status", - "cmd 2>/dev/null", - "read /dev/stdin, write /dev/stdout and /dev/stderr", - "/USR/BIN/env", - "https://example.com/var/www/x?y=%2Fz", - "dates 12/31/2025 and 1/2", - "run /ak:plan now", - ])("allows %s", (text) => expect(hasLocalPath(text)).toBe(false)); +describe("absolute local path detector", () => { + it.each(vectors.localPaths.refuse)("refuses %s", (text) => expect(hasLocalPath(text)).toBe(true)); + it.each(vectors.localPaths.allow)("allows %s", (text) => expect(hasLocalPath(text)).toBe(false)); }); describe("generic secret detector", () => { - // Built at runtime so no credential-shaped literal sits in the source. - const jwt = ["eyJ", "hbGciOiJIUzI1NiJ9", ".eyJ", "zdWIiOiIxIn0", ".c2ln"].join(""); - it.each([ - ["password=" + "hunter22", "secret-password"], - ["db_passwd: " + "hunter22", "secret-password"], - ["Authorization: Bearer " + "a1".repeat(12), "secret-bearer"], - ["postgres://admin:" + "s3cretpw" + "@db.internal/x", "secret-url-credentials"], - ["token " + jwt, "secret-jwt"], - [["-----BEGIN ", "OPENSSH PRIVATE KEY-----"].join(""), "secret-private-key"], - ["OPENAI_API_KEY=" + "x9".repeat(10), "secret-dotenv"], - ["export DB_PASSWORD=" + "q".repeat(12), "secret-dotenv"], - ])("finds %s", (text, rule) => expect(genericSecrets(text)).toContain(rule)); - - it.each(["password: ", "password=${PASSWORD}", "Bearer ", "https://example.com/a@b", "API_KEY=", "the password field", "AUTHOR_NAME=Jonathan"])( - "ignores %s", - (text) => expect(genericSecrets(text)).toEqual([]), - ); + it.each(vectors.secrets.map((v) => [joined(v), v.rules] as const))("finds in %j exactly %j", (text, rules) => expect(genericSecrets(text)).toEqual(rules)); }); describe("multi-line YAML password detector", () => { - const value = "hunter" + "22"; - it.each([ - `db:\n password:\n ${value}\n`, - `password: |\n ${value}\n`, - `password: >-\n ${value}\n more\n`, - `PASSWORD:\n\n ${value}\n`, - ])("finds %j", (text) => expect(hasMultilinePassword(null, text)).toBe(true)); - - it.each(["password: |\n \n", "password:\n ${DB_PASSWORD}\n", "password:\nnext: 1\n", "password: |\n ab\n"])( - "ignores %j", - (text) => expect(hasMultilinePassword(null, text)).toBe(false), - ); + it.each(vectors.multilinePasswords.refuse.map(joined))("finds %j", (text) => expect(hasMultilinePassword(null, text)).toBe(true)); + it.each(vectors.multilinePasswords.allow.map(joined))("ignores %j", (text) => expect(hasMultilinePassword(null, text)).toBe(false)); it("finds a new value under a key the base already has, not a block kept from the base", () => { + const value = "hunter" + "22"; const base = "db:\n password:\n \n"; expect(hasMultilinePassword(base, `db:\n password:\n ${value}\n`)).toBe(true); const kept = `db:\n password:\n ${value}\n`; expect(hasMultilinePassword(kept, `${kept}other: 1\n`)).toBe(false); + const list = `users:\n - password:\n ${value}\n`; + expect(hasMultilinePassword(list, `${list} - name: b\n`)).toBe(false); + }); +}); + +describe("forbidden keys through the envelope", () => { + it.each(vectors.keys.map((v) => [v.name, v] as const))("%s", (_name, v) => { + if (v.refusedAt === null) return expect(() => assertNoForbiddenKeys(v.value)).not.toThrow(); + expect(() => assertNoForbiddenKeys(v.value)).toThrow(`${v.refusedAt} is not accepted`); + }); + + it("never echoes the value", () => { + const value = "hunter" + "22"; + let message = ""; + try { + assertNoForbiddenKeys({ evidence: { password: value } }); + } catch (error) { + message = (error as Error).message; + } + expect(message).toContain("proposal.evidence.password"); + expect(message).not.toContain(value); }); }); diff --git a/docs/personal-kits-api.md b/docs/personal-kits-api.md index c326530..84a38cd 100644 --- a/docs/personal-kits-api.md +++ b/docs/personal-kits-api.md @@ -90,7 +90,10 @@ without an explicit user approval. `ak:` (any case) is always shared, whether or not `SKILLX_SHARED_SKILL_ALIASES` is set. That variable (comma or whitespace separated identities) adds further shared skills to this default. Other - `@agentkit/*` skills may be overlaid. + `@agentkit/*` skills may be overlaid. AgentKit kit composition owns the + canonical list: the non-`ak-` skills that `kits/core` or more than one + official kit ships. Operators set the variable from that list; SkillX never + derives it. - A fork alias that starts with `ak-` or `ak:` is refused with 422 `identity.reserved_alias` (AgentKit code). - Kit names, identities and fork identities pass the full identity rules @@ -264,7 +267,11 @@ Nothing activates implicitly. - **A human approves**: review (approve or reject) and activation of a session-derived revision need an interactive web session. An API key, such as the one AgentKit submits with, gets 403 - `kit.interactive_approval_required`. The proofs record `credential`. + `kit.interactive_approval_required`. The proofs record `credential`. A + rebase or conflict-resolution draft that carries a session-derived revision + forward (through any chain of rebases and resolutions) inherits the same + gate: it activates only from a web session and only on a private kit. A + manual revision in the chain ends the inheritance. - **No chaining on unreviewed proposals**: a from-revision that is `proposed` (or any session-derived revision that is not `reviewed`) fails with 409 `kit.review_required`, and an unreviewed proposal is never usable as a @@ -310,8 +317,17 @@ verifies the result like any revision. messages, conversation, environment, secret or token, working directory, history, tool output or logs, at any depth, fails with 422 `proposal.forbidden_field`. The message names the key path, never its value. - A key with a numeric value (an evidence signal) is allowed. Unknown fields - are refused. An unknown top-level body field is 400 `invalid_request`. + Keys are compared after percent-decoding, NFKC and lowercasing, with + separators removed (`Session_Log`, `api-key`, `%70assword`, full-width + letters). A key longer than 64 code points or with a character outside + printable ASCII is refused. A string that holds a JSON object or array + (outside `contentBase64` and `patch`) is checked as nested keys. A key with + a numeric value (an evidence signal) is allowed. Unknown fields are refused. + An unknown top-level body field is 400 `invalid_request`. The key lists and + the content rules are the shared policy `skillx.kit-proposal-policy/v1` in + `packages/contracts/src/fixtures/kit-proposal-policy-v1.json`; AgentKit + applies the same file before upload, and both sides test against + `kit-proposal-policy-vectors-v1.json` next to it. - **Visibility**: anything but `"private"` fails with 422 `proposal.private_only`. - **Evidence**: optional. `summary` is at most 2048 UTF-8 bytes @@ -324,21 +340,28 @@ verifies the result like any revision. - Fails with 422 `proposal.content_refused`, nothing stored: a secret (vendor token formats, `password=`/`passwd:` assignments, a YAML `password:` key whose new value sits on the following indented lines or in - a `|` / `>` block scalar, even when the key line is kept from the base, + a `|` / `>` block scalar, also as a list item `- password:` or a quoted + key `"password":`, even when the key line is kept from the base, Bearer tokens, URLs with `user:pass@`, JWTs, private key blocks, dotenv-style secret keys), binary content, text with a byte order mark (UTF-8/16/32), and every absolute local path, matched case-insensitively and also JSON-escaped (`\/`, `\\`) or percent-encoded (`%2F`, `%5C`, `%3A`): any drive-letter path, UNC shares (`\\host\share` and - `//host/share`), `file:` and `smb:` URLs, a bare `/home`, `/Users`, - `/root`, `/mnt`, `/media`, `/Volumes`, `/var/folders`, `/tmp`, `/private` - or `/run/user`, and any other POSIX path of two or more segments (such as - `/opt/...`, `/srv/...`, `/var/www/...`, `/workspaces/...`). Only these - system paths are allowed: `/usr/`, `/bin/`, `/sbin/`, `/etc/`, `/proc/`, - `/dev/null`, `/dev/stdin`, `/dev/stdout` and `/dev/stderr` (a `..` segment - that climbs out of them is refused). Web URLs (`https://host/path`) are not - local paths. `error.findings` lists `danger::` codes, never - the matched text. + `//host/share`), `file:` and `smb:` URLs, editor links to a file + (`vscode://file/...`), a bare `/home`, `/Users`, `/root`, `/mnt`, + `/media`, `/Volumes`, `/var/folders`, `/tmp`, `/private` or `/run/user`, + a path glued to a flag (`-I/opt/x`, `--include=/opt/x`), and any other + POSIX path of two or more segments in any script (such as `/opt/...`, + `/srv/...`, `/var/www/...`, `/Usuários/...`). Only these system paths are + allowed: `/bin/`, `/sbin/`, `/etc/`, `/proc/`, the system directories of + `/usr/` and `/usr/local/` (`bin`, `lib`, `share`, `include` and the others + listed in the policy; `/usr/home` and `/usr/local/` are refused), + `/dev/null`, `/dev/stdin`, `/dev/stdout` and `/dev/stderr` (a `..` + segment that climbs out of them is refused). URLs of network schemes + (`https://host/path`, `ssh://`, `s3://`, ...) are not local paths. + `error.findings` lists `danger::` codes, never the matched + text. Every detector runs in time linear in its input, so a hostile + request body cannot exhaust the Worker CPU. - Any other `danger` finding, such as prompt injection or a `javascript:` URL, stores the proposal as `quarantined`. A quarantined proposal cannot be reviewed or activated. From fae341858535384cea81bae15d45121f0c9eb5ef Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Thu, 15 Oct 2026 17:14:16 +0700 Subject: [PATCH 3/8] fix(kits): gate every activation derived from a session revision A revision is gated when it or any ancestor, through any origin, is session-derived; a missing ancestor counts as gated. A gated revision activates only from a web session, on a private kit, as a private document. Activating any public document now needs a web session. --- apps/web/app/lib/personal-kits/kit-http.ts | 11 ++- .../app/lib/personal-kits/kit-lifecycle.ts | 40 ++++---- .../kit-proposal-activation.test.ts | 92 ++++++++++++++++--- .../personal-kits/kit-proposal-safety.test.ts | 2 +- .../tests/personal-kits/kit-safety.test.ts | 9 +- 5 files changed, 114 insertions(+), 40 deletions(-) diff --git a/apps/web/app/lib/personal-kits/kit-http.ts b/apps/web/app/lib/personal-kits/kit-http.ts index 8c484ee..e8afa3e 100644 --- a/apps/web/app/lib/personal-kits/kit-http.ts +++ b/apps/web/app/lib/personal-kits/kit-http.ts @@ -30,11 +30,12 @@ export const kitNotFound = () => releaseError(404, "not_found", "Not found"); export const badRequest = (message: string) => releaseError(400, "invalid_request", message); export const conflict = (code: string, message: string) => releaseError(409, code, message); -/** Review and activation of a session-derived revision: a human in a web session, never an API key. */ -export function requireInteractive(caller: { interactive: boolean }): void { - if (!caller.interactive) { - throw releaseError(403, "kit.interactive_approval_required", "approve session-derived revisions signed in on the web, not with an API key"); - } +/** + * Review and activation of a session-derived revision, and any activation + * that makes a kit public: a human in a web session, never an API key. + */ +export function requireInteractive(caller: { interactive: boolean }, message = "approve session-derived revisions signed in on the web, not with an API key"): void { + if (!caller.interactive) throw releaseError(403, "kit.interactive_approval_required", message); } /** `@handle/name` of the route; malformed is 400. */ diff --git a/apps/web/app/lib/personal-kits/kit-lifecycle.ts b/apps/web/app/lib/personal-kits/kit-lifecycle.ts index e64aa4e..872a922 100644 --- a/apps/web/app/lib/personal-kits/kit-lifecycle.ts +++ b/apps/web/app/lib/personal-kits/kit-lifecycle.ts @@ -4,9 +4,10 @@ * exact `revisionDigest`, re-checks every base of the effective lineage and * moves `effective_revision_id` under the generation guard; nothing becomes * effective implicitly. A proposal (`proposed`) must be reviewed first, and - * a session-derived revision activates only on a private kit and only from - * an interactive web session, and so does a rebase or conflict-resolution - * draft that carries one forward. A quarantined revision anywhere in the chain + * a revision with a session-derived revision anywhere in its parent chain + * (any origin; an unreadable parent counts) activates only on a private kit, + * as a private document, and only from an interactive web session. Any + * activation that makes a kit public needs a web session too. A quarantined revision anywhere in the chain * fails the lock with `kit.locked_incompatible`. * Rejection is final for that revision; a proposal is rejected only through * its web-session review, never through the generic reject. The lock is derived on read from the @@ -64,26 +65,27 @@ function requireApproval(raw: unknown, rev: RevisionRow): void { } /** - * True for a session-derived revision and for a rebase or conflict-resolution - * revision that carries one forward (through any chain of rebases and - * resolutions): it inherits the web-session activation gate. A manual - * revision is a document its author submits whole, so the chain stops there. + * True when `rev` or any revision in its parent chain, whatever its origin, + * is session-derived: it inherits the web-session activation gate. A parent + * that cannot be read (missing, or in another kit) counts as session-derived, + * so the check fails closed. */ async function derivesFromSession(db: D1Database, kitId: string, rev: RevisionRow): Promise { if (rev.origin === "session-derived") return true; - if (rev.origin !== "rebase" && rev.origin !== "resolution") return false; - const found = await db + const row = await db .prepare( `WITH RECURSIVE chain(id, origin, parent) AS ( SELECT id, origin, parent_revision_id FROM personal_kit_revisions WHERE id = ? AND kit_id = ? UNION - SELECT r.id, r.origin, r.parent_revision_id FROM personal_kit_revisions r JOIN chain c ON r.id = c.parent - WHERE r.kit_id = ? AND c.origin IN ('rebase', 'resolution')) - SELECT 1 AS found FROM chain WHERE origin = 'session-derived' LIMIT 1`, + SELECT r.id, r.origin, r.parent_revision_id FROM personal_kit_revisions r JOIN chain c ON r.id = c.parent WHERE r.kit_id = ?) + SELECT COUNT(*) AS n, + COALESCE(SUM(origin = 'session-derived'), 0) AS tainted, + COALESCE(SUM(parent IS NOT NULL AND parent NOT IN (SELECT id FROM chain)), 0) AS dangling + FROM chain`, ) .bind(rev.id, kitId, kitId) - .first<{ found: number }>(); - return found !== null; + .first<{ n: number; tainted: number; dangling: number }>(); + return row === null || Number(row.n) === 0 || Number(row.tainted) > 0 || Number(row.dangling) > 0; } /** `POST .../activate` and `POST .../rollback` (rollback: a revision that was effective before). */ @@ -96,16 +98,20 @@ export async function activateRevision(caller: KitCaller, kitName: string, body: if (!ACTIVATABLE.has(rev.state)) throw conflict("kit.revision_unusable", `the revision is ${rev.state}`); const sessionDerived = rev.origin === "session-derived"; if (sessionDerived && rev.state !== "reviewed") throw conflict("kit.review_required", "a session-derived revision must be reviewed"); + const doc = documentOf(rev); if (await derivesFromSession(caller.env.DB, kit.id, rev)) { requireInteractive(caller); - // Activation would make the kit private; a public kit is forked to a private one instead. - if (kit.visibility !== "private") throw conflict("proposal.private_kit_required", "a session-derived revision activates on a private kit only; fork this kit to a private kit"); + // Both the kit and the visibility the activation writes stay private; a public kit is forked to a private one instead. + if (kit.visibility !== "private" || doc.visibility !== "private") { + throw conflict("proposal.private_kit_required", "a revision derived from a session-derived revision activates on a private kit, as a private document, only"); + } } + // Publishing is a human decision whatever the origin: an API key never makes a kit public. + if (doc.visibility !== "private") requireInteractive(caller, "activate a public revision signed in on the web, not with an API key"); if (rollback && (rev.activated_at === null || rev.id === kit.effective_revision_id)) throw badRequest("rollback targets an earlier effective revision"); if (!rollback && rev.id === kit.effective_revision_id) throw badRequest("the revision is already effective"); const check = await checkLock(caller, rev); if (check.reasons.length > 0) return incompatible(check.reasons); - const doc = documentOf(rev); const at = caller.now.getTime(); // A reviewed proposal keeps its review proof next to the activation approval. const review = sessionDerived && rev.approval_json !== null ? (JSON.parse(rev.approval_json) as { review?: unknown }) : null; diff --git a/apps/web/tests/personal-kits/kit-proposal-activation.test.ts b/apps/web/tests/personal-kits/kit-proposal-activation.test.ts index 1bc3b09..0986118 100644 --- a/apps/web/tests/personal-kits/kit-proposal-activation.test.ts +++ b/apps/web/tests/personal-kits/kit-proposal-activation.test.ts @@ -1,8 +1,10 @@ /** - * A rebase or conflict-resolution draft that carries a reviewed - * session-derived revision forward inherits its activation gate: only a - * web session activates it, never the API key that submitted the proposal. - * Drafts that descend from manual revisions keep activating with a key. + * Activation gate of session-derived content. A revision with a + * session-derived revision anywhere in its parent chain, whatever its own + * origin (manual, rebase, resolution), or with a parent that cannot be read, + * activates (and rolls back) only from a web session and only as a private + * document on a private kit. Any activation that makes a kit public needs a + * web session too; other private activations keep working with an API key. */ import { beforeEach, describe, expect, it, vi } from "vitest"; import { BODY, createKitHarness, skillText } from "./kit-harness"; @@ -20,6 +22,10 @@ const WEB = { key: null }; const MINE = { "SKILL.md": skillText(BODY.replace("two\n", "two (mine)\n")), "notes.md": "my notes\n" }; const LEARNED = { ...MINE, "notes.md": "my notes\nprefer small steps\n" }; const v2 = (from: string, to: string) => ({ "SKILL.md": skillText(BODY.replace(from, to)) }); +const gated = (res: { status: number; body: Record }) => { + expect(res.status).toBe(403); + expect(res.body.error.code).toBe("kit.interactive_approval_required"); +}; beforeEach(async () => { session.user = { id: "user-a" }; @@ -38,22 +44,38 @@ async function reviewedProposal(): Promise<{ rev: Rev; generation: number }> { return { rev, generation: review.body.generation }; } +const activation = (rev: Rev, generation: number) => ({ expectedGeneration: generation, revisionId: rev.id, approval: { revisionDigest: rev.digest } }); + +/** The reviewed proposal, made effective from a web session; returns the generation after it. */ +async function effectiveProposal(): Promise<{ rev: Rev; generation: number }> { + const { rev, generation } = await reviewedProposal(); + const act = await k.call("activate", activation(rev, generation), WEB); + expect(act.status).toBe(200); + return { rev, generation: act.body.generation }; +} + +/** A manual revision; its parent is the effective revision. */ +async function manualRevision(generation: number, note: string, visibility = "private") { + const document = k.doc([await k.entry("@alpha/demo", "1.0.0", { ...LEARNED, "notes.md": note })], { visibility }); + const res = await k.call("revisions", { expectedGeneration: generation, idempotencyKey: k.idem(), document }); + expect(res.status).toBe(201); + return { rev: res.body.revision as Rev & { parentRevisionId: string }, generation: res.body.generation as number }; +} + const rebase = (fromRevisionId: string, version: string, expectedGeneration: number) => k.call("rebase", { expectedGeneration, idempotencyKey: k.idem(), fromRevisionId, updates: [{ package: "@alpha/demo", version }] }); /** Activation by API key is refused with the interactive gate; a web session then activates. */ -async function expectWebOnly(rev: Rev, generation: number) { - const body = { expectedGeneration: generation, revisionId: rev.id, approval: { revisionDigest: rev.digest } }; - const byKey = await k.call("activate", body); - expect(byKey.status).toBe(403); - expect(byKey.body.error.code).toBe("kit.interactive_approval_required"); - const byWeb = await k.call("activate", body, WEB); +async function expectWebOnly(rev: Rev, generation: number, action = "activate") { + gated(await k.call(action, activation(rev, generation))); + const byWeb = await k.call(action, activation(rev, generation), WEB); expect(byWeb.status).toBe(200); const stored = k.h.d1.raw.prepare("SELECT approval_json FROM personal_kit_revisions WHERE id = ?").get(rev.id) as { approval_json: string }; expect(JSON.parse(stored.approval_json)).toMatchObject({ credential: "web-session" }); + return byWeb.body.generation as number; } -describe("activation gate of revisions derived from a proposal", () => { +describe("revisions derived from a proposal", () => { it("gates a rebase draft of a reviewed proposal, also through a chain of rebases", async () => { const { rev, generation } = await reviewedProposal(); await k.publish("@alpha/demo", "2.0.0", v2("eight\n", "eight (v2)\n")); @@ -82,12 +104,54 @@ describe("activation gate of revisions derived from a proposal", () => { await expectWebOnly(res.body.revision, res.body.generation); }); - it("keeps API-key activation for drafts that descend from manual revisions", async () => { + it("gates a manual revision stored over an effective proposal, and a rebase of it", async () => { + const { rev, generation } = await effectiveProposal(); + const next = await manualRevision(generation, "edited by hand\n"); + expect(next.rev).toMatchObject({ origin: "manual", parentRevisionId: rev.id }); + const after = await expectWebOnly(next.rev, next.generation); + await k.publish("@alpha/demo", "2.0.0", v2("eight\n", "eight (v2)\n")); + const rebased = await rebase(next.rev.id, "2.0.0", after); + expect(rebased.body.revision).toMatchObject({ origin: "rebase", parentRevisionId: next.rev.id }); + await expectWebOnly(rebased.body.revision, rebased.body.generation); + }); + + it("gates a rollback onto a session-derived revision", async () => { + const { rev, generation } = await effectiveProposal(); + const next = await manualRevision(generation, "edited by hand\n"); + const after = await expectWebOnly(next.rev, next.generation); + await expectWebOnly(rev, after, "rollback"); + }); + + it("keeps a revision derived from a proposal private, even from a web session", async () => { + const { generation } = await effectiveProposal(); + const pub = await manualRevision(generation, "published\n", "public"); + const act = await k.call("activate", activation(pub.rev, pub.generation), WEB); + expect(act.status).toBe(409); + expect(act.body.error.code).toBe("proposal.private_kit_required"); + }); + + it("gates a revision whose parent cannot be read (fails closed)", async () => { + await k.publish("@alpha/demo", "2.0.0", v2("eight\n", "eight (v2)\n")); + const res = await rebase(manual.id, "2.0.0", 1); + expect(res.status).toBe(201); + k.sql("DROP TRIGGER personal_kit_revisions_immutable"); + k.sql("UPDATE personal_kit_revisions SET parent_revision_id = ? WHERE id = ?", "pkr_00000000-0000-0000-0000-000000000000", manual.id); + gated(await k.call("activate", activation(res.body.revision, res.body.generation))); + }); +}); + +describe("activations without session-derived content", () => { + it("keeps API-key activation for private drafts that descend from manual revisions", async () => { await k.publish("@alpha/demo", "2.0.0", v2("eight\n", "eight (v2)\n")); const res = await rebase(manual.id, "2.0.0", 1); expect(res.status).toBe(201); - const draft = res.body.revision as Rev; - const act = await k.call("activate", { expectedGeneration: res.body.generation, revisionId: draft.id, approval: { revisionDigest: draft.digest } }); + const act = await k.call("activate", activation(res.body.revision, res.body.generation)); expect(act.status).toBe(200); }); + + it("makes a kit public only from a web session", async () => { + const pub = await manualRevision(1, "published\n", "public"); + await expectWebOnly(pub.rev, pub.generation); + expect((await k.call("")).body.visibility).toBe("public"); + }); }); diff --git a/apps/web/tests/personal-kits/kit-proposal-safety.test.ts b/apps/web/tests/personal-kits/kit-proposal-safety.test.ts index 7f17049..5762e94 100644 --- a/apps/web/tests/personal-kits/kit-proposal-safety.test.ts +++ b/apps/web/tests/personal-kits/kit-proposal-safety.test.ts @@ -166,7 +166,7 @@ describe("proposal approval", () => { const rev = (await submit(await proposal(with_("x\n")), 1)).body.revision; await approve(rev, 2); const pub = (await k.call("revisions", { expectedGeneration: 3, idempotencyKey: k.idem(), document: k.doc([await k.entry("@alpha/demo", "1.0.0", MINE)], { visibility: "public" }) })).body.revision; - expect((await k.call("activate", { expectedGeneration: 4, revisionId: pub.id, approval: { revisionDigest: pub.digest } })).status).toBe(200); + expect((await k.call("activate", { expectedGeneration: 4, revisionId: pub.id, approval: { revisionDigest: pub.digest } }, WEB)).status).toBe(200); const act = await k.call("activate", { expectedGeneration: 5, revisionId: rev.id, approval: { revisionDigest: rev.digest } }, WEB); expect(act.status).toBe(409); expect(act.body.error.code).toBe("proposal.private_kit_required"); diff --git a/apps/web/tests/personal-kits/kit-safety.test.ts b/apps/web/tests/personal-kits/kit-safety.test.ts index 145a528..1e912e1 100644 --- a/apps/web/tests/personal-kits/kit-safety.test.ts +++ b/apps/web/tests/personal-kits/kit-safety.test.ts @@ -9,7 +9,8 @@ import { ReleaseHttpError } from "~/lib/releases/release-http"; import { BODY, createKitHarness, skillText } from "./kit-harness"; import type { KitHarness } from "./kit-harness"; -vi.mock("~/lib/auth/session-helpers", () => ({ getSession: async () => null })); +const session = vi.hoisted(() => ({ user: null as { id: string } | null })); +vi.mock("~/lib/auth/session-helpers", () => ({ getSession: async () => (session.user === null ? null : { user: session.user }) })); let k: KitHarness; const V1 = { "SKILL.md": skillText(BODY) }; @@ -22,14 +23,16 @@ beforeEach(async () => { await k.publish("@alpha/demo", "1.0.0", V1); }); -/** A revision of user-b's kit `@beta/base`; activated when `activate` is set. */ +/** A revision of user-b's kit `@beta/base`; activated when `activate` is set (in user-b's web session: a public activation needs one). */ async function betaRevision(generation: number, visibility: "private" | "public", activate: boolean, files = MINE) { const document = { ...k.doc([await k.entry("@alpha/demo", "1.0.0", files)], { visibility }), kit: "@beta/base", localId: "local/base" }; const res = await k.call("revisions", { expectedGeneration: generation, idempotencyKey: k.idem(), document }, { ...BETA, key: k.otherKey }); expect(res.status).toBe(201); const rev = res.body.revision as { id: string; digest: string }; if (activate) { - const ok = await k.call("activate", { expectedGeneration: generation + 1, revisionId: rev.id, approval: { revisionDigest: rev.digest } }, { ...BETA, key: k.otherKey }); + session.user = { id: "user-b" }; + const ok = await k.call("activate", { expectedGeneration: generation + 1, revisionId: rev.id, approval: { revisionDigest: rev.digest } }, { ...BETA, key: null }); + session.user = null; expect(ok.status).toBe(200); } return rev; From e5ebbfd81ef638ff66cf214804af9945f5f0973c Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Thu, 15 Oct 2026 17:14:16 +0700 Subject: [PATCH 4/8] fix(kits): tighten the proposal path allowlist and secret rules Pin the key normalization order and Unicode version, escape key paths, and match password and secret assignments through the shared key normalization. The path allowlist keeps exact well-known entries and binary or library subtrees only, and the path detector reads %2E, NFD and zero-width forms. The cost test asserts linear growth. --- .../lib/personal-kits/proposal-detectors.ts | 90 +++-- .../personal-kits/proposal-path-detector.ts | 93 +++-- .../proposal-detector-cost.test.ts | 100 +++-- .../src/fixtures/kit-proposal-policy-v1.json | 135 +++++-- .../kit-proposal-policy-vectors-v1.json | 363 +++++++++++++++++- .../src/personal-kit/proposal-policy.test.ts | 37 +- .../src/personal-kit/proposal-policy.ts | 86 +++-- 7 files changed, 708 insertions(+), 196 deletions(-) diff --git a/apps/web/app/lib/personal-kits/proposal-detectors.ts b/apps/web/app/lib/personal-kits/proposal-detectors.ts index 9251704..93e91b4 100644 --- a/apps/web/app/lib/personal-kits/proposal-detectors.ts +++ b/apps/web/app/lib/personal-kits/proposal-detectors.ts @@ -1,25 +1,46 @@ /** * Text detectors for session-derived content. Absolute local paths are - * refused by `proposal-path-detector`. Generic credentials (password - * assignments, also a YAML `password:` key, list item or quoted key whose - * value sits on the next lines, Bearer tokens, URLs with `user:pass@`, JWTs, - * private key blocks and dotenv-style secret keys) complement the vendor - * token rules of the release scanner. Detectors return rule names only, - * never the matched text. Every pattern runs in time linear in the text - * (bounded scheme names, one start per token or line), so hostile input - * cannot exhaust the Worker CPU. The shared vectors are in - * `@skillx/contracts` (`kit-proposal-policy-vectors-v1.json`). + * refused by `proposal-path-detector`. Generic credentials complement the + * vendor token rules of the release scanner: + * - a `key: value`, `key = value` or `"key": "value"` assignment (YAML, JSON, + * code, `.env`) whose key names a password or a secret (`dbPassword`, + * `client_secret`, `apiKey`, `GITHUB_TOKEN`), compared through the shared + * key normalization of `@skillx/contracts`, so content rules and envelope + * keys agree; a YAML password key whose value sits on the following lines; + * - Bearer tokens, URLs with `user:pass@`, JWTs and private key blocks. + * Detectors return rule names only, never the matched text. Every pattern + * runs in time linear in the text (bounded keys and scheme names, one start + * per token or line), so hostile input cannot exhaust the Worker CPU. The + * shared vectors are in `@skillx/contracts` (`kit-proposal-policy-vectors-v1.json`). */ +import { secretKeyKind } from "@skillx/contracts"; + export { hasLocalPath } from "./proposal-path-detector"; -const SECRET_KEY = /SECRET|TOKEN|PASSWORD|PASSWD|PWD|API_?KEY|ACCESS_?KEY|PRIVATE_?KEY|CLIENT_?SECRET|CREDENTIALS?/; -/** A value that is not an obvious placeholder (`<...>`, `${...}`, `$VAR`, `%VAR%`, `***`). */ -const VALUE = String.raw`["']?(?![<$%{*])[^\s"'<>]`; -/** `KEY=value` at the start of a line; the key is tested against `SECRET_KEY` on its own. */ -const DOTENV_LINE = new RegExp(String.raw`(?:^|\n)[ \t]*(?:export[ \t]+)?([A-Za-z0-9_]+)[ \t]*=[ \t]*${VALUE}{8,}`, "g"); +/** + * An obvious placeholder value: `<...>`, `%VAR%`, `{...}`, `***`, `${...}` or + * a whole `$VAR`. Any other value, also one that starts with `$`, is real. + */ +const PLACEHOLDER = String.raw`(?:[<%{*]|\$\{|\$[A-Z_][A-Z0-9_]*(?![^\s"'<>]))`; +/** An optional YAML anchor or tag before the value (`&a`, `!secret`). */ +const ANCHOR = String.raw`(?:[&!][^\s"'<>]{0,64}[ \t]+)?`; +/** The same before a block value, where the anchor or tag may end the key line. */ +const BLOCK_ANCHOR = String.raw`(?:[&!][^\s"'<>]{0,64}[ \t]*)?`; +/** + * `key value` on one line: a key of at most 128 characters, optionally + * quoted, at a key boundary; group 1 is the key, 2 the separator, 3 the + * value. A key past the 64-character policy limit counts as a secret key. + */ +const ASSIGNMENT = new RegExp( + String.raw`(?]+)`, + "g", +); +/** A `KEY=value` (or `export KEY=value`) line start before the key at the end of this window. */ +const DOTENV_START = /(?:^|\n)[ \t]*(?:export[ \t]+)?["']?$/; +const MIN_PASSWORD = 4; +const MIN_SECRET = 8; const LINE_RULES: ReadonlyArray<[rule: string, re: RegExp]> = [ - ["secret-password", new RegExp(String.raw`(?): void { + for (const m of text.matchAll(ASSIGNMENT)) { + const [, key, sep, value] = m as unknown as [string, string, string, string]; + if (/^[0-9.]+$/.test(value)) continue; + const kind = secretKeyKind(key); + if (kind === "password") { + if (value.length >= MIN_PASSWORD) rules.add("secret-password"); + continue; + } + if (value.length < MIN_SECRET) continue; + const dotenv = sep === "=" && DOTENV_START.test(text.slice(Math.max(0, m.index! - 80), m.index!)); + if (dotenv && secretKeyKind(key, true) !== null) rules.add("secret-dotenv"); + else if (kind === "secret") rules.add("secret-assignment"); + } } /** Generic credential rules matched by `text`, sorted. */ export function genericSecrets(text: string): string[] { - const rules = LINE_RULES.filter(([, re]) => re.test(text)).map(([rule]) => rule); - if (hasJwt(text)) rules.push("secret-jwt"); - if (hasDotenvSecret(text)) rules.push("secret-dotenv"); - return rules.sort(); + const rules = new Set(LINE_RULES.filter(([, re]) => re.test(text)).map(([rule]) => rule)); + if (hasJwt(text)) rules.add("secret-jwt"); + assignmentRules(text, rules); + return [...rules].sort(); } /** @@ -62,18 +101,17 @@ export function genericSecrets(text: string): string[] { * plain indented block or a `|` / `>` block scalar). Group 2 is the block. */ const YAML_PASSWORD_BLOCK = new RegExp( - String.raw`(?:^|\n)([ \t]*)(?:-[ \t]+)*["']?[A-Za-z0-9_-]*(?:password|passwd|passphrase|pwd)["']?[ \t]*:[ \t]*(?:[|>][-+0-9]*[ \t]*)?(?:#[^\n]*)?\n((?:[ \t]*\n)*\1[ \t]+\S[^\n]*(?:\n\1[ \t]+[^\n]*|\n[ \t]*(?=\n))*)`, + String.raw`(?:^|\n)([ \t]*)(?:-[ \t]+)*["']?[A-Za-z0-9_-]*(?:password|passwd|passphrase|pwd)["']?[ \t]*:[ \t]*${BLOCK_ANCHOR}(?:[|>][-+0-9]*[ \t]*)?(?:#[^\n]*)?\n((?:[ \t]*\n)*\1[ \t]+\S[^\n]*(?:\n\1[ \t]+[^\n]*|\n[ \t]*(?=\n))*)`, "gi", ); -/** An obvious placeholder value (`<...>`, `${...}`, `$VAR`, `%VAR%`, `***`), optionally quoted. */ -const PLACEHOLDER = /^["']?[<$%{*]/; +const BLOCK_PLACEHOLDER = new RegExp(String.raw`^["']?${PLACEHOLDER}`); /** The password blocks of `text` that hold a real value. */ function passwordBlocks(text: string): string[] { const blocks: string[] = []; for (const m of text.matchAll(YAML_PASSWORD_BLOCK)) { const value = m[2]!.trim(); - if (value.length >= 4 && !PLACEHOLDER.test(value)) blocks.push(m[0].replace(/^\n/, "")); + if (value.length >= MIN_PASSWORD && !BLOCK_PLACEHOLDER.test(value)) blocks.push(m[0].replace(/^\n/, "")); } return blocks; } diff --git a/apps/web/app/lib/personal-kits/proposal-path-detector.ts b/apps/web/app/lib/personal-kits/proposal-path-detector.ts index 22b6965..6ff0127 100644 --- a/apps/web/app/lib/personal-kits/proposal-path-detector.ts +++ b/apps/web/app/lib/personal-kits/proposal-path-detector.ts @@ -1,16 +1,18 @@ /** * Absolute local path detector for session-derived content. Every absolute * local path is refused, in plain, JSON-escaped (`\/`, `\\`) and - * percent-encoded (`%2F`, `%5C`, `%3A`) form, case-insensitively: a - * drive-letter path, a UNC share (`\\host\share` or `//host/share`), a - * `file:` or `smb:` URL, an editor link to a file (`vscode://file/...`), a - * bare user, temp, mount or volume root (`/home`, `/Users`, `/tmp`, ...), - * a path glued to a compiler flag (`-I/opt/x`) and any POSIX path of two or - * more segments, in any script (`/Usuários/...`). Only well-known system - * paths stay allowed: `/bin/`, `/sbin/`, `/etc/`, `/proc/`, the system - * directories of `/usr/` and `/usr/local/` (not `/usr/home` or - * `/usr/local/`), and `/dev/null`, `/dev/stdin`, `/dev/stdout`, - * `/dev/stderr`. URLs of network schemes (`https://host/path`) are not paths. + * percent-encoded (`%2F`, `%5C`, `%3A`, `%2E`) form, case-insensitively, + * after NFC normalization and with invisible format characters (zero-width + * spaces and joiners) removed: a drive-letter path, a UNC share (`\\host\share` + * or `//host/share`), a `file:` or `smb:` URL, an editor link to a file + * (`vscode://file/...`), a user home in an `ssh`/`sftp` URL, a bare user, + * temp, mount or volume root (`/home`, `/Users`, `/tmp`, ...), a path glued to + * a compiler flag (`-I/opt/x`) and any POSIX path of two or more segments, in + * any script (`/Usuários/...`). Only the shared system allowlist stays + * allowed: whole subtrees of binary and library directories (`/usr/bin/...`, + * `/usr/local/lib/...`) and exact well-known files and directories + * (`/etc/hosts`, `/proc/cpuinfo`, `/dev/null`, ...). URLs of network schemes + * (`https://host/path`) are not paths. * * Every pattern runs in time linear in the text: separator runs are * collapsed first, scheme names are bounded and each path run is read once, @@ -19,13 +21,13 @@ import { KIT_PROPOSAL_POLICY } from "@skillx/contracts"; /** Not part of a path or word: what may precede an absolute path. */ -const START = String.raw`(? = [ /\b(?:file|smb):[\\/]/i, // An editor or IDE link to a file: vscode://file/..., cursor://file/... /\b[a-z][a-z0-9+.-]{0,31}:(?:\\*\/){2}file[\\/:]/i, + // A user home on the remote side of an ssh or sftp URL (ssh://kai@devbox/home/kai). + /\b(?:ssh|sftp|scp|git\+ssh|svn\+ssh):(?:\\*\/){2}[^\s/\\"'<>`]*(?:\\*\/)+(?:home|Users|root)(?![A-Za-z0-9_-])/i, // A bare user, temp, mount or volume root, after any non-path character (also `\/home`). new RegExp(String.raw`${START}${SEP}(?:home|Users|root|mnt|media|Volumes|var${SEP}folders|tmp|private|run${SEP}user)(?![\p{L}\p{N}_-])`, "iu"), ]; /** A POSIX path of two or more segments (also `//host/share`). */ const POSIX_PATH = new RegExp(String.raw`${START}${SLASHES}${SEGMENT}${SLASHES}(?:${SEGMENT})?`, "giu"); -/** The first three raw segments of a path, read from its start. */ -const SEGMENTS = new RegExp(String.raw`${SLASHES}([${SEG_CHARS}]*)(?:${SLASHES}([${SEG_CHARS}]*))?(?:${SLASHES}([${SEG_CHARS}]*))?`, "uy"); -/** The rest of the path run a match starts. */ +/** The path run a match starts. */ const PATH_RUN = new RegExp(String.raw`[${SEG_CHARS}\\/]*`, "uy"); -const DOT_DOT = /(?:^|[\\/])\.\.(?=[\\/]|$)/g; +const DOT_DOT = /(?:^|\/)\.\.(?=\/|$)/; /** The shared system allowlist of `skillx.kit-proposal-policy/v1`. */ const ALLOW = KIT_PROPOSAL_POLICY.content.localPathSystemAllowlist; -const SYSTEM = { - roots: new Set(ALLOW.roots), - usr: new Set(ALLOW.usr), - usrLocal: new Set(ALLOW.usrLocal), - dev: new Set(ALLOW.dev), -}; +const EXACT = new Set(ALLOW.exact); +const SUBTREES = ALLOW.subtrees; /** A URL of a network scheme; its path is not a local path. Other schemes (`vscode:`, `chrome-extension:`) keep theirs. */ const WEB_URL = - /(?`]*/gi; + /(?`,)\]]*/gi; /** A short flag glued to a path (`-I/opt/x`, `-isystem/opt/x`), after a word boundary of a command line. */ const GLUED_FLAG = /(? text.replace(/[\\/]{9,}/g, (run) => run.slice(0, 4) + run.slice(-4)); -function allowedSystemPath(segments: Array): boolean { - const [root, second, third] = segments.map((s) => s?.toLowerCase()); - if (SYSTEM.roots.has(root!)) return true; - if (root === "usr") { - if (second === undefined || second === "") return true; - if (second !== "local") return SYSTEM.usr.has(second); - return third === undefined || third === "" || SYSTEM.usrLocal.has(third); - } - // `/dev/null` and the standard streams, but not a path below them. - return root === "dev" && second !== undefined && SYSTEM.dev.has(second) && third === undefined; +/** + * True for a path run on the allowlist: compared in lower case with every + * separator run as one `/` and trailing separators and dots removed, it + * equals an exact entry or lies inside a subtree; a `..` segment refuses it. + */ +function allowedSystemPath(run: string): boolean { + const path = run.replace(/[\\/]+/g, "/").replace(/[/.]+$/, "").toLowerCase(); + if (DOT_DOT.test(path)) return false; + return EXACT.has(path) || SUBTREES.some((root) => path === root || path.startsWith(`${root}/`)); } +/** The text as the detector reads it: NFC, no format characters, decoded separators, collapsed runs, flags apart. */ +const prepare = (text: string) => + collapseSeparators(decodeSeparators(text.replace(/\p{Cf}/gu, "").normalize("NFC"))).replace(GLUED_FLAG, "$1 "); + /** True when `text` names an absolute local path. */ export function hasLocalPath(text: string): boolean { - const decoded = collapseSeparators(decodeSeparators(text)).replace(GLUED_FLAG, "$1 "); + const decoded = prepare(text); if (LOCAL_PATHS.some((re) => re.test(decoded))) return true; const plain = decoded.replace(WEB_URL, " "); let runEnd = -1; - let lastDotDot = -1; for (const m of plain.matchAll(POSIX_PATH)) { const start = m.index!; - if (start >= runEnd) { - // A new path run: read it once and remember its last `..` segment. - PATH_RUN.lastIndex = start; - PATH_RUN.exec(plain); - runEnd = PATH_RUN.lastIndex; - lastDotDot = -1; - for (const d of plain.slice(start, runEnd).matchAll(DOT_DOT)) lastDotDot = start + d.index!; - } - // `/usr/bin/../../home/...` climbs out of the system root. - if (lastDotDot >= start) return true; - SEGMENTS.lastIndex = start; - const segments = SEGMENTS.exec(plain)!; - if (!allowedSystemPath([segments[1], segments[2], segments[3]])) return true; + // Each path run is judged once, as a whole, from its first match. + if (start < runEnd) continue; + PATH_RUN.lastIndex = start; + PATH_RUN.exec(plain); + runEnd = PATH_RUN.lastIndex; + if (!allowedSystemPath(plain.slice(start, runEnd))) return true; } return false; } diff --git a/apps/web/tests/personal-kits/proposal-detector-cost.test.ts b/apps/web/tests/personal-kits/proposal-detector-cost.test.ts index f3463c8..c1bfc93 100644 --- a/apps/web/tests/personal-kits/proposal-detector-cost.test.ts +++ b/apps/web/tests/personal-kits/proposal-detector-cost.test.ts @@ -1,52 +1,74 @@ /** - * The proposal detectors run in time linear in the text: a request body of - * about 1 MiB built to make a backtracking pattern quadratic (long separator - * runs, `a-a-a-...` scheme runs, `eyJ-eyJ-...`, repeated keys) must scan in - * well under the Worker CPU limit. The bound is generous for slow runners; - * a quadratic pattern takes minutes on these inputs. + * The proposal detectors run in time linear in the text. Each input is built + * to make a backtracking pattern quadratic (long separator runs, `a-a-a-...` + * scheme runs, `eyJ-eyJ-...`, repeated keys, blank lines under a YAML key) + * and scanned at 128 KiB and at 1 MiB: 8 times the text may cost at most 24 + * times the time (a quadratic pattern costs 64 times), measured as the best + * of several runs, with a small floor so timer noise on fast inputs is not + * read as growth. No wall-clock bound, so a slow runner does not fail it. */ import { describe, expect, it } from "vitest"; import { genericSecrets, hasLocalPath, hasMultilinePassword } from "~/lib/personal-kits/proposal-detectors"; const SIZE = 1024 * 1024; -const fill = (unit: string) => unit.repeat(Math.ceil(SIZE / unit.length)).slice(0, SIZE); -const BOUND_MS = 1500; +const SMALL = SIZE / 8; +const repeatTo = (unit: string, size: number) => unit.repeat(Math.ceil(size / unit.length)).slice(0, size); +const fill = (unit: string) => repeatTo(unit, SIZE); +const MAX_RATIO = 24; +const FLOOR_MS = 5; +const RUNS = 3; -const HOSTILE: Record = { - "slashes": fill("/"), - "backslashes": fill("\\"), - "JSON-escaped slashes": fill("\\/"), - "percent-encoded slashes": fill("%2F"), - "double-encoded slashes": fill("%252F"), - "scheme-like dashes": fill("a-"), - "scheme-like pluses": fill("a+"), - "drive-like colons": fill("a:"), - "JWT-like heads": fill("eyJ-"), - "dollar segments": fill("/usr/$"), - "allowed system paths": fill(" /usr/bin/x"), - "climbing segments": fill("/a/.."), - "editor links": fill("x://file"), - "glued flags": fill(" -I"), - "flag-like dashes": " -" + fill("a"), - "one long segment": " /" + fill("a"), - "non-ASCII segments": fill("/é"), - "UNC-like runs": fill("\\\\ab"), - "secret key words": fill("TOKEN"), - "password words": fill("password"), - "password assignments": fill("password: "), - "credential URLs": fill("x://a:"), - "YAML password keys": fill("password:\n"), - "YAML list password keys": fill("- password:\n \n"), - "private key heads": fill("-----BEGIN A "), +type Build = (size: number) => string; +const unit = (u: string): Build => (size) => repeatTo(u, size); +const after = (head: string, u: string): Build => (size) => head + repeatTo(u, size); + +const HOSTILE: Record = { + "slashes": unit("/"), + "backslashes": unit("\\"), + "JSON-escaped slashes": unit("\\/"), + "percent-encoded slashes": unit("%2F"), + "double-encoded slashes": unit("%252F"), + "scheme-like dashes": unit("a-"), + "scheme-like pluses": unit("a+"), + "drive-like colons": unit("a:"), + "JWT-like heads": unit("eyJ-"), + "dollar segments": unit("/usr/$"), + "allowed system paths": unit(" /usr/bin/x"), + "climbing segments": unit("/a/.."), + "editor links": unit("x://file"), + "glued flags": unit(" -I"), + "flag-like dashes": after(" -", "a"), + "one long segment": after(" /", "a"), + "non-ASCII segments": unit("/é"), + "UNC-like runs": unit("\\\\ab"), + "secret key words": unit("TOKEN"), + "password words": unit("password"), + "password assignments": unit("password: "), + "credential URLs": unit("x://a:"), + "YAML password keys": unit("password:\n"), + "YAML list password keys": unit("- password:\n \n"), + "private key heads": unit("-----BEGIN A "), + "blank lines under a YAML password key": (size) => "password:\n" + repeatTo(" \n", size) + "x\n", }; describe("detector cost on hostile input", () => { - it.each(Object.entries(HOSTILE))("scans %s in linear time", (_name, text) => { - const started = performance.now(); - hasLocalPath(text); - genericSecrets(text); - hasMultilinePassword(text, text); - expect(performance.now() - started).toBeLessThan(BOUND_MS); + /** The best of `RUNS` timings of every detector on `text`. */ + function cost(text: string): number { + let best = Infinity; + for (let i = 0; i < RUNS; i++) { + const started = performance.now(); + hasLocalPath(text); + genericSecrets(text); + hasMultilinePassword(text, text); + best = Math.min(best, performance.now() - started); + } + return best; + } + + it.each(Object.entries(HOSTILE))("scans %s in linear time", (_name, build) => { + const small = cost(build(SMALL)); + const large = cost(build(SIZE)); + expect(large).toBeLessThan(MAX_RATIO * Math.max(small, FLOOR_MS)); }); it("still finds a path or secret at the end of a hostile run", () => { diff --git a/packages/contracts/src/fixtures/kit-proposal-policy-v1.json b/packages/contracts/src/fixtures/kit-proposal-policy-v1.json index 7dae5a5..ba0bc12 100644 --- a/packages/contracts/src/fixtures/kit-proposal-policy-v1.json +++ b/packages/contracts/src/fixtures/kit-proposal-policy-v1.json @@ -2,15 +2,19 @@ "schema": "skillx.kit-proposal-policy/v1", "envelope": "skillx.kit-proposal/v1", "forbiddenKeys": { - "description": "A key anywhere in a proposal is normalized, then refused when it equals an exact name or contains a stem. A key longer than maxKeyLength (Unicode code points of the key as received) or not printable ASCII after normalization cannot be checked and is refused too. A key whose value is a number is exempt (an evidence signal such as sessions: 3 cannot carry session content). String values outside the file-content fields that parse as a JSON object or array are checked as nested documents.", + "description": "A key anywhere in a proposal is normalized by the steps below, in order, then refused when it equals an exact name or contains a stem. A key that a step refuses cannot be checked and is refused too. A key whose value is a number is exempt (an evidence signal such as sessions: 3 cannot carry session content). String values outside the file-content fields that parse as a JSON object or array, also through up to percentDecodeRounds layers of JSON strings, are checked as nested documents. A refused key is reported by its path: .key for [A-Za-z0-9_$-]+ keys, otherwise [\"key\"] (the key as a JSON string), [index] for arrays, […] for a key longer than maxKeyLength (never echoed), and #json after a string parsed as JSON.", "normalization": [ - "percent-decode %XX escapes until nothing changes (at most 3 rounds), each escape to the code unit of its byte", - "Unicode NFKC", - "lowercase", + "refuse when the key has more than maxKeyLength Unicode code points, as received", + "percent-decode %XX escapes (each to the code unit of its byte) while any remain, at most percentDecodeRounds rounds; refuse when an escape remains", + "refuse when any character is outside printable ASCII (U+0020 to U+007E) and its fullwidth forms (U+FF01 to U+FF5E)", + "Unicode NFKC (only fullwidth forms change, so the result is the same in every Unicode version since 3.2; unicodeVersion pins the reference)", "refuse when any character is outside printable ASCII (U+0020 to U+007E)", + "lowercase A-Z only", "remove every character outside [a-z0-9]" ], + "unicodeVersion": "15.0", "maxKeyLength": 64, + "percentDecodeRounds": 3, "exact": [ "chat", "chats", @@ -67,6 +71,7 @@ "binary-content", "byte-order-mark", "local-path", + "secret-assignment", "secret-bearer", "secret-dotenv", "secret-jwt", @@ -74,42 +79,96 @@ "secret-private-key", "secret-url-credentials" ], - "localPathSystemAllowlist": { - "roots": [ - "bin", - "sbin", - "etc", - "proc" - ], - "usr": [ - "bin", - "include", - "lib", - "lib32", - "lib64", - "libexec", - "sbin", - "share", - "src" + "secretKeys": { + "description": "Keys of key: value, key = value and \"key\": value assignments in file content, normalized like forbiddenKeys. A key names a password (secret-password, value of 4 or more characters) or a secret (secret-assignment, 8 or more) when its normalized form ends with a stem. On a KEY=value line (optionally after export) a key that is not a password key and holds any stem anywhere is secret-dotenv instead (8 or more). A key that cannot be normalized names a secret. Placeholder values (<...>, %VAR%, {...}, ***, ${...}, a whole $VAR) and plain numbers are not secrets.", + "password": [ + "passphrase", + "passwd", + "password", + "pwd" ], - "usrLocal": [ - "bin", - "cellar", - "etc", - "include", - "lib", - "lib64", - "libexec", - "man", - "opt", - "sbin", - "share" + "secret": [ + "accesskey", + "apikey", + "credential", + "credentials", + "privatekey", + "secret", + "secretkey", + "token" + ] + }, + "localPathSystemAllowlist": { + "description": "A POSIX path of two or more segments is allowed only when, in lower case with every separator run as one / and trailing separators and dots removed, it equals an exact entry or lies inside a subtree entry. A .. segment refuses it.", + "subtrees": [ + "/bin", + "/sbin", + "/usr/bin", + "/usr/include", + "/usr/lib", + "/usr/lib32", + "/usr/lib64", + "/usr/libexec", + "/usr/local/bin", + "/usr/local/include", + "/usr/local/lib", + "/usr/local/lib64", + "/usr/local/libexec", + "/usr/local/sbin", + "/usr/sbin" ], - "dev": [ - "null", - "stderr", - "stdin", - "stdout" + "exact": [ + "/dev/null", + "/dev/random", + "/dev/shm", + "/dev/stderr", + "/dev/stdin", + "/dev/stdout", + "/dev/tty", + "/dev/urandom", + "/dev/zero", + "/etc", + "/etc/apt/sources.list", + "/etc/crontab", + "/etc/environment", + "/etc/fstab", + "/etc/group", + "/etc/hostname", + "/etc/hosts", + "/etc/localtime", + "/etc/nginx/nginx.conf", + "/etc/os-release", + "/etc/passwd", + "/etc/profile", + "/etc/resolv.conf", + "/etc/shadow", + "/etc/shells", + "/etc/ssh/sshd_config", + "/etc/ssl/certs", + "/etc/ssl/certs/ca-certificates.crt", + "/etc/sudoers", + "/etc/timezone", + "/proc/cpuinfo", + "/proc/loadavg", + "/proc/meminfo", + "/proc/mounts", + "/proc/self/limits", + "/proc/self/status", + "/proc/stat", + "/proc/uptime", + "/proc/version", + "/usr", + "/usr/local", + "/usr/local/cellar", + "/usr/local/etc", + "/usr/local/man", + "/usr/local/opt", + "/usr/local/share", + "/usr/share", + "/usr/share/doc", + "/usr/share/fonts", + "/usr/share/man", + "/usr/share/zoneinfo" ] } } diff --git a/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json b/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json index d98c290..23ba3a3 100644 --- a/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json +++ b/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json @@ -86,18 +86,27 @@ "refusedAt": "proposal.api_key" }, { - "name": "dotted", + "name": "dotted (bracket-quoted in the path)", "value": { "api.key": "x" }, - "refusedAt": "proposal.api.key" + "refusedAt": "proposal[\"api.key\"]" }, { "name": "spaced title case", "value": { "Tool Calls": [] }, - "refusedAt": "proposal.Tool Calls" + "refusedAt": "proposal[\"Tool Calls\"]" + }, + { + "name": "brackets in a key stay unambiguous", + "value": { + "a[0]": { + "secret": "x" + } + }, + "refusedAt": "proposal[\"a[0]\"].secret" }, { "name": "stem inside a longer key", @@ -183,49 +192,77 @@ "value": { "%74ranscript": "x" }, - "refusedAt": "proposal.%74ranscript" + "refusedAt": "proposal[\"%74ranscript\"]" }, { "name": "double percent-encoded", "value": { "%2574oken": "x" }, - "refusedAt": "proposal.%2574oken" + "refusedAt": "proposal[\"%2574oken\"]" + }, + { + "name": "an escape left after the last round", + "value": { + "%2525252573ession": "x" + }, + "refusedAt": "proposal[\"%2525252573ession\"]" + }, + { + "name": "a percent sign that is not an escape", + "value": { + "100%": "x" + }, + "refusedAt": null }, { "name": "fullwidth letters (NFKC)", "value": { "ｔｒａｎｓｃｒｉｐｔ": "x" }, - "refusedAt": "proposal.ｔｒａｎｓｃｒｉｐｔ" + "refusedAt": "proposal[\"ｔｒａｎｓｃｒｉｐｔ\"]" + }, + { + "name": "fullwidth upper case", + "value": { + "ＴＯＫＥＮ": "x" + }, + "refusedAt": "proposal[\"ＴＯＫＥＮ\"]" + }, + { + "name": "a compatibility ligature outside the fullwidth forms", + "value": { + "file": "x" + }, + "refusedAt": "proposal[\"file\"]" }, { "name": "zero-width character", "value": { "pro​mpt": "x" }, - "refusedAt": "proposal.pro​mpt" + "refusedAt": "proposal[\"pro​mpt\"]" }, { "name": "Cyrillic homoglyph", "value": { "рrompt": "x" }, - "refusedAt": "proposal.рrompt" + "refusedAt": "proposal[\"рrompt\"]" }, { "name": "any non-ASCII key", "value": { "café": "x" }, - "refusedAt": "proposal.café" + "refusedAt": "proposal[\"café\"]" }, { - "name": "key longer than the limit", + "name": "key longer than the limit (never echoed)", "value": { "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa": "x" }, - "refusedAt": "proposal.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "refusedAt": "proposal[…]" }, { "name": "key at the limit", @@ -298,6 +335,15 @@ }, "refusedAt": "proposal.evidence.summary#json.token" }, + { + "name": "JSON inside a JSON string", + "value": { + "evidence": { + "summary": "\"{\\\"transcript\\\": \\\"x\\\"}\"" + } + }, + "refusedAt": "proposal.evidence.summary#json.transcript" + }, { "name": "text that is not JSON", "value": { @@ -307,6 +353,15 @@ }, "refusedAt": null }, + { + "name": "a quoted word is not JSON", + "value": { + "evidence": { + "summary": "\"small steps\" help" + } + }, + "refusedAt": null + }, { "name": "file content fields are not parsed", "value": { @@ -404,7 +459,33 @@ "////////////////////////////////////////home/kai", "\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\srv01\\share\\x", "x \\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/\\/srv/x", - "chrome-extension://abc/home/kai" + "chrome-extension://abc/home/kai", + "/usr/src/app/config/prod.yml", + "/usr/src/acme-internal/.env", + "/proc/self/root/home/kai/.ssh/id_rsa", + "/proc/1/cwd/x", + "/proc/self/cwd", + "/proc/1/root", + "/etc/letsencrypt/live/acme-internal.com/privkey.pem", + "/etc/acme/app.conf", + "/usr/share/acme/x", + "/usr/share/doc/x", + "/usr/local/share/acme/x", + "/usr/local/etc/acme/x", + "/usr/local/opt/acme", + "/usr/local/opt/node", + "/usr/bin/%2e%2e/%2e%2e/home/kai/.ssh", + "/usr/bin/%252e%252e/%252e%252e/srv/x", + "/​home/kai", + "/ho‍me/kai", + "/Données/x", + "/usr/bin/⁠../../srv/x", + "https://x.io),/home/kai/x", + "https://a.b/c,/Users/kai/x", + "[doc](https://a.b/c]/srv/x", + "ssh://kai@devbox/home/kai/work", + "sftp://host/Users/kai/x", + "git+ssh://git@host/home/git/repo.git" ], "allow": [ "#!/usr/bin/env node", @@ -432,9 +513,8 @@ "-I/usr/include", "-L/usr/local/lib", "/usr/lib/node_modules/x", - "/usr/share/doc/x", "/usr/local", - "/usr/local/opt/node", + "/usr/local/", "https://example.com/usr/home/x", "vscode://ms-python.python", "git+https://github.com/a/b", @@ -443,7 +523,20 @@ "a -b", "a/b//c", "////////////////////////////////////////", - "postgres://db.internal/app" + "postgres://db.internal/app", + "/proc/cpuinfo", + "/etc/ssl/certs/ca-certificates.crt", + "/usr/share/zoneinfo", + "/usr/share/doc", + "/dev/urandom", + "see /etc/hosts.", + "/usr/local/Cellar", + "/usr/local/opt", + "/etc/", + "\"\\/usr\\/bin\\/env\"", + "/usr/lib/x86_64-linux-gnu/libc.so.6", + "(https://example.com/a/b), next", + "/usr/bin/./env" ] }, "secrets": [ @@ -557,10 +650,226 @@ "qqqqqqqqqqqq" ], "rules": [ - "secret-dotenv", "secret-password" ] }, + { + "parts": [ + "SECRET_KEY_BASE=", + "ab12ab12ab12ab12" + ], + "rules": [ + "secret-dotenv" + ] + }, + { + "parts": [ + "export github_token=", + "ab12ab12ab12ab12" + ], + "rules": [ + "secret-dotenv" + ] + }, + { + "parts": [ + "\"dbPassword\": \"", + "hunter", + "22", + "\"" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "adminPassword: ", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "url?smtpPwd=", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "const dbPassword = \"", + "hunter", + "22", + "\";" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "spring.datasource.password=", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "- password: ", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "apiKey: ", + "sk", + "-live-", + "ab12ab12ab12ab12" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "client_secret: ", + "ab12ab12ab12" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "{\"accessToken\": \"", + "ab12ab12ab12", + "\"}" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "GITHUB_TOKEN: ", + "ab12ab12ab12" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "const awsSecretAccessKey = '", + "ab12ab12ab12ab12ab12", + "'" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "let clientCredentials = ", + "ab12ab12ab12" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "password: &a ", + "hunter", + "22", + "secret" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "password: !s ", + "hunter", + "22", + "secret" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "password: $", + "ecretP@ss" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "password: $DB_PASSWORD" + ], + "rules": [] + }, + { + "parts": [ + "token: ${{ secrets.GITHUB_TOKEN }}" + ], + "rules": [] + }, + { + "parts": [ + "apiKey: %API_KEY%" + ], + "rules": [] + }, + { + "parts": [ + "max_tokens: 4096" + ], + "rules": [] + }, + { + "parts": [ + "tokenizer: whitespace" + ], + "rules": [] + }, + { + "parts": [ + "secret_name: my-secret-value" + ], + "rules": [] + }, + { + "parts": [ + "password_file: /run/secrets/db" + ], + "rules": [] + }, + { + "parts": [ + "token: abc" + ], + "rules": [] + }, { "parts": [ "password: " @@ -677,6 +986,28 @@ "22", "\n" ] + }, + { + "parts": [ + "password: &pw\n ", + "hunter", + "22", + "\n" + ] + }, + { + "parts": [ + "password: !secret |\n ", + "hunter", + "22", + "\n" + ] + }, + { + "parts": [ + "password:\n $", + "ecretP@ss\n" + ] } ], "allow": [ diff --git a/packages/contracts/src/personal-kit/proposal-policy.test.ts b/packages/contracts/src/personal-kit/proposal-policy.test.ts index f050275..6c4f851 100644 --- a/packages/contracts/src/personal-kit/proposal-policy.test.ts +++ b/packages/contracts/src/personal-kit/proposal-policy.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import vectors from "../fixtures/kit-proposal-policy-vectors-v1.json"; -import { KIT_PROPOSAL_POLICY, KIT_PROPOSAL_POLICY_SCHEMA, findForbiddenProposalKey, isForbiddenProposalKey, normalizeProposalKey } from "./proposal-policy"; +import { KIT_PROPOSAL_POLICY, KIT_PROPOSAL_POLICY_SCHEMA, findForbiddenProposalKey, isForbiddenProposalKey, normalizeProposalKey, secretKeyKind } from "./proposal-policy"; /** * The shared `skillx.kit-proposal-policy/v1` rules and their vectors, the @@ -47,6 +47,21 @@ describe("forbidden key vectors", () => { expect(normalizeProposalKey("😀".repeat(64))).toBeNull(); }); + it("folds only fullwidth forms through NFKC and pins the Unicode version", () => { + expect(KIT_PROPOSAL_POLICY.forbiddenKeys.unicodeVersion).toBe("15.0"); + expect(normalizeProposalKey("ＴＯＫＥＮ")).toBe("token"); + // A compatibility ligature outside the fullwidth block is refused, not folded. + expect(normalizeProposalKey("file")).toBeNull(); + expect(normalizeProposalKey("%2525252573ession")).toBeNull(); + expect(normalizeProposalKey("100%")).toBe("100"); + }); + + it("writes unambiguous key paths and never echoes a long key", () => { + expect(findForbiddenProposalKey({ "a.b": { transcript: "x" } }, 32)?.path).toBe('proposal["a.b"].transcript'); + expect(findForbiddenProposalKey({ "a[0]": { transcript: "x" } }, 32)?.path).toBe('proposal["a[0]"].transcript'); + expect(findForbiddenProposalKey({ ["t".repeat(65)]: "x" }, 32)?.path).toBe("proposal[…]"); + }); + it("bounds the nesting, also through embedded JSON", () => { let deep: unknown = {}; for (let i = 0; i < 40; i++) deep = { a: deep }; @@ -58,3 +73,23 @@ describe("forbidden key vectors", () => { expect(findForbiddenProposalKey({ summary: text }, 8)).toMatchObject({ reason: "too_deep" }); }); }); + +describe("secret key names of the content rules", () => { + it("names password and secret keys in any case and separator style", () => { + for (const key of ["password", "dbPassword", "DB_PASSWORD", "db-passwd", "userPwd", "sshPassphrase"]) expect(secretKeyKind(key)).toBe("password"); + for (const key of ["apiKey", "API_KEY", "clientSecret", "client_secret", "GITHUB_TOKEN", "accessToken", "awsCredentials", "privateKey"]) expect(secretKeyKind(key)).toBe("secret"); + }); + + it("matches a stem at the end of the key, anywhere only on a dotenv line", () => { + expect(secretKeyKind("tokenizer")).toBeNull(); + expect(secretKeyKind("passwordHint")).toBeNull(); + expect(secretKeyKind("TOKEN_URL")).toBeNull(); + expect(secretKeyKind("TOKEN_URL", true)).toBe("secret"); + expect(secretKeyKind("PASSWORD_FILE", true)).toBe("password"); + }); + + it("treats a key that cannot be normalized as a secret key", () => { + expect(secretKeyKind("pаssword")).toBe("secret"); + expect(secretKeyKind("k".repeat(65))).toBe("secret"); + }); +}); diff --git a/packages/contracts/src/personal-kit/proposal-policy.ts b/packages/contracts/src/personal-kit/proposal-policy.ts index ff596f0..8c6b40b 100644 --- a/packages/contracts/src/personal-kit/proposal-policy.ts +++ b/packages/contracts/src/personal-kit/proposal-policy.ts @@ -1,10 +1,10 @@ /** * `skillx.kit-proposal-policy/v1`: the forbidden-key rules of a - * `skillx.kit-proposal/v1` envelope, shared with AgentKit, which refuses the - * same keys locally before it uploads a proposal. The single source is - * `fixtures/kit-proposal-policy-v1.json`; both sides test against - * `fixtures/kit-proposal-policy-vectors-v1.json`. This module only applies - * the JSON rules, so a list change is a JSON change. + * `skillx.kit-proposal/v1` envelope and the secret-key names of the content + * rules, shared with AgentKit, which refuses the same keys locally before it + * uploads a proposal. The single source is `fixtures/kit-proposal-policy-v1.json`; + * both sides test against `fixtures/kit-proposal-policy-vectors-v1.json`. This + * module only applies the JSON rules, so a list change is a JSON change. */ import policy from "../fixtures/kit-proposal-policy-v1.json"; @@ -15,28 +15,30 @@ const EXACT = new Set(policy.forbiddenKeys.exact); const CONTAINS = policy.forbiddenKeys.contains; const CONTENT_FIELDS = new Set(policy.forbiddenKeys.contentFields); const MAX_KEY_LENGTH = policy.forbiddenKeys.maxKeyLength; +const ROUNDS = policy.forbiddenKeys.percentDecodeRounds; +const ESCAPE = /%[0-9a-f]{2}/i; -/** Decodes `%XX` escapes (each to the code unit of its byte) until nothing changes, at most 3 rounds. */ -function percentDecode(key: string): string { +/** Decodes `%XX` escapes (each to the code unit of its byte) until nothing changes; null when escapes remain after the last round. */ +function percentDecode(key: string): string | null { let out = key; - for (let i = 0; i < 3; i++) { - const next = out.replace(/%([0-9a-f]{2})/gi, (_, h: string) => String.fromCharCode(parseInt(h, 16))); - if (next === out) break; - out = next; - } - return out; + for (let i = 0; i < ROUNDS && ESCAPE.test(out); i++) out = out.replace(/%([0-9a-f]{2})/gi, (_, h: string) => String.fromCharCode(parseInt(h, 16))); + return ESCAPE.test(out) ? null : out; } /** * The comparable form of a key (`[a-z0-9]*`), or null when the key cannot be - * checked: longer than the limit, or not printable ASCII once decoded and - * NFKC-folded (zero-width characters, homoglyphs, any other script). + * checked: longer than the limit, still percent-encoded after the last round, + * or holding a character other than printable ASCII and its fullwidth forms + * (zero-width characters, homoglyphs, any other script). Only fullwidth + * forms reach NFKC, so the result does not depend on the Unicode version. */ export function normalizeProposalKey(key: string): string | null { if ([...key].length > MAX_KEY_LENGTH) return null; - const folded = percentDecode(key).normalize("NFKC").toLowerCase(); + const decoded = percentDecode(key); + if (decoded === null || !/^[\x20-\x7e！-～]*$/.test(decoded)) return null; + const folded = decoded.normalize("NFKC"); if (!/^[\x20-\x7e]*$/.test(folded)) return null; - return folded.replace(/[^a-z0-9]/g, ""); + return folded.replace(/[A-Z]/g, (c) => c.toLowerCase()).replace(/[^a-z0-9]/g, ""); } /** True when a key is refused whatever its (non-numeric) value. */ @@ -45,17 +47,49 @@ export function isForbiddenProposalKey(key: string): boolean { return normal === null || EXACT.has(normal) || CONTAINS.some((stem) => normal.includes(stem)); } +const SECRET_KEYS = policy.content.secretKeys; +const endsWithAny = (key: string, stems: string[]) => stems.some((stem) => key.endsWith(stem)); + +/** + * What a key in file content (`key: value`, `key = value`, `"key": value`) + * names: `"password"` or `"secret"` when its normalized form ends with one of + * the policy stems, `null` otherwise. `dotenv` (a `KEY=value` line) matches a + * stem anywhere in the key. A key that cannot be normalized names a secret. + */ +export function secretKeyKind(key: string, dotenv = false): "password" | "secret" | null { + const normal = normalizeProposalKey(key); + if (normal === null) return "secret"; + const has = dotenv ? (stems: string[]) => stems.some((s) => normal.includes(s)) : (stems: string[]) => endsWithAny(normal, stems); + if (has(SECRET_KEYS.password)) return "password"; + return has(SECRET_KEYS.secret) ? "secret" : null; +} + export type ProposalKeyFinding = { reason: "forbidden"; path: string } | { reason: "too_deep"; path: string }; -/** Parses a string that holds a JSON object or array, or returns undefined. */ +/** Parses a string that holds a JSON object or array (also through JSON string layers), or returns undefined. */ function embeddedJson(text: string): unknown { - const t = text.trimStart(); - if (!t.startsWith("{") && !t.startsWith("[")) return undefined; - try { - return JSON.parse(t) as unknown; - } catch { - return undefined; + let t = text.trimStart(); + for (let layer = 0; layer < ROUNDS + 1; layer++) { + if (!t.startsWith("{") && !t.startsWith("[") && !t.startsWith('"')) return undefined; + let parsed: unknown; + try { + parsed = JSON.parse(t) as unknown; + } catch { + return undefined; + } + if (typeof parsed !== "string") return parsed; + t = parsed.trimStart(); } + return undefined; +} + +/** + * One key of a path: `.key` for `[A-Za-z0-9_$-]+`, otherwise `["key"]` (a + * JSON string), and `[…]` for a key longer than the limit, which is never echoed. + */ +function keySegment(key: string): string { + if (/^[A-Za-z0-9_$-]+$/.test(key) && key.length <= MAX_KEY_LENGTH) return `.${key}`; + return [...key].length > MAX_KEY_LENGTH ? "[…]" : `[${JSON.stringify(key)}]`; } /** @@ -63,7 +97,7 @@ function embeddedJson(text: string): unknown { * value), or the first place it nests deeper than `maxDepth`; null when the * document passes. A key with a number value is exempt. A string outside the * file-content fields that holds a JSON object or array is checked as a - * nested document at `#json`. + * nested document at `#json`. Paths use `.key`, `["key"]` and `[index]`. */ export function findForbiddenProposalKey(value: unknown, maxDepth: number, at = "proposal", depth = 0): ProposalKeyFinding | null { if (typeof value === "object" && value !== null && depth >= maxDepth) return { reason: "too_deep", path: at }; @@ -76,7 +110,7 @@ export function findForbiddenProposalKey(value: unknown, maxDepth: number, at = } if (typeof value !== "object" || value === null) return null; for (const [key, child] of Object.entries(value)) { - const path = `${at}.${key}`; + const path = at + keySegment(key); if (typeof child === "number") continue; if (isForbiddenProposalKey(key)) return { reason: "forbidden", path }; const nested = typeof child === "string" && !CONTENT_FIELDS.has(key) ? embeddedJson(child) : undefined; From 299bbfb02e7b3c0445b558c215a2b8858f9843b7 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Thu, 15 Oct 2026 17:14:16 +0700 Subject: [PATCH 5/8] docs(kits): document the inherited proposal gate and scan rules --- docs/personal-kits-api.md | 111 +++++++++++++++++++++++++------------- 1 file changed, 74 insertions(+), 37 deletions(-) diff --git a/docs/personal-kits-api.md b/docs/personal-kits-api.md index 84a38cd..87e15f4 100644 --- a/docs/personal-kits-api.md +++ b/docs/personal-kits-api.md @@ -159,7 +159,7 @@ Every kit has a `generation`. `GET` returns it, with an `ETag` of `"g"`. | `POST /revisions/:id/reject` `{expectedGeneration}` | Reject a draft. Rejecting the effective revision fails with 409 `kit.revision_effective`. A session-derived proposal is refused with 403 `kit.interactive_approval_required`; it is rejected only through `POST /revisions/:id/review` in a web session. | | `GET /diff?from=&to=` | Read-only structural diff: changed document fields, and added, removed and changed entries with their overlay paths. | | `POST /preview-update` `{updates:[{package,version}], fromRevisionId?, includeForks?}` | Rebase report only. Writes nothing. | -| `POST /rebase` `{expectedGeneration, idempotencyKey, updates, fromRevisionId?, includeForks?}` | Clean: 201, a `draft` revision with origin `rebase` (never activated). Otherwise: 200 `{status:"needs-review", conflictId, report}`. A newer conflict supersedes older open ones. | +| `POST /rebase` `{expectedGeneration, idempotencyKey, updates, fromRevisionId?, includeForks?}` | Clean: 201, a `draft` revision with origin `rebase` (never activated implicitly). Otherwise: 200 `{status:"needs-review", conflictId, report}`. A newer conflict supersedes older open ones. | | `GET /conflicts`, `GET /conflicts/:id` | Conflict records: the report, the resolution and `reevalRequired`. | | `POST /conflicts/:id/resolve` `{expectedGeneration, idempotencyKey, approvedBy:"user", decisions, agentSuggestion?}` | The user's decision becomes a `draft` revision with origin `resolution`. | | `POST /activate`, `POST /rollback` `{expectedGeneration, revisionId, approval:{revisionDigest}}` | Make a revision effective. `rollback` re-activates an earlier effective revision. | @@ -232,7 +232,10 @@ written. ### Activation, rollback and lock Activation requires `approval.revisionDigest` to equal the exact revision -digest, otherwise 409 `kit.approval_required`. Before moving +digest, otherwise 409 `kit.approval_required`. Activating (or rolling back +to) a revision whose document is `public` needs an interactive web session, +whatever its origin: an API key gets 403 `kit.interactive_approval_required`, +so a kit becomes public only by a signed-in owner. Before moving `effective_revision_id`, it re-checks every pinned base of the lineage: - A base that is quarantined, revoked, no longer entitled or removed blocks @@ -261,17 +264,25 @@ proposal is always private. The owner reviews it, then activates it as usual. Nothing activates implicitly. - **Private kits only**: a proposal for a public kit fails with 409 - `proposal.private_kit_required` (fork it to a private kit first), and so - does activating a session-derived revision once the kit has become public. - The kit's visibility is never flipped by a proposal. + `proposal.private_kit_required` (fork it to a private kit first). So does + activating a gated revision (below) while the kit is public, or when its + own document is `public`: the check covers the visibility the kit would + have after activation, not only its current one. The kit's visibility is + never flipped by a proposal. - **A human approves**: review (approve or reject) and activation of a session-derived revision need an interactive web session. An API key, such as the one AgentKit submits with, gets 403 - `kit.interactive_approval_required`. The proofs record `credential`. A - rebase or conflict-resolution draft that carries a session-derived revision - forward (through any chain of rebases and resolutions) inherits the same - gate: it activates only from a web session and only on a private kit. A - manual revision in the chain ends the inheritance. + `kit.interactive_approval_required`. The proofs record `credential`. +- **The gate is inherited**: a revision is gated when it, or any ancestor in + its parent chain, is session-derived, whatever the origins in between + (manual, rebase, resolution). A parent that cannot be found counts as + gated. A gated revision activates (or is rolled back to) only from a web + session, only on a private kit, and only as a private document. + **Consequence**: once a kit has had a session-derived revision, every later + revision descends from it, so every later activation of that kit needs a + web session, and API-key activation and going public stay off for that kit + for good. To publish such work, start a new kit from a document without + that history. - **No chaining on unreviewed proposals**: a from-revision that is `proposed` (or any session-derived revision that is not `reviewed`) fails with 409 `kit.review_required`, and an unreviewed proposal is never usable as a @@ -317,11 +328,20 @@ verifies the result like any revision. messages, conversation, environment, secret or token, working directory, history, tool output or logs, at any depth, fails with 422 `proposal.forbidden_field`. The message names the key path, never its value. - Keys are compared after percent-decoding, NFKC and lowercasing, with - separators removed (`Session_Log`, `api-key`, `%70assword`, full-width - letters). A key longer than 64 code points or with a character outside - printable ASCII is refused. A string that holds a JSON object or array - (outside `contentBase64` and `patch`) is checked as nested keys. A key with + A path is `proposal`, then `.key` for a key of `[A-Za-z0-9_$-]`, `["key"]` + (a JSON string) for any other key, so `.` and `[` in a key are never + ambiguous, `[index]` for an array item, `#json` for a JSON string read as + nested keys, and `[…]` for a key over the length limit, which is never + echoed. Keys are compared in this order: refused when longer than 64 code + points; percent-decoded up to 3 rounds (refused when an escape remains); + refused unless every character is printable ASCII or its fullwidth form + (U+FF01 to U+FF5E); NFKC (Unicode 15.0, pinned in the policy; only the + fullwidth forms change, so the result does not depend on the runtime's + Unicode version); refused unless printable ASCII; `A-Z` lowercased; + everything but `a-z0-9` removed (`Session_Log`, `api-key`, `%70assword`, + fullwidth letters). A string that holds a JSON object or array, also + inside JSON string layers (outside `contentBase64` and `patch`), is + checked as nested keys. A key with a numeric value (an evidence signal) is allowed. Unknown fields are refused. An unknown top-level body field is 400 `invalid_request`. The key lists and the content rules are the shared policy `skillx.kit-proposal-policy/v1` in @@ -337,28 +357,45 @@ verifies the result like any revision. composition (the lines a changed file gains, a new file whole), plus the evidence summary as `evidence/summary.md`. Lines kept from the base are not scanned, so a finding already in the base never refuses a proposal. - - Fails with 422 `proposal.content_refused`, nothing stored: a secret - (vendor token formats, `password=`/`passwd:` assignments, a YAML - `password:` key whose new value sits on the following indented lines or in - a `|` / `>` block scalar, also as a list item `- password:` or a quoted - key `"password":`, even when the key line is kept from the base, - Bearer tokens, URLs with `user:pass@`, JWTs, private key blocks, - dotenv-style secret keys), binary content, text with a byte order mark - (UTF-8/16/32), and every absolute local path, matched case-insensitively - and also JSON-escaped (`\/`, `\\`) or percent-encoded (`%2F`, `%5C`, - `%3A`): any drive-letter path, UNC shares (`\\host\share` and - `//host/share`), `file:` and `smb:` URLs, editor links to a file - (`vscode://file/...`), a bare `/home`, `/Users`, `/root`, `/mnt`, - `/media`, `/Volumes`, `/var/folders`, `/tmp`, `/private` or `/run/user`, - a path glued to a flag (`-I/opt/x`, `--include=/opt/x`), and any other - POSIX path of two or more segments in any script (such as `/opt/...`, - `/srv/...`, `/var/www/...`, `/Usuários/...`). Only these system paths are - allowed: `/bin/`, `/sbin/`, `/etc/`, `/proc/`, the system directories of - `/usr/` and `/usr/local/` (`bin`, `lib`, `share`, `include` and the others - listed in the policy; `/usr/home` and `/usr/local/` are refused), - `/dev/null`, `/dev/stdin`, `/dev/stdout` and `/dev/stderr` (a `..` - segment that climbs out of them is refused). URLs of network schemes - (`https://host/path`, `ssh://`, `s3://`, ...) are not local paths. + - Fails with 422 `proposal.content_refused`, nothing stored: a secret, + binary content, text with a byte order mark (UTF-8/16/32), or an + absolute local path. + - Secrets: vendor token formats; a `key: value`, `key = value` or + `"key": "value"` assignment (YAML, JSON, `.env`, code) whose key ends with + a password stem (`secret-password`: `password`, `dbPassword`, + `DB_PASSWD`, a value of 4 or more characters) or a secret stem + (`secret-assignment`: `clientSecret`, `apiKey`, `accessToken`, + `awsCredentials`, a value of 8 or more characters), in any case and + separator style, compared through the same key normalization as the + envelope (the stems are `content.secretKeys` in the policy); a + `KEY=value` line whose key holds a stem anywhere (`secret-dotenv`); a + YAML `password:` key whose new value sits on the following indented lines + or in a `|` / `>` block scalar, also as a list item `- password:`, a + quoted key `"password":`, or after an anchor or tag (`&pw`, `!secret`), + even when the key line is kept from the base; Bearer tokens, URLs with + `user:pass@`, JWTs and private key blocks. A YAML anchor or tag before an + inline value is skipped. Placeholders (`<...>`, `%VAR%`, `{...}`, `***`, + `${...}`, a whole `$VAR`) and numbers are not secrets; any other value, + also one that starts with `$`, is. + - Paths: every absolute local path, matched case-insensitively, after NFC + normalization and with invisible format characters (zero-width spaces + and joiners) removed, and also JSON-escaped (`\/`, `\\`) or + percent-encoded (`%2F`, `%5C`, `%3A`, `%2E`): any drive-letter path, UNC + shares (`\\host\share` and `//host/share`), `file:` and `smb:` URLs, + editor links to a file (`vscode://file/...`), a user home in an `ssh` or + `sftp` URL, a bare `/home`, `/Users`, `/root`, `/mnt`, `/media`, + `/Volumes`, `/var/folders`, `/tmp`, `/private` or `/run/user`, a path + glued to a flag (`-I/opt/x`, `--include=/opt/x`), and any other POSIX + path of two or more segments in any script (such as `/opt/...`, + `/srv/...`, `/var/www/...`, `/usr/src/...`, `/proc/self/root/...`, + `/Usuários/...`). Only the policy's `localPathSystemAllowlist` is allowed: + whole `subtrees` of binary and library directories (`/usr/bin/...`, + `/usr/lib/...`, `/usr/local/bin/...`), and `exact` well-known + directories and files, matched as a whole (`/etc/hosts`, `/proc/cpuinfo`, + `/dev/null`, `/usr/share/zoneinfo`; not `/usr/share/doc/x` or + `/usr/local/opt/x`). A `..` segment refuses the path. URLs of network + schemes (`https://host/path`, `s3://`, ...) are not local paths; a path + glued after a URL's `,`, `)` or `]` is still read. `error.findings` lists `danger::` codes, never the matched text. Every detector runs in time linear in its input, so a hostile request body cannot exhaust the Worker CPU. From 17cc6627114063edb428e10068755eb85572a1b3 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Thu, 15 Oct 2026 17:37:10 +0700 Subject: [PATCH 6/8] fix(kits): follow the document lineage in the session activation gate Check the history of every parent revision the lock lineage loads, so a kit whose document parent is a session-derived revision activates only from a web session. A new kit always starts private. --- .../app/lib/personal-kits/kit-lifecycle.ts | 21 ++++++++++----- .../app/lib/personal-kits/kit-revisions.ts | 8 ++++-- apps/web/app/lib/personal-kits/kit-verify.ts | 8 ++++-- .../kit-proposal-activation.test.ts | 26 +++++++++++++++++-- 4 files changed, 51 insertions(+), 12 deletions(-) diff --git a/apps/web/app/lib/personal-kits/kit-lifecycle.ts b/apps/web/app/lib/personal-kits/kit-lifecycle.ts index 872a922..6a9cc15 100644 --- a/apps/web/app/lib/personal-kits/kit-lifecycle.ts +++ b/apps/web/app/lib/personal-kits/kit-lifecycle.ts @@ -4,8 +4,9 @@ * exact `revisionDigest`, re-checks every base of the effective lineage and * moves `effective_revision_id` under the generation guard; nothing becomes * effective implicitly. A proposal (`proposed`) must be reviewed first, and - * a revision with a session-derived revision anywhere in its parent chain - * (any origin; an unreadable parent counts) activates only on a private kit, + * a revision with a session-derived revision anywhere in its history (its + * revision chain, any origin, and the revision chain of every parent revision + * its document lineage names; an unreadable parent counts) activates only on a private kit, * as a private document, and only from an interactive web session. Any * activation that makes a kit public needs a web session too. A quarantined revision anywhere in the chain * fails the lock with `kit.locked_incompatible`. @@ -22,6 +23,7 @@ import type { KitCaller } from "./kit-http"; import { commitKitMutation, documentOf, getRevision, ownedKit, writableKit } from "./kit-store"; import type { KitRow, RevisionRow } from "./kit-store"; import { chainView, loadChain } from "./kit-verify"; +import type { LoadedChain } from "./kit-verify"; import { generationConflict } from "./kit-revisions"; const ACTIVATABLE = new Set(["draft", "reviewed"]); @@ -29,6 +31,8 @@ const ACTIVATABLE = new Set(["draft", "reviewed"]); interface LockCheck { /** Null when the lineage is incomplete; `reasons` then says why. */ lock: Awaited> | null; + /** The parent revisions the document lineage loaded. */ + parents: LoadedChain["parents"]; documents: unknown[]; reasons: string[]; warnings: string[]; @@ -38,7 +42,7 @@ interface LockCheck { export async function checkLock(caller: KitCaller, rev: RevisionRow): Promise { const reasons: string[] = rev.state === "quarantined" ? ["the revision is quarantined"] : []; const loaded = await loadChain(caller, documentOf(rev), reasons); - if (loaded.incomplete) return { lock: null, documents: [], reasons, warnings: [] }; + if (loaded.incomplete) return { lock: null, parents: loaded.parents, documents: [], reasons, warnings: [] }; const { chain } = loaded; const warnings: string[] = []; for (const { entry } of resolveLineage(chain)) { @@ -52,7 +56,7 @@ export async function checkLock(caller: KitCaller, rev: RevisionRow): Promise 0) return incompatible(check.reasons); const at = caller.now.getTime(); // A reviewed proposal keeps its review proof next to the activation approval. diff --git a/apps/web/app/lib/personal-kits/kit-revisions.ts b/apps/web/app/lib/personal-kits/kit-revisions.ts index 2483840..7be2d22 100644 --- a/apps/web/app/lib/personal-kits/kit-revisions.ts +++ b/apps/web/app/lib/personal-kits/kit-revisions.ts @@ -77,7 +77,11 @@ export async function readRevision(caller: KitCaller, kitName: string, id: strin return json(revisionView(await getRevision(caller.env.DB, kit.id, id), true)); } -/** Validates, verifies and stores one revision under the generation guard. */ +/** + * Validates, verifies and stores one revision under the generation guard. A + * new kit always starts private: only an activation sets its visibility, so + * an API key never creates a public kit. + */ export async function storeRevision(caller: KitCaller, req: StoreRequest): Promise<{ revision: RevisionRow; generation: number; warnings: string[]; replayed: boolean }> { const db = caller.env.DB; const existing = await findKit(db, req.kitName); @@ -107,7 +111,7 @@ export async function storeRevision(caller: KitCaller, req: StoreRequest): Promi const ok = await commitKitMutation(db, { kitId, generation: req.expectedGeneration, - create: existing === null ? { name: req.kitName, publisherId, ownerUserId: caller.userId, localId: doc.localId ?? null, visibility: doc.visibility } : undefined, + create: existing === null ? { name: req.kitName, publisherId, ownerUserId: caller.userId, localId: doc.localId ?? null, visibility: "private" } : undefined, statements: [ { sql: `INSERT INTO personal_kit_revisions (id, kit_id, revision_number, parent_revision_id, revision_digest, document_json, state, origin, diff --git a/apps/web/app/lib/personal-kits/kit-verify.ts b/apps/web/app/lib/personal-kits/kit-verify.ts index e993263..5171a54 100644 --- a/apps/web/app/lib/personal-kits/kit-verify.ts +++ b/apps/web/app/lib/personal-kits/kit-verify.ts @@ -62,6 +62,8 @@ export interface LoadedChain { foreign: Set; /** A parent was unavailable (reported in `reasons`), so `chain` stops before it. */ incomplete: boolean; + /** The parent revision rows behind `chain[1..]`, with the id of their kit. */ + parents: Array<{ kitId: string; row: RevisionRow }>; } /** @@ -76,6 +78,7 @@ export async function loadChain(caller: KitCaller, doc: PersonalKitDocument, rea const { env, userId } = caller; const chain = [doc]; const foreign = new Set(); + const parents: LoadedChain["parents"] = []; let parentPublic = true; for (let cur = doc; cur.parent !== undefined; cur = chain[chain.length - 1]!) { if (chain.length >= MAX_LINEAGE_DEPTH || chain.some((d, i) => i > 0 && d.kit === cur.parent!.kit) || cur.parent.kit === doc.kit) { @@ -92,14 +95,15 @@ export async function loadChain(caller: KitCaller, doc: PersonalKitDocument, rea const detail = `parent revision ${cur.parent.revisionDigest} of ${cur.parent.kit} is not available`; if (reasons === undefined) throw kitError(KIT_CODES.baseUnavailable, "parent", detail); reasons.push(detail); - return { chain, parentPublic: false, foreign, incomplete: true }; + return { chain, parentPublic: false, foreign, incomplete: true, parents }; } if (kit!.visibility !== "public") parentPublic = false; if (!owned) foreign.add(chain.length); + parents.push({ kitId: kit!.id, row: row! }); chain.push(documentOf(row!)); } resolveLineage(chain); - return { chain, parentPublic, foreign, incomplete: false }; + return { chain, parentPublic, foreign, incomplete: false, parents }; } /** The chain as returned to the caller: own documents in full, foreign ancestors as digest references only. */ diff --git a/apps/web/tests/personal-kits/kit-proposal-activation.test.ts b/apps/web/tests/personal-kits/kit-proposal-activation.test.ts index 0986118..aa58a6a 100644 --- a/apps/web/tests/personal-kits/kit-proposal-activation.test.ts +++ b/apps/web/tests/personal-kits/kit-proposal-activation.test.ts @@ -1,7 +1,8 @@ /** * Activation gate of session-derived content. A revision with a - * session-derived revision anywhere in its parent chain, whatever its own - * origin (manual, rebase, resolution), or with a parent that cannot be read, + * session-derived revision anywhere in its history, whatever its own origin + * (manual, rebase, resolution), also through the revision of a parent kit + * its document names, or with a parent that cannot be read, * activates (and rolls back) only from a web session and only as a private * document on a private kit. Any activation that makes a kit public needs a * web session too; other private activations keep working with an API key. @@ -138,6 +139,18 @@ describe("revisions derived from a proposal", () => { k.sql("UPDATE personal_kit_revisions SET parent_revision_id = ? WHERE id = ?", "pkr_00000000-0000-0000-0000-000000000000", manual.id); gated(await k.call("activate", activation(res.body.revision, res.body.generation))); }); + + it("gates a kit whose document parent is a session-derived revision of another kit", async () => { + const { rev } = await reviewedProposal(); + const CHILD = { kit: "@alpha/child" }; + const document = k.doc([], { kit: CHILD.kit, localId: "local/child", parent: { kit: "@alpha/my-kit", revisionDigest: rev.digest } }); + const stored = await k.call("revisions", { expectedGeneration: 0, idempotencyKey: k.idem(), document }, CHILD); + expect(stored.status).toBe(201); + const child = stored.body.revision as Rev; + gated(await k.call("activate", activation(child, 1), CHILD)); + const byWeb = await k.call("activate", activation(child, 1), { ...WEB, ...CHILD }); + expect(byWeb.status).toBe(200); + }); }); describe("activations without session-derived content", () => { @@ -154,4 +167,13 @@ describe("activations without session-derived content", () => { await expectWebOnly(pub.rev, pub.generation); expect((await k.call("")).body.visibility).toBe("public"); }); + + it("creates every kit private, also from a public first document", async () => { + const CHILD = { kit: "@alpha/child" }; + const document = k.doc([await k.entry("@alpha/demo", "1.0.0", MINE)], { kit: CHILD.kit, localId: "local/child", visibility: "public" }); + const res = await k.call("revisions", { expectedGeneration: 0, idempotencyKey: k.idem(), document }, CHILD); + expect(res.status).toBe(201); + expect((await k.call("", undefined, CHILD)).body.visibility).toBe("private"); + gated(await k.call("activate", activation(res.body.revision, 1), CHILD)); + }); }); From e974a38591286d1e8ba7854b54d412959e93c1af Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Thu, 15 Oct 2026 17:37:10 +0700 Subject: [PATCH 7/8] fix(kits): read assignments inside values and flags in the secret scan Read an assignment value in a bounded lookahead so keys inside values and after flags are found, accept := and =>, and allow up to two qualifiers after a secret stem. Read Unicode slashes, refuse escapes left after three decoding rounds, and stop URLs at more delimiters. --- .../lib/personal-kits/proposal-detectors.ts | 20 +- .../personal-kits/proposal-path-detector.ts | 28 ++- .../proposal-detector-cost.test.ts | 8 + .../src/fixtures/kit-proposal-policy-v1.json | 15 +- .../kit-proposal-policy-vectors-v1.json | 211 +++++++++++++++++- .../src/personal-kit/proposal-policy.test.ts | 5 + .../src/personal-kit/proposal-policy.ts | 19 +- 7 files changed, 280 insertions(+), 26 deletions(-) diff --git a/apps/web/app/lib/personal-kits/proposal-detectors.ts b/apps/web/app/lib/personal-kits/proposal-detectors.ts index 93e91b4..40d4352 100644 --- a/apps/web/app/lib/personal-kits/proposal-detectors.ts +++ b/apps/web/app/lib/personal-kits/proposal-detectors.ts @@ -22,17 +22,21 @@ export { hasLocalPath } from "./proposal-path-detector"; * a whole `$VAR`. Any other value, also one that starts with `$`, is real. */ const PLACEHOLDER = String.raw`(?:[<%{*]|\$\{|\$[A-Z_][A-Z0-9_]*(?![^\s"'<>]))`; -/** An optional YAML anchor or tag before the value (`&a`, `!secret`). */ -const ANCHOR = String.raw`(?:[&!][^\s"'<>]{0,64}[ \t]+)?`; -/** The same before a block value, where the anchor or tag may end the key line. */ -const BLOCK_ANCHOR = String.raw`(?:[&!][^\s"'<>]{0,64}[ \t]*)?`; +/** Up to two YAML anchors or tags before the value (`&a`, `!secret`, `&a !s`). */ +const ANCHOR = String.raw`(?:[&!][^\s"'<>]{0,64}[ \t]+){0,2}`; +/** The same before a block value, where an anchor or tag may end the key line. */ +const BLOCK_ANCHOR = String.raw`(?:[&!][^\s"'<>]{0,64}[ \t]*){0,2}`; /** - * `key value` on one line: a key of at most 128 characters, optionally - * quoted, at a key boundary; group 1 is the key, 2 the separator, 3 the - * value. A key past the 64-character policy limit counts as a secret key. + * `key value` on one line (`:`, `=`, `:=`, `=>`): a key of at most 128 + * characters, optionally quoted or in brackets (`cfg["password"]`), at a key + * boundary (also after `--` of a flag); group 1 is the key, 2 the separator, + * 3 the value. The value (at most 256 characters) is read in a lookahead, so + * the scan resumes right after the separator and finds a key inside a value + * (`Server=db;Password=...`, `?user=a&password=...`), and each start reads a + * bounded window. A key past the 64-character policy limit counts as a secret key. */ const ASSIGNMENT = new RegExp( - String.raw`(?]+)`, + String.raw`(?|[:=])[ \t]*${ANCHOR}["']?(?=(?!${PLACEHOLDER})([^\s"'<>]{1,256}))`, "g", ); /** A `KEY=value` (or `export KEY=value`) line start before the key at the end of this window. */ diff --git a/apps/web/app/lib/personal-kits/proposal-path-detector.ts b/apps/web/app/lib/personal-kits/proposal-path-detector.ts index 6ff0127..af50770 100644 --- a/apps/web/app/lib/personal-kits/proposal-path-detector.ts +++ b/apps/web/app/lib/personal-kits/proposal-path-detector.ts @@ -1,9 +1,11 @@ /** * Absolute local path detector for session-derived content. Every absolute * local path is refused, in plain, JSON-escaped (`\/`, `\\`) and - * percent-encoded (`%2F`, `%5C`, `%3A`, `%2E`) form, case-insensitively, - * after NFC normalization and with invisible format characters (zero-width - * spaces and joiners) removed: a drive-letter path, a UNC share (`\\host\share` + * percent-encoded (`%2F`, `%5C`, `%3A`, `%2E`, up to three rounds; an + * encoded separator or dot left after them refuses the text) form, + * case-insensitively, after NFC normalization, with Unicode slash forms + * (`／`, `∕`, `⁄`, `＼`) read as separators and invisible format characters + * (zero-width spaces and joiners) removed: a drive-letter path, a UNC share (`\\host\share` * or `//host/share`), a `file:` or `smb:` URL, an editor link to a file * (`vscode://file/...`), a user home in an `ssh`/`sftp` URL, a bare user, * temp, mount or volume root (`/home`, `/Users`, `/tmp`, ...), a path glued to @@ -60,21 +62,28 @@ const SUBTREES = ALLOW.subtrees; /** A URL of a network scheme; its path is not a local path. Other schemes (`vscode:`, `chrome-extension:`) keep theirs. */ const WEB_URL = - /(?`,)\]]*/gi; + /(?`,;|&{}()[\]]*/gi; /** A short flag glued to a path (`-I/opt/x`, `-isystem/opt/x`), after a word boundary of a command line. */ const GLUED_FLAG = /(? text.replace(/[\uff0f\u2215\u2044\u29f8]/g, "/").replace(/[\uff3c\u2216\u29f9]/g, "\\"); + /** Shortens every run of more than 8 separators to its first and last 4, which every pattern reads the same way. */ const collapseSeparators = (text: string) => text.replace(/[\\/]{9,}/g, (run) => run.slice(0, 4) + run.slice(-4)); @@ -91,12 +100,13 @@ function allowedSystemPath(run: string): boolean { /** The text as the detector reads it: NFC, no format characters, decoded separators, collapsed runs, flags apart. */ const prepare = (text: string) => - collapseSeparators(decodeSeparators(text.replace(/\p{Cf}/gu, "").normalize("NFC"))).replace(GLUED_FLAG, "$1 "); + collapseSeparators(decodeSeparators(unicodeSlashes(text.replace(/\p{Cf}/gu, "").normalize("NFC")))).replace(GLUED_FLAG, "$1 "); /** True when `text` names an absolute local path. */ export function hasLocalPath(text: string): boolean { const decoded = prepare(text); - if (LOCAL_PATHS.some((re) => re.test(decoded))) return true; + // Still encoded after the last round: the path cannot be read, so it is refused. + if (ENCODED.test(decoded) || LOCAL_PATHS.some((re) => re.test(decoded))) return true; const plain = decoded.replace(WEB_URL, " "); let runEnd = -1; for (const m of plain.matchAll(POSIX_PATH)) { diff --git a/apps/web/tests/personal-kits/proposal-detector-cost.test.ts b/apps/web/tests/personal-kits/proposal-detector-cost.test.ts index c1bfc93..9452ec4 100644 --- a/apps/web/tests/personal-kits/proposal-detector-cost.test.ts +++ b/apps/web/tests/personal-kits/proposal-detector-cost.test.ts @@ -44,6 +44,14 @@ const HOSTILE: Record = { "secret key words": unit("TOKEN"), "password words": unit("password"), "password assignments": unit("password: "), + "chained assignments": unit("a="), + "flag-like assignments": unit("-a-a-a="), + "quoted key chains": unit('"k":'), + "long keys": unit("k".repeat(128) + "="), + "long secret values": unit("token=" + "v".repeat(300) + ";"), + "anchors and tags": unit("password: &a !s "), + "encoded dot layers": unit("%25"), + "Unicode slashes": unit("\uff0fa"), "credential URLs": unit("x://a:"), "YAML password keys": unit("password:\n"), "YAML list password keys": unit("- password:\n \n"), diff --git a/packages/contracts/src/fixtures/kit-proposal-policy-v1.json b/packages/contracts/src/fixtures/kit-proposal-policy-v1.json index ba0bc12..dac698e 100644 --- a/packages/contracts/src/fixtures/kit-proposal-policy-v1.json +++ b/packages/contracts/src/fixtures/kit-proposal-policy-v1.json @@ -80,7 +80,7 @@ "secret-url-credentials" ], "secretKeys": { - "description": "Keys of key: value, key = value and \"key\": value assignments in file content, normalized like forbiddenKeys. A key names a password (secret-password, value of 4 or more characters) or a secret (secret-assignment, 8 or more) when its normalized form ends with a stem. On a KEY=value line (optionally after export) a key that is not a password key and holds any stem anywhere is secret-dotenv instead (8 or more). A key that cannot be normalized names a secret. Placeholder values (<...>, %VAR%, {...}, ***, ${...}, a whole $VAR) and plain numbers are not secrets.", + "description": "Keys of key: value, key = value and \"key\": value assignments in file content, normalized like forbiddenKeys. A key names a password (secret-password, value of 4 or more characters) or a secret (secret-assignment, 8 or more) when its normalized form holds a stem followed by at most two qualifiers (or by nothing): secretkeybase, apikeyprod, privatekeyid and clientsecretv2 are secret keys, secretname and tokenizer are not. A qualifier is one of qualifiers, or a number with an optional v. Separators are :, =, := and =>. On a KEY=value line (optionally after export) a key that is not a password key and holds any stem anywhere is secret-dotenv instead (8 or more). A key that cannot be normalized names a secret. Placeholder values (<...>, %VAR%, {...}, ***, ${...}, a whole $VAR) and plain numbers are not secrets.", "password": [ "passphrase", "passwd", @@ -96,6 +96,19 @@ "secret", "secretkey", "token" + ], + "qualifiers": [ + "base", + "dev", + "hash", + "id", + "key", + "live", + "old", + "prod", + "staging", + "test", + "value" ] }, "localPathSystemAllowlist": { diff --git a/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json b/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json index 23ba3a3..b3d3ea0 100644 --- a/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json +++ b/packages/contracts/src/fixtures/kit-proposal-policy-vectors-v1.json @@ -485,7 +485,18 @@ "[doc](https://a.b/c]/srv/x", "ssh://kai@devbox/home/kai/work", "sftp://host/Users/kai/x", - "git+ssh://git@host/home/git/repo.git" + "git+ssh://git@host/home/git/repo.git", + "／home／kai／.ssh", + "∕srv∕acme∕x", + "⁄opt⁄acme⁄x", + "C:＼Users＼kai", + "/usr/bin/%25252e%25252e/home/kai/.ssh", + "/usr/bin/%2525252e%2525252e/srv/x", + "https://x.io;/srv/acme-internal/x", + "https://x.io|/opt/acme/secret.yml", + "https://x.io}/srv/x/y", + "https://x.io?a=1&b=/srv/acme/x", + "https://x.io{/opt/acme/x" ], "allow": [ "#!/usr/bin/env node", @@ -791,6 +802,156 @@ "secret-assignment" ] }, + { + "parts": [ + "Server=db;User Id=sa;Password=", + "hunter", + "22", + "!;" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "https://api.x/login?user=kai&password=", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "a=1,password=", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "mysql -u root --password=", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "--client-secret=", + "ab12ab12ab12" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "curl --api-key=", + "ab12ab12ab12" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "secret_key_base: ", + "ab12ab12ab12ab12ab12ab12ab12ab12" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "api_key_prod: ", + "ab12ab12ab12" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "\"private_key_id\": \"", + "ab12ab12ab12ab12ab12", + "\"" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "client_secret_v2: ", + "ab12ab12ab12" + ], + "rules": [ + "secret-assignment" + ] + }, + { + "parts": [ + "passwordValue: ", + "hunter", + "22" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "password := \"", + "hunter", + "22", + "\"" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "'password' => '", + "hunter", + "22", + "'" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "cfg[\"password\"] = \"", + "hunter", + "22", + "\"" + ], + "rules": [ + "secret-password" + ] + }, + { + "parts": [ + "password: &a !s ", + "hunter", + "22", + "secret" + ], + "rules": [ + "secret-password" + ] + }, { "parts": [ "password: &a ", @@ -911,6 +1072,54 @@ "AUTHOR_NAME=Jonathan" ], "rules": [] + }, + { + "parts": [ + "secretName: db-credentials-ref" + ], + "rules": [] + }, + { + "parts": [ + "tokenizerName: cl100k_base_x" + ], + "rules": [] + }, + { + "parts": [ + "passwordless: enabled" + ], + "rules": [] + }, + { + "parts": [ + "passwordPolicy: strict-mode" + ], + "rules": [] + }, + { + "parts": [ + "token_count: many-many" + ], + "rules": [] + }, + { + "parts": [ + "--password-file=secrets.txt" + ], + "rules": [] + }, + { + "parts": [ + "secretsManager: aws-secrets" + ], + "rules": [] + }, + { + "parts": [ + "a=a=a=a=a=a=a=a" + ], + "rules": [] } ], "multilinePasswords": { diff --git a/packages/contracts/src/personal-kit/proposal-policy.test.ts b/packages/contracts/src/personal-kit/proposal-policy.test.ts index 6c4f851..67586be 100644 --- a/packages/contracts/src/personal-kit/proposal-policy.test.ts +++ b/packages/contracts/src/personal-kit/proposal-policy.test.ts @@ -88,6 +88,11 @@ describe("secret key names of the content rules", () => { expect(secretKeyKind("PASSWORD_FILE", true)).toBe("password"); }); + it("allows at most two qualifiers after a stem", () => { + for (const key of ["secret_key_base", "api_key_prod", "private_key_id", "client_secret_v2", "passwordValue", "API_KEY_PROD_V2"]) expect(secretKeyKind(key)).not.toBeNull(); + for (const key of ["secretName", "secretsManager", "passwordless", "passwordPolicy", "token_count", "api_key_prod_v2_old"]) expect(secretKeyKind(key)).toBeNull(); + }); + it("treats a key that cannot be normalized as a secret key", () => { expect(secretKeyKind("pаssword")).toBe("secret"); expect(secretKeyKind("k".repeat(65))).toBe("secret"); diff --git a/packages/contracts/src/personal-kit/proposal-policy.ts b/packages/contracts/src/personal-kit/proposal-policy.ts index 8c6b40b..91495ce 100644 --- a/packages/contracts/src/personal-kit/proposal-policy.ts +++ b/packages/contracts/src/personal-kit/proposal-policy.ts @@ -48,20 +48,25 @@ export function isForbiddenProposalKey(key: string): boolean { } const SECRET_KEYS = policy.content.secretKeys; -const endsWithAny = (key: string, stems: string[]) => stems.some((stem) => key.endsWith(stem)); +/** A stem followed by at most two qualifiers (`base`, `prod`, `id`, `v2`, ...) at the end of a normalized key. */ +const stemAtEnd = (stems: string[]) => new RegExp(`(?:${stems.join("|")})(?:${SECRET_KEYS.qualifiers.join("|")}|v?[0-9]+){0,2}$`); +const PASSWORD_KEY = stemAtEnd(SECRET_KEYS.password); +const SECRET_KEY = stemAtEnd(SECRET_KEYS.secret); /** * What a key in file content (`key: value`, `key = value`, `"key": value`) - * names: `"password"` or `"secret"` when its normalized form ends with one of - * the policy stems, `null` otherwise. `dotenv` (a `KEY=value` line) matches a - * stem anywhere in the key. A key that cannot be normalized names a secret. + * names: `"password"` or `"secret"` when its normalized form holds one of the + * policy stems followed by nothing or by at most two qualifiers + * (`secret_key_base`, `api_key_prod`, `private_key_id`), `null` otherwise + * (`secretName`, `tokenizer`). `dotenv` (a `KEY=value` line) matches a stem + * anywhere in the key. A key that cannot be normalized names a secret. */ export function secretKeyKind(key: string, dotenv = false): "password" | "secret" | null { const normal = normalizeProposalKey(key); if (normal === null) return "secret"; - const has = dotenv ? (stems: string[]) => stems.some((s) => normal.includes(s)) : (stems: string[]) => endsWithAny(normal, stems); - if (has(SECRET_KEYS.password)) return "password"; - return has(SECRET_KEYS.secret) ? "secret" : null; + const names = (stems: string[], atEnd: RegExp) => (dotenv ? stems.some((s) => normal.includes(s)) : atEnd.test(normal)); + if (names(SECRET_KEYS.password, PASSWORD_KEY)) return "password"; + return names(SECRET_KEYS.secret, SECRET_KEY) ? "secret" : null; } export type ProposalKeyFinding = { reason: "forbidden"; path: string } | { reason: "too_deep"; path: string }; From 4d77e700819315c38b7ce60f686c908d2e712ec1 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Thu, 15 Oct 2026 17:37:10 +0700 Subject: [PATCH 8/8] docs(kits): describe what the session gate follows --- docs/personal-kits-api.md | 57 ++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/docs/personal-kits-api.md b/docs/personal-kits-api.md index 87e15f4..5f79a70 100644 --- a/docs/personal-kits-api.md +++ b/docs/personal-kits-api.md @@ -155,7 +155,7 @@ Every kit has a `generation`. `GET` returns it, with an `ETag` of `"g"`. |---|---| | `GET` | Kit summary: `kit`, `localId`, `visibility`, `generation`, `latestRevisionNumber`, `effectiveRevision`, `openConflicts`. | | `GET /revisions`, `GET /revisions/:id` | Revision list, or one revision with its `document`. | -| `POST /revisions` `{expectedGeneration, idempotencyKey, document}` | Store a `draft` revision with origin `manual`. 201 `{revision, generation, replayed, warnings}`. The first write creates the kit. | +| `POST /revisions` `{expectedGeneration, idempotencyKey, document}` | Store a `draft` revision with origin `manual`. 201 `{revision, generation, replayed, warnings}`. The first write creates the kit, always `private`: only an activation sets the kit's visibility, so an API key never creates a public kit. | | `POST /revisions/:id/reject` `{expectedGeneration}` | Reject a draft. Rejecting the effective revision fails with 409 `kit.revision_effective`. A session-derived proposal is refused with 403 `kit.interactive_approval_required`; it is rejected only through `POST /revisions/:id/review` in a web session. | | `GET /diff?from=&to=` | Read-only structural diff: changed document fields, and added, removed and changed entries with their overlay paths. | | `POST /preview-update` `{updates:[{package,version}], fromRevisionId?, includeForks?}` | Rebase report only. Writes nothing. | @@ -274,15 +274,22 @@ Nothing activates implicitly. as the one AgentKit submits with, gets 403 `kit.interactive_approval_required`. The proofs record `credential`. - **The gate is inherited**: a revision is gated when it, or any ancestor in - its parent chain, is session-derived, whatever the origins in between - (manual, rebase, resolution). A parent that cannot be found counts as - gated. A gated revision activates (or is rolled back to) only from a web - session, only on a private kit, and only as a private document. - **Consequence**: once a kit has had a session-derived revision, every later - revision descends from it, so every later activation of that kit needs a - web session, and API-key activation and going public stay off for that kit - for good. To publish such work, start a new kit from a document without - that history. + its revision chain, is session-derived, whatever the origins in between + (manual, rebase, resolution). The same holds for the revision a document's + `parent` names in another kit, and for that revision's own chain, at every + level of the lineage. A parent that cannot be found or read counts as + gated, or fails the activation with `kit.locked_incompatible`. A gated + revision activates (or is rolled back to) only from a web session, only on + a private kit, and only as a private document. +- **What the gate follows**: the gate follows revision history, not + content. A manual revision's parent is the kit's *effective* revision, so + once a session-derived revision has been effective, the revisions built on + it stay gated. A session-derived revision that never became effective + gates nothing but its own rebases and resolutions and the kits whose document `parent` names it. Rolling back to an + earlier private revision with no session-derived ancestor (allowed with an + API key) ends the gate for the revisions built after it. Content copied + into a revision by hand carries no history, so it is not gated; making it + public still needs a web session. - **No chaining on unreviewed proposals**: a from-revision that is `proposed` (or any session-derived revision that is not `reviewed`) fails with 409 `kit.review_required`, and an unreviewed proposal is never usable as a @@ -361,26 +368,34 @@ verifies the result like any revision. binary content, text with a byte order mark (UTF-8/16/32), or an absolute local path. - Secrets: vendor token formats; a `key: value`, `key = value` or - `"key": "value"` assignment (YAML, JSON, `.env`, code) whose key ends with - a password stem (`secret-password`: `password`, `dbPassword`, - `DB_PASSWD`, a value of 4 or more characters) or a secret stem - (`secret-assignment`: `clientSecret`, `apiKey`, `accessToken`, - `awsCredentials`, a value of 8 or more characters), in any case and + `"key": "value"` assignment, also with `:=` or `=>`, in brackets + (`cfg["password"] = ...`), as a flag (`--password=...`) or inside another + value (`Server=db;Password=...`, `?user=a&password=...`), in YAML, JSON, + `.env` or code, whose key holds a stem followed by nothing or by at most + two qualifiers of `content.secretKeys.qualifiers` or a version number + (`secret_key_base`, `api_key_prod`, `private_key_id`; not `secretName`, + `passwordless` or `tokenizer`): a password stem (`secret-password`: + `password`, `dbPassword`, `DB_PASSWD`, a value of 4 or more characters) + or a secret stem (`secret-assignment`: `clientSecret`, `apiKey`, + `accessToken`, `awsCredentials`, a value of 8 or more characters), in any case and separator style, compared through the same key normalization as the envelope (the stems are `content.secretKeys` in the policy); a `KEY=value` line whose key holds a stem anywhere (`secret-dotenv`); a YAML `password:` key whose new value sits on the following indented lines or in a `|` / `>` block scalar, also as a list item `- password:`, a - quoted key `"password":`, or after an anchor or tag (`&pw`, `!secret`), + quoted key `"password":`, or after up to two anchors or tags (`&pw`, + `!secret`, `&a !s`), even when the key line is kept from the base; Bearer tokens, URLs with `user:pass@`, JWTs and private key blocks. A YAML anchor or tag before an - inline value is skipped. Placeholders (`<...>`, `%VAR%`, `{...}`, `***`, + inline value is skipped (up to two). Placeholders (`<...>`, `%VAR%`, `{...}`, `***`, `${...}`, a whole `$VAR`) and numbers are not secrets; any other value, also one that starts with `$`, is. - Paths: every absolute local path, matched case-insensitively, after NFC - normalization and with invisible format characters (zero-width spaces - and joiners) removed, and also JSON-escaped (`\/`, `\\`) or - percent-encoded (`%2F`, `%5C`, `%3A`, `%2E`): any drive-letter path, UNC + normalization, with Unicode slash forms (`／`, `∕`, `⁄`, `＼`) read as + separators and invisible format characters (zero-width spaces and + joiners) removed, and also JSON-escaped (`\/`, `\\`) or percent-encoded + (`%2F`, `%5C`, `%3A`, `%2E`, decoded up to three `%25` layers; a text + still holding an encoded separator or dot after that is refused): any drive-letter path, UNC shares (`\\host\share` and `//host/share`), `file:` and `smb:` URLs, editor links to a file (`vscode://file/...`), a user home in an `ssh` or `sftp` URL, a bare `/home`, `/Users`, `/root`, `/mnt`, `/media`, @@ -395,7 +410,7 @@ verifies the result like any revision. `/dev/null`, `/usr/share/zoneinfo`; not `/usr/share/doc/x` or `/usr/local/opt/x`). A `..` segment refuses the path. URLs of network schemes (`https://host/path`, `s3://`, ...) are not local paths; a path - glued after a URL's `,`, `)` or `]` is still read. + glued after a URL's `,`, `;`, `|`, `&`, `(`, `)`, `[`, `]`, `{` or `}` is still read. `error.findings` lists `danger::` codes, never the matched text. Every detector runs in time linear in its input, so a hostile request body cannot exhaust the Worker CPU.