Release v0.6.2 #33
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "release/0.6.2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Release v0.6.2 — Hardening & Kiosk Mode
Epics
Changes Since Original PR
Deferred to v0.7.0
Checklist
go test ./...)golangci-lint run ./...)templ generate && goimports && gofumpt)manifest.jsonversion matches0.6.2Stats
References
Version: v0.6.2 — Hardening & Kiosk Mode (Gravity PM)
PR Review
Verdict: Acceptable
This release covers a broad surface — kiosk mode, OIDC session revalidation, security hardening, accessibility, and several bug fixes across 26 commits. The code is well-structured, follows project conventions, and has solid test coverage for new features. No blocking issues found.
Findings
Test Results
golangci-lint run ./...)templ generate && goimports && gofumpt)Human Review Items
2 items flagged for human review — see items marked with HUMAN REVIEW.
Key Findings
internal/handlers/kiosk.goclientIP()ignoresX-Forwarded-Forbehind reverse proxy —internal/handlers/kiosk.gointernal/auth/revalidate.gointernal/middleware/mode.gointernal/handlers/kiosk.go[IMPORTANT] Lines 31-48: DeletionTracker bypass at size cap. When the tracker map reaches 10,000 entries,
Record()silently drops new IPs (line 44). However,Allow()returnstruefor unknown IPs (count0 < 20). This means any IP arriving after the 10,000th gets unlimited deletions until the daily reset. Suggested fix:Allow()should returnfalsewhen the map is full and the IP is not already tracked:[IMPORTANT] Lines 119-125:
clientIP()ignoresX-Forwarded-For. Behind Caddy (per the Gravity Stack architecture),r.RemoteAddrwill always be the proxy's IP. All kiosk visitors share a single 20-deletion pool instead of getting individual limits. The same issue exists inratelimit.goandlocal.go— this is a pre-existing pattern, but kiosk mode makes it more impactful since per-IP tracking is the primary access control.HUMAN REVIEW: Decide whether to trust
X-Forwarded-Forfrom Caddy (requires knowing that no untrusted proxy sits in front). If Caddy is always the direct upstream,X-Forwarded-Foris safe to trust. This would be a cross-cutting change affecting the rate limiter, login tracker, and deletion tracker.internal/auth/revalidate.go[IMPORTANT] Lines 97, 105, 114-115, 123: Silent DB error suppression. Five
_, _ = m.pool.Exec(...)calls discard both the command tag and error. If a session DELETE or UPDATE fails (transient DB error, connection timeout), the failure is completely invisible. At minimum, log at WARN level:[MINOR] Lines 121-124: When the access token is empty or userinfo fails,
revalidated_atis bumped and the return value is"refreshed". This means sessions with expired/missing tokens are counted as "refreshed" in the sweep stats (line 84), which is misleading. A separate status like"skipped"would give more accurate telemetry.[MINOR] Line 70:
_ = json.Unmarshal(rawData, &ss.data)silently ignores unmarshal errors. If session data is corrupted, the session proceeds with an empty map, eventually getting destroyed whenprovider_idfails UUID parse. The self-healing is fine, but the silent discard means corrupted session data is never logged.internal/middleware/mode.go[IMPORTANT] Line 109: Kiosk DELETE allowed too broadly.
r.Method == http.MethodDeletepasses through for any kiosk-allowed path — including/static/,/events/,/search,/demo/. DELETE was intended only for item deletion (/sections/{id}/items/{id}). Downstream handlers likely return 404/405, but for defense-in-depth, restrict DELETE to item paths:[MINOR] Lines 84-94: Guest mode now blocks mutations for unauthenticated users. This is a behavioral change — previously guest mode allowed all operations. Any guest-mode deployment where unauthenticated users were creating/modifying content will break. This should be called out in the release notes.
HUMAN REVIEW: Confirm this breaking change is intentional and documented. Users upgrading from 0.6.1 in guest mode may be surprised by new 403 responses on POST/PUT/DELETE.
internal/services/kiosk.go[MINOR] Lines 337-350:
StartDailyResetpasses the parentctxtorunReset. When ctx is cancelled, Go's random select may fireticker.Cbeforectx.Done(), causingrunResetto execute with a cancelled context. DB operations will fail and log errors during graceful shutdown. Addingif ctx.Err() != nil { return }at the top ofrunResetwould silence the noise.[NITPICK] Lines 73-168:
TakeSnapshotuses N+1 queries (pages → sections per page → items per section). Acceptable for a one-time operation at setup, but a single join query would be more efficient for large dashboards.internal/services/password.golen(password)checks bytes, not characters. Multi-byte UTF-8 characters (emoji, CJK) would hit the limit at fewer visible characters. The validation is correct (bcrypt operates on bytes), but the error message is misleading. Suggest: "at most 72 bytes" orutf8.RuneCountInStringwith "characters".internal/handlers/sse.go/internal/sse/hub.goclose(c.Events)inUnsubscribe(hub.go:61) is safe becauseBroadcastholdsRLockwhileUnsubscribeholds the exclusiveLock— they cannot execute concurrently. Good use of theRWMutexpattern. The correspondingokcheck in sse.go is correct.components/layouts/base.templbuttonelements. The generic modal inpartials/modal.templhas a more comprehensive selector (a[href], button, textarea, input, select, [tabindex]). If the confirm modal ever adds inputs, the focus trap would miss them. Consider using the same comprehensive selector.components/partials/user_settings.templ@changehandler hastry { ... } catch(e) {}that silently swallows JSON parse errors. Addingconsole.warnin the catch would aid debugging without changing behavior.static/js/page-prefetch.js.catch()handler (end ofanimatedSwap) resets theanimatingflag but silently swallows all errors. Addingconsole.errorwould help diagnose animation failures in production.internal/handlers/register.goemailRerejects some RFC 5321 valid addresses (IDN, quoted local parts). This is standard practice and fine for the project scope. Good improvement over the previouscontains("@")check.internal/services/password_test.goTestRejectsTooLong: Convoluted string construction via byte replacement whenstrings.Repeat("a", 73)would be clearer. TheAcceptsMaxLengthtest below uses a simpler pattern.Review conducted from a fresh context against the current
release/0.6.2branch state (26 commits, ~40 changed files). All findings are new.Release v0.6.2 — Hardening & Kiosk Modeto Release v0.6.2PR Review Followup
Responding to review posted on 2026-02-23 (review comment).
Triage Summary
Fixes Applied
The following findings have been resolved on this branch:
5edd41703f3be5c6aa4e0Deferred to v0.7.0
The following items have been tracked for the next version:
Human Review Items (Unchanged)
These items require human judgment and were not auto-triaged:
clientIP()ignoresX-Forwarded-Forbehind reverse proxy — decide whether to trustX-Forwarded-Forfrom Caddy. This is a cross-cutting change affecting the rate limiter, login tracker, and deletion tracker.Test Results
golangci-lint run ./...)This followup was generated by the pr-followup skill.