diff --git a/pkg/models/task_comments.go b/pkg/models/task_comments.go index a61356404..37cbf7e87 100644 --- a/pkg/models/task_comments.go +++ b/pkg/models/task_comments.go @@ -185,10 +185,8 @@ func (tc *TaskComment) Update(s *xorm.Session, a web.Auth) error { return err } - // The doer must come from the authenticated user, not from the request - // body: tc.Author is bound from the payload and could be omitted (nil) or - // spoofed. CanUpdate already guarantees the authenticated user is the - // comment's author, so resolving the doer from the session is correct. + // Resolve the doer from the session, not from tc.Author: the latter is bound + // from the request body and could be omitted (nil) or spoofed. doer, err := GetUserOrLinkShareUser(s, a) if err != nil { return err diff --git a/pkg/routes/api/v2/task_comments.go b/pkg/routes/api/v2/task_comments.go index 58afe5c33..ebe38c117 100644 --- a/pkg/routes/api/v2/task_comments.go +++ b/pkg/routes/api/v2/task_comments.go @@ -29,19 +29,15 @@ import ( "github.com/danielgtaylor/huma/v2/conditional" ) -// taskCommentListBody is the list-response envelope. models.TaskComment.ReadAll -// returns []*models.TaskComment, so that's the element type. type taskCommentListBody struct { Body Paginated[*models.TaskComment] } // RegisterTaskCommentRoutes wires the nested TaskComment CRUD onto the Huma API. -// Every operation binds two path params: {task} → TaskID and {commentid} → ID. // -// The resource is feature-gated by config.ServiceEnableTaskComments. The flag is -// checked here rather than in the central wiring: the registrar runs at -// RegisterAll time, after the config has loaded, so a disabled instance simply -// registers no comment routes. +// The feature gate is checked here, not in the central wiring: the registrar +// runs at RegisterAll time after the config has loaded, so a disabled instance +// registers no comment routes at all. func RegisterTaskCommentRoutes(api huma.API) { if !config.ServiceEnableTaskComments.GetBool() { return @@ -126,13 +122,13 @@ func taskCommentsRead(ctx context.Context, in *struct { if err != nil { return nil, err } - // ReadOne resolves the comment scoped to its parent task — the TaskID guards - // against reading a comment of one task through another (IDOR). + // TaskID scopes the lookup to the parent task, guarding against reading a + // comment of one task through another (IDOR). comment := &models.TaskComment{ID: in.ID, TaskID: in.TaskID} if _, err := handler.DoReadOne(ctx, comment, a); err != nil { return nil, translateDomainError(err) } - // PreconditionFailed wants the unquoted etag; response header uses RFC 9110 quoted form. + // PreconditionFailed wants the unquoted etag; the response header uses the RFC 9110 quoted form. etag := fmt.Sprintf("%d-%d", comment.ID, comment.Updated.UnixNano()) if in.HasConditionalParams() { if err := in.PreconditionFailed(etag, comment.Updated); err != nil { diff --git a/pkg/webtests/huma_task_comment_test.go b/pkg/webtests/huma_task_comment_test.go index 62244c413..3edd097c6 100644 --- a/pkg/webtests/huma_task_comment_test.go +++ b/pkg/webtests/huma_task_comment_test.go @@ -30,36 +30,14 @@ import ( "github.com/stretchr/testify/require" ) -// TestHumaTaskComment is the nested feature-gated reference test for /api/v2. -// Comments live under /tasks/{task}/comments/{commentid}, so the harness binds -// two path params: basePath carries the literal {task} and idParam picks -// {commentid}. +// TestHumaTaskComment ports the v1 webtest coverage (TestTaskComments + +// TestTaskCommentIDOR) to /api/v2, plus v2-specific HTTP assertions (status +// codes, ETag). It re-proves the full permission/sharing matrix independently +// because the v1 routes and their tests will be removed. // -// The resource is gated behind config.ServiceEnableTaskComments, which -// InitDefaultConfig (called by setupTestEnv) defaults to true — so the -// registrar registers the routes and these tests can reach them. -// -// This is a 1:1 port of the v1 webtest coverage (TestTaskComments + -// TestTaskCommentIDOR) plus the HTTP-layer assertions specific to v2 (status -// codes, ETag). It re-proves the complete permission/sharing matrix -// independently of v1 because the v1 routes (and their tests) will be removed. -// -// Fixtures used by the matrix (all run as testuser1 unless noted): -// - task 1 (project 1, owned by testuser1) has comment 1 (author 1). -// - task 35 (project 21, owned by testuser1) has comment 15 (author 1) and -// comment 17 (author -2, a link share) — used for the link-share read case. -// - tasks 15–26 carry comments 3–14, all authored by user 5/6 (never -// testuser1), behind the project-share matrix below. testuser1 is granted -// access via these shares but is never the comment author, which is what -// proves the author-only update/delete rule (vs. plain access denial): -// 15→team-ro, 16→team-write, 17→team-admin, 18→user-ro, 19→user-write, -// 20→user-admin, 21→parent-team-ro, 22→parent-team-write, -// 23→parent-team-admin, 24→parent-user-ro, 25→parent-user-write, -// 26→parent-user-admin. -// - task 13 (project 2) is reachable by link share id 2 (write) — used for -// the link-share create case asserting author_id == -2. -// - task 34 (project 20, owned by user 13) is inaccessible to testuser1; -// comment 18 lives there — used for the IDOR negative. +// The crux of the author-only rule: across tasks 15–26, testuser1 is granted +// access through every share kind but never authored the comments (user 5/6 +// did), so a 403 there exercises authorship rather than plain access denial. func TestHumaTaskComment(t *testing.T) { // task 1 belongs to project 1, owned by testuser1. onTask1 := webHandlerTestV2{ @@ -69,10 +47,9 @@ func TestHumaTaskComment(t *testing.T) { t: t, } require.NoError(t, onTask1.ensureEnv()) - // onTaskAs builds a handler for a different task while sharing the one Echo - // instance (so the JWT signing secret and the single fixture load stay - // valid across the whole matrix). v2 does not reload fixtures per request, - // so subtests below are arranged to avoid clobbering each other's rows. + // onTaskAs reuses the one Echo instance (and its single fixture load) for a + // different task. v2 does not reload fixtures per request, so the subtests + // are ordered to avoid clobbering each other's rows. onTaskAs := func(taskID string, u *user.User) *webHandlerTestV2 { return &webHandlerTestV2{ user: u, @@ -105,14 +82,12 @@ func TestHumaTaskComment(t *testing.T) { assert.Contains(t, rec.Body.String(), `comment 17`) }) t.Run("order_by desc", func(t *testing.T) { - // order_by is an exposed query param; just assert it is accepted. rec, err := onTask35.testReadAllWithUser(url.Values{"order_by": []string{"desc"}}, nil) require.NoError(t, err) assert.Contains(t, rec.Body.String(), `comment 15`) }) t.Run("Search filter", func(t *testing.T) { - // q narrows results to matching comments; mirrors the v1 model - // ReadAll search test (search "COMMENT 15" returns only comment 15). + // Mirrors the v1 model ReadAll search test: search is case-insensitive. rec, err := onTask35.testReadAllWithUser(url.Values{"q": []string{"COMMENT 15"}}, nil) require.NoError(t, err) assert.Contains(t, rec.Body.String(), `comment 15`)