fix(api): enforce project visibility on all project sub-resources#319
fix(api): enforce project visibility on all project sub-resources#319cavidelizade wants to merge 1 commit into
Conversation
Secret projects (network != public) were hidden by ProjectService.GetByID — GET .../projects/:id/ returns 404 to a workspace member who isn't a project member — but every project-scoped sub-resource only checked workspace membership + project-in-workspace, never the project's visibility. A plain workspace member could therefore read and mutate a secret project's issues, states, labels, cycles, modules, estimates, intake, comments, pages and views, download its attachment binaries, and export its data through the sub-resource routes. Add a shared enforceProjectVisibility helper that mirrors the gate in GetByID (public → any workspace member; secret → workspace admin/owner or a member of the project, else 404) and call it from every ensureProjectAccess plus AttachmentService.AuthorizeDownload and ExportService.ExportIssues. Covered by TestSecretProjectVisibility, which drives the intake and states services end-to-end for outsider / workspace-admin / project-member / project-lead against a secret project. Refs Devlaner#314 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds centralized visibility enforcement for non-public projects and applies it across project-scoped services, attachment downloads, and exports. Integration tests verify access for public projects, workspace administrators, project members, project leads, and unauthorized workspace members. ChangesProject visibility enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/internal/service/comment.go (1)
77-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the final return statement.
Since
enforceProjectVisibilityis the final operation executed inensureProjectAccessacross these services, and its return type (error) directly matches the outer function's return signature, you can simplify theif err != nilblocks to return the execution result directly.
apps/api/internal/service/comment.go#L77-L80: Replace lines 77-80 withreturn enforceProjectVisibility(ctx, s.ps, s.ws, wrk.ID, projectID, userID).apps/api/internal/service/cycle.go#L100-L103: Replace lines 100-103 withreturn enforceProjectVisibility(ctx, s.ps, s.ws, wrk.ID, projectID, userID).apps/api/internal/service/issue_view.go#L51-L54: Replace lines 51-54 withreturn enforceProjectVisibility(ctx, s.ps, s.ws, wrk.ID, projectID, userID).apps/api/internal/service/module.go#L58-L61: Replace lines 58-61 withreturn enforceProjectVisibility(ctx, s.ps, s.ws, wrk.ID, projectID, userID).apps/api/internal/service/page.go#L126-L129: Replace lines 126-129 withreturn enforceProjectVisibility(ctx, s.projectStore, s.ws, wrk.ID, projectID, userID).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/internal/service/comment.go` around lines 77 - 80, In each ensureProjectAccess implementation, return the final enforceProjectVisibility call directly instead of wrapping it in an error check followed by nil: update apps/api/internal/service/comment.go lines 77-80, cycle.go lines 100-103, issue_view.go lines 51-54, and module.go lines 58-61 with the existing project-store arguments, and page.go lines 126-129 using s.projectStore. Preserve the current arguments and function behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/internal/service/comment.go`:
- Around line 77-80: In each ensureProjectAccess implementation, return the
final enforceProjectVisibility call directly instead of wrapping it in an error
check followed by nil: update apps/api/internal/service/comment.go lines 77-80,
cycle.go lines 100-103, issue_view.go lines 51-54, and module.go lines 58-61
with the existing project-store arguments, and page.go lines 126-129 using
s.projectStore. Preserve the current arguments and function behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a79e559-13bd-49da-915a-c97aebcadca7
📒 Files selected for processing (14)
apps/api/internal/service/access.goapps/api/internal/service/access_test.goapps/api/internal/service/attachment.goapps/api/internal/service/comment.goapps/api/internal/service/cycle.goapps/api/internal/service/estimate.goapps/api/internal/service/export.goapps/api/internal/service/intake.goapps/api/internal/service/issue.goapps/api/internal/service/issue_view.goapps/api/internal/service/label.goapps/api/internal/service/module.goapps/api/internal/service/page.goapps/api/internal/service/state.go
|
@martian56 this one closes the access-control gap in #314 — secret projects were reachable through their sub-resource routes (issues, comments, attachments, exports…) by any workspace member, even though the project itself 404s for them. Centralised the visibility check in one shared helper and wired it into every sub-resource path, with a test that drives it end-to-end for outsider/admin/project-member/lead. CI's green and there are no open comments. I left the guest-role part of the issue out on purpose since it needs a call from you on what guests should actually see. Good to merge? |
1 participant
Summary
Project-scoped sub-resources didn't honour a project's visibility (
network). A secret project (network != public) is hidden byProjectService.GetByID—GET .../projects/:id/returns 404 to a workspace member who isn't a project member — but every sub-resource route only checked workspace membership + project-in-workspace, so the data stayed reachable.Linked issues
Refs #314
Type of change
fix:) — corrects broken behaviorSurface
apps/api/)What changed
internal/service/access.go→enforceProjectVisibility(ctx, ps, ws, workspaceID, projectID, userID). It mirrors the gate already inProjectService.GetByID: public project → any workspace member; secret project → workspace admin/owner or a member of the project, otherwiseErrProjectNotFound(404, so we don't leak existence).ensureProjectAccesshelpers (issue,issue_view,module,cycle,comment,attachment,page,estimate,intake,label,state), plus the two paths that don't go through it:AttachmentService.AuthorizeDownload(attachment binary downloads) andExportService.ExportIssues(per selected project).TestSecretProjectVisibility— drives the intake and states services end-to-end against a secret project for four callers: outsider (denied, 404), workspace admin (allowed), project member (allowed), project lead (allowed); and confirms the same project is readable by any member while it's public.Why this approach
The gate already existed in
GetByID; the bug was that sub-resources re-implemented access as bare workspace membership. Rather than duplicate the visibility logic 13 more times, I put it in one shared helper and called it right after the existingIsInWorkspacecheck, so the change is small and uniform and there's a single place to reason about visibility. ReturningErrProjectNotFoundkeeps the 404 behaviour consistent withGetByID(avoids leaking that a secret project exists).Scope note — workspace "guest" role
Issue #314 also mentions that the
guestrole (RoleGuest) currently gets full member access becauseIsMemberignores role, and the project'sguest_view_all_featuresflag isn't enforced. I left that out of this PR on purpose: it needs a decision on what guests are actually meant to see/do, whereas the secret-project leak is an unambiguous fix. Happy to follow up once the intended guest capability is settled — I'll keep #314 open for it.Database / migrations
Breaking changes
Test plan
go test ./...(API) green, including the newTestSecretProjectVisibilitygo vet ./...clean--no-verifyon commit)AI assistance
Claude Code (Opus 4.8)— and AI-assisted commits include aCo-Authored-By:trailerSummary by CodeRabbit
New Features
Bug Fixes