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

Allow cross-origin requests #3301

Closed
wants to merge 1 commit into from
Closed

Conversation

pfgithub
Copy link
Contributor

See: #3109

This enables cross-origin access to support external web clients without requiring a proxy. No oauth support yet, so you have to give clients your username and password unfortunately. This does not add any config option to disable cross-origin access, I don't think one is needed but if it's important I can add an environment variable to turn it off.

I haven't tested this yet because I'm on a phone

@mollthecoder
Copy link

I don't know the lemmy codebase, or how it works internally super well, but I do have a couple recommendations, if they make sense for lemmy.

  1. Test it. Really.
  2. Add an easy way to disable it for instances that don't want it.

@pfgithub
Copy link
Contributor Author

@mollthecoder I'll test it when I'm back at a computer. It may be wanted to disable cors on the /user/register endpoint (so a random website can't register a new account and use it to post from your IP without your consent) and eventually /user/login if oauth gets implemented.

I did not add a config option because I don't think it makes sense - why would an instance want to disable it? If an instance really doesn't want to support external clients, they'll have to add their own code to filter and block mobile apps as well. I believe it should be possible to block cross-origin requests by editing the nginx config, I can add an example of that somewhere.

@mollthecoder
Copy link

@mollthecoder I'll test it when I'm back at a computer. It may be wanted to disable cors on the /user/register endpoint (so a random website can't register a new account and use it to post from your IP without your consent) and eventually /user/login if oauth gets implemented.

I did not add a config option because I don't think it makes sense - why would an instance want to disable it? If an instance really doesn't want to support external clients, they'll have to add their own code to filter and block mobile apps as well. I believe it should be possible to block cross-origin requests by editing the nginx config, I can add an example of that somewhere.

Yeah, I'd agree that sites shouldn't be allowed to register accounts. If the user lacks an account but requires one for the service, the site can simply redirect to the instance's official sign-up page.
As for disabling CORS, even if it doesn't make a ton of sense, I think it should be easy to disable CORS as to give the instance admins a choice. Or, at least have an example to block CORS requests using the nginx config, like you said.

@Nutomic
Copy link
Member

Nutomic commented Jun 26, 2023

Couldnt this also be abused by other websites to make posts in the name of a user while he is logged in, or to request private data? Seems risky to change this and makes cors entirely useless. Although Im not familiar with cors so I might be wrong.

@diamondburned
Copy link
Contributor

Couldnt this also be abused by other websites to make posts in the name of a user while he is logged in, or to request private data? Seems risky to change this and makes cors entirely useless. Although Im not familiar with cors so I might be wrong.

I believe not. Doing so would require accessing the user's JWT, which should be stored in Lemmy's frontend local storage (not sure if this is actually the case).

It would allow other websites to make requests to Lemmy on the user's browser, but it would still need the identifying token to actually act on their behalf. Disabling CORS should not affect this.

I could be wrong, though. I'm no CORS expert and have a hard time with it myself. But without this change, my web client cannot work with Lemmy instances running v0.18 and later.

@Nutomic
Copy link
Member

Nutomic commented Jun 26, 2023

The jwt is stored in a cookie, so I believe it will be sent automatically with any request to the Lemmy domain (even if triggered by another site).

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just completely removes all CORs security. If you'd like that to be disabled, you can either:

  • Make a PR to add disabling cors security as a site setting.
  • Use a debug version a lemmy that already uses Cors::permissive

.allow_any_origin()
.allow_any_method()
.allow_any_header()
.max_age(3600);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the debug section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debug section isn't needed if cross-origin requests are always allowed - the only extra thing it enables is allowing credentials:true which lemmy-js-client doesn't use

@diamondburned
Copy link
Contributor

The jwt is stored in a cookie, so I believe it will be sent automatically with any request to the Lemmy domain (even if triggered by another site).

The server should set a Same-Site=Strict to guard against this. MDN's Set-Cookie page documents this.

I believe the default behavior (Lax) protects against this already, but it's probably not hard to just test it with a fetch(x, { credentials: "include" }).

This just completely removes all CORs security. If you'd like that to be disabled, you can either:

* Make a PR to add disabling cors security as a site setting.

* Use a debug version a lemmy that already uses `Cors::permissive`

I think the main problem with this is that realistically, not many instances will actually do that, since it requires awareness of the issue (CORS is disabled--web clients won't work) and where to go to disable it.

It simply doesn't make sense that Lemmy in particular doesn't work with web clients even though Mastodon does just fine.

@pfgithub pfgithub marked this pull request as draft June 26, 2023 19:59
@pfgithub
Copy link
Contributor Author

I'm marking this as a draft until I can test it properly and disable access to /auth/signup. I don't believe lemmy is vulnerable to csrf attacks right now, so allowing cors shouldn't have any security implications.

@diamondburned
Copy link
Contributor

diamondburned commented Jun 27, 2023

Quick heads up: Beehaw currently uses 0.17.4, and checking its jwt cookie reveals that its SameSite is actually None, which is super bad!

image

In order for this PR to be ready, I think it should also change the Set-Cookie code to include SameSite: Strict (and ideally also HttpOnly: true), otherwise the JWT token might actually leak.

Edit: I've also confirmed this with a v0.18.0 instance.

@diamondburned
Copy link
Contributor

It also might be best to leave the Access-Control-Allow-Credentials header out entirely. After all, third-party clients can just include the token in the request body within the $.auth JSON field, so I don't think it's at all necessary.

If we want to go ahead with allowing credentials, it's worth thinking about what would happen to existing cookies. Those might not get updated until the user accesses the main website, so they might have a chance of leaking(?).

@Nutomic
Copy link
Member

Nutomic commented Jun 27, 2023

FYI the cookie is being saved here so thats the place to fix it: https://github.com/LemmyNet/lemmy-ui/blob/773176084cd380541bbe041de39a4a572554d1a3/src/shared/services/UserService.ts#L36

@diamondburned
Copy link
Contributor

This isomorphic-cookie library seems very outdated. It doesn't allow adding a sameSite attribute...

@diamondburned
Copy link
Contributor

@Nutomic If lemmy-ui is using JS to store the JWT as a cookie, why is it not storing it in the local storage instead?

@diamondburned

This comment was marked as outdated.

@diamondburned
Copy link
Contributor

I've made PR #3421 that replaces this PR.

@pfgithub pfgithub closed this Jul 1, 2023
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.

None yet

5 participants