diff --git a/AGENT-INSTALL.md b/AGENT-INSTALL.md index 3d53d90b..ae8b0b23 100644 --- a/AGENT-INSTALL.md +++ b/AGENT-INSTALL.md @@ -180,8 +180,16 @@ would have stopped while it is still in dry-run. Two separate paths, with differ What a detection report contains, per matched rule — on every phase, whatever fired it: the rule id, the revision of the rule when the bundle carried one, the identifier of the rule bundle in use, which phase matched, -whether it was enforced, the request path **with the query string's values removed**, -that query's parameter names, the method, and a timestamp. Each batch also carries a count of reports dropped when traffic outran the flush, +the rule's category and the action it declares, whether it was enforced, +the request path **with the query string's values removed**, +that query's parameter names, the method, and a timestamp. + +The category and the declared action say what KIND of rule matched — "a secret-exposure rule that +redacts", "an SSRF rule that blocks". Both are read from the rule your guard was served, and both are +`null` when that rule declares neither: a rule whose class nobody can state is reported as one, not +filled in. Neither is the same as `enforced`, which is whether the rule acted on this particular +request: a rule declaring `block` while only observing reports exactly that, and that is what a +detect-only deployment consists of. Each batch also carries a count of reports dropped when traffic outran the flush, so a partial sample is not read as a complete one. Two fields depend on the phase, because one kind of detection has a client and the other does not. A diff --git a/src/protect/defaults.js b/src/protect/defaults.js index 020746d8..c78ac062 100644 --- a/src/protect/defaults.js +++ b/src/protect/defaults.js @@ -206,6 +206,11 @@ export const DEFAULT_EGRESS_RULES = [ title: 'Outbound request to an internal / metadata address (SSRF)', phase: 'egress', category: 'ssrf', + // Declared, not implied. Nothing in the egress path reads it — a match refuses the call, and the + // rule's mode decides whether it actually did — but the action is part of how a rule describes + // itself to whatever reports on it, and a rule that declares nothing is reported as a rule nobody + // can classify. + action: 'block', rule_v2: [{ parameter: 'egress.host', match: { type: 'internal_host' } }] } ]; diff --git a/src/protect/detections.js b/src/protect/detections.js index 869a5388..dd2a6153 100644 --- a/src/protect/detections.js +++ b/src/protect/detections.js @@ -1,4 +1,5 @@ import { pulseFetch } from '../pulse-token.js'; +import { ACTIONS } from './rules/contract.js'; import { clientIpFields } from './client-ip.js'; import { readRuleParameters, ruleParameters } from './rule-parameters.js'; import { isSafeOrigin } from './safe-origin.js'; @@ -17,8 +18,13 @@ import { isSafeOrigin } from './safe-origin.js'; * ## The payload is deliberately small * * `rule_id`, route PATH, the parameters the rule reads, a timestamp, whether it was enforced, the phase, - * the bundle identity, and the rule's own revision where the bundle carried one. That is enough to count - * hits per rule, compare them against traffic, and decide whether a rule is wrong. + * the rule's category and the action it declares, the bundle identity, and the rule's own revision where + * the bundle carried one. That is enough to count hits per rule, compare them against traffic, and + * decide whether a rule is wrong. + * + * The category and the declared action say what KIND of rule matched, so a redaction and a blocked + * request are not one number. Both come from the rule and are `null` when it declares neither. + * Neither is `enforced`: a rule declaring `block` while only observing enforces nothing. * * It also carries the values of the parameters the matched rule NAMES, under a plan derived from that * rule — because counting that a rule fired is not enough to act on it. What may be captured is the @@ -98,6 +104,69 @@ const ATTEMPT_TIMEOUT_MS = 10_000; */ const STOP_BUDGET_MS = 5_000; +/** + * The shape a category has to have to mean anything to the platform receiving it. + * + * No length bound here: the bound is applied separately, so a category that is a real slug and merely + * too long can be told apart from one that was never a category. Only the first has anything to say + * about the bound. + * + * A slug. `unknown` is excluded because that is the word the platform files an ABSENT class under, so a + * category spelled that way would be sent as a real class and then counted as a missing one — two + * spellings of one thing, disagreeing about which they are. + */ +const CATEGORY_PATTERN = /^[a-z][a-z0-9-]*$/; +const RESERVED_CATEGORY = 'unknown'; + +/** + * A rule's declared class, as it can be reported. + * + * Four outcomes, and the difference between the last two is the point: + * + * - absent → `null`, unmarked. The rule declared nothing, which is a true report. + * - unrecognisable → `null`, unmarked. Not a class this platform has a meaning for, so reporting it + * would put arbitrary text where a class belongs — and nothing was lost, which is what the mark is + * about. + * - recognisable but too long → `null`, MARKED. There was a class here and it could not be carried: it + * cannot be shortened without becoming a different class, and one sent as a class would be a category + * nobody authored, counted beside real ones. + * - recognisable and short enough → carried as it is. + * + * Recognition is tested BEFORE the length, and the order is the whole of what the mark means. Reversed, + * any long enough run of nonsense is reported as "there was a class here that would not fit" — which is + * a claim about a class that never existed, and the reader has no way to see it is false. Only a value + * that would have been carried at a shorter length has anything to say about the bound. + * + * Recognition happens here rather than only at the far end. Both sides work from the same contract, and + * a value the receiver will refuse costs an event's budget to be discarded — while travelling as though + * it were known, which is the part that misleads: `Uppercase` or `has spaces` would arrive looking like + * a class and be filed as a missing one. + * + * @param {unknown} declared + * @param {(value: string) => boolean} recognised + * @returns {{ value: string | null, dropped: boolean }} + */ +function declaredClass(declared, recognised) { + if (typeof declared !== 'string' || declared === '') return { value: null, dropped: false }; + if (!recognised(declared)) return { value: null, dropped: false }; + if (declared.length > MAX_CLASS_CHARS) return { value: null, dropped: true }; + + return { value: declared, dropped: false }; +} + +/** @param {string} value */ +const recognisedCategory = (value) => value !== RESERVED_CATEGORY && CATEGORY_PATTERN.test(value); + +/** + * The actions a rule may declare, from the rule contract this package already enforces. + * + * Imported rather than restated: a list written out here would be a second copy of the vocabulary, and + * the copy that fell behind would file a real action as unrecognisable. + * + * @param {string} value + */ +const recognisedAction = (value) => ACTIONS.includes(value); + /** * Size bounds, applied per event and per batch. * @@ -118,6 +187,18 @@ const MAX_PARAMETER_CHARS = 64; * reader must not use it as a key believing it does. */ const MAX_IDENTIFIER_CHARS = 256; +/** + * The bound on a rule's declared class, set by what the platform accepts rather than by what fits. + * + * A longer identifier can be shortened and still be useful: a route that was cut is still the route + * prefix. A class cannot. `secret-exposure` shortened is a different class, and a shortened one sent as + * a class would be a category nobody authored, counted alongside real ones. + * + * So a class beyond the bound is reported as ABSENT and marked, not shortened. The platform would refuse + * it anyway — sending a value known to be unacceptable only spends the event's budget to have it + * discarded, and sending a truncated one spends it to have the wrong thing recorded. + */ +const MAX_CLASS_CHARS = 64; /** * Bounds re-applied to captured evidence at the wire. * @@ -878,6 +959,15 @@ export function createDetectionReporter(opts) { const id = capText(String(ruleId), MAX_IDENTIFIER_CHARS); const revision = capText(revisionOf(detection.rule) ?? '', MAX_IDENTIFIER_CHARS); const etag = capText(rulesEtag ?? '', MAX_IDENTIFIER_CHARS); + // Both declared fields come from the RULE, which is the only thing that declares them. + // + // A detection also carries a top-level `category` copied from the rule at each site that raises + // one. Reading that copy would make this report depend on every one of those copies staying + // right: a site that forgot one, or set it from something else, would report a class the rule + // does not have — and it would look exactly like a correct report. The rule is the source, so + // there is nothing to keep in step. + const classCategory = declaredClass(detection.rule?.category, recognisedCategory); + const classAction = declaredClass(detection.rule?.action, recognisedAction); for (const [name, field] of [ ['rule_id', id], ['rule_revision', revision], @@ -885,6 +975,14 @@ export function createDetectionReporter(opts) { ]) { if (field.truncated) truncated.push(name); } + // Marked when a class was there and could not be carried, so its absence is never read as a rule + // that declared nothing. + for (const [name, field] of [ + ['category', classCategory], + ['action', classAction], + ]) { + if (field.dropped) truncated.push(name); + } queue.push({ rule_id: id.value, @@ -908,7 +1006,23 @@ export function createDetectionReporter(opts) { ...(query.total > queryKeys.length ? { query_keys_total: query.total } : {}), // Who asked. Capped, since it is client-supplied text and this is an event with a size bound. user_agent: userAgent === null ? null : userAgent.value, + // What KIND of match this was: which phase it happened in, what class of thing the rule is for, + // and what the rule DECLARES it does about it. + // + // Three separate facts, and none of them is `enforced` below. A rule declaring `block` while + // observing reports exactly that — `action: 'block'`, `enforced: false` — which is what a + // dry-run window consists of. Reading either off the other would describe such a window as + // protection that never happened, or as rules that do nothing. + // + // `null` where the rule says nothing, never a guess. A consumer can tell "this rule is for + // secret exposure" from "we cannot say what this rule is for", and a filled-in value would take + // that distinction away for the sake of a tidier field. phase: detection.phase ?? null, + // `null` where the rule declared nothing, and also where what it declared could not be carried: + // both are "we cannot say what this rule is for", which is a different fact from a class we do + // know, and `truncated` distinguishes the second from the first. + category: classCategory.value, + action: classAction.value, // The state this detection was handled under, which is the whole point: `false` is a rule that // saw traffic it would have stopped. enforced: detection.mode === 'block', diff --git a/src/protect/protect.d.ts b/src/protect/protect.d.ts index 63abf3b8..54e365d6 100644 --- a/src/protect/protect.d.ts +++ b/src/protect/protect.d.ts @@ -168,8 +168,12 @@ export interface CreateProtectionOptions { * * What it sends on EVERY detection: the rule id and its revision, the request path, the query string's * parameter NAMES, the method, the parameters the rule reads, the phase, whether it was enforced, the - * rule-bundle ETag, and a timestamp — plus the values of the parameters the matched rule names, under a - * capture plan derived from that rule. + * rule-bundle ETag, a timestamp, and the rule's category and the action it declares — plus + * the values of the parameters the matched rule names, under a plan derived from that rule. + * + * The category and the declared action say what KIND of rule matched. Both are read from the rule + * itself and are `null` when it declares neither. Neither is the same as `enforced`: + * a rule declaring `block` while only observing reports that action with `enforced` false. * * Two fields depend on the phase. A request or response detection also carries the user agent and the * client address with its provenance. An egress detection carries neither: the call was the diff --git a/tests/endpoint-disclosure.test.ts b/tests/endpoint-disclosure.test.ts index 0095ed45..5c4ccb36 100644 --- a/tests/endpoint-disclosure.test.ts +++ b/tests/endpoint-disclosure.test.ts @@ -270,6 +270,18 @@ describe('shipped docs disclose every endpoint the package calls', () => { expect(text, 'must scope the client fields to the phases that have a client').toMatch( /request or response detection/i, ); + // The classification, on the same terms. A payload field described in AGENT-INSTALL.md and + // nowhere else is documented for whoever reads that file and undocumented for the caller whose + // editor shows the type declaration — and the completeness check over the payload reads only + // AGENT-INSTALL.md, so it cannot notice. + expect(text, "must say the rule's class travels").toMatch( + /category and the action it declares/i, + ); + // And that it is not the same fact as enforcement, which is the pair a reader is most likely to + // conflate: a rule declaring `block` while observing enforces nothing. + expect(text, 'must separate the declared action from what was enforced').toMatch( + /declaring `?block`? while (only )?observing/i, + ); expect(text, 'and must say an egress detection carries neither').toMatch( /egress detection[^.]*(carries neither|no user agent)/i, ); diff --git a/tests/protect/detection-classification.test.ts b/tests/protect/detection-classification.test.ts new file mode 100644 index 00000000..48718098 --- /dev/null +++ b/tests/protect/detection-classification.test.ts @@ -0,0 +1,234 @@ +import { describe, expect, it, vi } from 'vitest'; +import { createDetectionReporter } from '../../src/protect/detections.js'; +import { ACTIONS } from '../../src/protect/rules/contract.js'; + +/** + * What KIND of match a detection was, on the wire. + * + * Three facts travel: the phase it happened in, the class of thing the rule is for, and what the rule + * DECLARES it does about it. None of them is `enforced`, which is whether it actually did. + * + * The declared action and the enforced state are independent, and a dry-run window is exactly where + * they differ: every rule in it declares an action and enforces nothing. Reading either off the other + * would describe such a window as protection that never happened, or as a fleet of rules that do + * nothing. + * + * Nothing is filled in. A rule that declares no class is reported as declaring none, because "this rule + * is for secret exposure" and "we cannot say what this rule is for" are different facts and only one of + * them is available. + * + * Both are read from the RULE. A detection also carries a top-level `category` copied from the rule at + * each site that raises one, and reading that copy would make this report depend on every one of those + * copies staying right — a site that set it from something else would report a class the rule does not + * have, indistinguishably from a correct report. + */ +function reporterWith(overrides: Record = {}) { + const posts: Array<{ url: string; body: any }> = []; + const fetchImpl = vi.fn(async (url: string, init?: RequestInit) => { + posts.push({ url, body: JSON.parse(String(init?.body ?? '{}')) }); + + return new Response('{}', { status: 202 }); + }); + const reporter = createDetectionReporter({ + siteUuid: 'site-1', + baseUrl: 'https://x.test/monitor/pulse', + rulesEtag: '"v7"', + fetchImpl: fetchImpl as unknown as typeof fetch, + ...overrides, + }); + + return { reporter, posts }; +} + +const drain = () => new Promise((resolve) => setTimeout(resolve, 0)); + +/** One reported event, from one recorded detection. */ +async function reported(detection: Record) { + const { reporter, posts } = reporterWith(); + reporter.record({ phase: 'response', mode: 'dry-run', path: '/x', ...detection } as any); + reporter.flush(); + await drain(); + + return posts[0].body.detections[0]; +} + +const ruleWith = (extra: Record = {}) => ({ + id: 'pulse-1', + rule_v2: [{ parameter: 'response.body', match: { type: 'contains', value: 'x' } }], + ...extra, +}); + +describe('the classification on the wire', () => { + it('reports the phase, the category and the declared action', async () => { + const event = await reported({ + rule: ruleWith({ category: 'secret-exposure', action: 'redact' }), + }); + + expect(event.phase).toBe('response'); + expect(event.category).toBe('secret-exposure'); + expect(event.action).toBe('redact'); + }); + + it('reports a declared action that did not act', async () => { + // The state a dry-run window consists of, and the reason these are two fields. + const event = await reported({ + rule: ruleWith({ category: 'info-exposure', action: 'block' }), + mode: 'dry-run', + }); + + expect(event.action).toBe('block'); + expect(event.enforced).toBe(false); + }); + + it('reports the same declared action when it did act', async () => { + // The control: the declared action does not change with the mode. Only `enforced` does. + const event = await reported({ + rule: ruleWith({ category: 'info-exposure', action: 'block' }), + mode: 'block', + }); + + expect(event.action).toBe('block'); + expect(event.enforced).toBe(true); + }); + + it('reports null for a rule that declares no class', async () => { + // Never a guess. A filled-in value would take away the distinction between a rule whose class is + // known and one whose class nobody can state. + const event = await reported({ rule: ruleWith() }); + + expect(event.category).toBeNull(); + expect(event.action).toBeNull(); + }); + + it('reports each field independently of the other', async () => { + // A rule may declare one and not the other, and each has to travel on its own account. + const onlyCategory = await reported({ rule: ruleWith({ category: 'ssrf' }) }); + expect(onlyCategory.category).toBe('ssrf'); + expect(onlyCategory.action).toBeNull(); + + const onlyAction = await reported({ rule: ruleWith({ action: 'encode' }) }); + expect(onlyAction.category).toBeNull(); + expect(onlyAction.action).toBe('encode'); + }); + + it('drops and marks a real category that is too long to carry', async () => { + // A slug, and one character past what the receiving contract accepts. Not shortened — + // `secret-exposure` cut short is a different class, and one sent as a class would be a category + // nobody authored, counted beside real ones. So it travels as absent, and marked, because there was + // a class here and it could not be carried. + const event = await reported({ rule: ruleWith({ category: 'c'.repeat(65) }) }); + + expect(event.category).toBeNull(); + expect(event.truncated).toContain('category'); + }); + + it('marks nothing for an over-length value that was never a category', async () => { + // The mark is a claim about a class that existed. Applied to nonsense that happens to be long, it + // reports "there was a class here that would not fit" — about something that was never a class, and + // with nothing in the report to show the claim is false. + const event = await reported({ rule: ruleWith({ category: 'C'.repeat(65) }) }); + + expect(event.category).toBeNull(); + expect(event.truncated).toBeUndefined(); + }); + + it('marks nothing for an over-length action, because no action is long', async () => { + // Every action the contract names is short, so an over-length one is not an action at any length. + // Only the category can reach recognised-but-too-long. + const event = await reported({ rule: ruleWith({ action: 'a'.repeat(65) }) }); + + expect(event.action).toBeNull(); + expect(event.truncated).toBeUndefined(); + }); + + it('carries a category at exactly the accepted length', async () => { + // The boundary in the direction that must still work: a bound one character tight would silently + // drop the longest legitimate category. Category only — no action in the vocabulary is this long, + // so pinning a 64-character action would pin a value the receiver refuses. + const event = await reported({ rule: ruleWith({ category: 'c'.repeat(64) }) }); + + expect(event.category).toBe('c'.repeat(64)); + expect(event.truncated).toBeUndefined(); + }); + + it('carries every action the rule contract names', async () => { + // The vocabulary is imported rather than restated here, so this cannot drift from it: a list + // written out in this file would pass while the one in the reporter fell behind. + for (const action of ACTIONS) { + const event = await reported({ rule: ruleWith({ action }) }); + expect(event.action, `contract action "${action}"`).toBe(action); + expect(event.truncated, `contract action "${action}"`).toBeUndefined(); + } + }); + + it('reports nothing for a class the receiver would not recognise', async () => { + // These are short enough to fit and would have travelled as though known, then been filed as + // missing at the far end — arriving looking like a class while counting as the absence of one. + // + // Unmarked, because nothing was lost in transit: the mark distinguishes "declared something we + // could not carry" from "declared nothing", and an unrecognisable value is neither. + for (const category of ['unknown', 'Uppercase', 'has spaces', '-leading-dash', '9leading-digit']) { + const event = await reported({ rule: ruleWith({ category }) }); + expect(event.category, `category ${JSON.stringify(category)}`).toBeNull(); + expect(event.truncated, `category ${JSON.stringify(category)}`).toBeUndefined(); + } + + for (const action of ['obliterate', 'BLOCK', 'redact ', 'unknown']) { + const event = await reported({ rule: ruleWith({ action }) }); + expect(event.action, `action ${JSON.stringify(action)}`).toBeNull(); + expect(event.truncated, `action ${JSON.stringify(action)}`).toBeUndefined(); + } + }); + + it('reports the rule when a copied class disagrees with it', async () => { + // The detection's own `category` is a copy made where the detection was raised. The rule is what + // declares the class, so the rule wins — otherwise a wrong copy reports a class the rule does not + // have, and there is nothing in the report to show it happened. + const event = await reported({ + rule: ruleWith({ category: 'secret-exposure', action: 'redact' }), + category: 'something-else-entirely', + }); + + expect(event.category).toBe('secret-exposure'); + }); + + it('reports nothing for a rule that declares no class, whatever the copy says', async () => { + // The same, in the direction that would invent a class rather than mis-state one. + const event = await reported({ rule: ruleWith(), category: 'invented-by-the-caller' }); + + expect(event.category).toBeNull(); + }); + + it('marks nothing when the class fits', async () => { + // The other half: `truncated` present at all is a claim, so it must not appear for a short value. + const event = await reported({ + rule: ruleWith({ category: 'secret-exposure', action: 'redact' }), + }); + + expect(event.truncated).toBeUndefined(); + }); + + it('puts no rule text on the wire beyond the class', async () => { + // The rule object reaches the reporter whole. Only its identity and its declared class may travel; + // a match value or a pattern must not, and the serialized payload is what is scanned rather than + // the object built, so a field added later fails here. + const { reporter, posts } = reporterWith(); + reporter.record({ + phase: 'response', + mode: 'dry-run', + path: '/x', + rule: ruleWith({ + category: 'secret-exposure', + action: 'redact', + title: 'A title nobody asked for', + rule_v2: [{ parameter: 'response.body', match: { type: 'contains', value: 'SUPERSECRET' } }], + }), + } as any); + reporter.flush(); + await drain(); + + const wire = JSON.stringify(posts[0].body); + expect(wire).not.toContain('SUPERSECRET'); + expect(wire).not.toContain('A title nobody asked for'); + }); +}); diff --git a/tests/protect/detection-payload-contract.test.ts b/tests/protect/detection-payload-contract.test.ts index 7ed03153..bce79d7b 100644 --- a/tests/protect/detection-payload-contract.test.ts +++ b/tests/protect/detection-payload-contract.test.ts @@ -39,6 +39,8 @@ const FIELD_DISCLOSURE: Record = { user_agent: /user\s+agent/i, parameters: /parameter names/i, phase: /which phase matched/i, + category: /the rule's category and the action it declares/i, + action: /the rule's category and the action it declares/i, enforced: /whether it was enforced/i, rules_etag: /identifier of the rule bundle/i, rule_revision: /revision of the rule/i, diff --git a/tests/protect/detections.test.ts b/tests/protect/detections.test.ts index b827fd4d..7a0ff473 100644 --- a/tests/protect/detections.test.ts +++ b/tests/protect/detections.test.ts @@ -32,6 +32,8 @@ const ALLOWED_KEYS = [ 'user_agent', 'parameters', 'phase', + 'category', + 'action', 'enforced', 'rules_etag', 'rule_revision', @@ -83,6 +85,10 @@ describe('the detection payload', () => { query_keys: ['url'], parameters: ['server.REQUEST_URI', 'get.url'], phase: 'request', + // Null because `pinnedRule` declares neither, which is the honest report for a rule that says + // nothing — distinguishable from a rule whose class we do know. + category: null, + action: null, // The point of the channel: this rule did not block, and that is the interesting case. enforced: false, rules_etag: '"v7"',