diff --git a/.github/scripts/pr-walkthrough-workflow-contract_test.py b/.github/scripts/pr-walkthrough-workflow-contract_test.py index 8577c2b883..b693ddb894 100644 --- a/.github/scripts/pr-walkthrough-workflow-contract_test.py +++ b/.github/scripts/pr-walkthrough-workflow-contract_test.py @@ -363,6 +363,17 @@ class PRWalkthroughWorkflowContractTest(unittest.TestCase): self.assertNotIn("OPENCODE_API_KEY", self.link) self.assertTrue(PR_BODY_HELPER.is_file()) + def test_link_job_retries_transient_github_api_responses(self) -> None: + for value in ( + "pr_response_ok=false", + "for attempt in 1 2 3; do", + "python3 -c 'import json; json.load(open(\"pr-response.json\", encoding=\"utf-8\"))'", + 'test "$pr_response_ok" = true', + "patch_ok=false", + 'test "$patch_ok" = true', + ): + self.assertIn(value, self.link) + def test_lint_workflow_runs_contract_and_body_helper_tests(self) -> None: lint_workflow = LINT_WORKFLOW.read_text(encoding="utf-8") for command in ( diff --git a/.github/workflows/pr-walkthrough.yml b/.github/workflows/pr-walkthrough.yml index 51ce45571c..776f959788 100644 --- a/.github/workflows/pr-walkthrough.yml +++ b/.github/workflows/pr-walkthrough.yml @@ -392,7 +392,18 @@ jobs: test -f scripts/pr-walkthrough-pr-body test -n "$PUBLIC_URL" - gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" > pr-response.json + pr_response_ok=false + for attempt in 1 2 3; do + if gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" > pr-response.json && + python3 -c 'import json; json.load(open("pr-response.json", encoding="utf-8"))'; then + pr_response_ok=true + break + fi + if [ "$attempt" -lt 3 ]; then + sleep 2 + fi + done + test "$pr_response_ok" = true set +e python3 scripts/pr-walkthrough-pr-body \ @@ -410,8 +421,18 @@ jobs: fi test "$update_status" -eq 0 - gh api --method PATCH \ - "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" \ - --input pr-update.json \ - --silent + patch_ok=false + for attempt in 1 2 3; do + if gh api --method PATCH \ + "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" \ + --input pr-update.json \ + --silent; then + patch_ok=true + break + fi + if [ "$attempt" -lt 3 ]; then + sleep 2 + fi + done + test "$patch_ok" = true echo "Added the walkthrough link to the top of the PR description." >> "$GITHUB_STEP_SUMMARY" diff --git a/apps/backend/internal/backendapp/services.go b/apps/backend/internal/backendapp/services.go index bb11470ada..acc8955b01 100644 --- a/apps/backend/internal/backendapp/services.go +++ b/apps/backend/internal/backendapp/services.go @@ -239,7 +239,7 @@ func provideServices(cfg *config.Config, log *logger.Logger, repos *Repositories if githubSvc != nil { githubSvc.SetCredentialBroker(github.NewCredentialBrokerFromBroker(gitCredentialBroker)) } - shareHTTP := initShareHandlers(dbPool, repos.Task, githubSvc, log, version) + shareHTTP := initShareHandlers(dbPool, repos.Task, taskSvc, githubSvc, log, version) // Plumb code-host branch listing into the task service so provider-backed // ("Remote") repos serve branches from their owning provider rather than relying @@ -1046,12 +1046,13 @@ func initSentryService(dbPool *db.Pool, eventBus bus.EventBus, secretsStore secr func initShareHandlers( dbPool *db.Pool, taskRepo share.TaskReader, + authorizer share.TaskAccessAuthorizer, githubSvc *github.Service, log *logger.Logger, version string, ) *share.HTTPHandlers { h, _, err := share.Provide( - dbPool.Writer(), dbPool.Reader(), taskRepo, githubSvc, log, + dbPool.Writer(), dbPool.Reader(), taskRepo, authorizer, githubSvc, log, share.Config{KandevVersion: version}, ) if err != nil { diff --git a/apps/backend/internal/task/share/http.go b/apps/backend/internal/task/share/http.go index 2e1e9868b5..c09ec14392 100644 --- a/apps/backend/internal/task/share/http.go +++ b/apps/backend/internal/task/share/http.go @@ -53,6 +53,8 @@ type errorBody struct { Code string `json:"code,omitempty"` } +const shareAuthorizationFailureMessage = "failed to authorize share access" + func toShareResponse(s *Share) shareResponse { out := shareResponse{ ID: s.ID, @@ -112,7 +114,12 @@ func displayURL(stored string) string { // httpCreate handles POST /api/v1/tasks/:id/sessions/:sessionId/shares. // Query string dry_run=true returns the snapshot inline without uploading. func (h *HTTPHandlers) httpCreate(c *gin.Context) { + taskID := c.Param("id") sessionID := c.Param("sessionId") + if taskID == "" { + c.JSON(http.StatusBadRequest, errorBody{Error: "task id is required"}) + return + } if sessionID == "" { c.JSON(http.StatusBadRequest, errorBody{Error: "session id is required"}) return @@ -120,7 +127,10 @@ func (h *HTTPHandlers) httpCreate(c *gin.Context) { ctx := c.Request.Context() if strings.EqualFold(c.Query("dry_run"), "true") { - snap, err := h.svc.PreviewSnapshot(ctx, sessionID) + snap, err := h.svc.PreviewSnapshot(ctx, taskID, sessionID) + if h.writeAuthorizationFailure(c, err, sessionID) { + return + } if mapped, status := mapShareError(err); mapped != nil { c.JSON(status, mapped) return @@ -129,7 +139,15 @@ func (h *HTTPHandlers) httpCreate(c *gin.Context) { return } - if err := h.svc.CheckBackendAccess(ctx, sessionID); err != nil { + if err := h.svc.CheckBackendAccess(ctx, taskID, sessionID); err != nil { + if h.writeAuthorizationFailure(c, err, sessionID) { + return + } + if errors.Is(err, ErrNotFound) { + mapped, status := mapShareError(err) + c.JSON(status, mapped) + return + } c.JSON(http.StatusPreconditionFailed, errorBody{ Error: err.Error(), Code: "github_credential_missing", @@ -140,7 +158,10 @@ func (h *HTTPHandlers) httpCreate(c *gin.Context) { // The gist is a static file: nobody makes a request to us when they open // it, so the creator's locale is the only one we will ever know. Resolve // it here and thread it down rather than letting the builders guess. - share, err := h.svc.CreateShare(ctx, sessionID, i18n.FromRequest(c.Request)) + share, err := h.svc.CreateShare(ctx, taskID, sessionID, i18n.FromRequest(c.Request)) + if h.writeAuthorizationFailure(c, err, sessionID) { + return + } if mapped, status := mapShareError(err); mapped != nil { h.logServerError(err, "create share failed", sessionID) c.JSON(status, mapped) @@ -151,13 +172,26 @@ func (h *HTTPHandlers) httpCreate(c *gin.Context) { // httpList returns every share row for the session, including revoked. func (h *HTTPHandlers) httpList(c *gin.Context) { + taskID := c.Param("id") sessionID := c.Param("sessionId") + if taskID == "" { + c.JSON(http.StatusBadRequest, errorBody{Error: "task id is required"}) + return + } if sessionID == "" { c.JSON(http.StatusBadRequest, errorBody{Error: "session id is required"}) return } - rows, err := h.svc.ListBySession(c.Request.Context(), sessionID) + rows, err := h.svc.ListBySession(c.Request.Context(), taskID, sessionID) + if h.writeAuthorizationFailure(c, err, sessionID) { + return + } if err != nil { + if errors.Is(err, ErrNotFound) { + mapped, status := mapShareError(err) + c.JSON(status, mapped) + return + } h.logServerError(err, "list shares failed", sessionID) c.JSON(http.StatusInternalServerError, errorBody{Error: "failed to list shares"}) return @@ -177,6 +211,9 @@ func (h *HTTPHandlers) httpRevoke(c *gin.Context) { return } if err := h.svc.RevokeShare(c.Request.Context(), shareID); err != nil { + if h.writeAuthorizationFailure(c, err, shareID) { + return + } if errors.Is(err, ErrNotFound) { c.JSON(http.StatusNotFound, errorBody{Error: "share not found"}) return @@ -199,6 +236,8 @@ func mapShareError(err error) (*errorBody, int) { return nil, 0 } switch { + case errors.Is(err, ErrAuthorization): + return &errorBody{Error: shareAuthorizationFailureMessage}, http.StatusInternalServerError case errors.Is(err, ErrSessionNotShareable): return &errorBody{Error: "session has no shareable content yet", Code: "session_not_shareable"}, http.StatusConflict case errors.Is(err, ErrNotFound): @@ -209,6 +248,15 @@ func mapShareError(err error) (*errorBody, int) { return &errorBody{Error: err.Error(), Code: "gist_upload_failed"}, http.StatusBadGateway } +func (h *HTTPHandlers) writeAuthorizationFailure(c *gin.Context, err error, ref string) bool { + if !errors.Is(err, ErrAuthorization) { + return false + } + h.logServerError(err, "share authorization failed", ref) + c.JSON(http.StatusInternalServerError, errorBody{Error: shareAuthorizationFailureMessage}) + return true +} + func (h *HTTPHandlers) logServerError(err error, msg, ref string) { if h.log == nil { return diff --git a/apps/backend/internal/task/share/http_test.go b/apps/backend/internal/task/share/http_test.go index 993e01d80d..3d2532090f 100644 --- a/apps/backend/internal/task/share/http_test.go +++ b/apps/backend/internal/task/share/http_test.go @@ -20,7 +20,7 @@ func init() { gin.SetMode(gin.TestMode) } func newHandlersForTest(t *testing.T, reader TaskReader, backend Backend, authed bool) (*HTTPHandlers, *Service) { t.Helper() repo := newTestRepo(t) - svc := New(repo, reader, backend, nil, "v-test") + svc := New(repo, reader, pairShareAccess{}, backend, nil, "v-test") if mock, ok := backend.(*mockBackend); ok && !authed { mock.accessErr = errors.New("connect a GitHub account to share tasks publicly") } @@ -61,6 +61,48 @@ func TestHTTP_Create_HappyPath(t *testing.T) { } } +func TestHTTP_Create_RejectsMismatchedTaskSession(t *testing.T) { + t.Parallel() + reader := completedSession() + backend := &mockBackend{nextID: "gist-mismatch"} + handlers, _ := newHandlersForTest(t, reader, backend, true) + router := newGinRouter(handlers) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/tasks/other-task/sessions/s-1/shares", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusNotFound { + t.Fatalf("want 404 for mismatched task/session, got %d: %s", rec.Code, rec.Body.String()) + } + if backend.uploads != 0 || len(backend.accessWorkspaces) != 0 { + t.Fatalf("mismatched request reached backend: uploads=%d access=%v", backend.uploads, backend.accessWorkspaces) + } +} + +func TestHTTP_List_RejectsMismatchedTaskSession(t *testing.T) { + t.Parallel() + reader := completedSession() + backend := &mockBackend{nextID: "gist-list"} + handlers, svc := newHandlersForTest(t, reader, backend, true) + router := newGinRouter(handlers) + + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") + if err != nil { + t.Fatalf("create: %v", err) + } + req := httptest.NewRequest(http.MethodGet, "/api/v1/tasks/other-task/sessions/s-1/shares", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusNotFound { + t.Fatalf("want 404 for mismatched task/session, got %d: %s", rec.Code, rec.Body.String()) + } + if strings.Contains(rec.Body.String(), share.ID) || strings.Contains(rec.Body.String(), "gist-list") { + t.Fatalf("mismatched list exposed share metadata: %s", rec.Body.String()) + } +} + func TestHTTP_Create_RejectsCreatedSessionWith409(t *testing.T) { t.Parallel() reader := completedSession() @@ -104,6 +146,96 @@ func TestHTTP_Create_BlocksWhenGitHubUnauthenticated(t *testing.T) { } } +func TestHTTP_Create_HidesAuthorizationInfrastructureErrors(t *testing.T) { + t.Parallel() + reader := completedSession() + backend := &mockBackend{} + handlers, svc := newHandlersForTest(t, reader, backend, true) + svc.authorizer = &recordingShareAccess{taskSessionErr: errors.New("db transient")} + router := newGinRouter(handlers) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/tasks/t-1/sessions/s-1/shares", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("want 500, got %d: %s", rec.Code, rec.Body.String()) + } + var body errorBody + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode: %v", err) + } + if body.Error != "failed to authorize share access" || body.Code != "" { + t.Fatalf("unexpected authorization error body: %+v", body) + } + if strings.Contains(rec.Body.String(), "db transient") { + t.Fatalf("authorization infrastructure error leaked: %s", rec.Body.String()) + } + if backend.uploads != 0 || len(backend.accessWorkspaces) != 0 { + t.Fatalf("authorization infrastructure error reached backend: uploads=%d access=%v", backend.uploads, backend.accessWorkspaces) + } +} + +func TestHTTP_Create_DryRun_HidesAuthorizationInfrastructureErrors(t *testing.T) { + t.Parallel() + reader := completedSession() + backend := &mockBackend{} + handlers, svc := newHandlersForTest(t, reader, backend, true) + svc.authorizer = &recordingShareAccess{taskSessionErr: errors.New("db transient")} + router := newGinRouter(handlers) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/tasks/t-1/sessions/s-1/shares?dry_run=true", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("want 500, got %d: %s", rec.Code, rec.Body.String()) + } + var body errorBody + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode: %v", err) + } + if body.Error != "failed to authorize share access" || body.Code != "" { + t.Fatalf("unexpected authorization error body: %+v", body) + } + if strings.Contains(rec.Body.String(), "db transient") { + t.Fatalf("authorization infrastructure error leaked: %s", rec.Body.String()) + } + if backend.uploads != 0 || len(backend.accessWorkspaces) != 0 { + t.Fatalf("authorization infrastructure error reached backend: uploads=%d access=%v", backend.uploads, backend.accessWorkspaces) + } +} + +func TestHTTP_Create_SecondAuthorization_HidesInfrastructureErrors(t *testing.T) { + t.Parallel() + reader := completedSession() + backend := &mockBackend{} + handlers, svc := newHandlersForTest(t, reader, backend, true) + svc.authorizer = &recordingShareAccess{taskSessionErrs: []error{nil, errors.New("db transient")}} + router := newGinRouter(handlers) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/tasks/t-1/sessions/s-1/shares", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("want 500, got %d: %s", rec.Code, rec.Body.String()) + } + var body errorBody + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode: %v", err) + } + if body.Error != "failed to authorize share access" || body.Code != "" { + t.Fatalf("unexpected authorization error body: %+v", body) + } + if strings.Contains(rec.Body.String(), "db transient") { + t.Fatalf("authorization infrastructure error leaked: %s", rec.Body.String()) + } + if backend.uploads != 0 { + t.Fatalf("second authorization failure reached backend Upload: %d", backend.uploads) + } +} + func TestHTTP_Create_DryRunReturnsSnapshotWithoutUpload(t *testing.T) { t.Parallel() reader := completedSession() @@ -138,7 +270,7 @@ func TestHTTP_List_ReturnsSharesNewestFirst(t *testing.T) { handlers, svc := newHandlersForTest(t, reader, backend, true) router := newGinRouter(handlers) - if _, err := svc.CreateShare(context.Background(), "s-1", "en"); err != nil { + if _, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en"); err != nil { t.Fatalf("create: %v", err) } req := httptest.NewRequest(http.MethodGet, "/api/v1/tasks/t-1/sessions/s-1/shares", nil) @@ -164,7 +296,7 @@ func TestHTTP_Revoke_Success(t *testing.T) { handlers, svc := newHandlersForTest(t, reader, backend, true) router := newGinRouter(handlers) - share, err := svc.CreateShare(context.Background(), "s-1", "en") + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if err != nil { t.Fatalf("create: %v", err) } @@ -180,6 +312,80 @@ func TestHTTP_Revoke_Success(t *testing.T) { } } +func TestHTTP_Revoke_RejectsUnauthorizedShare(t *testing.T) { + t.Parallel() + reader := completedSession() + backend := &mockBackend{nextID: "gist-denied"} + handlers, svc := newHandlersForTest(t, reader, backend, true) + router := newGinRouter(handlers) + + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") + if err != nil { + t.Fatalf("create: %v", err) + } + svc.authorizer = &recordingShareAccess{sessionErr: ErrNotFound} + + req := httptest.NewRequest(http.MethodDelete, "/api/v1/shares/"+share.ID, nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusNotFound { + t.Fatalf("want 404 for unauthorized revoke, got %d: %s", rec.Code, rec.Body.String()) + } + if len(backend.deletes) != 0 { + t.Fatalf("unauthorized revoke called backend Delete: %v", backend.deletes) + } + row, err := svc.GetByID(context.Background(), share.ID) + if err != nil { + t.Fatalf("get share: %v", err) + } + if row.RevokedAt != nil { + t.Fatal("unauthorized revoke mutated the share row") + } +} + +func TestHTTP_Revoke_HidesAuthorizationInfrastructureErrors(t *testing.T) { + t.Parallel() + reader := completedSession() + backend := &mockBackend{nextID: "gist-infra"} + handlers, svc := newHandlersForTest(t, reader, backend, true) + router := newGinRouter(handlers) + + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") + if err != nil { + t.Fatalf("create: %v", err) + } + svc.authorizer = &recordingShareAccess{sessionErr: errors.New("db transient")} + + req := httptest.NewRequest(http.MethodDelete, "/api/v1/shares/"+share.ID, nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("want 500, got %d: %s", rec.Code, rec.Body.String()) + } + var body errorBody + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode: %v", err) + } + if body.Error != "failed to authorize share access" || body.Code != "" { + t.Fatalf("unexpected authorization error body: %+v", body) + } + if strings.Contains(rec.Body.String(), "db transient") { + t.Fatalf("authorization infrastructure error leaked: %s", rec.Body.String()) + } + if len(backend.deletes) != 0 { + t.Fatalf("authorization infrastructure error reached backend Delete: %v", backend.deletes) + } + row, err := svc.GetByID(context.Background(), share.ID) + if err != nil { + t.Fatalf("get share: %v", err) + } + if row.RevokedAt != nil { + t.Fatal("authorization infrastructure error mutated the share row") + } +} + func TestDisplayURL_NormalizesAllFormats(t *testing.T) { t.Parallel() cases := []struct { diff --git a/apps/backend/internal/task/share/provider.go b/apps/backend/internal/task/share/provider.go index 8823d43ceb..6b13f0baaf 100644 --- a/apps/backend/internal/task/share/provider.go +++ b/apps/backend/internal/task/share/provider.go @@ -1,6 +1,7 @@ package share import ( + "errors" "fmt" "github.com/jmoiron/sqlx" @@ -23,16 +24,20 @@ type Config struct { func Provide( writer, reader *sqlx.DB, taskReader TaskReader, + authorizer TaskAccessAuthorizer, githubResolver GitHubClientResolver, log *logger.Logger, cfg Config, ) (*HTTPHandlers, func() error, error) { + if authorizer == nil { + return nil, nil, errors.New("share: task access authorizer is required") + } repo, err := NewRepository(writer, reader, log) if err != nil { return nil, nil, fmt.Errorf("share: provide repository: %w", err) } backend := NewWorkspaceGistBackend(githubResolver) - svc := New(repo, taskReader, backend, log, cfg.KandevVersion) + svc := New(repo, taskReader, authorizer, backend, log, cfg.KandevVersion) h := NewHTTPHandlers(svc, log) // Cleanup is a true no-op — the repository doesn't own its database // connection (the pool is owned by cmd/kandev). diff --git a/apps/backend/internal/task/share/service.go b/apps/backend/internal/task/share/service.go index 3b71e5e686..e7b5005f33 100644 --- a/apps/backend/internal/task/share/service.go +++ b/apps/backend/internal/task/share/service.go @@ -15,28 +15,33 @@ import ( // Service coordinates snapshot construction, backend upload, and persistence. type Service struct { - repo *Repository - taskRepo TaskReader - backend Backend - log *logger.Logger - kandevVer string + repo *Repository + taskRepo TaskReader + authorizer TaskAccessAuthorizer + backend Backend + log *logger.Logger + kandevVer string } // New constructs a Service. All arguments are required. -func New(repo *Repository, taskRepo TaskReader, backend Backend, log *logger.Logger, kandevVer string) *Service { +func New(repo *Repository, taskRepo TaskReader, authorizer TaskAccessAuthorizer, backend Backend, log *logger.Logger, kandevVer string) *Service { return &Service{ - repo: repo, - taskRepo: taskRepo, - backend: backend, - log: log, - kandevVer: kandevVer, + repo: repo, + taskRepo: taskRepo, + authorizer: authorizer, + backend: backend, + log: log, + kandevVer: kandevVer, } } // PreviewSnapshot builds and returns the redacted snapshot that would be // uploaded for the given session, without uploading or persisting anything. // Returns ErrSessionNotCompleted if the session is not yet completed. -func (s *Service) PreviewSnapshot(ctx context.Context, taskSessionID string) (*Snapshot, error) { +func (s *Service) PreviewSnapshot(ctx context.Context, taskID, taskSessionID string) (*Snapshot, error) { + if err := s.authorizeTaskSessionAccess(ctx, taskID, taskSessionID); err != nil { + return nil, err + } return BuildSnapshot(ctx, s.taskRepo, taskSessionID, s.kandevVer) } @@ -47,7 +52,10 @@ func (s *Service) PreviewSnapshot(ctx context.Context, taskSessionID string) (*S // published artifact; the artifact is static, so this is the only point at // which a locale can be chosen. Pass i18n.DefaultLocale when there is no // request to resolve one from. -func (s *Service) CreateShare(ctx context.Context, taskSessionID, locale string) (*Share, error) { +func (s *Service) CreateShare(ctx context.Context, taskID, taskSessionID, locale string) (*Share, error) { + if err := s.authorizeTaskSessionAccess(ctx, taskID, taskSessionID); err != nil { + return nil, err + } workspaceID, err := s.workspaceForSession(ctx, taskSessionID) if err != nil { return nil, err @@ -96,6 +104,9 @@ func (s *Service) RevokeShare(ctx context.Context, shareID string) error { if err != nil { return err } + if err := s.authorizeSessionAccess(ctx, row.TaskSessionID); err != nil { + return err + } if row.IsRevoked() { return nil } @@ -121,7 +132,10 @@ func (s *Service) RevokeShare(ctx context.Context, shareID string) error { // CheckBackendAccess resolves the task's workspace and validates its backend // identity. Dry-run previews intentionally do not call this method. -func (s *Service) CheckBackendAccess(ctx context.Context, taskSessionID string) error { +func (s *Service) CheckBackendAccess(ctx context.Context, taskID, taskSessionID string) error { + if err := s.authorizeTaskSessionAccess(ctx, taskID, taskSessionID); err != nil { + return err + } workspaceID, err := s.workspaceForSession(ctx, taskSessionID) if err != nil { return err @@ -152,7 +166,10 @@ func (s *Service) workspaceForSession(ctx context.Context, taskSessionID string) } // ListBySession returns every share row for a session (including revoked). -func (s *Service) ListBySession(ctx context.Context, taskSessionID string) ([]*Share, error) { +func (s *Service) ListBySession(ctx context.Context, taskID, taskSessionID string) ([]*Share, error) { + if err := s.authorizeTaskSessionAccess(ctx, taskID, taskSessionID); err != nil { + return nil, err + } return s.repo.ListByTaskSession(ctx, taskSessionID) } diff --git a/apps/backend/internal/task/share/service_authz.go b/apps/backend/internal/task/share/service_authz.go new file mode 100644 index 0000000000..9f27cb474d --- /dev/null +++ b/apps/backend/internal/task/share/service_authz.go @@ -0,0 +1,50 @@ +package share + +import ( + "context" + "errors" + "fmt" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// TaskAccessAuthorizer is the narrow task-service authorization surface used +// by share operations. The task service owns identity and workspace policy; +// the share service only decides when that policy must run. +type TaskAccessAuthorizer interface { + AuthorizeTaskSessionAccess(ctx context.Context, taskID, sessionID string) error + AuthorizeSessionAccess(ctx context.Context, sessionID string) error +} + +// ErrAuthorization marks an authorization failure that is not an object +// access denial. HTTP callers must not receive the underlying infrastructure +// error or mistake it for a missing backend credential. +var ErrAuthorization = errors.New("share authorization failed") + +func (s *Service) authorizeTaskSessionAccess(ctx context.Context, taskID, sessionID string) error { + if err := s.authorizer.AuthorizeTaskSessionAccess(ctx, taskID, sessionID); err != nil { + return normalizeAuthorizationError(err) + } + return nil +} + +func (s *Service) authorizeSessionAccess(ctx context.Context, sessionID string) error { + if err := s.authorizer.AuthorizeSessionAccess(ctx, sessionID); err != nil { + return normalizeAuthorizationError(err) + } + return nil +} + +func normalizeAuthorizationError(err error) error { + if err == nil { + return nil + } + if errors.Is(err, ErrNotFound) || + errors.Is(err, repoerrors.ErrTaskNotFound) || + errors.Is(err, repoerrors.ErrWorkspaceNotFound) || + errors.Is(err, models.ErrTaskSessionNotFound) { + return ErrNotFound + } + return fmt.Errorf("%w: %w", ErrAuthorization, err) +} diff --git a/apps/backend/internal/task/share/service_authz_test.go b/apps/backend/internal/task/share/service_authz_test.go new file mode 100644 index 0000000000..f6e1657cfa --- /dev/null +++ b/apps/backend/internal/task/share/service_authz_test.go @@ -0,0 +1,188 @@ +package share + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +type allowShareAccess struct{} + +func (allowShareAccess) AuthorizeTaskSessionAccess(context.Context, string, string) error { + return nil +} + +func (allowShareAccess) AuthorizeSessionAccess(context.Context, string) error { + return nil +} + +type pairShareAccess struct{} + +func (pairShareAccess) AuthorizeTaskSessionAccess(_ context.Context, taskID, sessionID string) error { + if taskID != "t-1" || sessionID != "s-1" { + return ErrNotFound + } + return nil +} + +func (pairShareAccess) AuthorizeSessionAccess(_ context.Context, sessionID string) error { + if sessionID != "s-1" { + return ErrNotFound + } + return nil +} + +type recordingShareAccess struct { + taskSessionCalls int + sessionCalls int + taskSessionErr error + taskSessionErrs []error + sessionErr error +} + +func (a *recordingShareAccess) AuthorizeTaskSessionAccess(context.Context, string, string) error { + call := a.taskSessionCalls + a.taskSessionCalls++ + if call < len(a.taskSessionErrs) { + return a.taskSessionErrs[call] + } + return a.taskSessionErr +} + +func (a *recordingShareAccess) AuthorizeSessionAccess(context.Context, string) error { + a.sessionCalls++ + return a.sessionErr +} + +type countingTaskReader struct { + base TaskReader + getTaskCalls int + getSessionCalls int + listMessageCalls int +} + +func (r *countingTaskReader) GetTask(ctx context.Context, id string) (*models.Task, error) { + r.getTaskCalls++ + return r.base.GetTask(ctx, id) +} + +func (r *countingTaskReader) GetTaskSession(ctx context.Context, id string) (*models.TaskSession, error) { + r.getSessionCalls++ + return r.base.GetTaskSession(ctx, id) +} + +func (r *countingTaskReader) ListMessages(ctx context.Context, sessionID string) ([]*models.Message, error) { + r.listMessageCalls++ + return r.base.ListMessages(ctx, sessionID) +} + +func TestNormalizeAuthorizationError(t *testing.T) { + t.Parallel() + operationalErr := errors.New("db transient") + tests := []struct { + name string + in error + want error + }{ + {name: "nil", in: nil, want: nil}, + {name: "share not found", in: ErrNotFound, want: ErrNotFound}, + {name: "task not found", in: repoerrors.ErrTaskNotFound, want: ErrNotFound}, + {name: "workspace not found", in: repoerrors.ErrWorkspaceNotFound, want: ErrNotFound}, + {name: "session not found", in: models.ErrTaskSessionNotFound, want: ErrNotFound}, + {name: "operational failure", in: operationalErr, want: ErrAuthorization}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := normalizeAuthorizationError(tt.in) + if tt.want == nil { + if got != nil { + t.Fatalf("normalizeAuthorizationError(%v) = %v, want nil", tt.in, got) + } + return + } + if !errors.Is(got, tt.want) { + t.Fatalf("normalizeAuthorizationError(%v) = %v, want %v", tt.in, got, tt.want) + } + }) + } +} + +func TestService_TaskSessionAuthorizationPrecedesSensitiveReads(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + reader := &countingTaskReader{base: completedSession()} + backend := &mockBackend{} + authorizer := &recordingShareAccess{taskSessionErr: ErrNotFound} + svc := New(repo, reader, authorizer, backend, nil, "v") + ctx := context.Background() + + if _, err := svc.PreviewSnapshot(ctx, "t-foreign", "s-1"); !errors.Is(err, ErrNotFound) { + t.Fatalf("preview error = %v, want ErrNotFound", err) + } + if _, err := svc.CreateShare(ctx, "t-foreign", "s-1", "en"); !errors.Is(err, ErrNotFound) { + t.Fatalf("create error = %v, want ErrNotFound", err) + } + if err := svc.CheckBackendAccess(ctx, "t-foreign", "s-1"); !errors.Is(err, ErrNotFound) { + t.Fatalf("backend access error = %v, want ErrNotFound", err) + } + if _, err := svc.ListBySession(ctx, "t-foreign", "s-1"); !errors.Is(err, ErrNotFound) { + t.Fatalf("list error = %v, want ErrNotFound", err) + } + + if reader.getTaskCalls != 0 || reader.getSessionCalls != 0 || reader.listMessageCalls != 0 { + t.Fatalf("authorization denial reached task reader: %+v", reader) + } + if backend.uploads != 0 || len(backend.accessWorkspaces) != 0 { + t.Fatalf("authorization denial reached backend: uploads=%d access=%v", backend.uploads, backend.accessWorkspaces) + } + if authorizer.taskSessionCalls != 4 { + t.Fatalf("task-session authorization calls = %d, want 4", authorizer.taskSessionCalls) + } +} + +func TestService_RevokeAuthorizesBeforeRevokedShortcutAndMutation(t *testing.T) { + t.Parallel() + repo := newTestRepo(t) + backend := &mockBackend{nextID: "gist-1"} + authorizer := &recordingShareAccess{} + svc := New(repo, completedSession(), authorizer, backend, nil, "v") + + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") + if err != nil { + t.Fatalf("create: %v", err) + } + authorizer.sessionErr = ErrNotFound + if err := svc.RevokeShare(context.Background(), share.ID); !errors.Is(err, ErrNotFound) { + t.Fatalf("revoke error = %v, want ErrNotFound", err) + } + if len(backend.deletes) != 0 { + t.Fatalf("denied revoke called backend Delete: %v", backend.deletes) + } + row, err := repo.GetByID(context.Background(), share.ID) + if err != nil { + t.Fatalf("get share: %v", err) + } + if row.RevokedAt != nil { + t.Fatal("denied revoke mutated the share row") + } + + authorizer.sessionErr = nil + if err := svc.RevokeShare(context.Background(), share.ID); err != nil { + t.Fatalf("authorized revoke: %v", err) + } + if len(backend.deletes) != 1 { + t.Fatalf("authorized revoke Delete calls = %d, want 1", len(backend.deletes)) + } + authorizer.sessionErr = ErrNotFound + if err := svc.RevokeShare(context.Background(), share.ID); !errors.Is(err, ErrNotFound) { + t.Fatalf("denied repeat revoke error = %v, want ErrNotFound", err) + } + if len(backend.deletes) != 1 { + t.Fatalf("denied repeat revoke called backend again: %v", backend.deletes) + } +} diff --git a/apps/backend/internal/task/share/service_test.go b/apps/backend/internal/task/share/service_test.go index 706a6b321c..356101ec5e 100644 --- a/apps/backend/internal/task/share/service_test.go +++ b/apps/backend/internal/task/share/service_test.go @@ -81,9 +81,9 @@ func TestService_CreateShare_HappyPath(t *testing.T) { t.Parallel() repo := newTestRepo(t) mock := &mockBackend{nextID: "abc", nextURL: "https://gist.github.com/u/abc"} - svc := New(repo, completedSession(), mock, nil, "test-version") + svc := New(repo, completedSession(), allowShareAccess{}, mock, nil, "test-version") - share, err := svc.CreateShare(context.Background(), "s-1", "en") + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if err != nil { t.Fatalf("create: %v", err) } @@ -111,9 +111,9 @@ func TestService_CreateShare_RejectsPreHistorySession(t *testing.T) { reader := completedSession() reader.session.State = models.TaskSessionStateCreated mock := &mockBackend{} - svc := New(repo, reader, mock, nil, "v") + svc := New(repo, reader, allowShareAccess{}, mock, nil, "v") - _, err := svc.CreateShare(context.Background(), "s-1", "en") + _, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if !errors.Is(err, ErrSessionNotShareable) { t.Fatalf("expected ErrSessionNotShareable, got %v", err) } @@ -128,9 +128,9 @@ func TestService_CreateShare_AllowsRunningSession(t *testing.T) { reader := completedSession() reader.session.State = models.TaskSessionStateRunning mock := &mockBackend{nextID: "g1"} - svc := New(repo, reader, mock, nil, "v") + svc := New(repo, reader, allowShareAccess{}, mock, nil, "v") - share, err := svc.CreateShare(context.Background(), "s-1", "en") + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if err != nil { t.Fatalf("running session should be shareable, got %v", err) } @@ -143,9 +143,9 @@ func TestService_CreateShare_BackendErrorLeavesNoRow(t *testing.T) { t.Parallel() repo := newTestRepo(t) mock := &mockBackend{uploadErr: errors.New("gateway down")} - svc := New(repo, completedSession(), mock, nil, "v") + svc := New(repo, completedSession(), allowShareAccess{}, mock, nil, "v") - _, err := svc.CreateShare(context.Background(), "s-1", "en") + _, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if err == nil { t.Fatal("expected upload error") } @@ -165,9 +165,9 @@ func TestService_RevokeShare_DeletesAndMarksRevoked(t *testing.T) { t.Parallel() repo := newTestRepo(t) mock := &mockBackend{nextID: "abc"} - svc := New(repo, completedSession(), mock, nil, "v") + svc := New(repo, completedSession(), allowShareAccess{}, mock, nil, "v") - share, err := svc.CreateShare(context.Background(), "s-1", "en") + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if err != nil { t.Fatalf("create: %v", err) } @@ -195,8 +195,8 @@ func TestService_CreateShare_FailsClosedWithoutTaskWorkspace(t *testing.T) { reader := completedSession() reader.task.WorkspaceID = "" mock := &mockBackend{} - svc := New(repo, reader, mock, nil, "v") - _, err := svc.CreateShare(context.Background(), "s-1", "en") + svc := New(repo, reader, allowShareAccess{}, mock, nil, "v") + _, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if !errors.Is(err, ErrWorkspaceRequired) || mock.uploads != 0 { t.Fatalf("CreateShare() error = %v, uploads = %d", err, mock.uploads) } @@ -206,9 +206,9 @@ func TestService_RevokeShare_IsIdempotentOnSecondCall(t *testing.T) { t.Parallel() repo := newTestRepo(t) mock := &mockBackend{nextID: "abc"} - svc := New(repo, completedSession(), mock, nil, "v") + svc := New(repo, completedSession(), allowShareAccess{}, mock, nil, "v") - share, err := svc.CreateShare(context.Background(), "s-1", "en") + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if err != nil { t.Fatalf("create: %v", err) } @@ -229,9 +229,9 @@ func TestService_RevokeShare_SurvivesUpstream404(t *testing.T) { t.Parallel() repo := newTestRepo(t) mock := &mockBackend{nextID: "abc"} - svc := New(repo, completedSession(), mock, nil, "v") + svc := New(repo, completedSession(), allowShareAccess{}, mock, nil, "v") - share, err := svc.CreateShare(context.Background(), "s-1", "en") + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if err != nil { t.Fatalf("create: %v", err) } @@ -253,9 +253,9 @@ func TestService_RevokeShare_PropagatesOtherBackendErrors(t *testing.T) { t.Parallel() repo := newTestRepo(t) mock := &mockBackend{nextID: "abc"} - svc := New(repo, completedSession(), mock, nil, "v") + svc := New(repo, completedSession(), allowShareAccess{}, mock, nil, "v") - share, err := svc.CreateShare(context.Background(), "s-1", "en") + share, err := svc.CreateShare(context.Background(), "t-1", "s-1", "en") if err != nil { t.Fatalf("create: %v", err) } @@ -278,9 +278,9 @@ func TestService_PreviewSnapshot_ReturnsRedactedWithoutUpload(t *testing.T) { t.Parallel() repo := newTestRepo(t) mock := &mockBackend{} - svc := New(repo, completedSession(), mock, nil, "v") + svc := New(repo, completedSession(), allowShareAccess{}, mock, nil, "v") - snap, err := svc.PreviewSnapshot(context.Background(), "s-1") + snap, err := svc.PreviewSnapshot(context.Background(), "t-1", "s-1") if err != nil { t.Fatalf("preview: %v", err) } @@ -303,8 +303,8 @@ func TestService_CreateShare_ForwardsLocaleToBackend(t *testing.T) { t.Run(locale, func(t *testing.T) { t.Parallel() mock := &mockBackend{} - svc := New(newTestRepo(t), completedSession(), mock, nil, "v") - if _, err := svc.CreateShare(context.Background(), "s-1", locale); err != nil { + svc := New(newTestRepo(t), completedSession(), allowShareAccess{}, mock, nil, "v") + if _, err := svc.CreateShare(context.Background(), "t-1", "s-1", locale); err != nil { t.Fatalf("CreateShare: %v", err) } if len(mock.uploadLocales) != 1 || mock.uploadLocales[0] != locale { diff --git a/apps/web/e2e/tests/auth/share-authorization.spec.ts b/apps/web/e2e/tests/auth/share-authorization.spec.ts new file mode 100644 index 0000000000..b96efd44b2 --- /dev/null +++ b/apps/web/e2e/tests/auth/share-authorization.spec.ts @@ -0,0 +1,215 @@ +import { expect, type APIResponse } from "@playwright/test"; +import { backendFixture as test } from "../../fixtures/backend"; +import { acceptInvite, createInviteToken, setupAdmin } from "../../helpers/auth"; + +type ShareResponse = { + id: string; + revoked_at?: string; +}; + +type SnapshotResponse = { + messages?: Array<{ blocks?: Array<{ text?: string }> }>; +}; + +async function expectJSON(response: APIResponse, status: number): Promise { + const body = await response.text(); + expect(response.status(), body).toBe(status); + return JSON.parse(body) as T; +} + +async function expectNotFound(response: APIResponse, forbidden: string[]): Promise { + const body = await response.text(); + expect(response.status(), body).toBe(404); + for (const value of forbidden) { + expect(body).not.toContain(value); + } +} + +test.describe.serial("share authorization", () => { + const ADMIN = { email: "share-admin@e2e.dev", password: "adminpass123", displayName: "Admin" }; + const MEMBER_A = { + email: "share-attacker@e2e.dev", + password: "attackerpass123", + displayName: "Member A", + }; + const MEMBER_B = { + email: "share-owner@e2e.dev", + password: "ownerpass123", + displayName: "Member B", + }; + + test.beforeAll(async ({ backend }) => { + await backend.restart({ KANDEV_FEATURES_AUTH: "true" }); + }); + + test.afterAll(async ({ backend }) => { + await backend.restart(); + }); + + test("keeps a member's share routes private from another user", async ({ browser, backend }) => { + test.setTimeout(90_000); + const adminContext = await browser.newContext({ baseURL: backend.frontendUrl }); + const attackerContext = await browser.newContext({ baseURL: backend.frontendUrl }); + const ownerContext = await browser.newContext({ baseURL: backend.frontendUrl }); + + try { + await setupAdmin(adminContext, backend.baseUrl, ADMIN); + const attackerInviteToken = await createInviteToken(adminContext, backend.baseUrl, { + email: MEMBER_A.email, + role: "member", + }); + const ownerInviteToken = await createInviteToken(adminContext, backend.baseUrl, { + email: MEMBER_B.email, + role: "member", + }); + await acceptInvite(attackerContext, backend.baseUrl, attackerInviteToken, MEMBER_A); + await acceptInvite(ownerContext, backend.baseUrl, ownerInviteToken, MEMBER_B); + + const workspaceResponse = await ownerContext.request.post( + `${backend.baseUrl}/api/v1/workspaces`, + { data: { name: "Private Share Workspace" } }, + ); + const workspace = await expectJSON<{ id: string }>(workspaceResponse, 200); + + const githubResponse = await ownerContext.request.put( + `${backend.baseUrl}/api/v1/github/mock/workspace-connections/${workspace.id}`, + { + data: { + source: "pat", + status: "active", + login: "share-owner-mock", + }, + }, + ); + await expectJSON(githubResponse, 200); + + const taskResponse = await ownerContext.request.post( + `${backend.baseUrl}/api/v1/_test/tasks`, + { + data: { + workspace_id: workspace.id, + title: "Private share task", + state: "IN_PROGRESS", + }, + }, + ); + const task = await expectJSON<{ task_id: string }>(taskResponse, 200); + + const sessionResponse = await ownerContext.request.post( + `${backend.baseUrl}/api/v1/_test/task-sessions`, + { + data: { + task_id: task.task_id, + state: "COMPLETED", + completed_at: new Date().toISOString(), + }, + }, + ); + const session = await expectJSON<{ session_id: string }>(sessionResponse, 200); + + const transcriptMarker = "B_PRIVATE_SHARE_TRANSCRIPT_MARKER"; + const messageResponse = await ownerContext.request.post( + `${backend.baseUrl}/api/v1/_test/messages`, + { + data: { + session_id: session.session_id, + type: "message", + content: transcriptMarker, + }, + }, + ); + await expectJSON(messageResponse, 200); + + const sharePath = `/api/v1/tasks/${task.task_id}/sessions/${session.session_id}/shares`; + const ownerPreview = await ownerContext.request.post( + `${backend.baseUrl}${sharePath}?dry_run=true`, + ); + const preview = await expectJSON(ownerPreview, 200); + expect(JSON.stringify(preview)).toContain(transcriptMarker); + + const foreignValues = [task.task_id, session.session_id, transcriptMarker]; + const attackerPreview = await attackerContext.request.post( + `${backend.baseUrl}${sharePath}?dry_run=true`, + ); + await expectNotFound(attackerPreview, foreignValues); + + const attackerPublish = await attackerContext.request.post(`${backend.baseUrl}${sharePath}`); + await expectNotFound(attackerPublish, foreignValues); + + const ownerSharesBeforePublish = await ownerContext.request.get( + `${backend.baseUrl}${sharePath}`, + ); + const emptyShares = await expectJSON<{ shares: ShareResponse[] }>( + ownerSharesBeforePublish, + 200, + ); + expect(emptyShares.shares).toHaveLength(0); + + const ownerPublish = await ownerContext.request.post(`${backend.baseUrl}${sharePath}`); + const share = await expectJSON(ownerPublish, 201); + expect(share.id).toBeTruthy(); + + const attackerList = await attackerContext.request.get(`${backend.baseUrl}${sharePath}`); + await expectNotFound(attackerList, [task.task_id, session.session_id, share.id]); + + const attackerRevoke = await attackerContext.request.delete( + `${backend.baseUrl}/api/v1/shares/${share.id}`, + ); + await expectNotFound(attackerRevoke, [share.id]); + + const ownerSharesAfterDeniedRevoke = await ownerContext.request.get( + `${backend.baseUrl}${sharePath}`, + ); + const activeShares = await expectJSON<{ shares: ShareResponse[] }>( + ownerSharesAfterDeniedRevoke, + 200, + ); + expect(activeShares.shares).toEqual([expect.objectContaining({ id: share.id })]); + expect(activeShares.shares[0]?.revoked_at).toBeUndefined(); + + const otherTaskResponse = await ownerContext.request.post( + `${backend.baseUrl}/api/v1/_test/tasks`, + { + data: { + workspace_id: workspace.id, + title: "Mismatched share task", + state: "IN_PROGRESS", + }, + }, + ); + const otherTask = await expectJSON<{ task_id: string }>(otherTaskResponse, 200); + const mismatchedPath = `/api/v1/tasks/${otherTask.task_id}/sessions/${session.session_id}/shares`; + + const mismatchedPreview = await attackerContext.request.post( + `${backend.baseUrl}${mismatchedPath}?dry_run=true`, + ); + await expectNotFound(mismatchedPreview, foreignValues); + + const mismatchedList = await attackerContext.request.get( + `${backend.baseUrl}${mismatchedPath}`, + ); + await expectNotFound(mismatchedList, [share.id, transcriptMarker]); + + const ownerRevoke = await ownerContext.request.delete( + `${backend.baseUrl}/api/v1/shares/${share.id}`, + ); + const revokeBody = await ownerRevoke.text(); + expect(ownerRevoke.status(), revokeBody).toBe(204); + + const revokedSharesResponse = await ownerContext.request.get( + `${backend.baseUrl}${sharePath}`, + ); + const revokedShares = await expectJSON<{ shares: ShareResponse[] }>( + revokedSharesResponse, + 200, + ); + expect(revokedShares.shares).toEqual([ + expect.objectContaining({ id: share.id, revoked_at: expect.any(String) }), + ]); + } finally { + await ownerContext.close(); + await attackerContext.close(); + await adminContext.close(); + } + }); +}); diff --git a/docs/plans/share-authorization-boundary/plan.md b/docs/plans/share-authorization-boundary/plan.md new file mode 100644 index 0000000000..9f0b6c6be9 --- /dev/null +++ b/docs/plans/share-authorization-boundary/plan.md @@ -0,0 +1,144 @@ +--- +created: 2026-08-25 +status: done +requirements: + - REQ-AUTH-AUTH-001 + - REQ-AUTH-PUBLIC-SHARE-LINKS-001 +system_design: + - ../../specs/auth/system-design/public-share-links.md +legacy_specs: [] +--- + +# Implementation Plan: Share Authorization Boundary + +## Overview + +Before this implementation, the share service read task-session data through +the raw task repository. Therefore, the caller identity did not limit preview +and list operations. The same service omission also left publish and revoke +dependent on a GitHub authorization check. + +This plan adds the task-service authorization boundary first. Then it adds an +auth-project regression test against the assembled auth-enabled server with an +admin setup context and two member contexts: attacker A and owner B. + +## Scope + +### In scope + +- Authorize every user-facing share service method before sensitive reads or + mutations. +- Authorize the task ID, the session ID, and their relationship on nested + share routes. +- Authorize revoke before the revoked-state shortcut and provider call. +- Return `404 Not Found` for foreign, missing, and mismatched objects. +- Preserve the auth-disabled synthetic-identity behavior. +- Add service, handler, and auth-project regression tests. + +### Out of scope + +- Changes to snapshot content or redaction rules. +- Changes to the `task_shares` schema or share lifecycle. +- Changes to GitHub credential selection or provider authorization. +- Changes to the share dialog or responsive UI. +- Hosted share storage, workspace sharing, and team-owned shares. + +## Confirmed root cause (pre-change) + +`backendapp` passed `repos.Task` to `share.Provide`. The share service had no +task access authorizer. `PreviewSnapshot` and `ListBySession` used +caller-supplied session IDs without an ownership check. The HTTP handlers also +ignored the task ID in nested routes. + +A focused temporary test used a real SQLite task repository and a member +identity. The test read a foreign transcript marker through `PreviewSnapshot`. +It also created, listed, and revoked a foreign share through the raw service. +The temporary test passed and was then removed. + +Before this implementation, the assembled GitHub backend blocked foreign +publish and active-share revoke. This provider check was not a valid service +authorization boundary. Preview and list never reached it. + +## Technical approach + +### Share service boundary + +- Add a narrow authorizer interface in `internal/task/share`. +- Require `AuthorizeTaskSessionAccess` and `AuthorizeSessionAccess`. +- Pass the authorizer through `share.New` and `share.Provide`. +- Keep `TaskReader` for snapshot construction after authorization succeeds. +- Change nested service methods to accept both `taskID` and `sessionID`. +- Run the task-session pair check before task, session, message, share, or + provider access. +- In `RevokeShare`, load the share row only to resolve its session ID. Then + authorize before the revoked-state shortcut, provider call, or row update. + +### HTTP contract + +- Read both route parameters in create, preview, and list handlers. +- Pass both IDs to the share service. +- Map task-service not-found sentinels to the existing share `404` response. +- Keep provider and snapshot errors on their current response paths. + +### Startup wiring + +- `backendapp` creates the share handlers. Pass `taskSvc` as the authorizer. +- Keep `repos.Task` as the raw snapshot reader. +- Keep the GitHub workspace authorizer as defense in depth. + +## Tests + +| Acceptance criterion | Evidence | +| --- | --- | +| `AC-AUTH-AUTH-001.7` | Service tests deny foreign and mismatched task-session access before dependencies run. | +| `AC-AUTH-AUTH-001.7` | HTTP tests return 404 and omit transcript, share, and provider details. | +| `AC-AUTH-PUBLIC-SHARE-LINKS-001.2` | Existing preview tests remain green for an authorized caller. | +| `AC-AUTH-PUBLIC-SHARE-LINKS-001.4` | Existing publish and revoke tests remain green for an authorized caller. | + +## E2E tests + +Add `apps/web/e2e/tests/auth/share-authorization.spec.ts` to the isolated auth +project. The test uses the admin context only to create two member accounts. +Member B owns the task, session, transcript marker, and share. Member A +receives 404 for preview, publish, list, and revoke, including a mismatched +task-session pair. The test also proves that B's share remains active. + +This flow covers `AC-AUTH-AUTH-001.7` at the assembled HTTP boundary. + +## Work orders + +- [x] [Task 01: Enforce share service authorization](task-01-enforce-share-service-authorization.md) +- [x] [Task 02: Prove two-user share isolation](task-02-prove-two-user-share-isolation.md) + +## Verification results + +Task 01 and Task 02 are complete. The handler regressions and assembled +member-to-member auth-project regression pass. + +Remediation verification: + +```text +cd apps/backend && go test -tags fts5 ./internal/task/share -count=1 +145 passed + +cd apps/web && pnpm e2e:raw --project=auth tests/auth/share-authorization.spec.ts +1 passed + +make -C apps/backend lint +0 issues + +python3 scripts/lint-spec-files.py --all +All specification files passed. +``` + +## Risks + +- A late authorization check can read transcript data before it returns an + error. +- The revoked-state shortcut can preserve a share-ID existence oracle. +- Broad error mapping can turn provider or database errors into false 404 + responses. +- Constructor changes can leave a test or alternate startup path without an + authorizer. +- An E2E fixture can bypass the same production boundary that the test must + prove. diff --git a/docs/plans/share-authorization-boundary/task-01-enforce-share-service-authorization.md b/docs/plans/share-authorization-boundary/task-01-enforce-share-service-authorization.md new file mode 100644 index 0000000000..479161b59e --- /dev/null +++ b/docs/plans/share-authorization-boundary/task-01-enforce-share-service-authorization.md @@ -0,0 +1,118 @@ +--- +id: "01-enforce-share-service-authorization" +title: "Enforce share service authorization" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-AUTH-AUTH-001 + - REQ-AUTH-PUBLIC-SHARE-LINKS-001 +acceptance_criteria: + - AC-AUTH-AUTH-001.7 + - AC-AUTH-PUBLIC-SHARE-LINKS-001.2 + - AC-AUTH-PUBLIC-SHARE-LINKS-001.4 +system_design: + - ../../specs/auth/system-design/public-share-links.md +--- + +# Task 01: Enforce Share Service Authorization + +## Summary + +Add the task-service access boundary to every user-facing share method. Reject +foreign and mismatched objects before transcript reads, provider calls, or +share mutations. + +## In scope + +- Add the narrow share access-authorizer interface. +- Pass the authorizer through constructors and production startup wiring. +- Pass both nested route IDs into create, preview, list, and backend probes. +- Authorize revoke through its stored session ID. +- Map ownership denials to `404 Not Found`. +- Map authorization infrastructure failures in every share handler to a fixed + generic `500 Internal Server Error` and log the wrapped cause server-side. +- Add focused service and HTTP regression tests before production changes. +- Preserve authorized, synthetic, and internal caller behavior. + +## Out of scope + +- Browser E2E coverage. +- Frontend behavior or copy. +- Snapshot, persistence, and provider-credential changes. + +## Acceptance + +- Every nested share method authorizes the task-session pair before it reads a + dependency. +- Revoke authorizes before the revoked-state shortcut, provider call, and row + update. +- Preview, both publish authorization stages, and revoke do not expose an + authorization infrastructure error or misclassify it as a provider failure. +- Foreign and mismatched requests return 404. Authorized and auth-disabled + requests keep their current successful behavior. + +## Verification + +```bash +cd apps/backend +go test -tags fts5 ./internal/task/share -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/task/share/service.go` +- `apps/backend/internal/task/share/http.go` +- `apps/backend/internal/task/share/provider.go` +- `apps/backend/internal/task/share/service_authz_test.go` +- `apps/backend/internal/task/share/http_authz_test.go` +- `apps/backend/internal/task/share/service_test.go` +- `apps/backend/internal/task/share/http_test.go` +- `apps/backend/internal/backendapp/services.go` + +## Dependencies + +None. + +## Risks + +- Create, preview, list, and backend-access methods must start with the pair + authorization check. +- Revoke must read one share row before it can resolve the owning session. +- Not-found mapping must not hide unrelated storage or provider errors. + +## Parallelism + +`sequential` + +## Inputs + +- `AC-AUTH-AUTH-001.7` in the auth requirements. +- The authorization boundary in the public-share system design. +- `task.Service.AuthorizeTaskSessionAccess` and + `task.Service.AuthorizeSessionAccess`. +- Existing share service and HTTP tests. + +## Results + +Implemented the task-service authorization boundary for preview, publish, +backend access, list, and revoke operations. Nested share routes now pass and +authorize both task and session IDs. Foreign and mismatched access maps to the +existing 404 response, and revoke authorizes before the revoked shortcut, +provider delete, or local mutation. The HTTP handlers now map authorization +infrastructure failures to a fixed generic 500 response and log the wrapped +cause without exposing it to callers. + +Verification: + +```text +cd apps/backend && go test -tags fts5 ./internal/task/share -count=1 +145 passed + +cd apps/backend && go test -tags fts5 -run '^$' ./internal/backendapp +package compiled; no tests selected + +make -C apps/backend lint +0 issues +``` diff --git a/docs/plans/share-authorization-boundary/task-02-prove-two-user-share-isolation.md b/docs/plans/share-authorization-boundary/task-02-prove-two-user-share-isolation.md new file mode 100644 index 0000000000..23a69efe6f --- /dev/null +++ b/docs/plans/share-authorization-boundary/task-02-prove-two-user-share-isolation.md @@ -0,0 +1,101 @@ +--- +id: "02-prove-two-user-share-isolation" +title: "Prove two-user share isolation" +status: done +wave: 2 +depends_on: + - "01-enforce-share-service-authorization" +plan: "plan.md" +requirements: + - REQ-AUTH-AUTH-001 + - REQ-AUTH-PUBLIC-SHARE-LINKS-001 +acceptance_criteria: + - AC-AUTH-AUTH-001.7 + - AC-AUTH-PUBLIC-SHARE-LINKS-001.2 + - AC-AUTH-PUBLIC-SHARE-LINKS-001.4 +system_design: + - ../../specs/auth/system-design/public-share-links.md +--- + +# Task 02: Prove Two-User Share Isolation + +## Summary + +Add an auth-project Playwright test for the complete HTTP route matrix. The test +uses an admin context only for setup, then separate authenticated member A and +member B contexts with real production route wiring. + +## In scope + +- Start the auth project with a file-specific database. +- Invite member A and member B with separate browser contexts. +- Create member B's workspace, task, completed session, transcript marker, and + share. +- Prove that member A cannot preview, publish, list, or revoke B's share data. +- Prove that a mismatched task-session pair returns 404. +- Prove that B can still use the owner paths after A's denied requests. +- Prove that A's denied revoke does not revoke B's share. + +## Out of scope + +- New UI interactions, screenshots, or mobile coverage. +- Real GitHub network access. +- Additional authorization audits outside the share routes. + +## Acceptance + +- Every foreign or mismatched share request made by member A returns 404 without + the transcript marker or share metadata. +- No denied publish creates a share. No denied revoke changes B's active share. +- B can preview, list, publish, and revoke through the same assembled routes. + +## Verification + +```bash +cd apps/web +pnpm e2e:raw --project=auth tests/auth/share-authorization.spec.ts +``` + +## Files likely touched + +- `apps/web/e2e/tests/auth/share-authorization.spec.ts` +- If a reusable account helper is required, `apps/web/e2e/helpers/auth.ts` +- If context-bound seed support is required, `apps/web/e2e/helpers/api-client.ts` + +## Dependencies + +- Task 01 must complete first. + +## Risks + +- Test-only seed routes must use B's authenticated context. +- The test must use the mock GitHub client. It must not call the network. +- File-specific database cleanup must restore the normal backend fixture. + +## Parallelism + +`sequential` + +## Inputs + +- Task 01 service and HTTP behavior. +- `apps/web/e2e/helpers/auth.ts`. +- `apps/web/e2e/tests/auth/auth-lifecycle.spec.ts` two-user patterns. +- The auth-project instructions in `apps/web/e2e/README.md`. + +## Results + +Implemented an auth-project Playwright regression against the assembled +production routes. The admin context only creates two member accounts. Member +B seeds the member-owned workspace, mock GitHub connection, task, completed +session, and transcript through the owner context. Member A receives `404` for +preview, publish, list, and revoke, including a mismatched task-session pair, +without transcript or share metadata. Member B can preview, publish, observe +the active share after the denied revoke, and revoke it. + +Verification: + +```bash +cd apps/web && pnpm e2e:raw --project=auth tests/auth/share-authorization.spec.ts +1 passed +``` diff --git a/docs/specs/auth/system-design/public-share-links.md b/docs/specs/auth/system-design/public-share-links.md index fb95b1bc06..eb1a188500 100644 --- a/docs/specs/auth/system-design/public-share-links.md +++ b/docs/specs/auth/system-design/public-share-links.md @@ -2,6 +2,7 @@ status: draft system: auth requirements: + - REQ-AUTH-AUTH-001 - REQ-AUTH-PUBLIC-SHARE-LINKS-001 created: 2026-05-21 owners: @@ -11,12 +12,19 @@ owners: ## Purpose and boundaries -This design preserves the technical source detail for `REQ-AUTH-PUBLIC-SHARE-LINKS-001` during migration. +This design defines the public-share surface and its authorization boundary. +The auth system owns this contract because a share exposes a task-session +transcript and provider-backed publication controls. + +The task service owns workspace authorization. The GitHub integration owns +credential selection and provider operations. A provider authorization check +is defense in depth. It is not the share service authorization boundary. ## Requirement mapping | Requirement | Design section | | --- | --- | +| `REQ-AUTH-AUTH-001` | [Authorization boundary](#authorization-boundary) | | `REQ-AUTH-PUBLIC-SHARE-LINKS-001` | [Migrated source detail](#migrated-source-detail) | ## Migrated source detail @@ -150,10 +158,10 @@ no user IDs. It is intentionally portable. ## API surface -Endpoints live under the `/api/v1` prefix to match the rest of the kandev -HTTP surface. kandev's HTTP layer has no per-request user middleware -today — authorisation is "this kandev instance can reach this task in its -local store", same as every other task endpoint. +Endpoints live under the `/api/v1` prefix to match the other Kandev HTTP +routes. The global auth middleware supplies the caller identity. The share +service applies object-level authorization before it reads transcript or share +data. ```http POST /api/v1/tasks/:taskId/sessions/:sessionId/shares @@ -189,18 +197,46 @@ type Service struct{ /* unexported */ } func BuildSnapshot(ctx context.Context, repo TaskReader, taskSessionID, kandevVersion string) (*Snapshot, error) -func (s *Service) CreateShare(ctx context.Context, taskSessionID string) (*Share, error) +func (s *Service) CheckBackendAccess(ctx context.Context, taskID, taskSessionID string) error +func (s *Service) CreateShare(ctx context.Context, taskID, taskSessionID, locale string) (*Share, error) func (s *Service) RevokeShare(ctx context.Context, shareID string) error -func (s *Service) ListBySession(ctx context.Context, taskSessionID string) ([]*Share, error) -func (s *Service) PreviewSnapshot(ctx context.Context, taskSessionID string) (*Snapshot, error) +func (s *Service) ListBySession(ctx context.Context, taskID, taskSessionID string) ([]*Share, error) +func (s *Service) PreviewSnapshot(ctx context.Context, taskID, taskSessionID string) (*Snapshot, error) type Backend interface { Name() string - Upload(ctx context.Context, snap *Snapshot) (externalID, externalURL string, err error) - Delete(ctx context.Context, externalID string) error + CheckAccess(ctx context.Context, workspaceID string) error + Upload(ctx context.Context, workspaceID string, snap *Snapshot, locale string) (externalID, externalURL string, err error) + Delete(ctx context.Context, workspaceID, externalID string) error } ``` +## Authorization boundary + +`share.Service` receives a narrow task access authorizer during startup. The +production authorizer is `task.Service`. The share package can keep the raw +task reader for snapshot construction after authorization succeeds. + +Create, preview, list, and backend-access probes accept both route IDs. Each +method calls `AuthorizeTaskSessionAccess` before it reads a task, session, +message, or share row. This check proves ownership and proves that the session +belongs to the supplied task. + +Revoke first loads the share row to resolve its session ID. It then calls +`AuthorizeSessionAccess` before it checks `revoked_at`, calls the provider, or +updates the row. This order prevents an already-revoked share from becoming an +existence oracle. + +A foreign task, a foreign session, and a mismatched task-session pair return +the same `404 Not Found` response. The response does not contain transcript, +share, workspace, or provider details. An authorization denial stops all +provider calls and share mutations. + +A synthetic identity keeps the auth-disabled single-user behavior. An internal +caller with no identity also keeps the existing unscoped service behavior. +The GitHub workspace authorizer remains a second authorization layer for +provider operations. + ### Share URL format The user-facing share URL routes through **gist.githack.com** — a @@ -250,18 +286,18 @@ There is no "draft" or "scheduled" state; publish is synchronous. ## Permissions -- kandev's HTTP layer has no per-request user identity today; the - authorisation contract for share endpoints is the same as for every - other task endpoint — "this kandev instance can see this task in its - local store." If/when kandev grows multi-user, the same row filter that - scopes tasks scopes shares. -- The GitHub credential used for the upload is the credential bound to - the running kandev instance. Shares are owned by that GitHub account, - not by a kandev service account. +- When auth is enabled, users can preview, publish, list, and revoke shares + only for sessions in their own workspaces. +- A route that contains a task ID and a session ID must authorize both IDs and + their relationship. +- The GitHub credential comes from the authorized workspace. The workspace + automation principal owns the published gist. - No "team share" or "delegate to another user" surface in v0. ## Failure modes +- **Foreign or mismatched task-session access** returns `404 Not Found` before + snapshot construction, share listing, provider access, or local mutation. - **Session has no shareable content yet** (state is `CREATED` or `STARTING` — i.e., the agent hasn't run) when POSTing a share → API returns `409 Conflict` with `code: "session_not_shareable"`. The Share @@ -272,6 +308,11 @@ There is no "draft" or "scheduled" state; publish is synchronous. - **No GitHub credential** on the kandev instance → API returns `412 Precondition Failed` with `code: "github_credential_missing"`. The UI surfaces a CTA to the GitHub settings page. +- **Authorization infrastructure failure** during task or session access + resolution → API returns `500 Internal Server Error` with + `error: "failed to authorize share access"` and no error code. The underlying + error is logged server-side and is not exposed as a missing GitHub credential + or a gist-provider failure. - **GitHub API failure during upload** (network, rate limit, 5xx) → API returns `502 Bad Gateway` with the upstream message. No row is written. The user can retry; no partial state is left behind. @@ -307,6 +348,18 @@ There is no "draft" or "scheduled" state; publish is synchronous. ## Scenarios +- **GIVEN** users A and B and a session in B's workspace, **WHEN** A previews, + publishes, or lists shares for that session, **THEN** the API returns 404 and + does not expose B's transcript or share metadata. + +- **GIVEN** users A and B and an active or revoked share for B's session, + **WHEN** A revokes that share, **THEN** the API returns 404 and does not call + the provider or update the share row. + +- **GIVEN** an owned task and a different owned session, **WHEN** a caller uses + both IDs in one share route, **THEN** the API returns 404 before any share + operation. + - **GIVEN** a session past the pre-history states (anything other than `CREATED` / `STARTING`), **WHEN** the task chat panel renders, **THEN** a Share button is visible in the panel header. @@ -388,3 +441,7 @@ There is no "draft" or "scheduled" state; publish is synchronous. with no kandev row). - Embedding rich workspace context (file tree, repo HEAD SHA, agent config). v0 captures only what is needed to render the conversation. + +## Related decisions + +- [Opt-in Authentication and Per-User Workspace Scoping](../../../decisions/2026-07-24-opt-in-authentication.md)