search

soofstad / react-oauth2-pkce

Provider agnostic OAuth2 Authorization Code flow with PKCE for React
MIT License
125 stars 53 forks source link

Fetch with credentials in postWithXForm() #188

Closed MichaelRoosz closed 1 month ago

MichaelRoosz commented 1 month ago

What does this pull request change?

It enables credentials when doing fetch requests. Specifically it enables TLS client certificate authentication for OPTIONS requests in Firefox. This fixes 403/forbidden errors, that would happen otherwise, since the request would be done without client certificate authentication and thus fail with 403 when using Firefox.

Why is this pull request needed?

When the server is enforcing client certificate authentication, we must set the fetch option "credentials" to "include", to ensure all browsers are allowing client certificate authentication. For example, this is required in Firefox for "OPTIONS" requests.

Issues related to this change

This will require the server to respond with proper CORS response headers, but this should already be the case (for example when using Keycloak). If you prefer to make this configurable, I am willing to implement this, just let me know how you would like to implement it.

soofstad commented 1 month ago

Hi @MichaelRoosz Thank you for the pull-request. I like this addition, but it should definitely be a opt-in on a per-request basis. Setting this as default for all request is a serious security problem. If you update the PR with this change, and fix up the tests, I'll be happy to merge it :slightly_smiling_face:

MichaelRoosz commented 1 month ago

Hello @soofstad, thank you for your feedback. I have updated the pr and made sure all tests are passing.

MichaelRoosz commented 1 month ago

I updated it, now also includes a test.

MichaelRoosz commented 1 month ago

The linting check should now also be fixed.

MichaelRoosz commented 1 month ago

Hello @soofstad, thanks a lot for your feedback, I have updated the pull-request accordingly. I have set the default to 'same-origin', since this is the default behaviour when the 'credentials' property is not set (as documented here https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch)

MichaelRoosz commented 1 month ago

thank you @soofstad , I have updated the pr 😃