Allow cross-origin requests #3421

diamondburned · 2023-06-30T05:22:01Z

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 · 2023-06-30T06:56:26Z

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 · 2023-06-30T07:04:32Z

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 · 2023-06-30T08:35:56Z

@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 · 2023-06-30T08:42:17Z

@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 · 2023-07-01T20:13:05Z

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 · 2023-07-02T01:27:52Z

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

diamondburned · 2023-07-02T02:20:47Z

@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 · 2023-07-02T02:50:48Z

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 · 2023-07-02T03:02:40Z

@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 · 2023-07-02T04:49:52Z

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

Xyphyn · 2023-07-02T15:08:48Z

+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 · 2023-07-02T17:51:25Z

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

diamondburned · 2023-07-02T18:50:52Z

@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?

src/lib.rs

diamondburned · 2023-07-04T00:52:59Z

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 · 2023-07-04T16:38:24Z

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

Xyphyn · 2023-07-04T20:26:23Z

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

diamondburned · 2023-07-04T20:44:39Z

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

diamondburned · 2023-07-05T07:48:38Z

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 · 2023-07-05T16:04:14Z

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 · 2023-07-05T18:06:39Z

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.

src/lib.rs

auouymous · 2023-07-05T23:07:59Z

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

NatoBoram · 2023-07-06T02:47:29Z

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

diamondburned · 2023-07-06T03:32:35Z

@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.

Co-authored-by: pfg <pfg@pfg.pw>

kacperwyczawski · 2023-07-06T11:57:46Z

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

Zetaphor · 2023-07-06T20:14:18Z

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

This also closes #3109

PR LemmyNet/lemmy#3421 is merged!

* Support bypassing the proxy for some Lemmy instances Since LemmyNet/lemmy#3421 was merged, Lemmy instances on 0.18.1 and newer allow cross-origin requests. * Fix image uploading not being possible cross origin (yet) --------- Co-authored-by: Alexander Harding <2166114+aeharding@users.noreply.github.com>

diamondburned requested review from Nutomic and dessalines as code owners June 30, 2023 05:22

This was referenced Jun 30, 2023

CORS support for external web clients? #3109

Closed

Allow cross-origin requests #3301

Closed

diamondburned force-pushed the pr-3301 branch 2 times, most recently from e1cd832 to 0f51468 Compare June 30, 2023 05:35

diamondburned force-pushed the pr-3301 branch from 0f51468 to f447b57 Compare June 30, 2023 08:40

diamondburned force-pushed the pr-3301 branch from f447b57 to 20971df Compare June 30, 2023 09:15

Zetaphor mentioned this pull request Jul 1, 2023

Re-add cors origin, default to * #3408

Closed

aeharding mentioned this pull request Jul 1, 2023

Securing self-hosted instances? aeharding/voyager#144

Closed

diamondburned force-pushed the pr-3301 branch from 33cab59 to c9bbfbe Compare July 2, 2023 03:11

Nutomic force-pushed the pr-3301 branch from c9bbfbe to 66b7924 Compare July 3, 2023 09:51

dessalines requested changes Jul 3, 2023

View reviewed changes

src/lib.rs Show resolved Hide resolved

Nutomic force-pushed the pr-3301 branch from 66b7924 to 0163d11 Compare July 4, 2023 11:14

diamondburned force-pushed the pr-3301 branch from 0163d11 to d16af95 Compare July 4, 2023 20:47

Nutomic force-pushed the pr-3301 branch from d16af95 to 8d11385 Compare July 5, 2023 11:27

dessalines requested changes Jul 5, 2023

View reviewed changes

geneccx mentioned this pull request Jul 5, 2023

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

Closed

4 tasks

diamondburned force-pushed the pr-3301 branch from 8d11385 to 257484f Compare July 5, 2023 22:03

dessalines requested changes Jul 5, 2023

View reviewed changes

dessalines reviewed Jul 5, 2023

View reviewed changes

src/lib.rs Show resolved Hide resolved

NatoBoram mentioned this pull request Jul 6, 2023

Static builds NatoBoram/Leanish#6

Closed

diamondburned force-pushed the pr-3301 branch from 257484f to 59ca0b2 Compare July 6, 2023 03:30

diamondburned force-pushed the pr-3301 branch from 59ca0b2 to 48d16cf Compare July 6, 2023 03:39

diamondburned requested a review from dessalines July 6, 2023 05:59

diamondburned force-pushed the pr-3301 branch 2 times, most recently from c24adce to 7032694 Compare July 6, 2023 07:03

Allow cross-origin requests

3086491

Co-authored-by: pfg <pfg@pfg.pw>

diamondburned force-pushed the pr-3301 branch from 7032694 to 3086491 Compare July 6, 2023 07:36

dessalines approved these changes Jul 6, 2023

View reviewed changes

dessalines merged commit 084f603 into LemmyNet:main Jul 6, 2023
1 check passed

diamondburned added a commit to diamondburned/slemmy that referenced this pull request Jul 7, 2023

Delete CORS notice in README

4a32df2

PR LemmyNet/lemmy#3421 is merged!

This was referenced Jul 8, 2023

Add UNPROXIED_LEMMY_SERVERS to configure bypassing the proxy aeharding/voyager#322

Closed

Support bypassing the proxy for some Lemmy instances aeharding/voyager#338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow cross-origin requests #3421

Allow cross-origin requests #3421

diamondburned commented Jun 30, 2023

ayan4m1 commented Jun 30, 2023

diamondburned commented Jun 30, 2023

auouymous commented Jun 30, 2023

diamondburned commented Jun 30, 2023 •

edited

aeharding commented Jul 1, 2023

ayan4m1 commented Jul 2, 2023

diamondburned commented Jul 2, 2023

auouymous commented Jul 2, 2023

diamondburned commented Jul 2, 2023 •

edited

diamondburned commented Jul 2, 2023

Xyphyn commented Jul 2, 2023

Zetaphor commented Jul 2, 2023

diamondburned commented Jul 2, 2023

dessalines left a comment

diamondburned commented Jul 4, 2023

kacperwyczawski commented Jul 4, 2023 •

edited

Xyphyn commented Jul 4, 2023

diamondburned commented Jul 4, 2023

diamondburned commented Jul 5, 2023

dessalines commented Jul 5, 2023

dessalines left a comment

diamondburned commented Jul 5, 2023

dessalines left a comment

auouymous commented Jul 5, 2023

NatoBoram commented Jul 6, 2023

diamondburned commented Jul 6, 2023

kacperwyczawski commented Jul 6, 2023

Zetaphor commented Jul 6, 2023

Allow cross-origin requests #3421

Allow cross-origin requests #3421

Conversation

diamondburned commented Jun 30, 2023

ayan4m1 commented Jun 30, 2023

diamondburned commented Jun 30, 2023

auouymous commented Jun 30, 2023

diamondburned commented Jun 30, 2023 • edited

aeharding commented Jul 1, 2023

ayan4m1 commented Jul 2, 2023

diamondburned commented Jul 2, 2023

auouymous commented Jul 2, 2023

diamondburned commented Jul 2, 2023 • edited

diamondburned commented Jul 2, 2023

Xyphyn commented Jul 2, 2023

Zetaphor commented Jul 2, 2023

diamondburned commented Jul 2, 2023

dessalines left a comment

Choose a reason for hiding this comment

diamondburned commented Jul 4, 2023

kacperwyczawski commented Jul 4, 2023 • edited

Xyphyn commented Jul 4, 2023

diamondburned commented Jul 4, 2023

diamondburned commented Jul 5, 2023

dessalines commented Jul 5, 2023

dessalines left a comment

Choose a reason for hiding this comment

diamondburned commented Jul 5, 2023

dessalines left a comment

Choose a reason for hiding this comment

auouymous commented Jul 5, 2023

NatoBoram commented Jul 6, 2023

diamondburned commented Jul 6, 2023

kacperwyczawski commented Jul 6, 2023

Zetaphor commented Jul 6, 2023

diamondburned commented Jun 30, 2023 •

edited

diamondburned commented Jul 2, 2023 •

edited

kacperwyczawski commented Jul 4, 2023 •

edited