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

Fix CORS when using credentials headers #58

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TimotheFCN
Copy link

What this does

The goal of this PR is to fix CORS issues when using credentials.
In my setup, I am using lucia to manage authentication.
The server being on different port than my main app, I needed to use CORS, but got issues with credentials.

My setup

My setup is very specific. It might be the cause of the issues I encountered but maybe others will too at some point.
I am using sveltekit for my webapp. rxdb-server is installed as a dependency meaning it is both built for the server and shipped to the client.
As I cannot simply route traffic to rxdb-server endpoints with sveltekit (still working on that, it would be really cool to have a specific "plug&play" sveltekit adapter), I am simply running the rxdb-server on a different port.
In my authHandler for rxdb-server, I use lucia to authenticate the client like this:
const sessionId = auth.readSessionCookie(requestHeaders.cookie);
if (sessionId) {
const session = await auth.validateSession(sessionId);
...
}

I am running the server in sveltekit "dev mode" (with Vite) on port 5173 as you will see in the errors I copied here.
I used the options { cors: 'http://localhost:5173' } both when declaring my server and my endpoint.

The errors

First I got the two following errors:

Access to fetch at 'http://localhost:6942/my-endpoint/0/pull?lwt=0&id=26&limit=100' from origin 'http://localhost:5173' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

Access to fetch at 'http://localhost:6942/my-endpoint/0/pullStream' from origin 'http://localhost:5173' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: The value of the 'Access-Control-Allow-Credentials' header in the response is '' which must be 'true' when the request's credentials mode is 'include'.

I added the option 'credentials: true' in the setCors function of adapter-express/index.ts but still got

Access to fetch at 'http://localhost:6942/my-endpoint/0/pullStream' from origin 'http://localhost:5173' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

For both /pullStream and /pull?lwt=0&id=26&limit=100

Leading me to simply bypass the setCors function when credentials are used to declare the CORS options server-wide and not from specific endpoints.

I then got the following error:

GET http://localhost:6942/my-endpoint/0/pull?lwt=0&id=26&limit=100 401 (Unauthorized)

Which was fixed by adding credentials: 'include' in each fetch requests in replication-server/index.ts

Conclusion

While this may not be the optimal way to achieve the expected results, it does indeed fix my issues and doesn't seems to have any impact on existing instances as all fields are optionals.
This does however means that premium server adapters (which I cannot access) must include a "enableCredentials()" method since i changed the RxServerAdapter type. This shouldn't be a big deal tho.

Feel free to critique this PR, I was not really familiar with CORS before this. My only goal is to get it working properly but if it can be done a better way I would love to see how.

@TimotheFCN
Copy link
Author

Just wanted to add: The first issues might simply be fixed by changing how the setCors function handles the different paths (some endpoints might just not be covered). I didn't explore this possibility.

@pubkey
Copy link
Owner

pubkey commented Apr 10, 2024

Hi @TimotheFCN
The changes look good. Can you add a test case for that to be sure it will not break in the future and it also works in other (future) adapter implementations?

@pubkey pubkey added test missing Please add a test case bug Something isn't working and removed bug Something isn't working labels Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test missing Please add a test case
Projects
None yet
2 participants