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

No RexExp parser #100

Merged
merged 6 commits into from Feb 24, 2018
Merged

No RexExp parser #100

merged 6 commits into from Feb 24, 2018

Conversation

stash-sfdc
Copy link
Contributor

@stash-sfdc stash-sfdc commented Sep 26, 2017

I have nothing against RegExps in general, but for security conscious code, there's too many pitfalls.

This pull-request is about converting from using mostly RegExp to using mostly native JavaScript code. The main cookie parser and the RFC-compliant date parser are most affected.

When tough-cookie was originally written, the RegExps were used because it seemed easier to maintain. This pull request certainly increases the code complexity, but does so in the hopes that maintaining the RegExps would have been more time-consuming in the long run.

As a bonus, it seems that the pure JavaScript version is about 1 to 2% faster than the RegExp one, based on the time it takes to run the entire tough-cookie test suite (7.0s versus 6.9s now).

stash-sfdc added 6 commits Sep 21, 2017
Specifically to avoid any more hidden ReDoS in those regexes.

Seems to run tests in 6.9s vs 7.0s so might be a bit of a speed bonus on
some platforms!
Replaces a bunch of `[^\d]*$` with bounded `(?:[^\d]|$)`

Double checked the RFC6265 spec: time cannot have non-digits beside the
colons.
None of the regexps (at least, when they were removed) are vulnerable to
ReDoS. However, took this opportunity to check that the RFC is being
closer and more clearly documented where in the code.

Another way to put this: "regexps are magic and hinder code analysis"

Introduced some equivalence tests to ensure that certain "weird" dates
are indeed parsing the same as their "canonical" RFC6265 counterpart.
@awaterma awaterma self-requested a review Dec 18, 2017
@awaterma awaterma self-assigned this Dec 18, 2017
@stash stash added this to To Do in Roadmap via automation Jan 29, 2018
@stash stash removed this from To Do in Roadmap Jan 29, 2018
@stash stash merged commit 7564c06 into master Feb 24, 2018
3 checks passed
@stash
Copy link
Collaborator

@stash stash commented Feb 26, 2018

Integration tested request, jsdom, and zombie.

@awaterma
Copy link
Member

@awaterma awaterma commented Mar 5, 2018

Looks good. Thanks for merging.

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.

None yet

3 participants