mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 15:05:40 +00:00
062eef41b2abd664fbfaac8ca5b9be8287d25609
350 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
062eef41b2 |
docs: architecture guide + full .env.example + gitattributes + Makefile note (TASK-687) (#222)
Grouped nice-to-haves called out in the pre-launch audit. 1. docs/architecture.md — new contributor-focused architecture doc. CLAUDE.md covers the same ground but is agent-oriented; this is the human companion. Covers backend layout, request flow, frontend / data model / CLI↔daemon model / agent integration / testing. 2. .env.example — extended to document every PAD_* variable in docs/deployment.md (core, database, real-time events, security, email). Existing Postgres/Redis + encryption secrets kept at the top; new variables grouped by concern with inline comments and safe defaults commented out. 3. .gitattributes — normalize LF line endings repo-wide, mark binary assets, and flag web/build + web/.svelte-kit as generated so they don't pollute GitHub linguist stats or PR diffs. 4. Makefile — CAUTION comment on `make install` noting that the `killall -9 pad` step is system-wide; anyone else's pad daemon on the same machine gets killed too. Designed for single-developer local setups; not for shared hosts. Parent: PLAN-644. |
||
|
xarmian
|
5b14c2e35f |
fix(a11y): BottomSheet tabindex + roles page label associations (TASK-685) (#221)
Closes the four a11y warnings svelte-check surfaces today.
- BottomSheet.svelte: the <div role="dialog"> needs tabindex so screen
readers can focus it programmatically. Add tabindex="-1" — activates
when explicitly focused without putting it in the tab order (matches
the ARIA APG dialog pattern).
- roles/+page.svelte: three <label> elements for Icon & Name,
Description, and Tools had no associated control. Give each target
<input> a stable id (role-name, role-description, role-tools) and
point each <label for={id}>. The dialog renders a single instance at
a time so hardcoded ids are safe.
svelte-check before: 10 warnings (4 target + 6 pre-existing)
svelte-check after: 6 warnings (pre-existing only; no regressions)
Parent: PLAN-644.
|
||
|
xarmian
|
04db0c1a67 |
chore: add .editorconfig, golangci-lint, pre-commit (TASK-682) (#220)
* chore: add .editorconfig, golangci-lint, pre-commit (TASK-682) Prevents style churn from first-time external contributors by codifying the project's formatting and lint rules into shared config. Changes: - .editorconfig: tabs for code, 2-space for YAML/JSON/Markdown, LF everywhere, UTF-8; Makefile overrides enforce tab (syntactic). - .golangci.yml: enables gofmt, govet, errcheck, ineffassign, staticcheck, unused. Scoped to Go sources; excludes web/, docs/, deploy/, skills/. - .pre-commit-config.yaml: repo-local hygiene (trailing whitespace, EOF, YAML/JSON checks, merge-conflict markers, 500KB file cap, LF line endings), Go formatting (go-fmt, go-imports), and prettier for YAML/JSON/Markdown only (Svelte intentionally excluded — no Svelte prettier config yet). - CI: wires golangci-lint into .github/workflows/ci.yml via the official pinned action (v6.5.2 → SHA 55c2c144...). Uses only-new-issues: true so this PR is not blocked by the 17 pre-existing findings on main, which are tracked as IDEA-732 and will flip to strict enforcement after they're resolved. Verified: - golangci-lint run with this config locally; config parses and --new-from-rev=HEAD is clean - All YAML parses via yq - go build/vet/test + web build all green Parent: PLAN-644. Follow-up: IDEA-732 (fix legacy lint findings, flip to strict mode). * fix(ci): upgrade golangci-lint to v2 for Go 1.25 support (TASK-682) Per Codex review on PR #220: golangci-lint v1.x (including v1.64.8 which I originally pinned) is capped at Go 1.24 support. Running v1 binaries against Go 1.25 source can silently drop/misreport findings, defeating the purpose of a lint gate. Switch to: - Action: golangci/golangci-lint-action@v9.2.0 (pinned SHA 1e7e51e7...) - Lint version: v2.11.4 (latest stable v2) - Config rewritten to v2 YAML format (version: "2", linters.default, formatters section for gofmt, exclusions.paths) Verified: - `golangci-lint config verify` clean - `golangci-lint run --new-from-rev=HEAD` reports 0 issues on the current diff (still well under the safety cap since we're using only-new-issues: true) - Full run against main surfaces the expected legacy findings, which remain tracked in IDEA-732 Parent: PLAN-644. * fix(ci): anchor golangci-lint exclusion paths + add PR read perm (TASK-682) Addresses two follow-up comments from Codex on PR #220: P1 — golangci-lint v2 uses regex path matching for exclusions, so the bare patterns "web" and "skills" would also match unrelated Go files whose path contains those substrings (e.g. "internal/websocket"). Anchor with a leading "^" and trailing "/" so only the intended directory trees are skipped. P2 — The workflow already grants "contents: read" at top level, but golangci-lint-action with only-new-issues: true also fetches PR diff metadata from the GitHub API; the action docs list "pull-requests: read" as required for that path. Add it explicitly so the action doesn't fall back to scanning all code and defeating the purpose of scoped issue reporting. Parent: PLAN-644. |
||
|
xarmian
|
be9a96ba21 |
chore: run containers as non-root + harden K8s securityContext (TASK-680) (#219)
First thing external reviewers flag on a public repo: "why does this image run as root?". Fixes both Dockerfiles and the K8s deployment. Dockerfiles (Dockerfile + Dockerfile.goreleaser): - Add a non-login uid:1000 "pad" user via adduser - chown /data so the app can write its SQLite DB as the unprivileged user - Declare USER pad before the ENTRYPOINT deploy/k8s/deployment.yaml: - Pod-level securityContext: runAsNonRoot, runAsUser/Group 1000, fsGroup 1000 (so the emptyDir volume is group-writable), seccompProfile RuntimeDefault - Container-level securityContext: allowPrivilegeEscalation false, readOnlyRootFilesystem true, drop ALL capabilities Verified: - docker build succeeds; `docker inspect ... Config.User` = "pad" - Container running as uid 1000 serves /api/v1/health successfully - Container runs with --read-only rootfs + writable /data volume with no runtime errors (server only writes to /data, never /tmp) - deploy/k8s/deployment.yaml parses via yq; securityContext block structurally correct Parent: PLAN-644. |
||
|
xarmian
|
2b413f5d95 |
chore: add HEALTHCHECK to both Dockerfiles (TASK-681) (#218)
Before: neither Dockerfile declared HEALTHCHECK. docker-compose.yml added it at compose level, so "docker run ghcr.io/xarmian/pad:latest" had no health signal for Docker/Kubernetes/Swarm. Adds HEALTHCHECK to Dockerfile and Dockerfile.goreleaser probing GET /api/v1/health (served by internal/server/server.go:353). Uses wget from busybox (already present in alpine:3.21) so no extra apk install is needed. Interval 30s / timeout 5s / start-period 10s / retries 3 — conservative defaults safe for low-traffic single-user instances. Verified the built image reports the expected HEALTHCHECK via `docker inspect` and that the probe succeeds against a live container. Parent: PLAN-644. |
||
|
xarmian
|
cc2135490c |
chore: remove duplicate padicon.png at repo root (TASK-683) (#217)
The root copy is a byte-identical duplicate of web/static/padicon.png. Only the web/static copy is served (referenced via "/padicon.png" as og:image in web/src/routes/+layout.svelte). Removing 213KB of dead weight from the repo root. Verified no references to "./padicon.png" elsewhere in the codebase. Parent: PLAN-644. |
||
|
xarmian
|
844761cff4 |
chore: add community health files (TASK-679) (#216)
Adds three community health files flagged by the GitHub repo scan: - CODE_OF_CONDUCT.md: Contributor Covenant v2.1 verbatim, with the enforcement contact set to conduct@getpad.dev (matches the domain used for security@getpad.dev in SECURITY.md). - .github/CODEOWNERS: default catch-all routing review requests to @xarmian so PRs auto-request the maintainer. - .github/FUNDING.yml: enables the Sponsor button on the repo via github: xarmian; other platforms left commented for future. Parent: PLAN-644. |
||
|
xarmian
|
8d33dcbfc8 |
chore(ci): add Dependabot configuration (TASK-678) (#202)
Add .github/dependabot.yml covering four ecosystems:
- gomod (root) — Go modules for cmd/pad + internal/*
- npm (/web) — SvelteKit frontend dependencies
- github-actions (root) — pairs with TASK-677 SHA pinning
- docker (root) — both Dockerfile and Dockerfile.goreleaser
Strategy:
- Weekly schedule on Monday 06:00 PT (off-hours, avoids PR flood
on weekdays)
- Minor + patch updates grouped per ecosystem → one PR/week
- Major version bumps get their own PRs so breaking changes are
reviewed in isolation
- open-pull-requests-limit: 5 per ecosystem (3 for docker) keeps
backlog manageable
- Commit prefixes follow the conventional-commit style the project
already uses (chore(deps) / chore(ci) / chore(docker))
Pairs with TASK-677: for SHA-pinned Actions, Dependabot updates both
the commit SHA and the trailing '# vX.Y.Z' comment in one PR, so the
human-readable version label stays in sync.
Parent: PLAN-644.
|
||
|
xarmian
|
1052be7282 |
security(ci): pin all GitHub Actions to commit SHAs (TASK-677) (#201)
Every third-party Action in .github/workflows/ was using a floating tag (@v4, @v5, @v6). A compromised maintainer — or a tag that gets re-pointed at a malicious commit — could execute attacker code in CI with contents:write, packages:write, and the GHCR token in scope. Release.yml is especially exposed: a compromised step there could publish tampered binaries to GitHub Releases and GHCR. All 12 'uses:' references now pin to a 40-char commit SHA with a trailing '# vX.Y.Z' comment (the comment is what humans read during review; the SHA is what GitHub actually resolves): actions/checkout@34e114876b # v4.3.1 actions/setup-go@40f1582b24 # v5.6.0 actions/setup-node@49933ea528 # v4.4.0 docker/setup-buildx-action@8d2750c68a # v3.12.0 docker/login-action@c94ce9fb46 # v3.7.0 goreleaser/goreleaser-action@e435ccd777 # v6.4.0 Version bumps: pin to the newest release within the same major that was previously in use (so behavior stays the same — no major version jumps hidden inside a security PR). Dependabot (incoming in TASK-678) will track the commit-pinned refs and open PRs that update both the SHA and the version comment together. Parent: PLAN-644. |
||
|
xarmian
|
9909d7b7c6 |
fix(web): resolve 9 svelte-check errors blocking CI (TASK-674) (#200)
svelte-check was reporting 9 errors on main, blocking the CI gate.
All fixed:
1. EditCollectionModal: make `open` prop bindable ($bindable()). This
unblocks `bind:open={editCollectionOpen}` in two call sites:
- routes/[username]/[workspace]/[collection]/+page.svelte:1153
- routes/[username]/[workspace]/[collection]/[slug]/+page.svelte:1101
2. [slug]/+page.svelte: narrow `item` inside callback-bound expressions:
- Line 684 (.find closure) now uses a local @const for the slug
rather than re-reading item.parent_collection_slug inside the
callback (TS cannot narrow across the closure).
- Line 744 star toggle handler now short-circuits on item presence,
so both `item.slug` and `item.id` are safe.
3. auth/cli/[code]/+page.svelte: guard against `$page.params.code`
being `undefined` in both onMount and handleApprove.
4. console/settings/+page.svelte: add @types/qrcode dev dependency
so the dynamic `import('qrcode')` calls have proper typings.
`cd web && npx svelte-check` now reports 0 errors (warnings were
out of scope — addressed separately in TASK-685). `go build/vet/test`
and `cd web && npm run build` are green.
Parent: PLAN-644.
|
||
|
xarmian
|
ac744fce2b |
fix(docs): replace 'pad serve' with 'pad server start' (TASK-675) (#199)
The 'pad serve' command does not exist in this binary — its canonical name has been 'pad server start' for some time. Users following the systemd example in docs/deployment.md would get a non-starting service today. Six real references fixed: - cmd/pad/main.go:5714 — migrate-to-pg help text - cmd/pad/main.go:5795 — 'Next steps' instruction - docs/backup.md:81,94 — Postgres migration walkthrough - docs/deployment.md:116 — binary launch example - docs/deployment.md:164 — systemd ExecStart Repo-wide grep is now clean of 'pad serve' outside gitignored v1-archive/ and .pad/ (local workspace data). README.md was already correct. Parent: PLAN-644. |
||
|
xarmian
|
32dbf7b682 |
chore(release): generate changelog via GoReleaser (TASK-676) (#198)
* chore(release): generate changelog via GoReleaser (TASK-676)
The hand-maintained CHANGELOG.md had drifted — it still referenced
'Phases' (renamed to Plans in migration 024) and 'pad status' /
'pad next' (moved to 'pad project dashboard' / 'pad project next').
Rather than rewrite it (it would drift again), delete it and let
GoReleaser auto-generate release notes from conventional commits.
.goreleaser.yaml changelog block upgraded to:
- use GitHub's API for richer formatting ('use: github')
- group by feat/fix/perf/refactor/other with ordered sections
- exclude merge-commit noise on top of the existing docs/test/ci/chore
filters
Release notes from now on come from GitHub Releases (populated by
GoReleaser on tag push), which keeps changelog truth in one place.
Parent: PLAN-644.
* fix(release): anchor changelog group regexes to work with use: github
With changelog.use: github, GoReleaser prefixes entries with the commit
hash (e.g. `abc1234: feat(...): ...`), so regexes anchored at `^feat`
never match — every entry would fall into "Other changes". Prepend
`^.*?` to each group regex so the prefix is allowed.
Per Codex review on PR #198.
|
||
|
xarmian
|
bcd095eefa |
chore(repo): remove personal RTK AGENTS.md from root (TASK-672) (#197)
AGENTS.md at repo root was personal RTK (Rust Token Killer) dev-tool config — off-topic for the public repo. Delete it and add AGENTS.md to .gitignore so contributors can keep a local copy without committing it. Parent: PLAN-644. |
||
|
xarmian
|
0495098254 |
fix(docker): correct goreleaser image CMD (TASK-671) (#196)
Dockerfile.goreleaser had CMD ["serve"] but the pad binary exposes its HTTP server under `pad server start` (no "serve" command exists). As a result, `docker run ghcr.io/xarmian/pad:latest` exited immediately with "unknown command \"serve\"". Align with Dockerfile which already uses CMD ["server", "start"]. Parent: PLAN-644. |
||
|
xarmian
|
4f298db4d0 |
docs(readme): add 'Hardening for public deployments' section + govulncheck CI gate (PLAN-643 exit) (#195)
* docs(readme): add 'Hardening for public deployments' + govulncheck CI gate (PLAN-643 exit criteria) Closes the last two exit criteria of PLAN-643 (OSS Security Hardening): - README.md gains a full "Hardening for public deployments" section walking operators through the network boundary (bind addr, TLS, trusted proxies), secrets (PAD_ENCRYPTION_KEY, token scopes, bootstrap window), auth hardening (PAD_IP_CHANGE_ENFORCE, password strength UI messaging, PAD_CORS_ORIGINS), observability (PAD_METRICS_ TOKEN, audit-log shipping), and a deploy-day checklist. Cross- references every relevant env var documented elsewhere. - CI workflow gains a govulncheck step on the Go job, mirroring the existing `npm audit --audit-level=high --omit=dev` gate on the web job. Locally `govulncheck ./...` reports "No vulnerabilities found", so the first run on main should pass. Exit criteria for PLAN-643: [x] All CRITICAL + HIGH + MEDIUM findings closed and verified [x] `npm audit --audit-level=high --production` clean in web/ [x] CSP denies inline event handlers (script-src-attr 'none') [x] Docker default compose publishes to 127.0.0.1 only [x] README has a "Hardening for public deployments" section [x] `govulncheck ./...` clean * fix(ci): pin govulncheck to v1.2.0 instead of @latest (PLAN-643) Addresses Codex P2 on PR #195: tracking @latest on every CI run makes the gate non-deterministic — a future upstream release could change behavior or require a newer Go toolchain than the workflow's pinned `go-version: 1.25` and break unrelated PRs. Pin to the currently- released v1.2.0 (Go 1.26.2 toolchain) and update intentionally. * docs(readme): recommend pinned govulncheck install in hardening section Follows Codex P2 on PR #195: the earlier commit pinned the CI workflow to v1.2.0 but the README's hardening-checklist bullet still told operators to install @latest. Teams copying that into their own CI would re-introduce the non-determinism the pin was meant to fix. Update the docs to recommend a pinned tag (matching the workflow's v1.2.0) and note that the pin should be bumped intentionally. |
||
|
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). |
||
|
xarmian
|
c2b67f5a9d |
fix(server): gate cloud admin endpoints via requireCloudMode (TASK-655) (#182)
* fix(server): gate cloud admin endpoints via requireCloudMode (TASK-655) middleware_auth.go:184-189 and middleware_csrf.go:44-48 permanently exempted /api/v1/admin/plan, /admin/stripe-customer-id, and /admin/user-by-customer from RequireAuth and CSRFProtect — by path, not by credential. In self-host mode these endpoints still responded to every anonymous network caller (with "Cloud mode not configured"), confirming their existence and telegraphing that the auth surface was non-standard. Three tightly-coupled changes: 1. Narrow both carve-outs from path-based to credential-based. The new isCloudSecretAuthAttempt(r) helper checks for X-Cloud-Secret header or legacy ?cloud_secret query-param; only requests that present one bypass auth/CSRF. Cookie-based admin callers continue through the normal session + CSRF gate. 2. Wrap the three endpoints in a dedicated requireCloudMode group. Self-host mode → 404, no endpoint-existence disclosure. 3. Admin callers via cookie now properly require CSRF for these endpoints (they previously bypassed), bringing them in line with every other /admin/* endpoint. Tests (cloud_admin_gate_test.go): - TestCloudAdminGate_SelfHost_Returns404 — anon + X-Cloud-Secret in self-host → 404 (requireCloudMode fires). - TestCloudAdminGate_NoCloudSecret_RequiresAuth — cloud mode + no secret → 401 from auth gate (not the old "Cloud mode not configured"). - TestCloudAdminGate_ValidCloudSecret_PassesAuthAndCSRF — sidecar with matching X-Cloud-Secret reaches the handler; neither 401 nor 403. - TestCloudAdminGate_QueryParamSecret_BackwardCompat — legacy ?cloud_secret= on GET still works (TASK-656 removes this next). Parent: PLAN-643 (OSS Security Hardening). * fix(server): scope cloud-secret auth bypass to cloud admin paths per Codex P0 Codex caught a regression in the first cut: isCloudSecretAuthAttempt(r) only checked for the presence of X-Cloud-Secret/?cloud_secret, so setting either header on ANY path (e.g. GET /api/v1/workspaces) would bypass RequireAuth globally. An anonymous attacker could list or create workspaces just by adding one of those markers. Add a cloudAdminPaths whitelist and require the request path to be one of the three cloud admin endpoints before honoring the bypass. Defined as a map so a future /api/v1/... route can't accidentally inherit it. Regression test TestCloudAdminGate_BypassScopedToCloudPaths: - GET /workspaces + X-Cloud-Secret → 401 (not bypass) - GET /workspaces?cloud_secret=x → 401 (not bypass) - POST /workspaces + X-Cloud-Secret → 4xx (CSRF 403 or auth 401) * fix(server): make cloud-secret path gate visible at call sites Codex re-flagged the path scoping on PR #182 — even after the fix, the helper name 'isCloudSecretAuthAttempt' made the path scoping invisible at the call site. Split into two primitives: - isCloudAdminPath(path) — path whitelist check - hasCloudSecretMarker(r) — header/query marker check Both middleware now combine them explicitly: if isCloudAdminPath(path) && hasCloudSecretMarker(r) { ... } Behaviorally identical to the previous fix — tests still show GET /workspaces with X-Cloud-Secret returning 401, POST /workspaces with X-Cloud-Secret returning 403. Just makes the invariant readable in RequireAuth and CSRFProtect without having to jump to the helper. * fix(server): preserve body-cloud_secret auth for sidecar POSTs per Codex P1 Codex caught that POST sidecar calls carrying cloud_secret only in the JSON body (the current pad-cloud sidecar behavior) would fail at RequireAuth/CSRFProtect after this PR — handler-level validation never runs. Breaking deployed sidecars isn't the intent of TASK-655; TASK-656 deprecates body+query cloud_secret in favor of X-Cloud-Secret header exclusively, but that's a separate migration. Add body peek to hasCloudSecretMarker for POST/PUT requests with application/json content-type: - Read up to 64 KB of r.Body into a buffer. - Replace r.Body with an io.NopCloser wrapping the buffer so downstream handlers can still decode the JSON. - Return true if the parsed body has a non-empty cloud_secret field. Parse errors and missing fields → false (request falls through to the normal auth rejection, no permissiveness). The peek only runs when the caller is already hitting a cloud admin path via the explicit isCloudAdminPath() gate at the call sites, so the body-read cost is bounded to three endpoints. Test: TestCloudAdminGate_BodySecret_BackwardCompat posts with cloud_secret in the JSON body and no X-Cloud-Secret header, asserts the request reaches the handler (404 from unknown user_id, not 401/403 from middleware). |
||
|
xarmian
|
3544e42de1 |
chore(web): npm audit fix + CI audit gate (TASK-654) (#181)
web/package-lock.json had 11 advisories (1 low, 1 moderate, 9 high) before this PR: @sveltejs/kit (redirect/body-size), cookie<0.7.0, dompurify<=3.3.3, vite 7.0.0-7.3.1, lodash-es, picomatch, chevrotain, etc. All required a mix of `npm audit fix` and targeted upgrades. Changes: - web/package.json: upgrade @sveltejs/kit to ^2.57.1. Add `overrides` map pinning cookie to ^0.7.2 (upstream @sveltejs/kit@2.57.1 still ships cookie@0.6.0 which is LOW severity but trivially fixable). - web/package-lock.json: regenerated via `npm install` + `npm audit fix`. - .github/workflows/ci.yml: add `npm audit --audit-level=high --omit=dev` step after `npm ci`. Fails the build on any HIGH+ advisory in production deps; dev-only issues stay informational so CI isn't held hostage by unfixable upstream chevrotain/vite dev-server advisories. `npm audit --audit-level=high --omit=dev` now reports 0 vulnerabilities locally. `npm run build` and `go test ./...` remain green. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
7d3b468fc8 |
feat(server): gate /metrics behind loopback + bearer token (TASK-653) (#180)
cmd/pad/main.go:277 unconditionally registered Prometheus metrics and internal/server/server.go:229 served /metrics with no auth/CSRF. Any caller on the network could read workspace counts, API usage patterns, and (via label enumeration) user/workspace IDs. Three-layer gate: 1. Loopback-only default. No PAD_METRICS_TOKEN configured → /metrics accepts loopback peers only (safe for self-hosters running Prometheus on the same host, which is the common case). Non-loopback peers get 403 with a clear message. 2. Bearer-token mode. PAD_METRICS_TOKEN set → every scrape must send "Authorization: Bearer <token>", compared in constant time. Missing or wrong header → 401 with WWW-Authenticate: Bearer realm="metrics". 3. Rate-limit/logging chain still wraps the endpoint from the outer router.Use calls. Wiring: - internal/config/config.go — MetricsToken field + PAD_METRICS_TOKEN env. - cmd/pad/main.go — plumb cfg.MetricsToken into SetMetricsToken. - .env.example — document PAD_METRICS_TOKEN with openssl-rand hint. - internal/server/server.go — metricsAuth middleware + subtle.ConstantTimeCompare. Tests: metrics_auth_test.go covers loopback allowed, LAN denied, missing/wrong/correct Bearer, non-Bearer scheme rejected, WWW-Authenticate header, and the SetMetrics-absent 404. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
ae1df43438 |
fix(auth): rotate sessions on password change, TOTP off, OAuth unlink (TASK-652) (#179)
* fix(auth): rotate sessions on password change, TOTP off, OAuth unlink (TASK-652)
handleUpdateCurrentUser previously only updated the password — an
attacker who already stole a session cookie could continue using it
forever even after the owner "rotated" their password. Same issue on
the two other credential-surface-mutating endpoints: disabling 2FA
(handleTOTPDisable) and unlinking an OAuth provider (handleOAuthUnlink).
Extract rotateSessionsAfterCredentialChange:
1. store.DeleteUserSessions(userID) — kills every existing session.
2. Mint a fresh session for the caller via store.CreateSession.
3. Set the new session cookie + CSRF cookie so the caller stays
logged in and doesn't have to re-auth on the current tab.
Call the helper from all three handlers. Best-effort on the delete
step — if it fails we log and still mint a new cookie so the caller
isn't stranded.
Test: TestPasswordChange_InvalidatesOtherSessions establishes two
sessions, changes the password from one, and asserts that (a) a new
session cookie is set, (b) the OTHER session token is 401, and (c)
the original caller token is also 401 (replaced by the fresh one).
Parent: PLAN-643 (OSS Security Hardening).
* fix(auth): return fresh token for Bearer callers after rotation per Codex P2
Codex caught that rotateSessionsAfterCredentialChange only reissued
the caller's session via Set-Cookie. CLI / API clients that authenticate
with 'Authorization: Bearer padsess_...' would be locked out on the
next request after any credential change.
Change the helper to return the new token string. Each handler
(handleUpdateCurrentUser, handleTOTPDisable, handleOAuthUnlink) now
includes the fresh token in its JSON response body so Bearer-only
clients can update their stored credential. Cookie-based clients
continue to pick up the new session transparently via Set-Cookie.
|
||
|
xarmian
|
d86211fcdc |
feat(auth): per-email login rate limiter (TASK-651) (#178)
* feat(auth): per-email login rate limiter (TASK-651) handleLogin is rate-limited per-IP (5/min, in middleware_ratelimit.go), which is effective against a single attacker but useless against a botnet rotating source IPs to spray one victim's password reset email. Add a second limiter keyed on the lowercased email, 10 attempts/hour burst 10. Consumed inside handleLogin on every attempt (success or failure) — a legitimate user remembers their password within 1-2 tries and never hits the limit, but an attacker pounding one account from 50 IPs is locked out after 10 attempts regardless of where those attempts originate. The blocked attempt is logged to the audit log as ActionLoginFailed with reason=email_rate_limited so admins can see which accounts are being sprayed. Tests: - TestHandleLogin_PerEmailRateLimit exhausts the email limit from 10 distinct IPs, then verifies a fresh-IP attempt against the same email gets 429 while a different email from another fresh IP still gets the ordinary 401. - TestHandleLogin_EmailCaseInsensitive verifies the limiter key is normalized — alternating MIXED@/mixed@/Mixed@ all count against the same bucket. Parent: PLAN-643 (OSS Security Hardening). * fix(auth): retain AuthEmail buckets for 2h per Codex P1 Codex caught that ipRateLimiter's cleanup evicts inactive keys after 30 min, which defeats the 10/hour AuthEmail budget: an attacker bursts 10, waits ~30 min for eviction, bursts another 10 — 20 guesses/hour, not 10. Make retention per-config, and set AuthEmail's to 2 hours (≥ 2x the refill window) so the bucket survives the natural pause between spraying rounds. Per-IP limiters keep the 30-min default since their refill is sub-minute. * fix(auth): bound AuthEmail bucket keys by plausibility per Codex P1 Codex caught that the 2-hour retention window on AuthEmail creates a memory-DoS vector — a distributed attacker can POST many long garbage 'email' strings to /api/v1/auth/login and grow the limiter map without bound, since each call inserts a new bucket before any email validation. Add isPlausibleEmail() pre-filter: reject >254 chars (RFC 5321 cap) and strings without an '@' in the interior. Only plausible emails get a bucket; garbage still gets 401 from the password check below but never makes it into the map. Test: TestHandleLogin_ImplausibleEmail_NoBucketCreated hammers the endpoint with 500-char garbage from many IPs and verifies the AuthEmail map never holds a key starting with that garbage pattern. TestIsPlausibleEmail covers empty, missing @, leading/trailing @, over 254, unicode local part. |
||
|
xarmian
|
0657880d14 |
fix(auth): bind invitation acceptance to invitee email (TASK-650) (#177)
handleRegister and handleAcceptInvitation previously accepted any authenticated/creatable account as the invitee. If an attacker learned the invitation URL (email forwarding, shared screenshot, guessed code) they could register a brand-new account at their own address and claim the workspace seat, or sign into an existing account and attach the invitation to it. Add a case-insensitive strings.EqualFold check between the invitee email (inv.Email) and: - the signup form's Email field in handleRegister, before creating the account; and - the authenticated user's Email in handleAcceptInvitation. Mismatch returns 403 invitation_email_mismatch with a clear message pointing the user at the intended address. EqualFold normalizes the casing mismatch against the store's own ToLower() at create time. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
33b3f21a2c |
feat(auth): expire workspace invitations after 14 days (TASK-649) (#176)
* feat(auth): expire workspace invitations after 14 days (TASK-649)
A workspace invite code lives forever until accepted. A leaked code —
email forwarding, stale screenshot, git history — lets any attacker who
registers the invitee's email claim the workspace seat months or years
later.
Introduce a 14-day default expiry:
- New migration (SQLite 044 + Postgres 024) adds expires_at TEXT to
workspace_invitations with an index, backfilling existing rows to
created_at + 14 days so old codes also age out.
- Store CreateInvitation sets expires_at = now + InvitationTTL;
GetInvitation/GetInvitationByCode/ListWorkspaceInvitations read and
populate ExpiresAt. Legacy rows with NULL expires_at are treated as
non-expiring (backward compat for codes created before the migration).
- Model gains ExpiresAt *time.Time and an IsExpired() helper, nil-safe.
- handleAcceptInvitation returns 410 Gone "expired" for expired codes.
- handleRegister (invitation path) returns 410 Gone with the same
message so the signup flow surfaces expiry distinctly from "invalid
code".
Tests: models.TestWorkspaceInvitation_IsExpired covers nil/past/future
plus a nil-receiver safety check.
Parent: PLAN-643 (OSS Security Hardening).
* fix(store): backfill invitation expires_at in RFC3339 per Codex P1
Codex caught that the first cut of migration 044 (SQLite) and 024 (Postgres)
emitted space-separated timestamp strings, which parseTime silently rejects —
legacy invitations would all show up as zero-time ExpiresAt and be treated
as already-expired right after upgrade.
- SQLite: switch to strftime('%Y-%m-%dT%H:%M:%SZ', created_at, '+14 days').
- Postgres: use to_char(..., 'YYYY-MM-DD"T"HH24:MI:SS"Z"').
Add regression tests:
- TestCreateInvitation_SetsExpiresAt — fresh invitations get expiry ~14d out.
- TestMigration044_BackfillProducesRFC3339 — inserts a legacy row with NULL
expires_at, applies the same backfill expression as the migration, and
verifies the round-tripped ExpiresAt is non-zero, parses correctly, and
is ~InvitationTTL after created_at.
* fix(store): drop AT TIME ZONE cast in PG backfill per Codex P2
Codex flagged that '(timestamp + INTERVAL) AT TIME ZONE UTC' yields a
timestamptz, and to_char(timestamptz, ...) renders using the session's
TimeZone — on a non-UTC Postgres instance, legacy invitations get
offset-shifted values mislabeled with a 'Z' suffix.
created_at is already stored as UTC text, so casting it to a naive
timestamp and doing the interval math without further conversion is
both correct and tz-independent. to_char on a plain timestamp uses the
stored value as-is and the hardcoded 'Z' suffix labels it accurately.
|
||
|
xarmian
|
fc5a54dff7 |
fix(server): read raw TCP peer for loopback check (TASK-662) (#175)
* fix(server): read raw TCP peer for loopback check (TASK-662) TrustedProxyRealIP rewrites r.RemoteAddr when the peer is a trusted proxy. Without additional defense, an attacker reaching a trusted reverse proxy could set X-Forwarded-For: 127.0.0.1 and trick the bootstrap loopback check into accepting them as a local caller — reopening the full-instance-takeover path that TASK-660 closed at the spoof layer. Add CapturePeerAddr middleware that runs BEFORE TrustedProxyRealIP and stashes the untampered r.RemoteAddr in request context. Change requestIsLoopback to read via rawPeerAddr(r) (context-first, with a safe fallback for test paths that skip the middleware). r.RemoteAddr stays the rewritten value for the rate-limiter / audit-log paths that actually want the client's IP. Tests cover: direct loopback → true; direct LAN → false; trusted proxy forwarding spoofed 127.0.0.1 → false; untrusted peer with spoofed XFF=127.0.0.1 → false; and that rawPeerAddr falls back to r.RemoteAddr when CapturePeerAddr is absent. Parent: PLAN-643 (OSS Security Hardening). * fix(server): require loopback peer AND no proxy headers for bootstrap (Codex P1) Codex caught a regression in the initial PR: reading rawPeerAddr(r) made every request through a same-host reverse proxy look loopback, so a Caddy or nginx on 127.0.0.1 forwarding public traffic would let attackers reach the bootstrap endpoint from the internet. Tighten the rule to two independent conditions: 1. The untampered TCP peer is a loopback address. 2. Neither X-Forwarded-For nor X-Real-IP is set. A legitimate local CLI calling Pad directly satisfies both. A reverse proxy forwarding public traffic always sets the forwarding headers, so the presence of either disqualifies the request. The raw-peer check still defeats X-Forwarded-For spoofing from non-loopback attackers, and now also handles the Codex-flagged scenario where a local proxy is trusted or left misconfigured. Tests updated to cover: direct loopback no-headers allowed; loopback peer + XFF rejected; loopback peer + X-Real-IP rejected; IPv6 loopback allowed. |
||
|
xarmian
|
5fffee2b9e |
fix(docker): bind to 127.0.0.1 + require POSTGRES_PASSWORD (TASK-661) (#174)
* fix(docker): bind to 127.0.0.1 + require POSTGRES_PASSWORD (TASK-661)
A fresh Docker install previously published 7777 on 0.0.0.0 with a
hardcoded pad:pad Postgres credential. The bootstrap endpoint is
reachable until the first admin is created, so this combination lets
anyone who can route to the host claim the instance — and with M5's
X-Forwarded-For spoof (fixed in TASK-660) chained with the loopback
bootstrap check, it became a full takeover.
Changes:
- docker-compose.yml: publish "127.0.0.1:7777:7777" by default, with a
PAD_BIND_ADDR override for operators who intentionally want LAN
access. Require POSTGRES_PASSWORD via ${VAR:?err} so docker compose
refuses to start when it's unset — can't silently inherit a weak
default credential.
- docker-compose.prod.yml: drop the "change-me-in-production"
placeholder; require the same env var as the base file.
- .env.example: new file documenting POSTGRES_PASSWORD (required),
PAD_BIND_ADDR, REDIS_PASSWORD, PAD_CLOUD_SECRET, PAD_ENCRYPTION_KEY,
PAD_TRUSTED_PROXIES with generation instructions.
- README.md: add a Docker Compose section covering the .env workflow
and the loopback-default → LAN override.
Parent: PLAN-643 (OSS Security Hardening).
* fix(docker): use libpq keyword=value DSN to avoid URI-encoding the Postgres password per Codex P1
Passwords produced by 'openssl rand -base64' often include '/', '+', or ':'
which are reserved in URI userinfo. Injecting them into postgres://user:PASS@...
breaks sql.Open. Switch PAD_DATABASE_URL to the libpq keyword=value form
(host=... password=... dbname=...) where the password is parsed as a single
token regardless of special characters.
Also teach pgDbnameFromURL to parse both DSN shapes so 'pad db backup/restore'
still shows the correct database name in its confirmation prompt.
|
||
|
xarmian
|
ec9edef68c |
fix(server): gate RealIP on PAD_TRUSTED_PROXIES (TASK-660) (#173)
Replace the unconditional chimiddleware.RealIP with a middleware that only trusts X-Real-IP / X-Forwarded-For when the direct TCP peer is within a configured CIDR. With the safe default (PAD_TRUSTED_PROXIES unset) proxy headers are ignored entirely — the real TCP peer address is used for rate limiting, the bootstrap loopback check, and audit logs. Why: previously any client could set X-Forwarded-For to bypass per-IP rate limits AND the bootstrap loopback check (handlers_auth.go). On a direct-exposed Docker deploy (see M6, TASK-661) this compounded into a full-takeover chain. Gating RealIP breaks that chain even when the operator forgets to firewall the port. - internal/server/middleware_realip.go — new TrustedProxyRealIP middleware + ParseTrustedProxyCIDRs helper (accepts CIDRs or bare IPs, invalid entries logged+skipped, empty = nil result = no-op middleware). - internal/server/server.go — swap chimiddleware.RealIP for the gated version; add trustedProxyCIDRs field and SetTrustedProxies wiring. - internal/config/config.go — TrustedProxies field + PAD_TRUSTED_PROXIES env var. - cmd/pad/main.go — plumb config to the server. - internal/server/middleware_realip_test.go — covers no-trust default, untrusted peer, trusted peer with X-Real-IP, X-Forwarded-For first entry, and invalid header. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
204d63151f |
feat(server): strict-dynamic CSP + fail-fast missing index.html (TASK-375) (#172)
Completes the remaining items on the nonce-based CSP work: 1. Add 'strict-dynamic' to script-src. In CSP-L3 browsers this supersedes the 'self' host-list, so a future XSS that injects <script src="//evil"> is blocked even though 'self' is still listed (kept as fallback for older browsers). The SvelteKit bootstrap script already dynamically imports the runtime chunks, which is exactly the pattern strict-dynamic is designed to permit. 2. Fail fast when the embedded index.html can't be read. The previous silent-swallow returned blank HTML to every SPA request, which is a broken build that the operator should notice immediately. Panic at startup so the server refuses to come up with a broken UI. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
4297689e23 |
fix(server): add script-src-attr 'none' to CSP (TASK-648) (#171)
Inline event handlers (onerror, onload, onclick, …) bypass the script-src directive per CSP spec. Without script-src-attr 'none' an attacker who slips markup past the DOMPurify sanitizer can still execute JavaScript via event attributes — defeating the whole point of the nonce-based script-src. Add 'script-src-attr 'none'' to both CSP headers: - internal/server/middleware_security.go — strict policy for API responses - internal/server/server.go — nonce-based policy for HTML pages Defense-in-depth for TASK-647 (comment markdown sanitizer) and for any future regression in HTML-emitting paths. Parent: PLAN-643 (OSS Security Hardening). |
||
|
xarmian
|
42cc220024 |
fix(web): sanitize rendered markdown through DOMPurify (TASK-647) (#170)
* fix(web): sanitize rendered markdown through DOMPurify (TASK-647)
Comments (and any other caller of renderMarkdown) piped marked() output
straight to {@html}. A malicious comment could inject <script> / <img
onerror> / javascript: links that executed on every viewer's page —
stored XSS with full session takeover.
Wrap renderMarkdown's output in DOMPurify.sanitize with a strict
allowlist of markdown-produced tags and attributes. Also HTML-escape
the wiki-link title before interpolating it into the <a>/<span> so the
intermediate HTML is well-formed even for pathological titles.
Sanitization runs client-side only (adapter-static SPA mode has no
runtime SSR of user content). In SSR/prerender contexts we return ""
rather than emit unsanitized HTML — markdown-bearing views fetch their
data at runtime anyway, so the empty fallback is a no-op.
Parent: PLAN-643 (OSS Security Hardening).
* fix(web): allow ol start attribute in markdown sanitizer per Codex review
|
||
|
xarmian
|
2e00a6769a |
feat(web): wire WorkspaceSwitcher into mobile TopBar (TASK-640) (#169)
* feat(web): wire WorkspaceSwitcher into mobile TopBar (TASK-640)
Follow-up to TASK-637: the WorkspaceSwitcher component was built with a
BottomSheet branch on mobile but it was never rendered anywhere — the
TopBar had its own inline horizontal workspace list on both desktop
and mobile.
- Mobile: swap the TopBar's horizontal workspace list + "+" add button
+ "edit/reorder" button for a single <WorkspaceSwitcher /> chip. Tap
opens the BottomSheet of workspaces + "+ New Workspace". Removes the
horizontal-scroll discoverability problem when a user has many
workspaces.
- Desktop: unchanged. Still uses the inline list with drag-to-reorder.
- Users who want to reorder workspaces can do it on desktop; mobile
drag-reorder is a rarely-used workflow and the edit button added
visible chrome on cramped mobile chrome.
- WorkspaceSwitcher now calls `uiStore.onNavigate()` on select/create
so the mobile sidebar closes on workspace switch — preserves the
previous TopBar link behavior.
- Removed now-unused state + handlers: mobileEditMode, enterEditMode,
exitEditMode, handleMobileConsider, handleMobileFinalize, the
reorder-overlay markup and CSS, the currentUsername derived (it was
already unused).
Parent: PLAN-631.
* fix(web): let callers force WorkspaceSwitcher's mobile branch (Codex review)
Codex flagged a P2: TopBar branches mobile/desktop on uiStore.isMobile
(≤768px) but WorkspaceSwitcher uses its own 639.98px matchMedia. At
640–768px viewports (small tablets), the mobile TopBar would render
the desktop WorkspaceSwitcher dropdown — reintroducing the clipping
this PR was trying to fix.
- Add an optional `mobile?: boolean` prop to WorkspaceSwitcher that
overrides the internal viewport detection when passed. Auto-detect
still runs when the prop is omitted (for future callers).
- Mirror the rotation-reopen guard for the prop path: if `mobile`
flips to false while the sheet is open, close it.
- TopBar passes `mobile={true}` when rendering inside its mobile branch
so the decision stays consistent with `uiStore.isMobile`.
Per Codex review on PR #169.
|
||
|
xarmian
|
041472496b |
feat(web): select field editor renders as BottomSheet on mobile (TASK-638) (#168)
Scope note: the task also mentioned multi_select, but FieldEditor
currently has no custom UI for multi_select — it falls through to the
plain text input. Scoping this PR to `select`, where the absolute-
positioned inline dropdown is the actual mobile pain (clips off the
edge of the properties panel when the chip sits near the right edge).
A dedicated multi_select editor is a separate piece of work.
- Track `isMobile` via `matchMedia('(max-width: 639.98px)')`.
- Extract the options list into a `{#snippet selectOptions}` shared
between branches so markup doesn't duplicate.
- Mobile: on `dropdownOpen`, render `<BottomSheet title="Set {label}">`
with the options list. Sheet gated on `isMobile && dropdownOpen`
(gate-on-open pattern) so the sheet's global keydown listener isn't
mounted per idle FieldEditor.
- Desktop: unchanged inline `.select-dropdown` with keyboard nav.
- `handleWindowClick` bails early on mobile so it doesn't race the
sheet's backdrop/Escape dismissal.
- Viewport-change handler closes the dropdown if the breakpoint leaves
mobile so returning to mobile doesn't reopen the sheet.
- `selectOption` still calls `onchange(opt)` and closes — save
semantics unchanged.
Parent: PLAN-631.
|
||
|
xarmian
|
ee65e10562 |
feat(web): workspace switcher renders as BottomSheet on mobile (TASK-637) (#167)
The workspace switcher in the top bar is cramped on mobile; its
absolute-positioned dropdown runs off-screen when workspace names are
long or the list is deep.
- Track `isMobile` via `matchMedia('(max-width: 639.98px)')`.
- Extract workspace list + "+ New Workspace" row into a shared
`{#snippet workspaceList}`.
- Mobile: render the list inside `<BottomSheet title="Switch workspace">`
with roomier tap targets. Sheet gated on `open` (gate-on-open pattern)
so BottomSheet's global keydown listener isn't mounted when idle.
- Desktop: unchanged dropdown + backdrop.
- Viewport-change handler closes the sheet if we leave mobile so it
doesn't spring back open on rotation.
- Selecting a workspace navigates via `goto` as before; the sheet
unmounts naturally on navigation.
- "+ New Workspace" still closes the sheet and calls
`uiStore.openCreateWorkspace()` — the existing modal already works
well on mobile.
Parent: PLAN-631.
|
||
|
xarmian
|
e34c8e463a |
feat(web): view-mode selector renders as BottomSheet on mobile (TASK-636) (#166)
Scope note: the task described selectors for view-mode, sort-by, and
group-by, but only view-mode has a visible selector today (a 3-icon
segmented toggle). Sort and group-by are not user-selectable from the
collection page — they're derived from collection settings. Scoping
to the one visible selector that needed help; adding sort/group
selectors is a separate feature.
- On mobile (<640px), the segmented view-mode toggle is replaced by a
labeled chip ("View: Board ▾") that opens a BottomSheet titled
"Choose view" with each option labeled + iconed. Icon-only segmented
buttons are hard to decode on touch; labeled options are clearer.
- On desktop, the segmented 3-icon toggle is unchanged.
- Sheet mounted only when open (gate-on-open pattern).
- Breakpoint-change handler closes the sheet if the viewport leaves
mobile so it doesn't reopen on rotation back.
- saveViewMode + updateUrlFilters semantics preserved (localStorage +
URL sync unchanged).
Parent: PLAN-631.
|
||
|
xarmian
|
6fa82d9b74 |
feat(web): FilterBar parent filter renders as BottomSheet on mobile (TASK-635) (#165)
* feat(web): filter-bar parent filter renders as BottomSheet on mobile (TASK-635)
Scope note: the task description envisioned chip-driven per-field
dropdowns, but FilterBar today is simpler: status is an inline
segmented button row (doesn't clip, just wraps) and parent is a
native <select>. The pragmatic change that matches the task's intent
("mobile-friendly BottomSheet UX on the FilterBar") is the parent
filter — long plan names + inconsistent native <select> styling
across iOS/Android are the real mobile pain here.
- Status segmented group: unchanged (already mobile-safe; wraps to
second line when the toolbar is narrow).
- Parent filter on mobile: render as a chip trigger that opens a
BottomSheet titled "Filter by plan" with the same option list.
- Parent filter on desktop: native <select> unchanged.
- Sheet mounted conditionally on `parentSheetOpen` to avoid the
dormant global keydown listener (gate-on-open pattern from TASK-633).
Parent: PLAN-631.
* fix(web): reset parent sheet when viewport leaves mobile (Codex review)
Codex flagged a P2: when the parent filter sheet was open on mobile and
the viewport crossed above the mobile breakpoint (e.g. device rotation),
`parentSheetOpen` stayed `true`. The desktop branch hid the sheet, but
returning to mobile would immediately remount `{#if parentSheetOpen}`
and reopen the sheet without a user tap.
Fix: close the sheet in the `matchMedia` change handler whenever the
breakpoint no longer matches mobile.
Per Codex review on PR #165.
|
||
|
xarmian
|
72ecf66f53 |
feat(web): move-to menu renders as BottomSheet on mobile (TASK-634) (#164)
The "Move to…" dropdown on the item detail page sits in a cluster of
meta-actions near the right edge of the viewport; its absolute-positioned
list of collections clips off-screen on narrow mobile.
- Track `isMobile` via `matchMedia('(max-width: 639.98px)')` on the page.
- Extract the options list into a `{#snippet moveOptions}` so both
branches share the same markup.
- Mobile: render `<BottomSheet title="Move to…">` gated on
`isMobile && showMoveMenu` so the sheet (and its global keydown
listener) isn't mounted when the menu is closed.
- Desktop: unchanged `.move-dropdown` popover.
- Mobile sheet option rows get a roomier padding / larger font to be
thumb-reachable.
Parent: PLAN-631.
|
||
|
xarmian
|
424a60a5f4 |
feat(web): reaction picker renders as BottomSheet on mobile (TASK-633) (#163)
* feat(web): reaction picker renders as BottomSheet on mobile (TASK-633)
Swap `ReactionPicker` (used inside `TimelineCommentCard` for top-level
comments and replies) to a mobile-first BottomSheet branch while keeping
the existing absolute-positioned popover intact for desktop.
- Track `isMobile` via `matchMedia('(max-width: 639.98px)')` using the same
pattern as `QuickActionsMenu`/`EmojiPickerButton`.
- Mobile: render the 12 emoji options inside `<BottomSheet title="React">`
with a roomier 6-col grid + 48px tap targets since we have the viewport
width on our side.
- Desktop: unchanged popover.
- The outside-click `$effect` only attaches when open AND not mobile so it
doesn't race the sheet's own backdrop/Escape dismissal.
- Share the emoji grid between branches via a `{#snippet emojiGrid}` to
avoid duplication.
Parent: PLAN-631.
* fix(web): gate mobile ReactionPicker sheet on open (Codex review)
Codex flagged a P2 performance regression: on mobile the BottomSheet
instance was mounted for every ReactionPicker regardless of `open`, and
each mounted instance installs a global keydown listener via
`<svelte:window onkeydown>` inside BottomSheet. On comment-heavy
timelines (top-level comments + replies) this fans every keystroke out
through many dormant listeners.
Fix: additionally gate the mobile branch on `open`, matching the
desktop branch semantics (only mount when active).
Per Codex review on PR #163.
|
||
|
xarmian
|
174be6f045 |
feat(web): emoji picker renders as BottomSheet on mobile (TASK-632) (#162)
Swap `EmojiPickerButton` to a mobile-first BottomSheet branch while keeping
the existing absolute-positioned portal dropdown intact for desktop.
- Track `isMobile` via `matchMedia('(max-width: 639.98px)')` using the same
pattern as `QuickActionsMenu` (the reference implementation from TASK-628).
- When `isMobile`: render the picker inside a `<BottomSheet>` titled "Pick
an emoji" so the 300ish-px grid is readable full-width and can't clip.
- When `!isMobile`: unchanged — portal + `getBoundingClientRect` math still
owns positioning inside `<dialog>` modals and at the document root.
- `handleWindowClick` bails early on mobile so it doesn't race the sheet's
own backdrop/Escape dismissal.
Parent: PLAN-631.
|
||
|
xarmian
|
e4c2ff0a03 |
fix(web): simplify BottomSheet to fix broken mobile interactions (#161)
* fix(web): simplify BottomSheet to fix broken mobile interactions
The original BottomSheet layered on several advanced behaviors — portal
to body, module-level $state open-stack, focus trap, swipe-to-dismiss,
reactive z-index, stacked-sheet Escape routing — and something in that
stack broke click dispatch on Android Chromium for every button inside
the sheet except the existing quick-action rows.
Root cause investigation: every click handler I wrote in this session
failed to fire on Android (close button, backdrop, footer rows, even
an unrelated debug banner's dismiss), while the existing shipped UI
(CreateCollectionModal etc.) continued to work fine. That narrowed
the problem to something structural in the new components rather than
any specific CSS / event wiring.
Fix: rewrite BottomSheet as a ~100-line clone of the working
CreateCollectionModal pattern — plain {#if open} + overlay +
stopPropagation on the inner panel, nothing more. Mobile-first
CSS docks the sheet to the bottom of the viewport; a single
@media (min-width: 640px) rule centers it as a traditional modal on
desktop. No portal, no module-scope $state, no <svelte:window>, no
focus trap, no swipe gesture.
Tradeoffs deliberately accepted for now:
- Swipe-to-dismiss is gone. Backdrop tap + close button are the
dismissal paths; the viewport-scoped overlay makes this fine.
- No focus trap. Every other modal in the app already ships without
one, so this matches existing behavior.
- No stacked-sheet Escape prioritization. Single-sheet usage only.
Can be re-layered carefully later if any of those features are
actually needed, but only one feature at a time with mobile testing
between each.
* fix(web): restore Escape dismissal + ARIA dialog semantics on BottomSheet
Addresses both P2 comments from Codex on PR #161.
- Escape key closes the sheet. Added a svelte:window onkeydown that
early-returns when !open, matching the pattern used elsewhere in
the app. This is the keyboard dismissal path for desktop and users
with hardware keyboards on mobile — and the only keyboard path
when title is omitted (no close button rendered).
- Restored role=\"dialog\", aria-modal=\"true\", and aria-labelledby
(pointing at the visible title heading when one is set, falling
back to aria-label=\"Dialog\" otherwise). Without these, assistive
tech wouldn't announce modal context and users could continue
navigating background content.
Stable per-instance heading id uses \$props.id() (SSR-safe), bound to
a top-level const per the Svelte 5 placement rule.
Notably NOT reintroduced: focus trap, portal, module-scope stack,
swipe gesture, reactive z-index. Those were the culprits for the
Android click-dispatch regression and stay out of the simplified
implementation.
|
||
|
xarmian
|
1dde0d3b58 |
feat(web): inline + New and Manage affordances in QuickActionsMenu (TASK-629) (#160)
* feat(web): inline + New and Manage affordances in QuickActionsMenu (TASK-629) Add discovery paths for creating and editing quick actions directly from the menu. Closes the Problem 2 gap in IDEA-493 — the editor already existed inside EditCollectionModal but was effectively invisible from the menu surface. QuickActionsMenu (gated behind a new canEdit prop): - "+ New quick action" footer row → toggles an inline form (icon picker + label + monospace prompt input + template-variable help). On save, PATCHes the collection via api.collections.update, appends the new action to settings.quick_actions, and fires oncollectionupdated so the parent reloads. Toast on success / error. - "⚙️ Manage actions" footer row → fires onmanage, which the parent wires to open EditCollectionModal deep-linked to the Quick Actions tab. - Trigger button now stays visible for editors even when no actions exist yet, so they can bootstrap the first action without round-tripping through collection settings. EditCollectionModal: - New initialSection?: 'general' | 'fields' | 'display' | 'actions' prop. When set, opens the modal directly to that tab instead of the default 'general'. Default behavior unchanged. Route wiring: - [collection]/+page.svelte: passes wsSlug + canEdit={isOwner} + onmanage/oncollectionupdated; tracks editCollectionSection to deep-link the existing modal. - [collection]/[slug]/+page.svelte: same QuickActionsMenu wiring, plus imports + renders EditCollectionModal inline (it wasn't present on item detail before) so the "Manage actions" link works from item pages too. Parent: IDEA-493. * fix(web): preserve emoji picker in QuickActionsMenu + navigate on archive Addresses both P2 comments from Codex on PR #160. - QuickActionsMenu: the EmojiPickerButton portals its dropdown to document.body (.epb-dropdown). The outside-click guard was treating portal clicks as "outside" the menu and closing it, losing the in-progress emoji selection before the bound value could update. Added an exemption for .epb-dropdown and .emoji-picker-button in handleWindowClick so clicks inside the picker keep the menu open. - Item detail page: when EditCollectionModal archives the current collection, onupdated fires with no updated arg. The old handler just reloaded the sidebar, leaving the user on a now-invalid item route with stale state. It now also navigates back to the workspace root so follow-up actions don't hit deleted resources. * fix(web): redirect on collection slug change from item-page modal When an owner renames the current collection from the item detail page's EditCollectionModal, the collection's slug can change. The old onupdated handler updated local state but stayed on the now- stale /[collection]/[slug] URL — subsequent loadData() calls fetch by collSlug and would 404. Mirror the collection-page behavior: navigate to the new collection slug while preserving the item slug so the user stays on the same item under its new route. Addresses Codex round 2 P2 on PR #160. * fix(web): apply returned collection state in oncollectionupdated On the collection list page, the oncollectionupdated callback ignored the updated collection returned by api.collections.update and waited for loadCollection() to refetch. On slow responses, a user saving a second quick action in rapid succession would build the PATCH from stale collection.settings.quick_actions and overwrite the first action. Apply the returned collection to local state immediately, then still trigger loadCollection as a defensive refresh. The item detail page's handler already does the right thing, so only the collection page is affected. Addresses Codex round 3 P2 on PR #160. * fix(web): reload item after non-navigating collection edit from item page EditCollectionModal can change schema / field mappings. After a non-archive, non-rename save on the item detail page, the callback was only updating the collection reference — not the item — so stale item.fields could survive a rename or migration. A subsequent updateField() would then write the full stale fields JSON back to api.items.update and clobber migrated values. Call loadData() after non-navigating updates so the item is refetched alongside the collection. Navigation cases (archive, slug change) already trigger their own load via the route change, so we skip the reload on those branches. Addresses Codex round 4 P2 on PR #160. |