mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-14 15:16:35 +03:00
fix: skip design-hook scans for files outside the resolved project root
The per-edit and Stop deep passes gated on sensitive paths, generated paths, extension, config ignores, and size, but never on containment. Any file the session touched outside the project (harness scratchpad dirs under the system temp root, sibling checkouts) was scanned and judged against THIS project's config and DESIGN.md palette, producing design-system findings that are wrong by construction. Both loops now check isScanTargetInsideProject() (audit reason: outside-project), matching the gate hook-before-edit.mjs already had. Paths are canonicalized so a symlinked root doesn't split the comparison. The Stop pass re-checks containment itself because caches written by older hook versions can still list out-of-project paths. Umbrella-dir launches (issue #305) are unaffected: their projectCwd resolves to the edited file's own project root, so containment holds. Written with AI assistance (Claude Code). Co-Authored-By: Claude Code <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Code
parent
6c1aff7d1f
commit
ae03e9e09c
@@ -1332,6 +1332,24 @@ function isInsideProject(filePath, projectCwd) {
|
||||
}
|
||||
}
|
||||
|
||||
function canonicalPath(p) {
|
||||
try { return fs.realpathSync(p); } catch { return path.resolve(p); }
|
||||
}
|
||||
|
||||
// Containment gate for both scan passes. A session routinely touches files
|
||||
// that belong to no project or to a different one — harness scratchpad dirs
|
||||
// under the system temp root, sibling checkouts, one-off throwaway HTML — and
|
||||
// findings against those are judged with THIS project's config and DESIGN.md
|
||||
// palette, which is never right. Skip them (audit reason: outside-project).
|
||||
// Paths are canonicalized first so a symlinked root (macOS /tmp ->
|
||||
// /private/tmp) doesn't split the comparison; the realpath fallback for
|
||||
// missing paths is only correct because both scan loops check existence
|
||||
// before calling this.
|
||||
export function isScanTargetInsideProject(filePath, projectCwd) {
|
||||
if (!filePath || !projectCwd) return false;
|
||||
return isInsideProject(canonicalPath(filePath), canonicalPath(projectCwd));
|
||||
}
|
||||
|
||||
export function parseStaticStyleImports(content, fromFile, projectCwd) {
|
||||
if (!content || typeof content !== 'string') return [];
|
||||
const dir = path.dirname(fromFile);
|
||||
@@ -1690,6 +1708,10 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now =
|
||||
lastSkip = 'file-missing';
|
||||
continue;
|
||||
}
|
||||
if (!isScanTargetInsideProject(filePath, projectCwd)) {
|
||||
lastSkip = 'outside-project';
|
||||
continue;
|
||||
}
|
||||
|
||||
const maxFileBytes = config.limits?.maxFileBytes ?? DEFAULT_CONFIG.limits.maxFileBytes;
|
||||
if (maxFileBytes > 0) {
|
||||
@@ -2020,6 +2042,10 @@ export async function runStopHook({ stdinJson, env = {}, cwd = process.cwd(), no
|
||||
const relForMatch = relativize(filePath, projectCwd);
|
||||
if (matchesAnyGlob(relForMatch, config.ignoreFiles) || matchesAnyGlob(filePath, config.ignoreFiles)) continue;
|
||||
if (!fs.existsSync(filePath)) continue;
|
||||
// Caches written before this gate existed can still hold out-of-project
|
||||
// paths, so the Stop pass re-checks containment rather than trusting
|
||||
// the per-edit pass to have filtered them.
|
||||
if (!isScanTargetInsideProject(filePath, projectCwd)) continue;
|
||||
|
||||
scanned += 1;
|
||||
let content = '';
|
||||
|
||||
@@ -60,6 +60,7 @@ import {
|
||||
resolveProjectPlatform,
|
||||
isNativePlatform,
|
||||
normalizeIgnoreValueEntries,
|
||||
isScanTargetInsideProject,
|
||||
} from '../skill/scripts/hook-lib.mjs';
|
||||
import { normalizeIgnoreValueEntries as normalizeIgnoreValueEntriesCli } from '../cli/lib/impeccable-config.mjs';
|
||||
import { detectHtml, detectText } from '../cli/engine/detect-antipatterns.mjs';
|
||||
@@ -158,6 +159,44 @@ describe('SENSITIVE_PATH / GENERATED_PATH', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('isScanTargetInsideProject()', () => {
|
||||
let root;
|
||||
beforeEach(() => { root = mkTmp(); });
|
||||
afterEach(() => fs.rmSync(root, { recursive: true, force: true }));
|
||||
|
||||
it('accepts files under the project root and the root itself', () => {
|
||||
const file = path.join(root, 'src', 'Card.tsx');
|
||||
fs.mkdirSync(path.dirname(file), { recursive: true });
|
||||
fs.writeFileSync(file, 'noop');
|
||||
assert.equal(isScanTargetInsideProject(file, root), true);
|
||||
assert.equal(isScanTargetInsideProject(root, root), true);
|
||||
});
|
||||
|
||||
it('rejects siblings, temp scratchpads, and empty inputs', () => {
|
||||
const scratch = mkTmp();
|
||||
try {
|
||||
const outside = path.join(scratch, 'landing.html');
|
||||
fs.writeFileSync(outside, '<h1>x</h1>');
|
||||
assert.equal(isScanTargetInsideProject(outside, root), false);
|
||||
assert.equal(isScanTargetInsideProject('', root), false);
|
||||
assert.equal(isScanTargetInsideProject(outside, ''), false);
|
||||
} finally {
|
||||
fs.rmSync(scratch, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('treats symlinked and canonical forms of the same tree as one project', () => {
|
||||
const real = path.join(root, 'real');
|
||||
const link = path.join(root, 'link');
|
||||
fs.mkdirSync(path.join(real, 'src'), { recursive: true });
|
||||
fs.symlinkSync(real, link);
|
||||
const file = path.join(real, 'src', 'Card.tsx');
|
||||
fs.writeFileSync(file, 'noop');
|
||||
assert.equal(isScanTargetInsideProject(file, link), true);
|
||||
assert.equal(isScanTargetInsideProject(path.join(link, 'src', 'Card.tsx'), real), true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('readConfig()', () => {
|
||||
let cwd;
|
||||
beforeEach(() => { cwd = mkTmp(); });
|
||||
@@ -1532,6 +1571,49 @@ rounded:
|
||||
assert.equal(r.audit.skipped, 'sensitive');
|
||||
});
|
||||
|
||||
it('rejects files outside the project, like harness scratchpads', async () => {
|
||||
// Session cwd is a real project; the touched file is a throwaway HTML in
|
||||
// a temp dir elsewhere. Findings against it would be judged with this
|
||||
// project's config and DESIGN.md, so the scan must skip it entirely.
|
||||
fs.writeFileSync(path.join(cwd, 'package.json'), '{"name":"proj"}');
|
||||
const scratch = mkTmp();
|
||||
try {
|
||||
const file = path.join(scratch, 'id-test', 'landing.html');
|
||||
fs.mkdirSync(path.dirname(file), { recursive: true });
|
||||
fs.writeFileSync(file, '<h1>throwaway</h1>');
|
||||
const det = fakeDetector([finding('side-tab', 1)]);
|
||||
const r = await runHook({ stdinJson: JSON.stringify(eventFor(file)), env: {}, cwd, detector: det });
|
||||
assert.equal(r.stdout, '');
|
||||
assert.equal(r.audit.skipped, 'outside-project');
|
||||
assert.ok(!fs.existsSync(path.join(cwd, '.impeccable')), 'out-of-project edit must not dirty the cache');
|
||||
} finally {
|
||||
fs.rmSync(scratch, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('still scans a file whose project root is reached through a symlinked cwd', async () => {
|
||||
// macOS /tmp -> /private/tmp style: the session cwd is a symlink to the
|
||||
// project while the tool reports the canonical file path. Containment
|
||||
// compares canonical paths, so this is inside, not outside.
|
||||
const real = path.join(cwd, 'realproj');
|
||||
const link = path.join(cwd, 'proj-link');
|
||||
fs.mkdirSync(path.join(real, 'src'), { recursive: true });
|
||||
fs.writeFileSync(path.join(real, 'package.json'), '{"name":"proj"}');
|
||||
fs.symlinkSync(real, link);
|
||||
const file = path.join(real, 'src', 'Card.tsx');
|
||||
fs.writeFileSync(file, 'noop');
|
||||
const event = {
|
||||
session_id: 'sym-sid',
|
||||
cwd: link,
|
||||
hook_event_name: 'PostToolUse',
|
||||
tool_name: 'Edit',
|
||||
tool_input: { file_path: file },
|
||||
};
|
||||
const r = await runHook({ stdinJson: JSON.stringify(event), env: {}, cwd: link, detector: fakeDetector([]) });
|
||||
assert.notEqual(r.audit.skipped, 'outside-project');
|
||||
assert.match(r.stdout, /No deterministic design-quality issues found/);
|
||||
});
|
||||
|
||||
it('rejects extensions outside the allowlist', async () => {
|
||||
const file = writeFixture('docs/README.md', 'noop');
|
||||
const r = await runHook({ stdinJson: JSON.stringify(eventFor(file)), env: {}, cwd });
|
||||
@@ -3342,6 +3424,29 @@ describe('runStopHook()', () => {
|
||||
assert.equal(r.audit.skipped, 'no-touched-files');
|
||||
});
|
||||
|
||||
it('skips out-of-project files even when an older cache still lists them', async () => {
|
||||
// Caches written before the containment gate can hold scratchpad paths.
|
||||
// The deep pass re-checks containment instead of trusting the per-edit
|
||||
// pass to have filtered them.
|
||||
const sid = 'stop-outside';
|
||||
const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-scratch-'));
|
||||
try {
|
||||
const outside = path.join(scratch, 'landing.html');
|
||||
fs.writeFileSync(outside, '<h1>throwaway</h1>');
|
||||
persistCache(cwd, {
|
||||
version: 1,
|
||||
sessions: { [sid]: { updatedAt: Date.now(), files: { [outside]: { editCount: 1, findings: [] } } } },
|
||||
});
|
||||
const det = fakeDetector([finding('side-tab', 7)]);
|
||||
const stop = await runStopHook({ stdinJson: JSON.stringify(stopEvent(sid)), env: {}, cwd, detector: det });
|
||||
assert.equal(stop.stdout, '');
|
||||
assert.equal(stop.audit.skipped, 'stop-clean');
|
||||
assert.equal(stop.audit.scannedFiles, 0);
|
||||
} finally {
|
||||
fs.rmSync(scratch, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('a second Stop fire is silent: deep-pass findings are remembered', async () => {
|
||||
const sid = 'stop-twice';
|
||||
const file = write('src/Card.tsx', 'noop');
|
||||
|
||||
Reference in New Issue
Block a user