Skip to content

fix(handler): deny access when body.allowed is 'false' #94

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 20, 2021
Merged

fix(handler): deny access when body.allowed is 'false' #94

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e8962d
fix(handler): deny access when body.allowed is 'false'
jankapunkt Nov 29, 2021
5b5de24
fix(authorization): use simplified if-branch to check for body allow …
jankapunkt Dec 20, 2021
bcc59a9
Merge branch 'development' into fix-missing-body-allowed-check
jankapunkt Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/handlers/authorize-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,12 @@ AuthorizeHandler.prototype.handle = function(request, response) {
throw new InvalidArgumentError('Invalid argument: `response` must be an instance of Response');
}

if ('false' === request.query.allowed) {
const notAllowed = [
request.query.allowed,
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

return Promise.reject(new AccessDeniedError('Access denied: user denied access to application'));
}

Expand Down
18 changes: 18 additions & 0 deletions test/integration/handlers/authorize-handler_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,24 @@ describe('AuthorizeHandler integration', function() {
});
});

it('should throw an error if `allowed` is `false` body', function() {
const model = {
getAccessToken: function() {},
getClient: function() {},
saveAuthorizationCode: function() {}
};
const handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model });
const request = new Request({ body: { allowed: 'false' }, headers: {}, method: {}, query: {} });
const response = new Response({ body: {}, headers: {} });

return handler.handle(request, response)
.then(should.fail)
.catch(function(e) {
e.should.be.an.instanceOf(AccessDeniedError);
e.message.should.equal('Access denied: user denied access to application');
});
});

it('should redirect to an error response if a non-oauth error is thrown', function() {
const model = {
getAccessToken: function() {
Expand Down