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

Depandabot Alert: Bad documentation of error handling in ParseWithClaims can lead to potentially dangerous situations #652

Open
cantutar opened this issue Nov 8, 2024 · 2 comments

Comments

@cantutar
Copy link

cantutar commented Nov 8, 2024

[READ] Step 1: Are you in the right place?

  • For issues or feature requests related to the code in this repository
    file a GitHub issue.
    • If this is a feature request make sure the issue title starts with "FR:".
  • For general technical questions, post a question on StackOverflow
    with the firebase tag.
  • For general Firebase discussion, use the firebase-talk
    google group.
  • For help troubleshooting your application that does not fall under one
    of the above categories, reach out to the personalized
    Firebase support channel.

[REQUIRED] Step 2: Describe your environment

  • Operating System version: Windows 11 - go 1.23.0 - toolchain go1.23.2
  • Firebase SDK version: firebase.google.com/go/v4 v4.15.0
  • Library version: firebase.google.com/go/v4 v4.15.0
  • Firebase Product:

[REQUIRED] Step 3: Describe the problem

There is updated needed on version of dependecies caught by github depandabot
heres the desc:

Upgrade github.com/golang-jwt/jwt/v4 to version 4.5.1 or later. For example:

require github.com/golang-jwt/jwt/v4 v4.5.1

Package
github.com/golang-jwt/jwt/v4 (Go)

Affected versions
<= 4.5.0

Patched version
4.5.1

Summary

Unclear documentation of the error behavior in ParseWithClaims can lead to situation where users are potentially not checking errors in the way they should be. Especially, if a token is both expired and invalid, the errors returned by ParseWithClaims return both error codes. If users only check for the jwt.ErrTokenExpired using error.Is, they will ignore the embedded jwt.ErrTokenSignatureInvalid and thus potentially accept invalid tokens.

Fix

We have back-ported the error handling logic from the v5 branch to the v4 branch. In this logic, the ParseWithClaims function will immediately return in "dangerous" situations (e.g., an invalid signature), limiting the combined errors only to situations where the signature is valid, but further validation failed (e.g., if the signature is valid, but is expired AND has the wrong audience). This fix is part of the 4.5.1 release.

Workaround

We are aware that this changes the behaviour of an established function and is not 100 % backwards compatible, so updating to 4.5.1 might break your code. In case you cannot update to 4.5.0, please make sure that you are properly checking for all errors ("dangerous" ones first), so that you are not running in the case detailed above.

token, err := /* jwt.Parse or similar */
if token.Valid {
	fmt.Println("You look nice today")
} else if errors.Is(err, jwt.ErrTokenMalformed) {
	fmt.Println("That's not even a token")
} else if errors.Is(err, jwt.ErrTokenUnverifiable) {
	fmt.Println("We could not verify this token")
} else if errors.Is(err, jwt.ErrTokenSignatureInvalid) {
	fmt.Println("This token has an invalid signature")
} else if errors.Is(err, jwt.ErrTokenExpired) || errors.Is(err, jwt.ErrTokenNotValidYet) {
	// Token is either expired or not active yet
	fmt.Println("Timing is everything")
} else {
	fmt.Println("Couldn't handle this token:", err)
}

Steps to reproduce:

There is no new version of go sdk itself so, using latest version is enough.

Proof that comes from admin Sdk:

image

@google-oss-bot
Copy link
Collaborator

I found a few problems with this issue:

  • I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
  • This issue does not seem to follow the issue template. Make sure you provide all the required information.

@lahirumaramba
Copy link
Member

Thank you! Should be fixed in #651

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

No branches or pull requests

3 participants