Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Fix minor DoS attack on long headers or uris. #171

Merged
merged 3 commits into from
Jan 21, 2016
Merged

Fix minor DoS attack on long headers or uris. #171

merged 3 commits into from
Jan 21, 2016

Conversation

remy
Copy link

@remy remy commented Jan 20, 2016

Replaces #170

This PR, which is really just this commit fixes the DoS attack that was merged into 4.x code, but applies it to the 3.x code.

This seemed important specifically because request relies on [email protected] and have said they're not ready to drop node < 4 support (which would come via hawk@4).

So this patch will offer a 3.1.x version that has the vuln fixed, which should allow request to update their dependencies.

This particular patch was generated for Snyk users, but we'd rather see users be able to do updates over patches in their remediation.

I hope this patch will be considered and merge (for release to 3.1.3). Thanks!

Related to #168

Supports the fix on the 3.x branch allowing for https://github.com/request/request to pick up the fix in 3.1.x

Fixes request/request#2020
@remy
Copy link
Author

remy commented Jan 20, 2016

Note: the linting errors were failing at the checkout point of 66dd8f9 - which is why the build is failing. I've only focused the changes on the vulnerability patch, and happy to take advice to get the build fully passing (or if you're happy to go without the linting checks?).

@hueniverse
Copy link
Contributor

You need to also port the tests.

@remy
Copy link
Author

remy commented Jan 20, 2016

By "port the tests" what exactly are you referring to? The failing lint that was inherited from the previous commit (which is why the test is failing) or something else?

@gergoerdosi
Copy link

The tests from this commit ("errors on uri too long", "errors on header too long", etc): 0833f99#diff-ad3c25167d0354b9b277e3ab6f375274R1000

@remy
Copy link
Author

remy commented Jan 20, 2016

Perfect, will do.

@remy
Copy link
Author

remy commented Jan 21, 2016

Extra tests included and coverage back to 100% (also cleaned up lib/client.js to fix linting fails).

hueniverse added a commit that referenced this pull request Jan 21, 2016
Fix minor DoS attack on long headers or uris.
@hueniverse hueniverse merged commit bef99ae into mozilla:v3.1.x Jan 21, 2016
@hueniverse hueniverse added bug Bug or defect security Issue with security impact labels Jan 21, 2016
@hueniverse hueniverse self-assigned this Jan 21, 2016
@hueniverse hueniverse added this to the 3.1.3 milestone Jan 21, 2016
@landau
Copy link

landau commented Jan 21, 2016

Thanks for the work, @remy @hueniverse !

jakubpawlowicz added a commit to altmetric/bugsnag-node that referenced this pull request Jan 22, 2016
The newest version of `request` uses patched versions of `hawk` and
`is-my-json-valid`, recently highlighted for DoS vulnerabilities.

Since `request` 2.53 uses `hawk` 2.x, which wasn't patched, the only
way to fix this vulnerability is to update to the most recent `request`
version.

See: mozilla/hawk#171

This is generally a good practice to use semver dependency versioning.
jakubpawlowicz added a commit to altmetric/bugsnag-node that referenced this pull request Jan 22, 2016
The newest version of `request` uses patched versions of `hawk` and
`is-my-json-valid`, recently highlighted for DoS vulnerabilities.

Since `request` 2.53 uses `hawk` 2.x, which wasn't patched, the only
way to fix this vulnerability is to update to the most recent `request`
version.

See: mozilla/hawk#171

This is generally a good practice to use semver dependency versioning.
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bug or defect security Issue with security impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants