Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Commit

Permalink
feat(angular): added owasp configuration for frontend
Browse files Browse the repository at this point in the history 
Added configuration for owasp. Synchronize client owap configs with t…
  • Loading branch information
@lirantal
lirantal authored Sep 12, 2016
2 parents 67d1a5a + 0588eab commit 4f3a501
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 5 deletions.
10 changes: 10 additions & 0 deletions config/env/default.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,15 @@ module.exports = {
fileSize: 1 * 1024 * 1024 // Max file size in bytes (1 MB)
}
}
},
shared: {
owasp: {
allowPassphrases: true,
maxLength: 128,
minLength: 10,
minPhraseLength: 20,
minOptionalTestsToPass: 4
}
}

};
8 changes: 5 additions & 3 deletions modules/core/server/controllers/core.server.controller.js
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
'use strict';

var validator = require('validator');
var validator = require('validator'),
path = require('path'),
config = require(path.resolve('./config/config'));

/**
* Render the main application page
*/
exports.renderIndex = function (req, res) {

var safeUserObject = null;
if (req.user) {
safeUserObject = {
Expand All @@ -24,7 +25,8 @@ exports.renderIndex = function (req, res) {
}

res.render('modules/core/server/views/index', {
user: JSON.stringify(safeUserObject)
user: JSON.stringify(safeUserObject),
sharedConfig: JSON.stringify(config.shared)
});
};

Expand Down
6 changes: 6 additions & 0 deletions modules/core/server/views/layout.server.view.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@

<!--Application JavaScript Files-->
{{#each jsFiles}}<script type="text/javascript" src="{{this}}"></script>{{/each}}

<!--owasp config sync-->
<script type="text/javascript">
var sharedConfig = {{{ sharedConfig }}};
owaspPasswordStrengthTest.config(sharedConfig.owasp);
</script>

{{#if livereload}}
<!--Livereload script rendered -->
Expand Down
2 changes: 1 addition & 1 deletion modules/users/client/services/password-validator.client.service.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
}

function getPopoverMsg() {
var popoverMsg = 'Please enter a passphrase or password with 10 or more characters, numbers, lowercase, uppercase, and special characters.';
var popoverMsg = 'Please enter a passphrase or password with ' + owaspPasswordStrengthTest.configs.minLength + ' or more characters, numbers, lowercase, uppercase, and special characters.';

return popoverMsg;
}
Expand Down
2 changes: 1 addition & 1 deletion modules/users/server/config/strategies/local.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ module.exports = function () {
}
if (!user || !user.authenticate(password)) {
return done(null, false, {
message: 'Invalid username or password'
message: 'Invalid username or password (' + (new Date()).toLocaleTimeString() + ')'

This comment has been minimized.

Sign in to view

Copy link
@mleanos

mleanos Sep 12, 2016

Member

Is there really a case to be made for having this? It just seems off to me. If we really needed a date to be passed back from this error handler it would probably be better to do:

return done(null, false, {
  message: 'Invalid username or password',
  localTimeOccured: (new Date()).toLocaleTimeString()
});
});
}

Expand Down
5 changes: 5 additions & 0 deletions modules/users/server/models/user.server.model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,17 @@
* Module dependencies
*/
var mongoose = require('mongoose'),
path = require('path'),
config = require(path.resolve('./config/config')),
Schema = mongoose.Schema,
crypto = require('crypto'),
validator = require('validator'),
generatePassword = require('generate-password'),
owasp = require('owasp-password-strength-test');

owasp.config(config.shared.owasp);


/**
* A Validation function for local strategy properties
*/
Expand Down

10 comments on commit 4f3a501

@mleanos
Copy link
Member

Choose a reason for hiding this comment

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

@wansco I really like that you re-organized the config into a "shared" section. This seems like a really helpful way of avoiding accidentally exposing sensitive server-side configurations. Thank you!!

@hyperreality
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't get minOptionalTestsToPass config to work out the box, you have to validate using a property on the OWASP result object called 'strong' instead of 'error'... which then means the password requirement progress bar won't be accurate.

@hyperreality
Copy link
Contributor

@hyperreality hyperreality commented on 4f3a501 Sep 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

In other words, this PR is incomplete. password-validator.client.directive.js needs to be modified too, because that directive is built on the assumption that there will be 4 minimum optional tests and reads from OWASP's errors object which contains all optional test errors by default.

I just had a go at modifying the directive to make it work in all cases, but there's no way round the fact that you're going to get "Your password must contain..." errors when it the password might not need to contain said characters, and the modifications in general overcomplicated. I tried randomly selecting optional test errors to send the user but at that point decided you may as well be editing the original config files.

@wansco
Copy link
Contributor

@wansco wansco commented on 4f3a501 Sep 12, 2016

Choose a reason for hiding this comment

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

Good catch. owasp test result has "requiredTestErrors" and "optionalTestErrors" in addition to just the "errors" which are displayed as well as a count of optionalTestsPassed.

We could split them up to display the error text as following:
requiredTestErrors --- this is pretty much just the length test, I believe
[if optionalTestErrors]
At least [minOptionalTestsToPass - optionalTestsPassed] of the following
optionalTestErrors

I'll work on a real code example for that.

The progress bar item count should probably be 1+minOptionalTestsToPass and dynamically generated. I'll work through that as well.

I think the popover message is fine (in password-validator.client.service.js), but could be tweaked from:
var popoverMsg = 'Please enter a passphrase or password with ' + owaspPasswordStrengthTest.configs.minLength + ' or more characters, numbers, lowercase, uppercase, and special characters.';
to:
var popoverMsg = 'Please enter a passphrase or password with ' + owaspPasswordStrengthTest.configs.minLength + ' or more characters.\nAnd at least ' + owaspPasswordStrengthTest.configs.minOptionalTestsToPass + ' of the following: numbers, lowercase, uppercase, and special characters.';

@hyperreality
Copy link
Contributor

Choose a reason for hiding this comment

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

@wansco Great stuff, I'm keen to review it when it's done.

@wansco
Copy link
Contributor

@wansco wansco commented on 4f3a501 Sep 13, 2016

Choose a reason for hiding this comment

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

I made some of the above changes and pushed it to my repo, but it looks like the pull request was already accepted.

I think I resolved the popover message and the error messages, but am having a tough time with the progress bar. The password length is required, but the optional tests can range from 0-4 (or possibly more if the user extends the owasp tests... but I dont think we need to consider that in this scope)

If its zero optional tests, there's no point in having a progress bar. I'd argue that its really only needed if there are at minimum 2 optional tests (for 3 total tests to pass).

How would you feel about hardcoding the 3 cases of progressbar configurations (for 3, 4, & 5 total tests) and hiding the progressbar when there are fewer than 3 total tests to pass?

@hyperreality
Copy link
Contributor

@hyperreality hyperreality commented on 4f3a501 Sep 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Note that 4 - number of optional tests errors gives you how many optional tests you passed. But this number should not be allowed to exceed the minimum number of allowed optional tests. So use a ternary operator to kick it down if it exceeds that number. And add this number to required tests passed.

Then make this a percentage over the total number of tests needed (number of required tests plus minimum optional tests).

You would lose the pretty colours of the current progress bar (could make it red unless 100% then green), but it should work.

Perhaps even the number 4 shouldn't be hardcoded, get the quantity of optional rules from the result object too if you can.

@wansco
Copy link
Contributor

@wansco wansco commented on 4f3a501 Sep 13, 2016

Choose a reason for hiding this comment

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

owasp has a good amount of info in the return --- result.optionalTestsPassed, but for the sake of progress bar values, it should be capped.

For the required test, it has the min length, max length and repeated characters (that one is not in the popover message). I'm going to treat this as a single pass/fail: if there are no required test failures, then it gets a tick on the progress bar. That plus the MIN(optionalTestsPassed, minOptionalTestsToPass) should fill out the progress bar. I can keep the fancy colors if I define the progress bar for 3 tests (the required "one" plus 2 optional), 4 tests (required + 3), and 5 (required + 5). That is getting a bit excessive in the code, but logic to dynamically define these with colors is getting overly complex.

@hyperreality
Copy link
Contributor

Choose a reason for hiding this comment

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

But what if people want 0 or 1 optional tests, or add more of their own? I think you should make the progress bar dynamically adjustable along the lines I suggested. In terms of colours, a two-colour scheme for failure and success is just as good IMO and then you don't have to hardcode anything.

@wansco
Copy link
Contributor

@wansco wansco commented on 4f3a501 Sep 13, 2016

Choose a reason for hiding this comment

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

0 or 1 optional tests probably do not need a progress bar, so we can just hide it in signup.client.view.html (the error messages should handle user notification for that case). I do like the colors, so I duplicated the progress bar array, but we could eliminate that if its too verbose. I also provided the option if those 3 ranges are not defined, to fallback on the percentage dual color scheme in password-validator.client.directive.js. I think far more people will stick with the 2-4 optional tests rather than add more, but this "should" handle all of those cases.

I made these changes on my repo:
https://github.com/meanjs/mean/compare/master...wansco:master?expand=1

I'll be out of town for the next week or so, so I probably wont get to creating a new PR anytime soon. I could probably clean up password-validator.client.directive.js considerably and run this through a bunch more tests.

I also broke the existing tests by injecting $window in password-validator.client.directive.js (to get access to the sharedConfig object). I need to figure that one out.

Please sign in to comment.