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

Pass X-Cors-Headers between the client and the remote #471

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

infinity0
Copy link

This is basically what https://github.com/Zibri/cloudflare-cors-anywhere does, except it chooses to use "Cors-Received-Headers" for the incoming headers, which IMO is wrong as it doesn't start with X-.

As mentioned in #469 this is a way for client application code to process cookies of 3rd-party domains without going through the browser's native cookie mechanisms. Then, there is basically no chance of mixing things up as client application code does not have access to 3rd party cookies of other domains (e.g. the ones being proxied) anyway.

The general idea is that the client parses the cookie headers manually and stores them in some place other than the actual browser cookie storage for the site hosting the application, e.g.:

    let remoteCookies = [];
    ...

    const resp = await fetch(
      corsProxy + URL,
      ...
    )
    await resp.text();

    let corsHeaders = JSON.parse(resp.headers.get("x-cors-headers") || "{}");
    for (let cookie of corsHeaders["set-cookie"]) {
      cookie = cookie.split(";")[0];
      let cname = cookie.split("=")[0];
      if (ALLOW_COOKIES.includes(cname)) {
        remoteCookies.push(cookie);
      }
    }
    ...

    const resp2 = await fetch(
      corsProxy + URL2,
      {
        headers: {
          "X-Cors-Headers": JSON.stringify({"cookie": remoteCookies.join("; ")}),
          ...
        },
      }
    )
    const body2 = await resp2.text();

For simplicity I didn't restrict what can be sent out via X-Cors-Headers but if you prefer to restrict that to Cookie: only, that's also fine of course. (The incoming headers are restricted to Set-Cookie because the other headers are already available via the normal mechanisms.)

@Rob--W
Copy link
Owner

Rob--W commented Oct 9, 2023

One of the major benefits of CORS Anywhere is its simplicity: just prepend the proxy url and it works as if the destination server responded with CORS headers.

With the proposal, clients have to write a full cookie parser themselves, which is non-trivial. For people who need full-fledged cookie functionality, I would recommend a dedicated unprivileged domain with a wildcard subdomain, and then changing the URL by just appending the proxy domain to the original request URL. Then the server can transform domain cookies by appending the domain similarly, and take advantage of the browser's ability to separate cookies. There is some more complexity involved, such as accounting for public suffixes, but better than forcing the logic on the client.

Your client side code snippet already has an obvious bug for example: you're assuming that Set- Cookie lines can be concatenated and joined by ; and then returned to the server in the Cookie header. That logic is incorrect: there is one cookie per Set-Cookie header, and there may be zero or more additional flags (e.g. HttpOnly, domain, SameSite, etc.).

Moreover, CORS Anywhere follows redirect (and allows the browser to do so), potentially across origins. Your proposed client logic is vulnerable to the issues I sketched in your previous pull request, except now due to an insecure client implementation.

I have no interest in maintaining such dangerous logic, whether on the client or server. For that reason I won't merge this PR.

@infinity0
Copy link
Author

That logic is incorrect: there is one cookie per Set-Cookie header,

That's what this line does:

      cookie = cookie.split(";")[0];

But even if you were correct and my logic above was wrong, you are missing the point of the design. The whole point is not to write a fully-fledged cookie parser. The application developer using the proxy only needs to write logic that is correct for their specific use-case i.e. the specific 3rd-party website the application is interacting through the proxy with. They keep it correct through repeated real-world testing against that specific 3rd-party website.

There's no need to have all your code be 100% general and future-proof all the time in real-world development, because those cases simply won't come up.

@Rob--W
Copy link
Owner

Rob--W commented Oct 9, 2023

That logic is incorrect: there is one cookie per Set-Cookie header,

That's what this line does:

      cookie = cookie.split(";")[0];

But even if you were correct and my logic above was wrong, you are missing the point of the design.

Right, the "one cookie" part is implemented correctly. I should have emphasized the flags part: it doesn't account for cookie lifetimes as expires/max-age are not handled, and therefore a deleted cookie may be mirrored.

The whole point is not to write a fully-fledged cookie parser. The application developer using the proxy only needs to write logic that is correct for their specific use-case i.e. the specific 3rd-party website the application is interacting through the proxy with. They keep it correct through repeated real-world testing against that specific 3rd-party website.

That is fine in specific applications with a limited scope.

There's no need to have all your code be 100% general and future-proof all the time in real-world development, because those cases simply won't come up.

As a library maintainer, whatever I publish needs to work in the general case, ideally offered through an API that is easy to use. The solution offered here works for specific cases, with a non-standard API.

The ideal API is one where the result is the same regardless of whether a proxy is used. By supporting the functionality with Access-Control-Allow-Credentials and the domains I sketched in my last reply, the functionality works well without the need for client-side postprocessing. This is especially relevant for non-fetch requests where crossorigin requests are meaningful, such as images.

@infinity0
Copy link
Author

The ideal API is one where the result is the same regardless of whether a proxy is used.

I'm not sure what sort of use case you have in mind. The whole point of CORS-anywhere is that the proxy is necessary due to CORS errors. The result of not using the proxy is that things don't work.

My use case is using the proxy to access an actual 3rd-party site that I have no control over. It sounds like you're thinking of the use-case where CORS-anywhere is used as a convenience tool to access websites you do have control over but can't be bothered setting CORS headers the "proper" correct way through the actual source webserver.

By supporting the functionality with Access-Control-Allow-Credentials and the domains I sketched in my last reply, the functionality works well without the need for client-side postprocessing. [..]

I'm not sure what you're talking about here. CORS-anywhere currently doesn't set Access-Control-Allow-Credentials anywhere, in fact you rejected my last PR which was to set Access-Control-Allow-Credentials.

In fact, using Access-Control-Allow-Credentials is dangerous because it allows the browser to potentially pass credentials on the current domain to the proxy. This PR avoids this approach entirely, and is safe and not vulnerable to these leaks, because it does not rely on having the browser pass its existing cookies to the proxy at all. If the 3rd party website being proxied wants to set any cookies, the application logic handles this completely separately from the browser.

For example, suppose as the application developer you want to have an iframe to Google's public services, and don't want to deal with the user signing in. This PR will let you handle this case gracefully and safely, by receiving any anti-spam Set-Cookies from Google via X-Cors-Headers, allowing you to deal with this completely separately from the browser's own cookies from Google that include the user's private credentials.

@infinity0
Copy link
Author

infinity0 commented Oct 9, 2023

The solution offered here works for specific cases, with a non-standard API.

The solution offered in this PR works for specific 3rd-party websites that an application developer wants to proxy yes. Isn't that the general use case for CORS-anywhere? What sorts of people do you think will use CORS-anywhere to proxy to general arbitrary 3rd-party websites? Sure, this PR won't handle this specific use-case, but that's not how I imagine most people use CORS-anywhere.

In case that was confusing:

  1. general use case: proxying to specific 3rd-party websites <-- this PR
  2. specific use case: proxying to general 3rd-party websites <-- not this PR

Do you really think that (2) is a more common use-case of CORS-anywhere than (1)?

@Rob--W
Copy link
Owner

Rob--W commented Oct 9, 2023

The ideal API is one where the result is the same regardless of whether a proxy is used.

I'm not sure what sort of use case you have in mind. The whole point of CORS-anywhere is that the proxy is necessary due to CORS errors. The result of not using the proxy is that things don't work

The comparison is against when the destination (finally or doesn't even need to) supports CORS. Currently it's then a matter of removing the proxy URL prefix to get the API to be working as intended.

My use case is using the proxy to access an actual 3rd-party site that I have no control over.

To be clear, this is is the primary use case.

By supporting the functionality with Access-Control-Allow-Credentials and the domains I sketched in my last reply, the functionality works well without the need for client-side postprocessing. [..]

I'm not sure what you're talking about here. CORS-anywhere currently doesn't set Access-Control-Allow-Credentials anywhere, in fact you rejected my last PR which was to set Access-Control-Allow-Credentials.

I am describing the preferred approach to implementing generic support for cookies in the hypothetical situation where I would accept credentials. To be clear, I have consistently rejected all requests for cookies, as CORS Anywhere's scope is p ublic resources only.

Your approach here is a reasonable solution to work around this constraint.

In fact, using Access-Control-Allow-Credentials is dangerous because it allows the browser to potentially pass credentials on the current domain to the proxy. This PR avoids this approach entirely, and is safe and not vulnerable to these leaks, because it does not rely on having the browser pass its existing cookies to the proxy at all.

The only distinction is that leaks are application-specific and not automatic by the browser. That is an improvement over a trivial insecure Access-Control-Allow-Credentials header, but there are still two issues with the proposed approach:

  • server-side, redirects are followed automatically unless the maxRedirects option is set to 0. Request headers are generally included in the new request, potentially across origins.
  • even without server-side redirect following, the proposed client-side code doesn't check the origin of the final response URL. Any cookies received from a final redirect target can then be received by the initial URL.

Your use case describes mirroring of cookies that are essentially anonymous. For these it may be an acceptable trade-off to leave these issues unaddressed. If the cookies are tied to anything sensitive and there is any chance for a cross-origin redirect to happen at all, then these issues should not be ignored.

@bulk88
Copy link
Contributor

bulk88 commented Oct 15, 2023

With the proposal, clients have to write a full cookie parser themselves, which is non-trivial. For people who need full-fledged cookie functionality, I would recommend a dedicated unprivileged domain with a wildcard subdomain, and then changing the URL by just appending the proxy domain to the original request URL. Then the server can transform domain cookies by appending the domain similarly, and take advantage of the browser's ability to separate cookies. There is some more complexity involved, such as accounting for public suffixes, but better than forcing the logic on the client.

I've in a private fork of CORS-A added, path= cookie flag, and "/https://foo.com", works 90% of cases for my whitelisted special case forked codebase domains, that I dont want cookie sharing between all domains on my instance. Works for 90% of cases, but breaks down if '''path=/https://foo.com''' '''path=/https://api.foo.com''' '''path=/https://imgcdn.foo.com''' '''path=/https://graphql.foo.com''' '''path=/https://auth.foo.com''' need to share cookies. Your suggestion of wildcard DNS, works, ish, I've done it manually on a Cloudflare workers proxy, but I think a low amount of hosting stacks permit wildcard DNS. A crazy fix would be, but this breaks CORS-A public API. '''path=/https://com.foo/''' '''path=/https://com.foo.auth/''', yes, the REAL way dns domains are written under the hood with DNS protocol. These r ideas for private forks. It wud break public API.

I did have to import a cookie parser NPM to my fork, otherwise Set-Cookie: token="lETxAtQ9gh5kTmSiuCBwCA==;jEu3bbTJBQnA5rqDEYKWWg=="; Expires=Wed, 21 Oct 2015 07:28:00 GMT; Secure; HttpOnly goes very wrong. Oh yes, I've seen semicolon litterals in cookies. B85 and readable JSON in a cookie. BASE64ing JSON in a cookie, is good practice, not a fatal exception throw. Original HTTP spec, and this is what FF/Chrome/Safari/IE/Nestcape 4 follow in 2023 AFAIK, allow \ escape of ; and "

https://www.rfc-editor.org/rfc/rfc7230#section-3.2.6

.split('"') is a bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants