authorized icon indicating copy to clipboard operation
authorized copied to clipboard

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

Open markstos opened this issue 4 years ago • 1 comments
trafficstars

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 avatar Apr 01 '21 14:04 markstos

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 avatar Apr 01 '21 14:04 markstos