Skip to content

path: fix inconsistent basename suffix stripping with trailing slashes #62378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
murataslan1 wants to merge 2 commits into from
Closed

path: fix inconsistent basename suffix stripping with trailing slashes #62378

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1746a9
test_runner: avoid reading process state directly in run()
murataslan1 Mar 21, 2026
4f0bff1
path: fix inconsistent basename suffix stripping with trailing slashes
murataslan1 Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
path: fix inconsistent basename suffix stripping with trailing slashes 
The basename() suffix matching was done in a single pass with the path
component scanning, which caused incorrect results when:
- The path had trailing slashes (e.g., 'a/')
- The suffix contained path separators
- The suffix equaled the basename after a path separator

Fix by separating the two operations: first resolve the basename
(stripping directory and trailing slashes), then strip the suffix
from the result. This restores the pre-#5123 suffix stripping behavior
while keeping the optimized path scanning.

Fixes: #21358
  • Loading branch information
@murataslan1
murataslan1 committed Mar 21, 2026
commit 4f0bff1362330ccb29f3dc0cdbb1ff24741981ce
120 changes: 28 additions & 92 deletions lib/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,51 +913,7 @@ const win32 = {
start = 2;
}

if (suffix !== undefined && suffix.length > 0 && suffix.length <= path.length) {
if (suffix === path)
return '';
let extIdx = suffix.length - 1;
let firstNonSlashEnd = -1;
for (let i = path.length - 1; i >= start; --i) {
const code = StringPrototypeCharCodeAt(path, i);
if (isPathSeparator(code)) {
// If we reached a path separator that was not part of a set of path
// separators at the end of the string, stop now
if (!matchedSlash) {
start = i + 1;
break;
}
} else {
if (firstNonSlashEnd === -1) {
// We saw the first non-path separator, remember this index in case
// we need it if the extension ends up not matching
matchedSlash = false;
firstNonSlashEnd = i + 1;
}
if (extIdx >= 0) {
// Try to match the explicit extension
if (code === StringPrototypeCharCodeAt(suffix, extIdx)) {
if (--extIdx === -1) {
// We matched the extension, so mark this as the end of our path
// component
end = i;
}
} else {
// Extension does not match, so our result is the entire path
// component
extIdx = -1;
end = firstNonSlashEnd;
}
}
}
}

if (start === end)
end = firstNonSlashEnd;
else if (end === -1)
end = path.length;
return StringPrototypeSlice(path, start, end);
}
// Find the basename by scanning backwards, skipping trailing separators
for (let i = path.length - 1; i >= start; --i) {
if (isPathSeparator(StringPrototypeCharCodeAt(path, i))) {
// If we reached a path separator that was not part of a set of path
Expand All @@ -976,7 +932,19 @@ const win32 = {

if (end === -1)
return '';
return StringPrototypeSlice(path, start, end);

const base = StringPrototypeSlice(path, start, end);

// Strip suffix if provided and the basename ends with it
if (suffix !== undefined && suffix.length > 0 && suffix.length <= base.length) {
if (base === suffix)
return '';
if (StringPrototypeSlice(base, base.length - suffix.length) === suffix) {
return StringPrototypeSlice(base, 0, base.length - suffix.length);
}
}

return base;
},

/**
Expand Down Expand Up @@ -1478,51 +1446,7 @@ const posix = {
let end = -1;
let matchedSlash = true;

if (suffix !== undefined && suffix.length > 0 && suffix.length <= path.length) {
if (suffix === path)
return '';
let extIdx = suffix.length - 1;
let firstNonSlashEnd = -1;
for (let i = path.length - 1; i >= 0; --i) {
const code = StringPrototypeCharCodeAt(path, i);
if (code === CHAR_FORWARD_SLASH) {
// If we reached a path separator that was not part of a set of path
// separators at the end of the string, stop now
if (!matchedSlash) {
start = i + 1;
break;
}
} else {
if (firstNonSlashEnd === -1) {
// We saw the first non-path separator, remember this index in case
// we need it if the extension ends up not matching
matchedSlash = false;
firstNonSlashEnd = i + 1;
}
if (extIdx >= 0) {
// Try to match the explicit extension
if (code === StringPrototypeCharCodeAt(suffix, extIdx)) {
if (--extIdx === -1) {
// We matched the extension, so mark this as the end of our path
// component
end = i;
}
} else {
// Extension does not match, so our result is the entire path
// component
extIdx = -1;
end = firstNonSlashEnd;
}
}
}
}

if (start === end)
end = firstNonSlashEnd;
else if (end === -1)
end = path.length;
return StringPrototypeSlice(path, start, end);
}
// Find the basename by scanning backwards, skipping trailing slashes
for (let i = path.length - 1; i >= 0; --i) {
if (StringPrototypeCharCodeAt(path, i) === CHAR_FORWARD_SLASH) {
// If we reached a path separator that was not part of a set of path
Expand All @@ -1541,7 +1465,19 @@ const posix = {

if (end === -1)
return '';
return StringPrototypeSlice(path, start, end);

const base = StringPrototypeSlice(path, start, end);

// Strip suffix if provided and the basename ends with it
if (suffix !== undefined && suffix.length > 0 && suffix.length <= base.length) {
if (base === suffix)
return '';
if (StringPrototypeSlice(base, base.length - suffix.length) === suffix) {
return StringPrototypeSlice(base, 0, base.length - suffix.length);
}
}

return base;
},

/**
Expand Down
27 changes: 21 additions & 6 deletions test/parallel/test-path-basename.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ assert.strictEqual(path.basename('basename.ext/'), 'basename.ext');
assert.strictEqual(path.basename('basename.ext//'), 'basename.ext');
assert.strictEqual(path.basename('aaa/bbb', '/bbb'), 'bbb');
assert.strictEqual(path.basename('aaa/bbb', 'a/bbb'), 'bbb');
assert.strictEqual(path.basename('aaa/bbb', 'bbb'), 'bbb');
assert.strictEqual(path.basename('aaa/bbb//', 'bbb'), 'bbb');
assert.strictEqual(path.basename('aaa/bbb', 'bbb'), '');
assert.strictEqual(path.basename('aaa/bbb//', 'bbb'), '');
assert.strictEqual(path.basename('aaa/bbb', 'bb'), 'b');
assert.strictEqual(path.basename('aaa/bbb', 'b'), 'bb');
assert.strictEqual(path.basename('/aaa/bbb', '/bbb'), 'bbb');
assert.strictEqual(path.basename('/aaa/bbb', 'a/bbb'), 'bbb');
assert.strictEqual(path.basename('/aaa/bbb', 'bbb'), 'bbb');
assert.strictEqual(path.basename('/aaa/bbb//', 'bbb'), 'bbb');
assert.strictEqual(path.basename('/aaa/bbb', 'bbb'), '');
assert.strictEqual(path.basename('/aaa/bbb//', 'bbb'), '');
Comment on lines -27 to +28
Copy link
Copy Markdown
Member

@avivkeller avivkeller Mar 21, 2026

Choose a reason for hiding this comment

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

Given that the previous behavior had tests, it had to have been intentional. If it's intentional, is there a solid rationale for changing the behavior, it's been like this for ages, changing it now seems like it would break thousands of libraries

Copy link
Copy Markdown
Member

@atlowChemi atlowChemi Mar 22, 2026

Choose a reason for hiding this comment

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

In addition to that, I just checked with basename and the results I am getting are actually consistent with the existing (and not with the changes in this PR)

Copy link
Copy Markdown
Author

@murataslan1 murataslan1 Mar 22, 2026

Choose a reason for hiding this comment

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

Fair point. The original issue reporter (@ChALkeR) also noted these inconsistencies were found via brute-force comparison, not from real-world breakage. And you're right that the existing tests encode this behavior intentionally.

Given the pushback and the risk of breaking existing libraries, I'm happy to close this PR. The current behavior has been stable for years and the edge cases (trailing slashes + suffix matching) are unlikely to affect real code.

If there's interest in pursuing this as a semver-major in the future, the approach here (separate basename resolution from suffix stripping) would be the way to do it.

assert.strictEqual(path.basename('/aaa/bbb', 'bb'), 'b');
assert.strictEqual(path.basename('/aaa/bbb', 'b'), 'bb');
assert.strictEqual(path.basename('/aaa/bbb'), 'bbb');
Expand All @@ -44,8 +44,8 @@ assert.strictEqual(path.win32.basename('basename.ext\\\\'), 'basename.ext');
assert.strictEqual(path.win32.basename('foo'), 'foo');
assert.strictEqual(path.win32.basename('aaa\\bbb', '\\bbb'), 'bbb');
assert.strictEqual(path.win32.basename('aaa\\bbb', 'a\\bbb'), 'bbb');
assert.strictEqual(path.win32.basename('aaa\\bbb', 'bbb'), 'bbb');
assert.strictEqual(path.win32.basename('aaa\\bbb\\\\\\\\', 'bbb'), 'bbb');
assert.strictEqual(path.win32.basename('aaa\\bbb', 'bbb'), '');
assert.strictEqual(path.win32.basename('aaa\\bbb\\\\\\\\', 'bbb'), '');
assert.strictEqual(path.win32.basename('aaa\\bbb', 'bb'), 'b');
assert.strictEqual(path.win32.basename('aaa\\bbb', 'b'), 'bb');
assert.strictEqual(path.win32.basename('C:'), '');
Expand All @@ -60,6 +60,21 @@ assert.strictEqual(path.win32.basename('C:foo'), 'foo');
assert.strictEqual(path.win32.basename('file:stream'), 'file:stream');
assert.strictEqual(path.win32.basename('a', 'a'), '');

// Regression tests for https://github.com/nodejs/node/issues/21358
// Suffix stripping should work consistently regardless of trailing slashes
// or leading path components.
assert.strictEqual(path.posix.basename('a/', 'a'), '');
assert.strictEqual(path.posix.basename('a//', 'a'), '');
assert.strictEqual(path.posix.basename('/dd', 'dd'), '');
assert.strictEqual(path.posix.basename('d/dd', 'dd'), '');
assert.strictEqual(path.posix.basename('d/dd/', 'dd'), '');
assert.strictEqual(path.posix.basename('d/dd/', 'd'), 'd');
assert.strictEqual(path.win32.basename('a\\', 'a'), '');
assert.strictEqual(path.win32.basename('a\\\\', 'a'), '');
assert.strictEqual(path.win32.basename('\\dd', 'dd'), '');
assert.strictEqual(path.win32.basename('d\\dd', 'dd'), '');
assert.strictEqual(path.win32.basename('d\\dd\\', 'dd'), '');

// On unix a backslash is just treated as any other character.
assert.strictEqual(path.posix.basename('\\dir\\basename.ext'),
'\\dir\\basename.ext');
Expand Down
Loading