Skip to content

fix: normalizedUrl is set too early#5743

Open
seia-soto wants to merge 3 commits into
ghostery:masterfrom
seia-soto:fix-normalized-url-timing
Open

fix: normalizedUrl is set too early#5743
seia-soto wants to merge 3 commits into
ghostery:masterfrom
seia-soto:fix-normalized-url-timing

Conversation

@seia-soto

Copy link
Copy Markdown
Member

fixes #5703

Setting normalizedUrl earlier may result in performance degradation and inconsistent behaviour in matching. checkPattern function uses this property depending on the case sensitivity and using the full base64 string instead of shorten version would add a computation pressure.

fixes ghostery#5703

Setting `normalizedUrl` earlier may result in performance degradation and inconsistent behaviour in matching. `checkPattern` function uses this property depending on the case sensitivity and using the full base64 string instead of shorten version would add a computation pressure.
@seia-soto seia-soto self-assigned this Jun 23, 2026
@seia-soto seia-soto requested a review from remusao as a code owner June 23, 2026 18:21
@seia-soto seia-soto added the PR: Bug Fix 🐛 Increment patch version when merged label Jun 23, 2026
Comment thread packages/adblocker/src/request.ts Outdated
Co-authored-by: Philipp Claßen <philipp.classen@posteo.de>

@chrmod chrmod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if that lets to matching problems, please provide a proof in form of a test.

Also, that made me wonder if we should ever lowercase data urls. What do you think?

@seia-soto

Copy link
Copy Markdown
Member Author

At the bottom level, the affected scope would be restricted by the data URIs and the data URIs should not be lowercased at all. However, the original logic cuts the data URIs at comma which is not directly visible from the current diff. Therefore, it's safe that we can just proceed without considering it (maybe a comment worth it).

@seia-soto

Copy link
Copy Markdown
Member Author

The problem is the input value to the checkPattern function gets different depending on the filter case sensitivity led by the modifier. It happens because this.url value will have a modified value of data URIs and this.normalizedUrl will have a raw URL but lower cased.

@seia-soto

Copy link
Copy Markdown
Member Author

Added a failing test case but again - we rather should not process data uri contents.

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

Labels

PR: Bug Fix 🐛 Increment patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

normalizedUrl is set too early

3 participants