diff --git a/server/src/__tests__/plugin-config-masking.test.ts b/server/src/__tests__/plugin-config-masking.test.ts index d668690319f5..ac07dfa56c62 100644 --- a/server/src/__tests__/plugin-config-masking.test.ts +++ b/server/src/__tests__/plugin-config-masking.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { PLUGIN_CONFIG_SECRET_MASK, + TRAVERSED_SCHEMA_KEYWORDS, collectPluginConfigSecretValues, maskPluginConfigJson, mergeMaskedPluginConfig, @@ -1027,3 +1028,446 @@ describe("collectPluginConfigSecretValues — structured declared secrets", () = expect(JSON.stringify(diagnostic)).not.toContain("live-password"); }); }); + +// --------------------------------------------------------------------------- +// BLO-26530 — masking gaps found by paranoid review of the merged BLO-20871 work +// --------------------------------------------------------------------------- + +describe("mergeMaskedPluginConfig — duplicate incoming identities (BLO-26530)", () => { + const IDENTITY_SCHEMA = { + type: "object", + properties: { + targets: { + type: "array", + items: { + type: "object", + "x-paperclip-identity": "name", + properties: { name: { type: "string" }, url: { type: "string" } }, + }, + }, + }, + }; + + it("refuses to clone one stored credential onto two entries claiming the same identity", () => { + // Uniqueness was proven only in storage, so both incoming entries matched + // the single stored `alpha` and both were handed `token-alpha` — including + // the entry pointing at an endpoint the operator never gave it to. + const result = merge( + { + targets: [ + { name: "alpha", url: "https://a.example.com", token: PLUGIN_CONFIG_SECRET_MASK }, + { name: "alpha", url: "https://attacker.example.com", token: PLUGIN_CONFIG_SECRET_MASK }, + ], + }, + { targets: [{ name: "alpha", url: "https://a.example.com", token: "token-alpha" }] }, + IDENTITY_SCHEMA, + ); + + expect(result.unresolvedMaskPaths).toEqual(["targets.0", "targets.1"]); + expect(JSON.stringify(result.configJson)).not.toContain("token-alpha"); + // Nor may the sentinel itself be left behind for persistence. + expect(JSON.stringify(result.configJson)).not.toContain(PLUGIN_CONFIG_SECRET_MASK); + }); + + it("refuses a duplicated identity even when only one of the two entries is masked", () => { + const result = merge( + { + targets: [ + { name: "alpha", url: "https://a.example.com", token: PLUGIN_CONFIG_SECRET_MASK }, + { name: "alpha", url: "https://attacker.example.com", token: "operator-supplied" }, + ], + }, + { targets: [{ name: "alpha", url: "https://a.example.com", token: "token-alpha" }] }, + IDENTITY_SCHEMA, + ); + + expect(result.unresolvedMaskPaths).toEqual(["targets.0"]); + expect(JSON.stringify(result.configJson)).not.toContain("token-alpha"); + // An entry carrying no sentinel is still the caller's own value. + expect(result.configJson).toEqual({ + targets: [ + { name: "alpha", url: "https://a.example.com" }, + { name: "alpha", url: "https://attacker.example.com", token: "operator-supplied" }, + ], + }); + }); + + it("refuses when storage itself holds the identity twice, so neither side is proof", () => { + const result = merge( + { targets: [{ name: "alpha", url: "https://a.example.com", token: PLUGIN_CONFIG_SECRET_MASK }] }, + { + targets: [ + { name: "alpha", url: "https://a.example.com", token: "token-one" }, + { name: "alpha", url: "https://b.example.com", token: "token-two" }, + ], + }, + IDENTITY_SCHEMA, + ); + + expect(result.unresolvedMaskPaths).toEqual(["targets.0"]); + expect(JSON.stringify(result.configJson)).not.toContain("token-one"); + expect(JSON.stringify(result.configJson)).not.toContain("token-two"); + }); + + it("still restores by identity when it is unique on both sides", () => { + // Guards the fix against over-correction: the ergonomic path a manifest buys + // by declaring `x-paperclip-identity` must survive. + const stored = { + targets: [ + { name: "alpha", url: "https://a.example.com", token: "token-alpha" }, + { name: "beta", url: "https://b.example.com", token: "token-beta" }, + ], + }; + const result = merge( + { + targets: [ + { name: "beta", url: "https://b.example.com", token: PLUGIN_CONFIG_SECRET_MASK }, + { name: "alpha", url: "https://a.example.com", token: PLUGIN_CONFIG_SECRET_MASK }, + ], + }, + stored, + IDENTITY_SCHEMA, + ); + + expect(result.unresolvedMaskPaths).toEqual([]); + expect(result.configJson).toEqual({ + targets: [ + { name: "beta", url: "https://b.example.com", token: "token-beta" }, + { name: "alpha", url: "https://a.example.com", token: "token-alpha" }, + ], + }); + }); + + it("does not conflate a numeric identity with its string form", () => { + const stored = { targets: [{ id: 1, token: "token-numeric" }] }; + const result = merge( + { + targets: [ + { id: 1, token: PLUGIN_CONFIG_SECRET_MASK }, + { id: "1", token: PLUGIN_CONFIG_SECRET_MASK }, + ], + }, + stored, + { + type: "object", + properties: { + targets: { type: "array", items: { type: "object", "x-paperclip-identity": "id" } }, + }, + }, + ); + + // `1` is unique on both sides and resolves; `"1"` matches no stored entry. + expect(result.unresolvedMaskPaths).toEqual(["targets.1"]); + expect(result.configJson).toEqual({ + targets: [{ id: 1, token: "token-numeric" }, { id: "1" }], + }); + }); +}); + +describe("maskPluginConfigJson — conditional and dependent schemas (BLO-26530)", () => { + const SENTINEL = "sentinel-conditional-plaintext"; + /** Innocuously named, so only the schema marker can cover it. */ + const LOOKASIDE = { type: "string", writeOnly: true }; + + const cases: Record> = { + then: { + type: "object", + if: { properties: { mode: { const: "managed" } } }, + then: { properties: { lookaside: LOOKASIDE } }, + }, + else: { + type: "object", + if: { properties: { mode: { const: "never-matches" } } }, + else: { properties: { lookaside: LOOKASIDE } }, + }, + if: { + type: "object", + if: { properties: { lookaside: LOOKASIDE } }, + }, + not: { + type: "object", + not: { properties: { lookaside: LOOKASIDE } }, + }, + dependentSchemas: { + type: "object", + dependentSchemas: { mode: { properties: { lookaside: LOOKASIDE } } }, + }, + "draft-07 dependencies": { + type: "object", + dependencies: { mode: { properties: { lookaside: LOOKASIDE } } }, + }, + }; + + for (const [label, schema] of Object.entries(cases)) { + it(`masks a secret declared behind \`${label}\``, () => { + const masked = maskPluginConfigJson({ mode: "managed", lookaside: SENTINEL }, schema); + + expect(JSON.stringify(masked)).not.toContain(SENTINEL); + expect(masked).toEqual({ mode: "managed", lookaside: PLUGIN_CONFIG_SECRET_MASK }); + }); + + it(`round-trips a secret declared behind \`${label}\` losslessly`, () => { + const stored = { mode: "managed", lookaside: SENTINEL }; + const masked = maskPluginConfigJson(stored, schema) as Record; + const result = merge(JSON.parse(JSON.stringify(masked)), stored, schema); + + expect(result.unresolvedMaskPaths).toEqual([]); + expect(result.configJson).toEqual(stored); + }); + } + + it.each([ + ["writeOnly", { type: "string", writeOnly: true }], + ["format: secret-ref", { format: "secret-ref" }], + ["x-paperclip-secret", { type: "string", "x-paperclip-secret": true }], + ])("masks a %s marker hidden behind a conditional branch", (_label, marker) => { + // The acceptance criteria name all three markers, so each is exercised + // through a branch the walk previously skipped rather than only `writeOnly`. + const masked = maskPluginConfigJson( + { mode: "managed", lookaside: SENTINEL }, + { + type: "object", + if: { properties: { mode: { const: "managed" } } }, + then: { properties: { lookaside: marker } }, + }, + ); + + expect(JSON.stringify(masked)).not.toContain(SENTINEL); + expect(masked).toEqual({ mode: "managed", lookaside: PLUGIN_CONFIG_SECRET_MASK }); + }); + + it("does not let a `dependencies` required-list array break masking", () => { + // The draft-07 array form is a required-list, not a schema. It must be + // ignored rather than treated as an uninterpretable keyword. + const masked = maskPluginConfigJson( + { mode: "managed", endpoint: "https://alerts.example.com" }, + { type: "object", dependencies: { mode: ["endpoint"] } }, + ); + + expect(masked).toEqual({ mode: "managed", endpoint: "https://alerts.example.com" }); + }); + + it("masks a secret declared behind array `contains`", () => { + const masked = maskPluginConfigJson( + { list: [{ lookaside: SENTINEL }] }, + { + type: "object", + properties: { + list: { type: "array", contains: { properties: { lookaside: LOOKASIDE } } }, + }, + }, + ); + + expect(JSON.stringify(masked)).not.toContain(SENTINEL); + }); + + it("masks a secret declared behind `unevaluatedItems`", () => { + const masked = maskPluginConfigJson( + { list: [{ lookaside: SENTINEL }] }, + { + type: "object", + properties: { + list: { type: "array", unevaluatedItems: { properties: { lookaside: LOOKASIDE } } }, + }, + }, + ); + + expect(JSON.stringify(masked)).not.toContain(SENTINEL); + }); + + it("masks a secret declared behind `unevaluatedProperties`", () => { + const masked = maskPluginConfigJson( + { lookaside: SENTINEL }, + { type: "object", unevaluatedProperties: LOOKASIDE }, + ); + + expect(JSON.stringify(masked)).not.toContain(SENTINEL); + }); + + it("honours an explicit exemption inside a conditional branch", () => { + // Fail-closed must not override an author who spoke about the exact field. + const masked = maskPluginConfigJson( + { mode: "managed", lookaside: "not-a-secret" }, + { + type: "object", + if: { properties: { mode: { const: "managed" } } }, + then: { properties: { lookaside: { type: "string", "x-paperclip-secret": false } } }, + }, + ); + + expect(masked).toEqual({ mode: "managed", lookaside: "not-a-secret" }); + }); +}); + +describe("maskPluginConfigJson — uninterpretable schema keywords fail closed (BLO-26530)", () => { + const SENTINEL = "sentinel-unsupported-keyword-plaintext"; + + const cases: Record> = { + // Indirection this module deliberately does not resolve. + $dynamicRef: { type: "object", $dynamicRef: "#node" }, + $recursiveRef: { type: "object", $recursiveRef: "#" }, + // A schema over a string's decoded content, which the value walk never + // reaches — a marker inside it would otherwise be invisible. + contentSchema: { type: "object", contentSchema: { type: "object" } }, + // A keyword from a future draft, or a typo, must not become a hiding place. + unrecognised: { type: "object", frobnicateSchema: { properties: { lookaside: {} } } }, + }; + + for (const [label, schema] of Object.entries(cases)) { + it(`masks string leaves rather than trusting \`${label}\``, () => { + const masked = maskPluginConfigJson({ lookaside: SENTINEL }, schema); + + expect(JSON.stringify(masked)).not.toContain(SENTINEL); + }); + } + + it("keeps the structure so the config stays editable and round-trippable", () => { + // Fail-closed here forces suspicion, not a wholesale mask: collapsing the + // whole config to one sentinel would leave the operator no form to edit and + // nothing for the merge to restore into. + const schema = { type: "object", frobnicateSchema: {} }; + const stored = { nested: { lookaside: SENTINEL, count: 3 }, flag: true }; + + const masked = maskPluginConfigJson(stored, schema) as Record; + expect(masked).toEqual({ + nested: { lookaside: PLUGIN_CONFIG_SECRET_MASK, count: 3 }, + flag: true, + }); + + const result = merge(JSON.parse(JSON.stringify(masked)), stored, schema); + expect(result.unresolvedMaskPaths).toEqual([]); + expect(result.configJson).toEqual(stored); + }); + + it("leaves an ordinary manifest-shaped schema fully in the clear", () => { + // Guards the allowlist against being too narrow: every keyword a real + // plugin manifest uses today must stay inert. + const masked = maskPluginConfigJson( + { + endpoint: "https://alerts.example.com", + actor: "user", + retries: 3, + enabled: true, + labels: ["a", "b"], + extra: { note: "visible" }, + }, + { + $schema: "https://json-schema.org/draft/2020-12/schema", + $id: "https://example.com/manifest", + type: "object", + title: "Example config", + description: "Example", + required: ["endpoint"], + additionalProperties: { type: "object" }, + "x-paperclip-ui-order": ["endpoint"], + properties: { + endpoint: { type: "string", format: "uri", title: "Endpoint", maxLength: 200 }, + actor: { type: "string", enum: ["user", "app"], default: "user" }, + retries: { type: "integer", minimum: 0, maximum: 10 }, + enabled: { type: "boolean", default: false }, + labels: { type: "array", items: { type: "string" }, uniqueItems: true, minItems: 0 }, + }, + }, + ); + + expect(masked).toEqual({ + endpoint: "https://alerts.example.com", + actor: "user", + retries: 3, + enabled: true, + labels: ["a", "b"], + extra: { note: "visible" }, + }); + }); +}); + +describe("maskPluginConfigJson — every traversed keyword is really walked (BLO-26530)", () => { + const SENTINEL = "sentinel-traversal-drift-plaintext"; + /** Innocuously named, so only the schema marker can cover it. */ + const MARKER = { type: "string", writeOnly: true }; + + /** + * One case per member of `TRAVERSED_SCHEMA_KEYWORDS`, each hiding the marker so + * that *only* the named keyword can reach it. + * + * `TRAVERSED_SCHEMA_KEYWORDS` is what exempts a keyword from the fail-closed + * guard, so a keyword listed there but not actually walked is silently ignored + * rather than masked — which is precisely how the markers behind `if` / `then` + * / `else` / `dependentSchemas` / `contains` came back as plaintext. This table + * plus the completeness assertion below make that drift impossible: adding a + * keyword to the set without walking it fails here. + */ + const cases: Record; config: Record }> = { + // --- same instance location ------------------------------------------- + $ref: { + schema: { $ref: "#/$defs/covered", $defs: { covered: { properties: { lookaside: MARKER } } } }, + config: { lookaside: SENTINEL }, + }, + allOf: { schema: { allOf: [{ properties: { lookaside: MARKER } }] }, config: { lookaside: SENTINEL } }, + anyOf: { schema: { anyOf: [{ properties: { lookaside: MARKER } }] }, config: { lookaside: SENTINEL } }, + oneOf: { schema: { oneOf: [{ properties: { lookaside: MARKER } }] }, config: { lookaside: SENTINEL } }, + not: { schema: { not: { properties: { lookaside: MARKER } } }, config: { lookaside: SENTINEL } }, + if: { schema: { if: { properties: { lookaside: MARKER } } }, config: { lookaside: SENTINEL } }, + then: { schema: { then: { properties: { lookaside: MARKER } } }, config: { lookaside: SENTINEL } }, + else: { schema: { else: { properties: { lookaside: MARKER } } }, config: { lookaside: SENTINEL } }, + dependentSchemas: { + schema: { dependentSchemas: { mode: { properties: { lookaside: MARKER } } } }, + config: { mode: "managed", lookaside: SENTINEL }, + }, + dependencies: { + schema: { dependencies: { mode: { properties: { lookaside: MARKER } } } }, + config: { mode: "managed", lookaside: SENTINEL }, + }, + // --- child by property key -------------------------------------------- + properties: { schema: { properties: { lookaside: MARKER } }, config: { lookaside: SENTINEL } }, + patternProperties: { + schema: { patternProperties: { "^look": MARKER } }, + config: { lookaside: SENTINEL }, + }, + additionalProperties: { + schema: { additionalProperties: MARKER }, + config: { lookaside: SENTINEL }, + }, + unevaluatedProperties: { + schema: { unevaluatedProperties: MARKER }, + config: { lookaside: SENTINEL }, + }, + // --- child by array index --------------------------------------------- + items: { + schema: { properties: { list: { type: "array", items: MARKER } } }, + config: { list: [SENTINEL] }, + }, + prefixItems: { + schema: { properties: { list: { type: "array", prefixItems: [MARKER] } } }, + config: { list: [SENTINEL] }, + }, + additionalItems: { + schema: { + properties: { list: { type: "array", items: [{ type: "string" }], additionalItems: MARKER } }, + }, + config: { list: ["visible-first", SENTINEL] }, + }, + contains: { + schema: { properties: { list: { type: "array", contains: MARKER } } }, + config: { list: [SENTINEL] }, + }, + unevaluatedItems: { + schema: { properties: { list: { type: "array", unevaluatedItems: MARKER } } }, + config: { list: [SENTINEL] }, + }, + }; + + it("has a case for every traversed keyword", () => { + // Fails when a keyword is added to the set without a case proving the walk + // enters it — the drift this suite exists to prevent. + expect(Object.keys(cases).sort()).toEqual([...TRAVERSED_SCHEMA_KEYWORDS].sort()); + }); + + for (const [keyword, { schema, config }] of Object.entries(cases)) { + it(`reaches a marker hidden behind \`${keyword}\``, () => { + const masked = maskPluginConfigJson(config, { type: "object", ...schema }); + + expect(JSON.stringify(masked)).not.toContain(SENTINEL); + }); + } +}); diff --git a/server/src/__tests__/plugin-routes-authz.test.ts b/server/src/__tests__/plugin-routes-authz.test.ts index ad3aa4763121..b020195042ed 100644 --- a/server/src/__tests__/plugin-routes-authz.test.ts +++ b/server/src/__tests__/plugin-routes-authz.test.ts @@ -2358,3 +2358,108 @@ describe.sequential("plugin state routes stay board-only for agent actors (BLO-2 expect(res.status).toBe(501); }); }); + +// --------------------------------------------------------------------------- +// BLO-26530 — masking gaps in the merged BLO-20871 boundary, at the route level +// --------------------------------------------------------------------------- + +describe.sequential("plugin config masking gaps (BLO-26530)", () => { + const TARGET_SECRET = "sentinel-target-bearer-do-not-leak"; + const CONDITIONAL_SECRET = "sentinel-conditional-bearer-do-not-leak"; + + /** An array whose entries declare an immutable identity, plus a conditional arm. */ + const gapSchema = { + type: "object", + properties: { + targets: { + type: "array", + items: { + type: "object", + "x-paperclip-identity": "name", + properties: { + name: { type: "string" }, + url: { type: "string" }, + token: { type: "string", writeOnly: true }, + }, + }, + }, + mode: { type: "string" }, + }, + if: { properties: { mode: { const: "managed" } } }, + then: { properties: { lookaside: { type: "string", writeOnly: true } } }, + }; + + beforeEach(() => { + vi.clearAllMocks(); + mockRegistry.getConfig.mockReset(); + mockRegistry.upsertConfig.mockReset(); + ragHealthBucketCache.clear(); + mockSecretService.getById.mockResolvedValue({ id: secretId, companyId: companyA, status: "active" }); + mockSecretService.syncSecretRefsForTarget.mockResolvedValue([]); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it("refuses a write that posts one designated identity twice, leaving the stored credential untouched", async () => { + maskingPlugin(gapSchema); + const store = seedConfigStore({ + targets: [{ name: "alpha", url: "https://a.example.com", token: TARGET_SECRET }], + mode: "direct", + }); + const { app } = await createApp(adminActor()); + + const res = await request(app) + .post(`/api/plugins/${pluginId}/config`) + .send({ + companyId: companyA, + configJson: { + targets: [ + { name: "alpha", url: "https://a.example.com", token: "__redacted__" }, + { name: "alpha", url: "https://attacker.example.com", token: "__redacted__" }, + ], + mode: "direct", + }, + }); + + expect(res.status).toBe(400); + expect(res.body.unresolvedMaskPaths).toEqual(["targets.0", "targets.1"]); + // The credential must appear in neither resulting entry, and the rejected + // write must not have disturbed storage. + expect(JSON.stringify(res.body)).not.toContain(TARGET_SECRET); + expect(mockRegistry.upsertConfig).not.toHaveBeenCalled(); + expect(store.configJson).toEqual({ + targets: [{ name: "alpha", url: "https://a.example.com", token: TARGET_SECRET }], + mode: "direct", + }); + }, 20_000); + + it("never emits a secret declared behind a conditional branch to an authorized reader", async () => { + maskingPlugin(gapSchema); + seedConfigStore({ mode: "managed", lookaside: CONDITIONAL_SECRET }); + const { app } = await createApp(adminActor()); + + const res = await request(app).get(`/api/plugins/${pluginId}/config?companyId=${companyA}`); + + expect(res.status).toBe(200); + expect(JSON.stringify(res.body)).not.toContain(CONDITIONAL_SECRET); + expect(res.body.configJson).toEqual({ mode: "managed", lookaside: "__redacted__" }); + }, 20_000); + + it("round-trips a conditionally declared secret losslessly", async () => { + maskingPlugin(gapSchema); + const store = seedConfigStore({ mode: "managed", lookaside: CONDITIONAL_SECRET }); + const { app } = await createApp(adminActor()); + + const read = await request(app).get(`/api/plugins/${pluginId}/config?companyId=${companyA}`); + expect(read.status).toBe(200); + + const write = await request(app) + .post(`/api/plugins/${pluginId}/config`) + .send({ companyId: companyA, configJson: read.body.configJson }); + + expect(write.status).toBe(200); + expect(store.configJson).toEqual({ mode: "managed", lookaside: CONDITIONAL_SECRET }); + }, 20_000); +}); diff --git a/server/src/services/plugin-config-masking.ts b/server/src/services/plugin-config-masking.ts index e9ae7510108e..981862d2a524 100644 --- a/server/src/services/plugin-config-masking.ts +++ b/server/src/services/plugin-config-masking.ts @@ -199,6 +199,143 @@ function declaresSecret(node: SchemaNode): boolean { */ const UNRESOLVED_REF_NODE: SchemaNode = Object.freeze({ "x-paperclip-secret": true }); +/** + * A node injected where the schema at this location could not be fully + * understood — it carries a keyword this module neither traverses nor knows to + * be harmless (see {@link hasUnsupportedSchemaKeyword}). + * + * Unlike {@link UNRESOLVED_REF_NODE} this does not mask the location wholesale; + * it forces the name-heuristic's "suspect" state on, so every string *leaf* at + * and beneath the location is masked while the structure survives. That is + * enough to satisfy "never emit plaintext" — plaintext is a string — without + * collapsing an entire config to a single sentinel, which would leave the + * operator no editable form and nothing for + * {@link mergeMaskedPluginConfig} to restore into. + */ +const AMBIGUOUS_SCHEMA_NODE: SchemaNode = Object.freeze({ "x-paperclip-ambiguous": true }); + +/** + * Keywords whose values are subschemas, and which this module walks. + * + * Grouped by the instance location the subschema applies to, because that + * decides *where* the walk has to consider it: the same location + * ({@link expandSchemaNodes}), a child by key ({@link childNodesForKey}), or a + * child by index ({@link childNodesForIndex}). A schema-bearing keyword handled + * at the wrong level is as good as not handled at all. + * + * Exported for the drift test: listing a keyword here asserts the walk enters + * it, and a keyword listed but not actually walked would be silently *ignored* + * rather than failing closed — the exact shape of the BLO-26530 defect. The test + * hides a marker behind every member and requires each one to mask. + */ +export const TRAVERSED_SCHEMA_KEYWORDS: ReadonlySet = new Set([ + // Same instance location. + "$ref", + "allOf", + "anyOf", + "oneOf", + "not", + "if", + "then", + "else", + "dependentSchemas", + "dependencies", + // Child by property key. + "properties", + "patternProperties", + "additionalProperties", + "unevaluatedProperties", + // Child by array index. + "items", + "prefixItems", + "additionalItems", + "contains", + "unevaluatedItems", +]); + +/** + * Keywords that cannot hide a secret marker over any value this module walks — + * annotations, scalar assertions, and definition containers only reachable via + * `$ref`. + * + * `propertyNames` is inert here because its subschema constrains property *name* + * strings, and names are deliberately outside the masking contract (see + * {@link collectStringLeaves}). + * + * Deliberately absent, so they fail closed: `$dynamicRef` / `$recursiveRef` + * (indirection this module does not resolve) and `contentSchema` (a schema over + * a string's decoded content, which the walk never reaches as a value). + */ +const INERT_SCHEMA_KEYWORDS: ReadonlySet = new Set([ + "type", + "format", + "title", + "description", + "default", + "examples", + "example", + "enum", + "const", + "required", + "deprecated", + "readOnly", + "writeOnly", + "nullable", + "minLength", + "maxLength", + "pattern", + "minimum", + "maximum", + "exclusiveMinimum", + "exclusiveMaximum", + "multipleOf", + "minItems", + "maxItems", + "uniqueItems", + "minContains", + "maxContains", + "minProperties", + "maxProperties", + "propertyNames", + "contentEncoding", + "contentMediaType", + "discriminator", + "xml", + "externalDocs", + "$defs", + "definitions", + "$schema", + "$id", + "$comment", + "$anchor", + "$vocabulary", +]); + +/** + * Whether this node carries a keyword that is neither walked + * ({@link TRAVERSED_SCHEMA_KEYWORDS}) nor known harmless + * ({@link INERT_SCHEMA_KEYWORDS}). + * + * An allowlist rather than a denylist of known-dangerous keywords, so a keyword + * added to JSON Schema — or a typo'd one — fails closed on arrival instead of + * silently becoming a place to hide a `writeOnly` marker. That inverts the + * BLO-26530 failure: `if` / `then` / `else` / `dependentSchemas` / `contains` + * were untraversed, and a secret declared inside one came back as plaintext. + * + * `x-` vendor extensions are exempt. They are annotations by convention (this + * module's own markers live there), and treating every unrecognised one as + * suspicious would mask configs over a UI hint. + */ +function hasUnsupportedSchemaKeyword(node: SchemaNode): boolean { + for (const key of Object.keys(node)) { + if (TRAVERSED_SCHEMA_KEYWORDS.has(key)) continue; + if (INERT_SCHEMA_KEYWORDS.has(key)) continue; + if (key.startsWith("x-")) continue; + return true; + } + return false; +} + /** * Resolve a local JSON-Pointer `$ref` (`#/$defs/credential`) against the root * schema. Returns `null` for anything non-local or unresolvable, which the @@ -226,19 +363,24 @@ function resolveLocalRef(ref: string, root: SchemaNode | null): SchemaNode | nul } /** - * Flatten a set of schema nodes through the composition keywords and local - * `$ref` indirection, so a marker sitting on an `allOf` / `anyOf` / `oneOf` - * *branch node itself*, or on a `$defs` entry a field points at, is seen rather - * than only markers on that branch's `properties`. + * Flatten a set of schema nodes through every keyword whose subschema applies to + * the *same* instance location — composition (`allOf` / `anyOf` / `oneOf`), + * conditionals (`if` / `then` / `else`), dependent schemas, `not`, and local + * `$ref` indirection — so a marker sitting on such a branch node itself, or on a + * `$defs` entry a field points at, is seen rather than only markers on that + * branch's `properties`. * - * Applicability is deliberately not evaluated: a value covered by any branch of - * a composition is treated as covered by all of them. Masking is fail-closed — - * a field that is secret in only one `oneOf` branch must not be emitted in the - * clear just because another branch would have permitted it. + * Applicability is deliberately not evaluated: a value covered by any branch is + * treated as covered by all of them. Masking is fail-closed — a field that is + * secret in only one `oneOf` branch, or only in the `else` arm, must not be + * emitted in the clear just because another branch would have permitted it. This + * is why `if` is traversed alongside `then`/`else`, and why `dependentSchemas` is + * traversed without checking whether its trigger property is present. * * `$ref` targets are resolved against `root`. A ref that is external, dangling, * or cyclic contributes {@link UNRESOLVED_REF_NODE} instead, so an unreadable - * declaration masks rather than leaks. + * declaration masks rather than leaks. A node carrying a keyword this module does + * not understand contributes {@link AMBIGUOUS_SCHEMA_NODE} for the same reason. */ function expandSchemaNodes(nodes: SchemaNode[], root: SchemaNode | null = null): SchemaNode[] { const expanded: SchemaNode[] = []; @@ -252,6 +394,8 @@ function expandSchemaNodes(nodes: SchemaNode[], root: SchemaNode | null = null): seen.add(node); expanded.push(node); + if (hasUnsupportedSchemaKeyword(node)) expanded.push(AMBIGUOUS_SCHEMA_NODE); + const ref = node.$ref; if (typeof ref === "string") { const target = resolveLocalRef(ref, root); @@ -271,6 +415,23 @@ function expandSchemaNodes(nodes: SchemaNode[], root: SchemaNode | null = null): if (isPlainRecord(branch)) stack.push(branch); } } + + // Conditional arms and `not` are single subschemas over this same location. + for (const keyword of ["if", "then", "else", "not"] as const) { + const branch = node[keyword]; + if (isPlainRecord(branch)) stack.push(branch); + } + + // `dependentSchemas` (2020-12) and the schema form of draft-07 + // `dependencies` map a trigger property to a subschema over this location. + // The array form of `dependencies` is a required-list, not a schema. + for (const keyword of ["dependentSchemas", "dependencies"] as const) { + const dependents = node[keyword]; + if (!isPlainRecord(dependents)) continue; + for (const dependent of Object.values(dependents)) { + if (isPlainRecord(dependent)) stack.push(dependent); + } + } } return expanded; @@ -278,8 +439,9 @@ function expandSchemaNodes(nodes: SchemaNode[], root: SchemaNode | null = null): /** * Schema nodes that govern `record[key]`, honouring `properties`, - * `patternProperties` and `additionalProperties` (the last only when neither of - * the former claims the key, matching JSON Schema evaluation order). + * `patternProperties` and `additionalProperties` / `unevaluatedProperties` (the + * latter two only when neither of the former claims the key, matching JSON Schema + * evaluation order). */ function childNodesForKey(nodes: SchemaNode[], key: string): SchemaNode[] { const children: SchemaNode[] = []; @@ -309,8 +471,11 @@ function childNodesForKey(nodes: SchemaNode[], key: string): SchemaNode[] { } } - const additionalProperties = node.additionalProperties; - if (!claimed && isPlainRecord(additionalProperties)) children.push(additionalProperties); + if (claimed) continue; + for (const keyword of ["additionalProperties", "unevaluatedProperties"] as const) { + const fallback = node[keyword]; + if (isPlainRecord(fallback)) children.push(fallback); + } } return children; @@ -320,6 +485,11 @@ function childNodesForKey(nodes: SchemaNode[], key: string): SchemaNode[] { * Schema nodes that govern `array[index]`, honouring the 2020-12 `prefixItems` * + `items` pair as well as the draft-07 tuple form (`items` as an array with * `additionalItems` for the tail). + * + * `contains` applies to *at least one* entry without saying which, so it is + * applied to every index. That over-masks a `contains`-declared secret onto + * sibling entries, which is the fail-closed direction: the alternative is + * emitting the one entry it did govern in the clear. */ function childNodesForIndex(nodes: SchemaNode[], index: number): SchemaNode[] { const children: SchemaNode[] = []; @@ -344,6 +514,11 @@ function childNodesForIndex(nodes: SchemaNode[], index: number): SchemaNode[] { } else if (isPlainRecord(items) && !claimedByTuple) { children.push(items); } + + if (isPlainRecord(node.contains)) children.push(node.contains); + if (!claimedByTuple && isPlainRecord(node.unevaluatedItems)) { + children.push(node.unevaluatedItems); + } } return children; @@ -373,6 +548,16 @@ function nodesDeclareNotSecret(nodes: SchemaNode[]): boolean { return nodes.some((node) => node["x-paperclip-secret"] === false); } +/** + * Whether the schema at this location carries a keyword this module could not + * interpret, per {@link AMBIGUOUS_SCHEMA_NODE}. Every string leaf at and beneath + * such a location is masked, because a secret marker may be hiding in the part + * of the schema that was not understood. + */ +function nodesAreAmbiguous(nodes: SchemaNode[]): boolean { + return nodes.some((node) => node["x-paperclip-ambiguous"] === true); +} + /** * Add every string *leaf* reachable beneath `value` to `collector`. * @@ -476,11 +661,12 @@ export function maskPluginConfigJson( return mask(value); } - // An explicit `x-paperclip-secret: false` wins over the heuristic, and over - // a suspicious ancestor. + // An explicit `x-paperclip-secret: false` wins over the heuristic, over a + // suspicious ancestor, and over an uninterpretable schema keyword — the + // author has spoken about this exact field. const suspect = nodesDeclareNotSecret(nodes) ? false - : inheritedSuspect || (key !== null && matchesSecretFieldName(key)); + : inheritedSuspect || nodesAreAmbiguous(nodes) || (key !== null && matchesSecretFieldName(key)); if (isPlainRecord(value)) { const result: Record = {}; @@ -626,6 +812,26 @@ function containsMask(value: unknown): boolean { return false; } +/** + * How many times each usable identity value occurs among `entries`. + * + * Keyed by the raw scalar, so `1` and `"1"` stay distinct — matching the strict + * comparison used to find the stored counterpart. + */ +function identityOccurrences( + entries: readonly unknown[], + identityKey: string, +): Map { + const counts = new Map(); + for (const entry of entries) { + if (!isPlainRecord(entry)) continue; + const value = identityValue(entry, identityKey); + if (value === undefined) continue; + counts.set(value, (counts.get(value) ?? 0) + 1); + } + return counts; +} + /** * A value usable as an array entry's identity: a non-empty scalar that is not * itself masked (a masked identity would match everything). @@ -714,13 +920,17 @@ function matchesIgnoringMask(incoming: unknown, stored: unknown): boolean { * it. * - *Position.* Reordering `[["x",mask],["y",mask]]` swaps which endpoint each * credential belongs to. + * - *Duplicated identity.* Posting the same designated identity twice, each with + * a masked secret, would restore one stored credential onto both entries — + * cloning it onto a URL the operator never gave it to. * * So a masked entry is restored only on one of two proofs: * * 1. The manifest designates an immutable identity property via - * `x-paperclip-identity`, and exactly one stored entry carries that identity. - * Declaring it asserts the property never changes for a given entry, which is - * what makes reorder and edit safe. + * `x-paperclip-identity`, and that identity is carried by exactly one stored + * entry *and* exactly one incoming entry. Declaring it asserts the property + * never changes for a given entry, which is what makes reorder and edit safe; + * requiring uniqueness on both sides is what stops a duplicate from cloning. * 2. Failing that, the arrays are the same length and the entry at the same * index is exactly this entry with its secrets blanked * ({@link matchesIgnoringMask}). Anything else — a reorder, an insertion, a @@ -819,6 +1029,13 @@ export function mergeMaskedPluginConfig( nodes: SchemaNode[], ): unknown[] { const identityKey = designatedIdentityKey(nodes, root); + // Uniqueness must hold on the *incoming* side too. Proving only that one + // stored entry carries the identity lets two posted entries claiming the + // same identity both restore that single credential — cloning it onto an + // entry the operator never held it for, e.g. a second endpoint URL. + const incomingIdentityCounts = identityKey + ? identityOccurrences(incoming, identityKey) + : null; return incoming .map((entry, index) => { @@ -832,15 +1049,17 @@ export function mergeMaskedPluginConfig( let storedEntry: unknown; if (identityKey && isPlainRecord(entry)) { - // Proof 1: a manifest-designated immutable identity, matched uniquely. + // Proof 1: a manifest-designated immutable identity, carried by + // exactly one incoming entry and exactly one stored entry. const wanted = identityValue(entry, identityKey); - const matches = - wanted === undefined - ? [] - : storedArray.filter( - (candidate) => - isPlainRecord(candidate) && identityValue(candidate, identityKey) === wanted, - ); + const uniqueIncoming = + wanted !== undefined && incomingIdentityCounts?.get(wanted) === 1; + const matches = uniqueIncoming + ? storedArray.filter( + (candidate) => + isPlainRecord(candidate) && identityValue(candidate, identityKey) === wanted, + ) + : []; storedEntry = matches.length === 1 ? matches[0] : undefined; } else if (incoming.length === storedArray.length) { // Proof 2: same shape, same place, identical but for the secrets.