From bcade97fa46c0f1e06b53e81277d3169b3f5f1eb Mon Sep 17 00:00:00 2001 From: kolaente Date: Fri, 5 Jun 2026 10:22:46 +0200 Subject: [PATCH] fix(link-sharing): resolve share read permission via project id so by-id reads work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LinkSharing.CanRead resolved the parent project from the share hash, but a by-id read (GET /projects/{project}/shares/{share}) only carries the numeric id, never the hash — so the project lookup returned ErrProjectShareDoesNotExist and every read-one 404'd, even for the share's owner. This affected both v1 and v2. Resolve the project from ProjectID when it is set (the by-id read path), keeping the hash lookup as a fallback for resolving a share purely by its public hash. The permission semantic is unchanged — you can read a share if you can read its parent project; only the project lookup changes. ReadOne still scopes by id AND project_id, so a share id from another project the caller can access is not leaked (404, no IDOR). Flips the v2 webtest's pinned 404 cases to assert success and adds the cross-project IDOR and non-member negatives. --- pkg/models/link_sharing_permissions.go | 21 +++++++++--- pkg/webtests/huma_link_sharing_test.go | 44 ++++++++++++++++++++------ 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/pkg/models/link_sharing_permissions.go b/pkg/models/link_sharing_permissions.go index 359be43d7..8027583ab 100644 --- a/pkg/models/link_sharing_permissions.go +++ b/pkg/models/link_sharing_permissions.go @@ -28,11 +28,24 @@ func (share *LinkSharing) CanRead(s *xorm.Session, a web.Auth) (bool, int, error return false, 0, nil } - l, err := GetProjectByShareHash(s, share.Hash) - if err != nil { - return false, 0, err + // A by-id read carries the parent project but no hash, so resolve the + // project from ProjectID; only fall back to the hash lookup when ProjectID + // is absent (e.g. resolving a share purely by its public hash). + var project *Project + if share.ProjectID != 0 { + var err error + project, err = GetProjectSimpleByID(s, share.ProjectID) + if err != nil { + return false, 0, err + } + } else { + var err error + project, err = GetProjectByShareHash(s, share.Hash) + if err != nil { + return false, 0, err + } } - return l.CanRead(s, a) + return project.CanRead(s, a) } // CanDelete implements the delete permission check for a link share diff --git a/pkg/webtests/huma_link_sharing_test.go b/pkg/webtests/huma_link_sharing_test.go index a9453544f..2e3be919c 100644 --- a/pkg/webtests/huma_link_sharing_test.go +++ b/pkg/webtests/huma_link_sharing_test.go @@ -156,27 +156,51 @@ func TestHumaLinkSharing(t *testing.T) { }) }) - // ReadOne is a faithful port of the v1 by-id read, including its latent - // quirk: LinkSharing.CanRead resolves the parent project from the share's - // hash, but the /projects/{project}/shares/{share} route only carries the - // numeric id, so the hash is always empty and the project lookup always - // 404s — even for the share's owner. These cases pin that behavior so a - // future fix is a deliberate, reviewed change rather than an accident. t.Run("ReadOne", func(t *testing.T) { + t.Run("Normal", func(t *testing.T) { + // share 1 belongs to project 1, owned by testuser1. CanRead resolves + // the parent project from the path's {project}, so the by-id read + // succeeds and surfaces the caller's max_permission. + rec, err := onProjectAs("1").testReadOneWithUser(nil, map[string]string{"share": "1"}) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Contains(t, rec.Body.String(), `"hash":"test"`) + assert.Contains(t, rec.Body.String(), `"max_permission":2`) + assert.NotEmpty(t, rec.Result().Header.Get("ETag")) + }) + t.Run("Password is never serialized", func(t *testing.T) { + // share 4 is a password-protected share on project 1; the bcrypt hash + // must never appear in the response (password is write-only). + rec, err := onProjectAs("1").testReadOneWithUser(nil, map[string]string{"share": "4"}) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Contains(t, rec.Body.String(), `"sharing_type":2`) + assert.NotContains(t, rec.Body.String(), `$2a$`) + }) t.Run("Nonexisting", func(t *testing.T) { _, err := onProjectAs("1").testReadOneWithUser(nil, map[string]string{"share": "9999999"}) require.Error(t, err) assert.Equal(t, http.StatusNotFound, getHTTPErrorCode(err)) assertHandlerErrorCode(t, err, models.ErrCodeProjectShareDoesNotExist) }) - t.Run("Owner read by id 404s (v1 quirk)", func(t *testing.T) { - // testuser1 owns project 1 and its share 1, yet the by-id read still - // 404s because CanRead cannot resolve the project without the hash. - _, err := onProjectAs("1").testReadOneWithUser(nil, map[string]string{"share": "1"}) + t.Run("Share from another project (no IDOR)", func(t *testing.T) { + // share 2 belongs to project 2. Reading it under project 1 — which + // testuser1 can read — must 404: ReadOne scopes by id AND project_id, + // so the share from the other project is never leaked even though the + // caller has access to the project in the path. + _, err := onProjectAs("1").testReadOneWithUser(nil, map[string]string{"share": "2"}) require.Error(t, err) assert.Equal(t, http.StatusNotFound, getHTTPErrorCode(err)) assertHandlerErrorCode(t, err, models.ErrCodeProjectShareDoesNotExist) }) + t.Run("Forbidden non-member", func(t *testing.T) { + // user2 is not a member of project 1, so reading its share 1 is denied. + h := onProjectAs("1") + h.user = &testuser2 + _, err := h.testReadOneWithUser(nil, map[string]string{"share": "1"}) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) }) t.Run("Delete", func(t *testing.T) {