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

Authentication backend get_userinfo incorrectly assumes the endpoint responds with application/json #517

Open
sergei-maertens opened this issue Jan 10, 2024 · 7 comments · May be fixed by #521
Assignees

Comments

@sergei-maertens
Copy link

return user_response.json()

This line tries to decode the response content as JSON, which is one of the ways this endpoint may be implemented. The other way is that it responds with a JWT, having a Content-Type header of application/jwt;charset=utf-8 and that fails to decode as JSON, as the JWT itself needs to be processed.

Environment details:

  • OS: Arch Linux
  • Python: 3.10
  • Django: 3.2 LTS
  • mozilla-django-oidc: 3.0.0

How to reproduce:

  • Use an OIDC_OP_USER_ENDPOINT that responds with a JWT
  • Authenticate using OIDC
  • Observe JSONDecodeError crash
@sergei-maertens sergei-maertens changed the title Authentication backend get_userinfo incorrectly assumes the endpoint replies with application/json Authentication backend get_userinfo incorrectly assumes the endpoint responds with application/json Jan 10, 2024
@ttiikeri
Copy link

From an OIDC specification point of view, this a good point from @sergei-maertens. Under the 5.3.2 Successful UserInfo Response heading, here's what said about the application/jwt header:

If the UserInfo Response is signed and/or encrypted, then the Claims are returned in a JWT and the content-type MUST be application/jwt.

@sergei-maertens
Copy link
Author

@akatsoulas are you working on this? I've got an implementation in our downstream package that I'd prefer to see contributed back upstream, so I can draft a PR.

@ivanov17
Copy link

I'm having the same problem now.

There also seem to be a few more reports about this:

The first of them was published almost four years ago.

@akatsoulas, can we hope that this will be fixed soon?

@sergei-maertens
Copy link
Author

Awesome that you found these cross-references, I did search in the issue list but couldn't find anything.

Since I haven't heard back from @akatsoulas, I will just draft a PR with our implementation that can serve as a starting point, or at the least a way so that downstream packages can override the method in the backend to solve their own needs.

sergei-maertens added a commit to maykinmedia/mozilla-django-oidc that referenced this issue Feb 14, 2024
…ic utility

This allows us to re-use the verification functionality for
both the access token processing and userinfo response
data processing without duplicating the low-level
implementation.

Notable changes:

* the 'none' algorithm is blocked as this is a common
  exploit vector
* added some more documentation about the parameters
* added type hints to document expected parameter types
* did some slight refactoring/restructuring to make the
  code type-safe
sergei-maertens added a commit to maykinmedia/mozilla-django-oidc that referenced this issue Feb 14, 2024
@sergei-maertens sergei-maertens linked a pull request Feb 14, 2024 that will close this issue
@sergei-maertens
Copy link
Author

I have a draft PR #521 for this

@lebaudantoine
Copy link

Hey, @sergei-maertens are you still working on this? I have an implementation and would be super happy to offer it by the end of the week through a PR.

@sergei-maertens
Copy link
Author

Well I have the attached PR asking for some review/input, so I'd say this is blocked by maintainer availability rather than by me

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 a pull request may close this issue.

5 participants