Skip to content

drop in replace base64url due to a vulnerability #179

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

Merged
merged 1 commit into from
May 15, 2018
Merged

drop in replace base64url due to a vulnerability #179

merged 1 commit into from
May 15, 2018

Conversation

panva
Copy link
Contributor

@panva panva commented May 15, 2018

Hi @linuxwolf,

the base64url package has a found vulnerability currently reported by snyk and other tools. The report history shows attempts to contact the maintainer for a fix with no success since march 2017.

b64u is a drop-in replacement with the vulnerability fixed. It'd be great if you could fast-forward a patch release with this fix.

Also, there was a forgotten .only in the test suite.

@MeirionHughes
Copy link

maybe better to use base64-url as it has an engine requirement of 6+, which natively complains about new buffer(1000, encoding). b64u still supported node 4+ and has less downloads/stars.

@panva
Copy link
Contributor Author

panva commented May 15, 2018

b64u is not using new Buffer, so it's all good and has toBuffer which node-jose needs. Also support for lts/4 is needed

@MeirionHughes
Copy link

ah yes, I see; so many tabs open I must have been looking at something else. :D

@panva
Copy link
Contributor Author

panva commented May 15, 2018

Is there something in the build that makes it fail in lts/8?

@linuxwolf
Copy link
Member

linuxwolf commented May 15, 2018

Thanks @panva . Looking at the error log from Travis-CI, it appears that toBuffer isn't coming back as a function in the browser (Firefox release). Will have to do some investigation into why ...

@linuxwolf
Copy link
Member

Taking a quick look, it looks like webpack is importing it as an es6 module, with a default instead of the rest of the methods. The quick hack fix is:

var impl = require("b64u");
if (impl.default) {
    impl = impl.default;
}

Will have to determine what the better fix is long term, though.

@panva
Copy link
Contributor Author

panva commented May 15, 2018

That really looks weird. Isn’t there a webpack option for this?

@linuxwolf
Copy link
Member

@panva So far I can't find it.

It might involve having to add babel ... which is too much to include in this PR.

@linuxwolf
Copy link
Member

@panva Thanks very much!

I've added an issue (#180) to track getting rid of this default hack.

@linuxwolf linuxwolf merged commit e3aba8a into cisco:master May 15, 2018
@panva panva deleted the base64url-vuln branch May 15, 2018 18:13
@panva
Copy link
Contributor Author

panva commented May 15, 2018

Great thank you @linuxwolf, any chance for a patch release on the 0.11.x track? It would immidiately fix the dependent packages without requiring a 0.12 (to be released) update. (Semver below 1.0.0 doesnt pickup minor patches)

@linuxwolf
Copy link
Member

@panva Working that out today. It might be bending the semver promise, but not by much.

@panva
Copy link
Contributor Author

panva commented May 15, 2018

Applying this PR on top of the latest tag only would be great. No breaking then.

linuxwolf pushed a commit that referenced this pull request May 15, 2018
@linuxwolf
Copy link
Member

That's exactly what I'm doing now; I wasn't sure how far master had progressed from 0.11.0, but it's enough that a patch is safer.

Will push to npm today, but might still be a couple of hours from now.

linuxwolf added a commit that referenced this pull request May 15, 2018
* chore: replace base64url with b64u (#179)

* Release 0.11.1
@panva
Copy link
Contributor Author

panva commented May 15, 2018

[email protected] got released 🤦‍♂️

@linuxwolf
Copy link
Member

(I wonder if @vladikoff had anything to do with this ... 😛 )

Glad they updated; is it worth going back at some point in the future?

@panva
Copy link
Contributor Author

panva commented May 15, 2018

With the next patch I will, just so that my software uses more used/watched dependencies and with that ends up with cleaner dependency trees. For node-jose, your call.

@vladikoff
Copy link
Contributor

@linuxwolf I also had the same problem as mentioned in #179 (comment)

I'm leaning towards using [email protected] but up to you

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.

4 participants