Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@pixiebrix/get reports CORS errors and failed requests as "missing permissions" #3037

Closed
1 task
fregante opened this issue Mar 27, 2022 · 3 comments
Closed
1 task
Labels
bug Something isn't working user experience Improve the user experience (UX)

Comments

@fregante
Copy link
Contributor

This issue probably doesn't have a solution because it's a browser limitation.

Repro

  1. Add "HTTP Get" brick
  2. Set URL to https://ghosttext.fregante.com/
  3. Run the extension

You will see that this request succeeds without permissions.

  1. Append ?a=1 to the URL to skip the cache
  2. Go offline
  3. Run the extension

You will see ClientNetworkPermissionError: Insufficient browser permissions to make request. due to this logic:

const hasPermissions = await browser.permissions.contains({
origins: [url.href],
});
if (!hasPermissions) {
return new ClientNetworkPermissionError(
"Insufficient browser permissions to make request.",
maybeAxiosError
);
}

But the logic is actually incorrect:

  • you don't need host permission to request the page
  • the request failed due to a network error rather than a permission error
  1. Set URL to https://www.google.com
  2. Run the extension

See CORS error in the background page:

Access to fetch at 'https://www.google.com/' from origin 'chrome-extension://mpjjildhmpddojocokjkgmlkkkfjnepo' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

👆 this only happens if you're missing the www.google.com permissions, so the ClientNetworkPermissionError shown to the user is actually correct here.

Possible resolutions

  • Detect and differentiate network errors from CORS/permission errors

I don’t think this is actually feasible other than checking for navigator.onLine, but I don’t think it immediately turns false when a single request fails.

Almost related

@fregante fregante added bug Something isn't working user experience Improve the user experience (UX) labels Mar 27, 2022
@twschiller
Copy link
Contributor

twschiller commented Mar 30, 2022

you don't need host permission to request the page

From a PixieBrix security model, the extension should declare the network calls it can make. Related: see #3054.

As you point out, some GET requests don't technically require Chrome Extension permissions based on the CORS settings on the remote server. However, we don't want to rely on this nuance - messaging should be based on the permissions the user has granted

Long-term, we will want to have the check against the extensions declared permissions prior to making a network call and block unpermissioned requests (even if they would be allowed based on other permissions the user has granted). That will ensure that the extension will work when exported to other users

Go offline

Good point that we can't distinguish this. Could we potentially use https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/type? Or, in the enrich method we could ping https://app.pixiebrix.com to detect network connectivity?

Related

@fregante
Copy link
Contributor Author

fregante commented Mar 30, 2022

There's navigator.onLine to detect when we're fully offline, but I don’t think that works when the connection is just bad (e.g. far from the wifi hotspot / someone is microwaving some popcorn)

in the enrich method

Yes if for this check we use fetch directly, or else we'll end up in a loop

fregante added a commit that referenced this issue May 12, 2022
@fregante
Copy link
Contributor Author

A connectivity check was implemented. In the commit above I moved the check higher since being offline is the first thing the user should fix 🙂

It will be merged through #3073

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working user experience Improve the user experience (UX)
Development

No branches or pull requests

2 participants