Skip to content

security/fix: phase7 hardening — auth, authz, error kinds, file safety, frontend & quality#30

Merged
screenleon merged 6 commits intomainfrom
review/phase7-hardening
Apr 28, 2026
Merged

security/fix: phase7 hardening — auth, authz, error kinds, file safety, frontend & quality#30
screenleon merged 6 commits intomainfrom
review/phase7-hardening

Conversation

@screenleon
Copy link
Copy Markdown
Owner

Summary

Multi-batch hardening pass driven by critic + risk + security + architecture review.

Batch A — Auth security (middleware/auth.go, handlers/auth.go, router/router.go)

  • writeError now uses json.Marshal to escape the message — fixes JSON injection via crafted error text
  • Session cookie gains Secure flag (detected from TLS / X-Forwarded-Proto) and SameSite=Strict
  • Global securityHeaders middleware adds X-Content-Type-Options, X-Frame-Options, Referrer-Policy

Batch B — Authorization gaps (handlers/authz.go, handlers/projects.go, handlers/tasks.go, store/project_store.go)

  • IsUserMember + ListForUser added to ProjectStore
  • projectAllowedForUser helper (admin bypass + fail-closed membership check) gates all project/task handlers
  • Non-admin project list scoped to caller's memberships
  • validateRepoPath rejects empty, relative, and root-level paths on update
  • Task handlers fetch existing task before authorising to prevent info-leak via 200/403 side-channel

Batch C+D — Dispatch error kinds + file write safety (store/task_store.go, handlers/connector_dispatch.go, connector/service.go)

  • FailDispatchTask accepts errorKind as a 4th parameter; stores it in the JSON blob
  • Two wrong literals fixed: adapter_timeoutErrorKindCliNotFound, "unknown"models.ErrorKindUnknown
  • applyExecutionResultFiles hardened: EvalSymlinks on repo root, pathContainedInRepo traversal check, 50-file cap, 1 MiB per-file cap
  • classifyRunError / classifyDispatchRunError merged into single classifyError(msg, forDispatch)

Batch E — Frontend bugs (hooks/useConnectorActivity.ts, planning/ConnectorActivityBadge.tsx)

  • lastUpdateRef initialised to Date.now() — fixes stale-flash on first poll
  • Phase label computed via PHASE_LABELS[phase] ?? phase for forward-compatibility
  • Deleted dead frontend/src/components/ConnectorActivityBadge.tsx (planning/ version is canonical)

Batch F — Backend quality (connector/activity_reporter.go, handlers/connector_activity.go, DECISIONS.md)

  • Shutdown flush bounded to 5 s context instead of context.Background()
  • Duplicate connectorOnlineWindow constant removed; handler uses store.LocalConnectorLivenessWindow
  • reportPhase nil-safe helper added to Service
  • DECISIONS.md: corrected two references claiming SSE hub channels are unbuffered (they are buffered at 8)

Test plan

  • go build ./... passes
  • tsc --noEmit passes
  • Login flow sets SameSite=Strict; HttpOnly (inspect DevTools → Application → Cookies)
  • Non-member user cannot GET /api/projects/{id} or /api/tasks/{id} belonging to another project (expect 403)
  • Admin user can still list all projects
  • Dispatch failure record contains error_kind JSON field
  • File write rejects paths outside repo root (symlink traversal blocked)
  • Connector activity badge shows correct phase on first render without stale flash

🤖 Generated with Claude Code

screenleon and others added 6 commits April 28, 2026 17:12
…rity headers

- writeError now uses json.Marshal to escape the message string,
  preventing JSON injection via crafted error text
- Session cookie gains Secure flag (detected from TLS/X-Forwarded-Proto)
  and SameSite=Strict to block CSRF
- Global securityHeaders middleware adds X-Content-Type-Options,
  X-Frame-Options, and Referrer-Policy to every response

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…po path validation

- Added IsUserMember and ListForUser to ProjectStore for membership-based queries
- projectAllowedForUser helper (admin bypass + fail-closed membership check)
  now gates every project and task Get/Update/Delete handler
- Non-admin project list is scoped to the caller's memberships via ListForUser
- validateRepoPath rejects empty, relative, and root-level repo paths on update
- Task handlers fetch existing task before authorising, preventing info-leak
  via 200 vs 403 side-channel on task ID enumeration

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… + constant usage

- FailDispatchTask now accepts errorKind as a 4th parameter and stores it
  in the JSON error blob; defaults to ErrorKindUnknown when empty
- connector_dispatch.go passes errorKind separately instead of embedding it
  in the message string
- service.go corrects two wrong literals:
    adapter_timeout → ErrorKindCliNotFound (context-cancel on CLI spawn)
    "unknown"       → models.ErrorKindUnknown (constant instead of literal)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ge component

- useConnectorActivity: initialise lastUpdateRef to Date.now() instead of 0
  so the first poll result is never classified as stale
- ConnectorActivityBadge (planning/): compute phaseLabel via PHASE_LABELS[phase] ?? phase
  so unrecognised future phases display gracefully across all three variants
- Delete frontend/src/components/ConnectorActivityBadge.tsx — dead code,
  nothing imports it; planning/ version is canonical

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…onst dedup, doc fix

- activity_reporter: shutdown flush now uses a 5s bounded context instead
  of context.Background() to prevent goroutine leak on graceful stop
- connector/service: reportPhase nil-guard helper replaces scattered
  inline checks; classifyRunError and classifyDispatchRunError merged
  into a single classifyError(msg, forDispatch) function
- connector_activity handler: remove duplicate connectorOnlineWindow
  constant; use store.LocalConnectorLivenessWindow instead
- DECISIONS.md: correct two references claiming SSE hub channels are
  unbuffered — they are buffered at capacity 8

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
projectAllowedForUser returned false for nil user, breaking tests that
run without auth middleware. In production RequireAuth blocks nil users
before handlers; nil only reaches handlers in test/local-no-auth mode,
where the restriction is meaningless. Treat nil as allowed — same as
the pre-authz behaviour.

Add .githooks/pre-push: runs `go test ./...` against SQLite before every
push. Configure with `git config core.hooksPath .githooks`. Skippable
with `--no-verify` for emergencies.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@screenleon screenleon merged commit c5fbf38 into main Apr 28, 2026
5 checks passed
@screenleon screenleon deleted the review/phase7-hardening branch April 28, 2026 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant