Skip to content

Reject HTTP redirects on filter list and resource downloads#5621

Open
chrmod wants to merge 2 commits into
masterfrom
security/disable-redirects-on-resource-downloads
Open

Reject HTTP redirects on filter list and resource downloads#5621
chrmod wants to merge 2 commits into
masterfrom
security/disable-redirects-on-resource-downloads

Conversation

@chrmod
Copy link
Copy Markdown
Member

@chrmod chrmod commented Apr 24, 2026

Summary

Harden the adblocker against redirect-based supply-chain attacks. Until now, every remote resource fetch followed HTTP redirects by default, which meant a compromised or hijacked origin (GitHub raw content, cdn.ghostery.com) could quietly redirect clients to an attacker-controlled host serving malicious filter lists.

  • Extend the public Fetch type with an optional init argument and have fetchWithRetry pass { redirect: 'error' }, so all consumers going through FiltersEngine.fromLists / fromPrebuiltAds* now reject 3xx responses instead of silently following them.
  • Apply the same { redirect: 'error' } option to the Node fetch calls in the daily assets/update.js resource-refresh script.
  • Replace axios with native fetch in tools/stress-test-engine-update.ts (and drop the axios devDependency), again rejecting redirects explicitly.

The Fetch type change is backwards-compatible — callers passing globalThis.fetch or a narrower one-argument function remain assignable.

Test plan

  • Run FiltersEngine.fromPrebuiltAdsAndTracking(fetch) against the real CDN and confirm the engine still initializes successfully
  • Temporarily point one list URL to a server that returns a 301/302 and confirm initialization fails with a redirect error instead of loading the redirect target
  • Run assets/update.js and confirm resources are refreshed without errors
  • Run the stress-test tool and confirm it still downloads and validates lists via native fetch

Protects against redirect-based attacks where a compromised origin
(GitHub raw, cdn.ghostery.com) could redirect to an attacker-controlled
host serving malicious filter lists. Fetch and axios both follow
redirects by default, so a 3xx response was silently accepted.

Also replaces axios with native fetch in the stress-test tool to drop
an extra dependency.
@chrmod chrmod requested a review from remusao as a code owner April 24, 2026 09:57
@chrmod chrmod added the PR: Internal 🏠 Changes only affect internals label Apr 24, 2026
Copy link
Copy Markdown
Member

@seia-soto seia-soto left a comment

Choose a reason for hiding this comment

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

I don't know if avoiding redirect at all without a flag would be useful because many lists from GitHub may contain redirect unintentionally (like to raw.github...). There should be more similar cases as well. There's a pattern that people often try to do higher level processing in path level levering CDNs.

Migrating from axios to fetch, I wouldn't mind using both despite the recent accident.

Comment on lines -15 to +19
export type Fetch = (url: string) => Promise<FetchResponse>;
interface FetchInit {
redirect?: 'error' | 'follow' | 'manual';
}

export type Fetch = (url: string, init?: FetchInit) => Promise<FetchResponse>;
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.

It should be "RequestInit" from built-in.

@philipp-classen
Copy link
Copy Markdown
Member

philipp-classen commented Apr 28, 2026

Not following redirects of arbitrary resources blindly makes sense, but here the Ghostery Adblocker is download from Ghostery infrastructure. If an attacker can control it, they could directly serve malicious content. (There is not much to do in that scenario. The best that I could see would be to sign the resources and hope that attackers are not also able to fake those signatures.)

In other words, there is no compelling attack vector IMHO that justifies that we prevent redirects.

A side-effect in the PR is to replace Axios by native fetch. I do not mind, but it is a different topic. I would recommend to open a new one for that.

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

Labels

PR: Internal 🏠 Changes only affect internals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants