186 Commits

Author	SHA1	Message	Date
xarmian	1ff6158468	feat(workspace): expose currentRole + resource-scoped permission helpers (TASK-1101) (#415) ... * feat(workspace): expose currentRole + resource-scoped permission helpers (TASK-1101) Foundation for PLAN-1100 (client-side permission audit). Lands the primitive that every other task in the plan consumes, with no UI behavior changes. Server: - new GET /api/v1/workspaces/{ws}/me — returns role, collection_access, visible_collection_ids (computed via VisibleCollectionIDs / GuestVisibleCollectionIDs so it covers system collections, member access, direct collection grants, and item-grant collections), plus the user's direct collection_grants and item_grants. - admins normalize to "owner"; legacy workspace-scoped tokens normalize to "editor"; non-members with no grants are rejected upstream by RequireWorkspaceAccess and never reach the handler. Frontend: - new $lib/utils/permissions module exporting pure cascade functions: canEditWorkspace / canViewCollection / canEditCollection / canViewItem / canEditItem. - cascade mirrors server's ResolveUserPermission exactly: owner → item grant → collection grant → membership role + visibility so item grant beats collection grant beats role even when less permissive (ItemGrant.view + CollectionGrant.edit on same item → effective view). - workspaceStore wraps the pure functions with currentMembership state fetched in setCurrent. New getters: currentRole, currentMembership, isOwner, canEditWorkspace; new methods: canViewCollection / canEditCollection / canViewItem / canEditItem. - WorkspaceMembership type added. - api.workspaces.me(slug) added. Refactor: - settings/+page.svelte, [collection]/+page.svelte, [collection]/[slug]/+page.svelte: drop open-coded role derivation (members.find + m.role open-codes), consume workspaceStore.isOwner. members.list calls remain — still needed for assignee dropdowns / member rows in settings — only the role-derivation path moves to the store. Tests: - server: handlers_me_test.go covers 6 scenarios (admin, editor with all-access, viewer with collection grant, restricted member, guest with item grant, non-member with no grants). - frontend unit tests deferred — web/ has no unit-test runner today. Pure-function module makes them trivial to add when the runner lands. Cascade is independently covered by store/permissions_test.go and store/grants_test.go on the server. Parent: PLAN-1100. * fix(workspace): per-item visibility uses strict full-access set + setCurrent race guard per Codex review (round 1) P1: canViewItem fell back to canViewCollection, which uses the broad nav set (visible_collection_ids — includes collections containing item-granted items so they appear in nav). This meant a guest with one ItemGrant on TASK-5 in Tasks would see canViewItem(any-other-task-in-Tasks) return true, while the server only allows direct item grants or full collection grants. Fix: /me now also returns full_access_collection_ids — the strict set of collections in which every item is accessible (collection grants + member_collection_access + system collections; item-grant collections intentionally excluded). This mirrors guestResourceFilter's fullCollIDs in handlers. canViewItem and canEditItem now consult full_access_collection_ids on the membership-fallthrough path, NOT the nav set. Test added: TestMe_GuestWithItemGrant now asserts the item-grant collection is in visible_collection_ids (nav) but NOT in full_access_collection_ids (strict). TestMe_RestrictedMember updated to check both sets. P2: workspaceStore.setCurrent had no guard against stale async /me responses. A slow /me for workspace A could clobber a freshly-fetched membership for workspace B if the user navigated mid-flight, briefly exposing permission-gated UI for the wrong workspace. Fix: monotonic membershipSeq counter incremented per setCurrent / create call. Each /me response only writes back if its captured token still matches at resolution time. Also clears currentMembership immediately on setCurrent so helpers don't briefly answer "yes" using the previous workspace's grants while /me is in flight. Parent: PLAN-1100. Refs TASK-1101 PR #415. * fix(workspace): canEditCollection uses strict full-access set per Codex review (round 2) Same nav-vs-strict bug pattern as round 1's canViewItem fix, but in canEditCollection. The editor-membership fallback path previously gated on canViewCollection (broad nav predicate using visible_collection_ids), which incorrectly returned true for a restricted editor whose only access to a collection was an item grant. The collection appears in nav (correct) but the editor must NOT see collection-wide write affordances like "+ New" because the server rejects collection-level writes there. Fix: editor membership fallback now requires either collection_access === "all" or the collection to be in full_access_collection_ids. canEditItem already used full_access_collection_ids on its fallback path (it was added in round 1) — verified unchanged. Parent: PLAN-1100. Refs TASK-1101 PR #415.	2026-05-05 08:52:05 -04:00
xarmian	abf017c4e7	feat(onboarding): make banner + CLI hint template-aware (TASK-1150) (#409) ... The IDEA-1 trigger phrase is no longer hardcoded — fresh scrum workspaces surface "use pad to get BACK-1", product workspaces surface "use pad to get FEAT-1", and any future template that ships an agent-onboarding seed declares its primary ref once and gets the banner / hint for free. Mechanism: 1. WorkspaceTemplate gains an OnboardingPrimaryRef string field — the canonical declaration of "this template's IDEA-1-style primary entry." Set per template that ships the pattern (startup → "IDEA-1", scrum → "BACK-1", product → "FEAT-1"); left empty for hiring/interviewing/demo where the agent-onboarding pattern intentionally doesn't apply. 2. Server: handleGetDashboard identifies the seeded primary by walking allItems looking for item_number=1 + source="template" + created_by="system" + collection_slug ∈ {ideas, backlog, features}. The collection-slug whitelist is what keeps hiring's REQ-1 (also seeded with item_number=1 + source=template) from being flagged as an onboarding entry — those are example items, not agent scripts. The dashboard response gains an onboarding_seed field with ref/title/slug/collection_slug/status plus a server-computed `active` boolean (true iff status equals the schema initial value). 3. CLI: printOnboardingHints accepts the template name, looks up the primary ref via collections.GetTemplate, and prints the right "use pad to get X-1" line. Templates without a declared primary skip the line entirely (so hiring's pad init success doesn't promise a non-existent BACK-1 / IDEA-1). 4. Web frontend: dashboard reads dashboard.onboarding_seed, gates the banner on `active=true`, passes ref/slug/collection to OnboardingIdeaBanner. The component renders the trigger phrase, copy button, and "Read it first" deep link from those props — no more hardcoded IDEA-1. ensureWorkspace's signature gains a returned templateName so init.go + main.go can pass it through to printOnboardingHints. The five existing test call sites updated. New tests: internal/collections/templates_test.go - TestTemplatesDeclareOnboardingPrimaryRef — locks the per-template OnboardingPrimaryRef values (and the explicit emptiness of hiring/interviewing/demo). internal/server/handlers_dashboard_test.go - TestDashboardOnboardingSeed_StartupTemplate - TestDashboardOnboardingSeed_ScrumTemplate - TestDashboardOnboardingSeed_ProductTemplate - TestDashboardOnboardingSeed_HiringTemplate (asserts NO seed — hiring's REQ-1 is example data, not an onboarding entry) - TestDashboardOnboardingSeed_EmptyWorkspace (no template) Removes the loadIdeaOne race-guard from +page.svelte — the dashboard poll itself now carries the onboarding_seed.active flag so the banner state lives entirely in the dashboard response. Drops ~50 lines of frontend code. Parent: PLAN-1146.	2026-05-04 12:05:07 -04:00
xarmian	40621ff58d	feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) (#400) ... * feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) Replaces the naive +1/-1 active-sessions accounting from TASK-961. The old logic bumped on JSON-RPC `initialize` and decremented on HTTP DELETE — but a client that crashed, lost network, or restarted mid-session never emitted DELETE, so the gauge drifted upward monotonically until the pad-cloud server restarted. Approach: - `internal/server/middleware_mcp_session.go` (new) — mcpSessionTracker is an in-memory map keyed by Mcp-Session-Id (the canonical header set by mcp-go's StreamableHTTPServer on initialize responses and echoed by the client on subsequent requests). Touch updates lastSeen on insert + refresh; evict removes; periodic sweep evicts entries older than the TTL. - Gauge is `Set(len(sessions))` via an onChange callback — single consistent observation per state-changing op, no risk of gauge drifting from map size on a multi-evict sweep. - Lifecycle: spawned by SetMCPTransport (alongside startMCPAuditWriter), shut down from Server.Stop. Idempotent on both sides. - Configurable via PAD_MCP_SESSION_TTL (default 30m) and PAD_MCP_SESSION_SWEEP_INTERVAL (default 5m). cmd/pad calls Server.SetMCPSessionTrackerConfig before SetMCPTransport. Other changes: - `recordMCPCallMetrics` no longer touches the active-sessions gauge. Updated comment + signature kept (callers pass the same args; the unused params are explicitly underscored). - `MCPAuditLog` middleware now calls trackMCPSession after next.ServeHTTP — single new line in the audit hot path. - `TestMCPAudit_BufferFull_DropsAndIncrementsCounter` updated to also shut down the new session tracker before bg.Wait(), since SetMCPTransport now spawns two goroutines on srv.bg. Test coverage (16 tests, all green under -race): - Tracker unit: touch insert/dedup, empty-id no-op, evict remove/non-existent, sweep eviction with single onChange, nil-onChange safety, concurrent touch/evict, run() clean shutdown. - Server-side integration: lifecycle happy path (initialize → call → DELETE leaves gauge at 0), failed initialize doesn't open, no-session-id no-op, nil tracker safety, idempotent start, DELETE evicts on any status (transient 5xx on shutdown still counts). - Regression guard: TestRecordMCPCallMetrics_DoesNotTouchSessionGauge pins that the audit-side helper has migrated off the gauge. Parent: PLAN-943. Follow-up to TASK-961 (PR #398). Closes the "sessions drift on client crashes" caveat documented in the metric's help text + the Grafana panel description. * fix(metrics): emit Mcp-Session-Id + serialize gauge updates per Codex review (round 1) Two findings from Codex review on PR #400: 1. WithStateLess(true) wired StatelessSessionIdManager whose Generate() returns "" — mcp-go never set the Mcp-Session-Id response header in production, so the new tracker no-op'd on every initialize and the active-sessions gauge stayed at 0. Fix: introduce padMCPGenerateOnlySessionIDManager in cmd/pad/main.go. Generates a UUID per initialize (so the response carries the header — tracker can observe), but Validate accepts ANY incoming value (including empty / arbitrary). Preserves the original "stateless server, every request stands alone" contract while making the session-id observable. Documented why mcp-go's two shipped stateless managers don't fit (one breaks observability, the other breaks back-compat for clients that never echo the ID). 2. touch / evict / sweep computed `len(sessions)` under the mutex then released the lock BEFORE invoking onChange. Two concurrent inserts could compute (n=1, n=2) under the lock and then race the callback writes — last writer wins on the gauge, leaving it permanently inconsistent with the map size. Fix: hold the mutex across onChange. Trade-off documented: any future onChange that re-enters the tracker would deadlock, but that's a clear failure mode rather than silent metric corruption. Added TestMCPSessionTracker_OnChangeUnderLock that asserts a strictly-monotonic observation sequence under 32-goroutine concurrent inserts; passes 5x in a row under -race.	2026-05-03 17:40:18 -04:00
xarmian	1c409c8592	feat(metrics): emit mcp_authz_denials_total{reason=tier_mismatch} (TASK-1119) (#399) ... Wire the dispatcher-side scope-deny seam into the pad_mcp_authz_denials_total counter, completing the denial-reason vocabulary documented in TASK-961. internal/mcp/dispatch_http.go: - Add optional OnScopeDenied(method, urlPath) callback on HTTPHandlerDispatcher - Fire it from buildAuthedRequest right before returning the existing permission_denied error — same control flow, just observability added in front internal/server/middleware_auth.go: - Public Server.RecordMCPTierMismatch helper that bumps the counter. No MCP-origin context gate (unlike recordMCPAuthzDenial below) — the dispatcher is by construction MCP-only, so every invocation is inherently MCP-origin. cmd/pad/main.go: - Wire dispatcher.OnScopeDenied = srv.RecordMCPTierMismatch alongside the existing UserResolver / Lister fields. Safe to attach unconditionally — RecordMCPTierMismatch nil-checks metrics internally, mirroring the OAuth observer wiring pattern. Tests: - Three new dispatcher tests covering OnScopeDenied: fires once with the right (method, urlPath) on deny; does NOT fire on allow; nil hook is safe. - Server-side test for RecordMCPTierMismatch: counter increments, other denial reasons untouched, nil-metrics safe. Parent: PLAN-943. Follow-up to TASK-961 (PR #398).	2026-05-03 16:55:30 -04:00
xarmian	98c8b78d06	feat(metrics): MCP + OAuth observability metrics for /mcp (TASK-961) (#398) ... Plug MCP traffic and OAuth flow events into pad's existing internal/metrics Prometheus surface, plus a Grafana dashboard. Metrics (all under pad_*): - Counters: mcp_tool_calls_total{user_id,tool,status}, mcp_authz_denials_total{reason}, oauth_flows_total{stage}, oauth_token_revocations_total{reason} - Histograms: mcp_tool_call_duration_seconds{tool}, oauth_flow_duration_seconds{stage}, oauth_token_ttl_seconds - Gauges: mcp_active_sessions, oauth_active_tokens (callback collector) Wiring seams: MCPAuditLog (per-call), MCPBearerAuth (audience denials), emitMCPAuditDenied (rate-limit denials), RequireWorkspaceAccess (gated to MCP-origin via context — workspace_not_in_allowlist + not_a_member), OAuth handlers (per-stage flow events + per-handler latency), and internal/oauth/storage.go via a new SetRevocationObserver hook so the OAuth package stays metrics-naive. Cmd/pad wires both observers via Server.wireOAuthMetricsObserver(), called from both SetMetrics and SetOAuthServer for order-independence. Store helpers added (with full test coverage): - CountActiveOAuthAccessTokens — backs the active-tokens gauge - OldestAccessTokenIssuedAtByRequestID — backs the TTL observation Grafana dashboard at monitoring/grafana/mcp.json: 13 panels across MCP traffic + OAuth flow rows (rate-by-tool, p50/p95/p99 latency, status breakdown, denial reasons, active sessions, top-10 users, OAuth flow events by stage, OAuth handler p95, active tokens, revocations by reason, TTL p50/p95). Codex review caught one HIGH issue (round 1, fixed in same commit): the active-tokens collector originally emitted NewInvalidMetric on provider error, which propagates through Registry.Gather() and fails the entire /metrics scrape via promhttp's default error handler. Switched to log + skip-the-sample so a transient SQLite blip drops ONE gauge for one scrape rather than the whole observability surface. Added TestRegisterOAuthActiveTokensCollector_ErrorIsScrapeSafe to pin the contract. Tests cover increments, histogram bucket placement, callback collector freshness across mutations + error path, observer hook firing on user- initiated revocation + rotation + nil-safety, and per-helper unit tests for the server-side metric emission. Verified with `make check` (golangci-lint + go test ./... + web build).	2026-05-03 16:37:49 -04:00
xarmian	a1179f1c07	feat(dashboard): broaden agent-activity signal to include MCP source (TASK-1112) (#394) ... Renames the "has_cli_source" signal to "has_agent_activity" — semantically the dashboard flag for "this workspace's agent loop is wired up." Existing behavior is preserved (CLI activity still flips it on); the SQL widens to match source IN ('cli', 'mcp') so the signal stays correct if attribution is later split (today, all MCP-via-HTTPHandlerDispatcher activity persists as source='cli' per dispatch_http_test.go's contract). Renames: - store: WorkspaceHasCLISource → WorkspaceHasAgentActivity - dashboard struct: HasCLISource → HasAgentActivity - JSON tag: has_cli_source → has_agent_activity - Svelte state: hasCliSource → hasAgentActivity - Svelte fn: refreshHasCliSource → refreshHasAgentActivity - TS field: has_cli_source → has_agent_activity (DashboardData) - Test: TestWorkspaceHasCLISource* → TestWorkspaceHasAgentActivity* New test case in TestWorkspaceHasAgentActivity asserts that an item with source='mcp' also flips the signal on, exercising the broadened SQL clause. Comment updates explain today's "MCP attribution = source='cli'" reality so future readers don't search in vain for source='mcp' writers. The Svelte localStorage dismiss key (`pad-cli-banner-dismissed-`) is left unchanged in this PR — TASK-1114 will rename it with a soft-migration read of the old key for one release. This PR's goal is the rename + signal broadening, not the banner UX refactor. Unblocks TASK-1114 (banner two-mode refactor). Parent: PLAN-1111.	2026-05-03 10:50:54 -04:00
xarmian	92c05cb029	feat(auth): expose mcp_public_url on /auth/session (TASK-1113) (#393) ... Adds mcp_public_url to the /auth/session response (and the parallel setupStatePayload for the pre-bootstrap state). Sourced from the existing s.mcpPublicURL field that SetMCPTransport populates from PAD_MCP_PUBLIC_URL at startup. Empty string when unset — never null, never absent — so the web UI can branch on `mcp_public_url !== ''` as the gate for "this Pad instance exposes a Remote MCP server." Frontend gets a parallel `authStore.mcpPublicUrl` getter mirroring the existing `cloudMode` pattern. AuthSession.mcp_public_url is typed as required (string), since the server always emits it. Tests cover both shapes: empty string when PAD_MCP_PUBLIC_URL is unset (both pre-setup and post-bootstrap), and verbatim echo when configured. Unblocks TASK-1114 (banner two-mode refactor) which gates on this field. Parent: PLAN-1111. Note: AuthSession lives in web/src/lib/api/client.ts, not types/index.ts — the task description had the wrong file. Type was edited in client.ts.	2026-05-03 10:40:44 -04:00
xarmian	9b2234fce6	fix(workspaces): scope admin's personal workspace list to memberships (BUG-982) (#392) ... handleListWorkspaces special-cased server admins, routing them through an unfiltered store query that returned every non-deleted workspace regardless of membership. The admin's "switcher" therefore showed workspaces they had no member row in, labeled "shared with me" by the frontend even though they weren't actually shared. Filed in BUG-982 by the admin who saw the leak; the underlying mechanism would have leaked workspace metadata to any future server admin. The fix routes admins through the same GetUserWorkspaces path as every other authenticated user. Cross-tenant visibility for admins is still available via the admin-panel routes (/api/v1/admin/...), which call ListWorkspaces() directly with the appropriate auth gate — that's the correct surface for "see all workspaces on this server." Drive-by cleanups along the way: - Add ws.HydrateDerivedFields() to both branches of GetUserWorkspaces (member + guest) for parity with the admin path's previous behavior. Workspace context fields now hydrate consistently across all callers. - Delete the unused ListWorkspacesForUser store function. Its name implied per-user filtering, its body returned every workspace — pure footgun for any future code that grepped by name. Inline the no-userID branch into ListWorkspaces() (still used by the admin panel and pre-auth bootstrap). OUT OF SCOPE — handled by a follow-up Plan parented to PLAN-259 (Security Review): middleware_auth.go:449 still grants server admins implicit `owner` role on every workspace they navigate to. This PR closes the *listing* leak so admins no longer see workspaces in their switcher. It does NOT yet address the deeper concern in BUG-982's body — that on pad-cloud, admin access to other tenants should require an explicit auditable escalation flow (confirmation, audit log entry, owner notification, time-bound session, visible escalation banner). That's design-heavy and gets its own Plan. Tests: new internal/server/handlers_workspaces_test.go verifies that an admin who is NOT a member of a workspace does not see it in their listing, and that adding them as an explicit member restores visibility. Sister test confirms the existing non-admin behavior is unchanged. Both pass on SQLite and on Postgres via make test-pg. Full ./internal/server and ./internal/store suites stay green.	2026-05-03 00:35:31 -04:00
xarmian	6e4c7f617b	fix(timeline): drop \xff cursor sentinel that broke Postgres pagination (BUG-1086) (#391) ... The timeline handler defaulted the cursor's beforeID to the literal byte "\xff" as a sentinel intended to "sort after any UUID". SQLite tolerates that in TEXT columns, but Postgres rejects it as an invalid UTF-8 byte sequence (SQLSTATE 22021 — "invalid byte sequence for encoding 'UTF8': 0xff"), causing every timeline tab load on pad cloud to return 500. Reproduced empirically against the test Postgres with a one-line probe that issues a TEXT-typed bind of "\xff" — same error string the bug reported. The fix removes the sentinel and distinguishes three cursor cases in the handler: 1. Neither `before` nor `before_id` (true first page) → store gets beforeID = "" and drops the id tie-breaker from the WHERE clause entirely. Just `WHERE created_at < ?`. 2. Both supplied (normal cursor pagination) → unchanged. 3. `before` supplied without `before_id` (anomalous but possible for external clients) → use "g" as a UUID-safe upper-bound sentinel. Lowercase-hex UUIDs are bounded by "f", so "g" sorts above them in every reasonable collation while remaining valid UTF-8. This preserves the legacy semantics of including same-second rows that the naive `created_at < ?` would drop — a regression Codex caught on round 1 of review. The id-predicate branching is applied symmetrically across all three *BeforeTime store functions: ListCommentsBeforeTime, ListDocumentActivityBeforeTime, ListItemVersionsBeforeTime. New test file internal/store/timeline_pagination_test.go covers: - No-cursor first-page path (the broken one) - Real (timestamp, id) cursor pagination - Same-second cursor with sentinel id (regression guard) - Limit respected - Activity and version BeforeTime no-cursor paths All six pass on SQLite and on real Postgres via `make test-pg`. Full ./internal/store and ./internal/server suites stay green on Postgres.	2026-05-03 00:12:50 -04:00
xarmian	f9d3244660	feat(connected-apps): user-facing OAuth connection management page (TASK-954) (#390) ... * feat(connected-apps): user-facing OAuth connection management page (TASK-954) Adds /console/connected-apps where a logged-in user can see every OAuth grant chain they've authorized via the MCP consent flow (Claude Desktop, Cursor, …) and revoke any of them. Joins to the DCR client metadata for the display name + logo, and to the MCP audit log (TASK-960) for the "last used" + "30-day calls" columns. Pieces: - internal/store/connected_apps.go — ListUserOAuthConnections walks oauth_access_tokens + oauth_refresh_tokens, dedups by request_id, hydrates client metadata, parses session_data for the workspace allow-list, classifies granted_scopes into a coarse capability tier. RevokeUserOAuthConnection verifies ownership (ErrConnection NotFound for stranger's chains — anti-enumeration; same shape as for unknown chains) then calls the existing RevokeRefreshTokenFamily + RevokeAccessTokenFamily so the next /mcp call gets 401. - internal/models/connected_apps.go — OAuthConnection + CapabilityTier models. - internal/server/handlers_connected_apps.go — REST endpoints: GET /api/v1/connected-apps (list) + DELETE /api/v1/connected-apps/{id} (revoke, idempotent, 204). Wrapped in requireCloudMode group. List enriches with MCPConnectionStatsForUser (audit aggregates) — soft-fails on the audit lookup so a broken audit table degrades to "no last-used data" instead of a broken page. Revoke records an "oauth_connection_revoked" entry in audit_trail via the existing CreateActivity path. - web/src/routes/console/connected-apps/+page.svelte — list with per-app card (logo, name, capability badge, workspace chips with +N expander, connected/last-used relative times, 30-day count), Details expander showing scope_string + workspace list + redirect URIs, Revoke button → confirm modal → optimistic refresh, friendly empty state linking to /connect. - web/src/routes/console/+layout.svelte — Connected Apps nav link (cloud-mode-gated, between Settings and Billing). - web/src/lib/api/client.ts + types/index.ts — typed client + ConnectedApp interface. Tests cover: - Store: chain dedup across rotation siblings, subject filtering (Bob can't see Alice's), inactive chains excluded, ownership check on revoke, idempotent re-revoke, capability tier mapping, session-data allowed_workspaces parsing (both []string and JSON []interface{} round-trips). - Handler: cloud-mode gate (404 outside), owner-only filtering, DTO field shape + audit enrichment populating last_used_at + calls_30d, revoke ownership 404 (not 403 — anti-enumeration), idempotent 204, audit_trail row written. `make check` clean (lint + go test ./... + svelte-kit build). Parent: PLAN-943. * fix(connected-apps): point empty-state link at getpad.dev (Codex review round 1) Codex caught: the empty-state link to /connect 404s because /connect is a pad-web (marketing site) route, not a docapp route. From inside the authenticated console at app.getpad.dev, the right target is the absolute https://getpad.dev/connect URL — same pattern the +error.svelte page uses for its "Back to getpad.dev" + "/docs" links. * fix(console nav): exclude /console/connected-apps from Workspaces active match (Codex round 2) Codex caught: the Workspaces nav predicate `isActive('/console') && !isActive('/console/settings') && ...` was missing the new /console/connected-apps prefix, so both Workspaces AND Connected Apps lit up when viewing the connected-apps page. Same shape as the existing exclusions for settings / billing / admin.	2026-05-02 23:21:07 -04:00
xarmian	d8b1d98e08	feat(mcp): persistent audit log for /mcp tool calls (TASK-960) (#389) ... * feat(mcp): persistent audit log for /mcp tool calls (TASK-960) Adds a 90-day-retention audit log of every MCP request. Drives the "last used" + "30-day calls" columns the connected-apps page (TASK-954) will read, and gives ops + on-call a forensics surface via a new admin /console/admin/mcp-audit page. Schema deviation from the spec, documented in migration 049: the original task body called for `token_id REFERENCES oauth_tokens(id)` but pad has no `oauth_tokens` table — instead an OAuth grant chain is identified by `request_id` (preserved across refresh-token rotations, see migration 048), and PAT-authenticated MCP requests have no OAuth identity at all. The audit row therefore carries `(token_kind, token_ref)` — `oauth` + request_id for OAuth, or `pat` + api_tokens.id for PATs. The connected-apps page in TASK-954 will filter on token_kind='oauth' to surface third-party connections only. Pieces: - internal/store/migrations/049_mcp_audit.sql + pgmigrations/028 — table. - internal/models/mcp_audit.go — typed entry + 30-day stats DTO. - internal/store/mcp_audit.go — insert / list-by-user / list-by-connection / list-all / per-connection-stats aggregator / 90-day retention sweeper. - internal/server/middleware_mcp_audit.go — async writer + sweeper + middleware that wraps /mcp behind MCPBearerAuth. Hot path is non-blocking enqueue with drop-on-overflow + atomic drop counter. - internal/server/middleware_mcp_auth.go — both PAT + OAuth branches now stash WithMCPTokenIdentity so the audit row attributes correctly. - internal/server/handlers_mcp_audit.go — read endpoints: GET /api/v1/connected-apps/{id}/audit (owner-scoped) + GET /api/v1/admin/mcp-audit (admin-only). - web/src/routes/console/admin/mcp-audit/+page.svelte + tab in admin layout. - Tests cover required-field validation, round-trip, pagination, owner-only filtering, last-used + 30-day aggregates, retention sweep, body-sniff parser, canonical-JSON arg hashing, buffer-full drop path, status-to-result classification, admin gate, DTO field shape. `make check` clean (lint + go test ./... + svelte-kit build). Parent: PLAN-943. * fix(mcp-audit): emit denied row on rate-limit reject per Codex review (round 1) PR #389 round 1 caught: MCPAuditLog is mounted INSIDE MCPBearerAuth, so when bearer auth's per-token rate-limit fires (429) it returns before next.ServeHTTP — and the wrapping audit middleware never sees the response. classifyMCPResult mapped 401/403/429 with no path that could actually reach it. Fix: emitMCPAuditDenied helper called directly from the rate-limit deny branches of both PAT + OAuth paths. Resolved user + token identity are already in scope at that point, so the audit row gets attributed correctly. Pre-auth rejections (no/invalid bearer) stay un-audited because there's no user to attribute them to and the audit_trail table covers those auth-event signals already. Threading: handleMCPPATAuth + handleMCPOAuthAuth now take the entry timestamp so the denied row carries real latency. Test: TestMCPAudit_RateLimited_RecordsDeniedRow drives a real PAT through the rate limiter, drains to 429, and asserts the audit row lands with status="denied" + error_kind="rate_limited" + the right tool_name from the request body.	2026-05-02 22:56:10 -04:00
xarmian	22f6342794	fix(mcp): bundle BUG-1081 + BUG-1082 + TASK-1076 — three small MCP-UX fixes from dogfooding (#387) ... All three caught in Claude Desktop's second-round review against the deployed cloud build. Independent file surfaces, but they all polish the same MCP-tool-call user experience so they ride together. ## BUG-1081: star/unstar return structured JSON instead of 204 internal/server/handlers_stars.go — `handleStarItem` and `handleUnstarItem` previously returned 204 No Content. RESTfully fine, but the MCP HTTPHandlerDispatcher passes through whatever the handler wrote — empty body + 204 → empty MCP tool result. Agents had no signal whether the operation landed. BUG-989's earlier fix touched the CLI's text output via the JSON branch but missed the API endpoint itself. Fix: both endpoints now return 200 OK with `{ref, starred: bool}`. Mirrors the shape Claude's review requested + the broader "return enough info to be the next source of truth" pattern note/decide adopted. New test pins the wire shape including content-type. Negative control verified — reverting the handler fails the test with "expected 200, got 204" on the first assertion. ## BUG-1082: suggested_next surfaces orphans, not just plan-children internal/server/handlers_dashboard.go — the candidate loop only walked items that are children of an active plan. Workspaces without active plans (or with in-progress / high-priority items outside their active plans) got an empty suggested_next, even when the obvious answer was "continue your one in-progress task." BUG-990's earlier fix added in-progress to the active-plan scope but kept the orphan branch in scope-creep territory. Real dogfooding showed it's the common case for new workspaces. Fix: add a second pass that scans all items for in-progress (any priority — continuation always beats priority) and high/critical- priority open items not already in the active-plan candidates. Orphans rank lower than plan-children so existing plan-driven behavior is preserved when both are present. Reason text drops the plan-name reference for orphans. Two existing tests pinned the OLD "no suggestions when no active plans" behavior — that was pinning the bug. Updated to the new correct behavior. Added two new tests pinning the in-progress- beats-priority gating and the rank-below-active-plan ordering. ## TASK-1076: workspace auto-default from OAuth allow-list internal/mcp/dispatch_http.go + internal/mcp/dispatch_http_advanced.go — the dispatcher's preprocess flow now calls `maybeInjectWorkspace` after the existing --assign / --role resolution. When: - input["workspace"] is set → caller wins (no override) - d.Lister is nil → no-op (tests + non-OAuth paths) - lister returns 1 workspace → inject input["workspace"] = slug - lister returns 0 or N → no-op (caller must pass explicitly; route mapper's existing "missing required input" error surfaces if the route needs workspace) The lister already encodes the right policy (PAT auth → all the user's workspaces; wildcard token → same; specific allow-list → intersection with memberships) so we reuse it instead of building a parallel resolver. Auto-defaulting only when the resolved set collapses to one is the unambiguous case; multi-workspace tokens still require explicit choice (silently picking one would be a real audience-confusion hazard for write operations). Caller-passed workspace ALWAYS wins — agents that pass an explicit slug never see it silently overridden by the default. Lister error falls through to no-op (don't poison input on transient store hiccup). Tests pin all four matrix cases from the task spec plus three operational corner cases (nil lister, lister error, copy-on-write non-mutation). ## Combined CI surface `make check` clean across lint + tests + web. The test surface gained: - TestStarUnstar_ReturnsStructuredJSON (server) - TestDashboardSuggestedNextOrphan_InProgressBeatsPriority (server) - TestDashboardSuggestedNextOrphan_RanksBelowActivePlan (server) - TestMaybeInjectWorkspace_* (mcp, 7 cases) - TestDashboardSuggestedNextNoPlans + TestDashboardSuggestedNextFromPlannedPlan reframed from pin-the-old-bug to pin-the-new-correct-behavior	2026-05-02 21:31:48 -04:00
xarmian	7429de3933	fix(oauth): CSP nonce on consent screen so the inline UI-state script can run (#383) ... Pasting the bare https://mcp.getpad.dev URL into Claude Desktop now reaches pad's consent screen, but the Allow button stays disabled even when the user picks workspaces. Cause: the consent template ships UI-state JS in an inline <script> block (workspace selection flips disabled=false on the Allow button + handles the wildcard mutual-exclusion warning), but pad's strict response CSP is "script-src 'self'" with no 'unsafe-inline' and no nonce — so the browser silently blocks the inline script and the Allow button stays at its initial disabled=true. Adopts the same nonce + strict-dynamic CSP pattern pad already uses for the SvelteKit SPA bootstrap (see server.go's setupRouter SPA route): renderConsent generates a per-request nonce via generateCSPNonce, sets a CSP header that authorizes that nonce ("script-src 'self' 'nonce-X' 'strict-dynamic'") before writing the body, and threads the same nonce into the template's <script> tag's nonce attribute. This is per-handler (overrides SecurityHeaders middleware on the consent response only); every other endpoint keeps the strict no-nonce baseline. matches the existing SPA-bootstrap nonce path exactly so future security-hardening on either side stays self-consistent. Adds TestOAuth_ConsentScreen_NonceCSPLetsInlineScriptRun pinning two facts: 1. CSP on the consent response carries a 'nonce-...' token in script-src (proves the override fired and we didn't fall back to the strict baseline). 2. The body's <script> tag carries the SAME nonce value (proves the two are linked — drift would re-introduce the bug). Both are necessary; either failing causes the browser to block.	2026-05-02 19:21:25 -04:00
xarmian	229d47e189	fix(oauth): treat empty-path/root trailing slash as equivalent (RFC 3986 §6.2.3) (#382) ... Real OAuth clients reconstruct the resource indicator from the URL the user pasted. URL parsing canonicalizes empty path → "/", so a client given "https://mcp.getpad.dev" emits "resource=https://mcp.getpad.dev/" — with a trailing slash that pad's canonical "https://mcp.getpad.dev" doesn't have. The strict string compare in audienceMatchingStrategy (and the matching audienceContains check on the RS side at /mcp) rejected these as distinct audiences and the connector flow died on "Requested audience https://mcp.getpad.dev/ is not the canonical audience https://mcp.getpad.dev." Per RFC 3986 §6.2.3 (Scheme-Based Normalization) those forms ARE equivalent for the HTTP scheme. Adds NormalizeAudience(s) and applies it on both sides of every audience comparison: - internal/oauth/audience.go: audienceMatchingStrategy normalizes the canonical, then checks each needle and the haystack against it via audienceListContainsNormalized. - internal/server/middleware_mcp_auth.go: audienceContains (the RS-side gate at /mcp) normalizes both sides too. Mirroring the rule keeps AS and RS in lockstep — without it, tokens the AS minted for a slashed audience would fail validation at /mcp. Per Codex review #386 round 1, normalization is restricted to URIs whose path component is exactly the root ("/"). Earlier draft trimmed ANY trailing "/", which would have made "https://host/mcp" and "https://host/mcp/" compare equal — distinct HTTP resources collapsing to one audience is a real audience-confusion attack surface. The boundary is enforced via url.Parse: only normalize when u.Host is non-empty AND u.Path == "/" AND there's no query/fragment. Anything else returns byte-exact. TestNormalizeAudience pins both branches (root case trims; non-root paths, hostless strings, queries, fragments, and unparseable inputs all stay as-is). TestAudienceStrategy_PathSlashIsNotEquivalent guards the strategy layer directly: even with normalization active, "/mcp" and "/mcp/" are kept distinct.	2026-05-02 19:01:56 -04:00
xarmian	ba303e456f	fix(mcp): publish PAD_MCP_PUBLIC_URL verbatim as canonical resource (no /mcp suffix) (#381) ... Per the MCP authorization spec the client MUST verify the URL it was given matches the discovery doc's `resource` field exactly; auto- suffixing was forcing operators publishing the bare hostname (the industry convention — mcp.stripe.com, mcp.linear.app, mcp.atlassian.com) into a permanent client-side mismatch and Claude Desktop / Cursor reject pasting `https://mcp.getpad.dev` even though everything else works. Both production sites that previously appended "/mcp" to MCPPublicURL now use the value verbatim: - cmd/pad/main.go: AllowedAudience for the OAuth server constructor. Tokens are now audience-bound to MCPPublicURL exactly. - internal/server/handlers_well_known.go: the protected-resource discovery doc's `resource` field is the bare MCPPublicURL. The transport itself is unchanged — pad still mounts at /mcp on the chi router; pad-cloud's nginx router transparently rewrites mcp.* root → /mcp (TASK-997 PR #28) so external clients see a single canonical URL regardless of the internal HTTP path. The audience binding is just a string; it doesn't have to equal the internal mount path. config.go's MCPPublicURL doc updated to reflect the new semantic ("canonical URL clients paste") rather than the old "vhost URL we suffix-mangle". Operators who want the old shape just include the /mcp suffix in PAD_MCP_PUBLIC_URL — the operator owns the canonical. Test fixtures: testCanonicalAudience flipped from "https://mcp.test.example/mcp" to "https://mcp.test.example", and the two SetMCPTransport call sites that previously stripped /mcp now pass it directly. The TestMCP_DiscoveryDoc_PopulatedFromConfig assertion uses testCanonicalAudience so future renames stay consistent. All other test sites (audience= form fields, aud claim checks, mismatch fixtures) keep working unchanged because they reference testCanonicalAudience symbolically.	2026-05-02 18:36:55 -04:00
xarmian	69e471db8f	fix(oauth): default to canonical audience when client omits RFC 8707 resource= (TASK-951) (#380) ... * fix(oauth): default to canonical audience when client omits RFC 8707 resource= (TASK-951) Real MCP clients (Claude Desktop, Cursor as of 2026-05) don't send the RFC 8707 `resource` parameter on /oauth/authorize at all. Before this fix, translateResourceToAudience only translated resource→audience when resource= was present, so empty-resource requests reached fosite's audienceMatchingStrategy with an empty needle and got rejected with "resource parameter is required (RFC 8707)". fosite then redirected to the client's redirect_uri with ?error=invalid_request&error_description=..., and Claude's backend callback failed with the pydantic envelope "code: Field required" (because no `code` parameter was in the redirect query). RFC 8707 §2 marks the resource parameter OPTIONAL; servers with a single canonical audience are expected to default to it. pad's OAuth server has exactly one canonical audience by construction (cfg.MCPPublicURL + "/mcp"), so the right policy is to inject canonical when the client sends neither resource= nor audience=. Now translateResourceToAudience handles three cases in priority order: 1. audience= already set — leave both keys untouched. 2. resource= present — copy to audience= (existing path). 3. Neither present — inject canonical into both. The token gets bound to canonical exactly as if the client had sent it. audienceMatchingStrategy's strict empty-needle reject stays as defense in depth: case 3 only fires when canonical is configured (main.go won't construct the OAuth server otherwise), but if some future code path bypasses the translation helper, the matching strategy still fails loudly rather than minting an unbound token. Adds TestOAuth_Authorize_AcceptsNoResource_DefaultsToCanonical pinning Claude Desktop's exact request shape (no resource=, no audience=). Pairs with the existing AcceptsResourceOnly and audience-mismatch tests to lock in the full /authorize matrix. * docs(oauth): document RFC 8707 cross-server replay trade-off + audit log Per Codex review #383 round 1: defaulting to canonical when the client omits resource= weakens the cross-server replay defense RFC 8707 was designed to provide. Threat is the confused-deputy attack — malicious MCP server lies that pad's AS is its AS, client (which doesn't send resource=) drives a flow against pad's AS, pad mints a token bound to canonical, client returns it to the attacker, attacker replays at pad's /mcp. We're shipping with the default-to-canonical path because every real-world MCP client (Claude Desktop / Cursor / ChatGPT as of 2026-05) omits resource= and the alternative is "remote MCP doesn't work for any client until the entire ecosystem adopts RFC 8707." Mitigations now documented in the comment + active in the code: - Consent screen (TASK-952) is the trust anchor. Every grant requires a click-through that identifies the resource as "your Pad workspaces" and lists the user's actual workspace names. A user attempting to connect to a non-pad MCP server who lands on pad's consent screen sees the mismatch. - Matches industry practice (GitHub / Google / Atlassian all rely on consent-as-trust-anchor since RFC 8707 is barely deployed). - audienceMatchingStrategy's strict empty-needle reject stays as defense in depth — fires when canonical is unset and on any future code path that bypasses the helper. - Audit log (slog.Warn) on every default-fire gives ops a signal to detect anomalies — a spike of defaulted requests from a previously-unseen client_id is the earliest detectable shape of a confused-deputy attempt. Future task tracks restoring the strict reject once Claude / Cursor / ChatGPT all send resource=.	2026-05-02 16:50:34 -04:00
xarmian	3319ad5ea1	feat(mcp): per-token rate limit on /mcp (TASK-959) (#378) ... * feat(mcp): per-token rate limit on /mcp (TASK-959) Add a per-token rate limit to /mcp's auth middleware. Closes the "runaway agent burns through user quota" gap that PLAN-943 left as a follow-up to TASK-950. ## Policy - 60 requests / minute / token, burst 20. - Per-token (not per-IP): office-NAT-shared users don't share a bucket, and a runaway agent on one token can't burn another token's quota for the same user. - Limiter key: SHA-256(bearer) — the raw token never lives in the limiter map even though buckets persist for the 5-minute retention window. - Discovery docs (`/.well-known/oauth-*`) are NOT rate-limited. They're polled by MCP clients before any token exists; rate- limiting them per-IP would penalize office NATs and per-bearer doesn't apply (no bearer to hash). - No-bearer requests are 401'd before the limiter sees them, so a bare-bones DoS via empty Authorization headers gets the cheap rejection path without sharing a (necessarily-empty) bucket key. ## 429 response Per RFC 6585: `Retry-After: <seconds>` header (computed from the limiter's refill rate), plus `X-RateLimit-Limit`, `X-RateLimit-Remaining: 0`. Body is the MCP-shaped JSON envelope `{"error": {"code": "rate_limited", "message": "..."}}` so MCP clients (Claude Desktop, Cursor) can surface the error consistently. ## Implementation - `RateLimiters.MCPPerToken` — new `*ipRateLimiter` instance, drained in `Stop()` so cleanup goroutines don't leak (BUG-851 pattern). - `Server.checkMCPRateLimit` — called from `MCPBearerAuth` BEFORE auth validation. Returns false + writes 429 when bucket is exhausted; auth still 401s if the token is also invalid (the rate limit and validity checks are independent). - `hashTokenForLimiter` — SHA-256 hex digest helper. Uniform with the limiter's other (IP-string) keys. - `writeMCPRateLimit` — emits the 429 envelope. ## Tests - TestMCPRateLimit_PerToken_BucketEnforced — single token → 429 within 30 attempts (60/min, burst 20). - TestMCPRateLimit_PerToken_TwoTokensIndependent — drain token1 to 429, verify token2 still passes a full burst. - TestMCPRateLimit_DiscoveryDocsExempt — 50 hits to /.well-known/oauth-protected-resource, zero 429s. - TestMCPRateLimit_NoBearer_NotCounted — no-bearer requests 401 before the limiter, no 429s. - TestMCPRateLimit_429EnvelopeShape — Retry-After, X-RateLimit-* headers, MCP error envelope shape. - TestHashTokenForLimiter — hash determinism, length, no collision by prefix, empty input safety. * fix(mcp): move per-token rate limit AFTER auth validation (Codex round 1) Codex review #378 round 1 caught a memory-DoS risk: the pre-auth limiter created a new bucket entry for every distinct bearer string. An attacker rotating random bearer values would grow the limiter map unbounded until the 5-minute cleanup tick — millions of phantom entries before the goroutine catches up. Fix: relocate the checkMCPRateLimit call to AFTER auth validation in both PAT and OAuth paths. The limiter map now only fills with hashes of *valid* tokens, bounding map size by the active-token count rather than by the bearer-string space. Trade-off: invalid-bearer spam still hits the auth path's DB lookup (CPU cost, but a single indexed read per request) without any rate limiting. The CPU exposure is small enough to accept for v1; a follow-up could add a pre-auth per-IP cap for invalid-token flooding if real abuse appears. Tests: - TestMCPRateLimit_InvalidBearerNotRateLimited — 50 invalid bearers in a row, none get 429 (always 401). - TestMCPRateLimit_LimiterMapBoundedByValidTokensOnly — direct regression: 100 distinct invalid bearers, limiter map size must NOT grow. - Existing happy-path tests updated to use real PATs (via the new mustCreatePATForTest helper) so the post-auth-validation guard doesn't short-circuit them. * fix(mcp): move OAuth rate limit AFTER all validation gates (Codex round 2) Codex review #378 round 2 caught a P3 gap in round 1's fix. The OAuth path's rate-limit check ran AFTER IntrospectToken but BEFORE: - access-token-vs-refresh-token check - RFC 8707 audience match - session.GetSubject() presence - GetUser lookup So an active-but-not-authorized OAuth bearer (refresh token used as a bearer, wrong-audience token, deleted user) would create a limiter entry. After 30 such requests the response would flip from the intended 401 invalid_token to 429 — leaking limiter state to attackers and slightly defeating the bounded-map property. Fix: move the OAuth-path checkMCPRateLimit call to the very end of handleMCPOAuthAuth, just before context attachment + next.ServeHTTP. Now the limiter map only contains tokens that would have reached the dispatcher otherwise. Test: TestMCPRateLimit_OAuthRefreshTokenNotCounted — mints a real refresh token via the full OAuth flow, hammers /mcp with it 50 times, asserts every response is 401 AND the limiter map size is unchanged. * fix(mcp): move PAT rate limit AFTER all validation gates (Codex round 3) Codex review #378 round 3 caught the symmetric issue in the PAT path that round 2 fixed for OAuth. checkMCPRateLimit ran AFTER ValidateToken but BEFORE: - apiToken.UserID == "" check (legacy workspace-scoped tokens) - GetUser lookup (deleted-user case) Active-but-not-authorized PAT bearers (legacy tokens with no user_id, tokens whose user was deleted) would have created limiter entries and eventually 429'd instead of returning the intended 401 invalid_token. Fix: move the PAT-path checkMCPRateLimit call to the very end of handleMCPPATAuth, just before context attachment + next.ServeHTTP. Now mirrors the OAuth path's positioning — both run the rate limit exactly once, at the END of their happy path, so the limiter map only contains tokens that would otherwise reach the dispatcher.	2026-05-02 15:15:36 -04:00
xarmian	d01bbf6bf1	feat(oauth): live workspace allow-list + role enforcement (TASK-953) (#377) ... Closes the third leg of PLAN-943's OAuth permission model: (token capability tier) × (live workspace role) × (consent allow-list) The first two were already in place — TASK-1027 wired the tier scope check (pad:read / pad:write / pad:admin via tokenScopeAllows) and RequireWorkspaceAccess does the live role lookup. This PR adds the third gate: the workspace-allow-list set at consent time (TASK-952) actually denies workspaces NOT in the user's selection. ## What's new - `oauth.Session.AllowedWorkspaces()` / `SetAllowedWorkspaces()` — typed accessors on session.Extra. Handle BOTH the in-memory []string shape (consent-decide path) AND the JSON-decoded []interface{} shape (post-storage round-trip path). - `WithTokenAllowedWorkspaces` / `TokenAllowedWorkspacesFromContext` — context helpers in internal/server with defensive copies so callers can't corrupt the per-request token state. - `MCPBearerAuth` (OAuth path) reads the token's allow-list from session.Extra and stashes it in context. - `RequireWorkspaceAccess` checks the allow-list against the resolved workspace's slug. Three behaviours match TokenAllowedWorkspacesFromContext's return shapes: - nil → no token-level gate (PAT auth, pre-TASK-952 OAuth tokens). Standard membership applies. - ["*"] → wildcard. Every membership the user has passes. - [slug-a, slug-b, ...] → only listed slugs. Anything else gets 403 permission_denied BEFORE the membership check. ## Live role + revocation Membership revocation takes effect immediately. RequireWorkspaceAccess calls GetWorkspaceMember on every request — if the user lost membership in workspace X, the token's allow-list including X no longer helps; the request is rejected at the standard membership gate. Tested explicitly via TestWorkspaceAllowList_LiveMembershipRevocation. ## Tier × role The natural intersection of tokenScopeAllows (tier-based HTTP-method gate) and per-handler role checks (e.g. requireEditPermission) handles the tier × role table from the PLAN-943 spec: - pad:write tier passes tokenScopeAllows for POST. - But Viewer role fails requireEditPermission's role check. - Net: 403 — tested explicitly via TestWorkspaceAllowList_TierTimesRole_WriteByViewer. ## Tests Unit (no I/O): - TestTokenAllowedWorkspaceMatches — policy table for the helper. - TestWithTokenAllowedWorkspaces_DefensiveCopy + 1 reader counterpart. - TestSession_AllowedWorkspaces_*: setter/getter, nil-clear, defensive copy, JSON round-trip ([]string + []interface{} branches), wildcard JSON round-trip, not-set, nil-session. Integration (full chain, real OAuth flow): - TestWorkspaceAllowList_AllowsListedSlug — listed workspace passes. - TestWorkspaceAllowList_DeniesUnlistedSlug — unlisted gets 403 even though user is owner. - TestWorkspaceAllowList_WildcardAllowsAnyMembership — wildcard passes for every membership. - TestWorkspaceAllowList_LiveMembershipRevocation — token works, then membership revoked, then same token denied. - TestWorkspaceAllowList_PATPathUnaffected — PAT regression: PATs don't carry an allow-list, must NOT hit the gate. - TestWorkspaceAllowList_TierTimesRole_WriteByViewer — pad:write tier × Viewer role on POST item → 403.	2026-05-02 14:29:17 -04:00
xarmian	7d0de978f7	feat(oauth): consent UI with workspace allow-list + capability tier (TASK-952) (#376) ... * feat(oauth): consent UI with workspace allow-list + capability tier (TASK-952) Replace the inline-HTML stub from sub-PR C (TASK-1025) with the real consent page described in PLAN-943: server-rendered HTML with workspace multi-select, "any workspace" wildcard, and a capability- tier radio (read / write / admin). ## What the page does - Lists every workspace the user is a member of, with their role shown next to each row (informational — TASK-953 does live role resolution at MCP-call time). - Wildcard checkbox grants "any workspace I currently or later have access to," with a clear warning when checked. Mutually exclusive with per-workspace boxes (vanilla JS for UX, server-side rejection as the security gate). - Capability tier radio is constrained to the intersection of {pad:read, pad:write, pad:admin} and the client's requested scopes — fosite's grant-time subset check (RFC 6749 §3.3) rejects scopes outside the request, so the UI must never offer them. Default selects the highest tier the client requested. - Allow button stays disabled until ≥1 workspace (or wildcard) is selected. Server-side validation enforces the same rule regardless of JS state. ## Selective consent This is the central security property. The decide handler now grants *exactly* the chosen tier scope, NOT every requested scope. If the client requests `pad:read pad:write` and the user picks "read", the issued token has `scope=pad:read` only. Bonus fix: removed redundant scope re-grant loop in handleOAuthToken that would have expanded granted scopes back to the full requested set on every /token exchange — a real security bug that the auto-approve stub from sub-PR C masked because granted == requested for that flow. fosite's flow_authorize_code_token.go:134-138 + flow_refresh.go:91-103 copy GrantedScope/Audience automatically; our loop was undoing selective consent. ## Workspace allow-list storage The user's workspace selections live in `session.Extra["allowed_workspaces"]` (round-trips via storage.go's existing JSON marshal). Either `["*"]` for wildcard or a list of slugs. fosite's WriteIntrospectionResponse serializes Extra into the introspection response as top-level fields, so TASK-953's enforcement layer reads them off `/oauth/introspect` (or in-process via fosite.IntrospectToken). This sidesteps fosite's strict "granted ⊆ requested ⊆ client.Scopes" check — clients don't request `pad:workspaces:foo`, but the consent UI lets the user pick from their workspaces regardless. TASK-953 implements the live role resolution + workspace gate. ## Defense in depth - Server validates `capability_tier ∈ {read, write, admin}` AND that the chosen tier is among the client's requested scopes — fosite would reject otherwise with a less-readable error. - Server validates every non-wildcard slug is in the user's current membership table. A tampered form sending other slugs gets 400. - Wildcard wins: if a tampered POST sends both `*` and specific slugs, the result is `["*"]` only — never partial allow-list. ## Tests - TestConsent_RendersUserWorkspaces — multi-workspace list with role labels. - TestConsent_NoWorkspaces_ShowsEmptyState — clean empty state. - TestConsent_TierRadios_OnlyRequestedScopes — UI hides tiers the client didn't request. - TestConsent_ApproveWithSpecificWorkspaces — happy path, asserts introspection returns `allowed_workspaces=[alpha, beta]`. - TestConsent_ApproveWithWildcard — wildcard yields `["*"]`. - TestConsent_ApproveWithoutWorkspaceSelection_Rejected — 400 on empty allow-list. - TestConsent_ApproveWithUntrustedSlug_Rejected — defense in depth. - TestConsent_ApproveWithUnrequestedTier_Rejected — server tier validation matches UI's tier-radio constraint. - TestConsent_TokenScopeMatchesTierChoice_Read — selective consent: user picks read-only despite client requesting both, token has exactly `pad:read`. Existing tests + helpers updated to include the new consent fields (`capability_tier`, `allowed_workspaces`). * fix(oauth): prevent URL parameter pollution attack on consent UI (round 1) Codex review #376 round 1 caught a P1 security bug in the consent UI. The hidden-input round-trip used the full r.URL.Query() with only `csrf_token` stripped, so a malicious OAuth client could craft /oauth/authorize?...&capability_tier=admin&allowed_workspaces=* and the consent form would render those as hidden inputs BEFORE the user-controlled radios + checkboxes. On submit, the hidden values precede the user's selection in the form encoding, so: - r.FormValue("capability_tier") returns "admin" (first value matches the attacker's, not the user's) - r.PostForm["allowed_workspaces"] sees "*" first, the wildcard scan matches, the result is ["*"] regardless of which boxes the user actually checked Net effect: a user clicking through the consent UI for "read-only, just my docapp workspace" would silently authorize "admin, all workspaces" — without any visible cue that the values were wrong. Fix: build hidden inputs from an explicit allowlist of OAuth-standard authorize-request parameters (response_type, client_id, redirect_uri, scope, state, audience, resource, code_challenge, code_challenge_method, nonce). Anything outside the allowlist is silently dropped. This is strictly stronger than blocklisting consent-control names, because it also defends against future OAuth extensions adding new attacker- controllable params we haven't enumerated. Test: TestConsent_URLPollution_DoesNotOverrideUserSelection simulates the attack — GET /authorize with attacker params, asserts the rendered HTML contains zero `<input type="hidden" name="<attacker_name>">`, then completes the flow with the user's actual selection and confirms the issued token's scope matches the user's choice (pad:read), not the attacker's URL injection (pad:admin).	2026-05-02 14:08:11 -04:00
xarmian	924d82dae4	feat(oauth): MCPBearerAuth OAuth integration + public-info (TASK-1027) — closes TASK-951 (#375) ... * feat(oauth): MCPBearerAuth OAuth integration + public-info endpoint (TASK-1027, sub-PR E of TASK-951) Closes the OAuth server build-out by connecting sub-PRs A-D to the MCP transport from TASK-950 and shipping the consent-screen support endpoint. ## MCPBearerAuth OAuth path middleware_mcp_auth.go now branches on token shape: - pad_<60-hex> → existing PAT validation (TASK-950 path) - anything else → fosite.IntrospectToken via the new internal/oauth.Server.IntrospectToken wrapper (server-side, no HTTP roundtrip — pad-cloud is both auth server and resource server, so the public /oauth/introspect endpoint is for external clients only). OAuth path validation gates: - Token must be active (fosite returns ErrInactiveToken / ErrNotFound on revoked / unknown / expired tokens). - tokenUse must be access_token; refresh tokens explicitly rejected (RFC 6749 §1.5 — refresh tokens aren't bearers for resource calls). - Granted audience MUST contain the canonical MCP URL (RFC 8707 anti-replay; resource-server-side check defends against compromised or shared auth servers). - Subject must resolve to a real user row. Successful path stashes user + scopes via WithCurrentUser / WithTokenScopes. Scopes are translated from fosite's space-separated form to JSON-array form via oauthScopesToJSON. ## tokenScopeAllows pad:* extension Extended to recognize the OAuth scope vocabulary alongside PAT scopes: - pad:read ↔ read (GET/HEAD/OPTIONS only) - pad:write ↔ write (all methods) - pad:admin ↔ * (all methods) So MCP tool authorization stays uniform regardless of which transport issued the bearer. ## /api/v1/oauth/clients/{id}/public-info New read-only endpoint for the consent screen (TASK-952) and the OAuth-intent banner (TASK-1001, already shipped). Returns four non-sensitive fields: client_id, client_name, logo_uri, redirect_uris. - Auth-required (any logged-in user). - Cloud-mode-gated (404s outside cloud). - 404 for unknown clients. - Whitelisted leak surface — explicit fields, no embedded models.OAuthClient, so a future field addition (e.g. a confidential- client secret) doesn't accidentally appear here. ## Tests - TestMCP_OAuthAccessToken_Authenticates — happy path: full flow yields a token that authenticates against /mcp. - TestMCP_OAuthAccessToken_AudienceMismatch_Rejected — RFC 8707 resource-server check; mints a token, swaps the OAuth server for one with a different canonical, confirms 401. - TestMCP_OAuthRefreshToken_RejectedAtMCP — refresh tokens MUST NOT authenticate. - TestMCP_RevokedOAuthToken_Rejected — revocation takes effect at the resource server. - TestMCP_PATPath_StillWorks — regression for sub-PR D's coexistence with the OAuth path. - TestMCP_OAuthScopeReadOnly_StashesPadReadScope — scope round-trip. - TestOAuthClientPublicInfo_HappyPath / UnknownClient_404 / Unauthenticated_401 / NotMountedOutsideCloudMode — full coverage of the new endpoint. - TestE2E_ClaudeDesktopFlow — simulates the full sequence (discovery → DCR → authorize → token → /mcp call) Claude Desktop walks on first connect. - TestTokenScopeAllows extended with pad:* coverage. ## TASK-951 status Closes TASK-951 when this lands (5/5 sub-PRs done): - A: schema + storage layer (#370 / 2a00775) - B: fosite-backed authorization-server constructor (#371 / f6eeee4) - C: DCR + authorize + token endpoints + populated discovery (#372 / 48776a3) - D: revoke + introspect endpoints (#373 / 4250fb1) - E: MCPBearerAuth + public-info (this PR) * fix(oauth): fail-closed on empty OAuth scopes per Codex review (round 1) Codex caught a high-severity bug in oauthScopesToJSON: the helper mapped empty granted scopes to `[]`, which tokenScopeAllows interprets as the legacy "unrestricted" PAT shape (allow all methods). Combined with OAuth's RFC 6749 §3.3 rule that the `scope` parameter is OPTIONAL, this meant a client could: 1. Run the auth-code flow without requesting scopes. 2. Get back a token with empty granted_scopes. 3. Drive write MCP tools because MCPBearerAuth stashed `[]` and tokenScopeAllows fell through to the legacy unrestricted path. Fix: map empty OAuth scopes to JSON `null` instead. tokenScopeAllows denies on the "scopes == nil" branch (existing TASK-667 behavior), so the entire write surface is denied for empty-scope OAuth tokens. In production this path is hard to hit — sub-PR C's DCR handler defaults registered clients to `pad:read pad:write` when omitted, and audienceMatchingStrategy enforces canonical-audience matching at grant time. Defense-in-depth at the resource server is the right policy regardless. Test: TestOAuthScopesToJSON_FailClosedOnEmpty asserts both halves of the contract — the helper produces "null" for empty input, and tokenScopeAllows denies every method when fed that value.	2026-05-02 13:33:53 -04:00
xarmian	4250fb1976	feat(oauth): revoke + introspect endpoints (TASK-1026) (#373) ... * feat(oauth): revoke + introspect endpoints (TASK-1026, sub-PR D of TASK-951) Add the RFC 7009 revocation and RFC 7662 introspection endpoints, completing the spec'd surface that sub-PR C left as placeholders. - POST /oauth/revoke — fosite NewRevocationRequest delegates to our storage adapter's RevokeRefreshToken / RevokeAccessToken which walk the request_id (grant family) and mark every chain member inactive in one statement. Public clients authenticate by sending only client_id (token_endpoint_auth_method=none). - POST /oauth/introspect — fosite NewIntrospectionRequest with Bearer auth (a separate active access token). Returns {active:true, sub, scope, aud, client_id, exp, iat} for active tokens; bare {active:false} for unknown/revoked/expired (RFC 7662 §2.2 no-leak rule). Sub-PR E's MCPBearerAuth integration uses fosite.IntrospectToken directly server-side, but the public endpoint satisfies the discovery contract for clients that follow the chain. - Discovery doc populates revocation_endpoint + introspection_endpoint and their auth_methods_supported lists ("none" for both — public-clients-only model). Tests cover: - /revoke marks an access token inactive (verified via introspect). - /revoke on a refresh token revokes the entire grant family (paired access also goes inactive). - Refresh-token rotation: old pair becomes inactive, new pair active. - Refresh-token replay detection: replaying a rotated refresh kills the family (OAuth 2.1 §6.1, RFC 6819 §5.2.2.3). - Introspect happy path returns sub/scope/aud/client_id/exp. - Introspect on unknown token returns just {active:false} with no field leakage. - Introspect rejects requests with no Bearer Authorization header. - /revoke + /introspect 404 outside cloud mode. - Discovery doc advertises both endpoints + auth-methods lists. * fix(oauth): drop introspection_endpoint_auth_methods_supported per Codex review (round 1) Codex review of #373 caught a contradiction in the discovery doc: introspection_endpoint_auth_methods_supported: ["none"] advertised "no client authentication" for the introspection endpoint, but fosite's NewIntrospectionRequest rejects a request without Authorization: Bearer ... (and the test in this PR locks that in). A discovery-driven client would treat "none" as "post token+client_id unauthenticated" and get 401 — worse than no advertisement at all. Fix: omit introspection_endpoint_auth_methods_supported entirely. RFC 8414 §2 marks the field OPTIONAL; omission tells clients to negotiate auth out-of-band, which for our public-clients-only model means "send a separate active access token in the Authorization header." We document that in getpad.dev/mcp/local. revocation_endpoint_auth_methods_supported = ["none"] is kept and honest — fosite's NewRevocationRequest really does accept a public client posting only client_id (no Bearer required). * fix(oauth): RFC 7009 §2.2 idempotent revoke per Codex review (round 2) Codex caught that fosite v0.49 returns ErrInvalidRequest for the unknown-token path of NewRevocationRequest, which WriteRevocationResponse turns into 400. RFC 7009 §2.2 explicitly requires: "The authorization server responds with HTTP status code 200 if the token has been revoked successfully or if the client submitted an invalid token." The 400 break the entirely normal "client retried after a previous revoke succeeded" or "operator typo'd the token" cases. Fix: detect the bare ErrInvalidRequest from the !found branch via isRevocationUnknownToken (which inspects HintField — fosite sets the hint on every other ErrInvalidRequest path it returns from NewRevocationRequest) and write 200 directly. Genuine malformed requests (wrong method, unparseable body, empty form) still return 400 because their ErrInvalidRequest carries a hint. Tests: - TestOAuth_Revoke_UnknownToken_Returns200 — locks in 200 for the unknown-token path. - TestOAuth_Revoke_MalformedRequest_Returns400 — counterpart that ensures the 200 override doesn't accidentally swallow real malformed-request errors. * fix(oauth): require token param + remove dead-code revoke override (round 3) Codex round 3 noticed that POST /oauth/revoke with client_id but no token returned 200 OK — silently swallowing a missing-required- parameter error. RFC 7009 §2.1 marks `token` REQUIRED. Investigating the fix surfaced that round 2's isRevocationUnknownToken override was actually dead code: fosite v0.49's handler/oauth2/revocation.go's RevokeToken collapses ErrNotFound + ErrInactiveToken to nil via storeErrorsToRevocationError, so NewRevocationRequest returns nil and WriteRevocationResponse writes 200 natively for unknown tokens. The override never fired in any real path. Cleanup: - Replace the unused isRevocationUnknownToken + override with a pre-check that returns 400 invalid_request when `token` is missing. RFC 7009 §2.1 enforced; fosite's native idempotency handles unknown tokens. - Update TestOAuth_Revoke_UnknownToken_Returns200's comment to reflect that it pins fosite's native behavior (not our override). - Add TestOAuth_Revoke_MissingToken_Returns400 to lock in the pre-check. - Keep TestOAuth_Revoke_MalformedRequest_Returns400 — verifies fosite's own ErrInvalidRequest paths still surface as 400.	2026-05-02 12:47:54 -04:00
xarmian	48776a3967	feat(oauth): DCR + authorize + token endpoints + populated discovery (TASK-1025, sub-PR C of TASK-951) (#372) ... * feat(oauth): DCR + authorize + token endpoints + populated discovery doc (TASK-1025, sub-PR C of TASK-951) Third of 5 sub-PRs landing the OAuth 2.1 authorization server in PLAN-943. Mounts the three flow-driving HTTP endpoints over the fosite-backed server constructed in sub-PR B, replaces the TASK-950 501 stub with the real RFC 8414 discovery doc, and ships an inline-HTML consent stub as a TASK-952 placeholder so the auth-code flow runs end-to-end. What lands: - internal/server/handlers_oauth.go (744 LoC) - POST /oauth/register: RFC 7591 DCR. Hand-written, no fosite. Public clients only (token_endpoint_auth_method=none rejected for any other value), authorization_code + refresh_token grants only, code response type only. Validates redirect_uris (absolute, no fragment, https or loopback-http or custom- scheme like claude://, blocks file:/javascript:/data:/vbscript:). - GET /oauth/authorize: starts auth-code flow. fosite validates request shape (PKCE-S256 required, audience matched, redirect exact-match). If user has session → renders inline consent stub. If not → 302 to /login?redirect=<self> (TASK-998's plumbing in pad-cloud honors the redirect=). - POST /oauth/authorize/decide: processes consent decision. Form-bound CSRF token (the existing __Host-pad_csrf cookie, read from a hidden form field instead of header). Approve → fosite NewAuthorizeResponse → 303 to client.redirect_uri with code. Deny → fosite WriteAuthorizeError(access_denied). - POST /oauth/token: code + refresh exchange. fosite verifies PKCE verifier (S256-required) + RFC 8707 audience. Returns {access_token, token_type, expires_in, refresh_token, scope}. RefreshTokenScopes=[] from sub-PR B means refresh ALWAYS issues on authorize-code grant. - Inline consent stub: minimal HTML form with Approve/Deny, auto-grants every requested scope (TASK-952's UI replaces with workspace allow-list selection per TASK-953). - internal/server/handlers_well_known.go: handleOAuthAuthorizationServerStub → handleOAuthAuthorizationServer. Returns RFC 8414 metadata with all six endpoint URLs (revoke + introspect URLs sub-PR D fills with handlers; the URLs are stable now), advertised scopes, S256-only code_challenge_methods, resource_indicators_supported=true, authorization_response_iss_parameter_supported=true. - internal/server/server.go: Server.oauthServer field + SetOAuthServer + registerOAuthRoutes called from setupRouter inside an r.Group with requireCloudMode + SessionAuth (so /authorize can detect the logged-in user via __Host-pad_session; SessionAuth falls through gracefully when no cookie). - cmd/pad/main.go: oauthpkg.NewServer wired in cloud mode using cfg.EncryptionKey as HMAC secret + cfg.MCPPublicURL+/mcp as AllowedAudience. Wiring is conditional on PAD_MCP_PUBLIC_URL being set (the OAuth surface needs a canonical audience to bind tokens to). CSRF posture: middleware_csrf.go runs only on /api/* paths so /oauth/* is naturally exempt. The consent decision endpoint adds its own form-token check (validateConsentCSRFToken) using the same __Host-pad_csrf cookie the SPA uses, just with the token in a hidden form field rather than a header. Same security model, different transport. Tests (12, all passing): - TestOAuth_AuthorizationServerMetadata_PopulatedShape: pins RFC 8414 metadata fields including S256-only PKCE + resource_indicators_supported. - DCR (5): happy path; missing redirect_uris; bad redirect-URI shapes (relative, non-loopback http, fragment, javascript:); non-public client auth method rejected; unknown grant type rejected; not mounted outside cloud mode. - /authorize (3): redirects to /login when no session; renders consent stub when logged in; rejects audience mismatch via fosite's audienceMatchingStrategy. - /authorize/decide (2): rejects missing csrf_token; deny produces access_denied redirect. - Full PKCE flow: end-to-end /authorize/decide (approve) → /token with code_verifier → 200 with access+refresh tokens. - /token: rejects missing PKCE verifier. Replaces the 501 stub assertion in TestMCP_AuthServerStub with TestMCP_AuthServerMetadata_Mounted (just confirms 200; full shape lives in the OAuth-handler test). Out of scope: - /oauth/revoke + /oauth/introspect (sub-PR D, TASK-1026) - MCPBearerAuth OAuth introspection branch (sub-PR E, TASK-1027) - Real consent UI with workspace allow-list (TASK-952) * fix(oauth): translate RFC 8707 resource= to audience= + omit unmounted endpoints from discovery per Codex review (round 1) Two findings from PR #372 round 1: 1. P1: Real RFC 8707 clients (Claude Desktop / Cursor / ChatGPT) send `resource=` not `audience=`. fosite v0.49 reads only `audience` from the form, so audienceMatchingStrategy was hit with an empty needle and rejected every real-world authorize / token request. Tests masked the gap by sending both keys. Fix: translateResourceToAudience() copies r.Form["resource"] into r.Form["audience"] before each handler invokes fosite. Idempotent — if both keys are present, audience wins (test harness sends both for belt-and-suspenders). Applied at /authorize, /authorize/decide, and /token entry points. Test TestOAuth_Authorize_AcceptsResourceOnly sends ONLY resource= (no audience=) and asserts the request reaches the consent stub. Without the translation it 303s with invalid_request. 2. P2: /.well-known/oauth-authorization-server advertised /oauth/revoke + /oauth/introspect endpoints that don't exist yet (sub-PR D wires them). Real clients dialing those URLs would get 404. RFC 8414 §2 lists revocation_endpoint + introspection_endpoint as OPTIONAL, so omitting until the handlers ship is spec-compliant + honest. Fix: drop revocation_endpoint, introspection_endpoint, and their *_endpoint_auth_methods_supported counterparts from authServerMetadata. Sub-PR D's PR description includes "populate these here" as a follow-up. Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints asserts the four fields are absent. * fix(oauth): rate-limit /oauth/register + drop misleading iss flag per Codex review (round 2) Two findings from PR #372 round 2: 1. P1: /oauth/register is open by RFC 7591 design (Claude Desktop / Cursor self-register without prior auth) but had no rate limit. An attacker could flood the oauth_clients table indefinitely. Fix: extend RateLimit middleware to gate /oauth/register at the same 5/hour/IP rate the existing /api/v1/auth/register uses (RateLimiters.Register, burst 5). Added the OAuth route group to the s.RateLimit middleware chain so the new path actually runs through the limiter. Other /oauth/* endpoints aren't rate-limited here: /authorize rides session cookies (cheap to abuse but ineffective without a logged-in user), /token is PKCE-bound to a stored code (single-use), /authorize/decide is form-bound. Explicit per- endpoint /oauth/* limits arrive with TASK-959. Test TestOAuth_Register_RateLimited fires 5 requests successfully, asserts the 6th returns 429. 2. P2: Discovery doc advertised authorization_response_iss_parameter_supported=true, but the /authorize success path delegates to fosite v0.49 which doesn't add iss=<issuer> to the redirect. RFC 9207-aware clients seeing the flag would treat the missing parameter as a protocol violation. Fix: drop the field from authServerMetadata. RFC 8414 §2 marks it OPTIONAL — omission is spec-compliant. We'll add the parameter (+ post-processing of fosite's response) in a future PR if a real client requires it; today's MCP clients (Claude Desktop, Cursor, ChatGPT) don't. Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints extended to cover the field. * fix(oauth): gate auth-server discovery doc on oauthServer != nil per Codex review (round 3) Codex round 3 caught: /.well-known/oauth-authorization-server lives in the MCP route group (registerMCPRoutes), while the /oauth/{ register,authorize,token} handlers live in the OAuth route group (registerOAuthRoutes, gated on s.oauthServer != nil). A cloud deployment with PAD_MCP_PUBLIC_URL unset gets MCP routes mounted but NOT OAuth — the discovery doc would 200 with /oauth/* URLs that 404. Worse for clients than no document at all. Fix: handleOAuthAuthorizationServer now also nil-checks s.oauthServer; on nil it returns 503 with config_error, matching the existing fail-loud branch for when the issuer URL isn't configured. Ops detect the misconfiguration immediately rather than fielding "OAuth registration is failing with 404" tickets. Test: - TestOAuth_AuthorizationServerMetadata_503WhenOAuthDisabled builds a Server with SetCloudMode + SetMCPTransport (so the MCP route group mounts) but NOT SetOAuthServer; asserts the endpoint returns 503 with config_error. - TestMCP_AuthServerMetadata_Mounted renamed → TestMCP_AuthServerMetadata_MountedAndGated to reflect the new behavior under mcpEnabledTestServer (which doesn't wire OAuth). The full 200 happy path lives in TestOAuth_AuthorizationServerMetadata_PopulatedShape (uses oauthEnabledTestServer). * fix(oauth): apply gofmt to handlers_oauth_test + handlers_well_known * fix(oauth): bump go-jose/v3 to v3.0.4 to resolve GO-2025-3485 CI govulncheck rejected the build: fosite v0.49.0 transitively pulls github.com/go-jose/go-jose/v3@v3.0.3 which has GO-2025-3485 (DoS in JWS parsing). Affected call site: internal/server/handlers_oauth.go:408 — handleOAuthAuthorize calls fosite.NewAuthorizeRequest which eventually calls jose.ParseSigned. Fix: bump go-jose/v3 to v3.0.4 (the fixed version per the advisory). go mod tidy auto-bumped dependent indirect deps too. Verified locally: govulncheck ./... → "No vulnerabilities found" go test ./... → all green go build ./... → clean	2026-05-02 11:56:57 -04:00
xarmian	521853e0a1	feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) (#369) ... * feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) First public cut of pad-cloud as a remote MCP server (PLAN-943). Mounts the Streamable HTTP transport on /mcp, the RFC 9728 protected-resource discovery doc on /.well-known/oauth-protected-resource, and a 501 stub for RFC 8414 auth-server metadata that TASK-951 will fill in. - internal/server/handlers_mcp.go — Server.SetMCPTransport + chi route registration under cloud-mode gate (self-host stays free of MCP overhead unless explicitly opted in). - internal/server/middleware_mcp_auth.go — Bearer auth that produces the spec-shape 401 + WWW-Authenticate (resource_metadata pointer) MCP clients expect, distinct from /api/v1's JSON-only 401 envelope. Reuses the existing PAT (api_tokens) validation path; OAuth-issued tokens layer in via this same middleware in TASK-951. - internal/server/handlers_well_known.go — RFC 9728 discovery doc + RFC 8414 stub. URLs come from PAD_MCP_PUBLIC_URL + PAD_AUTH_SERVER_URL with request-host fallback for local dev. - internal/server/handlers_mcp_test.go — 7 tests covering cloud-off routes-absent, cloud-on-no-transport routes-absent, discovery doc shape, 501 stub, no-token 401+WWW-Authenticate, bad-format-token 401+WWW-Authenticate, and the valid-PAT happy path with user attached to transport context. - cmd/pad/main.go — wires mcpserver.NewServer + HTTPHandlerDispatcher + StreamableHTTPServer in cloud mode, after SetCloudMode. - internal/config — adds PAD_MCP_PUBLIC_URL and PAD_AUTH_SERVER_URL. Resources are intentionally skipped in this v1 — they require an HTTPResourceFetcher equivalent of ExecResourceFetcher and that's a follow-up task. Tools, prompts, instructions, and meta all flow through identically to the stdio surface (verified via spike against mcp-go v0.50.0's StreamableHTTPServer before writing the real PR). * fix(mcp): enforce PAT scopes on /mcp + WWW-Authenticate fallback per Codex review (round 1) Two findings from PR #369 round 1: 1. SECURITY: A PAT with scopes ["read"] could drive write MCP tools. MCPBearerAuth skipped tokenScopeAllows entirely; the dispatcher's synthesized in-process request bypassed TokenAuth's chain-level check (because WithCurrentUser was already set), so a read-scoped token could POST item create / PATCH update / DELETE silently. Fix: stash apiToken.Scopes via server.WithTokenScopes in MCPBearerAuth; re-check per synthesized request in HTTPHandlerDispatcher.executeRequest using the public server.TokenScopeAllows wrapper. Read-scoped tokens can still drive read-only tools (their HTTP method is GET) — only writes are rejected, with a structured permission_denied envelope. 2. DISCOVERY: writeMCPUnauthorized dropped the WWW-Authenticate header when PAD_MCP_PUBLIC_URL was unset. Cloud-mode deploys without that env var mounted /mcp but broke the discovery handshake — fresh MCP clients rely on the header to find /.well-known/oauth-protected- resource. Fix: pass *http.Request through to writeMCPUnauthorized, derive "https://" + r.Host as the fallback (matches handleOAuthProtected- Resource's existing fallback). Tests: - handlers_mcp_test.go: TestMCP_NoToken_FallsBackToHostWhenPublicURLUnset pins the WWW-Authenticate fallback. TestMCP_ReadScopedPAT_StashesScopes- InContext + TestTokenScopeAllows_PublicWrapper pin the scope-stash side. - dispatch_http_test.go: TestHTTPHandlerDispatcher_ScopeEnforcement_* pin the dispatcher-side enforcement (read-on-write rejected, read-on-read allowed, no-scope-context allows-all). - recordingHandler updated to handle nil r.Body so the read-only GET path can be exercised. * fix(mcp): move scope check to buildAuthedRequest so bulk-update can't bypass it per Codex review (round 2) Round 1 enforced scopes in executeRequest, but dispatch_http_project.go's item bulk-update path constructs each per-item PATCH directly via buildAuthedRequest + d.Handler.ServeHTTP, skipping executeRequest. Net result: a PAT with ["read"] scope could still mutate items through bulk-update even after the round-1 fix. Move the scope check from executeRequest into buildAuthedRequest so every synthesized request — main writes, RMW prefetches, bulk-update per-item PATCHes, link-create POSTs, attachment HEADs — passes through the same gate uniformly. The check is dropped from executeRequest to avoid double-checking; buildAuthedRequest is the universal funnel everything calls. Reads (GET/HEAD/OPTIONS) under ["read"] scope still pass — bulk- update's per-item GET prefetch succeeds, the subsequent PATCH fails at request-build time with permission_denied. The bulk operation returns successfully with all-errors recorded per ref (the "no abort on per-item failure" contract is unchanged). Test: TestHTTPHandlerDispatcher_ScopeEnforcement_BulkUpdateBlockedOnReadScope spies on the test handler; asserts the PATCH never reaches it under ["read"] scope and that each per-item entry carries permission_denied.	2026-05-02 09:25:25 -04:00
xarmian	48bbe7453b	fix(dashboard): suggested_next surfaces in-progress + filters blocked items (BUG-990) (#366) ... Pre-fix algorithm only considered status == "open" child items in active plans. Two consequences agents flagged in BUG-987 / BUG-990: 1. In-progress items never appeared. The most likely "what should I work on next" answer is "the work the user is already on" — pre-fix the engine returned [] when nothing was open AND a task was actively in-progress. 2. Blocked items appeared. Suggesting work that has unresolved blockers wastes the user's time when they go to start it. Algorithm changes (internal/server/handlers_dashboard.go): - Include both `open` and active-status (in-progress / fixing / exploring / etc., via existing isActiveStatus helper) child items. - Filter out items with at least one active "blocks" link from a non-done blocker. Mirrors the attention-section logic, factored into a new itemBlockedByActive helper. - Sort: in-progress first (always wins over open, regardless of priority), then by priority rank within each bucket. Pinned in test expectations. - Reason text distinguishes "In-progress task..." vs "Open task..." so agents see why an item was suggested. Tests: - TestDashboardSuggestedNext expectations updated for new ordering (in-progress wins over open). Test now covers the in-progress surfacing path the bug specifically wanted. - New TestDashboardSuggestedNext_FiltersBlockedItems exercises the blocker-filter — explicitly creates a blocks link and confirms the blocked task is suppressed even at critical priority. Out of scope: high-priority orphan suggestions (items not in any active plan). The bug item flagged this as a stretch goal; this PR sticks to the active-plan-children scope of the existing algorithm. Adding orphans would need a sort/relevance model since "everything high priority" can be a long list. Parent: BUG-990.	2026-05-01 21:48:04 -04:00
xarmian	d84f1180a7	feat(mcp): pluggable Dispatcher + HTTPHandlerDispatcher for remote MCP (TASK-965) (#343) ... * feat(mcp): pluggable Dispatcher + HTTPHandlerDispatcher for remote MCP (TASK-965) Architectural prerequisite for PLAN-943's remote MCP at /mcp. The existing ExecDispatcher (PLAN-942) shells out to the pad binary and inherits credentials from ~/.pad/credentials.json — fine for local stdio MCP where the user IS the subprocess owner, but unworkable for a multi-tenant /mcp endpoint where the dispatcher must serve many OAuth-authenticated users from a single process. This PR ships the alternative path: HTTPHandlerDispatcher calls pad-cloud's existing HTTP handler chain in-process, with the requesting user attached via context. Same handlers, same audit / event-bus / webhook plumbing — just no fork(). ## What's in - internal/mcp/dispatch.go: keeps the existing Dispatcher interface (so ExecDispatcher unchanged) and adds a context-keyed WithDispatchInput helper. The registry attaches the original JSON input map to the dispatch context so dispatchers that prefer structured data over reverse-parsed cliArgs can use it. - internal/mcp/registry.go: forwards the merged input (user-supplied values + session workspace + root flags) to the dispatcher via WithDispatchInput. ExecDispatcher ignores it. - internal/mcp/dispatch_http.go (new): HTTPHandlerDispatcher implementation with a routeTable[cmdPath]→RouteMapper mapping. Seed entry: `item create`. Adding more commands is one RouteMapper per cmdPath plus a routeTable insert. - internal/server/context.go (new): exported WithCurrentUser / WithAPITokenAuth / WithTokenWorkspaceID + read-only CurrentUserFromContext / IsAPITokenFromContext. Lets internal/mcp synthesize an authenticated request without reaching the package-private context keys. - internal/server/middleware_csrf.go: extends the existing "Bearer token requests skip CSRF" rule to also honor the ctxIsAPIToken context flag. Same semantic — non-cookie auth means no CSRF risk — but covers the in-process dispatch path where TokenAuth never sets the Authorization header. Safe because ctxIsAPIToken can only be set by trusted in-process code (TokenAuth on the live Bearer path, or server.WithAPITokenAuth from the dispatcher). ## What's tested - Unit: - TestHTTPHandlerDispatcher_RoutesItemCreate — full happy path with a recordingHandler asserting method/path/body/user-context. - TestHTTPHandlerDispatcher_UnsupportedToolReturnsErrorResult — tools not yet in the routeTable produce IsError-flagged results rather than panicking. - TestHTTPHandlerDispatcher_NoUserReturnsErrorResult — UserResolver returning nil produces an IsError, never a nil-deref. - TestHTTPHandlerDispatcher_HandlerErrorSurfacesAsToolError — 4xx handler responses come back as IsError MCP results matching ExecDispatcher's `pad <cmd> failed: <stderr>` format. - mapItemCreate validation + parseFieldKVP variants. - Integration: TestHTTPHandlerDispatcher_Integration drives the full *server.Server (real chi router, real SQLite store, full middleware chain) with a synthesized OAuth user and asserts the item lands in the DB. ## Scope discipline The DoD called for "dispatch item.create end-to-end" — that's the seed entry. Wiring the remaining ~70 MCP-exposed commands into routeTable is naturally a follow-up before TASK-950 ships /mcp to real users (captured as a separate task post-merge). Audit-log assertion in the integration test is deferred until TASK-960 (B6b) lands the audit log itself. Parent: PLAN-943. * fix(mcp): roll status/priority/category/parent into fields JSON per Codex review (round 1) Codex caught: mapItemCreate placed status / priority / category / parent at the top level of the JSON body, but handleCreateItem only reads them after unmarshalling the Fields string from the request. As written, MCP-driven `item create` would silently drop those flags — breaking parity with the CLI for almost every realistic call (parent- linked tasks, priority-set items, status-overridden ideas, etc.). Mirrored the CLI's behaviour (cmd/pad/main.go ~L2200): build a fields map from the named flags, overlay the repeatable --field entries on top, JSON-encode into ItemCreate.Fields. The handler's existing schema-validation + parent-resolution path now runs unchanged. Repeatable --field still wins last-write — locked into a new test so it doesn't drift. Also rejects --assign / --role with a clear error rather than silently dropping them. The CLI resolves user-name → user-ID and role-slug → role-ID via additional API calls before posting; replicating that pre-resolution belongs in a follow-up that expands the route table for production use. Failing loudly is better than partial parity. Tests: - TestHTTPHandlerDispatcher_RoutesItemCreate now asserts the status/priority/category/parent values land in fields, not the top level — guards against the regression directly. - TestMapItemCreate_ExplicitFieldOverridesNamedFlag locks the last-write-wins precedence between --status and --field status=... - TestMapItemCreate_RejectsUnsupportedAssignRole asserts the defensive error path for the deferred flags. Parent: PLAN-943. * fix(mcp): persist source=cli for HTTPHandlerDispatcher calls per Codex review (round 2) Codex caught: actorFromRequest derives source from the Authorization header — without one, dispatcher-driven calls would persist source="web" instead of source="cli", regressing dashboard/standup/ audit attribution vs. ExecDispatcher. Same pattern as the round-1 CSRF fix: extend actorFromRequest to also honor the ctxIsAPIToken context flag (which TokenAuth sets on the live Bearer-auth path and HTTPHandlerDispatcher sets via server.WithAPITokenAuth on synthesized requests). Both signals mean "non-cookie authenticated, attribute as CLI/agent traffic". Integration test now asserts source="cli" on the created item, so any future regression of this attribution surfaces immediately. Parent: PLAN-943. * fix(mcp): normalize collection aliases in HTTPHandlerDispatcher per Codex review (round 3) Codex caught: CLI's `item create task ...` works because cmd/pad/main.go's normalizeCollectionSlug maps singular/short forms ("task" → "tasks", "doc" → "docs", etc.) to the canonical slug before posting. HTTPHandlerDispatcher's mapItemCreate skipped that step, so the same documented call shape would 404 through the HTTP transport even though it worked through ExecDispatcher. Extracted the alias map to internal/collections.NormalizeSlug so the two transports stay in lockstep without duplication. cmd/pad/main.go's normalizeCollectionSlug now delegates to it; the in-process dispatcher calls it from mapItemCreate after pulling the collection out of input. TestMapItemCreate_NormalizesCollectionAliases locks every documented alias plus a passthrough case for custom collections. Parent: PLAN-943. * fix(server): WithTokenWorkspaceID actually clears on empty input per Codex review (round 4) Codex caught: the docstring said "Pass an empty string to clear" but the implementation early-returned `ctx` unchanged in that case, leaving any stale ctxTokenWorkspaceID set further up the chain active. Always overwrite so the contract holds: passing "" produces a context where tokenWorkspaceID(r) returns "", same as a never-set context. Parent: PLAN-943.	2026-05-01 11:51:28 -04:00
xarmian	5e27989ab8	feat(attachment): add pad attachment view|show|list CLI surfaces (IDEA-898) (#321) ... * feat(attachment): add `pad attachment view|show|list` CLI surfaces (IDEA-898) Agents and CLI users had no first-class way to fetch attachment bytes through the API: the only path to read an `` reference was to read the raw blob out of `~/.pad/attachments/<storage_key>`, which bypasses workspace ACLs, doesn't work on Pad Cloud / remote / Postgres deployments, skips the variant pipeline (TASK-872 / TASK-879 / TASK-880), and breaks when storage moves to S3. Three new subcommands wrap the existing REST endpoints: - `pad attachment view <id> [-o path]` — agent-friendly: with no `-o`, fetches to a fresh OS temp directory using the stored filename and prints just the absolute path on stdout (so `$(pad attachment view <id>)` composes cleanly into shell pipelines). Reuses `download`'s atomic temp-then-rename pattern via a shared helper. - `pad attachment show <id>` — HEAD-based metadata only; surfaces MIME, size, filename, ETag, Last-Modified. - `pad attachment list [--item REF] [--category X] [--attached|--unattached] [--collection ID] [--sort ...] [--limit N] [--offset N]` — workspace list. The `--item REF` flag resolves a TASK-5-style ref to a UUID client-side and passes it to a new `item_id` query param on the list endpoint (server side: AttachmentListFilters.ItemID, ~6 lines in the store + 1 in the handler). Skill update: `skills/pad/SKILL.md` gains a "Working with attachments" subsection plus a CLI Reference entry, both ending in the hard rule that agents must NEVER read directly from `~/.pad/attachments/`. * style(cli): gofmt AttachmentListParams field alignment CI's golangci-lint v2 flagged this with the gofmt formatter (configured with simplify: true in .golangci.yml). The contiguous Sort/Limit/Offset block at the end of the struct needs uniform column alignment — gofmt considers the doc comment above Sort attached to that field rather than a block separator, so the three int/string fields get aligned together. Verified locally with `golangci-lint run --timeout=5m ./...` (v2.11.4 to match CI) — 0 issues. Local make lint only runs `go vet ./...` and `golangci-lint` wasn't installed, which is why this slipped through; filing a separate follow-up to mirror the CI checks in the local workflow.	2026-04-30 16:06:08 -04:00
xarmian	10309fc599	fix(config): read PUBLIC_URL for emailed link generation (BUG-899) (#318) ... * fix(config): read PUBLIC_URL for emailed link generation (BUG-899) The Pad Cloud deployment binds pad to 0.0.0.0 (Dockerfile, k8s configmap, pad-cloud's docker-compose) and never set PAD_URL on the pad service, so cfg.BaseURL() fell through to "http://0.0.0.0:7777" — that string ended up in password-reset (and invite + share-link + admin-invitation) emails and was unreachable to recipients. Adds a PUBLIC_URL env var read by the server only (does not flip CLI to remote mode the way PAD_URL does — PUBLIC_URL is a generic env var name commonly set in unrelated deployment contexts). Stored in a separate Config.PublicURL field consulted by BaseURL() as a fallback after URL. Resolution order in BaseURL(): PAD_URL > PUBLIC_URL > host:port. Also logs a WARN at server startup if the resolved base URL has an unspecified bind-all host (0.0.0.0, ::, [::]) — a backstop that would have caught BUG-899 the first time email went out. Tests cover the precedence ladder, mode-not-flipping, PAD_URL-beats- PUBLIC_URL, and the BUG-899 repro shape (Host=0.0.0.0 with no URL set yields the broken http://0.0.0.0 URL). Companion change in pad-cloud/docker-compose.yml passes PUBLIC_URL through to the pad service so the Cloud deployment stops shipping broken email links. Parent: BUG-899 (TASK-908). * fix(config): keep PUBLIC_URL out of IsConfigured() per Codex review (round 2) PUBLIC_URL was setting LoadedFromEnv = true, which IsConfigured() consults to decide whether the CLI has explicit configuration. A generic PUBLIC_URL in the environment (very common name) would have made any host appear "configured" to the CLI and skipped the not-configured / setup branch — the exact footgun the separate-field design was supposed to avoid. PUBLIC_URL is purely a server-side fact; LoadedFromEnv is purely a CLI affordance. Stop conflating them. Adds a focused regression test pinning the IsConfigured() invariant. * fix(config): split PublicLinkBaseURL from BaseURL per Codex review (round 3) Round 2's BaseURL fall-through to PublicURL leaked PUBLIC_URL into ~20 CLI-client call sites (cli.NewClientFromURL(cfg.BaseURL()) patterns across cmd/pad/main.go, init.go, server_info.go, configure.go) — same footgun the separate-field design was meant to avoid: a developer with a host-level PUBLIC_URL set for unrelated reasons would have their CLI silently route requests to that URL instead of the local server. Restore BaseURL() to its original CLI-only contract (URL > host:port). Add PublicLinkBaseURL() with the URL > PublicURL > host:port ladder that's used at exactly the two server-side call sites that build emailed-link targets: - cmd/pad/main.go:279 srv.SetBaseURL(cfg.PublicLinkBaseURL()) - cmd/pad/main.go:464 email.NewSender(..., cfg.PublicLinkBaseURL()) Tests pin both contracts: BaseURL() ignores PublicURL even when set; PublicLinkBaseURL() honors the precedence ladder. PAD_URL still wins in both, preserving back-compat. * fix(config): drop public_url toml tag to prevent CLI persistence per Codex review (round 4) Round 3 left PublicURL serializable to ~/.pad/config.toml via toml: "public_url". A CLI user who runs `pad init` or `pad configure` on a host where PUBLIC_URL is set for unrelated reasons would end up with that URL persisted into their config file, surviving any later unset of the env var and contaminating server-side emailed link generation indefinitely (server reads ~/.pad/config.toml on the next boot). Switch the field to toml:"-". PUBLIC_URL is a deployment-time fact (env var / docker-compose / k8s); operators who want a config-file equivalent already have `url` (the PAD_URL path), which serializes properly. Adds a regression test pinning that Save() never writes PublicURL to the file.	2026-04-30 08:40:33 -04:00
xarmian	cec056cefe	feat(email): cloud-mode marketing footer in transactional emails (TASK-907) (#317) ... * feat(email): cloud-mode marketing footer in transactional emails (TASK-907) Extracts a shared HTML/plain shell helper for the five existing transactional-email templates (SendInvitation, SendWelcome, SendPasswordReset, SendPaymentFailed, SendTest) and adds a Cloud-only marketing footer that mirrors the auth-page AuthFooter component: GitHub / Docs / Changelog / Privacy / Terms link list plus a "© <year> Pad · Perpetual Software" copyright line. Self-hosted output (the default for any pad instance NOT in PAD_CLOUD/PAD_MODE=cloud) is byte-equivalent to the prior inline templates: same wordmark header, same body, same footer-note disclosure, no marketing links. Operators ship Pad under their own brand and getpad.dev's link list would be wrong on their notifications. Plumbing: - email.Sender gains a cloudMode bool + SetCloudMode/CloudMode accessors. Configure() does not touch cloudMode (it's set independently from API-key/from-addr config). - Server.SetCloudMode now propagates to s.email.SetCloudMode(true) so existing senders pick up the flag. - Server.SetEmailSender propagates s.cloudMode → e.cloudMode when email is wired AFTER cloud mode (handles the cmd/pad/main.go ordering where SetEmailSender is called from main). - Server.reconfigureEmail() (admin-settings reload path) does the same so an admin reconfiguring email mid-flight doesn't end up with a sender stuck in self-hosted mode. The email accent color (#2563eb) is preserved from the prior templates — it has known contrast properties on white email backgrounds. Email is light-themed for cross-client readability; the dark-theme tokens from docs/brand.md §3 are for in-app/auth surfaces, not transactional mail. Pinned with three regression tests: - self-hosted shell renders no Cloud-only markers - Cloud shell renders the link list in canonical order (GitHub → Docs → Changelog → Privacy → Terms) - plain-text shell branches identically Visual contract: docs/brand.md §7 (link order) and §6 (Pad wordmark). Companion to AuthHeader, AuthFooter, +error.svelte, and UserMenuResources already shipped on PLAN-900. Test plan: - go build ./... — clean - go vet ./... — clean - go test ./... — all pass (including new shell_test.go cases) - web/npm run check — 0 errors - web/npm run build — clean * fix(email): full canonical link list per Codex (round 2) Codex caught that the Cloud-mode email footer carried only 5 of the 9 canonical links from docs/brand.md §7 (GitHub / Docs / Changelog / Privacy / Terms — omitted Contribute / FAQ / Security / Sub-processors). The brand spec §1 says transactional emails get "Full parity" with the auth-page AuthFooter; my trim violated that contract. Add the four missing links to both the HTML and plain-text shells in the canonical order: GitHub → Docs → Changelog → Contribute → FAQ → Security → Privacy → Terms → Sub-processors. Update the regression tests to pin all 9 markers + their pairwise ordering. The "keep emails small" instinct that motivated the trim was a real design concern but not strong enough to defy the brand spec. If we later decide email needs a reduced subset, the right move is to update §7 in docs/brand.md FIRST (acknowledging email as a surface with a smaller link list) and trim the implementation to match.	2026-04-30 00:09:52 -04:00
xarmian	9c5f4d5165	fix(cli): construct auth login URL on CLI side to avoid 0.0.0.0 leak (TASK-839) (#311) ... * fix(cli): construct auth login URL on CLI side to avoid 0.0.0.0 leak (TASK-839) The server builds the CLI auth-approval URL from r.Host, which echoes back whatever Host header the CLI sent. When the local pad server is bound to a bind-all address (e.g. --host 0.0.0.0), the CLI's own config points at that address, so the URL printed by `pad auth login` ends up as http://0.0.0.0:7777/auth/cli/{code} — a bind address, not a usable browser destination. Construct the URL on the CLI instead, using cfg.BrowserURL() (which already rewrites 0.0.0.0 / :: / empty to 127.0.0.1, and returns the explicit URL verbatim for Remote/Cloud). The server-issued auth_url field is now ignored; session_code is what we actually need and is already returned separately. Extracts a small cliAuthBrowserURL helper so the wiring is unit-testable and adds regression coverage for IPv4 bind-all, IPv6 bind-all, empty host, explicit loopback, explicit Remote URL, and trailing-slash trim. * chore(lint): remove unused readBundleAsBytes test helper golangci-lint v2.11.4 (CI) flags this as unused — it was added in the import-bundle test scaffolding (TASK-885 / TASK-891 era) but no caller ever picked it up. Removing it unblocks the lint gate on main. Reviewable in isolation; pure deletion, no behavior change.	2026-04-29 22:03:27 -04:00
xarmian	47a4448afc	chore(import-bundle): audit + harden bundle import validation (TASK-891) (#308) ... * chore(import-bundle): audit + harden bundle import validation (TASK-891) Re-reviewed handlers_import_bundle.go before exposing the bundle import flow through the web UI under PLAN-890. The audit doc lives at DOC-895; this commit lands the small inline fixes. Findings + actions: - Duplicate pad-export.json now rejected (was: silently ran ImportWorkspace twice, stranding the first workspace as an orphan with no attachments). - Duplicate attachments/manifest.json now rejected (was: silently overwrote manifestByPath, dropping prior entries). - Defense-in-depth path-traversal guard added via isSafeBundleEntryName — rejects entries with `..` segments, absolute paths, or NUL bytes BEFORE the switch. Storage was already hash-keyed and safe, but the silent-skip behavior on malicious tar names was a poor audit story. - Auth/permissions now documented on the handler — RequireAuth middleware gates the endpoint; no per-workspace role check applies because the request creates a new workspace (mirrors handleCreateWorkspace). Tests added: TestImportBundle_RejectsDuplicateExport, TestImportBundle_RejectsDuplicateManifest, TestImportBundle_RejectsPathTraversal, TestIsSafeBundleEntryName. Two larger gaps deferred as their own tasks: - TASK-896 (partial-import orphan workspace on mid-stream failure — needs design discussion). - TASK-897 (per-user storage quota enforcement on import — gated on Phase 2 quota work; matches upload handler's warn-only Phase 1 policy today). Parent: PLAN-890. * fix(import-bundle): roll back partial workspace on validation reject per Codex review (round 1) Codex P1 on PR #308: when the duplicate-pad-export.json or duplicate-manifest.json guards fire, the workspace from the first occurrence has already been inserted by ImportWorkspace. The handler returned 400 but the orphan workspace stayed in the destination DB. A malformed/malicious bundle could repeatedly POST and pile up half-imported workspaces. Fix: when importBundle returns an importStatusError after creating a workspace, the handler now soft-deletes that workspace via DeleteWorkspace before returning the 400. Mid-stream errors that are NOT importStatusError (e.g. manifest decode after items inserted) intentionally keep the partial workspace — that's the existing design tracked under TASK-896 (partial-import design discussion). Tests extended: TestImportBundle_RejectsDuplicateExport and TestImportBundle_RejectsDuplicateManifest now also list the destination workspaces after the rejected import and assert the partial workspace does NOT appear. Parent: PLAN-890. * fix(import-bundle): cascade attachment tombstone on rollback per Codex review (round 2) Codex P1 round 2 on PR #308: when the duplicate-manifest guard fires AFTER blobs have already been rehydrated (e.g. bundle layout [pad-export, manifest, blob1, blob2, duplicate-manifest]), the previous fix soft-deleted the workspace but left the attachment rows live. Live rows pin blobs from orphan-GC and continue counting toward per-user storage usage even though the workspace is gone. Fix: added Store.SoftDeleteWorkspaceAttachments(workspaceID), a single bulk UPDATE that tombstones every live attachment row (originals AND thumbnails — both carry the same workspace_id) under a workspace. The handler's rollback path now calls this BEFORE DeleteWorkspace so orphan-GC reclaims the blobs after the grace window. Best-effort: any error in either op is logged with workspace context but the original 400 still flows. New test: TestImportBundle_RollbackTombstonesAttachments builds a real export bundle from a source workspace with one attachment, surgically appends a duplicate manifest.json AFTER the real entries, posts it, and asserts (a) 400, (b) workspace gone from listings, (c) zero live attachment rows on the destination. The pre-fix code left rows=1 live; the new path tombstones them. Parent: PLAN-890. * fix(import-bundle): return ws on path-traversal reject so rollback fires (Codex round 3) Codex P1 round 3 on PR #308: the path-traversal early-return at the top of the import loop returned (nil, importStatusError) instead of (ws, importStatusError). When a malicious path-traversal entry follows a valid pad-export.json, the workspace was already created — but because the handler saw ws == nil, it skipped the rollback cascade, leaving the workspace and any rehydrated attachments behind. Fix: return ws (which is nil before pad-export.json is processed, so the no-workspace cleanup path still works for first-entry-bad bundles, and non-nil after, so the cascade runs). One-line change keyed off the existing rollback flow. New test: TestImportBundle_PathTraversalAfterExportRollsBack hand-builds a tar with a valid pad-export.json followed by a "attachments/../../etc/passwd" entry, posts it, asserts 400, and asserts the partial workspace is GONE from listings. Pre-fix this test would have shown the workspace leaking through. Parent: PLAN-890.	2026-04-29 20:24:25 -04:00
xarmian	2bb7ac35e4	feat(attachments): orphan GC sweep with periodic scheduler (TASK-886) (#307) ... * feat(attachments): orphan GC sweep with periodic scheduler (TASK-886) Background job that reclaims attachments past the grace period. Two qualification criteria, both with a 30-day default grace: - item_id IS NULL AND deleted_at IS NULL AND created_at < cutoff (never-attached uploads — editor uploaded then tab-closed before attaching to an item) - deleted_at IS NOT NULL AND deleted_at < cutoff (soft-deleted via the Settings → Storage delete button or the DELETE /attachments/{id} endpoint) Reclamation is dedupe-aware: content-addressed storage means the same hash can be referenced by multiple rows, so the on-disk blob is only removed when the GC'd row is the LAST live reference to its content_hash. Otherwise the row drops and the blob stays for the remaining references. CountLiveAttachmentsForHash is the predicate. Per-row failures (resolve backend, blob delete, hard-delete) are logged and skipped; the sweep keeps making progress. Catastrophic errors (DB failure) return up to the loop, which logs and waits for the next tick rather than crashing the server. Lifecycle: - SetOrphanGCConfig overrides the default 24h interval / 30-day grace. cmd/pad reads PAD_ORPHAN_GC_INTERVAL / PAD_ORPHAN_GC_GRACE (Go duration syntax — 1m, 24h, 720h) so operators can tune without recompiling and tests can crank the interval down to 1ms to see sweeps land in CI. - StartOrphanGC kicks the loop. Idempotent — second call is a no-op so a misconfigured caller can't double-spawn. - Server.Stop() now signals the loop via stopOrphanGC() before s.bg.Wait(), so process shutdown drains the goroutine cleanly (BUG-842 invariant). - Each tick wraps the sweep in a 30m context timeout so a slow scan can't pin the goroutine across multiple intervals. Tests: - TestOrphanGC_ReclaimsSoftDeleted: upload → soft-delete → sweep with future cutoff → DB row gone + blob gone from FSStore. - TestOrphanGC_ReclaimsLongOrphans: upload → backdate created_at 31d → sweep with 30d grace cutoff → row reclaimed. - TestOrphanGC_KeepsRecentRows: upload → soft-delete → sweep with past cutoff → row stays. Catches a typo in the WHERE clause that would silently destroy live attachments. - TestOrphanGC_PreservesSharedBlob: two uploads with identical bytes (same hash, same blob), soft-delete only one → sweep → one row reclaimed BUT BlobsReclaimed=0 because the other row still references the blob. Pin for content-addressed dedupe. - TestOrphanGC_StartStop: loop spins up at 1ms interval, second StartOrphanGC is a no-op, Stop drains via testServer's cleanup. Parent: PLAN-866. Closes the phase 1 plan with full export → import → orphan-cleanup round-trip. * fix(attachments): protect referenced/in-flight blobs from orphan GC per Codex (round 1) Two real correctness issues Codex caught on PR #307: P1. The editor's normal upload flow leaves attachments.item_id NULL. The canonical association lives in markdown content (the editor PATCHes "pad-attachment:UUID" into the item) — but the GC's "never-attached past 30d" predicate only checked item_id. So a legitimate inline image could be hard-deleted 30 days after upload even though item content still references it. Added store.AttachmentReferencedInItems(workspaceID, attachmentID) that scans items.content + items.fields for "pad-attachment:UUID". The GC sweep now runs this check before reclaiming any never-attached row; if any live item references the attachment, the row is left alone (and re-checked next sweep). P2. Race between concurrent upload and GC. Upload calls AttachmentStore.Put (blob lands on disk) → THEN inserts the DB row. Between those two steps an orphan-GC sweep could count zero live refs for the hash, delete the blob, and the upload's row insert would then point at a missing blob. Added Server.inFlightUploadHashes (sync.Map of *atomic.Int64 counters) with markUploadInFlight / uploadInFlight helpers. Every Put + CreateAttachment site fences itself via markUploadInFlight: the upload handler, the transform handler, the thumbnail derivation pipeline, and the bundle-import rehydrate path. The GC sweep treats an in-flight hash as "another live ref" so it leaves the blob alone. Tests: - TestOrphanGC_KeepsReferencedNeverAttachedRows: upload (item_id NULL) → create item with pad-attachment: ref → backdate 31d → sweep with 30d cutoff → row stays. - TestOrphanGC_RespectsInFlightUploads: upload → soft-delete → register an in-flight upload at the same hash → sweep → DB row goes (it's tombstoned past grace) but blob stays so the in-flight upload can complete cleanly. The DB row still gets reclaimed in the in-flight case because the soft-deleted row is independently past grace; only the blob delete is fenced. That's correct: the blob remains usable for the incoming upload and the new upload will register its own attachments row. * fix(attachments): mutex-protect in-flight tracker + portable JSONB scan per Codex (round 2) Two fixes for the round-2 findings on PR #307: P1. Same-hash race in the in-flight upload tracker. The sync.Map + *atomic.Int64 design split increment from LoadOrStore-then-add and release-decrement from delete, so a release could see "0" and start deleting while another upload concurrently reloaded the same map entry and incremented to "1" — the second upload's signal then lived in a doomed map slot, invisible to subsequent uploadInFlight calls. Replaced with a plain map[string]int64 + sync.Mutex. Inc, dec, delete-when-zero all run under one critical section, so any inspection sees a consistent snapshot. Net cost is one mutex per mark/release; uncontended this is ~10ns and the upload path is already doing far more expensive work (Put + DB insert). Stress test: 20 goroutines × 500 iterations of mark→check→release on a shared hash. Every check must observe in-flight=true while the calling goroutine holds the mark. Final state must be empty. Runs cleanly under -race -count=3. P2. Postgres JSONB compatibility. items.fields is TEXT on SQLite but JSONB on PostgreSQL (per pgmigrations/001_initial.sql). LIKE on JSONB fails with a type error, so the orphan GC's reference scan would error on Postgres and skip every never-attached row — breaking orphan reclamation for those rows entirely. Cast fields::text in the Postgres dialect path: fieldsExpr := "fields" if s.dialect.Driver() == DriverPostgres { fieldsExpr = "fields::text" } Same approach used elsewhere in the store for dialect-sensitive text searches. * fix(attachments): close GC/upload TOCTOU + protect in-grace peers per Codex (round 3) P1 round 3: TOCTOU race between uploadInFlight check and store.Delete. The mutex protected the in-flight counter but not the GC's check-and-delete sequence. A new upload could call markUploadInFlight between our check and our blob delete, then run Put after the blob was gone — its CreateAttachment would insert a live row pointing at the missing hash. Fixed by holding inFlightHashesMu across the check + FS Delete: s.inFlightHashesMu.Lock() inFlight := s.inFlightHashes[hash] > 0 if !inFlight && others == 0 { store.Delete(ctx, key) } s.inFlightHashesMu.Unlock() A concurrent markUploadInFlight blocks until either we skip (because we observed in-flight) or finish deleting. Lock window is ms-class on FSStore; a per-hash lock can replace this server-wide mutex when S3 lands in Phase 2. P2 round 3: CountLiveAttachmentsForHash counted only live rows, so GC could reclaim the blob from row A (soft-deleted 31d ago) even when row B was also soft-deleted but only 1 day old — within grace, so its blob must stay reachable until its own grace lapses. Replaced with CountProtectingAttachmentsForHash which counts rows where deleted_at IS NULL OR deleted_at >= graceCutoff. The blob is preserved until every soft-deleted peer has aged past its own grace window. Tests: - TestOrphanGC_RespectsSoftDeletedInGracePeer: two rows sharing a hash, soft-delete both, backdate only one past 30d → sweep with 30d cutoff → older row reclaimed but blob stays for the still-in- grace peer. - existing TestOrphanGC_RespectsInFlightUploads still passes (still uses the in-flight signal correctly). * fix(attachments): dedupe blob-reclaim metric across same-hash peers per Codex (round 4) Codex round 4 noted that when multiple soft-deleted peers share a content_hash and all are past grace, the GC sweep would inflate BlobsReclaimed and BytesReclaimed: AttachmentStore.Delete treats a missing key as success, so the second peer's idempotent no-op delete still bumped the counter. Functional cleanup was correct (the blob really was gone after the first peer); only the metric / log line was wrong, which makes operator dashboards report fictitious bytes-reclaimed values. Track per-sweep reclaimed hashes in a map and skip the Delete call + counter increment for repeats. The DB row still gets hard-deleted on each peer. Test: TestOrphanGC_DedupesBlobReclaimMetric uploads twice with identical bytes (single shared blob), soft-deletes both, backdates deleted_at past grace → sweep deletes 2 rows and reports BlobsReclaimed=1 / BytesReclaimed=blobLen rather than 2 / 2*blobLen.	2026-04-29 19:25:55 -04:00
xarmian	134f55045d	feat(attachments): import workspace bundle with rehydrate + UUID remap (TASK-885) (#306) ... * feat(attachments): import workspace bundle with attachment rehydrate + UUID remap (TASK-885) POST /workspaces/import now accepts a tar.gz bundle (Content-Type: application/gzip) and rebuilds the workspace + attachments + items in one round trip. JSON imports still work — content-type dispatch in handleImportWorkspace routes the request. Three-phase flow: 1. Walk the tar, capture pad-export.json + manifest.json + every attachment blob into memory. 2. Run the existing ImportWorkspace path to create the workspace + collections + items + comments + links + versions. New IDs are generated; item.slug is preserved (the existing remap path doesn't re-slugify). 3. For each manifest entry, rehydrate the blob through the storage backend (re-validate MIME + re-hash defensively, don't trust the manifest), insert a fresh attachments row. Build an oldID→newID map keyed on attachment uuid. 4. Walk every imported item's content + fields, replace "pad-attachment:OLD" with "pad-attachment:NEW" in one transactional pass. Refresh FTS afterward (direct UPDATE bypasses triggers). Phase 2 errors per-attachment are logged and skipped — the workspace keeps importing rather than rolling back. The import handler returns the new workspace and the operator can inspect logs for any attachment that didn't make it. CLI: - pad workspace export now defaults to --bundle (.tar.gz) since pad import handles bundles. --json reverts to legacy items-only. - pad import auto-detects format by file extension (.tar.gz / .tgz → application/gzip). Other extensions go through the legacy JSON path. - New Client.PostRawWithContentType for explicit-content-type POSTs. Tests: - TestImportBundle_RoundTrip: upload → embed in markdown → export source → import to FRESH server → verify attachment list has 1 row with new UUID → item content rewritten to new UUID and old UUID is gone → download new blob matches original bytes. - TestImportBundle_LegacyJSONStillWorks: JSON content-type still hits the legacy path. - TestImportBundle_RejectsBadGzip: garbage gzip body returns 400. Parent: PLAN-866. With TASK-884 + TASK-885 merged, the round-trip acceptance criterion (export → import → images intact) is met. * fix(attachments): stream import end-to-end per Codex (round 1) Two memory regressions Codex caught on PR #306: P1 (server). importBundle was buffering every blob into a map[string][]byte during a first pass, then iterating the manifest on a second pass. A 2 GiB bundle full of 25 MiB attachments would pin ~2 GiB of heap. Reworked to single-pass streaming: pad-export.json → import workspace + build slug→id map attachments/manifest.json → index entries by tar path attachments/<uuid>.<ext> → look up entry, rehydrate now The export bundler always writes pad-export.json + manifest.json BEFORE any blob (deterministic order from handlers_export_bundle.go), so this works without buffering. Bundles that violate the ordering — a third-party tool that writes blobs first — return 400 with a clear error. Memory footprint now bounded by the largest single blob (≤25 MiB) regardless of bundle size. Stale blobs without a manifest entry are skipped (their bytes io.Copy'd to io.Discard so the tar reader stays in sync). Unknown top-level entries (forward-compat for future bundle additions) are also consumed and ignored rather than left dangling. P2 (CLI). pad import used os.ReadFile, buffering the entire bundle client-side before posting. Switched to os.Open + a new Client.PostStreamWithContentType helper that streams the body directly into the request — together with the server-side fix, import is end-to-end streaming. Tests: - TestImportBundle_RejectsOutOfOrderTar: hand-crafted bundle with a blob before pad-export.json returns 400 with "ordering" in the message. - existing TestImportBundle_RoundTrip / LegacyJSONStillWorks / RejectsBadGzip continue to pass under the new streaming flow. * fix(cli): give streaming endpoints a 1h timeout per Codex (round 2) Codex P1 round 2: PostStreamWithContentType + RawStream were both using the shared 10s-timeout httpClient. The default works fine for normal API calls but kills a multi-GiB bundle import or export over anything slower than a local network — Client.Timeout fires mid-stream with "Client.Timeout exceeded". Added a dedicated streamClient on Client with a 1h timeout, used by both RawStream (export bundle download) and PostStreamWithContentType (import bundle upload). 1h is generous enough for ~100 MB/s uplinks shipping a 350 GiB bundle and still caps a hung connection eventually. The 10s default stays in place for every other call — short timeouts are the right SLA for normal API requests and protect the CLI from hanging on a wedged server. * fix(attachments): make import bundle cap configurable per Codex (round 3) Codex P1: the 2 GiB import cap was hard-coded with a comment promising operator override "later" — but no setter existed, so workspaces over 2 GiB stream out fine on export and fail on re-import. Added Server.SetImportBundleMaxBytes wired from the PAD_IMPORT_BUNDLE_MAX_BYTES env var in cmd/pad/main.go. Mirrors the existing PAD_ATTACHMENT_MAX_BYTES pattern. Default stays at 2 GiB so the typical workspace works without configuration; operators with larger exports can raise it without recompiling. The per-blob cap (importBlobMaxBytes = 25 MiB) is intentionally kept constant — it bounds in-flight memory regardless of total bundle size, and a 25 MiB-per-blob ceiling matches the upload handler's default, so a bundle can never smuggle larger blobs than the upload endpoint accepts. * fix(attachments): scale per-blob import cap with PAD_ATTACHMENT_MAX_BYTES per Codex (round 4) Codex P1 round 4: importBlobMaxBytes was hard-coded at 25 MiB but the upload handler's per-file cap is configurable via PAD_ATTACHMENT_MAX_BYTES. An operator who raised the upload cap to allow 50 MiB attachments could export a workspace successfully (WorkspaceAttachmentsForExport doesn't gate on size) but the re-import would reject every blob over 25 MiB. Replaced the const with effectiveBlobMaxBytes() which reads s.attachmentMaxBytes (or falls back to defaultAttachmentMaxBytes). The pad-export.json cap also scales with this value (4×) so a content-heavy workspace doesn't trip its own JSON ceiling on a server with raised attachment limits. Error message on a too-large blob now points the operator at PAD_ATTACHMENT_MAX_BYTES so they know which knob to turn rather than digging through code to find the cap. * fix(attachments): independent metadata cap for bundle import per Codex (round 5) Codex P2 round 5: tying pad-export.json + manifest.json caps to PAD_ATTACHMENT_MAX_BYTES regressed deployments that LOWER the attachment cap. A 1 MiB attachment cap would force metadata to fit in 4 MiB / 1 MiB respectively — but metadata size scales with workspace item count, not attachment blob sizes, so a tight upload limit shouldn't gate it. Added importMetadataMaxBytes = 100 MiB constant for both metadata files. effectiveBlobMaxBytes() still drives the per-blob cap which genuinely tracks attachment-upload policy.	2026-04-29 18:56:43 -04:00
xarmian	a0336e0248	feat(attachments): bundle attachments + manifest in workspace export (TASK-884) (#305) ... * feat(attachments): bundle attachments + manifest in workspace export (TASK-884) GET /workspaces/{ws}/export?format=tar streams a gzip'd tar bundle: pad-export.json # the existing WorkspaceExport JSON attachments/manifest.json # uuid → {filename, mime, size, hash, ...} attachments/<uuid>.<ext> # original blobs only — no thumbnails Default (no ?format) keeps returning JSON so existing automation hitting the endpoint without a query param continues to work unchanged. The CLI's pad workspace export now opts into the bundle by default; pass --json for the legacy items-only output. Implementation: - store.WorkspaceAttachmentsForExport returns originals only (parent_id IS NULL); thumbnails are re-derived on import via the existing pipeline so shipping them would double the bundle size. - handleExportWorkspaceBundle streams chunks straight into the response writer rather than buffering — a workspace with multi- GB of attachments would otherwise pin that much memory. - AttachmentManifest is versioned (separate from WorkspaceExport version) so the bundle layout can evolve independently. - bundleAttachmentPath is exported (lowercase package fn) so the import path in TASK-885 can resolve manifest entries to tar entries without duplicating the filename logic. - CLI gates against writing binary tar.gz to a TTY and appends the conventional extension when -o is passed without one. Tests: - TestExportBundle_RoundTrip: two uploads → bundle contains pad-export.json + manifest + 2 blobs whose bytes match the uploads + manifest decodes cleanly + WorkspaceExport decodes. - TestExportBundle_HidesThumbnails: synthetic thumbnail row, the manifest excludes it. - TestExportBundle_LegacyJSONStillWorks: no ?format param returns application/json with a decodable WorkspaceExport (backward compat regression guard). Parent: PLAN-866. TASK-885 (import path + UUID remap) consumes the manifest produced here. * fix(attachments): stream export bundle + revert default to JSON per Codex (round 1) Two findings from Codex on PR #305: 1. CLI buffered the entire response in memory via RawGet → io.ReadAll, defeating the server-side streaming design and risking OOM on a multi-GB bundle. Added Client.RawStream which copies the response body straight into an io.Writer; export now opens the target file and streams directly into it. 2. Default tar.gz output broke `pad export → pad import` round trip because the import handler still only accepts JSON. Reverted the CLI default to JSON; bundle is now opt-in via --bundle. The flag docstring notes that TASK-885 will flip the default once import handles bundles. * fix(attachments): surface tar/gzip close errors and truncation per Codex (round 2) Codex round 2 finding: deferred tw.Close() / gzw.Close() ignored errors. If a backend returned fewer bytes than size_bytes claimed, io.Copy returned nil, the tar writer's "missed N bytes" trip fired at Close, and the handler still completed a 200 OK with a corrupt bundle that gunzip would later refuse to decompress — silently from the operator's perspective. Two changes: 1. The deferred close now logs both tw.Close() and gzw.Close() errors with structured context, so a corruption-on-finalize trip shows up in the operator log. 2. streamAttachmentToTar checks the bytes-copied count against a.SizeBytes after io.Copy and returns a per-attachment error when they disagree. The error is logged with attachment_id + storage_key so an operator can correlate the corruption with the row to investigate. Regression test: TestExportBundle_TruncatedBlobLogsError forces a size_bytes/blob desync via direct UPDATE and asserts the resulting bundle bytes don't decode cleanly. (HTTP status stays 200 because headers are already on the wire by the time we detect the desync; that's an inherent limitation of mid-stream errors, but the new logs + close-error surfacing make the failure observable.) * fix(attachments): X-Bundle-Status trailer for export-stream success per Codex (round 3) Codex P1 round 3: even with the per-blob truncation log + tar/gzip close-error logs, mid-stream failures looked successful to clients. The CLI's RawStream finished without a transport error, the file landed on disk, and "Exported workspace" printed regardless of whether the bundle was actually complete. Two complementary signals now mark a clean stream: 1. HTTP trailer X-Bundle-Status. The handler declares the trailer in the initial Trailer header and sets it to "ok" only after tw.Close() and gzw.Close() both return without error. CLI checks the trailer after streaming and discards the file + returns error if it's absent or non-"ok". 2. The handler skips the deferred clean close on the error path, leaving the gzip footer unwritten. A client that ignores the trailer (curl, third-party tooling) still sees a corrupt gzip stream that gunzip refuses to decompress. CLI: pad workspace export --bundle now removes any partial output file on failure rather than leaving a corrupt one behind. Client.RawStream signature changed to return (bytes, *http.Response, error) so callers can inspect resp.Trailer; the only caller is the export command. Tests: TestExportBundle_TruncatedBlobAbortsStream now asserts both signals (trailer absent + gzip/tar can't fully decode), and TestExportBundle_SuccessTrailer pins the happy-path trailer.	2026-04-29 18:30:43 -04:00
xarmian	d3a543db6f	feat(attachments): admin per-user storage quota override UI (TASK-883) (#304) ... * feat(attachments): admin per-user storage quota override UI (TASK-883) Surfaces the storage_bytes plan_overrides key in the admin user-detail page so operators can lift or tighten an individual user's quota without poking at JSON via the API directly. Frontend (console/admin/+page.svelte): - Dedicated "Storage quota override" input below the existing overrides grid. Storage is byte-counted, not row-counted, so a number input forcing the admin to type 536870912 for 512MB would be hostile. Accepts: • "10 GB" / "500MB" / "1.5 GB" (IEC shorthand) • "1024" (raw bytes) • "-1" (unlimited) • "" (clear → falls back to plan default) - Live parse preview ("= 10.0 GB (10,737,418,240 bytes)") so the admin can verify the unit was understood. - "Reset to plan default" button clears the field; save commits the absence as a removed override key. - Pre-fills with the current effective override formatted in the largest exact unit so a previously-set "10 GB" doesn't reload as "10737418240". Backend: - ActionPlanOverridesChanged audit constant. - handleAdminUpdateUser now logs an audit event with old/new override JSONs whenever plan_overrides is patched. Lets operators correlate a mysteriously-allowed upload with the override that enabled it. Tests: - TestAdminUpdateUser_StorageOverrideRoundTrip: PATCH with storage_bytes:1073741824 → GET shows the new override → audit feed contains plan_overrides_changed event → clearing the override removes it. - TestAdminUpdateUser_NonAdminForbidden: member-role user cannot PATCH another user's plan_overrides (regression guard for the audit-log path). Parent: PLAN-866. The Settings → Storage page (TASK-882) reflects the new effective limit immediately after save because both call the same WorkspaceStorageInfo helper. * fix(admin): parse plan_overrides JSON on read, clear via empty string per Codex (round 1) Two related bugs in the admin user-detail page that Codex caught in PR #304 round 1: 1. The save path sent JSON null when every override field was blank, but the Go handler uses a *string and JSON null decodes to a nil pointer — the handler's existing nil-vs-non-nil branch then skips the update, meaning "Reset to plan default" reported success without actually clearing the override. Fixed by sending "" (empty string) which routes through SetUserPlanOverrides("") and clears the column. 2. The form-populate path treated u.plan_overrides as an object while the API actually returns the raw column value as a JSON string. So `'storage_bytes' in ov` was checking string indices on a literal '{"storage_bytes":1073741824}' string, returning false, and any user with stored overrides loaded a blank form. This was a pre-existing bug in the workspaces / api_tokens / etc. fields too — fixed for all of them by parsing the JSON in parsePlanOverrides() before reading keys, with a defensive "future-proof" branch in case the API ever switches to a decoded object. TS type for AdminUser.plan_overrides updated to `string | null` to match the actual API contract. Backend regression test added (TestAdminUpdateUser_OmittedOverrides Preserved) that pins the other half of the contract: PATCH with plan_overrides absent must NOT clear the column. The test was straightforward to add because the existing test infrastructure (bootstrapFirstUser, doRequestWithCookie) already covers the admin auth path.	2026-04-29 17:56:19 -04:00
xarmian	504d348917	feat(attachments): Settings → Storage tab with attachment list (TASK-882) (#303) ... * feat(attachments): Settings → Storage tab with attachment list (TASK-882) Adds the Settings → Storage tab and the underlying list/delete API endpoints so workspace owners can audit and reclaim attachment bytes. Backend (TASK-882 needs this — there was no list/delete API yet): - store.WorkspaceAttachments: paginated list with filter (category, attached/unattached, collection_id) + sort allowlist (size, filename, created_at — each with desc variant). LEFT JOIN to items + collections enriches each row with item_title/slug + collection_slug for the "in [[Item]]" link. Hides derived (thumbnail) rows by default — they count toward quota but are managed automatically and would clutter the user-facing list. - store.SoftDeleteAttachment: tombstones the row + every variant. Blob on disk stays put; orphan GC reclaims past the grace period (TASK-886). - GET /workspaces/{ws}/attachments — viewer+, returns {attachments, total, limit, offset}. - DELETE /workspaces/{ws}/attachments/{id} — editor+. Refuses to delete derived rows directly (returns 400 with derived_attachment code) and invalidates the storage-usage cache. Frontend: - StorageTab.svelte component (lib/components/settings) with usage bar (color thresholds at 80%/100%, override badge), 5-select filter row (category, item, collection, sort, page size), attachment list with thumbnails (image variants via thumb-sm, emoji icon otherwise), item link, MIME, size, date, and per-row delete with confirm() dialog. Pagination footer with Prev/Next + "showing X–Y of Z". - TS api.attachments.list() / delete() + types. - Wired as a new "Storage" tab on the workspace settings page. Tests: - TestListAttachments_Pagination: 3 uploads, default + size-asc sort, limit/offset paging. - TestListAttachments_HidesDerived: synthetic thumbnail row, asserts the list excludes parent_id != NULL rows. - TestDeleteAttachment_HappyPath: upload → delete → list empty → storage usage drops to 0 (cache invalidation hook fires) → second delete returns 404. - TestDeleteAttachment_DerivedRefused: thumbnail rows can't be deleted directly via the API. Parent: PLAN-866. * fix(attachments): collection visibility + category gaps + item ref shape per Codex (round 1) Three findings from Codex on PR #303 round 1: P1 — Collection visibility leak. The storage list returned all workspace attachments without applying per-user collection access, so a member with collection_access=specific would receive hidden collections' attachment IDs/filenames/item titles and could then pull the bytes via the existing download endpoint. Fixed by threading visibleCollectionIDs(r, workspaceID) through to the store filter. nil = admin/no restriction; empty slice = zero visible collections (zero rows by design); explicit set = restrict i.collection_id IN (...). Orphans (item_id IS NULL) are excluded for restricted users since their filenames would still leak. P2 — item_ref shape didn't match the route. The store synthesized "<collection_slug>/<item_number>" and the UI inserted it verbatim into the URL, producing /user/ws/tasks/tasks/5. Dropped item_ref entirely; UI now builds URLs from item_slug + collection_slug which is the actual route shape. P2 — Category filter coverage. mimePrefixForCategory only handled image/video/audio. Selecting Documents/Text/Archive/Other in the UI silently passed through with no MIME predicate so the list showed everything. Replaced with mimePredicateForCategory which emits the right SQL fragment per bucket: prefix LIKE for the type/ buckets, explicit IN list for document/text/archive (mirroring the allowlist in internal/attachments/mime.go), and a NOT-IN composite for "other". Tests: - TestWorkspaceAttachments_VisibilityFilter: admin sees all 3 rows; restricted to one collection sees only that collection's row + orphan suppressed; empty visibility yields zero rows. - TestWorkspaceAttachments_CategoryFilters: image/document/text/ archive/other each return exactly the matching MIME types. * fix(attachments): item-level visibility on list + delete per Codex (round 2) Two more findings from Codex on PR #303 round 2: 1. The list filter used VisibleCollectionIDs alone — but that set includes collections containing any item-level grant for the user. A guest with one item granted in collection B would still receive attachment metadata for every item in collection B. Replaced with the (fullCollIDs, grantedItemIDs) tuple from guestResourceFilter so the SQL ORs collection-level full access against per-item grants, matching how handlers_search / handlers_activity narrow lists. 2. The delete endpoint validated workspace membership but never checked the attachment's parent item is visible to the caller. An editor with restricted collection access could delete attachments in hidden collections by guessing/obtaining the attachment ID. Added requireItemVisible after fetching the parent item, plus a fallback gate for orphan attachments (item_id IS NULL) so restricted users get 404 there as well. Store-level filter renamed: VisibleCollectionIDs → Restricted + FullCollectionIDs + GrantedItemIDs. Tests cover the collection-only, item-grant-only, and zero-visibility paths. * fix(attachments): allow deleting attachments when parent item is soft-deleted (round 3) Codex P2 from PR #303 round 3: the storage list intentionally surfaces attachments whose parent item has been soft-deleted (so the user sees what's still consuming quota), but the delete handler used GetItem, which filters soft-deleted out and returned 404 before SoftDeleteAttachment could run — turning every Delete button on those rows into a no-op. Fixed by adding store.GetItemIncludeDeleted (mirroring the existing GetItemBySlugIncludeDeleted) and switching the delete path to use it. The visibility check still keys off the (still-set) collection_id, so soft-deleting an item doesn't escalate access — restricted users still hit requireItemVisible's 404 if they couldn't see the parent. Regression test: create item → attach → soft-delete item → list still returns the row → delete returns 204. * fix(attachments): list surfaces attachments under soft-deleted parents (round 4) Codex round-4 finding: WorkspaceAttachments still LEFT JOIN'd items with AND i.deleted_at IS NULL, so attachments whose parent item was soft-deleted disappeared from the list — even though the previous round wired GetItemIncludeDeleted on the delete path. Net effect: restricted editors with access to that collection couldn't discover the row in the UI; only full-access users saw it as an orphan-looking entry. Fix: drop the deleted_at filter from the JOIN. The collection ACL predicate (i.collection_id IN ...) now sees the (still-set) collection_id from the soft-deleted item, so visibility behaves consistently for live and tombstoned parents. Soft-deleted items don't escalate access — the collection_id stays put. UX: response now carries item_deleted=true when the parent is soft-deleted; the StorageTab renders the title with strike-through + a small "deleted" badge instead of a clickable link (which would 404). Tests: - store-level: admin/full-access sees the row + ItemDeleted flag, restricted-to-correct-collection sees it, restricted-to-other- collection does not. - (existing TestDeleteAttachment_AfterParentSoftDeleted continues to pass on the handler side.)	2026-04-29 17:44:12 -04:00
xarmian	335762c2bf	feat(attachments): storage usage API + effective-limit computation (TASK-881) (#302) ... * feat(attachments): storage usage API + effective-limit computation (TASK-881) Adds GET /api/v1/workspaces/{ws}/storage/usage returning {used_bytes, limit_bytes, plan, override_active}. Resolves the effective limit through the existing three-tier chain (per-user override → platform setting → hardcoded plan default) and surfaces the override flag for the upcoming Settings → Storage and admin user-detail UIs. Implementation: - store.WorkspaceStorageInfo consolidates SUM(size_bytes) + owner-plan resolution in one call; WorkspaceStorageLimit is now a thin wrapper so the upload-time quota check and the API path stay consistent. - Server.storageInfoCache is a 30s TTL memoizer to absorb repeated Settings → Storage page loads. Invalidation hooks fire on upload, thumbnail derivation, and transform — the ~30s eventual-consistency window is bounded by TTL only when invalidation isn't reachable. - Defensive copy on cache read so a caller mutating the returned struct can't poison subsequent reads. - New CLI command `pad workspace storage` prints "X used of Y (Z%)" with IEC units (humanBytes helper) and surfaces the override flag. - TS api.attachments.storageUsage() + WorkspaceStorageInfo type ready for TASK-882's Settings → Storage page consumer. Tests: - Store-level: no-owner fallback, free-plan resolution chain, override flip, pro-plan override-active visibility, soft-delete exclusion. - Server-level: empty-workspace happy path, two uploads with cache invalidation between, dedicated cache TTL/invalidate/copy-safety test. Parent: PLAN-866. * fix(attachments): gate storage usage on viewer+ per Codex review (round 1) Codex correctly flagged that the storage/usage handler relied solely on RequireWorkspaceAccess, which admits item-grant guests with workspaceRole=="guest". Workspace-wide quota numbers (used_bytes, plan, override status) shouldn't surface to guests — every other workspace- level read handler uses requireMinRole("viewer") for exactly this case. Adds the explicit gate + a regression test that calls the handler with a guest-role context and asserts 403.	2026-04-29 17:05:19 -04:00
xarmian	756d91acad	fix(ci): gofmt + bump race-detector timeout to 30m (#299) ... CI on main has been failing since the PLAN-866 attachment work landed. Two independent issues: 1. gofmt failures (golangci-lint) — seven files in the attachments path had trailing-comment alignment that gofmt wanted nudged a column. Pure whitespace; ran `gofmt -w` across the affected files. golangci-lint's gofmt linter caught it on every PR / push since TASK-870 but we hadn't been watching those signals. Files cleaned: internal/attachments/{fs_store_test,mime, mime_test,processor_test}.go, internal/server/{ handlers_attachments_download_test,handlers_attachments_transform, render/attachments_test}.go. Local guard: `gofmt -l ./...` now exits clean. 2. Race-detector tests timed out at 20m on the GitHub-hosted runner. Two contributors: - PostgreSQL adds latency on every CREATE/DROP plus on the bcrypt hash inside auth/bootstrap (~3s per call under -race on the runner). Tests that bootstrap a fresh user (e.g. TestSessionIPChange_*) pay the full cost each time. - The PLAN-866 image-processing tests (thumbnail derivation, rotate / crop transform) added ~2-3 minutes of decode/encode work on top of the existing suite. The previous "20m gives margin without papering over a hang" comment was right at the time it was written; we now genuinely need more headroom. Bumped to 30m on both the SQLite and PostgreSQL race steps. Genuine deadlocks would still trip this and produce the goroutine-dump panic — we just stop confusing "slow but progressing" with "permanently hung". Reference points before / after: - TASK-875 main run #294: Go (PostgreSQL) finished in 17m48s ✓ - TASK-880 main run #298: Go (PostgreSQL) hit 20m timeout ✗ - Local: my new tests under -race add ~63s on a developer laptop (TestThumbnails + TestTransform + TestProcessor combined). Verification: go test ./... — pass go vet ./... — clean gofmt -l (recursively) — clean	2026-04-29 15:41:12 -04:00
xarmian	3bf1b60365	feat(attachments): editor image crop with aspect presets (TASK-880) (#298) ... Adds a drag-to-crop modal on top of the AttachmentImage toolbar introduced in TASK-879. The /transform endpoint already accepted the "crop" operation shape from TASK-879 — this PR wires the editor UI plus the supporting tests. Editor: - attachment-crop-modal.ts (new): pure-DOM crop modal in the same style as the existing image lightbox. Returns a Promise that resolves to the crop rect in ORIGINAL-IMAGE pixel coordinates when the user clicks Apply, or null on cancel / dismiss / image-load failure. - Image fits to a centered <dialog> via flex layout; backdrop click and Esc both cancel cleanly. - Crop rectangle starts at 80% of the image, centered. Body is a "move" handle; four corner handles resize. - Aspect presets: Free, 1:1, 4:3, 16:9. Preset clicks snap the current rect to the new ratio while preserving its center; subsequent corner drags clamp to the locked ratio. - Pointer events (touch + mouse for free) with setPointerCapture so drag continues even if the cursor leaves the handle. - Coordinate translation: rect in preview-pixel space → naturalWidth / offsetWidth scale → original-image pixel space. Result is clamped to natural bounds so a fractional- rounding overrun doesn't push the rect off-image. - attachment-image.ts: extracts swapNodeUuid() helper from runRotate so runCrop can share the setNodeMarkup + invalidate-old-metadata flow. The toolbar gains a fourth button (⌶ Crop…) that opens the modal pointed at the original variant. Per-format gating (refreshToolbarState) treats the crop button identically to the rotate trio — both go through /transform, so a libvips-only format (e.g. WebP on the pure-Go build) disables the whole toolbar with the same explanatory tooltip. - app.css: full styling for the crop modal — header with aspect toolbar, image stage with shadow-cutout overlay around the crop rect, four corner handles, footer with Cancel + Apply. Uses the existing CSS-variable palette so light/dark mode track automatically. Server tests (3 new): - TestTransform_CropProducesNewBlobAtRectDimensions: end-to-end PNG crop, verify the response dimensions AND that the served bytes decode at the same dimensions (guards against an encode-pipeline off-by-one). - TestTransform_CropClipsToImageBounds: rect that extends past the image boundary clips rather than 400ing — the editor's rounding can produce rect+1px past natural width/height in rare fractional-scale cases, and the processor's Crop intersects with image bounds for exactly this reason. - TestTransform_CropRejectsBadRect: missing rect, zero width, negative xy, rect entirely outside → 400. Parent: PLAN-866. Closes the editor-side image-tools track on top of TASK-878 (Processor) and TASK-879 (rotate / transform endpoint).	2026-04-29 15:02:07 -04:00
xarmian	f93b0ee4ce	feat(attachments): server-side rotate transform + editor toolbar (TASK-879) (#297) ... * feat(attachments): server-side rotate transform + editor toolbar (TASK-879) Adds the POST /transform endpoint and the editor's rotate toolbar on top of TASK-878's Processor abstraction. Rotation produces a NEW content-addressed attachment row; the editor swaps the AttachmentImage node's UUID via setNodeMarkup and the original ages into orphan GC. Server (internal/server/handlers_attachments_transform.go): POST /api/v1/workspaces/{slug}/attachments/{id}/transform with body {operation, ...params}. Phase 1 wires the "rotate" branch (degrees: 90 | 180 | 270 only — pixel-exact reorderings, no resampling, matches what the editor emits). The "crop" branch is parsed and validated but the transform path is wired in TASK-880; defining the wire format here keeps both PRs aligned. Auth: editor+ on the workspace. Cross-workspace and deleted-parent probes return 404 (not 403) so the new endpoint can't become a side-channel for ID enumeration. Unsupported MIME → 415; oversized image → 413; bad params → 400; missing processor → 503. Output format follows the same PNG-stays-PNG / else-JPEG policy as the thumbnail pipeline so derived blobs deduplicate cleanly. Tests (10): rotate 90 swaps WxH, rotate 180 keeps WxH, bad degrees → 400, unknown op → 400, non-existent attachment → 404, cross- workspace → 404, no processor → 503, derived row has fresh hash + inherits workspace/uploader/item, served bytes decode at the new dimensions, deleted-parent → 404. Web client (web/src/lib/api/client.ts + types): api.attachments.transform(slug, id, payload) hits the new endpoint with a discriminated AttachmentTransformRequest type. New api.server.capabilities() reads the public capability profile added in TASK-878. Both surface PadApiError on failure so the editor can show actionable messages. Editor: - attachment-metadata.ts (new): shared HEAD-probe cache extracted from attachment-chip.ts so AttachmentImage's toolbar can probe the image's MIME with the same zero-extra-network-cost deduplication. Adds mimeToFormat() — maps MIME to the canonical short format name the server's Capabilities reports. - attachment-chip.ts: swapped to use the shared cache. Behavior unchanged. - attachment-image.ts: NodeView now wraps the <img> in a positioned <span> and lazy-builds a 3-button rotate toolbar (rotate left 90°, rotate 180°, rotate right 90°). selectNode shows it; deselectNode hides it. On click → calls options.transform → setNodeMarkup with the returned UUID at getPos(); cached metadata for the OLD UUID is invalidated. Per-button gating via refreshToolbarState: empty supportedFormats list (degraded build) → all disabled with a "this build doesn't have image processing" tooltip. MIME probed and not in supportedFormats → disabled with a format- specific tooltip ("Image editing for image/webp requires libvips"). Otherwise → enabled with the action tooltip. - Editor.svelte: configures AttachmentImage with the workspace slug, the supportedFormats list (initially empty, populated asynchronously after capabilities resolve), and the transform callback wired to api.attachments.transform. Errors surface via console.error + window.alert — same fallback as the upload plugin until a centralized toast system lands. - app.css: wrapper + toolbar styles. Toolbar pinned top-right with absolute positioning; selected-state ring on the image; disabled button state at 40% opacity. Parent: PLAN-866. Unblocks TASK-880 (crop) — the /transform endpoint already accepts the crop op shape, the editor's toolbar pattern is the same, and the supportedFormats gating composes cleanly. * fix(attachments): rotate attribution + toolbar refresh per Codex review (round 1) Two findings from the round-1 Codex review: 1. The transform handler set UploadedBy = currentUserOrSystem(r), contradicting the comment that said "inherit attribution from the parent" and creating an audit-attribution drift whenever a user rotated/cropped someone else's upload. Inherit parent.UploadedBy instead — same policy as the thumbnail pipeline. Added TestTransform_DerivedRowInheritsUploadedByFromParent to lock in the contract. Removed the now-unused currentUserOrSystem helper. 2. The rotate toolbar's per-format gating could permanently stick in "all-disabled" state if the user selected an image before the async capabilities fetch resolved. supportedFormats started as [] (matching "no processor"), refreshToolbarState ran once in that state, and the later mutation of ext.options. supportedFormats had no observer to push the change down to already-open toolbar DOM. Fix: module-level toolbarRefreshers set, populated by each NodeView at ensureToolbar() and torn down in destroy(); a new notifyAttachmentImageCapabilitiesChanged() export iterates the set and re-runs each toolbar's refresh hook. Editor.svelte calls it after the capabilities fetch updates ext.options.supportedFormats, so any toolbar opened during the in-flight request snaps to its correct state the moment caps arrive. Verification: go test ./internal/server -run TestTransform passes (11 cases now); npm run check passes with the existing 6 warnings.	2026-04-29 14:54:48 -04:00
xarmian	02be33902f	feat(attachments): ImageProcessor interface + pure-Go impl + thumbnail pipeline (TASK-878) (#295) ... * feat(attachments): ImageProcessor interface + pure-Go impl + thumbnail pipeline (TASK-878) Adds the abstraction Phase 1 needs to derive thumbnail variants on upload, with a pure-Go default implementation that keeps Pad's single-binary distribution intact (no cgo). The libvips-tagged build (Phase 2 / Pad Cloud Docker) will replace processor_purego.go with a vips-backed implementation behind the same Processor interface — see DOC-865. internal/attachments/processor.go: Processor interface — Decode(io.Reader)→(image.Image, format), Resize(img, maxLong), Rotate(img, deg), Crop(img, rect), Encode(img, format, w), Capabilities(). Capabilities struct (image_formats, can_transcode, max_pixels) surfaces what the editor needs to gate per-format rotate/crop UI on (TASK-879/880). ErrUnsupportedFormat + ErrImageTooLarge are separate sentinels so callers can distinguish "format not supported" from "image dimensions too big". internal/attachments/processor_purego.go (//go:build !libvips): Uses github.com/disintegration/imaging plus the stdlib decoders. Supports PNG/JPEG/GIF/BMP/TIFF for all ops. WebP/AVIF/HEIC reach Decode and bounce out via ErrUnsupportedFormat — uploads still succeed (the MIME allowlist is the upload gate), but thumbnails skip and the editor disables rotate/crop UI per Capabilities. Memory ceiling: Decode peeks via image.DecodeConfig (header only) before allocating any pixel buffer and rejects images whose width*height exceeds MaxPixelsDefault (8000² = 64MP). At 4 bytes per pixel that caps the decode buffer at ~256 MiB and prevents an attacker uploading a forged 100kx100k claim from OOMing the server. The forged-CRC test exercises this gate. internal/server/handlers_attachments_thumbnails.go: deriveThumbnails(parentID) runs in goAsync after every image upload. Generates thumb-sm (256px long edge) + thumb-md (1024px), each as its own attachments row with parent_id pointing at the original. Server.Stop() drains the goroutine before SQLite closes, so tests can assert post-conditions deterministically. Skip cases: parent deleted (race), source format not supported (logged at debug), source already smaller than the variant's bound, variant already exists (idempotent reruns). Variants count toward workspace storage usage — DOC-865 is explicit about this and TestThumbnails_CountsTowardWorkspaceUsage proves it. Output format policy: PNG inputs stay PNG to preserve transparency; everything else encodes as JPEG q=85. internal/server/handlers_capabilities.go: GET /api/v1/server/capabilities returns the Processor's static capability profile under {image: {...}}. Public route — the editor needs it before login (e.g. shared-item preview surfaces). Reports an empty image-formats list when no processor is wired, signalling the editor to disable rotate/crop UI rather than 500-ing the editor mount. cmd/pad/main.go: wires SetImageProcessor(NewProcessor()) alongside SetAttachments at startup; logs the supported formats so operators know whether they're on the pure-Go or libvips build. Tests: - processor_test.go: 12 unit tests covering capability profile, decode round-trip for PNG/JPEG/GIF, rejection of unsupported formats and oversized images (forged-CRC PNG), resize aspect preservation + pass-through for already-small inputs, rotate multiples-of-90 + negative + 360-modulo handling, crop with bounds clipping + empty-intersection rejection, encode round- trip for PNG/JPEG, ThumbnailFormat/Mime/Ext policy. - handlers_attachments_thumbnails_test.go: 5 integration tests covering thumb-sm + thumb-md generation on PNG/JPEG uploads, skip-when-source-already-small, ?variant=thumb-md serving via the existing GET handler, workspace usage accounting. - handlers_capabilities tests cover the happy path + the no-processor degraded path. Parent: PLAN-866. Closes the thumbnail-fallback gap that TASK-874 / TASK-876 left open (thumb-md URLs were falling back to original because no thumbnails existed). Unblocks TASK-879 (rotation tool) and TASK-880 (crop tool) — both will reuse Processor.Rotate / Processor.Crop with the same Capabilities-driven UI gating. * fix(attachments): make /server/capabilities public per Codex review (round 1) Codex flagged that GET /api/v1/server/capabilities was registered inside the auth-gated API group but missing from isPublicAPIPath, so once any user existed the editor's pre-login fetch would 401 — contradicting the route's "public" register-time intent and breaking the share-preview surface. Fix: add the path to isPublicAPIPath. The handler is read-only, returns a static profile, and has no per-user state, so making it public has no security implication. Added TestServerCapabilities_PublicAfterBootstrap as a regression guard: it bootstraps an admin (so RequireAuth is active) and then fetches the endpoint with no auth cookie, asserting 200. * fix(attachments): make -tags libvips compile per Codex review (round 2) Codex flagged that build tag !libvips on processor_purego.go meant NewProcessor + the Thumbnail* helpers were absent under \`go build -tags libvips\`, so cmd/pad/main.go and the thumbnail handler — which call them unconditionally — broke that build. Two minimal fixes preserving the documented Phase 2 split: 1. Move ThumbnailFormat / ThumbnailMime / ThumbnailExt out of the tagged file and into processor.go (untagged). They're pure format-name policy, not implementation specifics, so both backends share the same definitions. 2. Add processor_libvips.go (//go:build libvips) with a stub NewProcessor that panics at runtime with a clear "Phase 2 hasn't shipped libvips yet" message. The libvips build now compiles; anyone actually instantiating the processor under that tag gets a loud failure rather than a silent degradation. Phase 2 will replace the body with the real govips-v2-backed implementation. Verified: \`go build ./...\` and \`go build -tags libvips ./...\` both clean. Existing tests still pass on the default tag. * fix(attachments): make tests compile under -tags libvips per Codex review (round 3) Codex flagged that running \`go test -tags libvips ./internal/attachments\` or \`./internal/server\` panicked through the libvips NewProcessor stub: processor_test.go and the thumbnail/capability server tests all called NewProcessor() unconditionally, even though the libvips build's stub is intentionally panicking until Phase 2 ships the real implementation. Three minimal fixes: 1. Tag processor_test.go !libvips. It tests the pure-Go implementation specifically — there's no value in running it under libvips, and the stub processor would explode the moment NewProcessor() ran. 2. Tag handlers_attachments_thumbnails_test.go !libvips. Same reasoning — these integration tests assert thumbnail derivation against a working processor. 3. Split testServerWithAttachments's processor wiring into two build-tagged helper files: * testimageprocessor_purego_test.go (//go:build !libvips) wires the real pure-Go processor. * testimageprocessor_libvips_test.go (//go:build libvips) is a no-op so the rest of the server test surface (uploads, downloads, auth, etc.) compiles + runs cleanly under -tags libvips. Verification: go build ./... — OK go build -tags libvips ./... — OK go test ./internal/attachments ./internal/server (default) — pass go test -tags libvips ./internal/server -run "TestUpload|TestDownload" — pass Phase 2 will introduce a real libvips test backend and drop the !libvips tags on the thumbnail tests. * fix(attachments): libvips binary boots cleanly per Codex review (round 4) Codex flagged that the libvips build still crashed at \`pad serve\` startup: cmd/pad/main.go calls attachments.NewProcessor() unconditionally, and the libvips stub was panicking — so any operator who built with -tags libvips today (Phase 2 isn't shipped yet) lost the entire server, not just image processing. Two minimal changes: 1. processor_libvips.go: stop panicking. Return nil + slog.Warn instead. Every call site already nil-checks the processor (the upload handler skips thumbnail derivation, the capabilities endpoint reports a degraded empty formats list), so the libvips-tagged binary now has the same runtime profile as a self-host build that opted out of image processing entirely — uploads succeed, originals display, only derived transformations are unavailable. The slog.Warn keeps the "this build doesn't have it yet" signal loud. 2. cmd/pad/main.go: skip srv.SetImageProcessor when NewProcessor returns nil, and log a "not wired" message in that branch. Distinguishes the wired vs. unwired states cleanly in the boot log. Phase 2 will replace processor_libvips.go's body with the real govips-v2-backed implementation; main.go's wiring is already shape- correct for that transition. Verification: go build ./... — OK go build -tags libvips ./... — OK go test ./... — pass (74s server tests included) go test -tags libvips ./internal/server -run "TestUpload|TestDownload|TestServerCapabilities_Public" — pass	2026-04-29 14:35:49 -04:00
xarmian	934794b606	feat(attachments): editor file chip node (TASK-877) (#293) ... * feat(attachments): editor file chip node (TASK-877) Custom Tiptap node `attachmentChip` for non-image `pad-attachment:UUID` references — Notion-style chip rendering with type icon, filename, and human-readable size. Node shape: - uuid: string — the attachments-row UUID - filename: string — display name; preserved across save/reload Markdown round-trip: - Serialize: `[filename](pad-attachment:UUID)` — same standard link syntax the markdown resolver in TASK-874 understands. `]` and `\` in the filename are escaped to keep the link label balanced. - Parse: markdown-it's link token produces `<a href="pad-attachment:UUID">filename</a>`. Our parseHTML rule `a[href^="pad-attachment:"]` runs at priority 1000 to beat SafeLink's default mark rule (priority 50), so attachment refs become a chip Node instead of a Link Mark on plain text. Editor display (NodeView): - <a class="file-chip"> with icon + name + optional size span - Icon: filename-extension heuristic on first paint, upgraded to a MIME-based icon once a single HEAD request resolves the canonical Content-Type. The HEAD goes against the existing GET handler — no new API endpoint required, and Go's net/http strips the body automatically for HEAD. - Size: rendered from Content-Length once HEAD resolves; hidden until then (CSS `:empty { display: none }`). - Module-level Promise cache keyed by `${ws}:${uuid}` deduplicates repeated chips for the same attachment and survives undo/redo without re-fetching. - target=_blank + download attribute so a click opens / saves the file with its canonical filename. - atom: true → Backspace/Delete remove the chip as a single unit. Styles in app.css (not Editor.svelte) so they reach the read-only markdown render path too — TASK-874's Go and TS resolvers emit the same `.file-chip` / `.file-chip-icon` / `.file-chip-name` / `.file-chip-size` markup. Parent: PLAN-866. Unblocks TASK-875 — the upload plugin needs a chip node to insert on successful non-image uploads. * fix(attachments): chip HEAD route + chip click handler per Codex review (round 1) Two findings from the round-1 Codex review: 1. chi router does not auto-route HEAD to GET handlers, so the chip's metadata HEAD probe was returning 405 and chip size + MIME-refined icons never loaded. Fix: register HEAD on the same path/handler; http.ServeContent already strips the body for HEAD on the seekable path, and the streaming fallback short-circuits before io.Copy so future S3-style backends don't burn GetObject bandwidth on HEAD. Tests added: HEAD returns 200 with Content-Type + Content-Length and an empty body; HEAD cross-workspace returns 404 (not 403) so the new endpoint can't become a side-channel for ID enumeration. 2. Editor.svelte installs a global anchor-click suppressor that preventDefaults every <a> inside the editor, so the chip looked clickable but did nothing in edit mode. Fix: the chip's NodeView now attaches an explicit click handler that calls window.open with the download URL and stops propagation before the global handler runs. Mirrors the AttachmentImage lightbox click pattern.	2026-04-29 13:35:15 -04:00
xarmian	5af54ddc05	feat(attachments): markdown reference resolver for pad-attachment:UUID (TASK-874) (#291) ... * feat(attachments): markdown reference resolver for pad-attachment:UUID (TASK-874) Add the shared step that translates `pad-attachment:UUID` markdown references into rendered HTML for image embeds, file chips, and missing placeholders. Wired into the editor preview path; Go-side helpers seed the future server-side rendering pipeline (export / shared item view). TS side (`web/src/lib/markdown/attachments.ts`): - Pure helpers: parseAttachmentHref, attachmentDownloadUrl, isImageMime, formatAttachmentSize, renderAttachmentImage/Chip/Missing - resolveAttachmentImage / resolveAttachmentLink for the marked hooks - Image MIME → <img src=...?variant=thumb-md data-attachment-id=...> - Non-image MIME (or link syntax) → file chip with download attribute - Missing/deleted → "Missing attachment" placeholder span `web/src/lib/utils/markdown.ts`: - renderer.image override (defaulting to marked's standard image when href is not pad-attachment:) - renderer.link checks for pad-attachment: prefix before the existing external/internal-link logic - renderMarkdown gains an optional attachmentResolver parameter; the resolver is threaded via a per-call module slot (synchronous render) - DOMPurify allowlist extended with data-attachment-id, download, width, height — ALLOW_DATA_ATTR stays false so only this single data-* attribute slips through Go side (`internal/server/render/attachments.go`): - Mirror of the TS API so server-rendered output matches client output byte-for-byte for the same input - ResolveAttachmentReferences scans markdown source via regex, skipping fenced code blocks (backtick + tilde), substitutes both image and link forms - Comprehensive table-driven tests (24 cases) covering: href parsing, URL building, MIME detection, size formatting, image/chip/missing rendering, escape safety against script-tag injection in alt / filename / display text, fenced-code skip, tilde fences, title suffix on link destinations, nil resolver pass-through, no false positives on non-attachment URLs, deterministic round-trip References are stored as opaque `pad-attachment:UUID` so a backend migration (FS → S3) can rewrite storage_keys without touching item content. See DOC-865 for the architecture. Parent: PLAN-866 (Attachments Phase 1). * fix(attachments): chip label double-escape + escaped-bracket lockstep per Codex review (round 1) Two findings from the round-1 Codex review: 1. TS chip labels were double-escaped. renderer.link was passing the parseInline(tokens) HTML output to resolveAttachmentLink, which feeds it into renderAttachmentChip → escapeHtml. A label like `[**Report**](pad-attachment:id)` rendered literal `&lt;strong&gt;Report&lt;/strong&gt;` instead of plain text. Switched to the link token's raw `text` field; markdown emphasis inside chip labels now degrades to literal markers (acceptable for filename-style labels) and matches what the Go regex extracts. 2. Go regex didn't accept CommonMark `\]` / `\\` escapes inside link/image labels, so `[Q1 \] report](pad-attachment:id)` resolved on the TS side (marked handles escapes) but stayed literal on the Go side — breaking the documented lock-step contract. Updated the regex to accept escaped characters inside the alt/text capture, and added unescapeMarkdownText to mirror marked's behavior of dropping the backslash before the label reaches the render helpers. Tests added: TestResolveAttachmentReferences_EscapedBrackets covers image alt, link text, and combined backslash/bracket escapes; TestUnescapeMarkdownText is the unit-level table for the unescape helper (including dangling-backslash and non-punctuation pass-through).	2026-04-29 13:09:42 -04:00
xarmian	00baf75576	feat(attachments): download/serve API with auth + Range support (TASK-872) (#289) ... Adds the GET endpoint that pairs with TASK-871's upload. Streams the blob from the resolved storage backend with proper headers, Range support, and cross-workspace defense. GET /api/v1/workspaces/{slug}/attachments/{attachmentID} Optional ?variant=thumb-sm|thumb-md - 200 inline render for images / video / audio / PDF / etc. - 200 attachment download for HTML / JS / forced-download MIMEs - 206 Partial Content on Range requests (video/audio seek) - 304 Not Modified on conditional GETs (If-Modified-Since etc.) - 400 unknown variant - 404 missing attachment OR cross-workspace probe (not 403, to avoid leaking existence of attachments in other workspaces) - 404 blob_missing if DB row exists but on-disk blob is gone (logs a warning since this is a "shouldn't happen" state) - 503 if attachments registry not configured internal/server/handlers_attachments.go handleGetAttachment looks up the row, gates cross-workspace via 404, optionally swaps to a derived variant via GetAttachmentVariant (silent fallback to original when the variant row doesn't exist yet — TASK-878 will populate them; this handler shipping today doesn't have to wait), resolves the storage backend via Registry, and hands off to http.ServeContent when the body satisfies io.ReadSeeker. FSStore returns *os.File so that's the common path and gets us Range / 206 / conditional GETs for free. Backends without Seek (a future S3 streaming reader) fall through to a plain io.Copy with no Range support — the contract is "Range works when the backend supports it, never breaks correctness". Headers: Content-Type from att.MimeType (already canonical post-allowlist) Content-Disposition: inline | attachment, filename sanitized to strip quotes/backslashes/control bytes (header-injection defense on top of the upload-time basenaming) Cache-Control: private, max-age=3600 (Phase 3 revisits for CDN) X-Content-Type-Options: nosniff (browser should never re-sniff; we already validated MIME at upload) Upload response now includes "url" again — TASK-871 had dropped it because the GET handler didn't exist yet. Slug-form path matches every other API endpoint. internal/store/attachments.go GetAttachmentVariant(parentID, variant) for the ?variant lookup. internal/server/server.go GET /workspaces/{slug}/attachments/{attachmentID} wired alongside the existing POST. Tests Happy-path PNG, HTML force-download, 404 missing, cross-workspace 404 (NOT 403), Range 206 with bytes 10-29 of an MP4 payload, variant fallback to original, unknown variant rejected, derived thumb-sm row honored when present, blob-missing 404, and the filename sanitizer table. Verification go build ./... — clean go vet ./... — clean go test ./... — all packages pass make install — server restarts on the new binary Parent: PLAN-866.	2026-04-29 12:34:36 -04:00
xarmian	48b9e18d34	feat(attachments): upload API with MIME sniff + dedupe + quota tracking (TASK-871) (#288) ... * feat(attachments): upload API with MIME sniff + dedupe + quota tracking (TASK-871) Wires the upload endpoint that turns a multipart POST into an attachments row plus a stored blob. Auth-gated (editor+), per-file size cap, hash-streaming, MIME allowlist with extension blocklist, fire-and-forget quota warning. POST /api/v1/workspaces/{slug}/attachments Multipart "file" field. Optional ?item_id=… or form item_id to associate at upload time. Returns {id, url, mime, size, width?, height?, filename, category, render_mode}. Errors: 400 bad multipart, 400 empty file, 401 unauthorized, 403 insufficient role, 413 over per-file cap, 415 MIME or extension rejection, 503 attachments not configured. internal/attachments/mime.go MIMEEntry + RenderMode + Category typed allowlist mirroring DOC-865. Default-deny. SniffMIME wraps http.DetectContentType. ValidateUpload cross-checks the sniff result against the filename extension and: (a) rejects when the extension maps to a *blocked* MIME — covers .svg (sniffs as text/xml; .svg ext makes the browser run embedded <script>) and .exe family (sniffs vary; extension is unambiguous); (b) rejects when the extension maps to an allowed MIME but the sniff's category disagrees — the "exe pretending to be png" case. Tests cover normalize/lookup/sniff plus happy path, exe-as-png, extension mismatch, SVG, .exe-by-extension-alone, text/plain accept, HTML force-download. internal/store/attachments.go CreateAttachment / GetAttachment / WorkspaceStorageUsage. Pointer scan for nullables; SUM(size_bytes) excludes soft-deleted rows but includes derived blobs (thumbnails are real bytes on disk). internal/server/handlers_attachments.go Body capped via http.MaxBytesReader BEFORE ParseMultipartForm spools any of it. Streams "file" part into an os.CreateTemp file, sha256ing in one io.MultiWriter pass — multi-GB POST never reaches RAM. Sniff on first 512 bytes; image dimension probe via stdlib image.DecodeConfig (PNG/JPEG/GIF). WebP/AVIF/HEIC accepted but width/height nil — matches the "pure-Go gracefully degrades" decision in DOC-865. Calls AttachmentStore.Put (which hash-verifies via the dedup fast path) and inserts the row. Quota check (CheckLimit + WorkspaceStorageUsage) runs in a goroutine — Phase 1 logs only; Phase 2 will enforce. Anonymous uploads on a fresh install (RequireWorkspaceAccess grants implicit owner without a current user) get uploaded_by="system". internal/server/server.go Server.attachments + attachmentMaxBytes fields and SetAttachments setter. Route POST /workspaces/{slug}/attachments wired inside the authenticated workspace block. cmd/pad/main.go Boot wiring: NewFSStore(<DataDir>/attachments) → Registry registered under "fs" → SetAttachments. PAD_ATTACHMENT_MAX_BYTES env override for the per-file cap. Tests internal/server/handlers_attachments_test.go covers: happy path PNG (1x1, dimensions resolve to 1×1) exe bytes with .png filename → 415 PNG bytes with .pdf filename → 415 (extension mismatch) empty body → 400 missing file part → 400 over the size cap → 413 same content uploaded twice → two rows, same content_hash + storage_key, WorkspaceStorageUsage = 2 × bytes (dedupe is at the blob layer, not the row layer) 8 concurrent uploads of identical bytes → all 201, no corruption no registry wired → 503 Verification go build ./... — clean go vet ./... — clean go test ./... — all packages pass make install — server restarts on the new binary Parent: PLAN-866. * fix(attachments): three Codex round-1 findings — drop premature url, accept Office docs, real quota probe 1. Upload response no longer returns "url". TASK-872 wires GET so any URL we return today is a 404 — pulling it out keeps clients from baking in the broken endpoint. 2. Office Open XML docs (.docx/.xlsx/.pptx) and OpenDocument formats (.odt/.ods/.odp) are zipped XML — http.DetectContentType correctly sniffs them as application/zip. Previously the validator's extension-vs-sniff category check rejected them as "mime_extension_mismatch" (archive vs document). Now: when the sniffed type is exactly application/zip and the extension maps to a document MIME, trust the extension and route to the document entry. Plain .zip with the same bytes still routes to archive. Test covers all six office/odf extensions plus the plain-zip case. 3. CheckLimit("storage_bytes") returned "unknown workspace feature" because featureCount only knows row-counted features (items, members, webhooks). The warning path silently dropped every probe. Added Store.WorkspaceStorageLimit which does the same three-tier resolution (user override → platform setting → hardcoded fallback) but returns the limit only — usage is computed separately via the existing WorkspaceStorageUsage. Self-hosted/pro plans return -1 (unlimited). Workspaces without an owner_id (fresh installs and legacy rows) also return -1, so a fresh-install upload no longer logs "owner not found". Switched maybeWarnStorageQuota to use WorkspaceStorageLimit + WorkspaceStorageUsage directly. Now also spawned via Server.goAsync so Stop() drains it (BUG-842 hygiene). Tests - TestValidateUpload_AcceptsOfficeOpenXMLAsZipBytes covers all six extensions + plain .zip - TestUpload_QuotaCheckResolves regression-tests finding 3: both storage helpers return non-error after a real upload - TestUpload_HappyPathPNG asserts the response no longer carries url Verification go build ./... — clean go vet ./... — clean go test ./... — all packages pass * fix(attachments): trim trailing blank line at EOF in mime.go per Codex review (round 2) Round 2 LOW: git diff --check flagged a "new blank line at EOF" on internal/attachments/mime.go. Cosmetic but addressed because the ship-tasks workflow requires zero findings (HIGH/MEDIUM/LOW alike) — leaving LOWs unfixed compounds across PRs and prevents the loop from ever converging clean on later work. * fix(attachments): alias stdlib MIME-sniff quirks per Codex review (round 3) http.DetectContentType returns names that don't match modern IANA conventions for two formats on the allowlist: audio/wave → audio/wav (.wav uploads) application/x-gzip → application/gzip (.gz uploads) Without aliasing, valid uploads of either format hit "mime_not_allowed" because the allowlist uses canonical names. Added a sniffAliases map applied inside SniffMIME so allowlist lookups always see the canonical form. Allowlist stays single-sourced; the fix is one map entry per quirk we discover. Tests: - TestSniffMIME_AliasesStdlibQuirks pins both aliases at the sniff layer - TestValidateUpload_AcceptsWAV / TestValidateUpload_AcceptsGzip verify the end-to-end accept path with real WAV (RIFF/WAVE) and gzip headers	2026-04-29 12:19:06 -04:00
xarmian	e5eae5e94e	feat(web): persistent dismissible CLI-connect banner on workspace pages (TASK-862) (#284) ... * feat(web): persistent dismissible CLI-connect banner on workspace pages (TASK-862) Final web piece of the web-first onboarding on-ramp from PLAN-859 / IDEA-750. A slim banner now nudges users to connect their workspace to the CLI on every workspace page, until they either dismiss it or actually do it. Server: - New store method WorkspaceHasCLISource(workspaceID) — backed by EXISTS(... WHERE source='cli' AND deleted_at IS NULL), so it's a cheap O(1) check that short-circuits on the first match. - Dashboard payload (GET /workspaces/{ws}/dashboard) gains HasCLISource bool (json: has_cli_source). - Unit tests cover empty workspace, web/skill items don't trip it, one cli item flips it on, soft-delete flips it back off, and cross-workspace isolation. Web: - New <ConnectBanner> Svelte 5 component (web/src/lib/components/ConnectBanner.svelte). Self-contained: reads dismissed state from localStorage, fetches has_cli_source itself, mounts <ConnectWorkspaceModal> internally. Two split $effect blocks per CONVE-606 — one for the localStorage sync, one for the dashboard fetch — so a workspace change doesn't entangle the two reactive lifecycles. - Banner is hidden while loading (hasCliSource === null) to avoid a flash-then-auto-hide on workspaces that already have CLI items. - Storage key pad-cli-banner-dismissed-${wsSlug} matches the existing onboarding-dismissed pattern. Per-browser only; TODO comment in source about backing it with a workspace_user_state row if cross- device persistence is wanted later. - Mounted in web/src/routes/[username]/[workspace]/+layout.svelte above {@render children()} so it appears on every workspace page (dashboard, collection lists, item detail, search, activity, etc.) and NOT on console/auth pages (the layout is workspace-scoped). - DashboardResponse type in web/src/lib/types/index.ts gains has_cli_source: boolean. Smoke-tested against the running server: the field is live in the dashboard payload and reflects reality (this workspace returns has_cli_source: true since it has many CLI-sourced items, so the banner is correctly auto-hidden here). Test plan: - go build ./... && go test ./... — all green (incl. new TestWorkspaceHasCLISource with 5 sub-cases). - cd web && npm run build — clean. - make install — clean, server restarted. - Svelte MCP autofixer ran on ConnectBanner.svelte — no issues. Parent: PLAN-859. Driving idea: IDEA-750. * fix(web/connect-banner): stale-response guard + refetch on modal close (Codex round 1) Two findings from Codex review on PR #284: 1. Stale-response race: rapid workspace switches could let a slow dashboard fetch from workspace A overwrite hasCliSource for workspace B after the user navigated. Capture the requested slug at fetch time, ignore the response if wsSlug has changed since. 2. Auto-hide didn't work in-session: if a user opened the banner modal, copied the command, ran it elsewhere, and closed the modal, the banner stayed visible because hasCliSource was stale. Refetch when the modal transitions from open → closed (the natural moment the user has just connected). Uses $effect.pre with a tracked previous value, matching the transition pattern in ShareDialog. The 'someone ran the CLI from another terminal without ever opening the modal' edge case is left for a follow-up — would require SSE item-created subscription, which is heavier than this PR's scope. * fix(server/items): persist source from auth context on create (Codex round 2) Codex caught an architectural bug while reviewing the TASK-862 banner work: items created via the CLI were persisting with source='web' (the column default) instead of 'cli', because handleCreateItem decoded ItemCreate from the body — which the CLI doesn't set Source on — and only consulted actorFromRequest AFTER persisting (for SSE / activity log emission). Result: TASK-862's has_cli_source dashboard signal would never flip on for normal CLI usage, so the connect-CLI banner would never auto-hide for users who actually wired up the CLI. Fix: in handleCreateItem, backfill input.Source from actorFromRequest before calling store.CreateItem, but only when the client didn't explicitly set it (so agents marking themselves as 'skill' still pass through unchanged). Test: TestCreateItemSourcePersistedFromAuth covers all three branches - bearer Authorization header → source=cli (uses bootstrap + a real session token in the header since the auth middleware validates token format and rejects fake values with 401 before the handler runs) - cookie session, no Authorization → source=web - explicit source in body wins over auth-derived (e.g. 'skill') * fix(web/connect-banner): seq counter for same-workspace race (Codex round 3) Round 3 caught a same-workspace race the slug guard didn't cover: an in-flight workspace-change fetch that resolves AFTER the modal-close refetch could overwrite the newer 'true' with the older 'false', making the banner reappear after the user actually wired up the CLI. Add a monotonic fetchSeq counter — captured at call time, rechecked before applying the response. Only the LATEST request's result wins, regardless of arrival order. The slug guard stays as a second-layer defense for cross-workspace races. * fix(web/connect-banner): guard banner keydown to currentTarget (Codex round 4) Round 4 caught a keyboard-event bubble: pressing Enter or Space on the dismiss X button also fired the banner-level keydown handler, so the user would dismiss AND open the modal in one stroke. Guard the parent handler with `e.target !== e.currentTarget` so it only reacts to keydown that originated on the banner itself. Tabbing to the dismiss button + Enter now ONLY dismisses. * fix(store): visibility-filter has_cli_source query (Codex round 5) Round 5 caught a P2 information leak: WorkspaceHasCLISource scanned the entire workspace regardless of caller visibility, so a guest with grants only on web-sourced items could still see has_cli_source return true (revealing that CLI items exist somewhere they can't see). That also produced wrong UX — the banner could auto-hide for guests who couldn't actually use the CLI. Extend the query to take optional collectionIDs/itemIDs filters matching the dashboard's existing visibility model: an item counts when its collection is in collectionIDs OR its id is in itemIDs (union — guest item-level grants can expose items in otherwise- hidden collections). Mirrors ListItems' filtering pattern incl. the "non-nil empty CollectionIDs = no visibility = short-circuit false" semantics. Handler now passes dashCollIDs and dashItemIDs to match the rest of the dashboard payload's filtering. New TestWorkspaceHasCLISourceVisibility covers the four cases: unfiltered sees all, visible-coll-only hides CLI items in hidden collections, item-level grant surfaces a hidden CLI item, and empty visibility short-circuits to false.	2026-04-29 10:05:34 -04:00
xarmian	715ec70e94	fix(server): drain ipRateLimiter cleanup goroutines on Stop() (BUG-851) (#276) ... NewRateLimiters spawned 9 ipRateLimiter cleanup goroutines per Server, each in an unbounded `for { time.Sleep(5*time.Minute); ... }` loop with no exit signal (middleware_ratelimit.go:78-89). Every testServer(t) call leaked all 9, accumulating across the 210-test internal/server suite. Under -race the goroutine count + sync overhead pushed the run past the default 10m timeout, which is why the `Run tests with race detector` step (gated to main pushes) has been failing on every main run since the step was added on 2026-04-13. This is the same flavor as BUG-842 part 2 (request-handler fire-and-forget goroutines drained via Server.bg WaitGroup). The rate-limiter case wasn't in BUG-842's scope: those goroutines are spawned at construction time, not at request time, so they need a different drain primitive. Changes: - ipRateLimiter gains stopCh + stopOnce + stopWg. cleanup() rewrites its loop as a select over stopCh and a 5-minute ticker, deferring stopWg.Done(). New Stop() closes stopCh once and waits for the cleanup goroutine to return. - RateLimiters gains a Stop() that walks all 9 limiters (nil-safe via the (*ipRateLimiter).Stop receiver guard). - Server.Stop() now also calls s.rateLimiters.Stop() after s.bg.Wait(). Test cleanups already call Server.Stop() (added in BUG-842), so no test-helper changes needed. - New TestServer_Stop_DrainsRateLimiterCleanup pins the contract: construct + Stop N servers, assert runtime.NumGoroutine() returns to baseline ±3. - .github/workflows/ci.yml: bump the -race timeout from the default 10m to 20m. The full server suite under -race takes ~13m on a dev laptop after the leak fix; 20m gives margin without papering over an actual hang. Both `Run tests with race detector` (SQLite) and `Run tests with race detector against PostgreSQL` are bumped. Verified locally: go test -race -timeout=1500s ./internal/server/ finishes ok in 776s (12m57s). Without the leak fix, the same command times out at 600s (10m) with a goroutine dump showing hundreds of ipRateLimiter.cleanup frames.	2026-04-28 17:20:03 -04:00
xarmian	0fd5d0cdfb	fix: green up Go (PostgreSQL) CI (BUG-842) (#275) ... * fix(store): swap plainto_tsquery → websearch_to_tsquery for PG FTS (BUG-842) `TestListItems_FTS_HyphenatedSearchTerm/task-five` has been failing on every Go (PostgreSQL) CI run because `plainto_tsquery('english', 'task-five')` doesn't match the asciihword lexeme(s) the english parser produces for an indexed `task-five-distinctive`. The result is that every PG full-text search for hyphenated terms returns zero rows. `websearch_to_tsquery` (Postgres 11+) is purpose-built for arbitrary user input and tokenizes hyphenated terms the same way `to_tsvector` does for the indexed document, so the query intersects the index correctly. Swapped in three spots in the postgres dialect — FTSMatch, FTSSnippet, FTSRank — and updated the caller-side comments that referenced plainto_tsquery. SQLite path is unchanged: it goes through items_fts MATCH with sanitizeFTSQuery, never through these methods. * fix(server): drain background goroutines on Stop() (BUG-842) `TestAdminBillingStats_SidecarSidecarError_DegradesToLocalOnly` (and other server tests) have been flaking on the Go (PostgreSQL) CI runner with `TempDir RemoveAll cleanup: directory not empty`. Root cause: several request handlers spawned bare `go func() { ... }()` goroutines that touched the SQLite WAL DB after the test function returned. testServer's t.Cleanup closed the store but had no way to drain those goroutines first, so a fire-and-forget WAL write could re-create the `-wal`/`-shm` files between Close() and t.TempDir's RemoveAll. Add a Server.bg sync.WaitGroup, a Server.goAsync helper that wraps a WaitGroup-tracked goroutine, and a Server.Stop() that blocks until every goAsync closure has finished. Convert the four known fire-and-forget sites to goAsync: - middleware_auth.go (TouchUserActivity) - handlers_auth.go (password reset email) - handlers_cloud.go (stripe_processed_events pruning) - handlers_members.go (workspace invitation email) Wire `srv.Stop()` into both testServer (server_test.go) and newMetricsTestServer (metrics_auth_test.go) so cleanup order is Stop → Close → TempDir RemoveAll. Add TestServer_Stop_DrainsBackgroundGoroutines to pin the contract: a goAsync goroutine must block Stop until it returns. * fix(store): correct PG FTS hyphenation via OR-combined plainto_tsquery (BUG-842) The previous attempt swapped plainto_tsquery → websearch_to_tsquery, which was wrong: websearch_to_tsquery treats `-` as a NEGATION operator (Google-style), so `task-five` becomes `task & !five` and the search returns 0 rows for the same reason as before. This commit reverts the swap and applies the actual fix. PG's english parser indexes `task-five-distinctive` as `{task-five-distinct, task, five, distinct}` — the asciihword AND its parts. plainto_tsquery applied to the partial query `task-five` produces `task-fiv & task & five`: the stemmed asciihword for the PARTIAL query (`task-fiv`) is NOT in the vector, so the AND fails. Replacing the hyphen with a space makes plainto emit `task & five`, which DOES match — but doing that unconditionally breaks `BUG-842`- style queries: PG indexes the `-842` suffix as a negative-number lexeme, so `plainto_tsquery('BUG-842')` matches via `-842`, while `plainto_tsquery('BUG 842')` searches for `842` and misses. The fix ORs the two query variants together so the search vector is matched against either the raw user query OR its hyphen-as-space form. Both `task-five` (against `task-five-distinctive`) and `BUG-842` (against `BUG-842 fix the cleanup race`) hit. Verified locally against postgres:17-alpine via PAD_TEST_POSTGRES_URL — both 10x stress and race-detector runs are green. Surfaces: - dialect.go: FTSMatch / FTSSnippet / FTSRank now consume TWO placeholders each in the PG dialect. - items.go: listItemsFTS PG branch + SearchItems PG branch update args to pass (raw, sanitized) for every PG `?` placeholder. - search.go: SearchItems main / count / facets PG branches updated likewise. New sanitizePGFTSQuery helper alongside sanitizeFTSQuery. - documents.go: ListDocuments PG branch updated. Tests: - TestListItems_FTS_HyphenatedSearchTerm extended with a `BUG-842` case to pin the OR-combined logic — naive hyphen-stripping would silently regress this. - New TestSanitizePGFTSQuery unit test. * chore: gofmt 11 files with import-order issues (BUG-842 PR cleanup) The Go (SQLite) CI job has been failing on `main` (and every PR built against it) because golangci-lint flags 11 files whose third-party imports are intermixed with internal imports — the import-grouping rule that gofmt enforces. None of these were introduced by the BUG-842 PR; they're pre-existing on main. The PR can't go green without this cleanup, though, so it's bundled here. Pure mechanical change — `gofmt -w <files>` only re-orders import groups; no logic changes. Files touched: cmd/pad/configure.go cmd/pad/main.go internal/cli/format.go internal/server/handlers_admin_invitations.go internal/server/handlers_admin_users.go internal/server/handlers_grants.go internal/server/handlers_share_links.go internal/server/handlers_stars.go internal/server/middleware_auth.go internal/store/store.go internal/store/store_test.go After this commit `gofmt -l ./cmd ./internal` returns clean.	2026-04-28 16:21:43 -04:00
xarmian	7cda0d7896	feat: rebrand to Perpetual Software + new tagline (IDEA-832) (#273) ... Migrates from xarmian/pad to PerpetualSoftware/pad across the entire repo and updates the product subtitle to "Collaborate with your AI agents". Go module rename - go.mod: github.com/xarmian/pad → github.com/PerpetualSoftware/pad - All Go imports updated across cmd/pad, internal/{cli,server,store, models,collections,items,events,metrics,webhooks} (~130 files) - Test fixtures with the literal repo slug ("xarmian/pad" in JSON shapes, SSH/HTTPS git URL strings, workspace_context fixtures) also updated, including the secondary repo entry (xarmian/pad-web → PerpetualSoftware/pad-web — pad-web was also moved to the org per branch context) Docs / config - README badges, install instructions, brew tap, Docker image, source build path, sponsor link (sponsor link kept as personal @xarmian) - Subtitle: "Project management for developers and AI agents." → "Collaborate with your AI agents." (README, manifests, web layout meta, .goreleaser homebrew description) - CONTRIBUTING.md, SECURITY.md, skills/INSTALL.md - .goreleaser.yaml: homebrew_casks owner, GHCR image, release github owner, cosign cert-identity regex, comments - .github/workflows/release.yml: tap/release comments - deploy/k8s/deployment.yaml: container image - docs/deployment.md: clone URL - web/static/{site.webmanifest,manifest.json}: description - web/src/routes/+layout.svelte: meta description + og:description Brew tap path is PerpetualSoftware/tap/pad (CamelCase, matches GitHub user case). GHCR image is ghcr.io/perpetualsoftware/pad (lowercased per GHCR's URL normalization). CODEOWNERS @xarmian and FUNDING.yml github: xarmian intentionally retained — those are the personal maintainer / sponsor account, separate from the org repo. Verification: go build ./..., go test ./... (all pkgs pass), web build, and make install all clean (TASK-844, TASK-845).	2026-04-28 12:26:39 -04:00
xarmian	8e067c19db	feat(admin): add /api/v1/admin/billing-stats proxy + cloud client (TASK-827) (#266) ... * feat(admin): add /api/v1/admin/billing-stats proxy + cloud client (TASK-827) New admin endpoint that powers the upcoming Pad Cloud Billing dashboard: GET /api/v1/admin/billing-stats merges Stripe-derived metrics from pad-cloud (active subs, MRR, ARR, churn, 30-day cancellations) with locally-computed aggregates from the users table (customers_by_plan, new_signups_30d in the last 30 days for plan='pro'). Architecture (PLAN-825 Option B): - pad-cloud (TASK-826, already merged) hosts the Stripe API access in one place; this PR adds the reverse pad → pad-cloud client method. - Existing internal/billing.CloudClient gains GetBillingMetrics(): GET on /admin/metrics/billing with the X-Cloud-Secret header (the same secret pad-cloud already validates inbound calls with). - New CloudSidecar.GetBillingMetrics() interface method keeps the server package free of HTTP/Stripe dependencies and lets tests inject fakes. - Existing fakeSidecar in handlers_account_test.go grows a no-op stub so the account-delete tests still satisfy the extended interface. Degradation contract: - The endpoint always returns 200. Two booleans tell the UI which fallback to render: cloud_unreachable=true (sidecar errored or unwired) and stripe_configured=false (sidecar reachable but no STRIPE_SECRET_KEY yet). - requireCloudMode + requireAdmin gate the route. Self-host gets 404, non-admin gets 403. Web glue: - Added AdminBillingStats type to web/src/lib/types/index.ts. - Added api.admin.getBillingStats() to web/src/lib/api/client.ts. The Billing tab and metric cards land in TASK-828. Tests: - Billing package: GetBillingMetrics happy path (verifies method, path, X-Cloud-Secret header, Accept header), Stripe-not-configured pass-through, non-200 → SidecarError, transport error stays bare, malformed JSON, nil/unconfigured client guards. - Server package: self-host 404, non-admin 403, admin happy path (merges local + remote correctly, handles plan="" → "free", filters new_signups_30d to plan='pro' AND created_at >30d ago), no-sidecar degrades to local-only, transport error degrades, sidecar 5xx degrades, stripe_configured=false propagates verbatim with cloud_unreachable=false. Part of PLAN-825 (Pad Cloud Admin Billing Dashboard). * fix(admin): address Codex review (round 1) on billing-stats proxy - Replace handler-side ListUsers walk with store.CountBillingAggregates (two scalar SQL queries: COUNT(*) GROUP BY plan + a single COUNT(*) for new pro signups). Removes the per-row TOTP decrypt overhead that ListUsers performs and bounds CPU/bandwidth as the user table grows. - Fix misleading TS comment on AdminBillingStats: clarify that "fully healthy" requires cloud_unreachable=false AND stripe_configured=true, not "both flags false" as previously stated. Adds TestCountBillingAggregates exercising empty store, mixed plans, empty-plan → "free" bucketing, and the 30-day cutoff filter for new pro signups. * fix(store): GROUP BY normalised plan expression in CountBillingAggregates Codex round 2 caught a real bug: SELECT projected the COALESCE'd plan but GROUP BY operated on the raw `plan` column, so users with plan='' and plan='free' produced two distinct result rows that both scanned as "free" in Go — the second iteration overwrote the first in CustomersByPlan, silently underreporting the free-tier count. Fix: GROUP BY COALESCE(NULLIF(plan, ''), 'free') so the grouping matches the projection. Test updated: insertWithPlanAndDate now seeds an explicit '' plan alongside two explicit 'free' rows and asserts the aggregate rolls them up to 3 — the previous test only used CreateUser which always inserts the column default ('free') and never exercised the empty-string path.	2026-04-27 14:42:53 -04:00
xarmian	e5e2bd7b86	chore: flip CI only-new-issues=false + scope lint policy (TASK-771) (#253) ... * chore: gate CI on full lint, scoped to checks we enforce (TASK-771) Flip golangci-lint-action's only-new-issues from true to false so CI fails on ANY linter finding, not just findings on PR-changed lines. This catches lint regressions on the next push instead of letting them drift into main. The gate flip is paired with a deliberate scope-down of .golangci.yml: 1. errcheck is disabled. The codebase has 325 pre-existing unchecked- error sites where the error is intentionally discarded (best-effort logging writes, defensive parses with zero-valued fallbacks, etc.). Auditing every site is its own project — bigger than IDEA-732 by an order of magnitude. Tracked as a follow-up if/when we want the safety net back. 2. staticcheck is restricted to the SA* check family (real-bug detectors). The ST*/QF*/S* families are stylistic/quick-fix suggestions we don't gate CI on yet — they would have re-flooded the lint output with capitalized error strings, De Morgan's law simplification suggestions, etc., that aren't bug-finding signals. Re-enable selectively if the team wants them. After scoping, the live linters are: govet, ineffassign, staticcheck (SA*), unused, gofmt — exactly the set that IDEA-732 cleaned up. Other changes in this PR: - Drop pull-requests:read permission. It was only required by the golangci-lint-action when only-new-issues=true (the action used it to fetch PR diff metadata). Not needed any more. - Update the Run-golangci-lint comment block to explain the new policy and reference the IDEA-732 cleanup PRs (#247/#249/#251/#252). - Replace the SA4017 //lint:ignore directive in cmd/pad/main.go:4631 with an inline //nolint:staticcheck — the multi-line //lint:ignore block was too far from the if statement for staticcheck's proximity rule, so the directive wasn't taking effect. - Apply gofmt -w on three files where post-deletion blank-line artifacts had drifted (cmd/pad/main.go imports, two trailing newline fix-ups in handlers_items.go and middleware_ratelimit.go). Verified: - `golangci-lint run ./...` reports 0 issues. - `go build ./...` clean. - `go vet ./...` clean. - `go test ./...` all pass. Parent: PLAN-644. * chore: address Codex round 1 on PR #253 (TASK-771) Two LOWs from Codex on the gate-flip PR: 1. //nolint:staticcheck was broader than necessary (suppressed any future staticcheck diagnostic on the line) and didn't self-report when the underlying false positive gets fixed upstream. Codex suggested swapping back to a tightly-placed //lint:ignore SA4017. I tried that, but golangci-lint v2's staticcheck integration does not honour //lint:ignore the way direct staticcheck does — the directive was silently no-op'd via golangci-lint while the same directive worked when staticcheck was invoked directly. So instead of fighting the linter wrapper, sidestep the false positive entirely: rewrite the keepalive check from `strings.HasPrefix(line, ":")` to `len(line) > 0 && line[0] == ':'`. Same observable behaviour for a single-byte ASCII prefix, no suppression directive needed at all, no exposure when staticcheck eventually fixes the false positive. 2. The new lint-step comment in ci.yml said main is "clean of staticcheck SA*/U1000" — but U1000 is reported by the standalone `unused` linter in .golangci.yml, not by staticcheck.checks. Tighten the comment to attribute each enforced check correctly. Verified: - `golangci-lint run ./...` reports 0 issues - `go test ./cmd/pad/...` passes (the SSE watch loop is exercised by reconcile_test.go and the broader integration tests).	2026-04-25 12:36:05 -04:00