Skip to content
GitLab
Projects Groups Topics Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Register
  • Sign in
  • ivatar ivatar
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributor statistics
    • Graph
    • Compare revisions
  • Issues 2
    • Issues 2
    • List
    • Boards
    • Service Desk
    • Milestones
  • Merge requests 0
    • Merge requests 0
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Artifacts
    • Schedules
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages and registries
    • Packages and registries
    • Container Registry
  • Monitor
    • Monitor
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Repository
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • Oliver Falk
  • ivatarivatar
  • Merge requests
  • !207

fix: validation for trusted urls

  • Review changes

  • Download
  • Patches
  • Plain diff
Merged Seth Falco requested to merge Seth/ivatar:fix-trusted-urls into devel Jul 15, 2022
  • Overview 1
  • Commits 1
  • Pipelines 6
  • Changes 4

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:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/events/UrlFilter

Implementation of UrlFilters handling on Firefox:
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/MatchURLFilters.jsm#68

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 config.py 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 Jul 16, 2022 by Seth Falco
Assignee
Assign to
Reviewers
Request review from
Time tracking
Source branch: fix-trusted-urls