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

[BUG] CORS Access-Control-Allow-Origin set wrong if using multiple origins #554

Open
1 task done
pimeys opened this issue Apr 22, 2024 · 1 comment · May be fixed by #628
Open
1 task done

[BUG] CORS Access-Control-Allow-Origin set wrong if using multiple origins #554

pimeys opened this issue Apr 22, 2024 · 1 comment · May be fixed by #628

Comments

@pimeys
Copy link

pimeys commented Apr 22, 2024

Is there an existing issue for this?

  • I have searched the existing issues

What version of workers-rs are you using?

0.1.0

What version of wrangler are you using?

3.34.2

Describe the bug

When setting multiple origins to the CORS configuration:

let cors = Cors::new().with_origins(["https://example.com", "https://lwn.net"]);
response.with_cors(&cors)?;

And then sending a pre-flight request with Origin set as https://example.com, the response header is set like this:

Access-Control-Allow-Origin: "https://example.com,https://lwn.net"

This will lead to a CORS error, because the header must be a single origin, not multiple. How tower-http does this is defined here:

https://github.com/tower-rs/tower-http/blob/main/tower-http/src/cors/allow_origin.rs#L124

If the origin value is a list of urls, it uses the header value from the request, finds the origin from the list and sets the header to be exactly one URL matching the request origin. If the origin is not defined in the list, the header should be omitted.

Steps To Reproduce

  1. Send a pre-flight request with Origin: https://example.com
  2. Respond from a worker with CORS origins set to ["https://example.com", "https://lwn.net"]
  3. Witness a CORS error in the browser
@OliverEvans96
Copy link
Contributor

Currently, Response::with_cors doesn't take the request headers as an argument, which it would need to in order to implement this correctly.

Here's how I'm working around this in my code:

    let req_origin = req.headers().get("Origin")?;
    let resp = router.run(req, env).await?;

    let allowed_origins = ["https://example.com", "https://lwn.net"];
    let cors_origins = req_origin
        .map(|o| allowed_origins.into_iter().filter(|&a| a == o).collect())
        .unwrap_or_else(Vec::new);

    let cors = Cors::new()
        .with_origins(cors_origins);

    let resp = resp.with_cors(&cors)?;

@OliverEvans96 OliverEvans96 linked a pull request Aug 29, 2024 that will close this issue
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 a pull request may close this issue.

2 participants