diff --git a/apps/cli/src/commands/scan.ts b/apps/cli/src/commands/scan.ts index 2bb84e1..edfbb57 100644 --- a/apps/cli/src/commands/scan.ts +++ b/apps/cli/src/commands/scan.ts @@ -36,6 +36,16 @@ export interface ScanCommandOptions { dependencies?: boolean; /** Globs to skip, merged with any `.threatcrushignore` at the scan root. */ exclude?: readonly string[]; + /** + * Also report security controls the tree shows no evidence of — headers, + * CSRF, rate limiting, body size limits. + * + * Off by default, and deliberately so. Every other finding points at a line; + * these point at an absence, which a reverse proxy or a gateway may already + * be covering from outside the repository. Opt-in keeps a normal scan made + * only of things that are actually there. + */ + missingControls?: boolean; } interface ScanOutcome { @@ -175,6 +185,7 @@ export async function scanCommand( let seen = 0; const report = scanPath(targetPath, { exclude: options.exclude, + missingControls: options.missingControls, onFile: () => { seen += 1; if (spinner) spinner.text = `Scanning files... (${seen} files)`; diff --git a/apps/cli/src/index.ts b/apps/cli/src/index.ts index 1e9299e..3f05416 100644 --- a/apps/cli/src/index.ts +++ b/apps/cli/src/index.ts @@ -260,6 +260,10 @@ program (value: string, previous: string[]) => [...previous, value], [] as string[], ) + .option( + "--missing-controls", + "also report security controls the tree shows no evidence of (headers, CSRF, rate limiting, body size limits)", + ) .option("-v, --verbose", "list the paths that could not be read") .action(async (targetPath: string, opts: { format?: string; @@ -268,6 +272,7 @@ program pathPrefix?: string; deps?: boolean; exclude?: string[]; + missingControls?: boolean; verbose?: boolean; }) => { const format = (opts.format ?? "text").toLowerCase(); @@ -291,6 +296,7 @@ program pathPrefix: opts.pathPrefix, dependencies: opts.deps, exclude: opts.exclude, + missingControls: opts.missingControls, verbose: opts.verbose, }); }); diff --git a/packages/scan/src/__tests__/controls.test.ts b/packages/scan/src/__tests__/controls.test.ts new file mode 100644 index 0000000..4650303 --- /dev/null +++ b/packages/scan/src/__tests__/controls.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, it } from 'vitest'; +import { ControlAudit, SECURITY_CONTROLS } from '../controls'; + +const auditOf = (files: Record): ControlAudit => { + const audit = new ControlAudit(); + for (const [path, text] of Object.entries(files)) audit.observe(path, text); + return audit; +}; + +const missingIds = (files: Record): string[] => + auditOf(files).missing().map((control) => control.id); + +describe('applicability', () => { + it('says nothing about a tree that serves no HTTP', () => { + // The most important test here. "This library has no CSRF token" is a + // category error, and reporting it is the fastest way to teach an operator + // that these findings are noise. + expect(missingIds({ 'sum.js': 'export const add = (a, b) => a + b;' })).toEqual([]); + }); + + it('reports nothing to anchor to when no server was found', () => { + expect(auditOf({ 'sum.js': 'export const add = (a, b) => a + b;' }).findings()).toEqual([]); + }); + + it('engages once a server is constructed', () => { + const missing = missingIds({ 'server.js': 'const app = express();\napp.listen(3000);' }); + expect(missing).toHaveLength(SECURITY_CONTROLS.length); + }); +}); + +describe('evidence', () => { + it('accepts a control installed in a different file from the server', () => { + // The whole reason this is a walk-wide accumulator rather than a per-file + // rule: the limiter in `app.js` protects the route in `routes/admin.js`. + const missing = missingIds({ + 'server.js': 'const app = express();', + 'middleware/security.js': "const helmet = require('helmet');\napp.use(helmet());", + }); + expect(missing).not.toContain('control-security-headers-absent'); + }); + + it('accepts a SameSite cookie as anti-CSRF', () => { + // A cookie the browser refuses to send cross-site is not reachable by the + // attack the control exists to stop. + const missing = missingIds({ + 'server.js': "const app = express();\napp.use(session({ cookie: { sameSite: 'strict' } }));", + }); + expect(missing).not.toContain('control-anti-csrf-absent'); + }); + + it('accepts a rate limiter', () => { + const missing = missingIds({ + 'server.js': "const app = express();\nconst rateLimit = require('express-rate-limit');", + }); + expect(missing).not.toContain('control-rate-limiting-absent'); + }); + + it('accepts a body size limit', () => { + const missing = missingIds({ + 'server.js': "const app = express();\napp.use(express.json({ limit: '100kb' }));", + }); + expect(missing).not.toContain('control-body-size-limit-absent'); + }); + + it('reports the ones with no evidence anywhere', () => { + const missing = missingIds({ + 'server.js': "const app = express();\napp.use(helmet());", + }); + expect(missing).not.toContain('control-security-headers-absent'); + expect(missing).toContain('control-anti-csrf-absent'); + expect(missing).toContain('control-rate-limiting-absent'); + }); +}); + +describe('findings', () => { + it('anchors to the file that builds the server', () => { + const findings = auditOf({ + 'lib/util.js': 'export const noop = () => {};', + 'src/server.js': 'const app = express();', + }).findings(); + expect(findings.every((finding) => finding.file === 'src/server.js')).toBe(true); + expect(findings.every((finding) => finding.line === 1)).toBe(true); + }); + + it('never claims more than `pattern` confidence', () => { + // An absence is not evidence. The confidence value is what caps the + // severity, and it is what the report shows the operator. + const findings = auditOf({ 'server.js': 'const app = express();' }).findings(); + expect(findings.length).toBeGreaterThan(0); + expect(findings.every((finding) => finding.confidence === 'pattern')).toBe(true); + expect(findings.every((finding) => finding.severity === 'medium')).toBe(true); + }); +}); diff --git a/packages/scan/src/__tests__/node-rules.test.ts b/packages/scan/src/__tests__/node-rules.test.ts new file mode 100644 index 0000000..b7928fe --- /dev/null +++ b/packages/scan/src/__tests__/node-rules.test.ts @@ -0,0 +1,382 @@ +import { describe, expect, it } from 'vitest'; +import { scanText } from '../text'; + +/** + * Same discipline as `code-rules.test.ts`: every class is tested as a *pair* — + * the vulnerable shape and the corrected shape that sits next to it in real + * code. A rule that only passes the first half has measured nothing, because a + * rule matching every line passes it too. + * + * Positives assert with `toContain` and negatives with `not.toContain` rather + * than on the length of the result. These sources deliberately look like real + * server code, so unrelated rules fire on them; asserting emptiness would make + * every test brittle to a rule it was not written about. + */ +const ruleIds = (path: string, source: string): string[] => + scanText(path, source).map((finding) => finding.ruleId); + +describe('code execution', () => { + it('flags a vm script whose source can be influenced', () => { + const source = ['const script = req.body.script;', 'vm.runInNewContext(script, sandbox);'].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-vm-untrusted-execution'); + }); + + it('stays silent on a vm script that is a constant', () => { + // A plugin loader evaluating a fixed expression is not this class. + expect(ruleIds('a.js', 'vm.runInNewContext("2 + 2", {});')).not.toContain( + 'js-vm-untrusted-execution', + ); + }); + + it('flags vm2 regardless of context', () => { + const source = "const { NodeVM } = require('vm2');"; + expect(ruleIds('a.js', source)).toContain('js-vm2-sandbox'); + }); + + it('flags a function-reconstructing deserialiser', () => { + const source = [ + "const serialize = require('node-serialize');", + 'const data = serialize.unserialize(input);', + ].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-function-deserialization'); + }); + + it('stays silent on a project’s own deserialize helper', () => { + // Without the import there is no reason to think this call executes + // anything. `deserialize` is one of the most reused names in the ecosystem. + const source = [ + 'function deserialize(raw) { return JSON.parse(raw); }', + 'const data = deserialize(input);', + ].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-function-deserialization'); + }); + + it('flags a template compiled from request data', () => { + const source = [ + 'const source = req.body.template;', + 'const render = handlebars.compile(source);', + ].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-template-injection'); + }); + + it('stays silent on a template read from disk', () => { + const source = [ + "const source = fs.readFileSync('views/email.hbs', 'utf-8');", + 'const render = handlebars.compile(source);', + ].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-template-injection'); + }); + + it('flags an interpolated shelljs command', () => { + const source = ["const shell = require('shelljs');", 'shell.exec(`git checkout ${branch}`);'].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-shelljs-command-execution'); + }); +}); + +describe('injection', () => { + it('flags a $where predicate assembled by concatenation', () => { + // The string holds the *other* quote character, which is the shape a + // single combined quote class silently fails to match. + const source = `const query = { $where: "this.name === '" + name + "'" };`; + expect(ruleIds('a.js', source)).toContain('js-nosql-where-expression'); + }); + + it('stays silent on an operator query with no $where', () => { + const source = 'const query = { balance: { $gt: 100 } };'; + expect(ruleIds('a.js', source)).not.toContain('js-nosql-where-expression'); + }); + + it('flags an XPath predicate built by concatenation', () => { + // Backticks quote the fixture because the source line itself contains both + // a double and a single quote — the shape the rule exists to catch. + const source = [ + "const xpath = require('xpath');", + `const nodes = xpath.select("//user[@name='" + name + "']", doc);`, + ].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-xpath-injection'); + }); + + it('stays silent on a constant XPath expression', () => { + const source = ["const xpath = require('xpath');", `const nodes = xpath.select("//user[@id='42']", doc);`].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-xpath-injection'); + }); + + it('flags a regular expression compiled from request data', () => { + const source = ['const pattern = req.query.filter;', 'const re = new RegExp(pattern);'].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-regex-from-input'); + }); + + it('stays silent on a literal regular expression', () => { + expect(ruleIds('a.js', `const re = new RegExp('^[a-z]+$');`)).not.toContain('js-regex-from-input'); + }); +}); + +describe('XML', () => { + it('flags a parser told to resolve entities', () => { + expect(ruleIds('a.js', 'const doc = libxml.parseXml(body, { noent: true });')).toContain( + 'js-xml-external-entities', + ); + }); + + it('stays silent when entity resolution is left off', () => { + expect(ruleIds('a.js', 'const doc = libxml.parseXml(body, { noent: false });')).not.toContain( + 'js-xml-external-entities', + ); + }); +}); + +describe('tokens', () => { + it('flags the none algorithm', () => { + expect(ruleIds('a.js', `const token = jwt.sign(claims, null, { algorithm: 'none' });`)).toContain( + 'js-jwt-none-algorithm', + ); + }); + + it('stays silent on a pinned real algorithm', () => { + expect( + ruleIds('a.js', `const claims = jwt.verify(token, pub, { algorithms: ['RS256'] });`), + ).not.toContain('js-jwt-none-algorithm'); + }); +}); + +describe('cryptography', () => { + it('flags a broken cipher', () => { + expect(ruleIds('a.js', `const c = crypto.createCipheriv('des-ede3-cbc', k, iv);`)).toContain( + 'js-broken-cipher-algorithm', + ); + }); + + it('stays silent on AES-GCM', () => { + expect(ruleIds('a.js', `const c = crypto.createCipheriv('aes-256-gcm', k, iv);`)).not.toContain( + 'js-broken-cipher-algorithm', + ); + }); + + it('flags ECB mode', () => { + expect(ruleIds('a.js', `const c = crypto.createCipheriv('aes-128-ecb', k, null);`)).toContain( + 'js-ecb-mode-cipher', + ); + }); + + it('flags the legacy passphrase-derived cipher API', () => { + expect(ruleIds('a.js', `const c = crypto.createCipher('aes-256-cbc', pass);`)).toContain( + 'js-legacy-cipher-api', + ); + }); + + it('stays silent on the explicit-IV replacement', () => { + // `createCipheriv` is the fix, and it shares a prefix with the defect. + expect(ruleIds('a.js', `const c = crypto.createCipheriv('aes-256-cbc', k, iv);`)).not.toContain( + 'js-legacy-cipher-api', + ); + }); +}); + +describe('cross-site scripting', () => { + it('flags auto-escaping turned off', () => { + expect(ruleIds('a.js', `nunjucks.configure('views', { autoescape: false });`)).toContain( + 'js-template-autoescape-disabled', + ); + }); + + it('flags serialize-javascript in unsafe mode', () => { + const source = [ + "const serialize = require('serialize-javascript');", + 'const payload = serialize(state, { unsafe: true });', + ].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-serialize-javascript-unsafe'); + }); + + it('flags an origin echoed back from the request', () => { + const source = `res.setHeader('Access-Control-Allow-Origin', req.headers.origin);`; + expect(ruleIds('a.js', source)).toContain('js-cors-origin-reflected'); + }); + + it('stays silent on a constant allowed origin', () => { + const source = `res.setHeader('Access-Control-Allow-Origin', 'https://app.example.com');`; + expect(ruleIds('a.js', source)).not.toContain('js-cors-origin-reflected'); + }); + + it('stays silent when the origin is checked against a list first', () => { + // The one correct way to echo an origin, and the reason this rule needs a + // guard of its own rather than the generic one. + const source = [ + 'if (allowedOrigins.includes(req.headers.origin)) {', + ` res.setHeader('Access-Control-Allow-Origin', req.headers.origin);`, + '}', + ].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-cors-origin-reflected'); + }); +}); + +describe('server-side request forgery', () => { + it('flags a headless browser sent to a computed URL', () => { + const source = [ + "const puppeteer = require('puppeteer');", + 'const target = req.query.url;', + 'await page.goto(target);', + ].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-headless-browser-navigation'); + }); + + it('stays silent on a constant URL', () => { + const source = ["const puppeteer = require('puppeteer');", `await page.goto('https://example.com/r');`].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-headless-browser-navigation'); + }); +}); + +describe('archive extraction', () => { + it('flags an entry name joined onto the destination', () => { + const source = [ + "const unzipper = require('unzipper');", + 'const dest = path.join(outDir, entry.path);', + 'fs.writeFileSync(dest, await entry.buffer());', + ].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-archive-entry-path'); + }); + + it('stays silent when containment is checked after the join', () => { + // The check necessarily comes *after* the path is built, which is why this + // rule looks forward as well as back. + const source = [ + "const unzipper = require('unzipper');", + 'const dest = path.join(outDir, entry.path);', + "if (!dest.startsWith(outDir + path.sep)) throw new Error('unsafe entry');", + ].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-archive-entry-path'); + }); +}); + +describe('Electron', () => { + it('flags node access in the renderer', () => { + expect(ruleIds('a.js', 'webPreferences: { nodeIntegration: true },')).toContain( + 'js-electron-node-integration', + ); + }); + + it('stays silent on the hardened defaults', () => { + expect( + ruleIds('a.js', 'webPreferences: { nodeIntegration: false, contextIsolation: true },'), + ).not.toContain('js-electron-node-integration'); + }); + + it('flags web security switched off', () => { + expect(ruleIds('a.js', 'webPreferences: { webSecurity: false },')).toContain( + 'js-electron-web-security-disabled', + ); + }); + + it('flags openExternal with a computed target', () => { + const source = ['const target = req.query.next;', 'shell.openExternal(target);'].join('\n'); + expect(ruleIds('a.js', source)).toContain('js-electron-open-external'); + }); +}); + +describe('hardening and resource limits', () => { + it('flags a security header explicitly disabled', () => { + expect(ruleIds('a.js', 'app.use(helmet({ frameguard: false }));')).toContain( + 'js-helmet-protection-disabled', + ); + }); + + it('flags a buffer write with bounds checking off', () => { + expect(ruleIds('a.js', 'buf.writeUInt32BE(value, 0, true);')).toContain( + 'js-buffer-bounds-check-disabled', + ); + }); + + it('stays silent on a checked buffer write', () => { + expect(ruleIds('a.js', 'buf.writeUInt32BE(value, 0);')).not.toContain( + 'js-buffer-bounds-check-disabled', + ); + }); + + it('flags an unzeroed allocation', () => { + expect(ruleIds('a.js', 'const buf = Buffer.allocUnsafe(1024);')).toContain( + 'js-uninitialized-buffer', + ); + }); + + it('stays silent on the zeroing allocator', () => { + expect(ruleIds('a.js', 'const buf = Buffer.alloc(1024);')).not.toContain('js-uninitialized-buffer'); + }); + + it('flags an ineffective body limit', () => { + expect(ruleIds('a.js', `app.use(express.json({ limit: '50mb' }));`)).toContain( + 'js-oversized-request-body-limit', + ); + }); + + it('stays silent on a limit that actually limits', () => { + expect(ruleIds('a.js', `app.use(express.json({ limit: '1mb' }));`)).not.toContain( + 'js-oversized-request-body-limit', + ); + }); +}); + +describe('information disclosure', () => { + it('flags a stack trace returned through a status chain', () => { + expect(ruleIds('a.js', 'res.status(500).send(err.stack);')).toContain('js-error-detail-returned'); + }); + + it('stays silent on a generic error message', () => { + expect(ruleIds('a.js', `res.status(500).json({ message: 'Internal server error' });`)).not.toContain( + 'js-error-detail-returned', + ); + }); +}); + +describe('the shared guard does not silence a whole neighbourhood', () => { + it('keeps reporting near a CORS header line', () => { + // Regression. `GENERIC_GUARD` is case-insensitive and its allow-list + // heuristic matched the "Allow" inside `Access-Control-Allow-Origin`. + // Since the guard is tested against an 8-line window, one header line + // suppressed every guardable rule around it — silently, and + // indistinguishably from having found nothing. + const source = [ + "app.post('/restore', (req, res) => {", + " res.setHeader('Access-Control-Allow-Origin', req.headers.origin);", + " const token = jwt.sign({ sub: req.query.id }, null, { algorithm: 'none' });", + ' res.status(500).send(err.stack);', + '});', + ].join('\n'); + + const ids = ruleIds('a.js', source); + expect(ids).toContain('js-jwt-none-algorithm'); + expect(ids).toContain('js-error-detail-returned'); + }); + + it('still guards on a real allow-list in code', () => { + // The fix is scoped to the CORS header prefix, so an actual allow-list + // must keep guarding exactly as before. Asserted with code rather than a + // comment: comment lines are already excluded from the guard window on + // purpose, since a comment mentioning an allow-list is not one. + const source = ["const allowlist = ['a.example.com'];", 'const re = new RegExp(req.query.q);'].join('\n'); + expect(ruleIds('a.js', source)).not.toContain('js-regex-from-input'); + }); +}); + +describe('confidence model', () => { + it('caps a bare construct at medium and escalates only with visible input', () => { + const bare = scanText('a.js', 'const re = new RegExp(pattern);').find( + (f) => f.ruleId === 'js-regex-from-input', + ); + // `needsContext` means the bare form is not reported at all, which is the + // stronger version of the cap. + expect(bare).toBeUndefined(); + + const withInput = scanText( + 'a.js', + ['const pattern = req.query.filter;', 'const re = new RegExp(pattern);'].join('\n'), + ).find((f) => f.ruleId === 'js-regex-from-input'); + expect(withInput?.confidence).toBe('contextual'); + }); + + it('reports an inherent rule as evidence at its declared severity', () => { + const finding = scanText('a.js', "const { NodeVM } = require('vm2');").find( + (f) => f.ruleId === 'js-vm2-sandbox', + ); + expect(finding?.confidence).toBe('evidence'); + expect(finding?.severity).toBe('high'); + }); +}); diff --git a/packages/scan/src/__tests__/template-rules.test.ts b/packages/scan/src/__tests__/template-rules.test.ts new file mode 100644 index 0000000..5e754d3 --- /dev/null +++ b/packages/scan/src/__tests__/template-rules.test.ts @@ -0,0 +1,142 @@ +import { describe, expect, it } from 'vitest'; +import { SCAN_EXTENSIONS, scanText } from '../text'; +import { TEMPLATE_EXTENSIONS } from '../template-rules'; + +/** + * Each engine is tested as a pair: the syntax that skips escaping, and the + * neighbouring syntax that does not. Getting the second half wrong is how a + * template rule ends up reporting every line of every view. + */ +const ruleIds = (path: string, source: string): string[] => + scanText(path, source).map((finding) => finding.ruleId); + +describe('Handlebars and Mustache', () => { + it('flags a triple-stache', () => { + expect(ruleIds('v.hbs', '
{{{ comment.body }}}
')).toContain('tpl-handlebars-unescaped'); + }); + + it('flags the ampersand form', () => { + expect(ruleIds('v.mustache', '
{{& comment.body }}
')).toContain( + 'tpl-handlebars-unescaped', + ); + }); + + it('stays silent on an escaped interpolation', () => { + expect(ruleIds('v.hbs', '
{{ comment.body }}
')).not.toContain('tpl-handlebars-unescaped'); + }); + + it('stays silent on the layout slot', () => { + // Every project using layouts has exactly one of these, and it is not a + // variable — it is where the rendered child template is spliced in. + expect(ruleIds('layout.hbs', '{{{body}}}')).not.toContain('tpl-handlebars-unescaped'); + }); +}); + +describe('Vue', () => { + it('flags a v-html binding', () => { + expect(ruleIds('C.vue', '
')).toContain('tpl-vue-v-html'); + }); + + it('stays silent on a moustache interpolation', () => { + expect(ruleIds('C.vue', '
{{ post.title }}
')).not.toContain('tpl-vue-v-html'); + }); +}); + +describe('Pug', () => { + it('flags unescaped buffered output', () => { + expect(ruleIds('v.pug', 'p!= user.bio')).toContain('tpl-pug-unescaped'); + }); + + it('flags unescaped interpolation', () => { + expect(ruleIds('v.pug', 'p Hello !{user.name}')).toContain('tpl-pug-unescaped'); + }); + + it('stays silent on escaped output', () => { + expect(ruleIds('v.pug', 'p= user.bio')).not.toContain('tpl-pug-unescaped'); + }); + + it('stays silent on a strict-inequality comparison', () => { + // `!==` contains `!=`. Anchoring to the tag position is what keeps inline + // JavaScript in a Pug file from reading as unescaped output. + expect(ruleIds('v.pug', '- if (a !== b)')).not.toContain('tpl-pug-unescaped'); + }); +}); + +describe('EJS', () => { + it('flags the raw output tag', () => { + expect(ruleIds('v.ejs', '
<%- comment.body %>
')).toContain('tpl-ejs-raw-output'); + }); + + it('stays silent on the escaping tag', () => { + // In EJS `<%= %>` escapes — the reverse of Underscore's convention, which + // is why that engine is in KNOWN_GAPS rather than covered here. + expect(ruleIds('v.ejs', '
<%= comment.body %>
')).not.toContain('tpl-ejs-raw-output'); + }); + + it('stays silent on a partial include', () => { + expect(ruleIds('v.ejs', `<%- include('partials/header') %>`)).not.toContain('tpl-ejs-raw-output'); + }); +}); + +describe('Dust', () => { + it('flags a suppressed escape filter', () => { + expect(ruleIds('v.dust', '
{body|s}
')).toContain('tpl-dust-escape-filter-off'); + }); + + it('stays silent on a plain reference', () => { + expect(ruleIds('v.dust', '
{body}
')).not.toContain('tpl-dust-escape-filter-off'); + }); +}); + +describe('Nunjucks, Twig and Jinja', () => { + it('flags the safe filter', () => { + expect(ruleIds('v.njk', '
{{ post.body | safe }}
')).toContain('tpl-jinja-safe-filter'); + }); + + it('flags an autoescape block turned off', () => { + expect(ruleIds('v.njk', '{% autoescape false %}')).toContain('tpl-jinja-safe-filter'); + }); + + it('stays silent on a plain interpolation', () => { + expect(ruleIds('v.njk', '
{{ post.body }}
')).not.toContain('tpl-jinja-safe-filter'); + }); +}); + +describe('Haml', () => { + it('flags unescaped output from a tag', () => { + expect(ruleIds('v.haml', '%p!= user.bio')).toContain('tpl-haml-unescaped'); + }); + + it('stays silent on escaped output', () => { + expect(ruleIds('v.haml', '%p= user.bio')).not.toContain('tpl-haml-unescaped'); + }); +}); + +describe('engine wiring', () => { + it('makes every template extension scannable', () => { + // A rule whose files the walker skips is a rule that silently never fires. + for (const extension of TEMPLATE_EXTENSIONS) { + expect(SCAN_EXTENSIONS.has(extension)).toBe(true); + } + }); + + it('caps a template finding at medium', () => { + // A template cannot show that the value is attacker-controlled. Reporting + // these as high would fill a report with claims nobody can triage. + const finding = scanText('v.hbs', '
{{{ comment.body }}}
')[0]; + expect(finding?.confidence).toBe('pattern'); + expect(finding?.severity).toBe('medium'); + }); + + it('still applies the JavaScript rules to a Vue file', () => { + // `.vue` is both a template and a script. Reaching the template rules must + // not cost it the code rules. + const source = [''].join('\n'); + expect(ruleIds('C.vue', source)).toContain('js-unescaped-html-sink'); + }); + + it('honours an inline suppression on a template line', () => { + const source = ['{{!-- threatcrush-disable-next-line tpl-handlebars-unescaped --}}', '{{{ trusted.html }}}'].join('\n'); + expect(ruleIds('v.hbs', source)).not.toContain('tpl-handlebars-unescaped'); + }); +}); diff --git a/packages/scan/src/code-rules.ts b/packages/scan/src/code-rules.ts index 84104e1..6931a62 100644 --- a/packages/scan/src/code-rules.ts +++ b/packages/scan/src/code-rules.ts @@ -37,6 +37,7 @@ * rule that would flag every session read in the codebase. See KNOWN_GAPS. */ +import { NODE_RULES } from './node-rules'; import type { Confidence, ScanLanguage, Severity } from './types'; import { severityFor } from './types'; @@ -206,7 +207,20 @@ export const GENERIC_GUARD = // The identifier must END at the escaper (with at most a known output-context // suffix). An earlier, looser form also matched `describe(`, which would have // silenced findings across every test file in every repository. - /\ballow(?:ed|list|_list|ed_hosts)?\b|\bwhitelist\b|\b\w{0,6}[Ee]sc(?:ape)?(?:[Hh]tml|HTML|[Xx]ml|XML|[Ss]ql|[Aa]ttr|[Jj]s|[Uu]ri|[Uu]rl)?\s*\(|\bhtml_escape\b|\bhtmlspecialchars\s*\(|\bsanitiz\w*\b|\bencoded\b|\brealpath\b|\bcommonpath\b|\bresolve\(\)\.startsWith\b|\bprocess\.env\b|\bos\.environ\b|\bgetenv\b|\bENV\s*\[|setObjectInputFilter|ObjectInputFilter/i; + // The `Access-Control-` lookbehind is not a nicety. This regex is + // case-insensitive, and `Access-Control-Allow-Origin` contains the word + // "Allow" followed by a non-word character — so the CORS header name matched + // the allow-list heuristic. Because the guard is tested against an 8-line + // *window*, one header line silently disabled every guardable rule near it: + // in a four-line Express error handler, the header on one line suppressed + // both the `none`-algorithm JWT finding and the returned stack trace below + // it. The failure mode is the dangerous kind — not fewer findings, none, and + // indistinguishable from clean code. + // + // Scoped to the header prefix rather than to a bare `allow(?!-)`, because + // this guard also runs over `.yml` and `.conf` files, where `allow-list:` + // and `allowed-hosts:` are ordinary keys that should still guard. + /(?(); + /** Where the server is built. Anchors the findings somewhere meaningful. */ + private serverFile: string | null = null; + + observe(relativePath: string, text: string): void { + if (this.serverFile === null && SERVER_FRAMEWORK.test(text)) { + this.serverFile = relativePath; + } + for (const control of SECURITY_CONTROLS) { + if (this.seen.has(control.id)) continue; + if (control.evidence.test(text)) this.seen.add(control.id); + } + } + + /** Controls with no evidence anywhere. Empty when the tree serves no HTTP. */ + missing(): readonly SecurityControl[] { + if (this.serverFile === null) return []; + return SECURITY_CONTROLS.filter((control) => !this.seen.has(control.id)); + } + + findings(): ScanFinding[] { + const anchor = this.serverFile; + if (anchor === null) return []; + + return this.missing().map((control) => ({ + ruleId: control.id, + title: control.title, + file: anchor, + line: 1, + // `pattern` is doing real work here: it caps the severity, and it says + // in the report itself how much the scanner is claiming. An absence is + // never `evidence`. + severity: severityFor(control.severity, 'pattern'), + confidence: 'pattern' as const, + message: `${control.title} — no evidence of one anywhere in the scanned tree (${control.cwe})`, + consequence: control.consequence, + cwe: control.cwe, + excerpt: '', + category: 'code' as const, + })); + } +} diff --git a/packages/scan/src/index.ts b/packages/scan/src/index.ts index 6ef6299..be92786 100644 --- a/packages/scan/src/index.ts +++ b/packages/scan/src/index.ts @@ -14,6 +14,14 @@ export { CODE_RULES, evaluateRule, GENERIC_GUARD, proseLines, untrustedPatternFor } from './code-rules'; export type { CodeRule } from './code-rules'; +export { NODE_RULES } from './node-rules'; + +export { evaluateTemplateRules, TEMPLATE_EXTENSIONS, TEMPLATE_RULES } from './template-rules'; +export type { TemplateMatch, TemplateRule } from './template-rules'; + +export { ControlAudit, SECURITY_CONTROLS } from './controls'; +export type { SecurityControl } from './controls'; + export { scanPackageJson, scanRequirementsTxt, detectTyposquat, editDistance } from './manifest-rules'; export type { ManifestFinding, SquatVerdict } from './manifest-rules'; diff --git a/packages/scan/src/node-rules.ts b/packages/scan/src/node-rules.ts new file mode 100644 index 0000000..c891946 --- /dev/null +++ b/packages/scan/src/node-rules.ts @@ -0,0 +1,516 @@ +/** + * Node.js-specific vulnerability rules. + * + * Why this file exists + * -------------------- + * `code-rules.ts` is deliberately cross-language: SQL injection, path + * traversal and weak hashing look much the same in six languages, so one rule + * covers all of them. That breadth is also its limit. A whole set of classes + * only exist inside the Node ecosystem — `vm2` sandbox escapes, Electron's + * renderer settings, `node-serialize`'s function-executing deserialiser, JWT's + * `none` algorithm, headless-browser navigation — and each one needs to know + * which package it is looking at before it can say anything useful. + * + * Those live here, evaluated by the same engine and obeying the same + * confidence model. Splitting them out keeps the cross-language table readable + * and gives the ecosystem-specific rules one place to document their + * preconditions. + * + * Provenance and licence + * ---------------------- + * The *list of classes* covered here was drawn up by reading what njsscan + * (github.com/ajinabraham/njsscan) reports on, to close a coverage gap against + * an established Node scanner. Nothing was copied. njsscan is LGPL-3.0 and + * this package is MIT, so its rule definitions could not be reused even in + * translation; a set of vulnerability class names is a fact about the + * ecosystem, not an expression of it. Every pattern, guard, title and + * consequence below was written from scratch against this engine's semantics, + * which are not njsscan's — it delegates to semgrep for AST matching, while + * these are line-oriented regexes with guard windows and a severity cap. + * + * That difference shapes the rules. Where njsscan can write "this call, inside + * a file that imported this module, not wrapped in an escaper", the closest + * honest equivalent here is `fileRequires` for the import plus a narrow + * pattern. Classes that genuinely need AST scope — entity-expansion limits, + * taint through a router — are absent rather than approximated. See KNOWN_GAPS + * at the foot of this file. + */ + +import type { CodeRule } from './code-rules'; + +/** + * A path is being assembled with containment already checked. + * + * Zip-slip is the one class in this file where the safe and unsafe forms are + * the *same call* — `join(dest, entry.fileName)` — distinguished only by + * whether the result is verified to stay under `dest`. So the guard has to + * recognise containment checks specifically, which the generic guard does not: + * a bare `startsWith` is too weak a signal to exonerate arbitrary rules, but + * next to a path join it is exactly the check that matters. + */ +const PATH_CONTAINMENT_GUARD = + /\bstartsWith\s*\(|\brelative\s*\(|\bnormalize\s*\(|\brealpath\b|\bisInside\b|\bwithin\s*\(|\bsanitiz\w*\b/i; + +/** Packages whose archive entries carry an attacker-chosen path. */ +const ARCHIVE_LIBRARY = + /\b(?:adm-zip|unzipper|yauzl|node-stream-zip|extract-zip|decompress|tar-stream|tar-fs)\b|require\s*\(\s*['"]tar['"]|from\s+['"]tar['"]/; + +/** Packages that drive a real browser, where a URL becomes a request from the server. */ +const HEADLESS_BROWSER = + /\b(?:puppeteer|playwright|phantom|phantomjs|wkhtmltopdf|wkhtmltoimage|chrome-aws-lambda|html-pdf)\b/; + +/** + * Evidence that a reflected origin was checked before being echoed. + * + * This rule cannot use the generic guard, and the reason is worth recording. + * `Access-Control-Allow-Origin` contains the word "Allow"; the generic guard's + * allow-list heuristic is case-insensitive and accepts a following `-`, so the + * *header name itself* matched it. The rule was silently unable to ever fire — + * it did not report fewer findings, it reported none, and only a failing test + * distinguished that from "no vulnerable code here". + * + * What genuinely exonerates a reflected origin is a membership test on the + * value, which is also the shape of the one correct implementation. + */ +const ORIGIN_ALLOWLIST_GUARD = /\b(?:includes|indexOf|has|test|find|some)\s*\(/; + +/** Deserialisers that reconstruct functions, not just data. */ +const FUNCTION_DESERIALIZER = /\b(?:node-serialize|serialize-to-js|funcster|cryo)\b/; + +/** + * A string literal being concatenated onto — either quote style, separately. + * + * One character class cannot do this. The interesting strings are the ones + * containing the *other* quote: + * + * "this.name === '" + name + "'" + * "//user[@id='" + id + "']" + * + * A combined `['"][^'"]*['"]` class stops dead at that inner quote and matches + * nothing, which silently drops the most common injection shape. `code-rules.ts` + * hit this with SQL and split the class per quote character; the same split is + * needed everywhere a rule looks for assembled strings, so it lives here once. + */ +const CONCATENATED_STRING = String.raw`(?:"[^"\n]*"|'[^'\n]*')\s*\+`; + +// A template literal with at least one interpolation. Written as a quoted +// string rather than with `String.raw`, because the pattern has to contain a +// backtick and a raw template literal cannot hold one unescaped. +const INTERPOLATED_TEMPLATE = '`[^`\\n]*\\$\\{'; + +export const NODE_RULES: readonly CodeRule[] = [ + // ── Code execution ─────────────────────────────────────────────────────── + { + id: 'js-vm-untrusted-execution', + title: 'script compiled and run by the `vm` module', + consequence: + 'Node’s `vm` is not a security boundary — it isolates globals, not the process. Code reaching it can walk back out through any object it is handed and runs with the server’s full privileges.', + cwe: 'CWE-94', + severity: 'critical', + languages: ['javascript', 'typescript'], + pattern: + /\bvm\s*\.\s*(?:runInNewContext|runInThisContext|runInContext|compileFunction)\s*\(|\bnew\s+vm\s*\.\s*Script\s*\(/, + // A `vm` call whose source is a build-time constant is a plugin loader, not + // a vulnerability. The class only becomes real once the script text can be + // influenced, so require that evidence before reporting at full severity. + needsContext: true, + }, + { + id: 'js-vm2-sandbox', + title: '`vm2` used as a sandbox', + consequence: + 'vm2 was discontinued after a series of escapes that its maintainer judged unfixable by design. Any code it runs should be assumed to run on the host.', + cwe: 'CWE-1104', + severity: 'high', + languages: ['javascript', 'typescript'], + pattern: /\bnew\s+(?:NodeVM|VMScript)\s*\(|\bfrom\s+['"]vm2['"]|require\s*\(\s*['"]vm2['"]\s*\)/, + // Nothing on the surrounding lines changes the answer: the package itself + // is the finding, the same way a broken cipher is. + inherent: true, + }, + { + id: 'js-function-deserialization', + title: 'deserialiser that reconstructs functions', + consequence: + 'These formats encode functions alongside data and invoke them on load, so parsing an attacker’s payload is executing it. No amount of validation after the parse call helps — the code has already run.', + cwe: 'CWE-502', + severity: 'critical', + languages: ['javascript', 'typescript'], + pattern: /\b(?:unserialize|deepDeserialize|deserialize)\s*\(/, + // Without this the rule fires on every project that happens to own a + // function called `deserialize`, which is most of them. The import is what + // makes the call the dangerous one. + fileRequires: FUNCTION_DESERIALIZER, + // The parse *is* the execution, so nearby input cannot make it worse and + // its absence cannot make it safe. Nobody round-trips a constant through + // these libraries. + inherent: true, + }, + { + id: 'js-template-injection', + title: 'template compiled from a non-constant source', + consequence: + 'Template languages are programming languages. A user-supplied template body is remote code execution, not cross-site scripting — the expression runs on the server before any output is escaped.', + cwe: 'CWE-1336', + severity: 'critical', + languages: ['javascript', 'typescript'], + pattern: + /\b(?:handlebars|Handlebars|hbs|ejs|pug|jade|nunjucks|eta|twig|dot|doT|liquid|mustache)\s*\.\s*(?:compile|compileFile|render|renderString)\s*\(\s*(?:`[^`\n]*\$\{|[a-zA-Z_$][\w$.]*\s*[,)]|[a-zA-Z_$][\w$.]*\s*\+)/, + // Rendering a template held in a variable is the normal case — it was read + // from a file at boot. Only the version where request data reaches the + // template *body* is this class. + needsContext: true, + }, + { + id: 'js-shelljs-command-execution', + title: 'shelljs command assembled from a string', + consequence: + '`shell.exec` runs its argument through a shell, so a `;` or backtick in an interpolated value runs as the server user.', + cwe: 'CWE-78', + severity: 'critical', + languages: ['javascript', 'typescript'], + pattern: new RegExp( + `\\b(?:shell|shelljs|sh)\\s*\\.\\s*exec\\s*\\(\\s*(?:${INTERPOLATED_TEMPLATE}|${CONCATENATED_STRING}|[a-zA-Z_$][\\w$]*\\s*\\+)`, + ), + fileRequires: /\bshelljs\b/, + }, + + // ── Injection ──────────────────────────────────────────────────────────── + { + id: 'js-nosql-where-expression', + title: 'MongoDB `$where` built by string assembly', + consequence: + '`$where` is evaluated as JavaScript by the database server, once per document. An interpolated value can rewrite the predicate to `true` or run a denial-of-service loop inside the database.', + cwe: 'CWE-943', + severity: 'critical', + languages: ['javascript', 'typescript'], + pattern: new RegExp( + `\\$where\\s*['"]?\\s*:\\s*(?:${INTERPOLATED_TEMPLATE}|${CONCATENATED_STRING})` + + `|\\.\\s*\\$where\\s*=\\s*(?:${INTERPOLATED_TEMPLATE}|${CONCATENATED_STRING})`, + ), + }, + { + id: 'js-xpath-injection', + title: 'XPath expression built by concatenation', + consequence: + 'A quote in the interpolated value closes the predicate early, so the query selects nodes the caller was never meant to read — the XML equivalent of `OR 1=1`.', + cwe: 'CWE-643', + severity: 'high', + languages: ['javascript', 'typescript'], + // An XPath expression is recognisable by its own syntax — a descendant + // axis or an attribute predicate. Matching on the *call* name alone would + // flag every `select(` in the ecosystem. The quoted forms are split per + // quote character for the reason given at CONCATENATED_STRING: the + // predicate that makes it XPath usually contains the other quote. + pattern: new RegExp( + '\\b(?:xpath|xpathSelect|select|selectNodes|selectSingleNode|evaluate|find)\\s*\\(\\s*(?:' + + '`[^`\\n]*(?:\\/\\/|\\[@)[^`\\n]*\\$\\{' + + '|"[^"\\n]*(?:\\/\\/|\\[@)[^"\\n]*"\\s*\\+' + + "|'[^'\\n]*(?:\\/\\/|\\[@)[^'\\n]*'\\s*\\+" + + ')', + ), + fileRequires: /\bxpath\b|\bxmldom\b|\blibxmljs\b|\bxpath\.js\b/, + }, + { + id: 'js-regex-from-input', + title: 'regular expression compiled from a variable', + consequence: + 'A caller who controls the pattern controls the matcher: they can supply catastrophic backtracking to hang the event loop, or a permissive pattern that defeats whatever the regex was validating.', + cwe: 'CWE-1333', + severity: 'medium', + languages: ['javascript', 'typescript'], + pattern: new RegExp( + `\\bnew\\s+RegExp\\s*\\(\\s*(?:${INTERPOLATED_TEMPLATE}|${CONCATENATED_STRING}|(?!['"\`/])[a-zA-Z_$][\\w$.]*\\s*[,)])`, + ), + needsContext: true, + }, + + // ── XML ────────────────────────────────────────────────────────────────── + { + id: 'js-xml-external-entities', + title: 'XML parser configured to resolve entities', + consequence: + 'An entity declaration in the document body makes the parser fetch a local file or an internal URL and paste the result into the parsed output — file disclosure and server-side request forgery from a document upload.', + cwe: 'CWE-611', + severity: 'high', + languages: ['javascript', 'typescript'], + // The option name is the finding. Every parser in the ecosystem defaults + // these off, so an explicit `true` is a deliberate re-enable. + pattern: /\b(?:noent|resolveEntities|expandEntities|externalEntities|resolveExternals)\s*:\s*(?:true|1)\b/, + inherent: true, + }, + + // ── Authentication and tokens ──────────────────────────────────────────── + { + id: 'js-jwt-none-algorithm', + title: 'JWT algorithm set to `none`', + consequence: + 'The `none` algorithm means the signature is not checked. Anyone can mint a token with any claims — including another user’s id or an admin role — by base64-encoding a header and a body.', + cwe: 'CWE-347', + severity: 'critical', + languages: ['javascript', 'typescript'], + pattern: /\balgorithms?\s*:\s*\[?\s*['"]none['"]/i, + inherent: true, + }, + + // ── Cryptography ───────────────────────────────────────────────────────── + { + id: 'js-broken-cipher-algorithm', + title: 'broken cipher selected', + consequence: + 'DES, 3DES, RC2, RC4, Blowfish and IDEA are all breakable with commodity hardware or have practical plaintext-recovery attacks. Data encrypted with them should be treated as encoded, not encrypted.', + cwe: 'CWE-327', + severity: 'high', + languages: ['javascript', 'typescript'], + pattern: + /\bcreate(?:Cipher|Decipher)(?:iv)?\s*\(\s*['"](?:des|des3|des-ede\w*|3des|rc2|rc4|bf|blowfish|cast5?|idea|seed)\b/i, + inherent: true, + }, + { + id: 'js-ecb-mode-cipher', + title: 'block cipher in ECB mode', + consequence: + 'ECB encrypts every block independently, so identical plaintext blocks produce identical ciphertext. Structure in the data survives encryption and blocks can be reordered or replayed without detection.', + cwe: 'CWE-327', + severity: 'high', + languages: ['javascript', 'typescript'], + pattern: /\bcreate(?:Cipher|Decipher)(?:iv)?\s*\(\s*['"][^'"\n]*-ecb\b/i, + inherent: true, + }, + { + id: 'js-legacy-cipher-api', + title: 'deprecated `createCipher` used', + consequence: + '`createCipher` derives the key from a passphrase with a single unsalted MD5 pass and uses a fixed all-zero IV, so the same passphrase always produces the same keystream. It was removed in Node 22.', + cwe: 'CWE-327', + severity: 'high', + languages: ['javascript', 'typescript'], + // `createCipheriv` is the correct API and shares the prefix, so the + // negative look-ahead is what separates the finding from the fix. + pattern: /\bcrypto\s*\.\s*create(?:Cipher|Decipher)\s*\(/, + lineGuard: /create(?:Cipher|Decipher)iv\s*\(/, + inherent: true, + }, + + // ── Cross-site scripting ───────────────────────────────────────────────── + { + id: 'js-template-autoescape-disabled', + title: 'template auto-escaping turned off', + consequence: + 'Auto-escaping is the control that makes a template engine safe by default. Disabling it globally means every interpolation in every template becomes an injection point, including ones written later by someone who assumed the default.', + cwe: 'CWE-79', + severity: 'high', + languages: ['javascript', 'typescript'], + pattern: /\bautoescape\s*:\s*false\b|\bescape\s*:\s*false\b|\bnoEscape\s*:\s*true\b/, + inherent: true, + }, + { + id: 'js-serialize-javascript-unsafe', + title: '`serialize-javascript` in unsafe mode', + consequence: + 'The `unsafe` flag turns off escaping of HTML-significant characters in the output. Embedding the result in a `