Skip to content

fix(ignore-filter): tolerate ./-prefixed and empty relative paths in isIgnored#453

Open
tirth8205 wants to merge 2 commits into
Egonex-AI:mainfrom
tirth8205:fix/ignore-filter-relative-path-guard
Open

fix(ignore-filter): tolerate ./-prefixed and empty relative paths in isIgnored#453
tirth8205 wants to merge 2 commits into
Egonex-AI:mainfrom
tirth8205:fix/ignore-filter-relative-path-guard

Conversation

@tirth8205

Copy link
Copy Markdown
Contributor

Problem

  • isIgnored is typed as (relativePath: string): boolean — a total function that always returns a boolean. But it delegates straight to ignore@7's ig.ignores() without any normalization, and ig.ignores() THROWS on two classes of perfectly ordinary relative-path strings: (1) an empty string, and (2) a path with a leading './'. These are not exotic inputs: path.relative(projectRoot, projectRoot) returns "" (the root itself), and file-walkers / glob results very commonly emit './src/index.ts'-style relative paths. In both cases the caller does not get false (or true) back — the whole scan crashes with an uncaught exception. Verified at runtime against the package's pinned ignore@7.0.5: isIgnored("") throws path must not be empty; isIgnored("./src/a.ts") throws path should be a path.relative()'d string, but got "./src/a.ts". The documented contract (boolean return) is silently violated for…

Fix

  • Normalize before delegating: treat falsy/empty as not-ignored and strip a leading './' so ignore@7 accepts it. e.g. isIgnored(relativePath: string): boolean { if (!relativePath) return false; const normalized = relativePath.replace(/^.//, ""); if (!normalized) return false; return ig.ignores(normalized); }, Verified this returns: isIgnored("") -> false, isIgnored("./dist/a.js") -> true, isIgnored("dist/a.js") -> true, isIgnored("src/a.ts") -> false. <30 LOC.

Testing

Adds unit test(s) that fail before the change and pass after. The full core test suite, eslint, and tsc --noEmit all pass locally on this branch.

Found via a static correctness audit of the ignore filter.

🤖 Generated with Claude Code

…isIgnored

IgnoreFilter.isIgnored delegated straight to ignore@7's ig.ignores(),
which throws on an empty string ("path must not be empty") and on a
leading './' ("path should be a path.relative()'d string"). Both arise
from ordinary inputs (path.relative(root, root) === "" and ./-prefixed
glob results), crashing the scan instead of returning a boolean.

Fix: normalize first — treat falsy/empty as not-ignored and strip a
leading './' before delegating to ig.ignores().

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@thejesh23 thejesh23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few concerns before this lands.

1. The normalization doesn't cover the other shapes path.relative actually produces. ig.ignores() from ignore@7 also throws on "../foo" (path escapes root) and on Windows backslash paths like ".\\src\\a.ts" / "src\\a.ts"path.relative(projectRoot, target) produces both. The PR fixes one stripping rule but the body claims the contract violation is fixed; on Windows or with out-of-root callers it still throws. Either reject these explicitly with false, convert separators / leading ../, or document the input constraint.

2. The regex is one-shot and order-sensitive. "./".replace(/^\.\//, "") yields "" (correctly caught by the second guard), but "././foo" becomes "./foo" — still in the ignore@7 reject set — and ".//foo" becomes "/foo", also rejected. Use a loop / a stronger normalization (path.posix.normalize after slash-conversion) so the function is closed over its own output.

3. Test gap on the negation. The new tests confirm ./dist/index.js is ignored and ./src/index.ts is not, but there's no test that the previously-throwing inputs from the PR body actually return rather than throw (expect(() => ...).not.toThrow()), and no coverage for "./" alone (the trim-to-empty path), which is the one branch a future refactor is most likely to drop.

Nit: the JSDoc on IgnoreFilter.isIgnored still says "given relative path" with no mention of the now-tolerated ./ prefix or empty-as-root semantics — worth one line so callers know what's contractually accepted.

…put shapes

Normalize backslash separators, repeated leading ./, and duplicate slash
runs before delegating to ignore@7, and treat out-of-root ../ paths as
not-ignored instead of letting ig.ignores() throw. Documents the accepted
input contract on the interface JSDoc and locks the previously-throwing
inputs with not.toThrow assertions plus a bare './' (trim-to-root) case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tirth8205

Copy link
Copy Markdown
Contributor Author

Addressed all four points; the fix now normalizes inside isIgnored rather than relying on a single strip.

1 — Other path shapes. Confirmed ../foo throws under ignore@7.0.5, so the function now returns false for out-of-root escapes (.., ../..., and ..\... after separator conversion) instead of throwing. One note for accuracy: in this pinned version backslash inputs do not throw — ig.ignores("dist\\a.log") and ig.ignores(".\\dist\\a.log") both return true — but I convert \\/ anyway so the behavior is explicit and OS-independent. Production callers only ever pass toPosix(relative(root, target)), so none of these shapes are reachable today; this is defensive but verified.

2 — One-shot, order-sensitive regex. Replaced the single replace(/^\.\//,"") with a normalization that is closed over its own output: collapse one-or-more leading ./ (/^(\.\/)+/), collapse duplicate slash runs (/\/{2,}/), and drop a leading root slash. ././foo and .//foo now normalize to foo and match correctly rather than re-entering the reject set. I deliberately did not adopt path.posix.normalize since it would also collapse .. segments and change matching semantics; the targeted collapses keep intent intact while staying total.

3 — Test gap. Added a not.toThrow block covering every previously-throwing input ("", "./", "./dist/index.js", "././foo.log", ".//foo.log", "../escapes/root.ts", and the backslash variants), an explicit isIgnored("./") === false case for the trim-to-root branch, the multi-.//double-slash collapsing cases, and backslash normalization.

Nit — JSDoc. Expanded the isIgnored doc comment to state the accepted contract: root-relative input, backslash + leading-./ + duplicate-slash normalization, empty path == project root → false, and ../ escape → false.

Full core suite green (699 passed); tsc --noEmit clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants