From e025209e3c50cef0583b1602ae2d076aa5b0486e Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 9 Apr 2026 17:04:14 +0200 Subject: [PATCH] fix(security): validate link share JWTs against DB on every request Previously GetLinkShareFromClaims built a *LinkSharing entirely from JWT claims with no DB interaction, so deleted shares and permission downgrades took up to 72h (the JWT TTL) to take effect. The permission and sharedByID claims were trusted blindly. GetLinkShareFromClaims now takes an *xorm.Session, looks up the share via GetLinkShareByID, verifies the hash claim against the DB row, and returns ErrLinkShareTokenInvalid when the row is missing or the hash mismatches. The permission and sharedByID claims are discarded; the DB row is authoritative. GetAuthFromClaims opens a read session for the link-share branch, mirroring the existing API-token branch. Token creation and the JWT format are unchanged, so already-issued tokens keep working except when the underlying share has been deleted or its hash no longer matches. Fixes GHSA-96q5-xm3p-7m84 / CVE-2026-35594. --- pkg/models/link_sharing.go | 49 ++++++------ pkg/models/link_sharing_test.go | 130 ++++++++++++++++++++++++++++++++ pkg/modules/auth/auth.go | 4 +- 3 files changed, 160 insertions(+), 23 deletions(-) diff --git a/pkg/models/link_sharing.go b/pkg/models/link_sharing.go index a08b351bb..23a3fd876 100644 --- a/pkg/models/link_sharing.go +++ b/pkg/models/link_sharing.go @@ -84,19 +84,15 @@ func (share *LinkSharing) GetID() int64 { return share.ID } -// GetLinkShareFromClaims builds a link sharing object from jwt claims -func GetLinkShareFromClaims(claims jwt.MapClaims) (share *LinkSharing, err error) { - projectID, is := claims["project_id"].(float64) - if !is { - return nil, &ErrLinkShareTokenInvalid{} - } - - permissionFloat, is := claims["permission"].(float64) - if !is { - return nil, &ErrLinkShareTokenInvalid{} - } - - id, is := claims["id"].(float64) +// GetLinkShareFromClaims resolves a link share JWT against the DB on every +// call so that deletion or permission downgrades take effect immediately +// instead of persisting for the (up to 72h) JWT TTL. The `permission` and +// `sharedByID` claims NewLinkShareJWTAuthtoken writes are intentionally +// ignored: the DB row is authoritative. +// +// Fixes GHSA-96q5-xm3p-7m84 / CVE-2026-35594. +func GetLinkShareFromClaims(s *xorm.Session, claims jwt.MapClaims) (share *LinkSharing, err error) { + idFloat, is := claims["id"].(float64) if !is { return nil, &ErrLinkShareTokenInvalid{} } @@ -104,18 +100,27 @@ func GetLinkShareFromClaims(claims jwt.MapClaims) (share *LinkSharing, err error if !is { return nil, &ErrLinkShareTokenInvalid{} } - sharedByID, is := claims["sharedByID"].(float64) - if !is { + + share, err = GetLinkShareByID(s, int64(idFloat)) + if err != nil { + // Only a missing row means the token is stale; everything else + // (DB down, etc.) must bubble up as 500 instead of 400. + if IsErrProjectShareDoesNotExist(err) { + return nil, &ErrLinkShareTokenInvalid{} + } + return nil, err + } + + // Defense in depth against a future bug that mints a token with a + // mismatched id/hash pair — the JWT signature alone already prevents + // tampering at rest. + if share.Hash != hash { return nil, &ErrLinkShareTokenInvalid{} } - share = &LinkSharing{} - share.ID = int64(id) - share.Hash = hash - share.ProjectID = int64(projectID) - share.Permission = Permission(permissionFloat) - share.SharedByID = int64(sharedByID) - return + // Never leak the hashed password through the auth object. + share.Password = "" + return share, nil } func (share *LinkSharing) getUserID() int64 { diff --git a/pkg/models/link_sharing_test.go b/pkg/models/link_sharing_test.go index 268024e60..53482d7b2 100644 --- a/pkg/models/link_sharing_test.go +++ b/pkg/models/link_sharing_test.go @@ -23,6 +23,7 @@ import ( "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/user" + "github.com/golang-jwt/jwt/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -216,3 +217,132 @@ func TestLinkSharing_toUser(t *testing.T) { assert.Equal(t, int64(-2), user.ID) }) } + +func TestGetLinkShareFromClaims(t *testing.T) { + // Mirrors NewLinkShareJWTAuthtoken, including the legacy `permission` + // and `sharedByID` claims so the tests below can prove they're ignored. + buildClaims := func(id int64, hash string, projectID int64, permission Permission, sharedByID int64) jwt.MapClaims { + return jwt.MapClaims{ + "type": float64(2), // AuthTypeLinkShare + "id": float64(id), + "hash": hash, + "project_id": float64(projectID), + "permission": float64(permission), + "sharedByID": float64(sharedByID), + } + } + + t.Run("valid share returns DB values", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + dbShare, err := GetLinkShareByID(s, 1) + require.NoError(t, err) + + claims := buildClaims(dbShare.ID, dbShare.Hash, dbShare.ProjectID, dbShare.Permission, dbShare.SharedByID) + + got, err := GetLinkShareFromClaims(s, claims) + require.NoError(t, err) + assert.Equal(t, dbShare.ID, got.ID) + assert.Equal(t, dbShare.Hash, got.Hash) + assert.Equal(t, dbShare.ProjectID, got.ProjectID) + assert.Equal(t, dbShare.Permission, got.Permission) + assert.Equal(t, dbShare.SharedByID, got.SharedByID) + }) + + t.Run("deleted share is rejected", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + dbShare, err := GetLinkShareByID(s, 1) + require.NoError(t, err) + claims := buildClaims(dbShare.ID, dbShare.Hash, dbShare.ProjectID, dbShare.Permission, dbShare.SharedByID) + + _, err = s.Where("id = ?", dbShare.ID).Delete(&LinkSharing{}) + require.NoError(t, err) + + _, err = GetLinkShareFromClaims(s, claims) + require.Error(t, err) + assert.True(t, IsErrLinkShareTokenInvalid(err), + "expected ErrLinkShareTokenInvalid for deleted share, got %T: %v", err, err) + }) + + t.Run("permission downgrade takes effect immediately", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + dbShare, err := GetLinkShareByID(s, 3) + require.NoError(t, err) + require.Equal(t, PermissionAdmin, dbShare.Permission, + "fixture precondition: share id=3 must start as admin") + + // Capture claims while the share is still admin, then downgrade. + claims := buildClaims(dbShare.ID, dbShare.Hash, dbShare.ProjectID, PermissionAdmin, dbShare.SharedByID) + + _, err = s.Where("id = ?", dbShare.ID).Cols("permission").Update(&LinkSharing{Permission: PermissionRead}) + require.NoError(t, err) + + got, err := GetLinkShareFromClaims(s, claims) + require.NoError(t, err) + assert.Equal(t, PermissionRead, got.Permission, + "permission must come from DB, not from the (stale) JWT claim") + }) + + t.Run("hash mismatch is rejected", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + dbShare, err := GetLinkShareByID(s, 1) + require.NoError(t, err) + + claims := buildClaims(dbShare.ID, "not-the-real-hash", dbShare.ProjectID, dbShare.Permission, dbShare.SharedByID) + + _, err = GetLinkShareFromClaims(s, claims) + require.Error(t, err) + assert.True(t, IsErrLinkShareTokenInvalid(err)) + }) + + t.Run("sharedByID comes from DB not from claim", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + dbShare, err := GetLinkShareByID(s, 1) + require.NoError(t, err) + + // Bogus sharedByID in the claim must be ignored in favor of the DB value. + claims := buildClaims(dbShare.ID, dbShare.Hash, dbShare.ProjectID, dbShare.Permission, 9999999) + + got, err := GetLinkShareFromClaims(s, claims) + require.NoError(t, err) + assert.Equal(t, dbShare.SharedByID, got.SharedByID) + }) + + t.Run("missing id claim is rejected", func(t *testing.T) { + s := db.NewSession() + defer s.Close() + + claims := jwt.MapClaims{ + "hash": "whatever", + } + _, err := GetLinkShareFromClaims(s, claims) + require.Error(t, err) + assert.True(t, IsErrLinkShareTokenInvalid(err)) + }) + + t.Run("missing hash claim is rejected", func(t *testing.T) { + s := db.NewSession() + defer s.Close() + + claims := jwt.MapClaims{ + "id": float64(1), + } + _, err := GetLinkShareFromClaims(s, claims) + require.Error(t, err) + assert.True(t, IsErrLinkShareTokenInvalid(err)) + }) +} diff --git a/pkg/modules/auth/auth.go b/pkg/modules/auth/auth.go index 402de54e9..4e2ebb94e 100644 --- a/pkg/modules/auth/auth.go +++ b/pkg/modules/auth/auth.go @@ -184,7 +184,9 @@ func GetAuthFromClaims(c *echo.Context) (a web.Auth, err error) { } typ := int(typFloat) if typ == AuthTypeLinkShare && config.ServiceEnableLinkSharing.GetBool() { - return models.GetLinkShareFromClaims(claims) + s := db.NewSession() + defer s.Close() + return models.GetLinkShareFromClaims(s, claims) } if typ == AuthTypeUser { return user.GetUserFromClaims(claims)