freelawproject / recap

This repository is for filing issues on any RECAP-related effort.
https://free.law/recap/
12 stars 4 forks source link

Chrome extension azurewebsites.us permission seems too broad #374

Closed jmhodges closed 1 month ago

jmhodges commented 1 month ago

Hey, the Chrome extension is requesting new permissions: read and change your data on azurewebsites.us.

That's the whole domain that, I believe, is used by a lot of different possible Azure-hosted sites. That permission seems much more broad then necessary (and makes me nervy about running the RECAP extension).

I suspect y'all could set it to need a specific RECAP-specific subdomain(s) if need be (or, even nicer, some RECAP-controlled domain itself).

(I'm also assuming this was an intentional decision on your part and not someone you didn't expect to have control to have futzed with the extension)

jmhodges commented 1 month ago

Ah, the extension is open source!

A friend and I took a look and think the problem might have cropped up in this PR that is about adding a helper method that also did some other work. Specifically, here

To be specific, the problem seems to be the azurewebsites.us addition to the content_scripts.matches part of the manifest.json that is much more general than similar domains listed in permissions

mlissner commented 1 month ago

I'm not entirely sure, but I think this might have been on purpose. There are real tradeoffs to be had here, because most extensions just ask for tons of permissions up front and then they don't have to keep popping things up all the time.

We ask for very narrow ones at the outset and then people get, well, nervy, when we ask for more and more to make it work in more places.

The other complicating factor is that we tried to predict all the URLs that we need to support ACMS, but we don't want to prompt more than once.

I guess we could expand the list in the content_scripts key so it matches the permissions key.

@ERosendo, any idea why we didn't do that?

I'm honestly torn on this. It's just easier to ask once and be broad. It's what most extensions do in my experience, and most users would just agree once and not have to think about permissions again from that point on. Whenever we expand like this people don't like it, even if they'd have breezed past it at the outset.

OTOH, obviously being more specific is what some of our users prefer, it's more secure and private, and those people aren't wrong!

ERosendo commented 1 month ago

Hey guys!!!

the problem seems to be the azurewebsites.us addition to the content_scripts.matches part of the manifest.json that is much more general than similar domains listed in permissions

While both the permissions and content_scripts sections in the extension's manifest allow specifying URLs, they use different methods for pattern matching.

For permissions, you can define URLs (or groups of URLs) using a specific format:

<scheme>://<host>/<path>

scheme: Must be one of the following: http, https, *( which matches only http or https) or file. The scheme must be separated from the rest of the pattern using a double slash (//):

host: This can be a hostname (e.g., www.example.com), include a * before the hostname to match subdomains (.example.com), or just a wildcard . - If you use a wildcard in the host pattern, it must be the first or only character, and it must be followed by a period (.) or forward slash (/).

path: A URL path (/example). For host permissions, the path is required but ignored.

The rule for subdomain matching within the URL format only allows for two possibilities: using *.azurewebsites.us, or adding each combination without using the wildcard. When we updated the list of permissions to include the Azure domain, this PR took the highly specific approach and included every single identified URL combination.

In the context_script array, the URLs within the matches property must adhere to the previously mentioned rules. However, for more granular control over which pages the file is injected into, the manifest offers additional properties: exclude_matches, include_globs, and exclude_globs. These properties work together to define a set of rules the extension uses to match URLs.

The extension will only inject a file into a page after evaluating these conditions:

I guess we could expand the list in the content_scripts key so it matches the permissions key.

@mlissner You're right about this. We can expand the matches property and include every single URL from the permissions list.

@ERosendo, any idea why we didn't do that?

I thought using include_globs would be much easier than manually listing every single URL.

The PR updated both the matches and include_globs properties to use the following pattern:

*://*-showdoc.azurewebsites.us/* 

It's important to note that we're using this pattern only to insert CSS files to guarantee the proper rendering of banners and buttons.

"content_scripts": [{
    "matches": ["*://*.uscourts.gov/*", "*://*.azurewebsites.us/*"],
    "include_globs": ["*://ecf.*", "*://ecf-train.*", "*://pacer.*", "*://*-showdoc.*"],
    "css": [
      "assets/css/style.css",
      "assets/css/font-awesome.css"
    ],
    "run_at": "document_end"
  }

Check the Chrome extensions documentation for examples of matches, include_globs, exclude_matches, and exclude_globs: https://developer.chrome.com/docs/extensions/reference/manifest/content-scripts#matching-samples

mlissner commented 1 month ago

Thanks Eduardo. Do you think you could stop the roll out of chrome if it's not too late, and we can add all the permissions tomorrow?

I'm getting a few questions about this. More than usual and more than I expected.

ERosendo commented 1 month ago

Do you think you could stop the roll out of chrome if it's not too late, and we can add all the permissions tomorrow?

@mlissner Chrome Web Store offers a rollback feature that allows us to revert to the previous version. This will pause the rollout of the new version with the broad permissions.

https://developer.chrome.com/docs/webstore/rollback

Let me know what you think about the rollback feature, i'm happy to initiate the process through the developer dashboard and update the context_script property to remove unnecessary permissions.