4.2 KiB
4.2 KiB
id, title, status, wave, depends_on, plan, spec
| id | title | status | wave | depends_on | plan | spec | |
|---|---|---|---|---|---|---|---|
| 02-handler-error-mapping | Handler error mapping + HTTP regression tests | pending | 1 |
|
plan.md | ../../specs/workflow-sync-workspace-authz/spec.md |
Task 02: Handler error mapping + HTTP regression tests
Map the new ErrWorkspaceNotFound denial to a sanitized 404 in the HTTP layer, and add the
HTTP-level regression coverage this package currently lacks entirely (handlers_test.go does not
exist yet).
Changes
apps/backend/internal/workflowsync/handlers.go:- Import
errors(already imported) andgithub.com/kandev/kandev/internal/task/repository/repoerrors. - Add
func workspaceDenied(err error) bool { return errors.Is(err, repoerrors.ErrWorkspaceNotFound) }(mirrorinternal/jira/handlers.go:18-22). - In
httpGetConfig,httpSetConfig,httpDeleteConfig,httpForceSync: checkworkspaceDenied(err)before the existing generic-error branch and respondctx.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}). Do not include the underlying service error or any config fields in this response.
- Import
- New file
apps/backend/internal/workflowsync/handlers_test.go— HTTP-level tests usinggin.New()+RegisterRoutes+httptest.NewRequest+authn.WithIdentityon the request context (pattern:internal/github/controller_auth_identity_test.go).
Acceptance (from spec Scenarios)
- Owner identity: GET/POST/DELETE/sync all succeed against their own workspace, unchanged from pre-fix behavior.
- Foreign member identity against a seeded victim workspace's config: all four routes return 404, and the response body contains none of the victim's repo owner/name/branch/path/project_path.
- The victim's config is byte-for-byte unchanged after the denied POST/DELETE attempts (re-read via the service and compare).
- Synthetic identity (auth disabled) succeeds on all four routes — no regression for the default path.
- The two "foreign member GET/POST" cases are the regression tests: written to fail against pre-task-01 code (they'd get 200 + leaked/accepted data) and pass after task 01 + this task.
Verification
cd apps/backend && go test ./internal/workflowsync/... -run 'TestHTTP|TestConfig|TestForceSync' -v
cd apps/backend && go test ./internal/workflowsync/... -v
cd apps/backend && golangci-lint run ./internal/workflowsync/... --new-from-rev=main --timeout=5m
cd apps/backend && go test -race ./internal/workflowsync/...
Results
- Status: done, including two follow-up fixes from an independent Codex review round.
go test ./internal/workflowsync/... -run 'TestHTTPHandlers' -v: all pass (TestHTTPHandlers_OwnerSucceedsOnAllRoutes,TestHTTPHandlers_SyntheticIdentitySucceedsOnAllRoutes,TestHTTPHandlers_ForeignMemberDeniedOnAllRoutesWithoutLeaking, each table-driven across get/post/delete/sync, plusTestHTTPHandlers_ForceSyncDeniesSecondLookupAfterAccessRevoked).- Regression confirmed: before the task-01 service change, the get/post/delete/sync subtests failed with 500s and leaked/accepted the foreign workspace's config (verified during TDD).
- Codex review finding (blocker, fixed):
httpForceSync's second call (GetConfigForWorkspace, used to build the response after a successful sync) did not mapErrWorkspaceNotFoundto 404 — a race where access is revoked between the sync and the follow-up read fell through to a generic 500. Fixed with the sameworkspaceDeniedcheck; covered byTestHTTPHandlers_ForceSyncDeniesSecondLookupAfterAccessRevoked(verified to fail pre-fix, pass post-fix). - Codex review finding (suggestion, fixed): owner/synthetic-identity coverage was GET-only and
the leak/mutation assertions didn't cover branch/path or the full config. Rewritten as
table-driven tests across all four operations (
TestHTTPHandlers_OwnerSucceedsOnAllRoutes,TestHTTPHandlers_SyntheticIdentitySucceedsOnAllRoutes), with distinctive branch/path fixture values and a full-structbefore/afterconfig comparison in the denial test. go test -race ./internal/workflowsync/... ./internal/backendapp/...: bothok.golangci-lint run ./internal/workflowsync/... --new-from-rev=main --timeout=5m: 0 issues.