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

To "be safe by default", overwriting values should throw an error #11

Open
markstos opened this issue Apr 1, 2021 · 1 comment · May be fixed by #13
Open

To "be safe by default", overwriting values should throw an error #11

markstos opened this issue Apr 1, 2021 · 1 comment · May be fixed by #13

Comments

@markstos
Copy link

markstos commented Apr 1, 2021

A large project may have several routes files with auth.action calls in several places.

Currently if auth.action('edit user') is defined in more than one place-- perhaps by accident, the new definition will overwrite the old definition.

This relates to #10 -- the perhaps-surprising singleton behavior.

Consider the following code:

const auth = require('authorized');

auth.role('admin', function(req, done) {
  done(null, req.user && req.user.admin);
});

auth.role('user', function(req, done) {
  done(null, req.user);
});

auth.action('edit user', ['admin']);
// This should throw an error
auth.action('edit user', ['admin','user']);

console.log('auth', auth )

Here the edit user action is defined twice. console.log shows the second, weaker, definition of the action has silently overwritten the first.

If two such statements appeared far apart in a project this could go unnoticed by peer review and might even pass unit testing if the two route files were tested in isolation and not when the second definition is loaded after the first.

I recommend a breaking change where an error is thrown when an attempting to overwrite a definition.

Optionally, a new method could be added like auth.replaceAction if it seems necessary to support the case where people for some reason really do want to replace the action definition. You could try a release without such a method and see if anyone actually needs it.

@markstos
Copy link
Author

markstos commented Apr 1, 2021

To be clear, I recommend preventing overwriting definitions for all of action, role and entity.

Any of these could lead to a hard to spot security vulnerability when the a weaker definition replaces a stricter one perhaps defined out of sight somewhere else.

markstos added a commit to RideAmigosCorp/authorized that referenced this issue Apr 2, 2021
… entities.

This prevents accidentally overwriting authorization rules with weaker
rules. Such an accident becomes more likely as apps grow in size and
could lead to an security vulnerability.

Fixes tschaub#11
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.

1 participant