Allow cross-origin requests

[diamondburned]

This PR modifies CORS behaviors so that when invoking from a different origin, the server will respond with the appropriate CORS headers that allow that origin instead of not including any header at all.

Here's a curl demoing these headers:

―❤―▶ curl -X POST -D - -H 'Origin: http://google.com' -H 'Content-Type: application/json' -d '{"username_or_email": "diamond", "password": "123123123123"}' http://0.0.0.0:8536/api/v3/user/login
HTTP/1.1 200 OK
content-length: 208
vary: Origin, Access-Control-Request-Method, Access-Control-Request-Headers
access-control-expose-headers: content-type, access-control-allow-origin
access-control-allow-origin: http://google.com
content-type: application/json
date: Fri, 30 Jun 2023 05:11:09 GMT

{"jwt":"eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJzdWIiOjEsImlzcyI6ImxlbW15LWFscGhhIiwiaWF0IjoxNjg4MTAxODcwfQ.wHWwTcIYOHs7rShfrQtZToexFZfbrXMDQzoQimR6QeQ","registration_created":false,"verify_email_sent":false}

Note: The presence of the header access-control-allow-origin: http://google.com given Origin: http://google.com is the most important part.

Note: This does not lead to the browser leaking cookies. Calling fetch with credentials: "include" will cause the browser to error out because there's no Access-Control-Allow-Credentials, which is what we want:

await fetch("http://0.0.0.0:8536/api/v3/site", { credentials: "include" })
Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at http://0.0.0.0:8536/api/v3/site. (Reason: expected ‘true’ in CORS header ‘Access-Control-Allow-Credentials’).

Uncaught (in promise) TypeError: NetworkError when attempting to fetch resource.
await fetch("http://0.0.0.0:8536/api/v3/site")
Response { type: "cors", url: "http://0.0.0.0:8536/api/v3/site", redirected: false, status: 200, ok: true, statusText: "OK", headers: Headers(4), body: ReadableStream, bodyUsed: false }

This PR fixes issue #3109.

This PR replaces/closes PR #3408 and #3301.

[ayan4m1]

This PR would break the existing ability to set a specific hostname, which breaks my use-case of only allowing my site access (I do not federate).

[diamondburned]

This PR would break the existing ability to set a specific hostname, which breaks my use-case of only allowing my site access (I do not federate).

Would doing this in your web server work?

(I can add the environment variable back. I just don't know enough Rust to do that, so it'll take me a while.)

[auouymous]

@ayan4m1 Do you know CORS only stops web browsers from accessing the API, and that nothing can be done to stop bots and apps from accessing it? Even if not federated, your users might want to use a third-party website which provides a better interface.

@diamondburned Try the code in #3408, but defaulting the localhost string to *, and using this PR's code if *, or using the code currently in lemmy if anything else.

[diamondburned]

@diamondburned Try the code in #3408, but defaulting the localhost string to *, and using this PR's code if *, or using the code currently in lemmy if anything else.

This PR's intention isn't to add a new behavior into LEMMY_CORS_ORIGIN. It's intended to change the default behavior to allow all CORS, as per issue #3109.

Edit: Nevermind, I see that that PR changes the default in the Docker env. I think it's best done in code, though.

It's also worth noting that the behavior of allow_any_origin makes the server echo back CORS headers that respect the request's Origin header, as opposed to send_wildcard which sends back * for the CORS headers. I don't think this really matters, but it's something to keep in mind.

This PR would break the existing ability to set a specific hostname, which breaks my use-case of only allowing my site access (I do not federate).

LEMMY_CORS_ORIGIN is now restored. You can set LEMMY_CORS_ORIGIN=https://example.com to restore the old behavior.

[aeharding]

Hi there! Could this please be merged in? This PR not being merged has been causing me a lot of stress with wefwef and lemmy.world in production the last few days.

[ayan4m1]

@auouymous Yes, I'm aware, it doesn't matter, I don't want to allow other clients at this time.

[diamondburned]

@auouymous Yes, I'm aware, it doesn't matter, I don't want to allow other clients at this time.

That'll work for blocking off web clients. You'll have a hard time blocking off regular app clients.

[auouymous]

There is one issue that needs to be addressed before this can be merged. LemmyNet/lemmy-ui#1713 added samesite=strict to the cookie but that will only be set when the cookie is modified, and the cookie has a one year expiration. The instance will need to forcefully logout or modify the cookie, otherwise all existing users will be vulnerable until they logout.

[diamondburned]

@auouymous Actually, that's not the case!

See:

Note: This does not lead to the browser leaking cookies. Calling fetch with credentials: "include" will cause the browser to error out because there's no Access-Control-Allow-Credentials, which is what we want:

await fetch("http://0.0.0.0:8536/api/v3/site", { credentials: "include" })
Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at http://0.0.0.0:8536/api/v3/site. (Reason: expected ‘true’ in CORS header ‘Access-Control-Allow-Credentials’).

Uncaught (in promise) TypeError: NetworkError when attempting to fetch resource.
await fetch("http://0.0.0.0:8536/api/v3/site")
Response { type: "cors", url: "http://0.0.0.0:8536/api/v3/site", redirected: false, status: 200, ok: true, statusText: "OK", headers: Headers(4), body: ReadableStream, bodyUsed: false }

The SameSite=strict change solely has to do with good conventions (omitting SameSite is not a good idea), but that change doesn't have much to do with this change.

Edit: to be clear, I'm confident about JS not leaking cookies. I think <iframe>s will still get this cookie, and some exploiting might be possible. I'll do some more testing.

Edit 2: I can only test this maybe after the 3rd, but I don't think there's any issue with this, even with a potential <iframe> exploit. As long as there's no destructive endpoint that can be invoked with a GET, then we should be fine. There isn't one in Lemmy, right..?

Edit 3: More evidence that this change isn't susceptible to <iframe> exploits: the server doesn't actually care about the cookie! It only cares about the auth field in the request. (side-note: having the token in GET URLs is a pretty bad idea as far as APIs go.)

[diamondburned]

CI seems to be failing because of an earlier commit upstream.

[Xyphyn]

+1 for this, I've been working on a web client and I have to make all requests on the server as of now meaning authentication will be kinda difficult.

[Zetaphor]

@diamondburned Try rerunning cargo +nightly fmt against the latest changes?

[diamondburned]

@diamondburned Try rerunning cargo +nightly fmt against the latest changes?

The formatting issue came from a separate PR and I don't want to fix that PR in my PR. Doing that makes it a bit harder to work on the codebase in the future.

I can make a separate PR to fix the issue separately, but I don't think it matters too much.

[dessalines]

Why did you remove the Cors::permissive debug mode?

[diamondburned]

Why did you remove the Cors::permissive debug mode?

The new change is effectively what permissive is, so I didn't see a point in keeping it.

The only reason why I didn't use it was because the documentation doesn't recommend it.

[kacperwyczawski]

Any updates on this? Lack of this header makes development of browser-based clients more difficult and expensive.

[Xyphyn]

I am facing major issues building a web client because of the strange CORS defaults, I'd like to see this merged soon.

[diamondburned]

@dessalines If it helps with the reviewing process, I can add back permissive debug mode.

[diamondburned]

CI seems to be failing with a "no space left on device" error:

   Compiling lemmy_api_crud v0.18.1-rc.9 (/woodpecker/src/github.com/LemmyNet/lemmy/crates/api_crud)
LLVM ERROR: IO failure on output stream: No space left on device
error: could not compile `lemmy_api_crud` (lib)

I don't think this is caused by my PR though.

[dessalines]

The new change is effectively what permissive is, so I didn't see a point in keeping it.

You removed the debug mode check tho, why? Just externalize the permissive config, and have it use that if lemmy is in debug mode, or that env var is missing.

[dessalines]

Put the debug mode check back in, and move the permissive config into a var or function somewhere that can be called in either case.

[diamondburned]

move the permissive config into a var or function somewhere that can be called in either case.

What do you mean by this? What should I need a function for? Shouldn't it just go:

• if debug
  • permissive
• else
  • if CORS variable
    • use default with set origin
  • else
    • add CORS allow headers

[dessalines]

Either that or:

• let cors_allowed_any_config = ...
• if debug
  • cors_allowed_any
• else
  • if CORS variable
    • use default with set origin
  • else
    • cors_allowed_any

Your PR still doesn't reflect that.

[auouymous]

@dessalines What about someone who wants to use LEMMY_CORS_ORIGIN while debugging?

[NatoBoram]

• Closes [Bug]: "Origin is not allowed to make this request" despite Access-Control-Allow-Origin: * #3481

[diamondburned]

@dessalines Added the debug_assertions check. The logic above can essentially be shortened down to if (debugging or no CORS variable) then permissive else restrictive.

[kacperwyczawski]

Finally! Thank you @diamondburned and other involved people :)

[Zetaphor]

• Closes [Bug]: "Origin is not allowed to make this request" despite Access-Control-Allow-Origin: * #3481

This also closes #3109