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

Upgrade golang-jwt to v4 #2699

Open
3 tasks done
inge4pres opened this issue Nov 8, 2024 · 6 comments
Open
3 tasks done

Upgrade golang-jwt to v4 #2699

inge4pres opened this issue Nov 8, 2024 · 6 comments

Comments

@inge4pres
Copy link

inge4pres commented Nov 8, 2024

Issue Description

The golang-jwt library imported in the middleware package suffers from a CVE.

A fix is present in v5 or v5 of the library, but upgrading to v5 changes the API.
An upgrade to v4.5.1 is enough to fix the vuln.

Checklist

  • Dependencies installed
  • No typos
  • Searched existing issues and docs

Expected behaviour

A SCA scan does not surface any vulnerabilities.

Actual behaviour

Vulnerabilty is flagged.

Version/commit

v4.12.0

@inge4pres inge4pres changed the title Upgrade golang-jwt to v5 Upgrade golang-jwt to v4 Nov 8, 2024
@aldas
Copy link
Contributor

aldas commented Nov 8, 2024

@vishr, @lammel maybe it is time to delete JWT middleware from core (this repo) and direct everyone to https://github.com/labstack/echo-jwt I really dislike the idea of major version bump for this.

I do not think upgrading JWT deps makes sense in this repo. It is a breaking change anyway. Upgrading to jwt to v4/v5 here is even worse as you might not notice the change until your casts to Token start panicing in handlers - (during runtime). At least removing the middleware would be impossible not do notice as your builds would fail after Echo version bump.

@aldas
Copy link
Contributor

aldas commented Nov 8, 2024

@inge4pres as a immediate remedy, please switch to using https://github.com/labstack/echo-jwt NB: make sure to create at lease one test in you app that uses jwt mw + you handler that checks for JWT Token.

This is because people often have user = c.Get("user").(*jwt.Token) or similar line in our handler to extract JWT Token from echo context and cast it to *jwt.Token. Now when Echo would silently upgrade to v4 or v5 in your handler go file, in imports you would still have github.com/golang-jwt/jwt but not github.com/golang-jwt/jwt/v5. That later one is actual version for jwt.Token type.

something like that

func TestMyHandlerWithJWTMW(t *testing.T) {
	e := echo.New()

	// replace this with https://github.com/labstack/echo-jwt
	e.Use(middleware.JWTWithConfig(middleware.JWTConfig{
		SigningKey: []byte("secret"),
	}))

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	req.Header.Set(echo.HeaderAuthorization, "bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWV9.TJVA95OrM7E2cBab30RMHrHDcEfxjoYZgeFONFh7HgQ")
	res := httptest.NewRecorder()

	// ADD HERE YOUR HANDLER TO ECHO to test
	// `user = c.Get("user").(*jwt.Token)` not panicing due different JWT version
	// That line would definetely panic if Echo would bump JWT version to v4/v5 but you did not upgrade JWT import in this handle .go file
	//e.GET("/", myHandler)

	e.ServeHTTP(res, req)

	assert.Equal(t, http.StatusOK, res.Code)
}

@lammel
Copy link
Contributor

lammel commented Nov 8, 2024

@vishr, @lammel maybe it is time to delete JWT middleware from core (this repo) and direct everyone to https://github.com/labstack/echo-jwt I really dislike the idea of major version bump for this.

I do not think upgrading JWT deps makes sense in this repo. It is a breaking change anyway. Upgrading to jwt to v4/v5 here is even worse as you might not notice the change until your casts to Token start panicing in handlers - (during runtime). At least removing the middleware would be impossible not do notice as your builds would fail after Echo version bump.

Guess this is the way to go. The echo core should require as view dependencies as possible to avoid backwards compatibility issues. Bumping versions for echo-jwt will be far easier.

@stevenwhitehead
Copy link
Contributor

@lammel @aldas made #2701 to remove the middleware instead

@inge4pres
Copy link
Author

Thanks for your inputs folks.
I'll go ahead and close #2700 .

@aldas
Copy link
Contributor

aldas commented Nov 25, 2024

for history sake. This is previous breaking change related to JWT #1946 and discussion about that #1940

List of related issues and PRs #2701 (comment)

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 a pull request may close this issue.

4 participants