Skip to content
This repository has been archived by the owner on May 21, 2022. It is now read-only.

Fix sec vuln with list of claims #426

Closed

Conversation

Waterdrips
Copy link
Contributor

@Waterdrips Waterdrips commented Sep 14, 2020

This PR adds a fix for #422

Tests were added for failing and passing states then the code was updated for the case in the JWT Spec that allows a list of "aud" as well as a single string "aud"

Signed-off-by: Alistair Hey [email protected]

@Waterdrips Waterdrips force-pushed the waterdrips-fix-security-vuln branch from 964bece to 63de7be Compare September 14, 2020 06:54
@Waterdrips Waterdrips force-pushed the waterdrips-fix-security-vuln branch from f8fbdb3 to 4ea2e3f Compare September 14, 2020 14:53
@@ -2,7 +2,7 @@ package jwt_test

import (
"fmt"
"github.com/dgrijalva/jwt-go"
"github.com/form3tech-oss/jwt-go"
Copy link

Choose a reason for hiding this comment

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

Why are this changes needed in example_test.go?
Functions should be named as Example - to have examples in godoc.
Also some external jwt-go is imported

if !ok {
strAud, ok := m["aud"].(string)
aud = append(aud, strAud)
if !ok {
Copy link

Choose a reason for hiding this comment

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

nit: if should be before aud = append(aud, strAud)

@Waterdrips
Copy link
Contributor Author

oh sorry - we decided to fork and fix in our company org and I left this PR open

@Waterdrips Waterdrips closed this Sep 15, 2020
@sev3ryn
Copy link

sev3ryn commented Sep 15, 2020

but why closing it? :)

I would be great to have vulnerability fix merged into upstream

@camin-mccluskey
Copy link

@Waterdrips Echoing @sev3ryn's comment - it would be ideal to have this fix live in the latest version. This is now categorised as a high severity vulnerability https://snyk.io/vuln/golang:github.com%2Fdgrijalva%2Fjwt-go and means we can no longer use the library in an enterprise environment

@Waterdrips
Copy link
Contributor Author

but why closing it? :)

I would be great to have vulnerability fix merged into upstream

theres an open PR addressing this from Match #385 which has not been addressed.

We have taken the decision to fix this on our oss fork rather than wait for some activity here.

@alexellis
Copy link

@dgrijalva if you have time to merge a fix like this, then we can all dump our forks to /dev/null. What are your thoughts?

@aleksandrzhiliaev
Copy link

Are we going to fix and bump the new version of the library?
It is still an issue and a lot of projects rely on it:
https://www.whitesourcesoftware.com/vulnerability-database/CVE-2020-26160

@brianmay
Copy link

See #286

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants