diff --git a/src/scan-session.mjs b/src/scan-session.mjs index ae97c1b..de8b2bb 100644 --- a/src/scan-session.mjs +++ b/src/scan-session.mjs @@ -112,7 +112,8 @@ export function scanEntries(repo, engine, entries, options = {}) { // Binary detection still reads the blob. Count every examined byte so // later commits cannot reset the total budget merely by using NUL data. stats.bytes_scanned += blob.length; - if (isBinary(blob)) { + const utf16 = decodeUtf16(blob); + if (utf16 === null && isBinary(blob)) { // Content rules match text lines, so a binary blob carries nothing // they could fire on. Skipping it loses no coverage: the skip is // recorded as 'binary', which does not mark the scan incomplete. @@ -122,7 +123,7 @@ export function scanEntries(repo, engine, entries, options = {}) { } stats.files_scanned += 1; const ranges = lineRanges.get(entry.path); - const matched = engine.checkContent(entry.path, blob.toString('utf8'), + const matched = engine.checkContent(entry.path, utf16 ?? blob.toString('utf8'), ranges && ranges.length ? { lineRanges: ranges } : {}); stats.findings_total += matched.length; for (const finding of matched) { @@ -240,6 +241,24 @@ function isBinary(buffer) { return false; } +// A byte-order mark declares the bytes are UTF-16 text, so their NUL padding is +// not evidence of a binary file. Returns the decoded text, or null to leave the +// blob to isBinary. Only the mark is trusted: guessing at unmarked NUL data +// would decode images into garbage and hand that to the content rules. +function decodeUtf16(buffer) { + if (buffer.length < 4 || buffer.length % 2 !== 0) return null; + const [first, second] = buffer; + // FF FE 00 00 opens UTF-32LE, which shares the UTF-16LE mark's first two bytes. + if (first === 0xff && second === 0xfe && !(buffer[2] === 0 && buffer[3] === 0)) { + return buffer.subarray(2).toString('utf16le'); + } + // swap16 mutates in place, so byte-swap a copy and leave the read batch alone. + if (first === 0xfe && second === 0xff) { + return Buffer.from(buffer.subarray(2)).swap16().toString('utf16le'); + } + return null; +} + // collectLineRanges builds a Map for all entries whose content // scan can be narrowed to changed hunks (W4, bug 12d-F1). It batches the work // into ONE `git diff --unified=0` per (commit, parent) pair rather than one diff @@ -288,17 +307,41 @@ function collectLineRanges(repo, entries) { // comes from git diff --raw -z, also unquoted). Paths containing newlines // are not handled, but they fall back to a whole-blob scan (safe). let currentPath = null; + // Hunk bodies must never be read as headers. An added line is rendered as + // `+` plus its content, so source starting `++ ` arrives as `+++ ...` and + // otherwise passes for a new-file header, hiding every later hunk of that + // file behind a path nothing scans. Each `@@` header declares how many + // body lines follow on both sides; consume exactly that many first. + let pending = 0; for (const line of output.split('\n')) { + if (pending > 0) { + if (line.startsWith('\\')) continue; // "\ No newline at end of file" + if (line.startsWith('-') || line.startsWith('+')) { + pending -= 1; + continue; + } + pending = 0; // body ended sooner than declared: resume header parsing + } + // Content cannot forge this prefix, so it is the one reliable resync point. + if (line.startsWith('diff --git ')) { + currentPath = null; + continue; + } // With --no-prefix the +++ line is `+++ path` (no b/ prefix). const plusMatch = /^\+\+\+ (.+)$/.exec(line); if (plusMatch) { currentPath = plusMatch[1] === '/dev/null' ? null : plusMatch[1]; continue; } - const match = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/.exec(line); - if (!match || !currentPath) continue; - const start = Number(match[1]); - const count = match[2] === undefined ? 1 : Number(match[2]); + const match = /^@@ -\d+(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(line); + if (!match) continue; + const removed = match[1] === undefined ? 1 : Number(match[1]); + const count = match[3] === undefined ? 1 : Number(match[3]); + // Set the budget before any bail below, or a skipped hunk re-exposes + // its body to the header branches above. + pending = removed + count; + if (!currentPath) continue; + const start = Number(match[2]); if (start === 0 || count === 0) continue; // pure deletion, no new content const range = { start, end: start + count - 1 }; const existing = byPath.get(currentPath); diff --git a/tests/scan-session.test.mjs b/tests/scan-session.test.mjs index 198989a..81c3018 100644 --- a/tests/scan-session.test.mjs +++ b/tests/scan-session.test.mjs @@ -90,10 +90,10 @@ test('binary blobs skip whole: no content rule reads them and the scan stays com execFileSync('git', ['init', '-q'], { cwd: dir }); execFileSync('git', ['config', 'user.name', 'Test'], { cwd: dir }); execFileSync('git', ['config', 'user.email', 'test@example.com'], { cwd: dir }); - // PowerShell 5.1 still writes UTF-16LE from `>`, `Out-File` and - // `Tee-Object`. One NUL per character classifies the blob as binary, - // and no content rule reads binary bytes: the marker text below would - // block as plain UTF-8, and here it produces no finding. + // NUL-bearing bytes with no byte-order mark stay binary. Nothing here + // says whether they are text, and guessing costs more than it buys: a + // misread image decoded into garbage would hand the content rules + // nonsense, and a block finding on clean unstages the user's asset. writeFileSync(join(dir, 'deploy-notes.txt'), Buffer.from('// generated by AI\n', 'utf16le')); execFileSync('git', ['add', '.'], { cwd: dir }); @@ -108,6 +108,65 @@ test('binary blobs skip whole: no content rule reads them and the scan stays com } }); +// A byte-order mark says the bytes are UTF-16 text, so the NUL padding is not +// evidence of a binary file. PowerShell 5.1 writes exactly this from `>`, +// `Out-File` and `Tee-Object`, which is how a Windows developer ends up with an +// AI-marker file that no content rule ever reads. +function repositoryWithBlob(name, bytes) { + const dir = mkdtempSync(join(tmpdir(), 'aim-session-bom-')); + execFileSync('git', ['init', '-q'], { cwd: dir }); + execFileSync('git', ['config', 'user.name', 'Test'], { cwd: dir }); + execFileSync('git', ['config', 'user.email', 'test@example.com'], { cwd: dir }); + writeFileSync(join(dir, name), bytes); + execFileSync('git', ['add', '.'], { cwd: dir }); + return dir; +} + +test('UTF-16 text with a byte-order mark is read by the content rules', () => { + const body = Buffer.from('// generated by AI\n', 'utf16le'); + const dir = repositoryWithBlob('deploy-notes.txt', Buffer.concat([Buffer.from([0xff, 0xfe]), body])); + try { + const repo = openRepo(dir); + const result = scanEntries(repo, newEngine('strict'), trackedEntries(repo)); + assert.equal(result.stats.files_scanned, 1, 'a BOM-marked UTF-16 blob must be scanned, not skipped'); + assert.ok( + result.findings.some((f) => f.matchedRuleIds.includes('marker.ai-authored')), + 'the marker in UTF-16LE text must produce the same finding it does in UTF-8', + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('big-endian UTF-16 text with a byte-order mark is read by the content rules', () => { + const body = Buffer.from('// generated by AI\n', 'utf16le').swap16(); + const dir = repositoryWithBlob('deploy-notes.txt', Buffer.concat([Buffer.from([0xfe, 0xff]), body])); + try { + const repo = openRepo(dir); + const result = scanEntries(repo, newEngine('strict'), trackedEntries(repo)); + assert.ok( + result.findings.some((f) => f.matchedRuleIds.includes('marker.ai-authored')), + 'the marker in UTF-16BE text must produce the same finding it does in UTF-8', + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('an odd-length blob claiming a byte-order mark is left to the binary skip', () => { + // swap16() throws on odd lengths; the pre-commit hook must not crash on a + // truncated file, so an impossible UTF-16 length falls through to binary. + const dir = repositoryWithBlob('truncated.bin', Buffer.from([0xfe, 0xff, 0x00, 0x2f, 0x00])); + try { + const repo = openRepo(dir); + const result = scanEntries(repo, newEngine('strict'), trackedEntries(repo)); + assert.equal(result.stats.skipped.binary, 1); + assert.equal(result.complete, true); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + test('git stderr from a passing blob read is captured, not printed', () => { const dir = mkdtempSync(join(tmpdir(), 'aim-session-stderr-')); try { @@ -269,3 +328,67 @@ test('hunk-narrowing: batched multi-file diff attributes each file its own hunks rmSync(dir, { recursive: true, force: true }); } }); + +// A unified diff renders an added line as `+` followed by its content, so a +// source line starting `++ ` arrives as `+++ ...` — the same shape as a new-file +// header. Hunk bodies must be consumed by the line budget the `@@` header +// declares, never re-read as headers, or everything after the forged line in +// that file is attributed to a path nobody scans. +const FORGERY_BASE = [ + '// top of file', + 'int i = 0;', + '', + 'func Fixture() {', + ' step()', + ' step()', + ' step()', + ' // benign line', + ' step()', + '}', +].join('\n') + '\n'; + +function commitForgedHeader(dir, secondLine) { + const lines = FORGERY_BASE.split('\n'); + lines[1] = secondLine; + lines[7] = '// generated by AI'; + writeFileSync(join(dir, 'main.go'), lines.join('\n')); + execFileSync('git', ['add', 'main.go'], { cwd: dir }); + execFileSync('git', ['commit', '-q', '-m', 'edit'], { cwd: dir }); +} + +test('a hunk body that reads like a file header does not hide the rest of the file', () => { + // Line 2 becomes `++ i;`, which git renders as `+++ i;`. Line 8 becomes a + // marker in a later hunk. The marker must still be scanned. + const dir = repositoryWithBase('main.go', FORGERY_BASE); + try { + commitForgedHeader(dir, '++ i;'); + + const repo = openRepo(dir); + const { head, parent } = headAndParent(dir); + const changes = commitChanges(repo, head, head, [parent]); + const result = scanEntries(repo, newEngine('strict'), changes.entries); + const markers = result.findings.filter((f) => f.matchedRuleIds.includes('marker.ai-authored')); + assert.ok(markers.length >= 1, 'a line rendering as `+++ path` must not blind the hunks after it'); + assert.deepEqual([...new Set(markers.map((f) => f.path))], ['main.go']); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a hunk body reading like a deleted-file header does not hide the rest of the file', () => { + // `++ /dev/null` renders as `+++ /dev/null`, which the header branch treats + // as a deletion and would drop every remaining hunk in the file. + const dir = repositoryWithBase('main.go', FORGERY_BASE); + try { + commitForgedHeader(dir, '++ /dev/null'); + + const repo = openRepo(dir); + const { head, parent } = headAndParent(dir); + const changes = commitChanges(repo, head, head, [parent]); + const result = scanEntries(repo, newEngine('strict'), changes.entries); + const markers = result.findings.filter((f) => f.matchedRuleIds.includes('marker.ai-authored')); + assert.ok(markers.length >= 1, 'a line rendering as `+++ /dev/null` must not blind the hunks after it'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +});