Skip to content

Fix disallowed-character stripping when replacement is an empty string#559

Merged
Trott merged 4 commits into
Trott:mainfrom
SimYunSup:fix/empty-replacement-bypass
Jul 21, 2026
Merged

Fix disallowed-character stripping when replacement is an empty string#559
Trott merged 4 commits into
Trott:mainfrom
SimYunSup:fix/empty-replacement-bypass

Conversation

@SimYunSup

Copy link
Copy Markdown
Contributor

When replacement is an empty string, disallowed characters are no longer stripped, so the output can contain characters outside the documented allowed set.

The per-character loop has a branch that preserves the replacement string when a character contains it:

} else if (char.includes(opts.replacement)) {

''.includes('') is always true, so with an empty replacement every character takes this branch and the disallowedChars filter in the final else never runs. The result:

slug('a<b>c')       // 'abc'
slug('a<b>c', '')   // 'a<b>c'   (before this change)

This is inconsistent: the default replacement strips < and >, but an empty replacement lets them through, even though both modes promise output within [A-Za-z0-9] (pretty) / [\w\-.~] (rfc3986).

The fix skips that branch when the replacement is empty, so an empty replacement falls through to the filter:

} else if (opts.replacement !== '' && char.includes(opts.replacement)) {

After the change, slug('a<b>c', '') returns 'abc', while slug('foo bar baz', '') still returns 'foobarbaz'. Added a test covering the empty-replacement case in both pretty and rfc3986 modes.

String.prototype.includes('') is always true, so an empty replacement
matched the replacement-preserving branch and skipped the disallowedChars
sanitizer, letting dangerous characters (< > " ' ( ) / and ..) pass
through unchanged (XSS / path traversal). Guard the branch with
opts.replacement !== '' so an empty replacement falls through to the
sanitizer. Add a regression test.
Comment thread test/slug.test.js Outdated
Comment thread test/slug.test.js Outdated
Comment thread test/slug.test.js Outdated
@Trott
Trott merged commit a330fce into Trott:main Jul 21, 2026
5 checks passed
@Trott

Trott commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Thanks for the fix. 🚀

github-actions Bot pushed a commit that referenced this pull request Jul 21, 2026
## [12.0.1](v12.0.0...v12.0.1) (2026-07-21)

### Bug Fixes

* sanitize disallowed characters when replacement is an empty string([#559](#559)) ([a330fce](a330fce))
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