From 3f8c4473783424e1642d2991b896add973249d97 Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Fri, 17 Jul 2026 11:00:55 +0200 Subject: [PATCH] [client] Rename isValidAccessToken to reflect audience-only check (#6806) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Describe your changes `isValidAccessToken` only decodes the JWT payload and checks the audience claim, but its name suggested full token validation. Rename it to `validateTokenAudience` and document what it does: a client-side audience/shape check on a token just obtained from the IdP over TLS. Token authenticity is enforced server-side by the management server, which verifies the signature against the IdP's JWKS (`shared/auth/jwt/validator.go`) on every request. Also harden the parser: a non-empty token lacking the three-part JWT structure caused an index-out-of-range panic (`strings.Split(token, ".")[1]`); the shape is now validated first. `parseEmailFromIDToken` is documented as best-effort UX data (login hint/display), never used for authorization. Added tests for audience matching, malformed tokens, and the panic regression. Changes: - Rename `isValidAccessToken` → `validateTokenAudience`; document that it does not verify the signature and that authenticity is enforced server-side. - Fix an index-out-of-range panic on a non-empty token lacking JWT structure (`strings.Split(token, ".")[1]`) by validating the three-part shape first. - Document `parseEmailFromIDToken` as best-effort/unverified, used only for the login-hint/display UX, never for an authorization decision. - Add `util_test.go` covering audience matching (string and array), missing audience, malformed payloads, and the panic regression. ## Issue ticket number and link Internal cleanup: rename a misleadingly-named client-side helper and harden JWT parsing against malformed input (`client/internal/auth/util.go`). ## Stack - `0.74.7-branch` - :warning: No PR associated with branch - \#6806 :point\_left: ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal client-side helper rename plus a panic hardening fix. No public API, gRPC, CLI/service flag, or configuration change; token authenticity enforcement (server-side JWKS verification) is unchanged. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from here: N/A *** View with Codesmith Autofix with Codesmith Need help on this PR? Tag /codesmith with what you need. Autofix is disabled. ## Summary by CodeRabbit - **Bug Fixes** - Improved access-token audience validation during device and PKCE authentication flows. - Malformed tokens now return clear validation errors instead of risking runtime failures. - Added support for validating both string and array audience claims. - **Tests** - Added coverage for malformed tokens, invalid claims, missing audiences, and panic prevention. --- client/internal/auth/device_flow.go | 2 +- client/internal/auth/pkce_flow.go | 7 +- client/internal/auth/util.go | 20 ++++-- client/internal/auth/util_test.go | 108 ++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 6 deletions(-) create mode 100644 client/internal/auth/util_test.go diff --git a/client/internal/auth/device_flow.go b/client/internal/auth/device_flow.go index e33765300..cf8b7a1f8 100644 --- a/client/internal/auth/device_flow.go +++ b/client/internal/auth/device_flow.go @@ -291,7 +291,7 @@ func (d *DeviceAuthorizationFlow) WaitToken(ctx context.Context, info AuthFlowIn UseIDToken: d.providerConfig.UseIDToken, } - err = isValidAccessToken(tokenInfo.GetTokenToUse(), d.providerConfig.Audience) + err = validateTokenAudience(tokenInfo.GetTokenToUse(), d.providerConfig.Audience) if err != nil { return TokenInfo{}, fmt.Errorf("validate access token failed with error: %v", err) } diff --git a/client/internal/auth/pkce_flow.go b/client/internal/auth/pkce_flow.go index 84fa8a214..91d6733ea 100644 --- a/client/internal/auth/pkce_flow.go +++ b/client/internal/auth/pkce_flow.go @@ -296,7 +296,7 @@ func (p *PKCEAuthorizationFlow) parseOAuthToken(token *oauth2.Token) (TokenInfo, audience = p.providerConfig.ClientID } - if err := isValidAccessToken(tokenInfo.GetTokenToUse(), audience); err != nil { + if err := validateTokenAudience(tokenInfo.GetTokenToUse(), audience); err != nil { return TokenInfo{}, fmt.Errorf("authentication failed: invalid access token - %w", err) } @@ -310,6 +310,11 @@ func (p *PKCEAuthorizationFlow) parseOAuthToken(token *oauth2.Token) (TokenInfo, return tokenInfo, nil } +// parseEmailFromIDToken extracts the email (or name) claim from an ID token +// without verifying its signature. The value is best-effort and used only as a +// UX convenience (login hint prefill and display); it never drives an +// authorization decision. The authoritative identity is established server-side +// from the signature-verified token. func parseEmailFromIDToken(token string) (string, error) { parts := strings.Split(token, ".") if len(parts) < 2 { diff --git a/client/internal/auth/util.go b/client/internal/auth/util.go index 31c81d701..1800584a2 100644 --- a/client/internal/auth/util.go +++ b/client/internal/auth/util.go @@ -20,14 +20,26 @@ func randomBytesInHex(count int) (string, error) { return hex.EncodeToString(buf), nil } -// isValidAccessToken is a simple validation of the access token -func isValidAccessToken(token string, audience string) error { +// validateTokenAudience checks that the token is a well-formed JWT whose +// audience claim matches the expected audience. +// +// It does NOT verify the token's cryptographic signature and therefore must not +// be treated as an authenticity check. The token is obtained by the client +// directly from the IdP token endpoint over TLS, and its signature is verified +// server-side by the management server against the IdP's JWKS +// (see shared/auth/jwt/validator.go). This function is only a client-side +// sanity check that the returned token targets the expected audience. +func validateTokenAudience(token string, audience string) error { if token == "" { return fmt.Errorf("token received is empty") } - encodedClaims := strings.Split(token, ".")[1] - claimsString, err := base64.RawURLEncoding.DecodeString(encodedClaims) + parts := strings.Split(token, ".") + if len(parts) != 3 { + return fmt.Errorf("token is not a well-formed JWT") + } + + claimsString, err := base64.RawURLEncoding.DecodeString(parts[1]) if err != nil { return err } diff --git a/client/internal/auth/util_test.go b/client/internal/auth/util_test.go new file mode 100644 index 000000000..7f225bb86 --- /dev/null +++ b/client/internal/auth/util_test.go @@ -0,0 +1,108 @@ +package auth + +import ( + "encoding/base64" + "encoding/json" + "testing" +) + +// makeJWT builds an unsigned JWT-shaped string (header.payload.signature) with +// the given claims payload. The signature part is arbitrary because +// validateTokenAudience intentionally does not verify it. +func makeJWT(t *testing.T, claims map[string]interface{}) string { + t.Helper() + header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"RS256","typ":"JWT"}`)) + payloadBytes, err := json.Marshal(claims) + if err != nil { + t.Fatalf("marshal claims: %v", err) + } + payload := base64.RawURLEncoding.EncodeToString(payloadBytes) + return header + "." + payload + ".unverified-signature" +} + +func TestValidateTokenAudience(t *testing.T) { + tests := []struct { + name string + token string + audience string + wantErr bool + }{ + { + name: "empty token", + token: "", + audience: "netbird", + wantErr: true, + }, + { + name: "not a JWT - no dots", + token: "notajwt", + audience: "netbird", + wantErr: true, + }, + { + name: "not a JWT - two parts only", + token: "header.payload", + audience: "netbird", + wantErr: true, + }, + { + name: "matching string audience", + token: makeJWT(t, map[string]interface{}{"aud": "netbird"}), + audience: "netbird", + wantErr: false, + }, + { + name: "mismatching string audience", + token: makeJWT(t, map[string]interface{}{"aud": "other"}), + audience: "netbird", + wantErr: true, + }, + { + name: "matching audience in array", + token: makeJWT(t, map[string]interface{}{"aud": []interface{}{"other", "netbird"}}), + audience: "netbird", + wantErr: false, + }, + { + name: "mismatching audience array", + token: makeJWT(t, map[string]interface{}{"aud": []interface{}{"a", "b"}}), + audience: "netbird", + wantErr: true, + }, + { + name: "missing audience claim", + token: makeJWT(t, map[string]interface{}{"sub": "user"}), + audience: "netbird", + wantErr: true, + }, + { + name: "invalid base64 payload", + token: "header.!!!not-base64!!!.sig", + audience: "netbird", + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := validateTokenAudience(tc.token, tc.audience) + if tc.wantErr && err == nil { + t.Fatalf("expected error, got nil") + } + if !tc.wantErr && err != nil { + t.Fatalf("expected no error, got %v", err) + } + }) + } +} + +// TestValidateTokenAudienceNoPanic guards the regression where a non-empty +// token without the JWT dot structure caused an index-out-of-range panic. +func TestValidateTokenAudienceNoPanic(t *testing.T) { + inputs := []string{"a", ".", "a.", "aaaa", "no-dots-here"} + for _, in := range inputs { + if err := validateTokenAudience(in, "netbird"); err == nil { + t.Fatalf("expected error for malformed token %q, got nil", in) + } + } +}