fix(llm-security): commons-loader - drop policy-driven root, ship path, fix cache [skip-docs]
Advisor review on the prior commit (69cad7c) caught a real detection-kill vulnerability before push: reading `commons.root` from the SCANNED TARGET's .llm-security/policy.json let a hostile cloned repo redirect llm-security's own detection corpus to an attacker-supplied (empty) one, with graceful-empty fallback making the substitution silent — a substitutive override, unlike sig.custom_rules_path's additive one. Dropped the policy import entirely; commons location is this plugin's own concern, resolved only from __dirname or an explicit test/dev override, never from policy or the scan target. Also fixed two issues the review surfaced: - Default vendor path was repo-root `shared/`, which package.json's `files` allowlist (bin/, scanners/, knowledge/) would never publish — moved under scanners/commons/, inside the directory that actually ships. Same defect class as2fe2915(green dev checkout, empty detection tables once installed). - Cache keyed success/failure together, so the first caller's `fallback` shape (e.g. []) leaked to a second caller expecting a different shape ({}) on the same missing artifact. Cache now stores a load-failed sentinel and returns each caller's own fallback. Loaded artifacts are also deep-frozen, since the cache hands out one shared object by reference to every caller. New/changed tests cover all four: a simulated hostile-target policy file is ignored, the failure-cache no longer cross-contaminates fallback shapes, and mutating a loaded artifact throws. Golden baseline unchanged; full suite 2063/2063. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QAYkRaBXT6tmWXTQAi1ZBg
This commit is contained in:
parent
69cad7c973
commit
b0de0ca6d8
2 changed files with 92 additions and 71 deletions
|
|
@ -7,51 +7,62 @@
|
|||
// hardcoded tables to this loader in Phase 5 step 4, table-by-table, behind
|
||||
// the golden gate.
|
||||
//
|
||||
// Modeled on signature-scanner.mjs's loadRules()/loadCustomRules() pair:
|
||||
// hooks run per-tool-call in fresh zero-dep processes, so resolution must be
|
||||
// a fast synchronous read of the vendored copy, never network. Cached once
|
||||
// per process. A missing/unvendored commons dir degrades to the caller's
|
||||
// fallback rather than crashing a hook — the same graceful-empty contract
|
||||
// loadRules() uses for a missing knowledge/signatures.json.
|
||||
// Modeled on signature-scanner.mjs's loadRules(): hooks run per-tool-call in
|
||||
// fresh zero-dep processes, so resolution must be a fast synchronous read of
|
||||
// the vendored copy, never network. Cached once per process. A
|
||||
// missing/unvendored commons dir degrades to the caller's fallback rather
|
||||
// than crashing a hook — the same graceful-empty contract loadRules() uses
|
||||
// for a missing knowledge/signatures.json.
|
||||
//
|
||||
// Deliberately NOT policy-driven, unlike loadCustomRules()'s
|
||||
// sig.custom_rules_path: this plugin scans untrusted cloned repos, and
|
||||
// `.llm-security/policy.json` lives in the *scanned target*. A
|
||||
// custom_rules_path override can only ever add findings a hostile target
|
||||
// supplies; a commons-root override would let that target *replace* this
|
||||
// plugin's own detection corpus wholesale, with graceful-empty fallback
|
||||
// making the substitution silent. The commons location is this plugin's own
|
||||
// concern, not the scan target's — so the only override is the explicit
|
||||
// `commonsRoot` option (tests, a local commons dev checkout), resolved from
|
||||
// this file's own location, never from policy or the scan target.
|
||||
//
|
||||
// Zero external dependencies — Node.js builtins only.
|
||||
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { join, dirname, isAbsolute, resolve } from 'node:path';
|
||||
import { join, dirname } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { getPolicyValue } from './policy-loader.mjs';
|
||||
|
||||
const __dirname = dirname(fileURLToPath(import.meta.url));
|
||||
|
||||
// Default vendored location: llm-security-commons is pulled in as a
|
||||
// pull-only subtree at the repo root, mirroring the portfolio-optimiser
|
||||
// family's `shared/` convention (docs/commons-extraction-plan.local.md).
|
||||
const DEFAULT_COMMONS_ROOT = join(__dirname, '..', '..', 'shared');
|
||||
// pull-only subtree under scanners/, which is the only source directory
|
||||
// `package.json`'s `files` allowlist ships (bin/, scanners/, knowledge/ —
|
||||
// scripts/ and tests/ do not publish, and neither would a repo-root dir).
|
||||
const DEFAULT_COMMONS_ROOT = join(__dirname, '..', 'commons');
|
||||
|
||||
// Cached, parsed artifacts, keyed by resolved absolute file path.
|
||||
// Cached parsed artifacts, keyed by resolved absolute file path. A failed
|
||||
// load caches this sentinel (not the caller's fallback value) so that two
|
||||
// callers requesting the same missing artifact with different fallback
|
||||
// shapes (`[]` vs `{}`) each still get their own fallback back, rather than
|
||||
// the first caller's shape leaking to the second.
|
||||
const _cache = new Map();
|
||||
const LOAD_FAILED = Symbol('commons-loader:load-failed');
|
||||
|
||||
/**
|
||||
* Resolve the commons root directory.
|
||||
* Precedence: explicit `commonsRoot` option > `commons.root` policy value
|
||||
* (relative paths resolve against `targetPath`) > the default vendored path.
|
||||
* @param {string|undefined} targetPath
|
||||
* @param {string|undefined} commonsRoot
|
||||
* @returns {string}
|
||||
*/
|
||||
function resolveCommonsRoot(targetPath, commonsRoot) {
|
||||
if (commonsRoot) return commonsRoot;
|
||||
const policyRoot = getPolicyValue('commons', 'root', null, targetPath);
|
||||
if (policyRoot && typeof policyRoot === 'string') {
|
||||
return isAbsolute(policyRoot) ? policyRoot : resolve(targetPath || process.cwd(), policyRoot);
|
||||
/** Recursively freeze a parsed JSON value so callers cannot mutate a shared cached table. */
|
||||
function deepFreeze(value) {
|
||||
if (value !== null && typeof value === 'object' && !Object.isFrozen(value)) {
|
||||
Object.freeze(value);
|
||||
for (const key of Object.keys(value)) deepFreeze(value[key]);
|
||||
}
|
||||
return DEFAULT_COMMONS_ROOT;
|
||||
return value;
|
||||
}
|
||||
|
||||
/**
|
||||
* Load and parse one commons JSON artifact.
|
||||
* Graceful fallback on any read/parse error — an unvendored, missing, or
|
||||
* invalid commons artifact must never crash a hook or scanner.
|
||||
* invalid commons artifact must never crash a hook or scanner. The returned
|
||||
* value is deep-frozen: every caller shares the same cached object, so
|
||||
* mutating it would silently leak across callers/targets; freezing makes
|
||||
* that throw instead.
|
||||
*
|
||||
* @param {string} artifactPath - relative path under the commons root,
|
||||
* without the `.json` extension (e.g. `'lexicon/injection-lexicon'`).
|
||||
|
|
@ -59,25 +70,27 @@ function resolveCommonsRoot(targetPath, commonsRoot) {
|
|||
* @param {*} [opts.fallback] - value returned on any load/parse failure
|
||||
* (default: `null`). Callers pick the shape-appropriate empty value
|
||||
* (`[]`, `{}`, ...) the same way loadRules() falls back to `[]`.
|
||||
* @param {string} [opts.commonsRoot] - explicit commons root, overriding
|
||||
* policy and the default (for tests and one-off callers).
|
||||
* @param {string} [opts.targetPath] - scan root used to resolve a
|
||||
* policy-relative `commons.root` and to locate `.llm-security/policy.json`.
|
||||
* @returns {*} Parsed JSON, or `opts.fallback` on failure.
|
||||
* @param {string} [opts.commonsRoot] - explicit commons root, overriding the
|
||||
* default vendored path (for tests and a local commons dev checkout).
|
||||
* @returns {*} Parsed, frozen JSON, or `opts.fallback` on failure.
|
||||
*/
|
||||
export function loadArtifact(artifactPath, opts = {}) {
|
||||
const { fallback = null, commonsRoot, targetPath } = opts;
|
||||
const root = resolveCommonsRoot(targetPath, commonsRoot);
|
||||
const { fallback = null, commonsRoot } = opts;
|
||||
const root = commonsRoot || DEFAULT_COMMONS_ROOT;
|
||||
const filePath = join(root, `${artifactPath}.json`);
|
||||
|
||||
if (_cache.has(filePath)) return _cache.get(filePath);
|
||||
if (_cache.has(filePath)) {
|
||||
const cached = _cache.get(filePath);
|
||||
return cached === LOAD_FAILED ? fallback : cached;
|
||||
}
|
||||
|
||||
let result;
|
||||
try {
|
||||
const raw = readFileSync(filePath, 'utf8');
|
||||
result = JSON.parse(raw);
|
||||
result = deepFreeze(JSON.parse(raw));
|
||||
} catch {
|
||||
result = fallback; // graceful: unvendored/missing/invalid -> caller's empty shape
|
||||
_cache.set(filePath, LOAD_FAILED); // graceful: unvendored/missing/invalid -> caller's fallback
|
||||
return fallback;
|
||||
}
|
||||
|
||||
_cache.set(filePath, result);
|
||||
|
|
|
|||
|
|
@ -12,12 +12,9 @@ import { writeFileSync, mkdirSync, rmSync } from 'node:fs';
|
|||
import { join } from 'node:path';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { loadArtifact, _resetCacheForTest } from '../../scanners/lib/commons-loader.mjs';
|
||||
import { _resetCacheForTest as _resetPolicyCacheForTest } from '../../scanners/lib/policy-loader.mjs';
|
||||
|
||||
const TEST_ROOT = join(tmpdir(), `llm-security-commons-loader-test-${Date.now()}`);
|
||||
const FIXTURE_COMMONS_ROOT = join(TEST_ROOT, 'fixture-commons');
|
||||
const POLICY_DIR = join(TEST_ROOT, '.llm-security');
|
||||
const POLICY_FILE = join(POLICY_DIR, 'policy.json');
|
||||
|
||||
function writeArtifact(relPath, data) {
|
||||
const filePath = join(FIXTURE_COMMONS_ROOT, `${relPath}.json`);
|
||||
|
|
@ -28,13 +25,11 @@ function writeArtifact(relPath, data) {
|
|||
describe('commons-loader', () => {
|
||||
beforeEach(() => {
|
||||
_resetCacheForTest();
|
||||
_resetPolicyCacheForTest();
|
||||
mkdirSync(FIXTURE_COMMONS_ROOT, { recursive: true });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
_resetCacheForTest();
|
||||
_resetPolicyCacheForTest();
|
||||
try { rmSync(TEST_ROOT, { recursive: true }); } catch {}
|
||||
});
|
||||
|
||||
|
|
@ -57,9 +52,11 @@ describe('commons-loader', () => {
|
|||
assert.deepEqual(data, {});
|
||||
});
|
||||
|
||||
it('degrades gracefully when commons has not been vendored yet (no override, no policy)', () => {
|
||||
// Phase 4 hasn't run in this repo checkout: the default `shared/` vendor
|
||||
// path does not exist. The loader must not throw.
|
||||
it('degrades gracefully when commons has not been vendored yet (no commonsRoot override)', () => {
|
||||
// Phase 4 hasn't run in this repo checkout: the default scanners/commons
|
||||
// vendor path does not exist. The loader must not throw, and — since the
|
||||
// commons location is never policy- or target-derived — this cannot be
|
||||
// influenced by an on-disk .llm-security/policy.json either.
|
||||
const data = loadArtifact('lexicon/injection-lexicon', { fallback: 'EMPTY' });
|
||||
assert.equal(data, 'EMPTY');
|
||||
});
|
||||
|
|
@ -73,7 +70,7 @@ describe('commons-loader', () => {
|
|||
assert.equal(second.entropyFloor, 4.5); // original value, not the mutation
|
||||
});
|
||||
|
||||
it('caches a failed load too, so a fixed-but-unread file still returns the cached fallback', () => {
|
||||
it('caches a failed load too, so a fixed-but-unread file still returns the fallback', () => {
|
||||
const first = loadArtifact('signatures/malware-signatures', { commonsRoot: FIXTURE_COMMONS_ROOT, fallback: [] });
|
||||
writeArtifact('signatures/malware-signatures', { rules: [{ id: 'late-arrival' }] });
|
||||
const second = loadArtifact('signatures/malware-signatures', { commonsRoot: FIXTURE_COMMONS_ROOT, fallback: [] });
|
||||
|
|
@ -81,36 +78,47 @@ describe('commons-loader', () => {
|
|||
assert.deepEqual(second, []);
|
||||
});
|
||||
|
||||
it('resolves commonsRoot from the commons.root policy value when no explicit override is given', () => {
|
||||
mkdirSync(POLICY_DIR, { recursive: true });
|
||||
writeFileSync(POLICY_FILE, JSON.stringify({ commons: { root: FIXTURE_COMMONS_ROOT } }));
|
||||
writeArtifact('codepoints/carriers', { zeroWidth: [''] });
|
||||
const data = loadArtifact('codepoints/carriers', { targetPath: TEST_ROOT, fallback: null });
|
||||
assert.deepEqual(data, { zeroWidth: [''] });
|
||||
});
|
||||
|
||||
it('resolves a relative commons.root policy value against targetPath', () => {
|
||||
mkdirSync(POLICY_DIR, { recursive: true });
|
||||
writeFileSync(POLICY_FILE, JSON.stringify({ commons: { root: './fixture-commons' } }));
|
||||
writeArtifact('lexicon/injection-lexicon', { version: '0.1.0', terms: [] });
|
||||
const data = loadArtifact('lexicon/injection-lexicon', { targetPath: TEST_ROOT, fallback: null });
|
||||
assert.deepEqual(data, { version: '0.1.0', terms: [] });
|
||||
});
|
||||
|
||||
it('an explicit commonsRoot option takes precedence over the commons.root policy value', () => {
|
||||
mkdirSync(POLICY_DIR, { recursive: true });
|
||||
writeFileSync(POLICY_FILE, JSON.stringify({ commons: { root: join(TEST_ROOT, 'does-not-exist') } }));
|
||||
writeArtifact('lexicon/injection-lexicon', { version: '0.1.0', terms: ['override-wins'] });
|
||||
const data = loadArtifact('lexicon/injection-lexicon', {
|
||||
targetPath: TEST_ROOT,
|
||||
commonsRoot: FIXTURE_COMMONS_ROOT,
|
||||
fallback: null,
|
||||
});
|
||||
assert.deepEqual(data, { version: '0.1.0', terms: ['override-wins'] });
|
||||
it('a cached failure does not leak one caller\'s fallback shape to another', () => {
|
||||
const arrayFallback = loadArtifact('missing/artifact', { commonsRoot: FIXTURE_COMMONS_ROOT, fallback: [] });
|
||||
const objectFallback = loadArtifact('missing/artifact', { commonsRoot: FIXTURE_COMMONS_ROOT, fallback: {} });
|
||||
assert.deepEqual(arrayFallback, []);
|
||||
assert.deepEqual(objectFallback, {}); // not [] from the first caller's cached fallback
|
||||
});
|
||||
|
||||
it('defaults the fallback to null when the caller passes none', () => {
|
||||
const data = loadArtifact('lexicon/injection-lexicon', { commonsRoot: FIXTURE_COMMONS_ROOT });
|
||||
assert.equal(data, null);
|
||||
});
|
||||
|
||||
it('freezes a loaded artifact so a caller mutation throws instead of leaking process-wide', () => {
|
||||
writeArtifact('lexicon/injection-lexicon', { version: '0.1.0', terms: ['a'] });
|
||||
const data = loadArtifact('lexicon/injection-lexicon', { commonsRoot: FIXTURE_COMMONS_ROOT });
|
||||
assert.throws(() => { data.terms.push('mutated'); }, TypeError);
|
||||
assert.throws(() => { data.version = '9.9.9'; }, TypeError);
|
||||
});
|
||||
|
||||
it('does not read .llm-security/policy.json from any target — commons.root is not honored', () => {
|
||||
// Simulates a hostile scanned target shipping .llm-security/policy.json
|
||||
// with {"commons":{"root": "..."}} to redirect detection data. Even
|
||||
// with such a file on disk and CLAUDE_PROJECT_ROOT pointed at it, the
|
||||
// loader must ignore it entirely and fall through to the caller's
|
||||
// explicit commonsRoot / default, never to policy.
|
||||
const hostileTargetRoot = join(TEST_ROOT, 'hostile-target');
|
||||
const hostilePolicyDir = join(hostileTargetRoot, '.llm-security');
|
||||
mkdirSync(hostilePolicyDir, { recursive: true });
|
||||
writeFileSync(
|
||||
join(hostilePolicyDir, 'policy.json'),
|
||||
JSON.stringify({ commons: { root: join(TEST_ROOT, 'attacker-controlled-empty-commons') } }),
|
||||
);
|
||||
const prevProjectRoot = process.env.CLAUDE_PROJECT_ROOT;
|
||||
process.env.CLAUDE_PROJECT_ROOT = hostileTargetRoot;
|
||||
try {
|
||||
writeArtifact('lexicon/injection-lexicon', { version: '0.1.0', terms: ['still-here'] });
|
||||
const data = loadArtifact('lexicon/injection-lexicon', { commonsRoot: FIXTURE_COMMONS_ROOT, fallback: null });
|
||||
assert.deepEqual(data, { version: '0.1.0', terms: ['still-here'] });
|
||||
} finally {
|
||||
if (prevProjectRoot === undefined) delete process.env.CLAUDE_PROJECT_ROOT;
|
||||
else process.env.CLAUDE_PROJECT_ROOT = prevProjectRoot;
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue