mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 19:06:33 +00:00
v0.4.0
97 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
a5b93c17c9 |
feat(api): add /items-changes?since=<seq> delta endpoint (TASK-1354) (#494)
* feat(api): add /items-changes?since=<seq> delta endpoint (TASK-1354) Adds the delta-fetch sibling of /items-index for the local-first read model (PLAN-1343 / DOC-1342 design decision #1). Clients track the workspace-scoped monotonic seq cursor returned by /items-index (TASK-1353) and poll /items-changes?since=<cursor> to apply just the rows that have mutated — without re-downloading the entire workspace. ## Endpoint GET /api/v1/workspaces/{ws}/items-changes?since=<seq>&limit=<n> - `since`: exclusive seq lower bound (returns `seq > since`). Defaults to 0 → full delta == /items-index modulo ordering. Bad input → 400. - `limit`: cap on rows. Defaults to 5000, clamped to 50000. Bad input → 400. ## Response { "changes": [...skinny rows with `deleted: bool`...], "cursor": "<decimal MAX(seq) or unchanged since when empty>" } Soft-deleted rows propagate (no `deleted_at IS NULL` filter on the backing scan) so a delta consumer can remove them from its local index without a second roundtrip. Parent metadata enrichment matches /items-index: the underlying GetItem filters soft-deleted parents so we never leak parent title/ref for an archived parent. Cursor contract: - Sorted ASC by seq → re-passing the response's cursor as `since` on the next poll is no-overlap, no-gap (strictly monotonic seq invariant from TASK-1352). - Empty response preserves the caller's `since` so position isn't lost. - Truncated-by-limit responses set cursor to the last row's seq. ## Tests - FullDeltaFromZero — three creates, since=0, ascending seq, every row deleted=false, cursor=MAX(seq). - IncrementalUpdateAndDelete — typical resume flow: snapshot cursor, mutate, delta returns exactly the mutated + tombstoned rows with the right `deleted` flag. - CursorRoundtripsCleanly — empty-poll after consuming, cursor preserved. - LimitTruncatesAndCursorResumes — paging contract holds end to end with no overlap. - InvalidParams — bad since / limit values rejected with 400. - EmptyWorkspace — cursor round-trips caller's since unchanged. ## Web TypeScript: `ItemChangeRow = ItemIndexRow & { deleted: boolean }`, `ItemChangesResponse = { changes, cursor }`. API client gains `api.items.changes(ws, sinceCursor, opts?)` with the same defensive content-strip as listIndex so a stray `content: ""` key from a Go zero-value can never clobber the canonical store. Parent: PLAN-1343. Depends on TASK-1352 (seq column) and TASK-1353 (seq cursor on /items-index). Unblocks the future client-side localIndex.applyDelta integration task. * fix(api): surface tombstones for item-grant users in /items-changes per Codex review (round 1) Codex round 1 caught that handleListItemsChanges was building its ItemIDs filter from guestResourceFilter, which itself uses GuestVisibleResources whose item-grant query filters out soft-deleted items. The result: a guest or restricted member with an item-level grant on a single item would see that ID disappear from the lookup as soon as the item was soft-deleted — and /items-changes would never emit a `deleted:true` tombstone, so the client would keep the stale row in its local index forever. Fix: - New Store.GuestVisibleResourcesIncludeDeleted that drops the `i.deleted_at IS NULL` / `c.deleted_at IS NULL` filters on both collection and item grants so tombstone IDs flow through. - New Server.guestResourceFilterIncludeDeletedItems delegate pointing at the new store helper. Implementation is shared with the live variant via guestResourceFilterCore so the member-collection-access + system-collection merge logic stays in one place. - handleListItemsChanges swaps to the include-deleted variant. Test: TestGuestVisibleResourcesIncludeDeleted_SurfacesTombstones covers both variants side-by-side — live drops the soft-deleted grant, include-deleted preserves it. * fix(store): assign per-row unique seqs in MigrateItemFieldValues per Codex review (round 2) Codex round 2 caught that the bulk UPDATE inside MigrateItemFieldValues gave every affected row the SAME MAX(seq)+1. A /items-changes?limit=N poll that cut through that equal-seq group would advance the cursor to the shared seq, and the next `seq > cursor` poll would silently miss the rest of the group — the cursor contract requires strict monotonicity. Switched to a per-row loop inside the migration transaction so every UPDATE re-reads MAX(seq) and each affected row ends up with a strictly unique seq. The workspace advisory lock makes the read-modify-write race-free on Postgres; SQLite's single-writer rule handles it implicitly. Trade-off: O(N) statements instead of O(1) for the bulk path. Option-rename is an admin one-off so the cost is acceptable (~1s/1000 rows on a warm SQLite connection). If future use cases demand a larger row budget, a single-statement UPDATE..FROM with ROW_NUMBER() CTE assigning per-row seqs would also work. Test: TestMigrateItemFieldValues_PerRowUniqueSeq confirms 5 rows in a single migration step all get unique seqs. |
||
|
xarmian
|
d6894def4f |
feat(web): collection page fetches via skinny /items-index endpoint (TASK-1349) (#491)
* feat(web): collection page fetches via skinny /items-index endpoint (TASK-1349)
Replaces every \`api.items.listByCollection(ws, coll)\` call in the
collection page with \`fetchSkinnyItems(ws, coll, includeArchived)\`,
which calls the local-first \`/items-index\` endpoint (TASK-1344)
through the typed client wrapper (TASK-1345). Items now ship
without the rich-text \`content\` body — the bulk of the per-row
wire size — until the user opens an item detail page, which still
goes through its existing full-item fetch.
Call sites updated:
- loadCollection — primary load + plans-names lookup
- SSE handler for item_created / item_archived / item_restored / item_updated
- Sync coordinator's full-refresh fallback
The skinny rows are widened to \`Item[]\` at the boundary by setting
\`content: ''\` on each row. This keeps the existing view component
type contract unchanged and means existing call sites that read
\`item.content\` see an empty string — already a "nothing to do"
sentinel in the markdown-checklist progress branch.
Documented regression — out of scope for this task: non-plans
collections used to display checklist progress derived from item
content's markdown checkboxes. With \`content\` no longer fetched
for the list view, that progress no longer appears. Plans
progress is unaffected (uses /plans-progress, not content
parsing). Re-introducing the feature requires either server-side
progress on the index endpoint or a separate lazy fetch — a
follow-up rather than a blocker for the bandwidth win.
In-scope behavior preserved:
- Item create/update flow: server still returns full items, dropped
into the array as-is; sync coordinator's incremental updates
similarly use the full-item type from the changes feed
- Server-side FTS search via \`searchResultIds\`: still id-keyed,
works against skinny rows
- List / Board / Table view components: already only read fields
present on the skinny row (title, fields, tags, sort_order…)
- Detail page fetch: unchanged — still goes through
\`api.items.get\` which returns the full Item with content
Parent: PLAN-1343.
* fix(api+web): add /collections/{coll}/checkbox-progress endpoint to preserve list-view checklist progress per Codex review (round 1)
Codex round 1 [P2] flagged that the original PR shipped a real
regression: non-plans collections used to compute markdown-checkbox
progress client-side from `item.content`, and the skinny
`/items-index` endpoint dropped `content` from the payload — so
list/board/table progress badges silently stopped appearing on
docs/tasks/custom collections.
This commit closes that gap with a new server endpoint that
computes the same `{item_id, total, done}` counts via
LENGTH/REPLACE arithmetic on the stored content, returning only
the small derived counts. No item bodies cross the wire.
Server (Go):
- `store.CollectionCheckboxProgress(workspaceID, collectionID)` —
SQL: `(LENGTH(content) - LENGTH(REPLACE(content, '- [ ]', '')))
/ 5 + (LENGTH(content) - LENGTH(REPLACE(content, '- [x]', '')))
/ 5` for total, the second clause alone for done. Same trick on
SQLite and PostgreSQL.
- `handleCollectionCheckboxProgress` — collection-visibility +
item-grant filter so guests / restricted members can't enumerate
items they shouldn't see. Mirrors `guestResourceFilter` exactly.
- Route: `GET /api/v1/workspaces/{ws}/collections/{coll}/checkbox-progress`.
- Test `TestCollectionCheckboxProgress` covers the math (open +
done counts), zero-result rows are filtered, unknown collection
→ 404, empty result → 200 + `[]`.
Web:
- `api.items.collectionCheckboxProgress(ws, coll)`
- Both call sites in `+page.svelte` (initial `loadCollection`
non-plans branch + `refreshProgress` non-plans branch) now
pull from the endpoint instead of parsing `item.content`.
- Drops the previous "documented regression" comment — the
feature is fully preserved.
Sub-100-byte response per item (vs. the full content body) so the
bandwidth win from `/items-index` is preserved. The endpoint scans
content server-side, but doesn't transmit it — the original
listByCollection call both scanned AND transmitted content.
Parent: PLAN-1343.
* fix(api+web): plumb include_archived through checkbox-progress per Codex review (round 2)
Codex round 2 [P2] caught that the Archived toggle path lost
checklist progress badges: `CollectionCheckboxProgress` hard-coded
`deleted_at IS NULL`, but the page-side fetch is called with the
same `showArchived` flag that toggles whether archived items
render. With the toggle on, archived non-plan items appeared in
the list but had no `itemProgress` row — the old client-side parse
would have counted them.
Fix: thread `includeArchived` through the call chain.
- store.CollectionCheckboxProgress(workspaceID, collectionID,
includeArchived bool) — appends `AND deleted_at IS NULL` only
when includeArchived is false. Default match the original
archived-off behavior.
- handleCollectionCheckboxProgress reads
?include_archived=true and forwards.
- api.items.collectionCheckboxProgress(ws, coll, { includeArchived })
on the client.
- +page.svelte's two call sites pass `showArchived` /
`includeArchived` exactly.
TestCollectionCheckboxProgress now archives one of the seeded
items and asserts:
- default response excludes the archived item (1 row)
- ?include_archived=true response includes it (2 rows)
Also clarified the const-doc on `checkboxCountSQL` to reflect the
dynamic deleted-at clause.
|
||
|
xarmian
|
e83e7ef413 |
feat(api): add /items/index skinny-projection endpoint (TASK-1344) (#486)
* feat(api): add /items/index skinny-projection endpoint (TASK-1344)
Foundation for PLAN-1343 (local-first read model). Adds a new
GET /api/v1/workspaces/{ws}/items/index endpoint that returns
every item in a workspace minus the rich-text `content` body,
so the client can hydrate an in-memory + IndexedDB index from a
single request and render every collection page from local
state without re-fetching.
Response: {items, total, cursor}. The cursor placeholder is the
max(updated_at) across the result set — Phase 2 replaces it with
a monotonic `seq` cursor. Sort is updated_at DESC, id ASC for
deterministic, cursor-friendly ordering.
Auth uses the same collection-visibility + item-grant filter as
handleListItems. Optional ?collection=<slug> filter for use by
collection pages. ?include_archived=true mirrors the existing
list behavior.
Parent: PLAN-1343.
* fix(api): move skinny-projection endpoint to /items-index per Codex review (round 1)
Codex round 1 [P2] flagged that the original `/items/index` path
shadowed the detail URL of any item whose slug is `index` — slugify
emits `index` for a title of "Index", and chi's static-over-wildcard
preference would route `GET /items/index` to the new index handler
instead of the existing `/items/{itemSlug}` detail handler.
Move the endpoint up to the workspace level as `/items-index`, sibling
to the existing `/plans-progress` route. Slugs cannot contain hyphens
adjacent to identifiers in a way that would collide with a static
workspace-level path, so this URL space is permanently safe.
New test `TestListItemsIndex_DoesNotShadowItemSlug` locks in the
contract: a real item titled "Index" still resolves through
`/items/{itemSlug}`, while `/items-index` returns the index wrapper.
|
||
|
xarmian
|
028db39217 |
feat(collab): periodic op-log GC sweeper for dormant items (TASK-1309) (#471)
The Yjs collab dumb-relay accumulates op-log rows indefinitely in item_yjs_updates. DOC-1307 surfaced 45-second p50 cold-reconnect latency on a single item with 5000 accumulated rows. Without GC, busy items keep growing. This adds a periodic background sweeper that prunes the entire op-log for items that are both DORMANT (no recent activity) AND FULLY FLUSHED (items.content has captured every op-log row). Whole-log only — Yjs op streams are causally linked, prefix-pruning corrupts replay; future cold connects lazy-seed from items.content. Components: - Store.ListDormantOpLogItemsBefore (joins items, filters watermark) - Store.PruneItemOpLogIfDormantBefore (atomic conditional DELETE) - Store.GetItemContentFlushedOpLogID (per-item watermark getter) - RoomManager.PruneSweep (per-item-locked, active-room-skip) - Server.StartOpLogGC / stopOpLogGC (mirrors orphan_gc.go pattern) - cmd/pad/main.go env vars PAD_OPLOG_GC_INTERVAL / PAD_OPLOG_GC_MIN_AGE - New (item_id, created_at) index for the dormancy query - New items.content_flushed_op_log_id column (id-based watermark, monotonic, no clock-skew or second-granularity false positives) + content_flushed_at (informational timestamp) Watermark policy: - Server-driven full-content writes (CLI / MCP / version restore / PruneAndApply) advance content_flushed_op_log_id to MAX(op-log.id) via subquery, atomic with the content UPDATE - Browser collab-snapshot 5s flushes do NOT advance the watermark — they can't prove their markdown captured every peer's ops, so letting them stamp would risk later GC-pruning unsynced peer edits - Schema-mismatch rebuild (TASK-1268) logs a WARN when it drops unflushed ops (data loss is unavoidable on schema bumps but visible) Stop ordering: collab.Close() now runs BEFORE bg.Wait() so a GC goroutine waiting on an itemLock behind an active Join can drain. Migration backfill: items WITH existing op-log rows keep NULL watermark (don't certify); items WITHOUT op-log rows get a synthetic 0 watermark (vacuous, harmless — no rows to compare against). Tests: - 6 RoomManager.PruneSweep tests (dormant prune / default minAge / empty / bails-on-Close / skips-active-room / skips-row-added-mid- sweep via fakeOpLog hook) - 5 Server.OpLogGC tests (prunes-dormant / start-idempotent / preserves-unflushed / backfill-doesnt-certify-unflushed / no-collab-noop) - TestCollabSnapshotDoesNotAdvanceOpLogWatermark in store - TestCollabSnapshotQueryOverridesBodyVersionSource in server (regression for body-attacker bypass) Seven rounds of Codex review — caught 5 P1s and 4 P2s I would have shipped under self-review: 1. Prefix-prune corrupts Yjs replay 2. Stop ordering deadlock 3. Missing index 4. Best-effort flush ⇒ data loss 5. Backfill over-certifies via metadata-PATCH 6. Second-granularity timestamp comparison 7. Schema-mismatch path drops unflushed silently 8. Browser flush stamps watermark beyond Y.Doc 9. Body version_source bypasses server policy |
||
|
xarmian
|
e7b1c3b5ae |
feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255) (#453)
* feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255)
Wires the OpBus + op-log + WS handler from prior phase-1 PRs into a
working dumb-relay collab server. Per-item Room created lazily on
first Join, kept alive across transient disconnects via a 60s grace
TTL, reclaimed when the grace expires with no fresh subscribers.
Components:
- internal/collab/room.go — Room struct + lifecycle
· roomConn pairs (id, conn, bus channel, write mutex). The id is
server-assigned per WS so writeLoop can suppress own-event echoes
without decoding the Y.Doc to read the Yjs ClientID.
· readLoop discriminates yMessageSync vs yMessageAwareness on
byte 0. Sync frames are persisted to the op-log AND broadcast;
awareness frames are broadcast only (presence is ephemeral).
Persistence happens BEFORE broadcast so a crash mid-publish loses
at most a live keystroke that the originator will replay on
reconnect anyway.
· writeLoop drains the bus subscription and writes non-self events
to the WS, gated by a per-conn write mutex (gorilla's "one writer
at a time" rule).
· removeConn arms a 60s graceTimer when the last conn drops; a
fresh addConn cancels the timer. onGraceExpired re-checks
len(conns) == 0 under the room mutex and only THEN sets
closing=true + calls back to the manager. The race between
"manager.getOrCreate found us" and "grace timer fired" is
handled by addConn returning errRoomClosing; the manager retries
via getOrCreate which mints a fresh Room.
- internal/collab/manager.go — RoomManager + RoomManagerConfig
· NewRoomManager wires production defaults (DefaultGraceTTL = 60s,
DefaultSchemaVersion = "1"). NewRoomManagerWithConfig accepts an
explicit config so tests can drop graceTTL to a few ms without
sleeping a minute. graceTTL is per-manager, not a package var,
so parallel tests with different TTLs don't trip the race
detector.
· Join is the public entry point: getOrCreate → addConn (with
retry on errRoomClosing) → replayTo → spawn writeLoop goroutine
→ run readLoop inline → wait for writeLoop drain → return. The
inline read keeps the HTTP handler in scope so its
`defer conn.Close()` doesn't fire until both loops exit.
· Close is for graceful server shutdown — closes every active
conn under the room mutex, then drains the manager's room map.
- internal/collab/manager_test.go — 7 tests covering: lazy create,
op-log replay-on-connect (two seed rows arrive in order), sync
broadcast + persist (peer B sees A's frame, originator does not
echo, op-log gains a row), awareness broadcast WITHOUT persist,
cross-item isolation (item-a frames don't leak to item-b
subscribers), grace-TTL reclaim with a 50ms config TTL, grace
cancel on reconnect within window, manager.Close shuts down
every active conn. All tests run with -race; the bus's
concurrent-publish test was already covered by TASK-1253.
- internal/server/handlers_collab.go — wire to RoomManager
· Returns 503 when s.collab is nil (matches the SSE handler's
"events bus not configured" 503 — fail loud rather than silently
accept the upgrade).
· Otherwise hands the upgraded conn to s.collab.Join, which
blocks until the WS closes. Unexpected close codes get the same
warn-log as before; normal closures stay quiet.
- internal/server/server.go — adds *collab.RoomManager field +
SetCollabRoomManager setter (nil-safe optional, like SetEventBus).
- cmd/pad/main.go — wires NewMemoryOpBus + NewRoomManager into
the running server alongside the event-bus wiring. Single-instance
only today; multi-replica fanout via Redis is a deferred IDEA per
the Plan body.
- internal/server/handlers_collab_test.go — adds
testServerWithCollab helper (so existing collab tests get a real
RoomManager) plus TestCollabUpgradeUnavailableWithoutRoomManager
which asserts the 503 path for unwired servers.
Parent: PLAN-1248. Phase 1 — Backend foundation.
* fix(collab): per-room appendMu + Server.Stop closes RoomManager per Codex review (round 1)
P1 — concurrent peers raced AppendYjsUpdate, violating the
single-writer-per-item contract documented on the store call. Each
peer's readLoop runs in its own goroutine, so two peers in the same
room could call AppendYjsUpdate concurrently. On Postgres that
risks the BIGSERIAL allocation-vs-commit-order cursor gap that
TASK-1252's contract was specifically guarding against. Add an
appendMu on Room held across the persist+publish sequence; reads,
awareness frames, and OTHER rooms remain unserialised.
Regression test (TestRoomManagerSerializesSyncAppends) drives 4
peers × 10 writes concurrently and asserts the op-log gains exactly
40 rows. Without appendMu this would intermittently surface fewer
rows or out-of-order ids on Postgres; with it the count is
deterministic and the race detector stays clean.
P2 — Server.Stop did not close s.collab. Active collab WS goroutines
+ grace timers could keep using s.store after the server's other
cleanup paths winding down. Add s.collab.Close() before
rateLimiters.Stop so any Join goroutines holding rate-limiter
handles can wind down cleanly. nil-safe via the existing collab
optional-attachment pattern.
* fix(collab): start writer before replay to avoid bus-overflow drops per Codex review (round 2)
P2: a joining peer subscribed to live events BEFORE its writer
goroutine started. During a long replay, live sync events would pile
up in the 64-event bus channel; once full, MemoryOpBus.Publish
silently drops them, leaving the new peer connected but permanently
missing those updates.
Restructure runConn to spawn the writer goroutine FIRST so it drains
the bus subscription concurrently with the replay. Both replay and
writer go through rc.writeMessage, which holds the per-conn write
mutex, so we never violate gorilla's one-writer-at-a-time rule.
Yjs CRDTs are commutative — applying live op 100 before replay op 50
yields the same final Y.Doc as the reverse order — so interleaving
is correct. The trade-off is a brief "out of causal order" UX wobble
during replay, which is acceptable: the alternative would require
either an unbounded queue or losing updates the way the original
order did.
* fix(pad): call srv.Stop() in serveCmd shutdown so collab sessions close per Codex review (round 3)
P2: serveCmd's SIGINT/SIGTERM path called srv.Shutdown but never
srv.Stop. http.Server.Shutdown does NOT terminate hijacked
connections (WebSockets), so active collab sessions kept running
until process exit and could race the deferred store close. The
RoomManager.Close path added in round 1 only fires inside Stop, so
without this call the production shutdown was effectively bypassing
the new cleanup.
Add srv.Stop() after srv.Shutdown in the serveCmd shutdown
sequence. Stop also runs the existing background-loop teardowns
(orphan GC, MCP audit writer, MCP session tracker) which were
previously already part of Stop's contract — those will continue to
fire as they always have, so this commit's only behavioural change
is "now also closes the collab room manager".
* fix(collab): WaitGroup drain barrier + bigger bus buffer per Codex review (round 3)
P1 — RoomManager.Close was not a true drain barrier. closeAll
closed the WebSockets but did NOT wait for the corresponding Join
goroutines (running runConn) to exit. Server.Stop returned before
in-flight collab work finished, racing the deferred store close
on process exit. Fix: track every Join in m.activeJoins
(sync.WaitGroup); Close iterates closeAll first (waking up every
reader by closing the conn), then activeJoins.Wait — guaranteeing
no collab goroutine is still running by the time Close returns.
P2 — replay-time bus overflow could still drop sync events on a
slow drain (writeLoop blocks on the same writeMu replayTo holds,
so a long replay starves the bus drain even with the writer
goroutine started before replay). Two-part response:
(a) Bump the per-subscriber bus channel buffer from 64 to 256.
Sized for a 5x safety margin on a 1k-row replay against a
chatty 5-peer room (~50 events/sec during a ~1s replay).
(b) The architectural fix — force-close subscribers on overflow,
honoring the bus's documented slow-peer recovery contract — is
filed as TASK-1273 follow-up. That requires extending the OpBus
interface (per-subscriber drop callback or counter) and an active
health-check tick in the room manager; both are out of scope for
TASK-1255's "lazy room + grace TTL" deliverable.
For PLAN-1248's single-instance scope and typical editor load,
256 covers realistic workloads. Pathological / load-test scenarios
exposing overflow can recover via Yjs's state-vector negotiation
on reconnect, and TASK-1273 will tighten that to an active kick.
* fix(collab): closed flag gates Join + Close idempotency per Codex review (round 4)
P2: http.Server.Shutdown does NOT wait for hijacked WebSocket
handlers, so a Join() call from a freshly-upgraded conn could fire
AFTER Close() returned. The previous Add-then-Wait pattern was
correct for already-started Joins but couldn't catch a Join that
hadn't yet hit Add when Close fired. Race: Close iterates the (empty)
rooms map, Wait sees zero waiters, Close returns; THEN Join hits
Add and proceeds against a torn-down store.
Add a `closed` flag gated by the same mutex that wraps
activeJoins.Add. Three orderings, all safe:
1. Add before Close.closed=true → Wait blocks until Done.
2. Close.closed=true before Add → Join sees closed=true under
the same lock and returns errManagerClosed without ever
incrementing the WaitGroup.
3. Close called twice → second call short-circuits (idempotent).
getOrCreate also gets a closed-flag short-circuit so a future
caller can't bypass the gate by skipping Join.
Test: TestRoomManagerJoinAfterCloseFailsFast asserts post-Close
Join returns errManagerClosed, plus a second Close() is a no-op.
All 15 collab tests pass under -race.
|
||
|
xarmian
|
2945ee27dd |
feat(server): add WebSocket handler at /api/v1/collab/{itemID} (TASK-1254) (#452)
* feat(server): add WebSocket handler at /api/v1/collab/{itemID} (TASK-1254)
WebSocket entry point for Yjs-based collaborative editing on a
single item under PLAN-1248. Bare-bones in this PR by design:
upgrade + log connect/disconnect + drain reads. Protocol logic
(forwarding to OpBus, persisting to op-log, awareness fan-out)
arrives in TASK-1255 (room manager).
Authorisation mirrors RequireWorkspaceAccess but keyed on the
item's workspace ID rather than a {slug} URL param — the WS URL
only carries itemID. Implementation re-uses the same access
ladder:
fresh-install escape hatch (no users)
→ grant
legacy workspace-scoped API token, no user
→ grant if token's workspace matches the item's workspace
OAuth token allow-list (TASK-953)
→ reject when workspace not on consented list
authenticated user
→ admin OR member OR has guest grants
User is re-fetched from the store on each upgrade (not trusted
from session-context cache) so a mid-session admin demotion or
member removal closes the upgrade path immediately. Mirrors
sseSubscriberStillHasAccess. Periodic per-connection
revalidation lives in TASK-1256.
Route registered alongside SSE (outside the jsonContentType
middleware group, but inside the auth middleware chain). Promotes
github.com/gorilla/websocket from indirect to direct dep and
bumps to v1.5.3 (latest stable; v1.5.0 was already in
go.mod transitively via another package).
Tests cover:
- fresh-install escape hatch grants the upgrade
- bootstrapped server rejects unauthenticated upgrade with 401
- non-member with valid session is rejected with 403
(NOT 401 — confirms the access path runs after auth, not before)
- unknown item surfaces as 404 (not 401/403 leak)
- empty itemID segment doesn't match the route
Test infrastructure note: dialCollab takes an explicit User-Agent
because pad's session-binding middleware hashes the UA at
CreateSession time and re-checks on every request — the dialer
must match what was stored, otherwise the cookie is rejected
before the workspace check fires (and we'd see a misleading 401
where 403 was expected).
Parent: PLAN-1248. Phase 1 — Backend foundation.
* style: gofmt handlers_collab_test.go per Codex review (round 1)
* fix(server): SetReadLimit + nginx upgrade headers for collab WS per Codex review (round 2)
P-MEDIUM #1: handleCollab.ReadMessage had no per-message size cap, so an
authenticated client could send an arbitrarily large frame and force
unbounded server-side buffering — the HTTP body limit applied by the
auth chain doesn't apply once the connection is upgraded. Set
SetReadLimit(1 MiB), generous for keystroke-rate Yjs ops and large
enough for a typical initial-sync state. ReadMessage returns an error
when exceeded, which the existing read loop handles as a normal close.
P-MEDIUM #2: deploy/nginx.conf routed /api/v1/collab/ through the
default `location /` block, which sets `Connection ""` (cleared so HTTP
keepalive works) — that strips the Upgrade header, so WebSocket
upgrades silently fail behind the documented nginx deployment. Add a
dedicated location block with proxy_set_header Upgrade $http_upgrade /
Connection "upgrade", same 24h read/send timeouts as SSE so an idle
editor tab does not get cut off mid-session.
* fix(server): enforce per-item visibility in collab WS upgrade per Codex review (round 3)
P2: authorizeCollabAccess granted upgrade to any workspace member or
guest-with-grants without checking whether THIS specific item was
visible to that user. A restricted member (collection_access=specific)
or a guest with grants on item A could upgrade /api/v1/collab/{itemID}
for an item B in a different collection — they'd see live edits to a
document they have no right to read.
Restructure the access ladder:
1. Workspace-level gate stays as-is: "any access at all?" If no
membership AND no grants → 403 (unchanged).
2. Item-level visibility check added on top, mirroring requireItemVisible
without depending on middleware-set request context (the WS path
doesn't go through RequireWorkspaceAccess):
- VisibleCollectionIDs nil → "all" access → grant.
- Item's collection in the visible set → grant.
- Item-level grant on this exact item → grant (covers guests
given access to a single item rather than a whole collection).
- Else → 404, mirroring requireItemVisible's "don't leak
existence" pattern.
Admin path returns nil before this check, so no change there.
Legacy workspace-scoped API tokens grant editor-equivalent access
on workspace match (predates the grants design); that branch is
untouched since legacy tokens don't have a user identity to scope
per-item grants against.
Test added: TestCollabUpgradeRejectsRestrictedMemberForeignCollection
— member with specific access to collA tries to upgrade for an item
in collB → 404. Existing 5 tests still pass.
* fix(server): strict per-item visibility check + sibling-grant test per Codex review (round 4)
P1 (round 4): VisibleCollectionIDs is broader than full-collection
access — it includes collections "anchored" by an item-level grant
(so the nav can still surface the parent collection of a granted
item). Round 3's check treated every visible collection as full
access; a guest with grant `item:A` could upgrade
/api/v1/collab/{B} for a sibling B in the same collection.
Tighten by mirroring guestResourceFilter / requireItemVisible:
1. Coarse stage stays — collection must be in the visible set.
2. NEW strict stage when the user has item-level grants:
(a) full collection grant on this collection → grant
(b) member's "specific" access list including this collection
→ grant
(c) item grant on THIS exact item → grant
Else → 404 (the visible-set hit was anchored by a sibling's
grant, not by full collection access).
When the user has NO item grants, the coarse-only check is
sufficient — visibility came from full collection access (member's
"specific" list, full collection grant, or "all" access).
Test added: TestCollabUpgradeRejectsGuestWithSiblingItemGrantOnly
— guest with item:A grant tries to upgrade for sibling B in the
same collection → 404 (the bug being regression-tested) AND verifies
the granted item A still upgrades cleanly to 101 Switching Protocols.
|
||
|
xarmian
|
40352a32e1 |
feat(auth): PAD_BYPASS_SETUP_TOKEN open-bootstrap escape hatch (#429)
Adds an env-var that lets self-host operators on trusted networks (Unraid behind a firewall, Tailscale-only deployments, homelabs) claim the first admin via the web UI without copying a bootstrap token out of the container logs. Behavior when PAD_BYPASS_SETUP_TOKEN=true: - handleBootstrap accepts non-loopback first-admin POSTs without an X-Bootstrap-Token header. The UserCount==0 invariant is unchanged, so the bypass auto-closes the moment the first admin claims the seat (subsequent bootstrap requests get 409 regardless of bypass). - handleSessionCheck returns setup_method=open so the /setup page skips the paste-token UI and renders the form directly. - Token generation is skipped at startup (no .bootstrap-token file written). A distinct WARN-flavored banner makes the open-mode trade-off obvious in operator logs. - Cloud mode (PAD_CLOUD/PAD_MODE=cloud) ignores the flag entirely. Three layers of defense: cmd/pad masks the env-var with !cfg.IsCloudServer(), Server.openBootstrapEnabled() checks !s.cloudMode, and the cloud branch in handleBootstrap never reads the bypass field. Unraid template gets a new "Bypass Setup Token" field (default false, Display="always") with a description that calls out the trust-the- network trade-off. Tests pin all the security-critical contracts: bypass admits non- loopback, bypass off keeps existing 403, cloud mode hard-ignores, loopback works either way, post-bootstrap gate stays closed, bypass wins over logs_token in session payload, cloud mode never advertises 'open' setup method. Codex review: CLEAN (round 1). |
||
|
xarmian
|
05a9665f50 |
feat(auth): first-run logs-token bootstrap flow (TASK-1167) (#424)
One-time bootstrap token generated on first start with no users in self-host mode. Token is logged in a banner the operator can grab from `docker logs`, persists at <DataDir>/.bootstrap-token (mode 0600), and bypasses the loopback-only gate via the X-Bootstrap-Token header — letting the user claim the first admin from a remote browser at /setup#token=<x>. Header-only contract + URL-fragment (browser-only, never transmitted) + log-redaction middleware keeps the secret out of access logs, proxy logs, and browser history. Cloud mode unchanged: token never loaded, never honored. Validate → UserCount-check → CreateUser → consume sequence is mutex-serialized to prevent concurrent valid-token requests from creating multiple admins. Part of PLAN-1166 (Pad on Unraid — Community Apps launch). |
||
|
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.
|
||
|
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. |
||
|
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).
|
||
|
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.
|
||
|
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.
|
||
|
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 /
|
||
|
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
|
||
|
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. |
||
|
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. |
||
|
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.
|
||
|
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.
|
||
|
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. |
||
|
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.)
|
||
|
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.
|
||
|
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.
|
||
|
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
|
||
|
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.
|
||
|
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.
|
||
|
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
|
||
|
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.
|
||
|
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.
|
||
|
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).
|
||
|
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.
|
||
|
xarmian
|
bf5ab5b366 |
chore: clear staticcheck SA + U1000 findings on main (TASK-764) (#249)
* chore: clear cosmetic staticcheck findings (TASK-764)
Apply zero-behavior-change fixes for 8 staticcheck findings on main:
- SA4023 cmd/pad/main.go:431 — drop always-true `if eventBus != nil`
guard. eventBus is wrapped in metrics.NewInstrumentedBus a few lines
above, which returns a concrete *InstrumentedBus that is never nil.
- SA1019 cmd/pad/main.go:3926 — replace deprecated strings.Title with
golang.org/x/text/cases.Title(language.English).String. golang.org/x/text
was already an indirect dep; now promoted to direct.
- SA4031 internal/server/handlers_changes.go:130 — delete dead
`if updatedItems == nil { ... }` block. make([]T, n) always returns
non-nil; the JSON marshalling already produced [] not null.
- SA9003 cmd/pad/init.go:351 — delete empty if branch and fold its
intent into the surrounding comment.
- SA9003 internal/server/handlers_dashboard.go:125 — replace empty
`if err == nil { ... }` branch with `_ = json.Unmarshal(...)` to
match the sibling settings parse and document the best-effort intent.
- SA4006 internal/cli/format.go:153 — drop the dead initial
`titlePart := item.Title` (overwritten in both branches below);
declare titlePart with `var` instead.
- SA4006 internal/store/workspaces.go:70 — drop the dead first call
to s.uniqueSlug; only the workspace-specific uniqueWorkspaceSlug
is meaningful (workspace slugs are globally unique, not workspace-
scoped like collection/item slugs).
- SA4000 internal/store/store_test.go:99 — remove always-true outer
`if idx := len(connStr) - len(connStr); idx >= 0` and unindent the
inner '?' query-string split.
go.mod side effects from `go mod tidy` under Go 1.26: golang.org/x/text
moves to direct (used directly now); pquerna/otp, prometheus/client_*
and trustelem/zxcvbn move from indirect to direct (they were already
used directly — Go 1.26's tidy correctly classifies them).
Verified:
- `go build ./...` clean
- `go vet ./...` clean
- `go test ./...` all pass (including the replaceDBName test path)
- `staticcheck -checks "SA1019,SA4000,SA4006,SA4023,SA4031,SA9003"` clean
except for handlers_dashboard.go:221 (SA4006, dashboard visibility-
filter dead block — handled in TASK-765)
Parent: PLAN-644.
* fix: clear SA5011 nil-deref in buildReconcileFindings (TASK-764)
extractItemStatus(item.Fields) on the first line of the function would
have panicked on a nil item before the `if item != nil && item.CodeContext
== nil` guard could fire. Staticcheck SA5011 flagged the inconsistency.
Drop the (item != nil) half of the guard — the function now documents
its non-nil contract in the doc comment. All callers (reconcile.go:204
plus three sites in cmd/pad/reconcile_test.go) already pass non-nil,
so this is documentation, not behaviour change.
Verified:
- `go build ./...` clean
- `go test ./cmd/pad/...` passes (the existing reconcile tests cover the
contract)
- `staticcheck -checks SA5011 ./...` clean
Parent: PLAN-644.
* chore: silence SA4017 false positive in watchCmd SSE loop (TASK-764)
cmd/pad/main.go SSE keepalive branch:
if strings.HasPrefix(line, ":") {
continue
}
Staticcheck SA4017 reports "HasPrefix doesn't have side effects and
its return value is ignored" — but the return value IS used as the
if condition. Two sibling strings.HasPrefix calls earlier in the same
for-loop body (matching "event: " and "data: " prefixes) are not
flagged, which strongly suggests an SSA-analysis quirk specific to
this branch rather than a real defect.
Suppress the finding with a //lint:ignore directive that explains
the false positive in-place. Rewriting to a different form (extract
to a bool var, comma-OK on a synthetic value, etc.) would be uglier
than the suppression comment.
Verified:
- `staticcheck -checks SA4017 ./...` clean
- `go build ./...` clean
Parent: PLAN-644.
* chore: delete dead code flagged by U1000 (TASK-764)
Pre-launch (no external contributors yet) — no consumer fork can be
relying on these unreferenced symbols, so we delete them rather than
carry the maintenance burden into v1.
## Helpers (14 functions, 1 type)
cmd/pad/main.go
- progressBar — never called
internal/cli/format.go
- stripHTMLTags — never called
internal/server/handlers_dashboard_test.go
- updateItem (test helper) — never called from any test
internal/server/handlers_items.go
- publishItemEvent — wrapper over publishItemEventWithName; all 5 call
sites use the *WithName variant directly.
- resolveRelationFields — never called.
- resolveRelationFieldFiltersForWorkspace, resolveRelationFieldFilters,
relationFilterKeys, resolveRelationFilterValue — closed loop of dead
helpers (each one only called by another dead one in the family).
- extractStatus — never called (cmd/pad/reconcile.go has its own copy).
internal/server/handlers_versions.go
- handleGetDiff (HTTP handler) — never wired into setupRouter.
- diffsToChanges, diffChange (type) — only used by handleGetDiff above.
- Removes now-unused imports `strconv` and `dmp` (sergi/go-diff).
internal/server/middleware_ratelimit.go
- writeTooManyRequests — never called; the live ratelimit middleware
uses a dedicated 429 path with Retry-After-Bucket headers.
internal/server/server.go
- guestVisibleItemIDs — never called. handlers_events.go had a
comment cross-reference; updated to drop the reference.
## Constants
internal/events/redis_bus.go
- reconnectDelay — never read.
internal/store/api_tokens.go
- defaultTokenExpiryDays — never read.
## Out of scope
The 5 unwired handlers in internal/server/handlers_documents.go are
left alone: they are the subject of TASK-769 (a product decision —
wire up vs. delete — that may want different treatment per handler).
The two SA4006/SA4010 findings on internal/server/handlers_dashboard.go
visibility-filter block are similarly left for TASK-765.
## Verified
- `go build ./...` clean
- `go vet ./...` clean
- `go test ./...` all pass
- `staticcheck -checks "SA*,U1000" ./...` clean except the two TASK-
765 / TASK-769 follow-ups noted above.
Parent: PLAN-644.
* docs: correct caller name in buildReconcileFindings doc (TASK-764)
Codex round 1 caught: the doc comment named the caller `reconcileSingle`
but the actual function is `reconcileItem` (cmd/pad/reconcile.go:204).
Fix the contract comment so it doesn't go stale on the first git blame.
|
||
|
xarmian
|
119e2d8aa2 |
feat(billing): payment-failed email endpoint + template (TASK-712 1 of 2) (#232)
* feat(billing): payment-failed email endpoint + template (TASK-712 1 of 2) Pairs with pad-cloud's invoice.payment_failed webhook handler (shipping next) to give paying users a chance to update their card before dunning exhausts and the subscription cancels. pad owns the Maileroo integration and the user→email mapping; the sidecar forwards the invoice metadata here. Changes: - email.Sender.SendPaymentFailed — new template (HTML + plain). Subject "Your Pad payment couldn't be processed"; body names the amount + next retry date when provided, falls back to generic copy when Stripe omits them, and CTAs to the billing portal so the user can update their card. Transactional (no unsubscribe link) — users who want the emails to stop either fix their card or cancel the subscription. - POST /api/v1/admin/payment-failed — new cloud-secret-gated endpoint (handlers_cloud.go). Accepts stripe_customer_id + optional pre- formatted amount_display + next_retry_display. Looks up the user, sends the email, logs a payment_failed_email_sent audit entry. Returns 200 + email_sent=false with a reason string for every non-error skip (unknown customer, no email on file, Maileroo not configured) so the sidecar never rolls back the Stripe webhook over an email failure. Returns 200 + email_sent=false + reason=send_failed when Maileroo itself errors — still no rollback. - Registered the path in cloudAdminPaths, the server router, and the CloudAdmin rate limiter so the sidecar's calls share the same rate bucket as /plan + /stripe-customer-id. - ActionPaymentFailedEmailSent audit constant for the new entry. - Three focused tests: cus_ prefix validation, unknown-customer 200, and email-not-configured 200. Added an entry to the cloud-mode gate table-driven test to confirm /admin/payment-failed also 404s when cloud mode is off. Parent: PLAN-645 (Pad Cloud Beta Readiness). TASK-712 bullet 3, pad side. pad-cloud's handlePaymentFailed wiring ships in a sibling PR. * fix(billing): audit every outcome; target user ID; add send-path tests (Codex round 1) Addresses PR #232 round 1 findings: MEDIUM — payment-failed handler only wrote an audit row on the actual send attempt, so no_customer / no_email_address / email_not_configured skip paths left no durable trail. Consolidated the audit + response into a single auditAndRespond closure called from every outcome branch, so operators can always reconstruct whether (and why) a customer was notified during dunning reconciliation. MEDIUM — audit UserID was set to actorID, which is empty for sidecar calls. /audit-log?user=<target-user-id> would never surface these events. Now set UserID to targetUser.ID whenever we have one; the no_customer branch still writes a row but with empty UserID (filtered only by action + stripe_customer_id metadata). Moved actor identity into an actor_is_admin metadata field instead. LOW — test coverage was thin: no assertion on the most important contract ("return 200 with reason=send_failed and still record the attempt"), no test of the happy send path, no audit-log assertions. Added email.Sender.SetEndpoint (exported, test-only — comment says so) so tests can point the Sender at a mock Maileroo server, plus three new tests: - TestPaymentFailed_HappyPath_SendsAndAudits - TestPaymentFailed_MailerooError_Returns200_SendFailed_AndAudits - TestPaymentFailed_UnknownCustomer_AuditsWithoutUserID The first two verify audit metadata per outcome; the third proves unknown-customer cases still leave a findable audit row. Thread-safety fix as a side-effect: Send/SendAs were reading s.endpoint outside the sender's RWMutex — fine before the mutable SetEndpoint existed, now a data race. Pulled the endpoint read into the same RLock scope as fromAddr/fromName. * fix: capture admin actor ID + audit-log formatter for payment_failed (Codex round 2) Addresses PR #232 round 2 findings: MEDIUM — auditAndRespond recorded actor_is_admin=true/false but not which admin. For manual operator-triggered calls, that meant the audit trail could not answer "who sent the dunning email?" when multiple admins touched the endpoint. Added admin_actor_id to the metadata whenever the authenticated caller has role=admin. Sidecar calls with no authenticated user still have no admin_actor_id, which correctly distinguishes them from manual admin operations. LOW — web/src/routes/console/admin/audit-log/+page.svelte falls back to "first 3 metadata keys" when no formatter exists for an action, which could hide the important reason/sent fields. Added a dedicated case for payment_failed_email_sent that renders either "sent (cus_...)" or "skipped: <reason> (cus_...)" depending on the outcome, matching the terse display style of the other switch cases. * fix(audit-log): distinguish send_failed from skip; surface admin actor (Codex round 3) Addresses PR #232 round 3 LOWs: - The formatter lumped every sent=false outcome under 'skipped', which conflates a genuine Maileroo delivery failure with a pre-send skip. Now: sent → 'sent (...)'; send_failed → 'send failed (...)'; other reasons → 'skipped (<reason>) (...)'. - admin_actor_id was recorded in metadata but invisible in the UI: the User column shows the target user via a.user_id. Appended 'by admin:<id>' to the formatted string whenever admin_actor_id is present, so manual operator calls are attributable at a glance. Sidecar calls have no admin_actor_id and render without the suffix. * fix(audit-log): register payment_failed_email_sent in action filter dropdown (Codex round 4) The backend emits payment_failed_email_sent and the custom formatter knows how to render it, but the audit-log page's ACTION_TYPES / ACTION_LABELS registry omitted the action, so admins couldn't filter for these events from the dropdown — undercutting the dunning reconciliation workflow this PR is adding. Added 'payment_failed_email_sent' to the ACTION_TYPES list and 'Payment Failed Email' to ACTION_LABELS. |
||
|
xarmian
|
775dd89fdc |
feat(cloud): add /admin/stripe-event-unmark endpoint (TASK-736 / 1 of 2) (#228)
* feat(cloud): add /admin/stripe-event-unmark endpoint (TASK-736 / 1 of 2) Parent: PLAN-645. Pair with pad-cloud follow-up. * fix(cloud): add processed_at race protection + audit log per Codex review (round 1) |
||
|
xarmian
|
6cda2da48d |
feat(billing): cancel Stripe customer on account delete (TASK-690) (#227)
* feat(billing): cancel Stripe customer on account delete (TASK-690) Parent: PLAN-645. Pair with pad-cloud PR #12. * fix(billing): abort on all non-200 per Codex review (round 1) * fix(billing): env wiring + docstrings + partial_delete test per Codex review (round 2) * fix(compose): wire cloud env vars from .env per Codex review (round 3) |
||
|
xarmian
|
0cbadf873b |
feat(server): durable Stripe webhook idempotency endpoint (TASK-696) (#226)
Adds a new cloud-gated admin endpoint that the pad-cloud sidecar uses
to record-or-detect-duplicate Stripe webhook events. Previously the
sidecar tracked processed event IDs in an in-memory map, which lost
state on restart and caused Stripe's 72h retries to re-run handlers.
Changes:
migrations/045 + pgmigrations/025
New stripe_processed_events(event_id PK, processed_at) table +
index on processed_at for the pruning query.
store/stripe_events.go
MarkStripeEventProcessed(eventID) — INSERT ... ON CONFLICT DO
NOTHING; returns alreadyProcessed from RowsAffected. Atomic.
PruneStripeProcessedEvents(maxAge) — DELETE WHERE processed_at < ?.
ShouldPruneStripeEvents() — ~1% random sample via crypto/rand.
server/handlers_cloud.go
handleStripeEventProcessed — POST /api/v1/admin/stripe-event-processed.
Validates cloud_secret, requires event_id with 'evt_' prefix,
returns {event_id, already_processed}. Opportunistically fires
a background prune ~1% of calls (7-day retention window covers
Stripe's 72h retry with a safe margin).
Adds the new path to cloudAdminPaths so the secret-marker gate
accepts X-Cloud-Secret / body-secret auth here too.
server/server.go
Registers POST /api/v1/admin/stripe-event-processed under the
existing requireCloudMode group.
server/middleware_ratelimit.go
Adds the new path to the cloud-admin rate-limit bucket alongside
/admin/plan, /admin/stripe-customer-id, /admin/user-by-customer.
server/cloud_admin_gate_test.go
Adds self-host-404 test case + two new tests:
TestStripeEventProcessed_RecordsAndDetectsDuplicates
TestStripeEventProcessed_ValidatesEventIDPrefix
Design notes in the PR body.
Parent: PLAN-645 (chunk 3).
|
||
|
xarmian
|
46fa72ca0f |
feat(server): log session IP changes, add optional PAD_IP_CHANGE_ENFORCE=strict (TASK-666) (#191)
* feat(server): log session IP changes, add optional PAD_IP_CHANGE_ENFORCE=strict (TASK-666)
Sessions stored a client IP at creation but never rechecked it. A stolen
cookie could be used from anywhere with no signal to the owner. This
change adds mid-lifetime IP-change detection without breaking legitimate
mobility (mobile roaming, VPN toggles, carrier NAT) by default.
- New audit action ActionSessionIPChanged captures {old_ip, new_ip} in
the audit metadata. Visible via the existing /api/v1/admin/audit-log.
- handleSessionIPChange wired into both SessionAuth (cookies) and
TokenAuth (padsess_ bearer). After UA check passes, compares stored
session IP to clientIP(r). On mismatch:
- log one audit row
- update the stored session IP so we don't spam the log
- strict mode: DeleteSession + 401 "session_ip_changed"
- default mode: let the request through
- Store.UpdateSessionIP lets middleware refresh the recorded IP without
tearing down the session.
- PAD_IP_CHANGE_ENFORCE=strict env var + ip_change_enforce TOML key +
Server.SetIPChangeEnforce setter (case-insensitive, trims whitespace).
- Table-driven tests cover log-only, strict rejection with session
destruction, and setter parsing edge cases.
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): dedupe session-IP-change audit via CAS, handle browser vs API paths per Codex review
Addresses two P2 comments on PR #191:
1. Race: parallel requests after an IP change could each emit
ActionSessionIPChanged before any of them updated the stored IP,
producing duplicate audit rows for a single transition.
- Replace UpdateSessionIP with UpdateSessionIPIfEquals (compare-and-set
on ip_address). Only the request that actually rotates the stored
value logs; concurrent siblings lose the CAS and skip logging.
- New test TestSessionIPChange_CASDedupesRace fires 20 concurrent
requests from the new IP and asserts exactly 1 audit row.
2. Strict-mode 401 on non-API paths:
- In current routing the SPA is mounted on the root router outside
the auth Group, so SessionAuth only fires for /api/* in practice.
The original concern about JSON 401s on browser navigation doesn't
surface today, but defense-in-depth keeps the code forward-safe:
restructure handleSessionIPChange to return a four-state outcome
(Continue / AllowedLogged / Revoked / Terminated) and only write
the JSON 401 on /api/* paths. Revoked + non-API falls through
unauthenticated so a future SPA-in-group configuration would still
render a login screen instead of raw JSON.
- Clear the session cookie (MaxAge=-1) in strict rejection so the
browser stops sending the now-revoked token on the next request.
TestSessionIPChange_StrictClearsCookies verifies the Set-Cookie.
Parent: PLAN-643 (OSS Security Hardening), TASK-666.
* fix(server): strict mode destroys session atomically, never rotate stored IP when destroying (TASK-666)
Addresses Codex P1 on PR #191: previously we rotated the session's stored
ip_address via UpdateSessionIPIfEquals BEFORE attempting DeleteSession.
If the DELETE failed (transient DB error) the row remained alive —
rebound to the attacker's new IP — so follow-up requests saw stored IP
== client IP and passed handleSessionIPChange's "match, no-op" branch.
That silently defeated strict enforcement.
- New Store.DeleteSessionIfExists returns (bool, error) to serve as the
CAS primitive for strict mode: only the caller whose DELETE affected a
row emits the audit entry, and a DB error fails closed (500 — "Unable
to validate session") rather than letting the request through.
- handleSessionIPChange splits into two paths:
* log-only mode: UpdateSessionIPIfEquals for CAS dedup (unchanged)
* strict mode: DeleteSessionIfExists is the CAS; stored IP is NEVER
rotated so any failure leaves the session bound to the OLD IP and
subsequent requests from the new IP still mismatch + still reject.
- TestSessionIPChange_StrictDestroysSessionAtomically regression test
verifies a second request from the new IP with the same token still
fails after the first strict-mode rejection.
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): exempt public API paths from strict IP-change termination (TASK-666)
Addresses Codex P2 on PR #191: SessionAuth runs for every /api/* path,
including public endpoints like /api/v1/auth/login, /api/v1/auth/register,
/api/v1/health, /api/v1/s/* (share links), and /api/v1/plan-limits. In
strict mode, a stale session cookie on those requests was rejected with
a 401 session_ip_changed BEFORE the public handler could run — the user
literally couldn't log back in because their own stale cookie blocked
the login call.
- Extract isPublicAPIPath as a shared helper between RequireAuth and
handleSessionIPChange so they can't drift out of sync.
- handleSessionIPChange strict-mode flow now: destroy session + clear
cookies + audit log (unchanged), then for public API paths return
Revoked so the handler still runs. For authenticated-only API paths
still return Terminated (401). For non-API paths return Revoked for
the SPA fallback.
- Updated TokenAuth Revoked handler to match: pass through unauth on
public paths, 401 on authenticated-only.
- TestSessionIPChange_StrictAllowsPublicAPIPaths regression test:
a stale session cookie on /api/v1/auth/login must NOT produce
session_ip_changed; /api/v1/plan-limits must still return 200.
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): short-circuit SessionAuth on token auth + fix IPv6 clientIP parsing (TASK-666)
Addresses two more Codex comments on PR #191:
P1 — SessionAuth 401'd API-token-authenticated requests:
TokenAuth sets currentUser for user-owned tokens AND tokenWorkspaceID
for legacy workspace-scoped tokens. SessionAuth short-circuited only on
currentUser, so a workspace-scoped-token request that happened to carry
a stale session cookie with a mismatched IP would be rejected by the
IP-change strict path before RequireAuth could honor the token. Extend
the short-circuit to also check tokenWorkspaceID; either signal is
enough to say "token auth already succeeded, skip cookie validation".
P2 — clientIP mangled IPv6 addresses:
clientIP used strings.LastIndex(":") on RemoteAddr. For bare IPv6
addresses like "2001:db8::1" (which TrustedProxyRealIP writes verbatim
from X-Forwarded-For, no brackets/port), that strips the final hextet
to "2001:db8:" — unusable for comparison in the new IP-change audit
path and incorrect for rate-limit keys too. Switch to net.SplitHostPort
which handles both "host:port" and "[ipv6]:port", falling back to the
raw RemoteAddr when no port is present (the trusted-proxy rewrite
case).
Tests:
- TestClientIP_IPv6NotMangled covers IPv4 w/wo port, bracketed IPv6,
bare IPv6 (no port, no brackets), and loopback forms.
- TestSessionAuth_ShortCircuitsOnAPITokenAuth exercises the worst case:
strict mode + valid API token + stale session cookie + new client IP.
Request must succeed (token wins) and NO new session_ip_changed audit
row must appear.
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): canonicalize IPs before session-IP-change comparison (TASK-666)
Addresses Codex P2 on PR #191: raw-string comparison of session.IPAddress
vs clientIP(r) would fire session_ip_changed spuriously when the same
IPv6 address arrived in different valid textual representations (the
trusted-proxy path writes X-Forwarded-For verbatim, and different hops
normalize differently — "2001:0db8::1" vs "2001:db8::1" etc.).
- canonicalIP helper: net.ParseIP + stringify to collapse equivalent
IPv6 forms (compressed vs expanded, case, leading zeros) and IPv4-in-
IPv6 into a single canonical string. Non-parseable inputs pass through
unchanged so debug/malformed values behave predictably.
- handleSessionIPChange compares and logs the canonical forms. The CAS
still passes session.IPAddress (the raw stored value) to the DB — the
compare-and-set is about row identity — but the new IP written in is
the canonical form so future comparisons are stable.
- TestCanonicalIP covers empty, IPv4, shorthand "::1", expanded 8-group
equivalent, mixed-case 2001:DB8::1, fully expanded 2001:0db8:…:0001,
and non-IP fallback.
Parent: PLAN-643 (OSS Security Hardening).
|
||
|
xarmian
|
a2eaac4a37 |
fix(server): reject CORS wildcard when credentials are on (TASK-664) (#188)
PAD_CORS_ORIGINS accepted any string (including '*') while the CORS middleware ran with AllowCredentials=true unconditionally. Browsers refuse the combination per the Fetch spec, so a typo like PAD_CORS_ORIGINS=* "worked" in curl but failed silently from every real browser — and without an explicit carve-out, an anon cross-origin fetch still rode the victim's cookies when origins were empty. - parseCORSOrigins: explicitly drop '*' with a log warning. When '*' was the ONLY configured origin, fall back to localhost defaults rather than producing an empty allowlist. - corsAllowCredentials: new helper — AllowCredentials=true only when an operator has set PAD_CORS_ORIGINS. Default false keeps a browser on a different origin from piggy-backing cookies on the user's session when no remote origin was expected in the first place. - server.go: wire up corsAllowCredentials(s.corsOrigins) into the cors.Options. Tests: - TestParseCORSOrigins gains three '*'-handling cases (lone '*', mixed, trailing '*'). - TestCorsAllowCredentials covers empty/whitespace default, explicit origins, and tab-only input. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
baa1f75847 |
fix(server): cap JSON body + header size (TASK-663) (#184)
* fix(server): cap JSON body + header size (TASK-663) decodeJSON called json.NewDecoder(r.Body).Decode(v) with no size limit. Any client could POST a multi-GB JSON blob and watch Pad stream the whole thing into one allocation — a single request could OOM the process. - internal/server/server.go: wrap r.Body in http.MaxBytesReader(..., 2 MB) inside decodeJSON. Every legitimate payload (item, collection, auth, etc.) is well under 100 KB so 2 MB is several orders of magnitude above real traffic. Factor out decodeJSONWithLimit(maxBytes) so future bulk-import endpoints can opt in to a larger cap without removing the wrapper. - internal/server/server.go: set MaxHeaderBytes = 64 KiB on the http.Server (default is 1 MB). Plenty for cookies/auth/CORS while cheaply rejecting header-flood DoS. Test: decode_json_test.go covers the 3 MiB body rejection, a happy path, and a custom-limit override that rejects a 1 MiB body under a 256 KiB cap. Parent: PLAN-643 (OSS Security Hardening). * fix(server): bump workspace import JSON cap to 64 MiB per Codex P1 Codex flagged that handleImportWorkspace inherits the new 2 MiB default cap, but WorkspaceExport contains full collections, items, comments, and item_versions for the workspace — a realistic project backup routinely exceeds 2 MiB, so existing exports stop re-importing. Switch to decodeJSONWithLimit(64 << 20). 64 MiB is multiple orders of magnitude above any realistic single-workspace backup while still far from heap-exhaustion territory. |
||
|
xarmian
|
c2b67f5a9d |
fix(server): gate cloud admin endpoints via requireCloudMode (TASK-655) (#182)
* fix(server): gate cloud admin endpoints via requireCloudMode (TASK-655) middleware_auth.go:184-189 and middleware_csrf.go:44-48 permanently exempted /api/v1/admin/plan, /admin/stripe-customer-id, and /admin/user-by-customer from RequireAuth and CSRFProtect — by path, not by credential. In self-host mode these endpoints still responded to every anonymous network caller (with "Cloud mode not configured"), confirming their existence and telegraphing that the auth surface was non-standard. Three tightly-coupled changes: 1. Narrow both carve-outs from path-based to credential-based. The new isCloudSecretAuthAttempt(r) helper checks for X-Cloud-Secret header or legacy ?cloud_secret query-param; only requests that present one bypass auth/CSRF. Cookie-based admin callers continue through the normal session + CSRF gate. 2. Wrap the three endpoints in a dedicated requireCloudMode group. Self-host mode → 404, no endpoint-existence disclosure. 3. Admin callers via cookie now properly require CSRF for these endpoints (they previously bypassed), bringing them in line with every other /admin/* endpoint. Tests (cloud_admin_gate_test.go): - TestCloudAdminGate_SelfHost_Returns404 — anon + X-Cloud-Secret in self-host → 404 (requireCloudMode fires). - TestCloudAdminGate_NoCloudSecret_RequiresAuth — cloud mode + no secret → 401 from auth gate (not the old "Cloud mode not configured"). - TestCloudAdminGate_ValidCloudSecret_PassesAuthAndCSRF — sidecar with matching X-Cloud-Secret reaches the handler; neither 401 nor 403. - TestCloudAdminGate_QueryParamSecret_BackwardCompat — legacy ?cloud_secret= on GET still works (TASK-656 removes this next). Parent: PLAN-643 (OSS Security Hardening). * fix(server): scope cloud-secret auth bypass to cloud admin paths per Codex P0 Codex caught a regression in the first cut: isCloudSecretAuthAttempt(r) only checked for the presence of X-Cloud-Secret/?cloud_secret, so setting either header on ANY path (e.g. GET /api/v1/workspaces) would bypass RequireAuth globally. An anonymous attacker could list or create workspaces just by adding one of those markers. Add a cloudAdminPaths whitelist and require the request path to be one of the three cloud admin endpoints before honoring the bypass. Defined as a map so a future /api/v1/... route can't accidentally inherit it. Regression test TestCloudAdminGate_BypassScopedToCloudPaths: - GET /workspaces + X-Cloud-Secret → 401 (not bypass) - GET /workspaces?cloud_secret=x → 401 (not bypass) - POST /workspaces + X-Cloud-Secret → 4xx (CSRF 403 or auth 401) * fix(server): make cloud-secret path gate visible at call sites Codex re-flagged the path scoping on PR #182 — even after the fix, the helper name 'isCloudSecretAuthAttempt' made the path scoping invisible at the call site. Split into two primitives: - isCloudAdminPath(path) — path whitelist check - hasCloudSecretMarker(r) — header/query marker check Both middleware now combine them explicitly: if isCloudAdminPath(path) && hasCloudSecretMarker(r) { ... } Behaviorally identical to the previous fix — tests still show GET /workspaces with X-Cloud-Secret returning 401, POST /workspaces with X-Cloud-Secret returning 403. Just makes the invariant readable in RequireAuth and CSRFProtect without having to jump to the helper. * fix(server): preserve body-cloud_secret auth for sidecar POSTs per Codex P1 Codex caught that POST sidecar calls carrying cloud_secret only in the JSON body (the current pad-cloud sidecar behavior) would fail at RequireAuth/CSRFProtect after this PR — handler-level validation never runs. Breaking deployed sidecars isn't the intent of TASK-655; TASK-656 deprecates body+query cloud_secret in favor of X-Cloud-Secret header exclusively, but that's a separate migration. Add body peek to hasCloudSecretMarker for POST/PUT requests with application/json content-type: - Read up to 64 KB of r.Body into a buffer. - Replace r.Body with an io.NopCloser wrapping the buffer so downstream handlers can still decode the JSON. - Return true if the parsed body has a non-empty cloud_secret field. Parse errors and missing fields → false (request falls through to the normal auth rejection, no permissiveness). The peek only runs when the caller is already hitting a cloud admin path via the explicit isCloudAdminPath() gate at the call sites, so the body-read cost is bounded to three endpoints. Test: TestCloudAdminGate_BodySecret_BackwardCompat posts with cloud_secret in the JSON body and no X-Cloud-Secret header, asserts the request reaches the handler (404 from unknown user_id, not 401/403 from middleware). |
||
|
xarmian
|
7d3b468fc8 |
feat(server): gate /metrics behind loopback + bearer token (TASK-653) (#180)
cmd/pad/main.go:277 unconditionally registered Prometheus metrics and internal/server/server.go:229 served /metrics with no auth/CSRF. Any caller on the network could read workspace counts, API usage patterns, and (via label enumeration) user/workspace IDs. Three-layer gate: 1. Loopback-only default. No PAD_METRICS_TOKEN configured → /metrics accepts loopback peers only (safe for self-hosters running Prometheus on the same host, which is the common case). Non-loopback peers get 403 with a clear message. 2. Bearer-token mode. PAD_METRICS_TOKEN set → every scrape must send "Authorization: Bearer <token>", compared in constant time. Missing or wrong header → 401 with WWW-Authenticate: Bearer realm="metrics". 3. Rate-limit/logging chain still wraps the endpoint from the outer router.Use calls. Wiring: - internal/config/config.go — MetricsToken field + PAD_METRICS_TOKEN env. - cmd/pad/main.go — plumb cfg.MetricsToken into SetMetricsToken. - .env.example — document PAD_METRICS_TOKEN with openssl-rand hint. - internal/server/server.go — metricsAuth middleware + subtle.ConstantTimeCompare. Tests: metrics_auth_test.go covers loopback allowed, LAN denied, missing/wrong/correct Bearer, non-Bearer scheme rejected, WWW-Authenticate header, and the SetMetrics-absent 404. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
fc5a54dff7 |
fix(server): read raw TCP peer for loopback check (TASK-662) (#175)
* fix(server): read raw TCP peer for loopback check (TASK-662) TrustedProxyRealIP rewrites r.RemoteAddr when the peer is a trusted proxy. Without additional defense, an attacker reaching a trusted reverse proxy could set X-Forwarded-For: 127.0.0.1 and trick the bootstrap loopback check into accepting them as a local caller — reopening the full-instance-takeover path that TASK-660 closed at the spoof layer. Add CapturePeerAddr middleware that runs BEFORE TrustedProxyRealIP and stashes the untampered r.RemoteAddr in request context. Change requestIsLoopback to read via rawPeerAddr(r) (context-first, with a safe fallback for test paths that skip the middleware). r.RemoteAddr stays the rewritten value for the rate-limiter / audit-log paths that actually want the client's IP. Tests cover: direct loopback → true; direct LAN → false; trusted proxy forwarding spoofed 127.0.0.1 → false; untrusted peer with spoofed XFF=127.0.0.1 → false; and that rawPeerAddr falls back to r.RemoteAddr when CapturePeerAddr is absent. Parent: PLAN-643 (OSS Security Hardening). * fix(server): require loopback peer AND no proxy headers for bootstrap (Codex P1) Codex caught a regression in the initial PR: reading rawPeerAddr(r) made every request through a same-host reverse proxy look loopback, so a Caddy or nginx on 127.0.0.1 forwarding public traffic would let attackers reach the bootstrap endpoint from the internet. Tighten the rule to two independent conditions: 1. The untampered TCP peer is a loopback address. 2. Neither X-Forwarded-For nor X-Real-IP is set. A legitimate local CLI calling Pad directly satisfies both. A reverse proxy forwarding public traffic always sets the forwarding headers, so the presence of either disqualifies the request. The raw-peer check still defeats X-Forwarded-For spoofing from non-loopback attackers, and now also handles the Codex-flagged scenario where a local proxy is trusted or left misconfigured. Tests updated to cover: direct loopback no-headers allowed; loopback peer + XFF rejected; loopback peer + X-Real-IP rejected; IPv6 loopback allowed. |
||
|
xarmian
|
ec9edef68c |
fix(server): gate RealIP on PAD_TRUSTED_PROXIES (TASK-660) (#173)
Replace the unconditional chimiddleware.RealIP with a middleware that only trusts X-Real-IP / X-Forwarded-For when the direct TCP peer is within a configured CIDR. With the safe default (PAD_TRUSTED_PROXIES unset) proxy headers are ignored entirely — the real TCP peer address is used for rate limiting, the bootstrap loopback check, and audit logs. Why: previously any client could set X-Forwarded-For to bypass per-IP rate limits AND the bootstrap loopback check (handlers_auth.go). On a direct-exposed Docker deploy (see M6, TASK-661) this compounded into a full-takeover chain. Gating RealIP breaks that chain even when the operator forgets to firewall the port. - internal/server/middleware_realip.go — new TrustedProxyRealIP middleware + ParseTrustedProxyCIDRs helper (accepts CIDRs or bare IPs, invalid entries logged+skipped, empty = nil result = no-op middleware). - internal/server/server.go — swap chimiddleware.RealIP for the gated version; add trustedProxyCIDRs field and SetTrustedProxies wiring. - internal/config/config.go — TrustedProxies field + PAD_TRUSTED_PROXIES env var. - cmd/pad/main.go — plumb config to the server. - internal/server/middleware_realip_test.go — covers no-trust default, untrusted peer, trusted peer with X-Real-IP, X-Forwarded-For first entry, and invalid header. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
204d63151f |
feat(server): strict-dynamic CSP + fail-fast missing index.html (TASK-375) (#172)
Completes the remaining items on the nonce-based CSP work: 1. Add 'strict-dynamic' to script-src. In CSP-L3 browsers this supersedes the 'self' host-list, so a future XSS that injects <script src="//evil"> is blocked even though 'self' is still listed (kept as fallback for older browsers). The SvelteKit bootstrap script already dynamically imports the runtime chunks, which is exactly the pattern strict-dynamic is designed to permit. 2. Fail fast when the embedded index.html can't be read. The previous silent-swallow returned blank HTML to every SPA request, which is a broken build that the operator should notice immediately. Panic at startup so the server refuses to come up with a broken UI. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
4297689e23 |
fix(server): add script-src-attr 'none' to CSP (TASK-648) (#171)
Inline event handlers (onerror, onload, onclick, …) bypass the script-src directive per CSP spec. Without script-src-attr 'none' an attacker who slips markup past the DOMPurify sanitizer can still execute JavaScript via event attributes — defeating the whole point of the nonce-based script-src. Add 'script-src-attr 'none'' to both CSP headers: - internal/server/middleware_security.go — strict policy for API responses - internal/server/server.go — nonce-based policy for HTML pages Defense-in-depth for TASK-647 (comment markdown sanitizer) and for any future regression in HTML-emitting paths. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
844e40f0a9 |
feat: add star/unstar API endpoints (#116)
* feat: add star/unstar API endpoints
Add REST API for item starring (PLAN-564, TASK-566):
- POST /workspaces/{ws}/items/{slug}/star — star item (idempotent, 204)
- DELETE /workspaces/{ws}/items/{slug}/star — unstar item (204 or 404)
- GET /workspaces/{ws}/items/{slug}/star — check star status ({"starred": bool})
- GET /workspaces/{ws}/starred — list starred items (?include_terminal=true)
All endpoints are scoped to the authenticated user, check item visibility
via RBAC/grants, and enrich list responses with parent links and refs.
* fix: enforce RBAC visibility filtering on starred items list
Apply the same collection/item grant filtering used by handleListItems
to handleListStarredItems. Without this, guests or restricted members
could see starred items from collections they no longer have access to.
|
||
|
xarmian
|
56adba4b58 |
feat: add invitation management panel for admin console (#107)
* feat: add invitation management panel for admin console Platform-wide view of all pending invitations with search, resend, and revoke. Resend creates a fresh invitation code and sends the email. New admin endpoints: GET/POST/DELETE for /admin/invitations. * fix: check email opt-out on resend, abort on stale delete, reload list Respect unsubscribe preferences before resending invitation emails. Abort resend if the old invitation was already accepted/revoked concurrently. Reload the full invitations list after resend since the row ID changes. |
||
|
xarmian
|
86451174ad |
feat: add user detail panel with workspace memberships (#106)
* feat: add user detail panel with workspace memberships
New GET /api/v1/admin/users/{id}/workspaces endpoint returning workspace
name, slug, role, and join date. Frontend loads memberships when a user
row is expanded and displays them as a linked list with role badges.
* fix: scope workspace fetch error/loading to active selection
Gate both the catch and finally blocks with a selectedId check so stale
requests from previously selected users don't wipe workspace data or
clear the loading indicator for the current selection.
|
||
|
xarmian
|
d968b551b7 |
feat: add account disable/deactivation (#104)
* feat: add account disable/deactivation for admin users Allow admins to soft-disable user accounts without deleting data. Disabled users get a 403 on all authenticated requests, their sessions are invalidated on disable, and they show as visually dimmed with a red "disabled" badge in the admin console. Includes migration for disabled_at column, auth middleware check, disable/enable endpoints with audit logging, and frontend toggle with confirmation dialog. * refactor: auto-discover migrations from embedded filesystem Replace hardcoded migration lists with fs.ReadDir on the embedded FS directories. New migrations are now picked up automatically by filename sort order — no need to manually register them in store.go. * fix: block disabled users at login and capture IDs before async calls Reject disabled accounts in the login handler before session creation, not just in RequireAuth middleware (which exempts auth routes). Also capture selectedId into a local const in all async admin panel functions to prevent stale updates if the selection changes during a request. * fix: enforce disabled check in OAuth and password reset flows, always invalidate sessions Block disabled users in all session-minting paths (OAuth login, password reset) not just password login. Also remove early return for already-disabled users in the disable endpoint so session invalidation always runs, handling retry after partial failure. |
||
|
xarmian
|
79d7d26a00 |
feat: add admin password reset for other users (#103)
* feat: add admin password reset for other users
New POST /api/v1/admin/users/{id}/reset-password endpoint. When email is
configured, sends a password reset link. Otherwise generates a temporary
password and invalidates existing sessions. Includes audit logging via
new password_reset_by_admin action and frontend UI with confirmation.
* fix: treat session revocation and email send as hard failures
Make session invalidation failure abort the reset instead of silently
continuing, and send the reset email synchronously so delivery failures
are surfaced to the admin caller.
|