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

Add more SASL validation and fix tests #2436

Merged
merged 11 commits into from
Dec 30, 2020

Conversation

sehrope
Copy link
Contributor

@sehrope sehrope commented Dec 29, 2020

Bunch of commits to clean up some of the SASL internals, add validation for function args, validation for data types of server responses, and some regex validation as well (e.g. base64 server responses should be valid base64). They're broken up into separate commits to make it easier to review and the final file is a bit easier to digest too.

Also removes the global asssert.throws(...) test helper as it was overwriting the real one in the built-in asserts module. The removed one did not actually check the error signature so it would report success for unrelated errors as long as the code block threw something (which was kind of confusing while testing this out...).

The only thing using that extra validation of the error signature was the SASL tests so it shouldn't break anything and all the tests pass with that change. The SASL tests have also been updated to add in some missing parameters. Previously they weren't supplying all the args needed to actually perform the tests so they'd have early or unrelated failures.

Comment on lines -114 to -123
assert.throws = function (offender) {
try {
offender()
} catch (e) {
assert.ok(e instanceof Error, 'Expected ' + offender + ' to throw instances of Error')
return
}
assert.ok(false, 'Expected ' + offender + ' to throw exception')
}

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, good call. Monkey-patching assert was such a bad idea I had such a long time ago. The use of globals throughout some of the tests is also gross....need to fix up one day soonish. 🤦

Copy link
Owner

@brianc brianc left a comment

Choose a reason for hiding this comment

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

Thank you so much for this! Most of the original SASL code was contributed by not me. This is v helpful. As far as merging goes, I typically do a squash merge to keep a somewhat "one commit per PR" flow on master. (I have accidentally or for some times on purpose reasons used merge commits in the past though...I'm not super consistent.) Do you have a preference? I appreciate all the separate commits on this PR to make reviewing easier & don't want to rob you of that work if you want them all to land on master individually. It can make pin-pointing a regression in the future easier and can make the job of people who want to see what changed between versions a bit easier though if its a single squash-merge so they can kinda go "github commit page -> link to each PR which landed the change".

@sehrope
Copy link
Contributor Author

sehrope commented Dec 29, 2020

My pleasure and thanks as always for this awesome lib.

Yes I like the separate commits mainly to simplify reviewing too. Especially for the incremental ones like function renames as it let you focus on the real changes in other commits. Plus it lets me know along the way what (I) broke.

No preference on how it gets merged though. FYI, the additional validations break the (broken) SASL tests so it's really only a full unit as a whole anyway.

Copy link
Collaborator

@charmander charmander left a comment

Choose a reason for hiding this comment

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

🎉

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 this pull request may close these issues.

3 participants