Skip to content

Don't assume secrets are Base64 encoded #2

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

Closed
wants to merge 1 commit into from
Closed

Don't assume secrets are Base64 encoded #2

wants to merge 1 commit into from

Conversation

celkins
Copy link

@celkins celkins commented Jun 14, 2014

The JOSE/JWT specs make no assumption that shared secrets are Base64 encoded. If a particular caller's secret needs to be decoded prior to verification, then it should be its responsibility to take care of that.

The JOSE/JWT specs make no assumption that shared secrets are Base64
encoded. If a particular caller's secret needs to be decoded prior to
verification, then it should be its responsibility to take care of
that.
@mgonto
Copy link
Contributor

mgonto commented Jun 14, 2014

Hey @celkins thanks for the PR!

I think you're right and we shouldn't assume that secrets are Base64 encoded.

What do you think about adding a Factory method JWTVerifier.withBase64Secret or something like that so that it's easier to actually create a verifier with an encoded one? Do you think you can add that so that even though we break backwards compatibility, we give users an easy way of changing their code? If so, I can then merge the PR.

Thanks again for the help!

@pose
Copy link
Contributor

pose commented Jun 15, 2014

Also, can you make sure that you add tests for both base64 (already done) and non-base64 secrets?

@pose
Copy link
Contributor

pose commented Jun 23, 2014

Hi @celkins,

How are you? Were you able to see our feedback? What do you think?

Thanks in advance,

@pose pose force-pushed the master branch 2 times, most recently from 020a9c3 to 16ccb61 Compare October 31, 2014 19:39
@mgonto mgonto closed this in 1e6f687 Dec 22, 2014
@mgonto
Copy link
Contributor

mgonto commented Dec 22, 2014

Hey there,

Thanks for the PR!

I've just implemented this with all tests corrected as well :).

Releasing soon a new version :).

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