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(handler): deny access when body.allowed is 'false' #94

Merged
merged 3 commits into from
Dec 20, 2021

Conversation

jankapunkt
Copy link
Member

Summary

Implements missing access check when body.allowed is 'false'

Linked issue(s)

oauthjs/node-oauth2-server#532
#89

Involved parts of the project

AuthorizeHandler

Added tests?

Integration tests added

OAuth2 standard

https://datatracker.ietf.org/doc/html/rfc6749.html#section-4.1.1

Reproduction

Clone, checkout branch, test

@jankapunkt jankapunkt added the compliance 📜 OAuth 2.0 standard compliance label Nov 29, 2021
@jankapunkt jankapunkt linked an issue Nov 29, 2021 that may be closed by this pull request
33 tasks
@jankapunkt jankapunkt mentioned this pull request Nov 29, 2021
33 tasks
@FStefanni
Copy link
Contributor

FStefanni commented Dec 5, 2021

Hi,

the pr seems a sure improvement.
What make me think, is the use of strings ('false') instead of booleans.
So I am not 100% sure this is correct w.r.t. types, and w.r.t. what could truly arrive.
Nevertheless, we could accept this pr, since it is an improvement, and in case catch possible
typing issues when switching to TypeScript.

Btw, I also checked the v5-dev (TypeScript) branch, and also there the check was with strings.

Regards.

jorenvandeweyer
jorenvandeweyer previously approved these changes Dec 5, 2021
@jorenvandeweyer
Copy link
Member

What make me think, is the use of strings ('false') instead of booleans.

The usage of strings is actually correct since all parameters are passed as a string, both in the query as in the body.

@jankapunkt
Copy link
Member Author

@HappyZombies @jwerre can you please second-check and merge if everything is okay?

@jankapunkt
Copy link
Member Author

@HappyZombies @jwerre please one of you need to 2nd check, since it would make no sense if I review my own PR ;-) If you also see no issues you can merge immediately

@jankapunkt
Copy link
Member Author

@Uzlopak would you mind taking a look at this?

request.body.allowed
].some(allowed => 'false' === allowed);

if (notAllowed) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah. It should be something like

if (request.query.allowed === 'false' || request.body.allowed === 'false')

Also why is there no check for request.body.allowed === false? Does formData not allow boolean values?

Also: is it safe? Doesn't OAUth2 server add the allowed url fragment when the authorization finished. So actually you would get a redirect which is a Get and not a post. Not that we accidently make it possible to bypass something? Seems unlikely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay then I would update it towards this:

const notAllowed = ['false', false].some(value => request.query.allowed === value || request.body.allowed === value)

Copy link
Member

Choose a reason for hiding this comment

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

FormData does indeed not allow booleans. They only allow strings and blobs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you like some()? Also query can not be a boolean value, as query has by definition only string values.

if (
    request.query.allowed === 'false' ||
    request.body.allowed === 'false' ||
    request.body.allowed === false
)

Is much easier to read and probably the most performant too.

Just wanted to also suggest, that we should actually check for the opposite. Like query.allowed !== 'true'. From security point of view an explicit allowed is better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jorenvandeweyer
UrlEncoded FormData, Thanks for looking up.

So it should be simply:

if (
    request.query.allowed === 'false' ||
    request.body.allowed === 'false'
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will do

@jankapunkt
Copy link
Member Author

Updated and merged development, please review again

@Uzlopak Uzlopak merged commit 4921a1c into development Dec 20, 2021
@Uzlopak Uzlopak deleted the fix-missing-body-allowed-check branch December 20, 2021 07:58
@jankapunkt jankapunkt removed a link to an issue Jan 13, 2022
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compliance 📜 OAuth 2.0 standard compliance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants