mirror of
https://github.com/go-vikunja/vikunja.git
synced 2026-05-10 15:15:41 -05:00
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.
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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))
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user