Skip to content

fix: validation for trusted urls

Seth Falco requested to merge Seth/ivatar:fix-trusted-urls into devel

The configuration might a bit verbose, so you're welcome to hate it, I thought this might be the easiest way to split everything, so it's easy to maintain, while having more certainty that it's safe. ^-^'

It's based on how UrlFilters are used for Web Extensions.

Docs on UrlFilters:

Implementation of UrlFilters handling on Firefox:

I didn't implement the entire thing the way Mozilla did, just enough to match the rules we actually use in the config.

This will ideally remove the possibility of open-redirects.

I could've just replaced the check for startswith instead of in, but I thought to support the broadest range of URLs we might encounter without having to rework the implementation, a flexible approach like this would be best. (Regardless of if the dynamic part of the URL is in a subdomain, route parameter which isn't the final one, etc.)

This approach also makes it a bit easier to support similar URLs, i.e. multiple subdomains or schemes for the same host.

Open Questions

How to handle no URL filters?

I'm not sure if can be overridden outside modifying the file itself, but it could be worth considering what the behavior should be when no URL filters are configured.

Should an empty list mean all URLs are trusted, or that the trusted URLs feature is disabled? Or that no URL is trusted, and that the default effectively does nothing?

I have no preference, just wanted to put the thought forward.

Breaking changes?

This might be considered a breaking change if the config is considered part of the public API. 🤔
If needed, I could add an adapter that parses string literals into the UrlFilter-like object defined in the config.

Edited by Seth Falco

Merge request reports