Allow Authorization header pass-through with X-Cors-Proxy-Allowed-Request-Headers

[maxschmeling]

Motivation for the change, related issues

This change comes out of our need to make authenticated external API requests from the WordPress backend in Remote Data Blocks.

Prior to this pull request, the CORS proxy would always strip out the Cookie, Authorization, and Host request headers for security / privacy reasons. However, that makes it impossible to authenticate WordPress backend requests through the proxy to APIs that require Authorization headers.

This change adds a X-Cors-Proxy-Allowed-Request-Headers and functionality to allow the Authorization header through when it is included in the new special header. This is a compromise that enables the authorization use case while preventing accidental or malicious exposure of credentials since the developer has to explicitly opt-in for the request.

Implementation details

The code updates filter_headers_by_name to accept an array of headers that are always removed and an array of headers that are removed unless the header name is present in X-Cors-Proxy-Allowed-Request-Headers. We only add Authorization to the latter array for now.

Testing Instructions (or ideally a Blueprint)

• Make a proxied remote request with an Authorization header and see that the header is removed.
• Make the same request but include X-Cors-Proxy-Allowed-Request-Headers=Authorization and see that the header makes it through.

[brandonpayton]

If a user directly navigates to a CORS proxy URL, is challenged by the target host with a WWW-Authenticate header, and submits credentials? Won't the browser automatically begin sending an Authorization: Basic XXXXX header?

If so, the browser will send this to every request to the CORS proxy, regardless of the proxy target. So all kinds of hosts might receive a Basic Authorization header that was only meant for one host.

This scenario might be contrived, but it is possible.

I don't think it is a good idea to simply allow all Authorization request headers. If we judge it is safe to allow some Authorization headers, I think we should constrain what is allowed. Perhaps the following measures would be good:

• Remove WWW-Authenticate headers from all responses**
• Only allow authorization headers of the form Authorization: Bearer XXXXX
• Alternately: Filter out authorization of the form Authorization: Basic XXXXX

** We remove Authorization headers from responses, but those aren't standard headers for HTTP responses. It shouldn't hurt anything though, and maybe they are omitted for a WP-related reason.

[maxschmeling]

It's definitely possible. I don't know if I'm totally convinced that it's a big enough risk to worry about (how often will people be browsing these URLs directly through the browser?) but better safe than sorry probably.

I think allowing basic auth requests through is valuable because there are APIs that require it, though much less common.

Going based just on the options listed, I would vote that it strips out WWW-Authenticate in responses but doesn't limit the Authorization header on requests. This would maintain support for endpoints that require basic auth from the fetch request.

However, I have one other thought. What if the proxy requires another non-standard header (X-Allow-Auth-Passthrough?) to be present before allowing the Authorization header to be passed through? This wouldn't completely eliminate the ability for credentials to be mistakenly passed, but it minimizes it quite a bit by requiring explicit opt-in. It's not ideal because it makes playground concerns less "transparent", but limits some risk.

Maybe some combination? The option I suggest just for Basic? Open to your thoughts.

[brandonpayton]

@maxschmeling I am uncomfortable with loosening the security of the CORS proxy in this case and feel like we probably do not have a complete understanding of all the ramifications. At first, I was going to argue against making the change at all but realized that the problem for Remote Data Blocks is something like:

With a regular PHP server, Remote Data Blocks isn't subject to CORS restrictions, but Remote Data Blocks in the Playground web app is subject to CORS restrictions because everything runs in the browser.

Is that right?

Here are some additional questions to help our understanding:

• It seems like web services that take Authorization: Bearer XXXXXX request headers would typically support CORS. Why is a CORS proxy required at all?
• When using Remote Data Blocks within playground.wordpress.net, where are the bearer tokens coming from?
• Are the tokens stored within the client-side instance of WP?
• Would it matter if the tokens were leaked?

I am concerned that secrets will be stored in OPFS by Playground and possibly be exposed to other sites that embed a Playground <iframe>, obtain a Playground client for the existing site, and use it to run arbitrary PHP on the site.

It's possible my mental model is incorrect, but I think an attack like that may be possible. @adamziel and @bgrgicak, what do you think?

[maxschmeling]

Two of our primary API integrations (Google Sheets and Airtable) do not support cross origin requests from the browser.

Our plugin stores the tokens in WP options. There are different routes we could go for sharing things depending on sensitivity. We wouldn't embed sensitive long living tokens in a blueprint that we're sharing. But it would be nice to be able to launch a test or demo that we can enter a token into and see our plugin function.

[brandonpayton]

Two of our primary API integrations (Google Sheets and Airtable) do not support cross origin requests from the browser.

Interesting! Thanks for the examples.

Our plugin stores the tokens in WP options. There are different routes we could go for sharing things depending on sensitivity. We wouldn't embed sensitive long living tokens in a blueprint that we're sharing. But it would be nice to be able to launch a test or demo that we can enter a token into and see our plugin function.

OK, this means the secrets would be persisted today if a playground.wordpress.net user saves a site.

If it turns out to be possible for sites embedding Playground to extract info from a previously-saved running site (and if we don't choose to limit it somehow), I wonder if we can do something to maintain secrets in memory only, maybe an in-memory-only site that cannot be saved or a way to obtain and access session-only secrets.

🤔 It's possible I'm overthinking this, but I'd rather be careful.

As for what the CORS proxy allows, you mentioned an interesting idea earlier:

However, I have one other thought. What if the proxy requires another non-standard header (X-Allow-Auth-Passthrough?) to be present before allowing the Authorization header to be passed through? This wouldn't completely eliminate the ability for credentials to be mistakenly passed, but it minimizes it quite a bit by requiring explicit opt-in. It's not ideal because it makes playground concerns less "transparent", but limits some risk.

I think this is probably a good idea. It isn't an ideal developer experience, but it would remove much of the risk of default behaviors to require an explicit opt-in.

Within the scope of this PR, maybe this is the a good, simple course of action to unblock you, and we can continue discussing risks of persisting secrets.

[chriszarate]

It isn't an ideal developer experience, but it would remove much of the risk of default behaviors to require an explicit opt-in.

Using the CORS proxy at all requires opt-in code changes, in order to target the proxy URL. I assume this is in the scenario where something like #1901 lands, and external requests hit the CORS proxy by default?

Either way, opting-in to allowing sensitive headers seems good to me. I would propose a header that explicitly enumerates the allowed headers (and therefore the headers that should be passed through to the destination URL). This could be merged with a default set of safe, always-allowed headers.

X-Playground-Proxy-Allowed-Headers: Authorization, X-User-ID

If CORS proxy usage takes off, I would eventually love to see opt-in and configuration take place at the blueprint level, so that plugin code modifications would not be required. But this approach will work great for now!

Remove WWW-Authenticate headers from all responses

Agreed. Browser auth challenges are firmly out-of-scope for a proxy that is assisting in simulated server-to-server communication.

If it turns out to be possible for sites embedding Playground to extract info from a previously-saved running site (and if we don't choose to limit it somehow), I wonder if we can do something to maintain secrets in memory only

I understand the concern and protective stance, even though it seems totally separate from which headers are allowed for the CORS proxy. My 2¢: Since data is persisted into OPFS by default, secrets will end up there despite all best efforts. Identifying secrets will require attention and opt-in from third-parties, which will not happen. Since OPFS is by definition private to the origin, wouldn't instance-specific pseudo-random subdomains solve for this? e.g., happy-ocean-karma.playground.wordpress.net?

[brandonpayton]

I understand the concern and protective stance, even though it seems totally separate from which headers are allowed for the CORS proxy.

@chriszarate Agreed. The concern about secret persistence is totally separate. I think it would be good if this PR added the kind of opt-in header you described:

X-Playground-Proxy-Allowed-Headers: Authorization, X-User-ID

There might still be request headers that are explicitly disallowed (like Cookie), but opting into allowing normally-blocked request headers seems like a good approach.

As for the name, may I propose X-Cors-Proxy-Allowed-Request-Headers? To reduce the risk that possible abuse poses to playground.wordpress.net, we are planning to move the proxy to a dedicated domain (and add other safety measures).

[adamziel]

@maxschmeling poke

[maxschmeling]

@adamziel and @brandonpayton: I've pushed a quick update, I'm adding tests and making sure it actually functions properly and should have it finalized shortly. But if you want to take a quick peak at the approach and tell me what you think that'd be great.

[brandonpayton]

Thanks for your work on this, @maxschmeling! I think this is in a pretty good place, but I'd like to leave allowed-header constraint for a later PR and left a note for that.

[brandonpayton]

I like this idea, but right now, we are passing through most request headers. Let's leave the decision about whether to constrain the allowed list to another PR.

Instead for the purpose of this PR, let's have a list that is something like $headers_requiring_opt_in in addition to $strictly_disallowed_headers.

[brandonpayton]

Thanks for the updates, @maxschmeling!

I ended up pushing some additional changes:

• made some opinionated changes to code style
• added some documentation explaining why we were treating headers a certain way
• started filtering out WWW-Authenticate response headers, which I'd forgotten we'd discussed

Let's let the tests run, but I think this is good to merge.

[adamziel]

Can we update the PR description to document the code change, similarly to what "developer notes" are in WP core? I'm confused what's the usage @maxschmeling @brandonpayton

[maxschmeling]

Can we update the PR description to document the code change, similarly to what "developer notes" are in WP core? I'm confused what's the usage @maxschmeling @brandonpayton

I've updated the PR description to be a little more clear. Let me know if that is good.

[adamziel]

@maxschmeling No edits visible on my end yet

[maxschmeling]

@maxschmeling No edits visible on my end yet

Ope, sorry, saved now.

[adamziel]

Lovely, thank you!

[brandonpayton]

Can we update the PR description to document the code change, similarly to what "developer notes" are in WP core? I'm confused what's the usage

Sorry, that was on me, @adamziel. Thanks for updating the description, @maxschmeling!