From a7446ed7f61aa1a3d7073c47571a56a5eb33b18c Mon Sep 17 00:00:00 2001 From: Sabine Maennel <5292683+sabinem@users.noreply.github.com> Date: Fri, 11 Sep 2026 06:53:55 +0200 Subject: [PATCH] fix(backend): report a rejected token as Unauthenticated, not as a crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two jwt versions were installed: `auth.go` reads tokens with v5, while `errors.go` matched v4's `*jwt.ValidationError` — a type v5 deleted. The match never fired, so every expired, malformed, badly signed or wrongly issued token fell past the Unauthenticated list to `codes.Internal`, and a routine expired session was reported and logged as a server fault. Matches v5's sentinels with `errors.Is`, adds the issuer case v5 raises that nothing was mapping, and drops the now-unused jwt/v4 dependency — `errors.go` was its only importer. The invitation page no longer has to read INTERNAL as an expired session, so a real fault can say so. The auth tests asserted error messages, which come from the library and matched either way. They now assert the gRPC code, and did fail before this change. --- CHANGELOG.md | 2 + components/backend/go.mod | 1 - components/backend/go.sum | 2 - .../backend/internal/middleware/auth_test.go | 20 ++++++++ .../backend/internal/middleware/errors.go | 51 ++++++++++++------- .../(public)/invite/[token]/+page.server.ts | 32 +++++------- 6 files changed, 69 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1d0e6b1..55b6ff33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,8 @@ written while it was being built. See [RELEASING.md](RELEASING.md). ### Fixed +- A server problem while joining a hackathon no longer claims that your sign-in + has expired. A rejected login and a real fault are told apart again. - Invitation links no longer fail with "This invitation is no longer valid" for people who have never used Hackagon before. The link was always fine — their account had simply never been created. diff --git a/components/backend/go.mod b/components/backend/go.mod index 7ae1c426..90974ab6 100644 --- a/components/backend/go.mod +++ b/components/backend/go.mod @@ -19,7 +19,6 @@ require ( github.com/MicahParks/keyfunc/v3 v3.8.0 github.com/casbin/casbin/v3 v3.8.1 github.com/casbin/ent-adapter v1.4.0 - github.com/golang-jwt/jwt/v4 v4.4.2 github.com/golang-jwt/jwt/v5 v5.3.1 github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.3.2 diff --git a/components/backend/go.sum b/components/backend/go.sum index 7c3dd192..f72c7d62 100644 --- a/components/backend/go.sum +++ b/components/backend/go.sum @@ -80,8 +80,6 @@ github.com/go-viper/mapstructure/v2 v2.4.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlnd github.com/goccy/go-yaml v1.18.0 h1:8W7wMFS12Pcas7KU+VVkaiCng+kG8QiFeFwzFb+rwuw= github.com/goccy/go-yaml v1.18.0/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= -github.com/golang-jwt/jwt/v4 v4.4.2 h1:rcc4lwaZgFMCZ5jxF9ABolDcIHdBytAFgqFPbSJQAYs= -github.com/golang-jwt/jwt/v4 v4.4.2/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0= github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q= diff --git a/components/backend/internal/middleware/auth_test.go b/components/backend/internal/middleware/auth_test.go index f403f820..e9c741b5 100644 --- a/components/backend/internal/middleware/auth_test.go +++ b/components/backend/internal/middleware/auth_test.go @@ -10,7 +10,9 @@ import ( "github.com/golang-jwt/jwt/v5" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" "github.com/swissdatasciencecenter/hackagon/components/backend/internal/config" "github.com/swissdatasciencecenter/hackagon/components/backend/internal/middleware" @@ -84,6 +86,7 @@ var _ = Describe("Auth Middleware", func() { _, err := validator.AuthFunc()(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("token is malformed")) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) }) It("rejects expired tokens", func() { @@ -96,6 +99,7 @@ var _ = Describe("Auth Middleware", func() { _, err := validator.AuthFunc()(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("token is expired")) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) }) It("rejects tokens with future not-before", func() { @@ -119,6 +123,21 @@ var _ = Describe("Auth Middleware", func() { _, err = validator.AuthFunc()(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("token is not valid yet")) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) + }) + + It("rejects a token from the wrong issuer", func() { + tokenString := testutils.GenerateTestToken( + "wrong-issuer-user", 24*time.Hour, "https://not-our-issuer.example", + ) + ctx := metadata.NewIncomingContext(context.Background(), metadata.Pairs( + "authorization", "Bearer "+tokenString, + )) + + validator := middleware.NewTestJWTValidator(cfg, keyfunc) + _, err := validator.AuthFunc()(ctx) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) }) }) @@ -158,6 +177,7 @@ var _ = Describe("Auth Middleware", func() { _, err := validator.AuthFunc()(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("token is malformed")) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) }) }) diff --git a/components/backend/internal/middleware/errors.go b/components/backend/internal/middleware/errors.go index 5f16e05a..c4bfe740 100644 --- a/components/backend/internal/middleware/errors.go +++ b/components/backend/internal/middleware/errors.go @@ -4,7 +4,7 @@ import ( "errors" "strings" - "github.com/golang-jwt/jwt/v4" + "github.com/golang-jwt/jwt/v5" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -35,25 +35,40 @@ func handleError(err error, setErrorCodes bool) error { return err } +// handleJwtError translates a failure from the jwt library into one of this +// package's own errors, which `setGrpcErrorCodes` then turns into +// `Unauthenticated`. +// +// Matched with `errors.Is` against the library's sentinel values. v5 reports a +// bad token by wrapping those sentinels; it has no error *type* to match on — +// the `*jwt.ValidationError` of v4 was removed. This file went on matching the +// v4 type while `auth.go` parsed with v5, so nothing here ever matched and +// every expired, malformed or badly signed token fell through to `Internal`, +// reported to callers as a server fault rather than a rejected login. +// +// Anything unrecognised is returned untouched, so an error raised by `auth.go` +// itself (a missing header, an unknown key id) reaches `setGrpcErrorCodes` as +// the sentinel it already is. func handleJwtError(errIn error) error { - var err *jwt.ValidationError - if errors.As(errIn, &err) { - switch { - case err.Is(jwt.ErrTokenExpired): - return ErrTokenExpired - case err.Is(jwt.ErrTokenMalformed): - return ErrTokenMalformed - case err.Is(jwt.ErrTokenNotValidYet): - return ErrTokenNotValidYet - case err.Is(jwt.ErrTokenUsedBeforeIssued): - return ErrTokenUsedBeforeIssued - case err.Is(jwt.ErrTokenSignatureInvalid): - if strings.Contains(err.Error(), "signing method") { - return ErrBadAlgorithm - } - - return ErrSignatureInvalid + switch { + case errors.Is(errIn, jwt.ErrTokenExpired): + return ErrTokenExpired + case errors.Is(errIn, jwt.ErrTokenMalformed): + return ErrTokenMalformed + case errors.Is(errIn, jwt.ErrTokenNotValidYet): + return ErrTokenNotValidYet + case errors.Is(errIn, jwt.ErrTokenUsedBeforeIssued): + return ErrTokenUsedBeforeIssued + case errors.Is(errIn, jwt.ErrTokenInvalidIssuer): + return ErrTokenInvalidIssuer + case errors.Is(errIn, jwt.ErrTokenSignatureInvalid): + // `WithValidMethods` rejects a wrong algorithm through the same + // sentinel, and only the message tells the two apart. + if strings.Contains(errIn.Error(), "signing method") { + return ErrBadAlgorithm } + + return ErrSignatureInvalid } return errIn diff --git a/components/frontend/src/routes/(public)/invite/[token]/+page.server.ts b/components/frontend/src/routes/(public)/invite/[token]/+page.server.ts index 6d3bf2f0..e7ab79b3 100644 --- a/components/frontend/src/routes/(public)/invite/[token]/+page.server.ts +++ b/components/frontend/src/routes/(public)/invite/[token]/+page.server.ts @@ -87,17 +87,13 @@ function authorizedFor(session: CustomSession | null) { * refusal falls back to the anonymous call: everything but that one field is * identical, and the page's whole point is being readable before signing in. * `usableSession` already screens out the refusal Auth.js reports, but a token - * can also lapse between refreshes, and the backend answers that with INTERNAL - * rather than UNAUTHENTICATED — see `TODO(backend: jwt-error-codes)` below. + * can also lapse between refreshes, and that arrives here as UNAUTHENTICATED. */ function askPreview(token: string, grpc?: AuthorizedGrpc) { if (!grpc) return publicHackathonClient().previewInvite({ token }) return grpc.hackathon.previewInvite({ token }).catch((e) => { - if ( - e instanceof ClientError && - (e.code === Status.UNAUTHENTICATED || e.code === Status.INTERNAL) - ) { + if (e instanceof ClientError && e.code === Status.UNAUTHENTICATED) { return publicHackathonClient().previewInvite({ token }) } throw e @@ -263,22 +259,22 @@ export const actions: Actions = { return fail(400, { message: e.details || "Some answers are not valid.", }) - // The backend refusing the token itself. UNAUTHENTICATED is what the - // middleware means to send. - // - // TODO(backend: jwt-error-codes): INTERNAL is in this branch because it - // is what actually arrives. `errors.go` matches jwt/**v4**'s - // `*ValidationError` while `auth.go` parses with **v5**, which removed - // that type — so `errors.As` never matches and every auth failure falls - // past the `unauthenticatedErrors` list to `codes.Internal`. Drop - // INTERNAL from here once that is fixed; until then a genuine server - // fault during a join is reported to the user as an expired session, - // which is the lesser of the two wrong answers available. - if (e.code === Status.UNAUTHENTICATED || e.code === Status.INTERNAL) + // The backend refusing the token itself. + if (e.code === Status.UNAUTHENTICATED) return fail(401, { message: "Your sign-in has expired. Sign in again — this invitation still works.", }) + // A real server fault, and now distinguishable from one: INTERNAL used + // to arrive for every rejected token too, so this page had to read it + // as an expired session. It no longer does, so a fault can say so and + // the invitation can be described as still good — which it is. + if (e.code === Status.INTERNAL) + return fail(500, { + message: + "Something went wrong on our side. Your invitation is still " + + "valid — please try again.", + }) } throw e }