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

fix: validate encode input and ensure zero-filled Buffer #25

Merged
merged 3 commits into from
May 15, 2018
Merged

fix: validate encode input and ensure zero-filled Buffer #25

merged 3 commits into from
May 15, 2018

Conversation

MeirionHughes
Copy link
Contributor

closes #24
requires: node ^4

@MeirionHughes MeirionHughes changed the title fix: used zero-filled Buffer allocation fix: validate encode input and ensure zero-filled Buffer May 15, 2018
@@ -10,7 +10,7 @@ export default function padString(input: string): string {
let position = stringLength;
let padLength = segmentLength - diff;
let paddedStringLength = stringLength + padLength;
let buffer = new Buffer(paddedStringLength);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

depreciated since node 4. Warning in Node 10. likely unsupported in Node 11

@MeirionHughes
Copy link
Contributor Author

Alternatively, drop node 4 support and add engine >=6 - new Buffer(1000, encoding) will natively throw an error.

Copy link
Collaborator

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

Closing #26 as a dupe

You are likely going to have to update lines 7 & 46 in test/base64url.test.js

I would also suggest adding 8 / 10 to the travis yaml

@MylesBorins
Copy link
Collaborator

@brianloveswords dropping support for 0.10 and 0.12 is arguably semver-major. Node.js does have a history of landing Semver-Major changes as Semver-Minor for security updates

Copy link
Collaborator

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM if travis.yml is updated and test is updated as mentioned in above comment

@brianloveswords brianloveswords merged commit 4fbd954 into brianloveswords:master May 15, 2018
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.

Security Vulnerability found on base64url
3 participants