-
Notifications
You must be signed in to change notification settings - Fork 201
fix: wopi context token check #11276
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
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
bf5e3cf
to
6737379
Compare
4182c57
to
90998f8
Compare
Ad. non-canonical header, the headers were not changed and are the same as in MS 365 doc: |
_, _, err = cs3JWTparser.ParseUnverified(wopiContext.AccessToken, cs3Claims) | ||
_, err = jwt.ParseWithClaims(wopiContext.AccessToken, cs3Claims, func(token *jwt.Token) (interface{}, error) { | ||
|
||
if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking into account that we're using SigningMethodHS256
everywhere, I'm not sure if we want to be more strict with the check or we're fine with just a comment to specify that we expect HS256 (while allowing other sizes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Updated the code to expect specifically HS256.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you've forgotten to update this code block 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks!
cf6dfaa
to
f2b406e
Compare
|
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: