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

allow typ in header to be optional. #29

Merged
merged 5 commits into from Dec 1, 2020

Conversation

devinstewart
Copy link
Member

Addresses #27

typ is optional based on https://tools.ietf.org/html/rfc7519#section-5.1

@devinstewart
Copy link
Member Author

ping @hapijs/tsc

Would like to get this in so that feature requests can be worked on.

Thanks as always

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a couple documentation nits.

API.md Outdated Show resolved Hide resolved
API.md Outdated Show resolved Hide resolved
devinstewart and others added 2 commits November 28, 2020 14:10
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
@devinstewart
Copy link
Member Author

Thanks @cjihrig, pulled in the doc nits.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 29, 2020

Ping @hapijs/tsc for another set of eyes.

There also seems to be some issues with the CI.

@devinstewart
Copy link
Member Author

devinstewart commented Nov 29, 2020

Gave it the old college try. Error seems to be in the mega line in test/mock.js:

const certDer = Forge.util.encode64(Forge.asn1.toDer(Forge.pki.certificateToAsn1(Forge.pki.certificateFromPem(certPem))).getBytes());

It seems interesting to me that when breaking it out, what is happening here is that it the cert is being converted to a PEM and then back. Example code:

 const certToPem = Forge.pki.certificateToPem(cert);
 const pemToCert = Forge.pki.certificateFromPem(certToPem);
 const certDer = Forge.util.encode64(Forge.asn1.toDer(Forge.pki.certificateToAsn1(pemToCert)).getBytes());

However, when removing the back and forth with:

const certDer = Forge.util.encode64(Forge.asn1.toDer(Forge.pki.certificateToAsn1(cert)).getBytes());

I do get errors locally on 0-2 tests, each run is inconsistent. I feel if I/we can figure out what is going on there, we can perhaps remove the "flakiness" from these tests.

Just thought I would give my 2¢ in case anybody wanted to dive in.

@devinstewart
Copy link
Member Author

Wasn't crazy about the solution, but it ran all 18 successfully in my local repo 5 out of 5 times. Going to let other fresh eyes take a look. I can revert what I've done here if requested.

@lloydbenson
Copy link
Contributor

seems reasonable to me. I noticed this flakiness earlier but didn't get time to look into it since there was the bigger picture of getting it all moved over that took priority. Thanks for looking into this!

@lloydbenson lloydbenson self-requested a review November 30, 2020 22:27
Copy link
Member

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

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

LGTM also

@devinstewart devinstewart linked an issue Dec 1, 2020 that may be closed by this pull request
@devinstewart devinstewart self-assigned this Dec 1, 2020
@devinstewart devinstewart added this to the 2.0.2 milestone Dec 1, 2020
@devinstewart devinstewart merged commit 3559394 into hapijs:master Dec 1, 2020
@devinstewart devinstewart deleted the optional_jwt_in_header branch December 4, 2020 04:03
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.

Incorrectly rejects valid JWTs
4 participants