mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-20 09:33:28 +00:00
504d348917c2fb8ed2c139bbbc352e07fccae19a
211 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
756d91acad |
fix(ci): gofmt + bump race-detector timeout to 30m (#299)
CI on main has been failing since the PLAN-866 attachment work
landed. Two independent issues:
1. gofmt failures (golangci-lint) — seven files in the attachments
path had trailing-comment alignment that gofmt wanted nudged a
column. Pure whitespace; ran `gofmt -w` across the affected
files. golangci-lint's gofmt linter caught it on every PR /
push since TASK-870 but we hadn't been watching those signals.
Files cleaned: internal/attachments/{fs_store_test,mime,
mime_test,processor_test}.go, internal/server/{
handlers_attachments_download_test,handlers_attachments_transform,
render/attachments_test}.go.
Local guard: `gofmt -l ./...` now exits clean.
2. Race-detector tests timed out at 20m on the GitHub-hosted runner.
Two contributors:
- PostgreSQL adds latency on every CREATE/DROP plus on the
bcrypt hash inside auth/bootstrap (~3s per call under -race
on the runner). Tests that bootstrap a fresh user (e.g.
TestSessionIPChange_*) pay the full cost each time.
- The PLAN-866 image-processing tests (thumbnail derivation,
rotate / crop transform) added ~2-3 minutes of decode/encode
work on top of the existing suite.
The previous "20m gives margin without papering over a hang"
comment was right at the time it was written; we now genuinely
need more headroom. Bumped to 30m on both the SQLite and
PostgreSQL race steps. Genuine deadlocks would still trip this
and produce the goroutine-dump panic — we just stop confusing
"slow but progressing" with "permanently hung".
Reference points before / after:
- TASK-875 main run #294: Go (PostgreSQL) finished in 17m48s ✓
- TASK-880 main run #298: Go (PostgreSQL) hit 20m timeout ✗
- Local: my new tests under -race add ~63s on a developer laptop
(TestThumbnails + TestTransform + TestProcessor combined).
Verification:
go test ./... — pass
go vet ./... — clean
gofmt -l (recursively) — clean
|
||
|
xarmian
|
3bf1b60365 |
feat(attachments): editor image crop with aspect presets (TASK-880) (#298)
Adds a drag-to-crop modal on top of the AttachmentImage toolbar
introduced in TASK-879. The /transform endpoint already accepted
the "crop" operation shape from TASK-879 — this PR wires the editor
UI plus the supporting tests.
Editor:
- attachment-crop-modal.ts (new): pure-DOM crop modal in the same
style as the existing image lightbox. Returns a Promise that
resolves to the crop rect in ORIGINAL-IMAGE pixel coordinates
when the user clicks Apply, or null on cancel / dismiss /
image-load failure.
- Image fits to a centered <dialog> via flex layout; backdrop
click and Esc both cancel cleanly.
- Crop rectangle starts at 80% of the image, centered. Body is
a "move" handle; four corner handles resize.
- Aspect presets: Free, 1:1, 4:3, 16:9. Preset clicks snap the
current rect to the new ratio while preserving its center;
subsequent corner drags clamp to the locked ratio.
- Pointer events (touch + mouse for free) with setPointerCapture
so drag continues even if the cursor leaves the handle.
- Coordinate translation: rect in preview-pixel space →
naturalWidth / offsetWidth scale → original-image pixel
space. Result is clamped to natural bounds so a fractional-
rounding overrun doesn't push the rect off-image.
- attachment-image.ts: extracts swapNodeUuid() helper from
runRotate so runCrop can share the setNodeMarkup +
invalidate-old-metadata flow. The toolbar gains a fourth
button (⌶ Crop…) that opens the modal pointed at the original
variant. Per-format gating (refreshToolbarState) treats the
crop button identically to the rotate trio — both go through
/transform, so a libvips-only format (e.g. WebP on the pure-Go
build) disables the whole toolbar with the same explanatory
tooltip.
- app.css: full styling for the crop modal — header with aspect
toolbar, image stage with shadow-cutout overlay around the
crop rect, four corner handles, footer with Cancel + Apply.
Uses the existing CSS-variable palette so light/dark mode
track automatically.
Server tests (3 new):
- TestTransform_CropProducesNewBlobAtRectDimensions: end-to-end
PNG crop, verify the response dimensions AND that the served
bytes decode at the same dimensions (guards against an
encode-pipeline off-by-one).
- TestTransform_CropClipsToImageBounds: rect that extends past
the image boundary clips rather than 400ing — the editor's
rounding can produce rect+1px past natural width/height in
rare fractional-scale cases, and the processor's Crop
intersects with image bounds for exactly this reason.
- TestTransform_CropRejectsBadRect: missing rect, zero width,
negative xy, rect entirely outside → 400.
Parent: PLAN-866. Closes the editor-side image-tools track on top of
TASK-878 (Processor) and TASK-879 (rotate / transform endpoint).
|
||
|
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
|
5af54ddc05 |
feat(attachments): markdown reference resolver for pad-attachment:UUID (TASK-874) (#291)
* feat(attachments): markdown reference resolver for pad-attachment:UUID (TASK-874)
Add the shared step that translates `pad-attachment:UUID` markdown
references into rendered HTML for image embeds, file chips, and missing
placeholders. Wired into the editor preview path; Go-side helpers seed
the future server-side rendering pipeline (export / shared item view).
TS side (`web/src/lib/markdown/attachments.ts`):
- Pure helpers: parseAttachmentHref, attachmentDownloadUrl, isImageMime,
formatAttachmentSize, renderAttachmentImage/Chip/Missing
- resolveAttachmentImage / resolveAttachmentLink for the marked hooks
- Image MIME → <img src=...?variant=thumb-md data-attachment-id=...>
- Non-image MIME (or link syntax) → file chip with download attribute
- Missing/deleted → "Missing attachment" placeholder span
`web/src/lib/utils/markdown.ts`:
- renderer.image override (defaulting to marked's standard image when
href is not pad-attachment:)
- renderer.link checks for pad-attachment: prefix before the existing
external/internal-link logic
- renderMarkdown gains an optional attachmentResolver parameter; the
resolver is threaded via a per-call module slot (synchronous render)
- DOMPurify allowlist extended with data-attachment-id, download,
width, height — ALLOW_DATA_ATTR stays false so only this single
data-* attribute slips through
Go side (`internal/server/render/attachments.go`):
- Mirror of the TS API so server-rendered output matches client output
byte-for-byte for the same input
- ResolveAttachmentReferences scans markdown source via regex,
skipping fenced code blocks (backtick + tilde), substitutes both
image and link forms
- Comprehensive table-driven tests (24 cases) covering: href parsing,
URL building, MIME detection, size formatting, image/chip/missing
rendering, escape safety against script-tag injection in alt /
filename / display text, fenced-code skip, tilde fences, title
suffix on link destinations, nil resolver pass-through, no false
positives on non-attachment URLs, deterministic round-trip
References are stored as opaque `pad-attachment:UUID` so a backend
migration (FS → S3) can rewrite storage_keys without touching item
content. See DOC-865 for the architecture.
Parent: PLAN-866 (Attachments Phase 1).
* fix(attachments): chip label double-escape + escaped-bracket lockstep per Codex review (round 1)
Two findings from the round-1 Codex review:
1. TS chip labels were double-escaped. renderer.link was passing the
parseInline(tokens) HTML output to resolveAttachmentLink, which feeds
it into renderAttachmentChip → escapeHtml. A label like
`[**Report**](pad-attachment:id)` rendered literal
`<strong>Report</strong>` instead of plain text. Switched
to the link token's raw `text` field; markdown emphasis inside chip
labels now degrades to literal markers (acceptable for filename-style
labels) and matches what the Go regex extracts.
2. Go regex didn't accept CommonMark `\]` / `\\` escapes inside link/image
labels, so `[Q1 \] report](pad-attachment:id)` resolved on the TS side
(marked handles escapes) but stayed literal on the Go side — breaking
the documented lock-step contract. Updated the regex to accept escaped
characters inside the alt/text capture, and added unescapeMarkdownText
to mirror marked's behavior of dropping the backslash before the label
reaches the render helpers.
Tests added: TestResolveAttachmentReferences_EscapedBrackets covers
image alt, link text, and combined backslash/bracket escapes;
TestUnescapeMarkdownText is the unit-level table for the unescape
helper (including dangling-backslash and non-punctuation pass-through).
|
||
|
xarmian
|
fc1c47f124 |
feat(attachments): CLI + TypeScript clients + types (TASK-873) (#290)
* feat(attachments): CLI + TypeScript clients + types (TASK-873)
Rounds out the API surface with Go and TS client methods + a
\`pad attachment\` Cobra subcommand for ops debugging.
internal/cli/client.go
AttachmentUploadResult struct mirrors POST /attachments JSON.
UploadAttachment streams a multipart file part via io.Pipe — never
buffers the upload in memory. itemRef is optional. Uses a fresh
http.Client with a 5-minute timeout per request so a 25 MiB upload
over a constrained link doesn't trip the package-shared 10s default.
DownloadAttachment streams the bytes into the caller's writer,
returning Content-Type + total bytes copied. Optional ?variant=
parameter for thumbnails (server falls back to original silently
per TASK-872).
cmd/pad/main.go
pad attachment upload <item-ref|-> <path> [--filename NAME]
pad attachment download <id> <out|-> [--variant thumb-sm|thumb-md]
Item arg accepts an issue ref (TASK-5) or slug; "-" means no parent.
Out arg "-" streams to stdout (with status messages on stderr) so
callers can pipe into image viewers etc. Resolves the item via
GetItem first so a typo'd ref fails fast with a useful error.
List + delete subcommands intentionally omitted — those endpoints
ship with TASK-881 (storage usage) and the future GC task. Adding
client methods that hit 404s would mislead callers; same logic kept
the upload response's "url" out of TASK-871 until TASK-872 wired GET.
web/src/lib/types/index.ts
Attachment interface mirroring the Go model (pointer types → optional).
AttachmentUploadResult interface for the upload response shape.
web/src/lib/api/client.ts
api.attachments.upload(workspaceSlug, file, itemId?) — multipart
POST via direct fetch (skips shared request() because that helper
hard-codes Content-Type: application/json). Carries CSRF, cookies,
and the same 401 → /login redirect.
api.attachments.downloadUrl(workspaceSlug, attachmentId, variant?)
is a pure URL builder so callers can wire <img src> directly without
going through fetch.
End-to-end smoke verified:
pad attachment upload TASK-869 /tmp/tiny.png # uploads PNG
pad attachment download <id> /tmp/dl.png # bytes are identical
cmp /tmp/tiny.png /tmp/dl.png # PASS
Verification
go build ./... — clean
go vet ./... — clean
go test ./... — all packages pass
cd web && npm run build — clean
make install — server restarts on the new binary
Parent: PLAN-866.
* fix(cli): atomic download — write to temp + rename so a failed download doesn't truncate the destination per Codex review (round 1)
P2: pad attachment download <bad-id> /existing/file used to wipe the
existing file on auth/network/404 errors because os.Create truncated
before the request was even attempted. The bytes were never written
because DownloadAttachment errored out, but the destination was
already 0 bytes — a footgun for anyone running the CLI in scripts.
Fix: for the file-path case, write to a sibling .tmp via os.CreateTemp
in the destination directory, fsync, close, then os.Rename only on
success. Same atomic-write pattern as FSStore.Put. The defer cleans
up the .tmp on any error path.
The stdout case (outPath == "-") is unchanged — bytes already
streamed to stdout can't be rolled back, so any partial write is
just visible to the caller as a short payload.
Verified end-to-end:
echo X > /tmp/existing.png
pad attachment download not-a-real-id /tmp/existing.png # errors
cat /tmp/existing.png # still "X" — file untouched
* docs(cli): clarify os.Rename atomic-replace behavior on Windows (Codex round 2 disagreement)
Round 2 flagged this as P2: "os.Rename does not replace an existing
destination on Windows." That is technically incorrect for modern Go.
Verified directly against the Go stdlib source:
src/internal/syscall/windows/syscall_windows.go:
func Rename(oldpath, newpath string) error {
...
return MoveFileEx(from, to, MOVEFILE_REPLACE_EXISTING)
}
MoveFileEx with MOVEFILE_REPLACE_EXISTING atomically replaces an
existing destination on Windows. This has been the behavior since
Go 1.5 (2015), so every version of Go this codebase supports already
gets the desired replace-on-rename semantics on every platform.
Added an inline code comment so future readers don't worry about the
same false alarm. No code-path change.
|
||
|
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
|
de4d28d576 |
feat(attachments): AttachmentStore interface + FSStore (TASK-870) (#287)
* feat(attachments): AttachmentStore interface + FSStore (TASK-870) Introduces the storage backend abstraction described in DOC-865 and ships its first concrete implementation. No call sites yet — TASK-871 (upload API) wires it in. internal/attachments/store.go AttachmentStore interface (Put/Get/Stat/Delete) and ErrNotFound sentinel. Put is documented as idempotent — concurrent Puts of the same hash converge — and required to verify that the streamed bytes actually hash to the supplied value. internal/attachments/registry.go Registry routes "<prefix>:<rest>" keys to the store registered for that prefix (Phase 1 = "fs"; Phase 2 will register "s3" alongside). Convenience Get/Stat/Delete helpers resolve + forward in one call so callers don't have to spell out the two-step pattern everywhere. Register panics if the prefix contains ':' since that would make the store unreachable. internal/attachments/fs_store.go FSStore writes to <baseDir>/<aa>/<bb>/<full-hash> with the first 4 hex chars sharding the directory tree two levels deep. Atomic writes: stream + hash to a randomized .tmp in the destination dir, fsync, then intra-directory rename. The streaming sha256 is verified against the supplied hash before the rename, so a mismatch never leaves a visible file. Idempotent fast path: if the canonical file already exists Put short-circuits (and drains the reader so callers don't get a stuck stream). Get returns wrapped ErrNotFound on missing keys; Delete on a missing key is a no-op (matches what the orphan GC needs). Tests cover put/get/stat/delete, hash mismatch, invalid hash format, idempotency, 16-goroutine concurrent Put of the same hash converging to one on-disk file with no orphan tmp files, registry routing, forward-error semantics, and the prefix-with-colon panic. Parent: PLAN-866. * fix(attachments): validate hash on every FSStore key + verify on fast path per Codex review (round 1) Round 1 raised two issues — both real, both fixed. 1. Path traversal in Get/Stat/Delete. extractHash only checked that the key began with "fs:" and the suffix was non-empty before passing it to pathFor(), which used the suffix as a path component. A key like "fs:../../etc/passwd" would escape baseDir for reads/stats/deletes. Fix: extractHash now requires the canonical 64-char lowercase-hex sha256 form via validHash. Same gate that Put already used; now it covers every public method. 2. Idempotent Put fast path skipped hash verification. If the canonical target file already existed, Put returned the key without checking that the supplied reader's bytes hashed to the supplied hash — violating the AttachmentStore.Put contract that implementations MUST verify on every call. A buggy upload path could associate the wrong bytes with an existing hash and silently succeed. Fix: stream r through a hasher when the target exists (no disk I/O), compare against the supplied hash, and reject on mismatch. Also dropped the dead "_short" branch in pathFor — every caller now goes through validHash. Tests added: - TestFSStore_GetStatDeleteRejectBadKeys covers empty/wrong-prefix/empty- hash/non-hex/wrong-length/path-traversal/path-separator/uppercase keys across all three read methods. - TestFSStore_PutFastPathStillVerifiesHash confirms the contract holds on the fast path: a second Put that lies about the hash is rejected with no corruption of the existing file. |
||
|
xarmian
|
6461aafd16 |
feat(store): attachments table + Attachment model (TASK-869) (#286)
Adds the schema groundwork for inline images and file uploads — see DOC-865 (Attachments — architecture & migration design). - migrations/047_attachments.sql — SQLite migration. Table + 4 indexes (workspace, item, hash, parent). Partial indexes on workspace/item/parent match the items table convention. The hash index is full (not partial) so dedupe can resurrect a soft-deleted blob if the same bytes are re-uploaded without writing a duplicate. - pgmigrations/026_attachments.sql — Postgres mirror with BIGINT for size_bytes; same partial-index pattern. - internal/models/attachment.go — Go model with all columns. Uses pointer types for nullable columns (item_id, width, height, parent_id, variant, deleted_at) so JSON omitempty works correctly. No call sites yet — purely schema groundwork. Verified the migration runs cleanly on a fresh install and on the live dev DB. Parent: PLAN-866. |
||
|
xarmian
|
e5eae5e94e |
feat(web): persistent dismissible CLI-connect banner on workspace pages (TASK-862) (#284)
* feat(web): persistent dismissible CLI-connect banner on workspace pages (TASK-862)
Final web piece of the web-first onboarding on-ramp from PLAN-859 / IDEA-750.
A slim banner now nudges users to connect their workspace to the CLI on
every workspace page, until they either dismiss it or actually do it.
Server:
- New store method WorkspaceHasCLISource(workspaceID) — backed by
EXISTS(... WHERE source='cli' AND deleted_at IS NULL), so it's a
cheap O(1) check that short-circuits on the first match.
- Dashboard payload (GET /workspaces/{ws}/dashboard) gains
HasCLISource bool (json: has_cli_source).
- Unit tests cover empty workspace, web/skill items don't trip it,
one cli item flips it on, soft-delete flips it back off, and
cross-workspace isolation.
Web:
- New <ConnectBanner> Svelte 5 component
(web/src/lib/components/ConnectBanner.svelte). Self-contained:
reads dismissed state from localStorage, fetches has_cli_source
itself, mounts <ConnectWorkspaceModal> internally. Two split
$effect blocks per CONVE-606 — one for the localStorage sync, one
for the dashboard fetch — so a workspace change doesn't entangle
the two reactive lifecycles.
- Banner is hidden while loading (hasCliSource === null) to avoid a
flash-then-auto-hide on workspaces that already have CLI items.
- Storage key pad-cli-banner-dismissed-${wsSlug} matches the existing
onboarding-dismissed pattern. Per-browser only; TODO comment in
source about backing it with a workspace_user_state row if cross-
device persistence is wanted later.
- Mounted in web/src/routes/[username]/[workspace]/+layout.svelte
above {@render children()} so it appears on every workspace page
(dashboard, collection lists, item detail, search, activity, etc.)
and NOT on console/auth pages (the layout is workspace-scoped).
- DashboardResponse type in web/src/lib/types/index.ts gains
has_cli_source: boolean.
Smoke-tested against the running server: the field is live in the
dashboard payload and reflects reality (this workspace returns
has_cli_source: true since it has many CLI-sourced items, so the
banner is correctly auto-hidden here).
Test plan:
- go build ./... && go test ./... — all green (incl. new
TestWorkspaceHasCLISource with 5 sub-cases).
- cd web && npm run build — clean.
- make install — clean, server restarted.
- Svelte MCP autofixer ran on ConnectBanner.svelte — no issues.
Parent: PLAN-859. Driving idea: IDEA-750.
* fix(web/connect-banner): stale-response guard + refetch on modal close (Codex round 1)
Two findings from Codex review on PR #284:
1. Stale-response race: rapid workspace switches could let a slow
dashboard fetch from workspace A overwrite hasCliSource for
workspace B after the user navigated. Capture the requested slug
at fetch time, ignore the response if wsSlug has changed since.
2. Auto-hide didn't work in-session: if a user opened the banner
modal, copied the command, ran it elsewhere, and closed the modal,
the banner stayed visible because hasCliSource was stale. Refetch
when the modal transitions from open → closed (the natural moment
the user has just connected). Uses $effect.pre with a tracked
previous value, matching the transition pattern in ShareDialog.
The 'someone ran the CLI from another terminal without ever opening
the modal' edge case is left for a follow-up — would require SSE
item-created subscription, which is heavier than this PR's scope.
* fix(server/items): persist source from auth context on create (Codex round 2)
Codex caught an architectural bug while reviewing the TASK-862 banner
work: items created via the CLI were persisting with source='web'
(the column default) instead of 'cli', because handleCreateItem decoded
ItemCreate from the body — which the CLI doesn't set Source on — and
only consulted actorFromRequest AFTER persisting (for SSE / activity
log emission). Result: TASK-862's has_cli_source dashboard signal
would never flip on for normal CLI usage, so the connect-CLI banner
would never auto-hide for users who actually wired up the CLI.
Fix: in handleCreateItem, backfill input.Source from actorFromRequest
before calling store.CreateItem, but only when the client didn't
explicitly set it (so agents marking themselves as 'skill' still
pass through unchanged).
Test: TestCreateItemSourcePersistedFromAuth covers all three branches
- bearer Authorization header → source=cli (uses bootstrap + a real
session token in the header since the auth middleware validates
token format and rejects fake values with 401 before the handler
runs)
- cookie session, no Authorization → source=web
- explicit source in body wins over auth-derived (e.g. 'skill')
* fix(web/connect-banner): seq counter for same-workspace race (Codex round 3)
Round 3 caught a same-workspace race the slug guard didn't cover: an
in-flight workspace-change fetch that resolves AFTER the modal-close
refetch could overwrite the newer 'true' with the older 'false',
making the banner reappear after the user actually wired up the CLI.
Add a monotonic fetchSeq counter — captured at call time, rechecked
before applying the response. Only the LATEST request's result wins,
regardless of arrival order. The slug guard stays as a second-layer
defense for cross-workspace races.
* fix(web/connect-banner): guard banner keydown to currentTarget (Codex round 4)
Round 4 caught a keyboard-event bubble: pressing Enter or Space on
the dismiss X button also fired the banner-level keydown handler,
so the user would dismiss AND open the modal in one stroke.
Guard the parent handler with `e.target !== e.currentTarget` so it
only reacts to keydown that originated on the banner itself. Tabbing
to the dismiss button + Enter now ONLY dismisses.
* fix(store): visibility-filter has_cli_source query (Codex round 5)
Round 5 caught a P2 information leak: WorkspaceHasCLISource scanned
the entire workspace regardless of caller visibility, so a guest
with grants only on web-sourced items could still see has_cli_source
return true (revealing that CLI items exist somewhere they can't see).
That also produced wrong UX — the banner could auto-hide for guests
who couldn't actually use the CLI.
Extend the query to take optional collectionIDs/itemIDs filters
matching the dashboard's existing visibility model: an item counts
when its collection is in collectionIDs OR its id is in itemIDs
(union — guest item-level grants can expose items in otherwise-
hidden collections). Mirrors ListItems' filtering pattern incl. the
"non-nil empty CollectionIDs = no visibility = short-circuit false"
semantics.
Handler now passes dashCollIDs and dashItemIDs to match the rest of
the dashboard payload's filtering. New TestWorkspaceHasCLISourceVisibility
covers the four cases: unfiltered sees all, visible-coll-only hides
CLI items in hidden collections, item-level grant surfaces a hidden
CLI item, and empty visibility short-circuits to false.
|
||
|
xarmian
|
53b5add4e9 |
fix(store): bump SQLite busy_timeout from 5s to 30s (BUG-853) (#277)
TestSQLiteConcurrentWritersNoBusy intermittently fails on the GitHub- hosted Go (SQLite) CI job with `database is locked (5) (SQLITE_BUSY)` under 25 concurrent writers × 5 ops. The test asserts ZERO errors so that BUG-748's `_txlock=immediate` regression stays pinned — but with the DSN's busy_timeout at 5s, the unluckiest writer on a slow shared runner can exceed the timeout: 125 serialized inserts under heavy contention from sibling test packages add up. Bumping busy_timeout to 30s gives a 6× margin over the worst observed CI run and ~50× the normal local p95. Genuine deadlocks don't happen with WAL + BEGIN IMMEDIATE, so the only thing the higher value costs is "how long we wait before declaring lock contention pathological". For Pad's workload, 30s is fine. Surfaced once BUG-851 (PR #276) cleared the rate-limiter goroutine leak that had been masking everything else on main. Verified locally: $ go test -count=20 -run TestSQLiteConcurrentWritersNoBusy \ ./internal/store/ ok github.com/PerpetualSoftware/pad/internal/store 6.102s Note: this is a single-character DSN change plus a doc-comment update; no code path or contract is altered. Read concurrency (WAL) is unchanged — we're not touching SetMaxOpenConns. |
||
|
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
|
afe721d202 |
feat(cli): add Cloud mode to pad init, drop Docker option (TASK-837, TASK-838) (#272)
Merging despite Go (PostgreSQL) red — those failures (TestListItems_FTS_HyphenatedSearchTerm/task-five + TestAdminBillingStats_SidecarSidecarError_DegradesToLocalOnly TempDir cleanup race) are pre-existing on main and tracked in BUG-842. Codex reviewed in 3 rounds (round 1 clean → round 2 found a real semantic bug → fix → round 3 clean). Tests, vet, and lint all green; remaining check failures are documented pre-existing. |
||
|
xarmian
|
189b22825e |
fix(cli): use cfg.BaseURL()/BrowserURL() in pad init success message + pad open (TASK-834) (#269)
* fix(cli): use cfg.BaseURL() in pad init success message (TASK-834) The "Or open the web UI at http://localhost:7777" line in printOnboardingHints was hardcoded, which is wrong for any non-local connection mode (Remote, Docker, eventual Cloud). The CLI already knows the configured base URL — it just used it to talk to the server. Same hardcoded URL existed in the workspace-onboard skip path ("You can activate conventions from the library: ..."). Both call sites now use cfg.BaseURL(), which yields the correct URL for every mode: - Local: http://127.0.0.1:7777 (default host:port) - Remote/Docker/Cloud: the configured URL (e.g. https://app.getpad.dev) printOnboardingHints now takes a *config.Config; both call sites already had cfg in scope. Parent: PLAN-833 (pad init UX gaps + Pad Cloud onboarding fixes). Source: IDEA-831 issue #5. * fix(config): add BrowserURL() that normalizes 0.0.0.0 to 127.0.0.1 Per Codex review (round 1): when local mode runs with --host 0.0.0.0 (bind-all), cfg.BaseURL() returned "http://0.0.0.0:7777" — a bind address that browsers don't reliably accept. BrowserURL() behaves like BaseURL() except that when constructing from host:port, an unspecified bind-all host (empty, "0.0.0.0", "::", "[::]") is rewritten to "127.0.0.1". Explicit URL configurations (Remote/Docker/Cloud) are returned unchanged. The two onboarding-hint call sites updated in the previous commit now use BrowserURL() so the success message and skip-path show a clickable URL in every supported configuration. Tests cover loopback, named hosts, empty/0.0.0.0/::/[::] normalization, and explicit-URL precedence. Parent: PLAN-833. * fix(cli): use BrowserURL() in pad open for bind-all safety Per Codex review (round 2): the 'pad open' command prints and opens cfg.BaseURL(), which produces 'http://0.0.0.0:7777' when the local server is bound bind-all. Same class of bug as the onboarding hint fix in this PR — switch to cfg.BrowserURL() so the URL is a usable browser destination. A second related issue Codex flagged — the server-issued CLI auth URL in doBrowserLogin (which goes through internal/server/handlers_cli_auth.go using r.Host) — is a different surface with multiple possible fix strategies and overlaps with the post-v0.1.0 OAuth-architecture work. Deferred to TASK-839 with a written-up runbook so it isn't lost. Parent: PLAN-833. |
||
|
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
|
c4b5a36330 |
feat: warn at startup when shipped FTS triggers are missing (TASK-824) (#265)
* feat(store): warn at startup when shipped FTS triggers are missing (TASK-824) Defensive follow-up to BUG-822, where the documents_* triggers had silently drifted off some production DBs and search was broken until a user noticed. The migration runner had no notion that the triggers should exist — the only invariant was "this migration ran without erroring," which is too weak when SQLite's table-rebuild path can leave auxiliary objects in a different state than the migration intended. Add a hardcoded list of expected FTS5 triggers (one row per trigger, naming the table it's attached to) and a one-shot validateFTSInvariants step at the end of Store.migrate(). Each missing trigger emits a structured slog.Warn that points the operator at the recovery migration (046). Choices: - SQLite-only. Postgres uses tsvector update functions in pgmigrations with a different invariant model. - Logging-only, no auto-repair. Auto-creating triggers here would mask legitimate future removals and obscure the source of truth (the migrations directory). The recovery path is a targeted migration like 046_restore_documents_fts_triggers.sql. - Non-fatal. A missing trigger doesn't block startup; the operator may have intentionally removed one and just not updated the list yet, and we'd rather warn loudly than refuse to boot. Tests: - TestStartupInvariants_AllFTSTriggersExist — fresh DB has all 9 expected triggers (forward-looking guard against future migrations that break one). - TestStartupInvariants_LogsOnMissingTrigger — drop a trigger, run validator, capture slog records, assert a warning naming the missing trigger was emitted. Manual verification on the production DB: - Clean DB (after migration 046): no warnings on startup. - After manually `DROP TRIGGER documents_ai`: server logs `level=WARN msg="FTS trigger missing — ..." trigger=documents_ai table=documents` immediately on startup. * test(store): address Codex review on TASK-824 — bidirectional drift + Record.Clone Two LOW findings from Codex's first pass: 1. recordCapturingHandler.Handle stored slog.Record values without cloning. Records have internal shared state; the documented pattern for retaining them is r.Clone() first. Test passed today only because nothing mutated the record after Handle, but the helper was relying on slog internals. 2. TestStartupInvariants_AllFTSTriggersExist only proved every entry in expectedFTSTriggers exists. It didn't catch the inverse: a future migration adding a new FTS-style trigger on items/comments/documents without also adding it to expectedFTSTriggers, leaving the new trigger off the invariant check forever. Add TestExpectedFTSTriggers_MatchesActual which queries sqlite_master for every trigger on items/comments/documents and asserts each is in the expected list. A new trigger that isn't tracked fails this test with a clear "update the list in store.go" message. If a future trigger on these tables is legitimately not FTS-related, the test failure points the developer at this guard and they can either add it to expectedFTSTriggers or extend the exclusion. |
||
|
xarmian
|
4608108acf |
fix: restore documents_fts triggers + rebuild index (BUG-822) (#264)
* fix(store): restore documents_fts triggers + rebuild index (BUG-822)
Some production DBs ended up missing the documents_ai/au/ad triggers,
even though migration 025 (which rebuilt the documents table for the
doc_type CHECK constraint change) was recorded as applied. Items_fts
and comments_fts triggers were unaffected — issue is isolated to the
documents table-rebuild path.
Without these triggers, INSERT INTO documents never propagates rows
into documents_fts, so newly-created documents are silently invisible
to search. Plain list views still surface them, masking the regression.
Migration 046 is idempotent and safe to apply on any DB:
1. DROP TRIGGER IF EXISTS for the three documents_* triggers — round-
trips for DBs that ran 025 cleanly, recovers DBs missing the
triggers.
2. CREATE TRIGGER for all three (matching the bodies in 001/025).
3. INSERT INTO documents_fts(documents_fts) VALUES ('rebuild') to
repopulate the FTS5 internal index from the current documents
table — recovers searchability for documents created while the
triggers were missing.
Postgres path uses a separate tsvector trigger function and is not
affected (only pgmigrations are applied there; this migration lives
in the SQLite migrations directory).
Tests:
- TestMigration046_DocumentsFTSTriggersExist — assert all three
documents_* triggers exist after migrations run.
- TestCreateDocument_IsSearchableImmediately — regression test for
the failure mode: create a doc, immediately search by a unique
title-keyword, assert it's findable.
Manual verification on the production DB:
- Triggers re-appeared after `make install` (migration 046 applied).
- POST /documents with title "BUG822verify distinctive" → immediately
findable via ?q=BUG822verify (returned 1 result, the new doc).
* test(store): pin the BUG-822 recovery path with a rebuild test
Codex review on the BUG-822 fix flagged that neither existing test would
fail if the `INSERT INTO documents_fts(documents_fts) VALUES('rebuild')`
step were removed from migration 046. The trigger-existence and
post-fix-search-works tests both pass on a clean migration run, but
they don't exercise the historical-recovery half of the migration —
the part that rescues already-broken DBs whose documents were inserted
while the triggers were missing.
Add TestMigration046_RebuildRecoversUnindexedDocs which:
1. Drops the documents_* triggers to simulate the broken state.
2. Inserts a document via the store path — won't reach FTS without
triggers.
3. Asserts the doc is invisible to ListDocuments (sanity-pinning the
broken state).
4. Runs just the rebuild step from migration 046.
5. Asserts the previously-unindexed doc is now searchable.
This locks in the recovery contract: removing the rebuild step from
046 will now make this test fail.
|
||
|
xarmian
|
dcf7c1d58e |
fix(store): apply Tag and Pinned filters in ListDocuments FTS branch (BUG-820) (#263)
The non-FTS path in ListDocuments applies Tag and Pinned filters (lines 31-42), but when params.Query is non-empty the FTS branch rebuilds query and args from scratch and only re-applies Type and Status — Tag and Pinned were silently dropped. Result: `/documents?q=foo&tag=urgent` returned all docs matching foo regardless of tag, similarly for pinned. Documents-side analog of BUG-812 (which fixed the equivalent issue on the items FTS path). Fix: mirror the Tag (s.dialect.JSONArrayContains on d.tags) and Pinned (d.pinned = TRUE/FALSE) filter blocks into the FTS branch after the existing Type/Status blocks. Backend-only — handlers and DocumentListParams already plumb both params through. Tests: - TestListDocuments_FTS_TagFilter — two docs match the search; only one has the tag; assert exactly the tagged one returned. - TestListDocuments_FTS_PinnedFilter — covers both pinned=true and pinned=false branches, asserting each narrows correctly. Manual verification: with two docs `BUG820scratch alpha` (tagged "urgent", pinned) and `BUG820scratch beta` (untagged, unpinned): - ?q=BUG820scratch → 2 docs - ?q=BUG820scratch&tag=urgent → 1 doc (alpha) - ?q=BUG820scratch&pinned=true → 1 doc (alpha) - ?q=BUG820scratch&pinned=false → 1 doc (beta) |
||
|
xarmian
|
068c208824 |
fix: sanitize SQLite FTS5 queries + whitespace guards (BUG-818) (#261)
* fix(store): sanitize FTS5 queries in listItemsFTS and SearchItems (BUG-818)
The sanitizeFTSQuery helper in internal/store/search.go wraps each
whitespace-delimited token in double quotes so SQLite FTS5 treats
specials (hyphens, AND/OR/NOT, parens) as literal characters rather
than boolean operators. Store.Search already used it; Store.listItemsFTS
and Store.SearchItems didn't, so any hyphen in `?search=` returned
HTTP 500 with "no such column: <suffix>" — including issue refs like
TASK-5, kebab-case slugs, dates, etc.
Apply sanitizeFTSQuery at the SQLite arg-binding sites in both unfixed
functions. Postgres branches stay unsanitized: plainto_tsquery accepts
arbitrary input safely (matches the existing pattern in Store.Search).
Tests:
- TestListItems_FTS_HyphenatedSearchTerm — exercises the listItems path
on multiple hyphenated queries via a table-driven sub-test.
- TestSearchItems_HyphenatedQuery — same regression on the SearchItems
path used by /api/v1/search.
- TestSanitizeFTSQuery — direct unit test covering empty, whitespace-
only, plain word, hyphenated phrase, multi-token, FTS5 boolean
operators (AND/OR/NOT), parens, embedded quotes (stripped),
surrounding whitespace, and unicode.
Manual verification: previously-500 queries now return 200 with results:
/items?search=match-me → HTTP 200, 2 items
/items?search=TASK-5 → HTTP 200, 8 items
/items?search=pad-cloud → HTTP 200, 103 items
* fix(store): address Codex review on PR for BUG-818
Codex review caught two extensions to the original BUG-818 fix:
1. MEDIUM — Store.ListDocuments (internal/store/documents.go) had the
same FTS5 boolean-parser vulnerability as Store.listItemsFTS and
Store.SearchItems before the original commit. Hyphenated /documents?q=
queries (e.g. ?q=release-notes-q2) returned HTTP 500 with "no such
column" the same way. Apply sanitizeFTSQuery in the SQLite branch;
leave Postgres unchanged.
2. LOW — Whitespace-only queries collapse to empty after FTS sanitization,
and SQLite FTS5 errors on `MATCH ''` with "syntax error near \"\"".
Add TrimSpace guards at the routing/entry points:
- listItems: route to FTS only if TrimSpace(Search) != ""
- SearchItems: short-circuit to empty results
- ListDocuments: same routing guard
- Store.Search: short-circuit to empty results
Tests:
- TestListDocuments_HyphenatedQuery — regression on the documents FTS path
- TestFTS_WhitespaceOnlyQuery_DoesNotCrash — covers all 3 entry points
(ListItems, SearchItems, ListDocuments) for spaces, tabs, mixed
whitespace
Manual verification (all 6 endpoints now HTTP 200):
- /workspaces/{ws}/items?search=task-five
- /workspaces/{ws}/items?search=<3 spaces>
- /workspaces/{ws}/documents?q=release-notes
- /workspaces/{ws}/documents?q=<3 spaces>
- /search?q=task-5
- /search?q=<3 spaces>
|
||
|
xarmian
|
10e17e0ca1 |
fix(store): apply Tag/ParentID/Assignee/AgentRole/Fields filters in listItemsFTS (BUG-812) (#260)
When `search` is set, ListItems routes through listItemsFTS, which historically only re-applied CollectionSlug, CollectionIDs, ItemIDs, and (post-BUG-734) ParentLinkID. Other filter parameters silently dropped: - Tag - ParentID (legacy items.parent_id column) - AssignedUserID - AgentRoleID (both ID-equality and slug-OR branches) - Fields (custom-field equality / IN-list) Result: combining ?search=foo with any of the above returned more items than the caller asked for. Web UI list filters chained with the search box, the per-collection filter chips, and any API consumer with the same combo were all affected. Fix: mirror the relevant filter blocks from the non-FTS listItems path into listItemsFTS, preserving isValidFieldKey injection guarding on field keys. Tests (internal/store/items_test.go): - TestListItems_FTS_TagFilter - TestListItems_FTS_ParentIDFilter - TestListItems_FTS_AssignedUserFilter - TestListItems_FTS_AgentRoleFilter (covers both role-ID and role-slug branches) - TestListItems_FTS_FieldFilter (single-value, IN-list, and the invalid-key silent-drop) Out of scope: IncludeArchived parity (FTS hardcodes deleted_at IS NULL), Sort parity (FTS deliberately sorts by relevance rank), Offset (FTS honors only Limit). Unrelated to BUG-812; can ship together later if desired. Manual verification: with two tasks `Bug812scratch alpha` (priority=high) and `Bug812scratch beta` (priority=low), `?search=Bug812scratch` returns both, `?search=Bug812scratch&priority=high` returns only alpha. |
||
|
xarmian
|
0bf710eea5 |
fix: hide item_links pointing to soft-deleted items (BUG-734) (#259)
* fix(store): hide item_links pointing to soft-deleted items (BUG-734)
Item-link queries that JOIN against `items` now also filter on
`deleted_at IS NULL` for both source and target. This prevents
`pad item related`, the lineage breadcrumb, and dashboard enrichment
from surfacing dangling endpoints when one side has been archived.
Affected queries in internal/store/items.go:
- GetItemLinks (powers `pad item related`, lineage, dashboard)
- GetItemLink (singular; fixed for consistency)
- GetParentForItem (breadcrumb / lineage; archived parent reads as none)
Other item_links queries already filtered on deleted_at; export.go
deliberately keeps all rows for backup correctness — left unchanged.
The link rows themselves are preserved on disk, so restoring a
soft-deleted item resurrects its relationships automatically.
Tests:
- TestItemLinks_HidesSoftDeletedEndpoints — delete + restore round-trip
on both source-side and target-side
- TestGetParentForItem_HidesSoftDeletedParent — parent breadcrumb path
Manually verified: PLAN + TASK with `implements` link, soft-delete the
TASK, `pad item related <PLAN>` correctly returns no implementers.
* fix(store): address Codex review findings on PR #259 (BUG-734)
Three follow-ups from Codex's review of the soft-delete filter on item-link
queries:
1. MEDIUM — GetParentMap now JOINs items on both sides and filters on
deleted_at IS NULL. handlers_dashboard.go uses this map directly to
detect orphaned tasks (items not present in the map are flagged), so
without the filter a task whose parent had been soft-deleted would
silently fail to appear as orphaned.
2. LOW — Revert the deleted_at filter on getItemLink (lowercase, private).
Its only caller is the post-insert readback in CreateItemLink, which
means filtering buys nothing user-facing and introduces a delete-race
window where a successful INSERT returns nil. SetParentLink's readback
was switched from GetItemLinks to getItemLink for the same reason.
User-facing surfaces still go through GetItemLinks (plural) and
GetParentForItem, both of which retain the filter.
3. LOW — Add an explicit comment in export.go documenting that item_links
are exported in full (including links to soft-deleted items), and why
that intentionally diverges from the user-facing query behavior.
Tests: TestGetParentMap_ExcludesSoftDeletedEndpoints exercises the
dashboard regression path on both source-side and target-side soft-delete,
plus the restore round-trip.
* fix(store): reject soft-deleted parent in ListItems UUID parent filter (BUG-734)
Codex review on
|
||
|
xarmian
|
e5e2bd7b86 |
chore: flip CI only-new-issues=false + scope lint policy (TASK-771) (#253)
* chore: gate CI on full lint, scoped to checks we enforce (TASK-771) Flip golangci-lint-action's only-new-issues from true to false so CI fails on ANY linter finding, not just findings on PR-changed lines. This catches lint regressions on the next push instead of letting them drift into main. The gate flip is paired with a deliberate scope-down of .golangci.yml: 1. errcheck is disabled. The codebase has 325 pre-existing unchecked- error sites where the error is intentionally discarded (best-effort logging writes, defensive parses with zero-valued fallbacks, etc.). Auditing every site is its own project — bigger than IDEA-732 by an order of magnitude. Tracked as a follow-up if/when we want the safety net back. 2. staticcheck is restricted to the SA* check family (real-bug detectors). The ST*/QF*/S* families are stylistic/quick-fix suggestions we don't gate CI on yet — they would have re-flooded the lint output with capitalized error strings, De Morgan's law simplification suggestions, etc., that aren't bug-finding signals. Re-enable selectively if the team wants them. After scoping, the live linters are: govet, ineffassign, staticcheck (SA*), unused, gofmt — exactly the set that IDEA-732 cleaned up. Other changes in this PR: - Drop pull-requests:read permission. It was only required by the golangci-lint-action when only-new-issues=true (the action used it to fetch PR diff metadata). Not needed any more. - Update the Run-golangci-lint comment block to explain the new policy and reference the IDEA-732 cleanup PRs (#247/#249/#251/#252). - Replace the SA4017 //lint:ignore directive in cmd/pad/main.go:4631 with an inline //nolint:staticcheck — the multi-line //lint:ignore block was too far from the if statement for staticcheck's proximity rule, so the directive wasn't taking effect. - Apply gofmt -w on three files where post-deletion blank-line artifacts had drifted (cmd/pad/main.go imports, two trailing newline fix-ups in handlers_items.go and middleware_ratelimit.go). Verified: - `golangci-lint run ./...` reports 0 issues. - `go build ./...` clean. - `go vet ./...` clean. - `go test ./...` all pass. Parent: PLAN-644. * chore: address Codex round 1 on PR #253 (TASK-771) Two LOWs from Codex on the gate-flip PR: 1. //nolint:staticcheck was broader than necessary (suppressed any future staticcheck diagnostic on the line) and didn't self-report when the underlying false positive gets fixed upstream. Codex suggested swapping back to a tightly-placed //lint:ignore SA4017. I tried that, but golangci-lint v2's staticcheck integration does not honour //lint:ignore the way direct staticcheck does — the directive was silently no-op'd via golangci-lint while the same directive worked when staticcheck was invoked directly. So instead of fighting the linter wrapper, sidestep the false positive entirely: rewrite the keepalive check from `strings.HasPrefix(line, ":")` to `len(line) > 0 && line[0] == ':'`. Same observable behaviour for a single-byte ASCII prefix, no suppression directive needed at all, no exposure when staticcheck eventually fixes the false positive. 2. The new lint-step comment in ci.yml said main is "clean of staticcheck SA*/U1000" — but U1000 is reported by the standalone `unused` linter in .golangci.yml, not by staticcheck.checks. Tighten the comment to attribute each enforced check correctly. Verified: - `golangci-lint run ./...` reports 0 issues - `go test ./cmd/pad/...` passes (the SSE watch loop is exercised by reconcile_test.go and the broader integration tests). |
||
|
xarmian
|
dd381e1066 |
chore: delete 5 unwired document handlers (TASK-769) (#252)
* chore: delete 5 unwired document handlers (TASK-769)
internal/server/handlers_documents.go had 5 dead HTTP handlers that
were drafted as Documents-v1 extensions but never wired into the
router (server.go:509 already labels Documents itself as "v1, will be
replaced by items in Phase 2"):
- handleQuickSave (POST /documents/quick-save) — title-based upsert
- handleBulkRead (POST /documents/bulk-read) — multi-doc fetch by IDs
- handleGetBacklinks (GET /documents/{id}/backlinks)
- handleGetLinks (GET /documents/{id}/links)
- handleGetContext (GET /documents/context?type=)
Investigation confirmed zero consumers:
- Not registered in setupRouter (`grep -n "QuickSave\|BulkRead\|Backlinks\|GetLinks\|GetContext" server.go` → empty).
- Not used by the SvelteKit frontend (`web/src/`).
- Not used by the CLI (`internal/cli/`).
- Pre-launch repo, no fork or downstream that could be relying on them.
Delete scope is intentionally limited to the HTTP handlers. The
underlying `Store.QuickSave / BulkRead / GetBacklinks / GetLinks /
GetContext` methods stay — they're tested at the store level
(internal/store/store_test.go) and preserve optionality if Phase 2
work needs to revive any of these features. `models.QuickSave` stays
for the same reason.
After this lands, IDEA-732's lint catalog is fully cleared on main
(staticcheck SA* + U1000 returns zero). TASK-771 (flip CI
only-new-issues=false) becomes safe.
Verified:
- `go build ./...` clean
- `go vet ./...` clean
- `go test ./...` all pass
- `staticcheck -checks "SA*,U1000" ./...` clean
- All `import "strings"` etc. still used elsewhere in file
Parent: PLAN-644.
* chore: also delete now-test-only document store helpers (TASK-769)
Codex round 1 on PR #252 flagged that the document-store helpers
retained for "Phase 2 optionality" are now exclusively kept alive by
their own store tests — Store.QuickSave, BulkRead, GetBacklinks,
GetLinks, GetContext are not called by any production code path after
the handler deletions in the previous commit. Same for the
models.QuickSave struct.
Pre-launch with no external consumers, optionality preservation has a
real cost (dead code on main, ongoing test maintenance). When Phase 2
needs any of these capabilities it is cheaper to re-derive them
against the Items model than to drag dead Documents-v1 plumbing
forward. So delete them now.
Removed:
- internal/store/documents.go: QuickSave (38 lines), BulkRead (28),
GetBacklinks (15), GetLinks (28), GetContext (41).
- internal/models/document.go: QuickSave struct.
- internal/store/store_test.go: TestQuickSave (38 lines), TestBulkRead
(16), TestDocumentLinking (29), TestContext (23).
Kept:
- TestDocumentLinkRename — exercises UpdateDocument's internal
link-rewriting path, not any of the deleted helpers.
- GetDocumentByTitle — still used by TestDocumentLinkRename.
- The full CRUD/restore handlers and their store methods — these are
still wired into setupRouter and have their own coverage.
Verified:
- `go build ./...` clean
- `go vet ./...` clean
- `go test ./...` all pass (TestDocumentLinkRename and the wider doc
CRUD/version/activity tests still cover the surviving paths).
- `staticcheck -checks "SA*,U1000" ./...` clean
- No new unused imports introduced (links package is still used by
documents.go for ReplaceTitle in UpdateDocument).
Parent: PLAN-644.
* chore: drop GetDocumentByTitle and refactor TestDocumentLinkRename (TASK-769)
Codex round 2 caught the chain — after deleting QuickSave/BulkRead/
GetBacklinks/GetLinks/GetContext, Store.GetDocumentByTitle was kept
alive by exactly one test (TestDocumentLinkRename), which was
re-fetching by title only because the test ignored the *Document
already returned by createTestDoc.
Use the createTestDoc return value instead, then drop GetDocumentByTitle
from the store. Same idea, cleaner test, one fewer test-only API on
the store. The rename behaviour (the actual thing under test) is
unchanged.
Verified:
- `go build ./...` clean
- `go test ./internal/store` and `./internal/server` pass
- `staticcheck -checks "SA*,U1000" ./...` still clean
Parent: PLAN-644.
* chore: drop now-orphaned links.Extract (TASK-769)
Codex round 3 caught the next link in the chain: after Store.GetLinks
was deleted, links.Extract had no remaining callers — links.ReplaceTitle
is the only Extract-package function still used (by UpdateDocument's
rename rewrite). The linkPattern regex was only used by Extract.
Drop linkPattern, the regexp import, and Extract itself. Leaves
ReplaceTitle and its private string helpers (replaceAll, indexOf)
intact.
The cleanup chain ends here: ReplaceTitle is still wired into a live
production path (Documents-v1 rename), and the supporting helpers
have no other roles to inherit.
Verified:
- `go build ./...` clean
- `go test ./internal/store` and `./internal/server` pass
- `staticcheck -checks "SA*,U1000" ./...` clean
Parent: PLAN-644.
|
||
|
xarmian
|
3c3251f5af |
chore: drop dead visibility-filter block in dashboard handler (TASK-765) (#251)
* chore: drop dead visibility-filter block in dashboard handler (TASK-765)
internal/server/handlers_dashboard.go had a 10-line block that built a
visibility-filtered `filtered` collections slice and reassigned it back
to `collections` — but `collections` was never read after the
reassignment, so the filtering had no effect. Staticcheck flagged it
as SA4006 (line 220) + SA4010 (line 217) — same dead block, two
findings.
Investigation confirmed this is leftover refactor scaffolding rather
than a missing-filter bug:
- The summary section (line 236) uses `allItems` from
`ListItems(workspaceID, {CollectionIDs: dashCollIDs, ItemIDs: dashItemIDs})`.
- Active plans, attention items, suggestions all use the same
`dashCollIDs`/`dashItemIDs` parameters plus inline
`isCollectionVisible(child.CollectionID, visibleIDs)` checks.
So visibility is already correctly applied to every dashboard output —
through the `dashCollIDs` / `dashItemIDs` path established earlier at
lines 185-190 — not through the deleted block. The previous comment
("drives the collection-summary section") was misleading; the summary
section reads items, not the `collections` slice.
Replace the dead block with an inline note explaining where visibility
*actually* gets applied so the next reader doesn't reach for the
filter pattern again.
Verified:
- `go build ./...` clean
- `go test ./internal/server/...` all pass (Dashboard tests cover this
path).
- `staticcheck -checks "SA4006,SA4010" ./...` clean
Parent: PLAN-644.
* docs: tighten dashboard visibility comment per Codex round 1 (TASK-765)
Codex review on PR #251 flagged that my replacement comment claimed
dashCollIDs/dashItemIDs is THE filter path for all dashboard outputs,
but for graph-walking outputs (plan progress, blocked attention,
suggested next) final visibility actually comes from a combination of
the ListItems-param filtering and per-item isCollectionVisible /
isItemVisibleToGuest checks.
Tighten the comment to call out both layers so future readers know
the canonical answer is "dashCollIDs/dashItemIDs PLUS per-item
visibility checks", not "dashCollIDs/dashItemIDs alone".
The MEDIUM finding (dashboard visibility tests don't exercise the
filter path) is a pre-existing coverage gap, not a regression from
this PR. Tracked as its own follow-up task under PLAN-644 rather than
expanding the scope of this cleanup. See the PR body for the link.
|
||
|
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
|
157ca4e88f |
chore: bump Go toolchain to 1.26 (TASK-763) (#247)
* chore: bump Go toolchain to 1.26 (TASK-763) Bump Go from 1.25 to 1.26 across all toolchain pins: - go.mod — go 1.25.0 → go 1.26.0 - Dockerfile — golang:1.25-alpine → golang:1.26-alpine - .github/workflows/ci.yml — three setup-go steps (Go, Go-Postgres, E2E jobs) - .github/workflows/release.yml — release pipeline No `toolchain` directive: the repo is pre-launch with no external contributors yet, so we set the floor where we want it (hard requirement). Verified locally before commit: - golangci-lint v2.11.4 builds and runs under Go 1.26.2 (pinned in CI) - golang:1.26-alpine and 1.26.2-alpine images present on Docker Hub - go build ./... clean - go vet ./... clean - go test ./... all pass Parent: PLAN-644 (OSS Repo Hygiene and Launch Polish). * chore: gofmt -w under Go 1.26 (TASK-763) Apply Go 1.26's gofmt to the codebase. ~41 files reformatted, all struct-tag whitespace realignment — no semantic changes. Verified: - gofmt -l ./cmd ./internal returns empty after - go build ./... still clean - go test ./... still passes (run before commit) Bundling the gofmt diff with the toolchain bump in the same PR because the formatting drift is a direct consequence of moving from 1.25 to 1.26; splitting them creates a mandatory two-PR ordering for no value. Parent: PLAN-644. * docs: bump documented Go floor to 1.26 (TASK-763) Match go.mod's hard 1.26.0 requirement in the source-build instructions. Caught by Codex review round 1 on PR #247. - README.md:158 — "Go 1.25+" → "Go 1.26+" - CONTRIBUTING.md:9 — "Go 1.25+" → "Go 1.26+" |
||
|
xarmian
|
7e56b20d0c |
fix(store): eliminate spurious SQLITE_BUSY on concurrent writes (#239)
* fix(store): eliminate spurious SQLITE_BUSY on concurrent writes
`pad item update --comment ...` (and any concurrent write workload)
intermittently failed with `internal error` and a server log line of
`update item: database is locked (5) (SQLITE_BUSY)`. The skill's CLI
reference even documented a workaround — "use a separate `pad item
comment` call rather than --comment on update" — but that just lowered
the contention probability; both call paths hit the same root cause.
Root cause
Go's default `db.Begin()` issues `BEGIN DEFERRED` on SQLite, which
takes only a SHARED lock at BEGIN time. The first INSERT/UPDATE in
the transaction tries to upgrade to a write lock — and SQLite refuses
that upgrade with SQLITE_BUSY *immediately* if any other connection
already holds the write lock. busy_timeout's wait-and-retry behavior
does NOT apply on lock-upgrade because waiting would risk deadlock
between two connections both holding SHARED locks. Net effect: under
even modest write concurrency, transactions fail in milliseconds
instead of waiting out the 5-second busy_timeout we configured.
Repro before the fix: 20 concurrent CreateItem calls produced ~4
SQLITE_BUSY errors. Under the running server, two PATCHes within a
few ms of each other (e.g. status update + activity-log write) hit
this regularly during workflow tooling like /ship-tasks.
Fix
Set `_txlock=immediate` in the DSN. Every `db.Begin()` now issues
`BEGIN IMMEDIATE`, acquiring the write lock up-front. Lock-acquisition
DOES honor busy_timeout, so concurrent writers wait up to 5 seconds
to serialize cleanly instead of failing fast. Reads are unaffected:
single-statement SELECTs don't open a transaction at the SQL layer.
Also fold `foreign_keys=on` into the DSN's `_pragma` list. FK
enforcement is per-connection in SQLite, so the previous
`db.Exec("PRAGMA foreign_keys=ON")` only configured the one
connection that received the call — every OTHER pool member ran
without FK enforcement. The DSN form applies it to every connection
the driver opens.
`journal_mode=WAL` stays as a `db.Exec` call because WAL is a
database-level setting recorded in the file header; it persists
across connections after the first one applies it.
Validation
- Reproduced the failure under the live binary: 20 concurrent PATCHes
in a tight loop produced 4 SQLITE_BUSY errors. After this change,
same workload: 0 errors.
- New regression test `TestSQLiteConcurrentWritersNoBusy` does 20
concurrent CreateItem calls and asserts zero errors. Skipped under
PAD_TEST_POSTGRES_URL (postgres has different concurrency model).
- Existing `TestConcurrentWritePerformance` benchmark now reports 0
errors at every concurrency level it tests (1, 5, 10, 25, 50
workers). Previously this benchmark was acknowledging non-zero
errors at high concurrency as expected.
- Full test suite green: go test ./... — all 14 packages pass.
* fix(store): document IMMEDIATE tradeoff + tighten regression test (Codex round 1)
Address all three findings from Codex review of #239:
MEDIUM — IMMEDIATE widens the writer critical section because update
flows now hold the write lock during diff/version-throttle reads and
slug-collision checks, not just the final UPDATE. Document this
tradeoff explicitly in the DSN comment block: the pre-fix behaviour
was "fail fast with BUSY" and the post-fix behaviour is "wait briefly
for cleanly serialized work" — strictly better. If a future hot path
produces pathologically long write transactions (>100ms holding the
lock), the right move is to narrow that specific transaction, not to
revert this fix.
LOW — Foreign-key enforcement was previously per-connection, applied
to only one pool member. Latent integrity violations in databases
written through other pool members (with FKs disabled) may now
surface as errors on the next write. Document the rollout note and
mention `PRAGMA foreign_key_check` as the diagnostic.
LOW — Tighten TestSQLiteConcurrentWritersNoBusy: the original 20×1
form gave goroutines no synchronization, so a slow CI runner could
sequentialize the work and let a regression slip through. New form
uses an explicit start gate (sync.WaitGroup acting as a barrier) so
all goroutines try to write at the same moment, plus 25 workers ×
5 ops each (125 total) so each goroutine produces several BEGIN/
COMMIT cycles. Still passes; significantly less prone to false
negatives on CI.
No code-path change beyond test tightening; the comment additions
are docstring-only.
* fix(store): use true barrier in concurrency test (Codex round 2)
Codex round 2 caught that the previous start-gate pattern wasn't a
real barrier:
var startGate sync.WaitGroup
startGate.Add(1)
for ... { go func() { startGate.Wait(); ... }() }
startGate.Done() // <-- fires before all goroutines reach Wait()
`startGate.Done()` runs as soon as the launch loop finishes, with no
guarantee the scheduler has actually run the goroutines yet. Late-
scheduled goroutines reach `startGate.Wait()` after Done() has already
fired and proceed without ever parking — so on a slow CI runner with
goroutine startup spread across tens of milliseconds, the contention
window we wanted to create simply doesn't exist, and a regressed
deferred-transaction build could quietly pass.
Switch to the standard two-WaitGroup barrier: every worker signals
"ready" via `ready.Done()` and parks on `release.Wait()`, the main
thread `ready.Wait()`s for all workers to confirm they're parked,
then `release.Done()`s to fire them all simultaneously. This
guarantees every goroutine reaches BEGIN IMMEDIATE inside the same
narrow contention window regardless of scheduler latency.
Confirmed: `go test -count=20 -run TestSQLiteConcurrentWritersNoBusy
./internal/store` — all 20 invocations green.
* docs(store): be honest about barrier imprecision + add empirical proof (Codex round 2)
Codex round 2 noted the two-WaitGroup pattern still has a small
unobservable gap between ready.Done() and release.Wait() in each
worker. That's technically correct — the barrier isn't mathematically
exact, and a worker descheduled in that gap could miss the simultaneous
release. The previous comment overstated the guarantee by calling it
a "TRUE barrier".
Soften the comment to acknowledge the gap honestly, AND back the test
with empirical proof: with `_txlock=immediate` removed from the DSN
this test reliably FAILS (22/125 errors per run, all SQLITE_BUSY).
With the fix in place, 20 consecutive `go test -count=20` invocations
all pass. So the small theoretical imprecision in the barrier doesn't
impair the test's regression-catching ability — the multiple-ops-per-
worker structure means even slightly-late workers still produce enough
concurrent BEGIN IMMEDIATE attempts to exercise the race.
Documentation-only commit. No code change.
* docs(store): comment-consistency cleanups (Codex round 3)
Two LOW findings, both pure doc:
1. Inline comment on `ready.Wait()` was still asserting "every worker
is parked on release.Wait()", contradicting the softened block
comment above. Change to "every worker has called ready.Done()
(best-effort gate)".
2. Block comment hardcoded "22 errors out of 125 ops per run" as
though it were a standing expectation. The exact rate is host-
and scheduler-dependent; reword as a representative observation
("a representative run on a developer laptop produced ~22 errors
...; the exact rate is host- and scheduler-dependent but
consistently >0").
No code change.
|
||
|
xarmian
|
b165e5fe7a |
fix(server): summarise structured field changes in activity feed (BUG-748) (#236)
* fix(server): summarise structured field changes in activity feed (BUG-748)
The activity-feed `metadata.changes` string is built by `diffFields()` in
`handlers_documents.go`, which used `fmt.Sprintf("%v", val)` to stringify
each old/new value. For structured fields (implementation_notes,
decision_log, or any other slice/map value in item.fields) Go's default
formatting dumps the raw map repr — e.g.
implementation_notes: → [map[created_at:2026-04-23T... details:Code audit
on 2026-04-23 found Phases 1, 2, and most of Phase 3 already implemented:
- **Phase 1a** ... created_by:user summary:Phases 1-3 verified shipped]]
Activity cards on the item detail page surfaced this verbatim, leaking
internal field shape into the UI.
Replace the bare `%v` with a `formatChangeValue` helper:
- Primitives (string/number/bool): unchanged Go default formatting.
- Slices: counted summary. Known fields get domain-specific phrasing
(`(1 note)` / `(N notes)` for implementation_notes, `(1 entry)` /
`(N entries)` for decision_log); unknown fields fall back to
`(N items)`.
- Maps/objects: `(object)` placeholder.
- nil: empty string.
This is a backend-only change. The frontend `TimelineActivityCard.svelte`
keeps splitting on `→` exactly as before, so the contract is unchanged
beyond the value-formatting.
Companion to PR #235 (frontend `.prose` class fix on TimelineCommentCard).
Together they close BUG-748 — markdown content was unrenderable both
in plain timeline comments AND in activity-feed change pills that
referenced structured field updates.
Tests: 9 new cases in handlers_documents_test.go covering primitives,
added/removed fields, implementation_notes single + plural, decision_log
single + plural, generic slice fallback, object fallback, invalid JSON,
and nil safety. All green.
Verified: go build ./..., go vet ./..., go test ./..., web/npm run build
all clean.
* fix(server): compare values, not display strings, in diffFields (Codex round 1)
Codex flagged two MEDIUM regressions in PR #236 round 1:
1. Object-valued fields (e.g. `convention`, `github_pr`) all stringify to
the same `(object)` label, so an in-place edit produced
oldStr == newStr == "(object)" and `diffFields()` silently dropped the
change from `metadata.changes` — the activity card stopped recording
that the field had been edited.
2. Same problem for slice fields when length is unchanged: replacing one
`implementation_note` with a different one (`{"summary":"original"}`
→ `{"summary":"revised"}`) collapsed both sides to `(1 note)` and
the change vanished from the activity log.
Switch the equality check from string-on-display to `reflect.DeepEqual`
on the raw decoded values. The display strings still go through
`formatChangeValue()` so the activity card stays clean (`(1 note) → (1
note)` for a same-cardinality replacement is coarse but correct — the
user knows something changed and can drill in via the timeline). For
truly identical values the entry is omitted, so no false positives.
`reflect.DeepEqual` is correct for the types `json.Unmarshal` into
`map[string]any` produces: nil, bool, float64, string, []any,
map[string]any.
New tests:
- TestDiffFieldsSameCardinalityArrayChangeStillReported
- TestDiffFieldsObjectMutationStillReported
Each also asserts the no-op case (identical input on both sides emits
nothing).
Verified: go build ./..., go vet ./..., go test -count=1 ./... all green.
|
||
|
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
|
69262c3b53 |
feat(server): periodically revalidate SSE subscriber membership (TASK-670) (#194)
* feat(server): periodically revalidate SSE subscriber membership (TASK-670)
handleSSE checked workspace access only at connection time. A removed
member kept receiving live events until they manually disconnected —
or, more commonly, indefinitely, because browser EventSource auto-
reconnects and the replay buffer filled any gaps. An owner who revoked
access had no way to stop the leak without restarting the server.
- New 60s membership revalidation ticker inside the SSE select loop.
- Store.sseSubscriberStillHasAccess mirrors RequireWorkspaceAccess's
access matrix: fresh install bypass, admin role, direct membership,
guest grants, legacy workspace-scoped API token. DB errors fail
OPEN (keep connection) so a transient blip doesn't bounce every
open tab; membership-absent fails CLOSED.
- On revocation we send the client a well-known {type:"unauthorized"}
event with a human-readable reason BEFORE closing the stream, so
frontend EventSource handlers can route to login / dismiss the
workspace instead of tight-looping to reconnect.
- sseMembershipRevalInterval is a package-level var so tests can
shrink it; pinned to the 30-300s reasonable range.
- Unit test exercises every branch: admin, active member, outsider,
removed member, guest-grant (skipped when default collections aren't
seeded), unauthenticated, legacy token scoped to same workspace, and
legacy token scoped to a different workspace.
Parent: PLAN-643 (OSS Security Hardening).
* fix(web): handle server-emitted 'unauthorized' SSE event in client (TASK-670)
Addresses Codex P2 on PR #194: the server emits `{type:"unauthorized"}`
before closing a revoked stream, but the Svelte SSE service only listened
for "connected", "sync_required", and item events. Without a handler,
the default `EventSource.onerror` would auto-reconnect indefinitely on
the next /api/v1/events request — exactly the tight-loop the server
event was meant to prevent.
- New 'unauthorized' SSEStatus so surrounding UI can react (e.g.
redirect to workspace list or show a "revoked" toast).
- Dedicated listener: on unauthorized, set status to 'unauthorized',
close the EventSource explicitly (this prevents browser auto-
reconnect), and null out currentWorkspace so a later connect()
doesn't treat the closed connection as "already connected".
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): recompute SSE visibility on each revalidation tick (TASK-670)
Addresses Codex P1 on PR #194: previous revision only rebuilt the
filter maps at connect time, so a user whose scope was NARROWED
mid-stream (role downgraded to viewer, collection access tightened
to "specific", item grants revoked) kept receiving events from
collections they no longer had access to. Revocation-of-membership
was caught, but scope-tightening was not.
- Extract the filter-map computation into a new sseVisibility struct
+ (*Server).computeSSEVisibility method. Same logic as before,
just reentrant so it can be re-run on a live connection.
- Store the snapshot in a local `vis` variable captured by the
sseEventVisible closure (reads the CURRENT snapshot, so the next
event dispatched after a tick sees the new permissions).
- On every revalidation tick where the subscriber still has access,
call computeSSEVisibility again and reassign `vis`. The cost is
one GetCollection + one GuestVisibleResources + friends per tick
per connection — acceptable at the 60s cadence.
- New TestComputeSSEVisibility_ReflectsCurrentGrants verifies that
a second call after membership revocation returns a different
snapshot (isGuest flip), pinning the regression.
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): jitter first SSE revalidation tick to avoid stampedes (TASK-670)
Addresses Codex P2 on PR #194: the earlier comment promised jitter but
the implementation wired a plain time.NewTicker(revalInterval). Every
stream then revalidated on a cadence tied to its connect time, which
synchronizes whenever a wave of clients connects close together (post-
deploy reconnect storm, login wave, cron-driven dashboard refresh).
The resulting periodic :00/:60 DB load spike is the exact anti-pattern
the comment warned about.
- Swap the Ticker for a Timer. First fire is delayed by a random
uniform [0, revalInterval) window using math/rand so connect-time
coincidence doesn't translate to revalidation-time coincidence.
- After the first fire, Timer.Reset(revalInterval) re-arms at the
regular cadence — the jitter from connect-time is persistent for
the lifetime of the connection, no need to re-jitter every tick.
- math/rand is fine here: this is load-spreading, not a security
primitive, so a deterministic-at-boot PRNG is acceptable.
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): re-fetch user during SSE revalidation to catch admin demotion (TASK-670)
Addresses Codex P1 on PR #194: sseSubscriberStillHasAccess early-
returned on currentUser(r).Role == "admin", but currentUser(r) is the
user snapshot cached in request context at SSE connect time. An admin
demoted mid-stream via /api/v1/admin/users/{userID} would keep the
admin short-circuit forever — the exact "admin forever" bug the
revalidation loop was meant to close.
- Re-fetch the user via s.store.GetUser(cachedUser.ID) at the start of
each revalidation pass so role changes, disabled flags, and account
deletions take effect on the next tick.
- User deleted → revoke.
- User disabled (IsDisabled) → revoke. Previously a disabled admin's
stream also leaked.
- All downstream checks (admin short-circuit, membership lookup, grant
check) use the fresh copy.
Tests:
- TestSSESubscriberStillHasAccess_AdminDemotion: bootstrap admin, hand
it to the request context, then demote to "member" in the DB and
verify access flips to false. Without the fresh fetch, this test
passes even though the real system leaks — pins the regression.
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): use fresh user for SSE visibility computation too (TASK-670)
Addresses Codex P1 on PR #194 (companion to the previous commit):
computeSSEVisibility called visibleCollectionIDs(r, ...), which reads
currentUser(r).Role — the cached snapshot placed in request context
when the SSE connection opened. A global admin demoted to "member"
mid-stream while keeping workspace membership would keep the admin
short-circuit forever — visibleCollectionIDs returned nil (all access)
based on the stale Role="admin", so events from collections outside
the user's new collection_access="specific" scope would keep flowing.
- computeSSEVisibility now re-fetches the user via s.store.GetUser
before computing visibility. Transient DB errors fall back to the
cached snapshot so a blip doesn't accidentally widen visibility.
- The admin short-circuit (visibleIDs nil) now comes from the fresh
user.Role, so demotion immediately trips the "no, actually filter"
path on the next revalidation tick.
Tests:
- TestComputeSSEVisibility_DemotedAdminGetsFilter: set up a global
admin who is a workspace member with collection_access="specific"
and NO granted collections. Before demotion the admin gets nil
(unrestricted). Demote to "member" → the snapshot must flip to a
non-nil visibleSlugSet (system collections only). The cached-role
bug would keep returning nil here.
Parent: PLAN-643 (OSS Security Hardening).
|
||
|
xarmian
|
a86cfb7cff |
feat(server): zxcvbn password strength check at registration / rotation / reset (TASK-669) (#193)
* feat(server): zxcvbn password strength check at registration / rotation / reset (TASK-669)
Previously all three entrypoints (bootstrap, register, password change,
password reset) only enforced 8 <= len <= 128. Top-of-breach-list
entries like "password", "password123", "qwerty1234", and "letmein1"
all passed that filter and could silently end up hashed into a real
account.
- New validatePasswordStrength helper wraps github.com/trustelem/zxcvbn
with:
* length guardrails (8-128) kept as cheap early exits
* user-input context (email, name) passed into the scorer so
Alice+"Alice2026" gets penalized as email-derived
* minimum score 2 (OWASP-recommended floor, "adequate for online
attack scenarios")
* empty context strings filtered — zxcvbn treats "" as a banned
substring which would incorrectly weaken every password
- Wired into all four validation points in handlers_auth.go:
bootstrap, register, PATCH /auth/me (password change), reset-password.
- Test suite uses a strong canonical password now
("correct-horse-battery-staple") so bootstrapFirstUser + login flows
don't fight the new check.
- Password_strength_test.go covers: length extremes, the RockYou
top-100 (password, 123456, qwerty, iloveyou, letmein1, …),
email-derived + name-derived patterns, and three acceptable
passphrases.
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): use pending name/username as strength-check context in PATCH /auth/me (TASK-669)
Addresses Codex P2 on PR #193: a PATCH that changed BOTH name and
password used the OLD user.Name as the zxcvbn user-input context, so
a caller could rename themselves to Zaphod + set password "zaphodzaphod"
in one request and slip the identity-derived penalty.
- When input.Name/input.Username are set in the PATCH, use those
pending values (not user.Name / user.Username) as the context for
validatePasswordStrength. Email stays as user.Email — email change
has its own flow and confirmation, not inline here.
- TestPasswordChange_RejectsPasswordDerivedFromPendingName pins the
fix with an integration-level regression test.
- TestValidatePasswordStrength_ContextPenalizesDerivedPasswords pins
the underlying unit behavior (context string actually tips the
score) so a future library swap can't silently regress.
Parent: PLAN-643 (OSS Security Hardening).
* fix(server): identity-aware reset strength check + username context on registration (TASK-669)
Addresses two Codex comments on PR #193:
P2 — reset handler ran a context-less strength check because
ConsumePasswordReset was atomic and gave us the user only after the
token was burned. That made /auth/reset-password enforce a weaker
policy than bootstrap/register/rotation and opened an identity-derived-
password bypass on the primary recovery endpoint.
- New Store.LookupPasswordReset is a read-only validation that returns
the user without consuming the token. handleResetPassword now does
two-phase: lookup → strength-check with full context (email, name,
username) → consume. On strength rejection the token is NOT burned
so the user can try again on the same reset link instead of having
to request another email.
P3 — registration strength check only passed email and name, not the
caller-supplied username. Identity-derived passwords keyed on the
username alone slipped past the zxcvbn user-input penalty.
- Added input.Username as the fourth context arg to
validatePasswordStrength in /auth/register.
Tests:
- TestPasswordReset_UsesIdentityContext: weak identity-derived password
rejected; same token then accepts a strong one (token preserved).
- TestRegister_IncludesUsernameInStrengthContext: username passed to
strength check penalizes username-derived passwords.
Parent: PLAN-643 (OSS Security Hardening).
|
||
|
xarmian
|
c10023ea8f |
fix(server): deny-by-default whitelist for API token scopes (TASK-667) (#192)
* fix(server): deny-by-default whitelist for API token scopes (TASK-667) tokenScopeAllows previously fell open on unrecognized scopes and on unparseable scope JSON. A typo like "read-only" silently granted full access — exactly the kind of landmine that a fresh token minted by an admin who misremembers the vocabulary would step on. New policy (deny-by-default): - Unparseable JSON → deny + warn (was allow). Data corruption or tampering should never fall open. - Unrecognized scopes → never contribute to allow; all unknowns on a given request get a single warning log so operators can spot typos. - Explicit wildcard "*" and "write" still allow all methods; "read" still allows safe methods only. - Empty scope string and empty JSON array `[]` still allow — these represent legacy pre-enforcement rows we don't want to break on upgrade. Test table updated: - old "unknown scope allows GET/POST" flipped to deny - new "read-only typo denies GET" regression pin - new "unknown+write/wildcard still allow" guard rails confirming that a recognized allow-granting scope alongside an unknown one still grants (unknown is logged, not failing the request) - old "invalid json allows all" flipped to deny Parent: PLAN-643 (OSS Security Hardening). * fix(server): reject JSON null token scopes (TASK-667) Addresses Codex P2 on PR #192: json.Unmarshal accepts the literal \`null\` without error and leaves the target slice nil, so "scopes": "null" would match the legacy empty-array allow-path and grant full access — bypassing the new deny-by-default intent whenever a client-side serializer emits null for a missing field. - Gate the "unrestricted" path on the raw string being "", ["*"], [ "*" ], or [] only (with whitespace trimming on the outside). "null" no longer slips through. - Post-unmarshal, any empty slice that wasn't one of those explicit allow-forms is logged as "non-array or null scopes; denying" and denied. - New test cases: "json null denies POST" / "json null denies GET". Parent: PLAN-643 (OSS Security Hardening). * fix(server): distinguish JSON null from empty array in token scopes (TASK-667) Addresses Codex P2 on PR #192: the previous raw-string whitelist for legacy empty-array tokens rejected valid whitespace-padded forms like \`[ ]\` or \`[\\n]\` that some clients emit. Those decoded to a non-nil empty slice, so a smarter check works: use the Go json package's nil-vs-empty distinction. - scopes == nil → JSON was literal null. Deny + warn (unchanged intent). - scopes != nil && len == 0 → explicit empty array regardless of whitespace. Allow (legacy unrestricted form, as documented). - scopes has entries → existing whitelist logic. Empty-string fast path kept for the no-column case; wildcard fast path now trims whitespace too. New tests: \`[ ]\`, \`[\\n]\`, \`[\\t]\` empty arrays and \`[ "*" ]\` wildcard all allow; \`null\` still denies. Parent: PLAN-643 (OSS Security Hardening). |
||
|
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
|
0a24078554 |
fix(server): derive CLI auth URL scheme from r.TLS, gate X-Forwarded-Proto on trusted proxies (TASK-665) (#190)
handleCreateCLIAuthSession previously accepted X-Forwarded-Proto from any
client to pick the URL scheme, letting an attacker forge https:// in the
terminal link printed by `pad auth login` on plain-HTTP self-host
deployments. Low-impact phishing (the user clicks in their own terminal),
but the safe default is to ignore unauthenticated proxy headers.
- Factor out cliAuthScheme(r, trustedCIDRs) with explicit precedence:
1. r.TLS != nil -> "https"
2. peer in PAD_TRUSTED_PROXIES -> use X-Forwarded-Proto (first value,
case-insensitive, must be "http" or "https")
3. otherwise -> "http"
- Use rawPeerAddr so the check works even after TrustedProxyRealIP has
rewritten r.RemoteAddr.
- Table-driven tests cover TLS, untrusted-peer spoofing, trusted-peer
forwarding, chained/case-insensitive/garbage X-Forwarded-Proto values.
Parent: PLAN-643 (OSS Security Hardening).
|
||
|
xarmian
|
6f468d37b6 |
fix(config): auto-generate PAD_ENCRYPTION_KEY on first run (TASK-668) (#189)
* fix(config): auto-generate PAD_ENCRYPTION_KEY on first run (TASK-668)
store/encryption.go silently accepted an empty key and stored TOTP
seeds in plaintext; cmd/pad/main.go only logged a WARN. Operators who
never saw the warning (or saw it and ignored it) ran for months with
sensitive data at rest in the clear.
Change: encryption is now mandatory. Resolution order inside
Config.EnsureEncryptionKey:
1. PAD_ENCRYPTION_KEY env var (EncryptionKeySource = "env").
2. encryption_key in config.toml (source = "config").
3. <DataDir>/encryption.key file (source = "file").
4. Generate a fresh 32-byte AES-256 key, persist it to the file
above with 0600 permissions, continue (source = "generated").
Generation step never fails silently — mkdir + write errors propagate
out of main.go and abort startup.
main.go:
- drop the "if cfg.EncryptionKey != '' { enable } else { warn }" fork.
- call cfg.EnsureEncryptionKey(), fail startup on error, log at WARN
when a key is freshly generated so operators notice the new file.
Tests (internal/config/encryption_key_test.go):
- generates when missing (file permissions 0600, 32-byte key).
- loads existing file (strips trailing newline).
- respects already-configured values (no file write).
- idempotent across restarts (same key across two Config objects
sharing a DataDir).
Parent: PLAN-643 (OSS Security Hardening).
* fix(config): refuse to auto-generate key in clustered deployments per Codex P1
Codex caught that auto-generating a per-process key on a Postgres-
backed multi-replica deployment would give each replica its own key —
cross-instance decryption of shared DB rows would fail with GCM auth
errors.
Change: EnsureEncryptionKey now takes an allowGenerate bool. main.go
passes (dbDriver != 'postgres'): single-instance SQLite deployments
get the zero-config auto-generation path; Postgres deployments must
set PAD_ENCRYPTION_KEY explicitly. Operators who DO share a volume
across replicas can pre-seed the file and it still loads (the
generate step is the only thing gated).
Tests:
- TestEnsureEncryptionKey_RefusesToGenerateWhenClustered — allowGenerate=false
+ no existing file → error, no file written.
- TestEnsureEncryptionKey_ClusteredWithPreSeededFileStillLoads — the
file path works in clustered mode when the file is already present.
- Existing idempotency test updated to exercise the mixed case (first
boot generates, second boot loads with allowGenerate=false).
* fix(config): atomic encryption key file creation per Codex P2
Codex caught that the check-then-write sequence for encryption.key had
a race: two processes starting together could both pass the os.ReadFile
IsNotExist check, generate different keys, and race the write.
Whichever process wrote first would end up with an in-memory key that
no longer matched the persisted file, and future restarts of THAT
process would decrypt with the 'wrong' key.
Switch to os.OpenFile with O_CREATE|O_EXCL: on EEXIST we re-read the
file and converge on whichever key won the race. Every racing process
ends up with the same key or a clear startup error.
Test: TestEnsureEncryptionKey_ConcurrentStartIsRaceSafe fires 16
goroutines at a shared DataDir and asserts they all observe the same
key. Also runs clean under -race.
* fix(config): fully-written key guaranteed via temp+hardlink per Codex P2
Codex caught that O_CREATE|O_EXCL + ReadFile-on-EEXIST still had a
window where a loser could read an empty/partial file between the
winner's create and its first write. Hex/length validation would then
fail startup with a confusing error.
Switch to temp-file + os.Link:
1. Write the full key to a uniquely-named temp file (fully closed).
2. os.Link(temp, keyPath) atomically creates the final file as a
hardlink to the complete temp inode. EEXIST means a loser; the
file they'd read is another process's already-complete temp.
3. defer os.Remove(tmpPath) cleans up in every path.
The race-safety test now also covers the 'read partial' case
implicitly — if any goroutine loaded an empty/partial key the hex
decode in main.go would fail in production; the test asserts all 16
goroutines observe the same non-empty key.
* fix(config): reject world/group-readable encryption.key per Codex P2
Codex flagged that the file-load path blindly accepted any mode on
encryption.key. On a multi-user host, a pre-seeded file chmod'd to
0644 would hand the AES key to every local user, defeating the whole
purpose of encrypting TOTP seeds at rest.
Stat the file and reject any mode where group or other bits are set
(0077 mask). Error message points the operator at the fix (chmod 600).
Skipped on Windows where Unix permission bits aren't enforced.
Test: TestEnsureEncryptionKey_RejectsWorldReadableFile pre-seeds the
file at 0644 and verifies startup fails with the chmod hint.
* fix(config): always allow key auto-gen; warn on Postgres per Codex P1
Codex caught that gating auto-generation on 'not postgres' broke the
first-boot experience for every Postgres deployment that wasn't already
provisioning PAD_ENCRYPTION_KEY — which includes our own
docker-compose.yml and deploy/k8s/configmap.yaml. Server would exit
with 'encryption key required' before even starting.
Revert the gate: EnsureEncryptionKey(true) always, for every driver.
In exchange, log a WARN specifically on Postgres when we generate a
key, pointing operators at the multi-replica concern.
Trade-off accepted: single-instance Postgres just works; multi-replica
operators get a visible warning and clear failure mode (GCM auth
errors on first cross-replica read) if they don't act on it. Better
than a startup crash for the single-replica majority.
* fix(config): Postgres requires explicit PAD_ENCRYPTION_KEY; provision it in deployments
Codex was right twice — both concerns are real, and this commit
resolves them together:
1. Restore the Postgres gate: EnsureEncryptionKey(false) when
dbDriver == "postgres". Multi-replica deployments must share a
key; auto-generating per pod would fail cross-replica decryption.
2. Update the shipped Postgres deployments to provision a shared
PAD_ENCRYPTION_KEY so first-boot works out of the box:
- docker-compose.yml: PAD_ENCRYPTION_KEY via ${VAR:?err} shell
substitution (fails "docker compose up" with a clear message
if missing, matching the POSTGRES_PASSWORD pattern).
- .env.example: document PAD_ENCRYPTION_KEY as REQUIRED on
Postgres with an "openssl rand -hex 32" hint.
- deploy/k8s/secret.yaml: add PAD_ENCRYPTION_KEY with a
CHANGE_ME placeholder, explain why the replicas: 2 deployment
requires a shared key.
SQLite deployments continue to auto-generate on first boot (the
TASK-668 happy path), so single-user installs stay zero-config.
|
||
|
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
|
e73196f590 |
fix(server): constant-time compare for CSRF token validation (TASK-659) (#187)
* fix(server): constant-time compare for CSRF token validation (TASK-659) The CSRF middleware compared the cookie and header tokens with Go's == operator, which short-circuits on the first byte mismatch. An attacker who can observe response timing can binary-search for the matching token prefix byte by byte — theoretically useful against a local attacker with precise timing, less so against remote attackers but still a hygiene fix. - middleware_csrf.go: switch to subtle.ConstantTimeCompare. Also explicitly check length equality first, because ConstantTimeCompare returns 0 for mismatched lengths and an earlier Go == check would leak a timing signal about "how many leading bytes matched before the length diverged." Existing CSRF tests (FreshInstallExempt, LoginSetsCSRFCookie, LogoutClearsCSRFCookie, AllMutationMethodsBlocked) continue to pass — the only semantic change is timing-safety on validation. Note on the task's HMAC binding suggestion: binding the CSRF token to the session via HMAC is tracked as a follow-up. It requires a stable server-side HMAC key (similar to the 2FA challenge secret), platform- settings persistence, and a session-cookie-dependent setCSRFCookie signature — larger change than this PR is scoped for. Parent: PLAN-643 (OSS Security Hardening). * fix(server): length-check CSRF as strings before allocating per Codex P2 Codex caught that converting both tokens to []byte up-front forces an allocation proportional to the attacker-controlled X-CSRF-Token header on every failing request — a mild DoS/GC-pressure vector. Compare string lengths first (no allocation), short-circuit on mismatch, and only convert to []byte when lengths match. The allocated path then runs subtle.ConstantTimeCompare for the timing-safe comparison. * fix(server): reject off-size CSRF tokens before allocating per Codex P2 Codex caught that the length-match check still allowed attacker- controlled equally-sized tokens of any size (up to MaxHeaderBytes) to trigger the []byte allocation pair. Since CSRF tokens are always csrfTokenLen*2 hex chars (64 bytes), we can safely reject any length that doesn't match the expected fixed size before allocating anything. - middleware_csrf.go: add expectedLen := csrfTokenLen * 2 (hex), reject any cookie/header whose length != expectedLen before converting to []byte. The subsequent subtle.ConstantTimeCompare then operates on fixed-size 64-byte copies. - middleware_csrf_test.go + handlers_auth_test.go: bump all test fixture tokens to 64 hex chars so the fixed-length validation accepts them. The test-only tokens are arbitrary hex (not generated by the real generator) — they just have to match the shape. |
||
|
xarmian
|
169b79380d |
feat(auth): bigger recovery codes + per-challenge attempt limit (TASK-658) (#186)
* feat(auth): bigger recovery codes + per-challenge attempt limit (TASK-658) generateRecoveryCodes produced 4 bytes of randomness (32 bits) encoded as hex — below the NIST SP 800-63B floor for backup authenticators and grindable online at a few thousand attempts per second. The 2FA verify endpoint also had no per-challenge-token limit on recovery attempts, so a captured challenge could be used to fuzz the entire recovery-code space before the 5-minute expiry. Changes: - handlers_2fa.go: generateRecoveryCodes now emits 10 bytes (80 bits) of entropy encoded as unpadded base32 — 16 chars of [A-Z2-7]. Base32 avoids the 0/O, 1/I/l ambiguity that would bite users typing from a printed backup. 80 bits ≈ 2^80 ≈ 1.2 * 10^24, well above any online grinding budget. - handlers_2fa.go: handleTOTPLoginVerify now rate-limits recovery-code attempts per-challenge-token. Key = "rc:" + SHA-256 of the challenge (so the limiter map never stores the raw HMAC token). Burst of 6 — enough for a user who mistypes a dash or two, nothing more. - middleware_ratelimit.go: new RecoveryCode *ipRateLimiter in the RateLimiters struct, configured at 6/hour burst 6. Test: TestGenerateRecoveryCodes_EntropyShape asserts the 16-char base32 shape and that 8 codes generated in one batch are all distinct (a smoke check on the entropy source). Parent: PLAN-643 (OSS Security Hardening). * fix(auth): normalize recovery code input before hashing per Codex P1 Codex caught that base32 codes are uppercase but users entering them from a mobile keyboard or copy-pasting with dashes would fail the hash comparison, locking out legitimate users and burning per- challenge attempt slots for every typo. Add normalizeRecoveryCode(): strips whitespace and dashes, uppercases the result. handleTOTPLoginVerify runs user input through it before calling store.ConsumeRecoveryCode. Generated codes are already uppercase base32, so the normalization is a no-op for correctly typed codes but catches every common formatting mistake. Test: TestNormalizeRecoveryCode covers lowercase, dashes, whitespace, newlines, and empty input. * fix(auth): legacy lowercase-hex recovery code fallback per Codex P1 Codex caught a backward-compat break: pre-TASK-658 codes were generated via hex.EncodeToString (lowercase), but normalization now uppercases before hashing — so a user with the legacy stored hash typing their exact code is rejected and eventually locked out. After the normalized consume attempt fails, retry once with the raw trimmed input so the original lowercase hex form still validates. No extra rate-limit slot — the limiter.Allow() was already charged. New codes generated post-fix are uppercase base32, so the normalized attempt succeeds on the first try and the fallback is a no-op. |
||
|
xarmian
|
2e21e8f534 |
fix(server): escape unsubscribe page via html/template + add strict CSP (TASK-657) (#185)
handleUnsubscribe piped email addresses through fmt.Sprintf straight
into an HTML string. If the Maileroo email validation ever regressed
to allow characters like '<', '>', or '"', the unsubscribe page would
reflect them into attribute context — a stored/reflected XSS surface
even on this single-purpose utility page.
- Switch to html/template which auto-escapes every {{.Field}} interpolation.
- Add a strict CSP (default-src 'none', script-src 'none', etc.),
Referrer-Policy: no-referrer, and X-Content-Type-Options: nosniff
to every response from this handler. The page needs none of those
sources anyway — only its own inline styles — so denying everything
else is defense in depth for any future regression.
Tests (handlers_unsubscribe_test.go):
- TestUnsubscribePage_EscapesUserInput feeds `"><script>alert('xss')</script>`
as an "email" and verifies the rendered body contains the escaped form
but not the raw tag.
- TestUnsubscribePage_SetsStrictCSP verifies the CSP directives and
nosniff header are present on every render path.
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
|
f23113ab76 |
fix(server): drop cloud_secret query-param fallback (TASK-656) (#183)
handleGetUserByCustomerID accepted ?cloud_secret= for GET sidecar calls. Query values land in access logs — our StructuredLogger records path + raw query, and any fronting reverse proxy typically logs the same. A log file compromise therefore became a compromise of the cloud trust boundary. Remove the fallback in two places: 1. handleGetUserByCustomerID — only checks X-Cloud-Secret header now (or admin auth via cookie/token). Comment explains why the convenience fallback was removed. 2. hasCloudSecretMarker — no longer honors ?cloud_secret on the auth/CSRF bypass path. Header or body-only for POSTs. Sidecars must send Authorization via the X-Cloud-Secret header. Pad Cloud deployment needs to be updated in lockstep; release notes should call this out. Tests: - TestCloudAdminGate_QueryParamSecret_Rejected flips the prior backward-compat test: ?cloud_secret on /user-by-customer now returns 401 (was 404 pre-fix). - TestCloudAdminGate_HeaderSecret_StillAuthenticates confirms the header form still reaches the handler on the same endpoint. Parent: PLAN-643 (OSS Security Hardening). |