fix(issues): validate related ids on work-item create and update - #257
Conversation
Create and update accepted state_id, label_ids, parent_id, and assignee_ids without checking they belonged to the right scope, so a stale or malicious client could attach a state/label from another project, a parent from another project/workspace, or an assignee who isn't a workspace member — corrupting boards, filters, analytics, and notifications. Both paths now run a validateRelations check up front: state and labels must belong to the same project, a parent must be another issue in the same project, and every assignee must be a workspace member. Invalid ids return 400 instead of being silently written. Wired the label store into the issue service so label ownership can be checked. Closes #124 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds server-side validation for issue state, label, parent, and assignee references on create and update, maps invalid relation errors to HTTP 400, wires the label store into issue service setup, and adds handler tests covering rejected and valid relation inputs. ChangesIssue relation validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant IssueHandler
participant IssueService
participant StateStore
participant LabelStore
Client->>IssueHandler: POST/PATCH issue with relation IDs
IssueHandler->>IssueService: Create(...) / Update(...)
IssueService->>StateStore: validate state/project scope
IssueService->>LabelStore: validate label/project scope
IssueService->>IssueService: validate parent and assignee scope
alt invalid relation
IssueService-->>IssueHandler: ErrInvalid*
IssueHandler-->>Client: 400 Bad Request
else valid relation
IssueService-->>IssueHandler: success
IssueHandler-->>Client: 201 Created / 200 OK
end
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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" Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@apps/api/internal/handler/issue_relation_validation_test.go`:
- Around line 51-74: `TestIssue_UpdateRejectsForeignRelations` is missing
coverage for rejecting a foreign `parent_id` on the Issue update path. Add an
update assertion in this test that PATCHes the same issue with a parent issue
from `otherProject` and expects `http.StatusBadRequest`, using the existing
`base`, `testutil.CreateIssue`, and `ts.PATCH` flow to match the other
foreign-relation checks.
In `@apps/api/internal/service/issue.go`:
- Around line 83-101: `validateRelations` currently only verifies that
`parent_id` belongs to the same project, so `IssueService.Update` can still
assign an issue as its own parent. Update the call path to pass the current
issue ID into `validateRelations` for update operations, and add an early
equality check in `validateRelations` to return `ErrInvalidParent` when
`parentID` matches the issue’s own ID before calling `s.is.GetByID`.
- Around line 84-107: In the validation path inside issue service logic (the
checks using s.states.GetByID, s.labels.GetByID, s.is.GetByID, and
s.ws.IsMember), stop collapsing every lookup failure into the invalid-*
sentinels. Keep mapping ErrRecordNotFound to
ErrInvalidState/ErrInvalidLabel/ErrInvalidParent/ErrInvalidAssignee, but return
any other error directly so database or query failures bubble up instead of
being treated as bad input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78167b76-791d-4d77-823a-7af6eb876730
📒 Files selected for processing (4)
apps/api/internal/handler/issue.goapps/api/internal/handler/issue_relation_validation_test.goapps/api/internal/router/router.goapps/api/internal/service/issue.go
…hecks CodeRabbit on PR #257: - reject setting an issue's parent to itself (validateRelations now takes the issue id and refuses a self-parent on update). - only "record not found" maps to the invalid-* sentinels; any other datastore error from the state/label/parent lookups or the membership check is returned so real failures surface as 5xx instead of 400. - cover the update-path parent rejection (foreign project + self) in the tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Closes #124. Issue create and update accepted
state_id,label_ids,parent_id, andassignee_idswithout checking they belonged to the right scope. A stale or malicious client could attach a state or label from another project, a parent from another project/workspace, or an assignee who isn't a workspace member — leaving the DB in a confusing state and skewing boards, filters, analytics, and notifications.How
Both create and update now run a
validateRelationscheck up front:state_idand everylabel_idmust belong to the same project.parent_idmust be another issue in the same project.assignee_idmust be a member of the workspace.Invalid ids return 400 with the specific reason instead of being written silently. The label store is wired into the issue service so label ownership can be checked; ownership stores are optional, so a check is skipped only if its store isn't configured (the router always wires them).
Testing
New
internal/handler/issue_relation_validation_test.go:Full
go test ./internal/handler ./internal/servicegreen, no regressions.AI assistance
Produced with the help of Claude Code (Claude Opus 4.8). AI-assisted commits carry a
Co-Authored-Bytrailer.Summary by CodeRabbit
400 Bad Requestwith a clear error message instead of a generic server error.