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

feat(gatsby-script): Proxy safelist #35629

Merged
merged 10 commits into from
May 12, 2022

Conversation

tyhopp
Copy link
Contributor

@tyhopp tyhopp commented May 11, 2022

Description

Use a safelist to determine what urls are safe to proxy for the off-the-main thread strategy.

Todo

  • Release new canary
  • Test that it works in Gatsby Cloud

Documentation

TBD

Related Issues

[sc-49857]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 11, 2022
@tyhopp tyhopp added type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement. topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 11, 2022
@tyhopp tyhopp added this to the Script Component milestone May 11, 2022
@tyhopp tyhopp mentioned this pull request May 11, 2022
12 tasks
@tyhopp tyhopp marked this pull request as draft May 11, 2022 05:14
@tyhopp tyhopp marked this pull request as ready for review May 11, 2022 11:07
@tyhopp tyhopp requested a review from mlgualtieri May 12, 2022 02:10
@tyhopp tyhopp force-pushed the feat-script-component-proxy-safelist branch from d0a4ffd to 3936619 Compare May 12, 2022 03:36
@tyhopp tyhopp force-pushed the feat-script-component-proxy-safelist branch from 3936619 to 2e4e189 Compare May 12, 2022 04:13
@pieh pieh merged commit 3c7d7d3 into feat-script-component May 12, 2022
@pieh pieh deleted the feat-script-component-proxy-safelist branch May 12, 2022 09:27
@pieh
Copy link
Contributor

pieh commented May 12, 2022

This wasn't explicitly merged, just I messed up with pushing to tracking branch changes from this PR. This PR can still be reviewed by checking "Changed files" and if there are any adjustments needed we will open new PR with them

Copy link
Contributor

@mlgualtieri mlgualtieri left a comment

Choose a reason for hiding this comment

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

Restricting calls to URLs that are explicitly defined is a good approach, as long as we don't allow any wildcards.

Some thoughts:

  • Can we block access to the partytown reverse proxy to only calls made from the same origin? It likely isn't a good idea to allow anyone or any site on the internet to use the proxy. So basically, lock down the proxy from being an open redirect and limit to use just on the actual domain it's set up for. My thought is that this can be explicitly denied in the code, but also it may be possible to make use of the Access-Control-Allow-Origin header and supply the origin that's allowed. I'm just not sure how the header would be sent for just the reverse proxy, which is why explicitly blocking anything except the same domain is maybe a better approach. (The HTTP header though has other advantages... see my 3rd bullet point.)

  • We should make sure that cookies/credentials do not leak through the reverse proxy. One way this risk is limited when using the <script> tag is to add the crossorigin="anonymous" parameter. I'm not sure where such a parameter would be applied within partytown, or if it's required, but it might be something to look into. (Ref: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/crossorigin)

  • One thing to consider is that the allowlist and any restrictions we code into this will likely be exposed client-side. Am I correct on that? If yes, it would be possible to bypass the restrictions to load any URL through the site or from any source. CORS may help with the scope though, since if we can restrict the loading of the partytown from a third-party source then we would also limit its ability to be easily abused. This gets back to my first bullet point about setting the Access-Control-Allow-Origin header for the partytown URL.

  • It may be beneficial to conceal the referrer from being sent to resources loaded with partytown. This would typically be handled with the Referrer-Policy: same-origin HTTP header, but again like my comment above I'm not sure where this header would go inside partytown, or if it's necessary.

@mlgualtieri
Copy link
Contributor

Can we block access to the partytown reverse proxy to only calls made from the same origin?

I was just re-reading my comments from yesterday, and I'm wondering if this is even possible. Is the proxy loaded through the site or directly by the visitor in the browser? Is it possible to understand the origin of the request from within partytown? Perhaps if we can send the referrer along?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants