mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
fix(b3-9): close the B3-tagged findings — OC-0345, OC-0346, OC-0376, OC-0377, OC-0378 (#1454)
* docs(b3-9): record B3-2's merge (#1450 =75d64dd4); B3-9 in progress Plan status line, B3-2 step-table row (DONE) and evidence block carry the squash SHA; docs/plans/README.md B3 row updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo * fix(api): panic log carries trace_id — tracing ahead of recoverer (OC-0346) recoverer snapshots telemetry.TraceIDFromContext before dispatch, so it needs the otelhttp span to exist already; it was mounted two slots ahead of telemetry.HTTPMiddleware and the trace_id attribute was always dropped. Move the tracing middleware above it; request-id binding, security headers and the body cap keep their relative positions. Test (otel build only — the default build hard-wires TraceIDFromContext to ""): go test -tags otel -run TestRecoverer_PanicLogCarriesTraceID ./api/ Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo * fix(admin): owner gate answers 503 on a role read fault, not 403 (OC-0345) ownerOnlyMiddleware collapsed `err != nil || role == nil` into 403 "role not found", so a transient GetRoleByID failure told the Owner they lack the Owner role. Split the outcomes: a store error logs and answers 503 SERVICE_UNAVAILABLE (the perimeter's contract); a genuinely missing role still answers 403. The existing whitebox tests, which inject only the user into the context, are unchanged. Test: TestOwnerOnlyMiddleware_RoleLookupFailureIs503 (roles table renamed, whitebox — through the full stack the perimeter would answer its own 503). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo * fix(service): verify-totp reports a store fault as 500, uncounted (OC-0377) challengeSecret folded a GetUserByID error into the same 401 "invalid or expired two-factor challenge" an expired challenge earns, and the attempt had already been charged to the per-user totp_fail cap. Split the outcome: a store error logs and returns the new service.ErrTOTPUnavailable (ErrInternal, "two-factor verification temporarily unavailable"); an unknown user or a missing secret still answers 401. The limiter reservation moves after the store read — the rule authenticate already applies — and still precedes the code compare, so the check-then-act it closes stays closed. Characterization row flipped in the same commit: `VerifyTOTPFailurePaths/ user lookup fails -> 500, challenge kept, attempt not counted` — after the fault ten wrong codes still answer 401 (the tenth would be 429 had the fault counted), then the eleventh is refused. `per-user failure cap spans challenges` unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo * fix(service): verify-totp keeps the verified second factor when the session insert fails (OC-0378) VerifyTOTP consumed the partial challenge before issueSession, so a store fault on the session insert discarded a verified second factor and sent the user back to the password step; the code was also marked used, so an immediate retry would have been refused as a replay. The claim stays atomic and first (two concurrent verifies can never both reach issueSession). On issueSession failure the challenge is restored under the same partial token — the client still holds it — and the accepted code is released, so the retry completes the login without another password step. auth gains PartialAuthStore.Restore and UsedTOTPCodeStore.Unmark, each tested in the leaf package. Characterization row flipped in the same commit: `VerifyTOTPFailurePaths/ session insert fails -> 500, the challenge and the code survive` — once the trigger is dropped the same token and the same code answer 200 with a token. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo * fix(service): registration commits the account, the invite use and the first session together (OC-0376) CreateUserWithInvite committed the user and burned the invite; the session insert ran outside that transaction, so a store fault there answered 500 with a half-registered account — a retry got "invalid invite or credentials" while a login with the same password worked. Option B from the ledger: the session token is generated first and the session row is inserted inside the same transaction (db.insertSession through dbgen.Queries.WithTx; no query or migration change, so no sqlc regen). A fault at any step rolls the whole registration back and the caller simply retries. The H-6 cap needs no eviction for a user with no sessions. Characterization row flipped in the same commit: `RegisterPolicyAndFailurePaths/ session insert fails -> 500, nothing committed` — user row absent, invite use_count 0, message "registration failed — please try again". db tests pass the three new arguments; the happy-path test asserts the session row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo * refactor(service): one session-token path for Register and issueSession OC-0376 gave Register its own auth.GenerateToken failure branch — a duplicate of the unreachable one issueSession already carried — and the auth slice's statement coverage dipped from 91.8% to 91.7% on that one statement. newSessionToken generates the token and hands its hash to a persist callback: CreateSession for login and verify-totp, the CreateUserWithInvite transaction for registration. Behaviour identical (the characterization file is green before and after); slice coverage 402/437 = 92.0%, service/auth.go 250/263 = 95.1%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo * chore(ledger): close the five B3-tagged findings; counts re-derived (PR #1454) OC-0346 →775eba50, OC-0345 →fb1afb8a, OC-0377 →f7015809, OC-0378 →be37d7ee, OC-0376 →85d86dc7: status fixed, fix.test, revertProof pass (hand reverse-apply per commit + verify-fixes.mjs). Ledger 315 fixed / 59 open → 320 / 54 (3 declined, 1 duplicate, 378). The four count-carrying documents are re-derived around every number, not just the totals (obs #100): docs/plans/README.md, hp-0-scorecard (54 open = 1 high / 12 medium / 41 low; three hunts; 53 of 54 resolve; Client 33 / Server 21), repo-health-issue-register (table, "eleven of which", OC-0345/OC-0346 rows marked fixed with this PR), b0-baseline. OC-0323 stays open — it rides B3-8's message/read-state family. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo * fix(service): verify-totp — cap check before the store read; release the code on a lost claim (Codex P2s, #1454) Two P2s from Codex on PR #1454, both verified against the code and fixed test-first: 1. An exhausted totp_fail window is refused by the read-only limiter.Check before challengeSecret, so rotating source IPs cannot drive user reads and secret decryptions past the per-user cap. The atomic Allow that records the attempt still runs after the store read (OC-0377: an outage charges nothing); the cap boundary is unchanged. Test: api/totp_cap_before_store_test.go — budget filled through the limiter, users table hidden, expects 429 (RED: 500 "temporarily unavailable", the store was read first). 2. A verify whose claim loses at Consume releases the code it marked, so a winner mid-recovery (Consume → issueSession failed → Restore) is not left with a live token behind a dead code until the authenticator rolls over. Test: service/auth_lost_claim_test.go — forces the interleaving through the store's GetUserByID (RED: "the losing claim left its code marked as used"). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo * docs(b3-9): evidence block, status line, step-table row; README B3 row (PR #1454) Per finding: pre-squash SHA, RED and GREEN lines, revert-proof, the two negative controls, what changed; the ledger diff and the re-derived count paragraphs; auth-slice coverage 402/437 = 92.0% (floor 392/427 = 91.8%), service/auth.go 250/263 = 95.1%; the otel-tagged run. OC-0323 recorded as riding B3-8. hp-3-scorecard untouched — the owner signs it as drafted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
J3vb
@@ -7967,13 +7967,19 @@
|
||||
"why": "`adminAuthMiddleware` already resolved the principal's `*db.Role` via `auth.ResolveTokenHash` and stored it in the request context under `adminRoleKey` (line 90). `ownerOnlyMiddleware` ignores that value, issues a second `GetRoleByID` on the request context, and collapses `err != nil` into the same 403 \"role not found\" it uses for a genuinely missing role. This is the exact fail-closed-as-authorization-denial collapse that the perimeter branch 60 lines above was explicitly fixed for (it now answers 503 SERVICE_UNAVAILABLE and logs, precisely so a DB outage is not reported as a bad credential), and that `api/middleware.go:117` was fixed for. The function's own doc comment (lines 120-121) claims it \"reads the user from context ... rather than re-authenticating, avoiding redundant DB queries\" — the redundant query it claims to avoid is the one that introduces the fault.",
|
||||
"repro": "Owner is signed into the admin panel. Any transient read failure on the `roles` lookup (SQLITE_BUSY / \"database is locked\" while a scheduled backup's `VACUUM INTO` runs, a disk I/O error, or a context deadline on the reader pool) hits `GetRoleByID` during a request to one of the nine owner-only routes registered in Server/admin/api.go:148-176 — `GET /admin/api/updates`, `POST /admin/api/updates/apply`, `POST /admin/api/backup`, `GET /admin/api/backups`, `DELETE /admin/api/backups/{name}`, `POST /admin/api/backups/{name}/restore`, `GET|POST /admin/api/tokens`, `DELETE /admin/api/tokens/{id}`. The perimeter middleware immediately before it already succeeded and put the correct, non-nil Owner role in the context, so the request is fully authenticated and authorized. The Owner nevertheless receives HTTP 403 `FORBIDDEN {\"code\":\"FORBIDDEN\",\"message\":\"role not found\"}` — the admin panel renders a permission-denied error telling the server Owner they lack the Owner role — and nothing is logged, unlike the perimeter path which logs the underlying error. Using the already-resolved `adminRoleKey` value (or mirroring the perimeter's 503 + slog on `err != nil`) makes the outcome correct.",
|
||||
"evidence": "// middleware.go:89-93 (perimeter already stores the role)\nctx := context.WithValue(r.Context(), adminUserKey, user)\nctx = context.WithValue(ctx, adminRoleKey, role)\n\n// middleware.go:59-69 (perimeter, after the OC fix: DB error != bad token)\ndefault:\n slog.ErrorContext(r.Context(), \"admin: token resolution failed\", \"error\", err)\n writeErr(w, http.StatusServiceUnavailable, \"SERVICE_UNAVAILABLE\", \"authentication service temporarily unavailable\")\n\n// middleware.go:130-134 (ownerOnlyMiddleware, unfixed sibling)\nrole, err := database.GetRoleByID(r.Context(), user.RoleID)\nif err != nil || role == nil {\n writeErr(w, http.StatusForbidden, \"FORBIDDEN\", \"role not found\")\n return\n}",
|
||||
"status": "open",
|
||||
"status": "fixed",
|
||||
"found": "2026-08-22",
|
||||
"hunt": "general-2026-08-22-b",
|
||||
"lens": "explore-1",
|
||||
"suggestedFix": "Split the two outcomes in ownerOnlyMiddleware rather than switching to the context role. Reading adminRoleKey would be the cleaner design but it breaks both TestOwnerOnlyMiddleware_RoleNotFound and TestOwnerOnlyMiddleware_OwnerPassesThrough, which inject only adminUserKey. Smallest change that preserves every locked behavior, at Server/admin/middleware.go:130-134:\n\n role, err := database.GetRoleByID(r.Context(), user.RoleID)\n if err != nil {\n slog.ErrorContext(r.Context(), \"admin: owner role lookup failed\", \"error\", err)\n writeErr(w, http.StatusServiceUnavailable, \"SERVICE_UNAVAILABLE\", \"authorization service temporarily unavailable\")\n return\n }\n if role == nil {\n writeErr(w, http.StatusForbidden, \"FORBIDDEN\", \"role not found\")\n return\n }\n\nrole==nil still yields 403 (test at middleware_and_spawn_test.go:211 unaffected), the owner path still yields 200, and the DB fault now matches the perimeter's 503 + slog contract.",
|
||||
"confidence": "medium",
|
||||
"finder": "opus"
|
||||
"finder": "opus",
|
||||
"fixed": "2026-08-30",
|
||||
"fix": {
|
||||
"commit": "fb1afb8a",
|
||||
"test": "Server/admin/middleware_and_spawn_test.go",
|
||||
"revertProof": "pass"
|
||||
}
|
||||
},
|
||||
{
|
||||
"id": "OC-0346",
|
||||
@@ -7984,13 +7990,19 @@
|
||||
"why": "`recoverer` is registered at router.go:286, two slots ahead of `telemetry.HTTPMiddleware()` at router.go:291, and it snapshots `telemetry.TraceIDFromContext(r.Context())` *before* calling `next.ServeHTTP`. At that moment no span exists in the request context (otelhttp is downstream), so `TraceIDFromContext` returns \"\" on every request and the `trace_id` attribute the recovery closure promises is always dropped by the `if traceID != \"\"` guard at line 666. The panic record — the one log line where trace correlation matters most — is the only one that silently loses it, while in-handler logs via logctx.go:38 get it correctly because they run inside the span.",
|
||||
"repro": "Build with `-tags otel`, set telemetry.enabled=true and exporter=\"otlp\" (or \"prometheus\"), then issue a request to any REST route whose handler panics (e.g. force a nil deref in a handler). The recovered-panic slog record contains method/path/panic/stack/req_id but never a trace_id attribute, even though otelhttp created a live span for that exact request and the trace is exported. Moving `r.Use(telemetry.HTTPMiddleware())` above `r.Use(recoverer)` (or reading the trace ID inside the deferred closure instead of before dispatch) makes the same request log the real trace ID.",
|
||||
"evidence": "router.go:286-291\n\tr.Use(recoverer) // slog-routing panic recovery ...\n\tr.Use(requestLogger)\n\tr.Use(telemetry.HTTPMiddleware())\n\nrouter.go:646-667\n\t// Capture correlation IDs before dispatch ... while the\n\t// panic log still carries req_id/trace_id.\n\treqID := middleware.GetReqID(r.Context())\n\ttraceID := telemetry.TraceIDFromContext(r.Context()) // <- no span yet: always \"\"\n\tdefer func() {\n\t\tif rec := recover(); rec != nil {\n\t\t\t...\n\t\t\tif traceID != \"\" {\n\t\t\t\tattrs = append(attrs, \"trace_id\", traceID)\n\t\t\t}\n\t\t\tslog.Error(\"http handler panic recovered\", attrs...)\n\nBuild-tag dependency: telemetry_otel.go's TraceIDFromContext is the only implementation that can ever return non-empty (telemetry_default.go:26 hardcodes \"\"), and it reads trace.SpanContextFromContext(ctx), which is populated by otelhttp.NewHandler in (*otelProvider).HTTPMiddleware — mounted after recoverer.",
|
||||
"status": "open",
|
||||
"status": "fixed",
|
||||
"found": "2026-08-22",
|
||||
"hunt": "general-2026-08-22-b",
|
||||
"lens": "explore-2",
|
||||
"suggestedFix": "In routerMiddleware (Server/api/router.go:278-292) move `r.Use(telemetry.HTTPMiddleware())` above `r.Use(recoverer)`. With otelhttp outermost, recoverer's r.Context() already carries the live span, so the existing line 650 capture yields the real trace ID, and recoverer still recovers handler panics because it remains outside every route handler. This is one line in the shared stack rather than a change in recoverer, and it keeps the deferred closure free of context calls (the contextcheck constraint the comment cites). It does not disturb the ordering the file comment calls a security property — request-id binding, security headers and the body cap keep their relative positions.",
|
||||
"confidence": "high",
|
||||
"finder": "opus"
|
||||
"finder": "opus",
|
||||
"fixed": "2026-08-30",
|
||||
"fix": {
|
||||
"commit": "775eba50",
|
||||
"test": "Server/api/recoverer_otel_test.go",
|
||||
"revertProof": "pass"
|
||||
}
|
||||
},
|
||||
{
|
||||
"id": "OC-0347",
|
||||
@@ -8495,12 +8507,18 @@
|
||||
"repro": "cd Server && go test -count=1 -run 'TestAuthCharacterization_RegisterPolicyAndFailurePaths/session_insert_fails' ./api/ — the row installs a BEFORE INSERT ON sessions trigger, registers with a valid invite, and pins today's outcome: 500, user row present, invite use_count 1.",
|
||||
"evidence": "Server/api/auth_handler.go:166 uid, err := database.CreateUserWithInvite(...) // commits user + invite use\nServer/api/auth_handler.go:201 if _, err := database.CreateSession(...); err != nil { // 500 after the commit\nServer/api/auth_handler.go:153 // Hash password before consuming the invite so that a hashing failure\n // does not burn a valid invite code.",
|
||||
"suggestedFix": "Either answer 201 without a token when the session insert fails after the account commit (the account exists; the client logs in), or move the session insert into the CreateUserWithInvite transaction so registration is atomic. Belongs to the AuthService in B3-2/B3-9, not to the handler.",
|
||||
"status": "open",
|
||||
"status": "fixed",
|
||||
"found": "2026-08-29",
|
||||
"hunt": "b3-1-auth-characterization-2026-08-29",
|
||||
"lens": "characterization",
|
||||
"confidence": "high",
|
||||
"finder": "claude"
|
||||
"finder": "claude",
|
||||
"fixed": "2026-08-30",
|
||||
"fix": {
|
||||
"commit": "85d86dc7",
|
||||
"test": "Server/api/auth_characterization_test.go, Server/db/coverage_boost_test.go",
|
||||
"revertProof": "pass"
|
||||
}
|
||||
},
|
||||
{
|
||||
"id": "OC-0377",
|
||||
@@ -8512,12 +8530,18 @@
|
||||
"repro": "cd Server && go test -count=1 -run 'TestAuthCharacterization_VerifyTOTPFailurePaths/user_lookup_fails' ./api/ — the row renames the users table after the challenge is issued and pins today's 401.",
|
||||
"evidence": "Server/api/totp_handler.go:140 user, err := database.GetUserByID(r.Context(), challengeUserID)\nServer/api/totp_handler.go:141 if err != nil || user == nil || user.TOTPSecret == nil {\nServer/api/totp_handler.go:142 writeJSON(w, http.StatusUnauthorized, errorResponse{ ... \"invalid or expired two-factor challenge\" })\nServer/api/auth_handler.go:488-497 the login sibling: a non-nil error is a genuine DB failure -> 500 \"login temporarily unavailable\"",
|
||||
"suggestedFix": "Split the condition: `err != nil` -> 500 INTERNAL_ERROR (\"two-factor verification temporarily unavailable\") without RegisterFailure and with the limiter reservation undone or not made; keep 401 for `user == nil || user.TOTPSecret == nil`. Fix in B3-9 after B3-2 lands, and flip the characterization row with it.",
|
||||
"status": "open",
|
||||
"status": "fixed",
|
||||
"found": "2026-08-29",
|
||||
"hunt": "b3-1-auth-characterization-2026-08-29",
|
||||
"lens": "characterization",
|
||||
"confidence": "high",
|
||||
"finder": "claude"
|
||||
"finder": "claude",
|
||||
"fixed": "2026-08-30",
|
||||
"fix": {
|
||||
"commit": "f7015809",
|
||||
"test": "Server/api/auth_characterization_test.go",
|
||||
"revertProof": "pass"
|
||||
}
|
||||
},
|
||||
{
|
||||
"id": "OC-0378",
|
||||
@@ -8529,12 +8553,18 @@
|
||||
"repro": "cd Server && go test -count=1 -run 'TestAuthCharacterization_VerifyTOTPFailurePaths/session_insert_fails' ./api/ — the row installs a BEFORE INSERT ON sessions trigger, verifies a valid code, pins the 500, drops the trigger and pins that the same partial token is now refused with 401.",
|
||||
"evidence": "Server/api/totp_handler.go:107 if _, ok := partialStore.Consume(partialToken); !ok { // challenge gone here\nServer/api/totp_handler.go:115 token, err := issueSession(r.Context(), database, user.ID, challenge.Device, challenge.IP) // fails after it",
|
||||
"suggestedFix": "Keep the claim atomic and first: Consume the challenge before issuing the session (as today), then on CreateSession failure re-issue or restore the challenge for the same user/device/IP so the verified second factor is not discarded. The restore must also keep the accepted verification usable: VerifyTOTPCodeOnce has already recorded (user, code) in UsedTOTPCodeStore for 90 s, so an immediate retry with the authenticator's still-current code would be refused as a replay - either carry the verified state on the restored challenge (retry issues the session without a new code) or roll back that MarkUsed claim together with the challenge. Do NOT issue the session before Consume: two concurrent requests holding the same partial token can pass Lookup with different valid codes from the +/-1 step window (the used-code store keys on (user, code), not the token), both would create sessions, and the losing Consume would leave an unreturned bearer session in the database; if the order must change, the loser has to revoke the session it created. Belongs to the AuthService in B3-2/B3-9.",
|
||||
"status": "open",
|
||||
"status": "fixed",
|
||||
"found": "2026-08-29",
|
||||
"hunt": "b3-1-auth-characterization-2026-08-29",
|
||||
"lens": "characterization",
|
||||
"confidence": "high",
|
||||
"finder": "claude"
|
||||
"finder": "claude",
|
||||
"fixed": "2026-08-30",
|
||||
"fix": {
|
||||
"commit": "be37d7ee",
|
||||
"test": "Server/api/auth_characterization_test.go, Server/auth/totp_test.go",
|
||||
"revertProof": "pass"
|
||||
}
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
@@ -128,7 +128,15 @@ func ownerOnlyMiddleware(database *db.DB, next http.Handler) http.Handler {
|
||||
}
|
||||
|
||||
role, err := database.GetRoleByID(r.Context(), user.RoleID)
|
||||
if err != nil || role == nil {
|
||||
if err != nil {
|
||||
// A read fault is an outage, not a missing role: answering 403
|
||||
// would tell the Owner they lack the Owner role. Mirror the
|
||||
// perimeter's contract above — log it, report 503 (OC-0345).
|
||||
slog.ErrorContext(r.Context(), "admin: owner role lookup failed", "error", err)
|
||||
writeErr(w, http.StatusServiceUnavailable, "SERVICE_UNAVAILABLE", "authorization service temporarily unavailable")
|
||||
return
|
||||
}
|
||||
if role == nil {
|
||||
writeErr(w, http.StatusForbidden, "FORBIDDEN", "role not found")
|
||||
return
|
||||
}
|
||||
|
||||
@@ -522,3 +522,52 @@ func TestSpawnDetached_CommandConstruction(t *testing.T) {
|
||||
t.Error("cmd.Stderr should not be nil")
|
||||
}
|
||||
}
|
||||
|
||||
// TestOwnerOnlyMiddleware_RoleLookupFailureIs503 pins OC-0345: a database
|
||||
// fault on the owner gate's role read is an outage, not a missing role, so the
|
||||
// Owner must get 503 SERVICE_UNAVAILABLE — never the 403 "role not found" a
|
||||
// genuinely absent role earns. Whitebox on purpose: through the full stack
|
||||
// adminAuthMiddleware reads the role first and would answer its own 503, so
|
||||
// the branch under test would never run.
|
||||
func TestOwnerOnlyMiddleware_RoleLookupFailureIs503(t *testing.T) {
|
||||
database := openWhiteboxTestDB(t)
|
||||
|
||||
uid, err := database.CreateUser(context.Background(), "ownerfault", "$2a$12$x", 1)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUser: %v", err)
|
||||
}
|
||||
user, err := database.GetUserByID(context.Background(), uid)
|
||||
if err != nil || user == nil {
|
||||
t.Fatalf("GetUserByID: %v", err)
|
||||
}
|
||||
// Every query against roles now fails with a non-sentinel error.
|
||||
if _, err := database.ExecContext(context.Background(), `ALTER TABLE roles RENAME TO roles_gone`); err != nil {
|
||||
t.Fatalf("hide roles: %v", err)
|
||||
}
|
||||
|
||||
reached := false
|
||||
next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
reached = true
|
||||
w.WriteHeader(http.StatusOK)
|
||||
})
|
||||
handler := ownerOnlyMiddleware(database, next)
|
||||
|
||||
ctx := context.WithValue(context.Background(), adminUserKey, user)
|
||||
req := httptest.NewRequest(http.MethodPost, "/backup", nil).WithContext(ctx)
|
||||
w := httptest.NewRecorder()
|
||||
handler.ServeHTTP(w, req)
|
||||
|
||||
if reached {
|
||||
t.Error("next handler was reached although the role could not be read")
|
||||
}
|
||||
if w.Code != http.StatusServiceUnavailable {
|
||||
t.Errorf("status = %d, want 503 (a role read fault is not a missing role)", w.Code)
|
||||
}
|
||||
var resp map[string]string
|
||||
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
|
||||
t.Fatalf("unmarshal: %v", err)
|
||||
}
|
||||
if resp["error"] != "SERVICE_UNAVAILABLE" {
|
||||
t.Errorf("error = %q, want SERVICE_UNAVAILABLE", resp["error"])
|
||||
}
|
||||
}
|
||||
|
||||
@@ -449,23 +449,22 @@ func TestAuthCharacterization_RegisterPolicyAndFailurePaths(t *testing.T) {
|
||||
t.Error("user row exists after a failed insert")
|
||||
}
|
||||
})
|
||||
t.Run("session insert fails after the user is committed -> 500", func(t *testing.T) {
|
||||
t.Run("session insert fails -> 500, nothing committed", func(t *testing.T) {
|
||||
database := newAuthTestDB(t)
|
||||
router := buildAuthRouter(database, auth.NewRateLimiter())
|
||||
code := seedInvite(t, database)
|
||||
failWrite(t, database, "INSERT", "sessions")
|
||||
rr := send(t, router, http.MethodPost, "/api/v1/auth/register", "", "", "", body(code))
|
||||
wantErr(t, rr, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to create session")
|
||||
// known: the account and the invite use are already committed when the
|
||||
// session insert fails, so the caller sees a 500 for a registration
|
||||
// that succeeded — a retry gets "invalid invite or credentials" while
|
||||
// a login with the same password works (ledger OC-0376).
|
||||
if userByName(t, database, "fresh") == nil {
|
||||
t.Error("user row missing: this row pins the partial-success behaviour, which has changed")
|
||||
// OC-0376 (fixed in B3-9): the first session is inserted inside the
|
||||
// registration transaction, so a store fault leaves no half-registered
|
||||
// account and does not burn the invite — the caller simply retries.
|
||||
if userByName(t, database, "fresh") != nil {
|
||||
t.Error("user row exists after the session insert failed")
|
||||
}
|
||||
if n := inviteUseCount(t, database, code); n != 1 {
|
||||
t.Errorf("invite use_count = %d, want 1 (pinned partial success)", n)
|
||||
if n := inviteUseCount(t, database, code); n != 0 {
|
||||
t.Errorf("invite use_count = %d, want 0 (transaction rolled back)", n)
|
||||
}
|
||||
wantErr(t, rr, http.StatusInternalServerError, "INTERNAL_ERROR", "registration failed — please try again")
|
||||
})
|
||||
}
|
||||
|
||||
@@ -777,16 +776,31 @@ func TestAuthCharacterization_VerifyTOTPFailurePaths(t *testing.T) {
|
||||
}
|
||||
const challengeGone = "invalid or expired two-factor challenge"
|
||||
|
||||
t.Run("user lookup fails -> 401", func(t *testing.T) {
|
||||
t.Run("user lookup fails -> 500, challenge kept, attempt not counted", func(t *testing.T) {
|
||||
database, router, _, secret := setup(t)
|
||||
pt := loginPartial(t, router, "two", "correctPass1", "", "")
|
||||
hideTable(t, database, "users")
|
||||
// known: totpChallengeSecret folds a database error into the same 401
|
||||
// an expired challenge gets, so a DB fault during the second factor
|
||||
// reads as a bad code and burns the caller's attempt budget (ledger
|
||||
// OC-0377). The plan's rule is 5xx for a non-sentinel error; pinned
|
||||
// as-is for B3-2, fixed in B3-9.
|
||||
wantErr(t, verify(t, router, pt, totpCode(t, secret), ""), http.StatusUnauthorized, "UNAUTHORIZED", challengeGone)
|
||||
// OC-0377 (fixed in B3-9): a store fault while loading the challenged
|
||||
// user is an outage, not a bad challenge — 5xx, the challenge stays
|
||||
// live, and the attempt is not charged to the per-user totp_fail cap.
|
||||
wantErr(t, verify(t, router, pt, totpCode(t, secret), ""), http.StatusInternalServerError, "INTERNAL_ERROR", "two-factor verification temporarily unavailable")
|
||||
if _, err := database.ExecContext(context.Background(), `ALTER TABLE users_gone RENAME TO users`); err != nil {
|
||||
t.Fatalf("restore users: %v", err)
|
||||
}
|
||||
// The cap is 10 per user: ten wrong codes must all still answer 401
|
||||
// "invalid two-factor code" — had the faulted attempt counted, the
|
||||
// tenth would be the 429. The first five ride the surviving challenge
|
||||
// (which proves it was kept), the next five a fresh one.
|
||||
attempt := 0
|
||||
for _, token := range []string{pt, loginPartial(t, router, "two", "correctPass1", "", "")} {
|
||||
for range 5 {
|
||||
attempt++
|
||||
wantErr(t, verify(t, router, token, wrongTOTPCode(t, secret), fmt.Sprintf("203.0.113.%d", attempt)), http.StatusUnauthorized, "UNAUTHORIZED", "invalid two-factor code")
|
||||
}
|
||||
}
|
||||
// Negative control: the counter is live — the eleventh attempt is refused.
|
||||
pt = loginPartial(t, router, "two", "correctPass1", "", "")
|
||||
wantErr(t, verify(t, router, pt, totpCode(t, secret), "203.0.113.99"), http.StatusTooManyRequests, "RATE_LIMITED", "too many failed attempts, try again later")
|
||||
})
|
||||
t.Run("secret removed after the challenge was issued -> 401", func(t *testing.T) {
|
||||
database, router, uid, secret := setup(t)
|
||||
@@ -809,18 +823,31 @@ func TestAuthCharacterization_VerifyTOTPFailurePaths(t *testing.T) {
|
||||
}
|
||||
wantErr(t, verify(t, router, pt, totpCode(t, secret), ""), http.StatusInternalServerError, "INTERNAL_ERROR", "failed to verify two-factor code")
|
||||
})
|
||||
t.Run("session insert fails -> 500 and the challenge is consumed", func(t *testing.T) {
|
||||
t.Run("session insert fails -> 500, the challenge and the code survive", func(t *testing.T) {
|
||||
database, router, _, secret := setup(t)
|
||||
pt := loginPartial(t, router, "two", "correctPass1", "", "")
|
||||
failWrite(t, database, "INSERT", "sessions")
|
||||
wantErr(t, verify(t, router, pt, totpCode(t, secret), ""), http.StatusInternalServerError, "INTERNAL_ERROR", "failed to create session")
|
||||
// known: the partial token was consumed before the session insert, so
|
||||
// the user must repeat the password step after a transient store
|
||||
// failure (ledger OC-0378). Pinned as-is; the code is also marked used.
|
||||
code := totpCode(t, secret)
|
||||
wantErr(t, verify(t, router, pt, code, ""), http.StatusInternalServerError, "INTERNAL_ERROR", "failed to create session")
|
||||
// OC-0378 (fixed in B3-9): the verified second factor is not discarded
|
||||
// by a store fault — the challenge is restored under the same partial
|
||||
// token and the accepted code is released, so once the store is back
|
||||
// the same token and the same code complete the login. (Restore alone
|
||||
// would refuse the retry as a replay.)
|
||||
if _, err := database.ExecContext(context.Background(), `DROP TRIGGER fault_insert_sessions`); err != nil {
|
||||
t.Fatalf("drop trigger: %v", err)
|
||||
}
|
||||
wantErr(t, verify(t, router, pt, totpCode(t, secret), "203.0.113.2"), http.StatusUnauthorized, "UNAUTHORIZED", challengeGone)
|
||||
rr := verify(t, router, pt, code, "203.0.113.2")
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("retry status = %d, want 200; body = %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
var res struct {
|
||||
Token string `json:"token"`
|
||||
Requires2FA bool `json:"requires_2fa"`
|
||||
}
|
||||
if err := json.Unmarshal(rr.Body.Bytes(), &res); err != nil || res.Token == "" || res.Requires2FA {
|
||||
t.Fatalf("retry body = %s (err %v), want a session token", rr.Body.String(), err)
|
||||
}
|
||||
})
|
||||
t.Run("per-user failure cap spans challenges -> 429 on the 11th attempt", func(t *testing.T) {
|
||||
_, router, _, secret := setup(t)
|
||||
|
||||
@@ -0,0 +1,75 @@
|
||||
//go:build otel
|
||||
|
||||
package api
|
||||
|
||||
// OC-0346: the recovered-panic log record must carry the request's trace_id.
|
||||
// Only the otel build can produce one (telemetry_default.go's
|
||||
// TraceIDFromContext is hard-wired to ""), so this file is tagged and CI's
|
||||
// untagged test run does not see it. Run it with
|
||||
//
|
||||
// go test -tags otel -count=1 -run TestRecoverer_PanicLogCarriesTraceID ./api/
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"regexp"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/go-chi/chi/v5"
|
||||
|
||||
"github.com/J3vb/OwnCord/Server/config"
|
||||
"github.com/J3vb/OwnCord/Server/telemetry"
|
||||
)
|
||||
|
||||
// TestRecoverer_PanicLogCarriesTraceID drives a panicking handler through the
|
||||
// real routerMiddleware stack with tracing on and asserts the panic record
|
||||
// carries the span's trace id. Before the fix recoverer was mounted ahead of
|
||||
// telemetry.HTTPMiddleware, so it captured the trace id from a context that
|
||||
// had no span yet and the attribute was always dropped.
|
||||
func TestRecoverer_PanicLogCarriesTraceID(t *testing.T) {
|
||||
shutdown, err := telemetry.Init(context.Background(), config.TelemetryConfig{
|
||||
Enabled: true,
|
||||
Exporter: "prometheus", // a real tracer provider, no network exporter
|
||||
ServiceName: "recoverer-test",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("telemetry.Init: %v", err)
|
||||
}
|
||||
t.Cleanup(func() { _ = shutdown(context.Background()) })
|
||||
|
||||
var logs bytes.Buffer
|
||||
prev := slog.Default()
|
||||
slog.SetDefault(slog.New(slog.NewJSONHandler(&logs, nil)))
|
||||
t.Cleanup(func() { slog.SetDefault(prev) })
|
||||
|
||||
r := chi.NewRouter()
|
||||
routerMiddleware(r, &config.Config{})
|
||||
r.Get("/boom", func(http.ResponseWriter, *http.Request) { panic("boom") })
|
||||
|
||||
rr := httptest.NewRecorder()
|
||||
r.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, "/boom", nil))
|
||||
if rr.Code != http.StatusInternalServerError {
|
||||
t.Fatalf("status = %d, want 500 (panic not recovered)", rr.Code)
|
||||
}
|
||||
|
||||
var rec map[string]any
|
||||
for _, line := range strings.Split(strings.TrimSpace(logs.String()), "\n") {
|
||||
var m map[string]any
|
||||
if json.Unmarshal([]byte(line), &m) == nil && m["msg"] == "http handler panic recovered" {
|
||||
rec = m
|
||||
break
|
||||
}
|
||||
}
|
||||
if rec == nil {
|
||||
t.Fatalf("no recovered-panic record in logs:\n%s", logs.String())
|
||||
}
|
||||
traceID, _ := rec["trace_id"].(string)
|
||||
if !regexp.MustCompile(`^[0-9a-f]{32}$`).MatchString(traceID) {
|
||||
t.Fatalf("panic record trace_id = %q, want the span's 32-hex trace id; record = %v", traceID, rec)
|
||||
}
|
||||
}
|
||||
@@ -271,7 +271,8 @@ func routerHealthDeps(cfg *config.Config, database *db.DB, getOnlineUsers *func(
|
||||
}
|
||||
|
||||
// routerMiddleware installs NewRouter's global middleware stack. The order is a
|
||||
// security property (request-id binding before the logger reads it, security
|
||||
// security property (request-id binding before the logger reads it, tracing
|
||||
// before panic recovery so the panic log carries the trace id, security
|
||||
// headers and the body cap before any handler runs) — keep it exactly as
|
||||
// written.
|
||||
func routerMiddleware(r chi.Router, cfg *config.Config) {
|
||||
@@ -282,12 +283,14 @@ func routerMiddleware(r chi.Router, cfg *config.Config) {
|
||||
// NOTE: middleware.RealIP is intentionally omitted — trusting X-Real-IP from
|
||||
// any source allows IP spoofing for rate-limit bypass. IP header trust is now
|
||||
// handled explicitly in clientIPWithProxies using the trusted_proxies config.
|
||||
r.Use(recoverer) // slog-routing panic recovery (replaces chi's stderr-only Recoverer)
|
||||
r.Use(requestLogger) // structured request/response logging
|
||||
// Phase B Step 8 — OpenTelemetry HTTP tracing. No-op when telemetry is
|
||||
// disabled or the otel build tag is not set, so this is safe to mount
|
||||
// unconditionally.
|
||||
// unconditionally. Mounted ahead of recoverer, which snapshots the trace
|
||||
// id before dispatch: the span must already exist for the panic record to
|
||||
// carry trace_id (OC-0346).
|
||||
r.Use(telemetry.HTTPMiddleware())
|
||||
r.Use(recoverer) // slog-routing panic recovery (replaces chi's stderr-only Recoverer)
|
||||
r.Use(requestLogger) // structured request/response logging
|
||||
r.Use(SecurityHeadersWithTLS(cfg.TLS.Mode))
|
||||
r.Use(MaxBodySizeUnless(defaultMaxBodySize, bodyCapExemptPrefixes...))
|
||||
|
||||
|
||||
@@ -0,0 +1,37 @@
|
||||
package api_test
|
||||
|
||||
// Codex P2 on PR #1454 (B3-9, OC-0377): once a user's totp_fail cap is
|
||||
// exhausted, verify-totp must refuse before it loads the user and decrypts
|
||||
// the secret — otherwise rotating source IPs (the per-user cap is the only
|
||||
// cross-IP defence) could drive store reads and decryptions without bound.
|
||||
// The read-only Check runs ahead of the store read; the atomic Allow that
|
||||
// records the attempt still sits after it, so an outage charges nothing.
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/J3vb/OwnCord/Server/auth"
|
||||
)
|
||||
|
||||
func TestVerifyTOTP_ExhaustedCapRefusesBeforeStoreRead(t *testing.T) {
|
||||
database := newAuthTestDB(t)
|
||||
limiter := auth.NewRateLimiter()
|
||||
router := buildAuthRouter(database, limiter)
|
||||
uid := seedUser(t, database, "capped", "correctPass1", 4)
|
||||
secret := enrolTOTP(t, database, uid)
|
||||
pt := loginPartial(t, router, "capped", "correctPass1", "", "")
|
||||
|
||||
// Exhaust the per-user budget (service: totpFailureRateLimit = 10 in
|
||||
// totpFailureWindow = 15 min) without a single HTTP failure.
|
||||
for range 10 {
|
||||
limiter.Allow(auth.Key("totp_fail", uid), 10, 15*time.Minute)
|
||||
}
|
||||
// Every user read now fails; an attempt that reaches the store answers
|
||||
// 500 "two-factor verification temporarily unavailable" (OC-0377).
|
||||
hideTable(t, database, "users")
|
||||
|
||||
rr := send(t, router, http.MethodPost, "/api/v1/auth/verify-totp", pt, "203.0.113.50", "", map[string]string{"code": totpCode(t, secret)})
|
||||
wantErr(t, rr, http.StatusTooManyRequests, "RATE_LIMITED", "too many failed attempts, try again later")
|
||||
}
|
||||
@@ -108,6 +108,16 @@ func (s *PartialAuthStore) Consume(token string) (PartialAuthChallenge, bool) {
|
||||
return entry, true
|
||||
}
|
||||
|
||||
// Restore puts a challenge Consume returned back under its original token —
|
||||
// the recovery path for a caller that claimed the challenge and then could
|
||||
// not finish the login (OC-0378). The entry keeps its expiry and failure
|
||||
// count, so a challenge that expired meanwhile is dropped by the next Lookup.
|
||||
func (s *PartialAuthStore) Restore(token string, challenge PartialAuthChallenge) {
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
s.entries[token] = challenge
|
||||
}
|
||||
|
||||
func (s *PartialAuthStore) RegisterFailure(token string, maxFailures int) bool {
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
@@ -198,6 +208,15 @@ func (s *UsedTOTPCodeStore) MarkUsed(userID int64, code string) bool {
|
||||
return true
|
||||
}
|
||||
|
||||
// Unmark forgets a code MarkUsed recorded so it can be accepted once more —
|
||||
// the companion of PartialAuthStore.Restore: a verification the caller could
|
||||
// not complete is released together with its challenge.
|
||||
func (s *UsedTOTPCodeStore) Unmark(userID int64, code string) {
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
delete(s.entries, fmt.Sprintf("%d:%s", userID, code))
|
||||
}
|
||||
|
||||
func (s *UsedTOTPCodeStore) cleanupExpiredLocked() {
|
||||
now := time.Now()
|
||||
for key, expiry := range s.entries {
|
||||
|
||||
@@ -376,3 +376,73 @@ func TestBuildTOTPURI_ContainsIssuerAndSecret(t *testing.T) {
|
||||
t.Fatalf("issuer = %q, want OwnCord", query.Get("issuer"))
|
||||
}
|
||||
}
|
||||
|
||||
// ─── OC-0378: Restore / Unmark ──────────────────────────────────────────────
|
||||
|
||||
func TestPartialAuthStore_RestoreAfterConsume(t *testing.T) {
|
||||
store := auth.NewPartialAuthStore(time.Minute)
|
||||
token, err := store.Issue(7, "device", "203.0.113.7")
|
||||
if err != nil {
|
||||
t.Fatalf("Issue: %v", err)
|
||||
}
|
||||
store.RegisterFailure(token, 5) // Failures=1 must survive the round trip
|
||||
|
||||
claimed, ok := store.Consume(token)
|
||||
if !ok {
|
||||
t.Fatal("Consume: challenge not found")
|
||||
}
|
||||
if _, ok := store.Lookup(token); ok {
|
||||
t.Fatal("consumed token still resolves")
|
||||
}
|
||||
|
||||
store.Restore(token, claimed)
|
||||
got, ok := store.Lookup(token)
|
||||
if !ok {
|
||||
t.Fatal("restored token does not resolve")
|
||||
}
|
||||
if got != claimed {
|
||||
t.Fatalf("restored challenge = %+v, want %+v", got, claimed)
|
||||
}
|
||||
if got.Failures != 1 {
|
||||
t.Fatalf("Failures = %d, want 1 (restore keeps the count)", got.Failures)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPartialAuthStore_RestoreExpiredStaysGone(t *testing.T) {
|
||||
store := auth.NewPartialAuthStore(20 * time.Millisecond)
|
||||
token, err := store.Issue(7, "device", "203.0.113.7")
|
||||
if err != nil {
|
||||
t.Fatalf("Issue: %v", err)
|
||||
}
|
||||
claimed, ok := store.Consume(token)
|
||||
if !ok {
|
||||
t.Fatal("Consume: challenge not found")
|
||||
}
|
||||
time.Sleep(40 * time.Millisecond)
|
||||
|
||||
store.Restore(token, claimed)
|
||||
if _, ok := store.Lookup(token); ok {
|
||||
t.Fatal("an expired challenge came back to life")
|
||||
}
|
||||
}
|
||||
|
||||
func TestUsedTOTPCodeStore_UnmarkAllowsReuse(t *testing.T) {
|
||||
store := auth.NewUsedTOTPCodeStore()
|
||||
if !store.MarkUsed(1, "123456") {
|
||||
t.Fatal("first MarkUsed refused")
|
||||
}
|
||||
if store.MarkUsed(1, "123456") {
|
||||
t.Fatal("replay accepted before Unmark")
|
||||
}
|
||||
if !store.MarkUsed(2, "123456") {
|
||||
t.Fatal("another user's identical code refused")
|
||||
}
|
||||
|
||||
store.Unmark(1, "123456")
|
||||
if !store.MarkUsed(1, "123456") {
|
||||
t.Fatal("code still marked after Unmark")
|
||||
}
|
||||
if store.MarkUsed(2, "123456") {
|
||||
t.Fatal("Unmark for user 1 released user 2's code")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -71,9 +71,13 @@ func (d *DB) CreateOwnerIfEmpty(ctx context.Context, username, passwordHash stri
|
||||
return uid, nil
|
||||
}
|
||||
|
||||
// CreateUserWithInvite atomically consumes an invite and creates the user in
|
||||
// the same transaction so a failed registration does not burn the invite.
|
||||
func (d *DB) CreateUserWithInvite(ctx context.Context, username, passwordHash string, roleID int, inviteCode string) (int64, error) {
|
||||
// CreateUserWithInvite atomically consumes an invite, creates the user and
|
||||
// inserts the account's first session in one transaction, so a failure at any
|
||||
// step — the session insert included (OC-0376) — leaves no half-registered
|
||||
// account and does not burn the invite. sessionTokenHash must already be
|
||||
// hashed. The H-6 session cap needs no eviction here: the user has no sessions
|
||||
// yet.
|
||||
func (d *DB) CreateUserWithInvite(ctx context.Context, username, passwordHash string, roleID int, inviteCode, sessionTokenHash, device, ip string) (int64, error) {
|
||||
tx, err := d.writer.BeginTx(ctx, nil)
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("CreateUserWithInvite begin: %w", err)
|
||||
@@ -114,6 +118,9 @@ func (d *DB) CreateUserWithInvite(ctx context.Context, username, passwordHash st
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("CreateUserWithInvite last insert id: %w", err)
|
||||
}
|
||||
if _, err := insertSession(ctx, d.q.WithTx(tx), uid, sessionTokenHash, device, ip); err != nil {
|
||||
return 0, fmt.Errorf("CreateUserWithInvite create session: %w", err)
|
||||
}
|
||||
if err := tx.Commit(); err != nil {
|
||||
return 0, fmt.Errorf("CreateUserWithInvite commit: %w", err)
|
||||
}
|
||||
@@ -254,9 +261,15 @@ func (d *DB) CreateSession(ctx context.Context, userID int64, tokenHash, device,
|
||||
"user_id", userID, "err", err)
|
||||
}
|
||||
|
||||
return insertSession(ctx, d.q, userID, tokenHash, device, ip)
|
||||
}
|
||||
|
||||
// insertSession inserts one session row through q — d.q, or d.q.WithTx(tx)
|
||||
// when the row must commit with other writes (CreateUserWithInvite).
|
||||
func insertSession(ctx context.Context, q *dbgen.Queries, userID int64, tokenHash, device, ip string) (int64, error) {
|
||||
expiresAt := time.Now().Add(sessionTTL).UTC().Format(sessionTimeLayout)
|
||||
deviceCopy, ipCopy := device, ip
|
||||
res, err := d.q.InsertSession(ctx, dbgen.InsertSessionParams{
|
||||
res, err := q.InsertSession(ctx, dbgen.InsertSessionParams{
|
||||
UserID: userID,
|
||||
Token: tokenHash,
|
||||
Device: &deviceCopy,
|
||||
|
||||
@@ -756,7 +756,7 @@ func TestCreateUserWithInvite_Success(t *testing.T) {
|
||||
t.Fatalf("CreateInvite: %v", err)
|
||||
}
|
||||
|
||||
uid, err := database.CreateUserWithInvite(context.Background(), "newuser", "hash", 4, code)
|
||||
uid, err := database.CreateUserWithInvite(context.Background(), "newuser", "hash", 4, code, "sess-newuser", "test", "127.0.0.1")
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUserWithInvite: %v", err)
|
||||
}
|
||||
@@ -769,12 +769,16 @@ func TestCreateUserWithInvite_Success(t *testing.T) {
|
||||
if inv == nil || inv.Uses != 1 {
|
||||
t.Errorf("invite uses = %v, want 1", inv)
|
||||
}
|
||||
// The first session commits with the account (OC-0376).
|
||||
if sess, err := database.GetSessionByTokenHash(context.Background(), "sess-newuser"); err != nil || sess == nil || sess.UserID != uid {
|
||||
t.Errorf("session = %+v, %v; want a session for user %d", sess, err, uid)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCreateUserWithInvite_InvalidCode(t *testing.T) {
|
||||
database := openMigratedMemory(t)
|
||||
|
||||
_, err := database.CreateUserWithInvite(context.Background(), "baduser", "hash", 4, "nonexistent-code")
|
||||
_, err := database.CreateUserWithInvite(context.Background(), "baduser", "hash", 4, "nonexistent-code", "sess-bad", "test", "127.0.0.1")
|
||||
if err == nil {
|
||||
t.Error("expected error for invalid invite code")
|
||||
}
|
||||
@@ -787,7 +791,7 @@ func TestCreateUserWithInvite_RevokedInvite(t *testing.T) {
|
||||
code, _ := database.CreateInvite(context.Background(), creatorID, 0, nil)
|
||||
_ = database.RevokeInvite(context.Background(), code)
|
||||
|
||||
_, err := database.CreateUserWithInvite(context.Background(), "revokeduser", "hash", 4, code)
|
||||
_, err := database.CreateUserWithInvite(context.Background(), "revokeduser", "hash", 4, code, "sess-revoked", "test", "127.0.0.1")
|
||||
if err == nil {
|
||||
t.Error("expected error for revoked invite")
|
||||
}
|
||||
@@ -801,7 +805,7 @@ func TestCreateUserWithInvite_ExpiredInvite(t *testing.T) {
|
||||
pastTime := time.Now().Add(-1 * time.Hour)
|
||||
code, _ := database.CreateInvite(context.Background(), creatorID, 0, &pastTime)
|
||||
|
||||
_, err := database.CreateUserWithInvite(context.Background(), "expireduser", "hash", 4, code)
|
||||
_, err := database.CreateUserWithInvite(context.Background(), "expireduser", "hash", 4, code, "sess-expired", "test", "127.0.0.1")
|
||||
if err == nil {
|
||||
t.Error("expected error for expired invite")
|
||||
}
|
||||
|
||||
+74
-22
@@ -216,7 +216,10 @@ var (
|
||||
// Second factor.
|
||||
ErrTOTPChallengeInvalid = &authError{ErrUnauthorized, "invalid or expired two-factor challenge"}
|
||||
ErrTOTPSecretUnreadable = &authError{ErrInternal, "failed to verify two-factor code"}
|
||||
ErrTOTPCodeInvalid = &authError{ErrUnauthorized, "invalid two-factor code"}
|
||||
// ErrTOTPUnavailable is a store fault while loading the challenged user
|
||||
// (OC-0377): an outage, not a bad challenge, so no attempt is charged.
|
||||
ErrTOTPUnavailable = &authError{ErrInternal, "two-factor verification temporarily unavailable"}
|
||||
ErrTOTPCodeInvalid = &authError{ErrUnauthorized, "invalid two-factor code"}
|
||||
// ErrTOTPAlreadyEnabled is written by the transport as 409
|
||||
// TOTP_ALREADY_ENABLED, not the generic CONFLICT code.
|
||||
ErrTOTPAlreadyEnabled = &authError{ErrConflict, "disable 2FA before re-enabling"}
|
||||
@@ -293,19 +296,27 @@ func (s *AuthService) Register(ctx context.Context, in RegisterInput) (*AuthResu
|
||||
return nil, ErrPasswordHash
|
||||
}
|
||||
|
||||
// Atomically consume the invite and create the user so failed
|
||||
// registrations do not burn a valid invite code.
|
||||
uid, err := s.st.CreateUserWithInvite(ctx, in.Username, hash, int(permissions.MemberRoleID), in.InviteCode)
|
||||
// The session token is issued through the same path as login's, but it is
|
||||
// persisted by CreateUserWithInvite's transaction, so the account, the
|
||||
// invite use and the first session commit together: a fault at any step
|
||||
// — the session insert included — rolls the whole registration back and
|
||||
// burns nothing (OC-0376).
|
||||
var uid int64
|
||||
token, err := newSessionToken(func(tokenHash string) (err error) {
|
||||
uid, err = s.st.CreateUserWithInvite(ctx, in.Username, hash, int(permissions.MemberRoleID), in.InviteCode,
|
||||
tokenHash, in.Device, in.IP)
|
||||
return err
|
||||
})
|
||||
if err != nil {
|
||||
// UNIQUE constraint violation → duplicate username → 400.
|
||||
// Any other DB error → 500.
|
||||
// Any other error → 500.
|
||||
switch {
|
||||
case db.IsUniqueConstraintError(err):
|
||||
return nil, ErrRegistrationRejected
|
||||
case errors.Is(err, db.ErrNotFound):
|
||||
return nil, ErrRegistrationRejected
|
||||
default:
|
||||
slog.Error("CreateUserWithInvite failed", "err", err, "username", in.Username)
|
||||
slog.Error("register: account creation failed", "err", err, "username", in.Username)
|
||||
return nil, ErrRegistrationFailed
|
||||
}
|
||||
}
|
||||
@@ -314,12 +325,6 @@ func (s *AuthService) Register(ctx context.Context, in RegisterInput) (*AuthResu
|
||||
db.WriteAudit(context.WithoutCancel(ctx), s.st, uid, "user_register", "user", uid,
|
||||
"new account created via invite")
|
||||
|
||||
// Issue session.
|
||||
token, err := issueSession(ctx, s.st, uid, in.Device, in.IP)
|
||||
if err != nil {
|
||||
return nil, ErrSessionIssue
|
||||
}
|
||||
|
||||
user, err := s.st.GetUserByID(ctx, uid)
|
||||
if err != nil || user == nil {
|
||||
slog.Error("failed to fetch user after registration", "user_id", uid, "error", err)
|
||||
@@ -467,12 +472,27 @@ func (s *AuthService) authenticate(ctx context.Context, in LoginInput) (*db.User
|
||||
// VerifyTOTP completes a challenge Login started and issues the session,
|
||||
// bound to the login request's device and IP rather than this one's.
|
||||
func (s *AuthService) VerifyTOTP(ctx context.Context, partialToken, code string) (*AuthResult, error) {
|
||||
code = strings.TrimSpace(code)
|
||||
challenge, ok := s.partial.Lookup(partialToken)
|
||||
if !ok {
|
||||
return nil, ErrTOTPChallengeInvalid
|
||||
}
|
||||
|
||||
// Refuse an exhausted per-user budget before touching the store: the
|
||||
// read-only Check costs nothing and charges nothing, so rotating source
|
||||
// IPs cannot drive user reads and secret decryptions past the cap
|
||||
// (Codex P2 on PR #1454). The atomic Allow below still records the
|
||||
// attempt only once the store read succeeded (OC-0377).
|
||||
totpRateLimitKey := auth.Key("totp_fail", challenge.UserID)
|
||||
if !s.limiter.Check(totpRateLimitKey, totpFailureRateLimit, totpFailureWindow) {
|
||||
return nil, ErrTooManyAttempts
|
||||
}
|
||||
|
||||
user, secret, err := s.challengeSecret(ctx, challenge.UserID)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Atomically record this attempt and reject once the per-user failure cap
|
||||
// is reached. Recording up-front — rather than a read-only Check now and
|
||||
// Allow only on failure — closes a TOCTOU where many concurrent requests
|
||||
@@ -485,16 +505,14 @@ func (s *AuthService) VerifyTOTP(ctx context.Context, partialToken, code string)
|
||||
// multiplier exists for shared-NAT per-IP limits; scaling a per-user
|
||||
// threshold with it would hand a distributed attacker more guesses.
|
||||
// Mirrors loginUserFailureThreshold staying unscaled in authenticate.
|
||||
// The reservation sits after the store read above, as in authenticate,
|
||||
// so an outage does not consume attempts (OC-0377); it still precedes
|
||||
// the code compare, the check-then-act the up-front record closes.
|
||||
if !s.limiter.Allow(totpRateLimitKey, totpFailureRateLimit, totpFailureWindow) {
|
||||
return nil, ErrTooManyAttempts
|
||||
}
|
||||
|
||||
user, secret, err := s.challengeSecret(ctx, challenge.UserID)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
if !auth.VerifyTOTPCodeOnce(secret, strings.TrimSpace(code), time.Now().UTC(), user.ID, s.usedCodes) {
|
||||
if !auth.VerifyTOTPCodeOnce(secret, code, time.Now().UTC(), user.ID, s.usedCodes) {
|
||||
// The attempt was already recorded atomically up-front via
|
||||
// limiter.Allow; only the per-partial-token counter is advanced here.
|
||||
s.partial.RegisterFailure(partialToken, partialAuthMaxFailures)
|
||||
@@ -503,12 +521,28 @@ func (s *AuthService) VerifyTOTP(ctx context.Context, partialToken, code string)
|
||||
|
||||
s.limiter.Reset(ctx, totpRateLimitKey)
|
||||
|
||||
if _, ok := s.partial.Consume(partialToken); !ok {
|
||||
claimed, ok := s.partial.Consume(partialToken)
|
||||
if !ok {
|
||||
// The claim lost: a concurrent verify consumed the challenge (or it
|
||||
// expired) after this request marked its code. Release the code —
|
||||
// if the winner is mid-recovery (Consume → issueSession failed →
|
||||
// Restore), the restored token must not be stuck behind this mark
|
||||
// until the authenticator rolls over (Codex P2 on PR #1454).
|
||||
s.usedCodes.Unmark(user.ID, code)
|
||||
return nil, ErrTOTPChallengeInvalid
|
||||
}
|
||||
|
||||
token, err := issueSession(ctx, s.st, user.ID, challenge.Device, challenge.IP)
|
||||
if err != nil {
|
||||
// The second factor was verified; a store fault must not discard it
|
||||
// (OC-0378). The claim stays atomic and first — two concurrent
|
||||
// verifies can never both reach issueSession — so on failure put the
|
||||
// challenge back under the same token (the client still holds it) and
|
||||
// release the accepted code: the retry completes the login without
|
||||
// another password step. Code first, then token, so a concurrent
|
||||
// retry never finds a live token with a dead code.
|
||||
s.usedCodes.Unmark(user.ID, code)
|
||||
s.partial.Restore(partialToken, claimed)
|
||||
return nil, ErrSessionIssue
|
||||
}
|
||||
|
||||
@@ -522,7 +556,14 @@ func (s *AuthService) VerifyTOTP(ctx context.Context, partialToken, code string)
|
||||
// returns their decrypted TOTP secret.
|
||||
func (s *AuthService) challengeSecret(ctx context.Context, challengeUserID int64) (*db.User, string, error) {
|
||||
user, err := s.st.GetUserByID(ctx, challengeUserID)
|
||||
if err != nil || user == nil || user.TOTPSecret == nil {
|
||||
if err != nil {
|
||||
// GetUserByID answers (nil, nil) for an unknown user, so a non-nil
|
||||
// error is a store fault: report the outage, not a bad challenge
|
||||
// (OC-0377). VerifyTOTP records no attempt for it.
|
||||
slog.Error("verify-totp: GetUserByID failed", "err", err, "user_id", challengeUserID)
|
||||
return nil, "", ErrTOTPUnavailable
|
||||
}
|
||||
if user == nil || user.TOTPSecret == nil {
|
||||
return nil, "", ErrTOTPChallengeInvalid
|
||||
}
|
||||
|
||||
@@ -802,17 +843,28 @@ func (s *AuthService) revokeOtherSessionsAfterAuthChange(ctx context.Context, us
|
||||
|
||||
// ─── Helpers moved from api/auth_handler.go ──────────────────────────────────
|
||||
|
||||
func issueSession(ctx context.Context, st Store, userID int64, device, ip string) (string, error) {
|
||||
// newSessionToken generates a bearer token and hands its hash to persist,
|
||||
// which stores the session row — CreateSession, or CreateUserWithInvite's
|
||||
// transaction for a registration. The token is returned only once persist
|
||||
// succeeded.
|
||||
func newSessionToken(persist func(tokenHash string) error) (string, error) {
|
||||
token, err := auth.GenerateToken()
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
if _, err := st.CreateSession(ctx, userID, auth.HashToken(token), device, ip); err != nil {
|
||||
if err := persist(auth.HashToken(token)); err != nil {
|
||||
return "", err
|
||||
}
|
||||
return token, nil
|
||||
}
|
||||
|
||||
func issueSession(ctx context.Context, st Store, userID int64, device, ip string) (string, error) {
|
||||
return newSessionToken(func(tokenHash string) error {
|
||||
_, err := st.CreateSession(ctx, userID, tokenHash, device, ip)
|
||||
return err
|
||||
})
|
||||
}
|
||||
|
||||
func (s *AuthService) require2FAEnabled(ctx context.Context) (bool, error) {
|
||||
return getBooleanSetting(ctx, s.st, "require_2fa", false)
|
||||
}
|
||||
|
||||
@@ -0,0 +1,74 @@
|
||||
package service
|
||||
|
||||
// Codex P2 on PR #1454 (B3-9, OC-0378): a verify whose challenge claim
|
||||
// loses — a concurrent request consumed the challenge after this one marked
|
||||
// its code — must release that code. Otherwise, when the winner is
|
||||
// mid-recovery (Consume → issueSession failed → Restore), the restored token
|
||||
// is stuck behind the loser's mark until the authenticator rolls over.
|
||||
//
|
||||
// The interleaving is forced deterministically: the store's GetUserByID runs
|
||||
// between Lookup and the code check, so consuming the challenge from inside
|
||||
// it leaves the token absent exactly when this request reaches Consume.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/J3vb/OwnCord/Server/auth"
|
||||
"github.com/J3vb/OwnCord/Server/db"
|
||||
)
|
||||
|
||||
type lostClaimStore struct {
|
||||
Store
|
||||
beforeUserRead func()
|
||||
}
|
||||
|
||||
func (s *lostClaimStore) GetUserByID(ctx context.Context, id int64) (*db.User, error) {
|
||||
s.beforeUserRead()
|
||||
return s.Store.GetUserByID(ctx, id)
|
||||
}
|
||||
|
||||
func TestVerifyTOTP_LostClaimReleasesTheCode(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
database := newTestDB(t)
|
||||
uid, err := database.CreateUser(ctx, "racer", "$2a$12$x", 4)
|
||||
if err != nil {
|
||||
t.Fatalf("CreateUser: %v", err)
|
||||
}
|
||||
secret, err := auth.GenerateTOTPSecret()
|
||||
if err != nil {
|
||||
t.Fatalf("GenerateTOTPSecret: %v", err)
|
||||
}
|
||||
key := make([]byte, 32)
|
||||
enc, err := auth.EncryptTOTPSecret(key, secret)
|
||||
if err != nil {
|
||||
t.Fatalf("EncryptTOTPSecret: %v", err)
|
||||
}
|
||||
if err := database.UpdateUserTOTPSecret(ctx, uid, &enc); err != nil {
|
||||
t.Fatalf("UpdateUserTOTPSecret: %v", err)
|
||||
}
|
||||
|
||||
var (
|
||||
svc *AuthService
|
||||
token string
|
||||
)
|
||||
st := &lostClaimStore{Store: database, beforeUserRead: func() { svc.partial.Consume(token) }}
|
||||
svc = NewAuthService(st, auth.NewRateLimiter(), key, nil)
|
||||
token, err = svc.partial.Issue(uid, "device", "203.0.113.9")
|
||||
if err != nil {
|
||||
t.Fatalf("Issue: %v", err)
|
||||
}
|
||||
code, err := auth.GenerateTOTPCode(secret, time.Now().UTC())
|
||||
if err != nil {
|
||||
t.Fatalf("GenerateTOTPCode: %v", err)
|
||||
}
|
||||
|
||||
if _, err := svc.VerifyTOTP(ctx, token, code); !errors.Is(err, ErrTOTPChallengeInvalid) {
|
||||
t.Fatalf("VerifyTOTP error = %v, want ErrTOTPChallengeInvalid (the claim lost)", err)
|
||||
}
|
||||
if !svc.usedCodes.MarkUsed(uid, code) {
|
||||
t.Fatal("the losing claim left its code marked as used")
|
||||
}
|
||||
}
|
||||
@@ -81,7 +81,7 @@ type Store interface {
|
||||
GetUserByUsername(ctx context.Context, username string) (*db.User, error)
|
||||
CreateUser(ctx context.Context, username, passwordHash string, roleID int) (int64, error)
|
||||
CreateOwnerIfEmpty(ctx context.Context, username, passwordHash string, roleID int) (int64, error)
|
||||
CreateUserWithInvite(ctx context.Context, username, passwordHash string, roleID int, inviteCode string) (int64, error)
|
||||
CreateUserWithInvite(ctx context.Context, username, passwordHash string, roleID int, inviteCode, sessionTokenHash, device, ip string) (int64, error)
|
||||
UpdateUserProfile(ctx context.Context, userID int64, username string, avatar, displayName, about *string) error
|
||||
UpdateUserCustomStatus(ctx context.Context, userID int64, customStatus *string) error
|
||||
UpdateUserPassword(ctx context.Context, userID int64, newPasswordHash string) error
|
||||
|
||||
+17
-16
@@ -10,21 +10,21 @@ authority**.
|
||||
|
||||
## Active — these drive current work
|
||||
|
||||
| Plan | State |
|
||||
| ----------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| [beta-product-requirements-2026-08-23](beta-product-requirements-2026-08-23.md) | Approved beta scope, frozen. 57 `BPR-*` requirements. |
|
||||
| [repo-health-roadmap-2026-08-23](repo-health-roadmap-2026-08-23.md) | Phase order and gates, B0–B10. **B0, B1 and B2 complete** (HP-0, HP-1 and HP-2 all accepted); **B3 is next** — opens with the layout-refactor first slice. B3–B10 not started. **Amended 2026-08-28:** dated lines in B3–B10, a "Phase execution pattern" section, and a truthful status header. **Amended 2026-08-29:** B3/B7/B9 lines binding the layout-refactor supplement below; current slice updated for B2-2. |
|
||||
| [repo-health-issue-register-2026-08-23](repo-health-issue-register-2026-08-23.md) | 88 planning rows. Public-safe; not a replacement for the ledger. |
|
||||
| [beta-requirements-traceability-2026-08-23](beta-requirements-traceability-2026-08-23.md) | Requirement → phase → evidence map. No row is release-qualified. |
|
||||
| [b0-baseline-2026-08-25](b0-baseline-2026-08-25.md) | **Supersedes the roadmap's "current evidence snapshot."** B0 measurements and dispositions. |
|
||||
| [b1-repository-foundation-2026-08-25](b1-repository-foundation-2026-08-25.md) | **B1-0 through B1-8 all done.** B1 execution plan. Re-verifies every RL-\* claim against HEAD; several are refuted. |
|
||||
| [hp-0-scorecard-2026-08-25](hp-0-scorecard-2026-08-25.md) | **HP-0 accepted 2026-08-25.** The single baseline-acceptance artifact. Part-closes `R-08`. |
|
||||
| [hp-1-scorecard-2026-08-27](hp-1-scorecard-2026-08-27.md) | **HP-1 accepted 2026-08-27.** Structural-diff proofs for the flatten and module rename, plus the B1 exit gate. |
|
||||
| [b2-protocol-trust-compat-2026-08-28](b2-protocol-trust-compat-2026-08-28.md) | **B2 complete — HP-2 accepted 2026-08-29.** B2-0, B2-1, B2-8 done 2026-08-28; B2-2 (B2-3/B2-4 folded in), B2-5, B2-6, B2-7, B2-9 done 2026-08-29. Scorecard below. |
|
||||
| [hp-2-scorecard-2026-08-29](hp-2-scorecard-2026-08-29.md) | **HP-2 accepted 2026-08-29.** Seven questions answered with commands; B2 exit gate, nine conditions met (1 at the slim epoch scope, 4 with one E2EE gap disclaimed). Owner follow-ups that do not gate B3: BPR-051 reader line, SEC-01/SEC-04 advisory IDs. |
|
||||
| [b3-server-architecture-guardrails-2026-08-29](b3-server-architecture-guardrails-2026-08-29.md) | **B3 in progress from 2026-08-29.** Execution plan: B3-0 inventory → B3-1/B3-2 auth slice → HP-3 → lifecycle, hub options, `ws` split, families; guardrails and the alpha dataset beside the slice. B3-0 (inventory + `db-import-boundary` rule) and B3-1 (auth characterization) merged 2026-08-29; B3-2 (auth vertical slice) in review 2026-08-30. |
|
||||
| [hp-3-scorecard-2026-08-29](hp-3-scorecard-2026-08-29.md) | **HP-3 draft 2026-08-30, awaiting the owner's signature.** Five questions on the auth vertical slice answered with commands: frozen set green at every pre-squash SHA, `api` db importers 12 → 10, B2 contracts unchanged, the pattern written as D4 in server.md, guardrails as they exist. |
|
||||
| [audit-2026-08-19-remediation](audit-2026-08-19-remediation.md) | Phases 1–6 done 2026-08-20; **phase 7 pending**. Its header still reads "in progress 2026-08-19" — stale; the phase table is correct. |
|
||||
| Plan | State |
|
||||
| ----------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| [beta-product-requirements-2026-08-23](beta-product-requirements-2026-08-23.md) | Approved beta scope, frozen. 57 `BPR-*` requirements. |
|
||||
| [repo-health-roadmap-2026-08-23](repo-health-roadmap-2026-08-23.md) | Phase order and gates, B0–B10. **B0, B1 and B2 complete** (HP-0, HP-1 and HP-2 all accepted); **B3 is next** — opens with the layout-refactor first slice. B3–B10 not started. **Amended 2026-08-28:** dated lines in B3–B10, a "Phase execution pattern" section, and a truthful status header. **Amended 2026-08-29:** B3/B7/B9 lines binding the layout-refactor supplement below; current slice updated for B2-2. |
|
||||
| [repo-health-issue-register-2026-08-23](repo-health-issue-register-2026-08-23.md) | 88 planning rows. Public-safe; not a replacement for the ledger. |
|
||||
| [beta-requirements-traceability-2026-08-23](beta-requirements-traceability-2026-08-23.md) | Requirement → phase → evidence map. No row is release-qualified. |
|
||||
| [b0-baseline-2026-08-25](b0-baseline-2026-08-25.md) | **Supersedes the roadmap's "current evidence snapshot."** B0 measurements and dispositions. |
|
||||
| [b1-repository-foundation-2026-08-25](b1-repository-foundation-2026-08-25.md) | **B1-0 through B1-8 all done.** B1 execution plan. Re-verifies every RL-\* claim against HEAD; several are refuted. |
|
||||
| [hp-0-scorecard-2026-08-25](hp-0-scorecard-2026-08-25.md) | **HP-0 accepted 2026-08-25.** The single baseline-acceptance artifact. Part-closes `R-08`. |
|
||||
| [hp-1-scorecard-2026-08-27](hp-1-scorecard-2026-08-27.md) | **HP-1 accepted 2026-08-27.** Structural-diff proofs for the flatten and module rename, plus the B1 exit gate. |
|
||||
| [b2-protocol-trust-compat-2026-08-28](b2-protocol-trust-compat-2026-08-28.md) | **B2 complete — HP-2 accepted 2026-08-29.** B2-0, B2-1, B2-8 done 2026-08-28; B2-2 (B2-3/B2-4 folded in), B2-5, B2-6, B2-7, B2-9 done 2026-08-29. Scorecard below. |
|
||||
| [hp-2-scorecard-2026-08-29](hp-2-scorecard-2026-08-29.md) | **HP-2 accepted 2026-08-29.** Seven questions answered with commands; B2 exit gate, nine conditions met (1 at the slim epoch scope, 4 with one E2EE gap disclaimed). Owner follow-ups that do not gate B3: BPR-051 reader line, SEC-01/SEC-04 advisory IDs. |
|
||||
| [b3-server-architecture-guardrails-2026-08-29](b3-server-architecture-guardrails-2026-08-29.md) | **B3 in progress from 2026-08-29.** Execution plan: B3-0 inventory → B3-1/B3-2 auth slice → HP-3 → lifecycle, hub options, `ws` split, families; guardrails and the alpha dataset beside the slice. B3-0 (inventory + `db-import-boundary` rule) and B3-1 (auth characterization) merged 2026-08-29; B3-2 (auth vertical slice) merged 2026-08-30; B3-9 (five of the six B3-tagged findings; OC-0323 rides B3-8) done 2026-08-30, PR #1454 in review. |
|
||||
| [hp-3-scorecard-2026-08-29](hp-3-scorecard-2026-08-29.md) | **HP-3 draft 2026-08-30, awaiting the owner's signature.** Five questions on the auth vertical slice answered with commands: frozen set green at every pre-squash SHA, `api` db importers 12 → 10, B2 contracts unchanged, the pattern written as D4 in server.md, guardrails as they exist. |
|
||||
| [audit-2026-08-19-remediation](audit-2026-08-19-remediation.md) | Phases 1–6 done 2026-08-20; **phase 7 pending**. Its header still reads "in progress 2026-08-19" — stale; the phase table is correct. |
|
||||
|
||||
## Partially implemented
|
||||
|
||||
@@ -67,7 +67,8 @@ Planning documents are not trackers. Do not read a defect count out of one.
|
||||
| Phase order and gates | [repo-health-roadmap-2026-08-23](repo-health-roadmap-2026-08-23.md) |
|
||||
| Current measured baseline | [b0-baseline-2026-08-25](b0-baseline-2026-08-25.md) |
|
||||
|
||||
Ledger at 2026-08-29: **315 fixed / 59 open / 3 declined / 1 duplicate = 378**.
|
||||
Ledger at 2026-08-30: **320 fixed / 54 open / 3 declined / 1 duplicate = 378**
|
||||
(B3-9, PR #1454, closed the five B3-tagged records).
|
||||
All 38 open records still resolved to a live `file:line` at
|
||||
`5cc0888964e26276d1aca145e83270a2c1b9febd` when that sweep was run — it was a
|
||||
manual pass, not something a command reproduces. What the tooling does check:
|
||||
|
||||
@@ -216,8 +216,8 @@ authority over the leftovers listed below. B1 is unblocked.
|
||||
which also refutes its own header note: repository-settings writes were **not**
|
||||
blocked from the agent sandbox.
|
||||
- Step 8: individual adjudication of the 38 open `OC-*` records. The count was
|
||||
verified as **315 fixed / 59 open / 3 declined / 1 duplicate = 378**, matching
|
||||
the register, and a staleness pass confirmed **all 38 still resolve to a live
|
||||
verified as **320 fixed / 54 open / 3 declined / 1 duplicate = 378** (counts
|
||||
re-derived 2026-08-30 after B3-9), matching the register, and a staleness pass confirmed **all 38 still resolve to a live
|
||||
`file:line`** at this commit — none is superseded by later work, so all 38 are
|
||||
genuinely open (11 medium, 27 low, all from hunt `general-2026-08-22-b`).
|
||||
Deciding each one is bughunt-fix work, not B0 work. The duplicate pairs the
|
||||
|
||||
@@ -6,7 +6,9 @@
|
||||
verified at `bf7b886d`
|
||||
**Status:** in progress — plan merged 2026-08-29 (PR #1447 = `ad4defc2`);
|
||||
B3-0 merged 2026-08-29 (PR #1448 = `d383d8c7`; closes entry-gate item 3);
|
||||
B3-1 merged 2026-08-29 (PR #1449 = `71d867cb`); B3-2 in progress 2026-08-30.
|
||||
B3-1 merged 2026-08-29 (PR #1449 = `71d867cb`); B3-2 merged 2026-08-30
|
||||
(PR #1450 = `75d64dd4`); B3-9 done 2026-08-30 (PR #1454 to `dev`, squash SHA
|
||||
recorded at merge; OC-0323 rides B3-8).
|
||||
Update this line, not only the step table, when a step lands.
|
||||
|
||||
Primary inputs:
|
||||
@@ -31,19 +33,19 @@ surface to it.
|
||||
|
||||
## Steps at a glance
|
||||
|
||||
| Step | What | Size | Parallel with |
|
||||
| -------- | ---------------------------------------------------------------------------------------------------------------------------------- | -------- | ---------------------------------------------------------------- |
|
||||
| **B3-0** | Boundary inventory: every upper-layer `db` import with a disposition; hub lifecycle; before-graph — **DONE 2026-08-29 (PR #1448)** | 1–2 days | B3-6, B3-7 |
|
||||
| **B3-1** | Auth characterization tests — enumeration, sentinels, sessions, TOTP, rate limits, failure paths — **DONE 2026-08-29 (PR #1449)** | 1 day | B3-6, B3-7 |
|
||||
| **B3-2** | The auth vertical slice (S-10): route → `service.AuthService` → `db`, behaviour-neutral | 2–3 days | B3-6, B3-7 |
|
||||
| **HP-3** | First vertical-slice review — scorecard | — | — |
|
||||
| **B3-3** | Lifecycle extraction: `main.go` → `internal/app/` with one composite close contract | 1–2 days | B3-4 |
|
||||
| **B3-4** | Hub constructor options (S-11): required collaborators validated at construction | 1 day | after B3-3 |
|
||||
| **B3-5** | `ws` in-package split (S-08): responsibilities into named files, pure moves + adjacent rewrites | 2–3 days | after B3-3/B3-4 |
|
||||
| **B3-6** | Guardrails: coverage floor (S-06), hub simulation + fault transport + fuzz seeds, benchmarks, rules | 3–4 days | B3-0..B3-2 |
|
||||
| **B3-7** | Alpha-shaped test dataset: seed profile + anonymised `v1.2.0-alpha.4` snapshot | 1–2 days | B3-0..B3-2 |
|
||||
| **B3-8** | Remaining domain families behind services (S-09), one PR each; S-03/S-04 fold into the channel family | spread | after HP-3, per-family |
|
||||
| **B3-9** | The B3-tagged findings: OC-0323, OC-0345, OC-0346 + B3-1's OC-0376, OC-0377, OC-0378 (test-first, `bughunt-fix` shape) | 1 day | OC-0345/0346: any; OC-0323: with B3-8; OC-0376..0378: after B3-2 |
|
||||
| Step | What | Size | Parallel with |
|
||||
| -------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | ---------------------------------------------------------------- |
|
||||
| **B3-0** | Boundary inventory: every upper-layer `db` import with a disposition; hub lifecycle; before-graph — **DONE 2026-08-29 (PR #1448)** | 1–2 days | B3-6, B3-7 |
|
||||
| **B3-1** | Auth characterization tests — enumeration, sentinels, sessions, TOTP, rate limits, failure paths — **DONE 2026-08-29 (PR #1449)** | 1 day | B3-6, B3-7 |
|
||||
| **B3-2** | The auth vertical slice (S-10): route → `service.AuthService` → `db`, behaviour-neutral — **DONE 2026-08-30 (PR #1450)** | 2–3 days | B3-6, B3-7 |
|
||||
| **HP-3** | First vertical-slice review — scorecard | — | — |
|
||||
| **B3-3** | Lifecycle extraction: `main.go` → `internal/app/` with one composite close contract | 1–2 days | B3-4 |
|
||||
| **B3-4** | Hub constructor options (S-11): required collaborators validated at construction | 1 day | after B3-3 |
|
||||
| **B3-5** | `ws` in-package split (S-08): responsibilities into named files, pure moves + adjacent rewrites | 2–3 days | after B3-3/B3-4 |
|
||||
| **B3-6** | Guardrails: coverage floor (S-06), hub simulation + fault transport + fuzz seeds, benchmarks, rules | 3–4 days | B3-0..B3-2 |
|
||||
| **B3-7** | Alpha-shaped test dataset: seed profile + anonymised `v1.2.0-alpha.4` snapshot | 1–2 days | B3-0..B3-2 |
|
||||
| **B3-8** | Remaining domain families behind services (S-09), one PR each; S-03/S-04 fold into the channel family | spread | after HP-3, per-family |
|
||||
| **B3-9** | The B3-tagged findings: OC-0323, OC-0345, OC-0346 + B3-1's OC-0376, OC-0377, OC-0378 (test-first, `bughunt-fix` shape) — **DONE 2026-08-30 (PR #1454; OC-0323 → B3-8)** | 1 day | OC-0345/0346: any; OC-0323: with B3-8; OC-0376..0378: after B3-2 |
|
||||
|
||||
Order: B3-0 → B3-1 → B3-2 → **HP-3** → B3-3 → B3-4 → B3-5 → B3-8. B3-6, B3-7
|
||||
and B3-9 run beside the slice (roadmap "Safe parallelism": guardrail tooling
|
||||
@@ -360,7 +362,7 @@ rewrite, remove old path — with B3-1's tests green after every commit.
|
||||
Exit: HP-3.
|
||||
|
||||
**Evidence, 2026-08-30** — branch `feat/b3-2-auth-slice` from `dev`
|
||||
`71d867cb`; PR #1450 to `dev` (squash SHA recorded at merge).
|
||||
`71d867cb`; PR #1450 to `dev`, merged 2026-08-30 as `75d64dd4`.
|
||||
|
||||
- **Pre-squash SHAs**, one per numbered item. For each, the characterization
|
||||
file was run against that exact tree in a detached worktree
|
||||
@@ -722,6 +724,133 @@ They are fixed here, after B3-2 has moved the orchestration into
|
||||
Any of the six that cannot land in B3 is re-tagged in HP-3's scorecard with
|
||||
the reason.
|
||||
|
||||
**Evidence, 2026-08-30** — branch `fix/b3-9-findings` from `dev` `75d64dd4`;
|
||||
PR #1454 to `dev` (squash SHA recorded at merge). Five of the six findings
|
||||
land here; **OC-0323 is not in this PR** — it rides B3-8's message/read-state
|
||||
family (the fix is a shared read-state query, the family's own
|
||||
characterization file is the right home) and stays `open`, low, tagged B3.
|
||||
|
||||
- **Shape.** Test-first, one fix commit per finding (the RED test and the
|
||||
fix, plus that finding's `// known:` row flipped to the fixed behaviour in
|
||||
the same commit), then one ledger commit closing all five with their
|
||||
pre-squash SHAs — a commit cannot cite its own SHA, and the
|
||||
`check:docs` count claims can only be re-derived once, so the flips and
|
||||
the four count-carrying documents move together in `8fdb51ed`.
|
||||
Revert-proof per finding: the fix's source hunk reverse-applied onto the
|
||||
committed tree, the test run RED, restored, run GREEN (lines below); then
|
||||
`.superpowers/verify-fixes.mjs` independently on the four untagged
|
||||
commits: `fb1afb8a`, `be37d7ee`, `85d86dc7` PASS at the branch head (red then green on server); `f7015809` PASS at its own tree (`git worktree add … f7015809`) — its reverse hunk no longer applies over `be37d7ee`, which edits the adjacent lines, so the script defers to a hand run there; both runs from a detached worktree so the script's reverse-applies never touched the working tree. The otel-tagged OC-0346 test is invisible to
|
||||
that script (it runs the untagged suite), so its proof is the hand run.
|
||||
- **Gates**, before every commit, as one `set -e` script with no pipes
|
||||
(the first ledger flip of the day was made over a red `check:docs` hidden
|
||||
behind `| grep` — dropped before push, obs #110): the four build-tag
|
||||
variants, `go vet ./...` and `go vet -tags otel ./api/`,
|
||||
`go test -race ./...`, `go test -tags deadlock -count=1 ./ws/`,
|
||||
`golangci-lint run` (0 issues), `sqlc generate` and `genprotocol` drift
|
||||
(clean — OC-0376 adds no query), the otel-tagged test, the frozen
|
||||
characterization file, `check:docs`, `check:hygiene`,
|
||||
`render-ledger.mjs --check`.
|
||||
`go test -count=1 -run TestAuthCharacterization ./api/` green at every
|
||||
commit: `3b7716e2`, `775eba50`, `fb1afb8a`, `f7015809`, `be37d7ee`,
|
||||
`85d86dc7`, `4b304f6e`, `8fdb51ed`, `1be1eea4` (this block's
|
||||
own commit is docs-only).
|
||||
|
||||
| Finding | Commit | Test | RED line (fix reverted) | GREEN |
|
||||
| ------- | ---------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------- |
|
||||
| OC-0346 | `775eba50` | `api/recoverer_otel_test.go` `TestRecoverer_PanicLogCarriesTraceID` — `go test -tags otel` | `recoverer_otel_test.go:73: panic record trace_id = "", want the span's 32-hex trace id` (req_id present, 500 returned) | `ok api 1.918s` |
|
||||
| OC-0345 | `fb1afb8a` | `admin/middleware_and_spawn_test.go` `TestOwnerOnlyMiddleware_RoleLookupFailureIs503` | `middleware_and_spawn_test.go:564: status = 403, want 503` | all 8 `TestOwnerOnlyMiddleware_*` `ok admin 1.360s` |
|
||||
| OC-0377 | `f7015809` | `VerifyTOTPFailurePaths/user lookup fails -> 500, challenge kept, attempt not counted` | `auth_characterization_test.go:787: status = 401, want 500` | `ok api 3.281s` |
|
||||
| OC-0378 | `be37d7ee` | `VerifyTOTPFailurePaths/session insert fails -> 500, the challenge and the code survive` + `auth/totp_test.go` `TestPartialAuthStore_Restore*`, `TestUsedTOTPCodeStore_UnmarkAllowsReuse` | `auth_characterization_test.go:843: retry status = 401, want 200` ("invalid or expired two-factor challenge") | `ok auth 0.902s`, `ok api 1.522s` |
|
||||
| OC-0376 | `85d86dc7` | `RegisterPolicyAndFailurePaths/session insert fails -> 500, nothing committed` + `db/coverage_boost_test.go` `TestCreateUserWithInvite_*` | ``auth_characterization_test.go:462: user row exists after the session insert failed` / `:465: invite use_count = 1, want 0 (transaction rolled back)` / `:467: body = {"INTERNAL_ERROR", "failed to create session"}, want {"INTERNAL_ERROR", "registration failed — please try again"}`` | ``ok db 0.578s` (4/4 `TestCreateUserWithInvite_*`, the happy path now asserts the session row), `ok api 1.487s`` |
|
||||
|
||||
- **Negative controls on the exact branch** (HP-2 obs #96): OC-0377 with the
|
||||
limiter reservation moved back ahead of the store read →
|
||||
`auth_characterization_test.go:799: status = 429, want 401` (the faulted
|
||||
attempt was counted); OC-0378 with `Restore` but no `Unmark` →
|
||||
`:843: retry status = 401 … "invalid two-factor code"` (the replay
|
||||
refusal). Both mutations reverted, rows green again.
|
||||
- **What changed, per finding.**
|
||||
1. OC-0346 — `routerMiddleware`: `telemetry.HTTPMiddleware()` mounted
|
||||
ahead of `recoverer`; request-id binding, security headers and the body
|
||||
cap keep their relative positions (the file comment now names tracing
|
||||
before recovery as part of the ordering property). The test drives a
|
||||
panicking handler through the real stack with a real tracer provider
|
||||
(`Exporter: "prometheus"`, no network) and reads the slog record.
|
||||
2. OC-0345 — `ownerOnlyMiddleware`: `err != nil` → log + 503
|
||||
`SERVICE_UNAVAILABLE` "authorization service temporarily unavailable";
|
||||
`role == nil` stays 403 "role not found". Not switched to the context
|
||||
role: `_RoleNotFound` and `_OwnerPassesThrough` inject only
|
||||
`adminUserKey` and are untouched. The new test is whitebox because the
|
||||
perimeter reads the role first and would answer its own 503.
|
||||
3. OC-0377 — `service.ErrTOTPUnavailable` (`ErrInternal`, "two-factor
|
||||
verification temporarily unavailable"); `challengeSecret` splits the
|
||||
store error from nil-user/nil-secret; `limiter.Allow(totp_fail…)` moves
|
||||
after the store read (authenticate's rule) and still precedes the code
|
||||
compare. The row proves "not counted" with ten wrong codes → 401 and an
|
||||
eleventh → 429. `per-user failure cap spans challenges` unchanged.
|
||||
4. OC-0378 — claim stays atomic and first; on `issueSession` failure
|
||||
`usedCodes.Unmark(user, code)` then `partial.Restore(token, claimed)`
|
||||
(code first, so a concurrent retry never finds a live token with a dead
|
||||
code). `auth.PartialAuthStore.Restore` keeps expiry and failure count
|
||||
(an expired challenge stays gone — leaf test); `UsedTOTPCodeStore.Unmark`
|
||||
is per (user, code). The same token and the same code then answer 200.
|
||||
5. OC-0376 — option **B** (atomic). The client (`Client/src/main.ts`
|
||||
`onRegister`) passes `result.token` straight to `wirePostAuth` with no
|
||||
token-less branch, so option A (201 without a token) would have needed
|
||||
client work; B is one `Store` method: `CreateUserWithInvite` takes the
|
||||
session token hash, device and IP and inserts the first session inside
|
||||
its transaction through `db.insertSession(ctx, d.q.WithTx(tx), …)` — the
|
||||
helper `CreateSession` now shares — no query or migration change (sqlc
|
||||
drift clean). `Register` generates the token before the transaction;
|
||||
the H-6 cap needs no eviction for a user with no sessions. A session
|
||||
insert fault now answers 500 "registration failed — please try again"
|
||||
with no user row and `use_count` 0.
|
||||
- **Codex round** (`1be1eea4`, two P2s on `VerifyTOTP`, both verified
|
||||
against the code and fixed test-first): (1) an exhausted `totp_fail`
|
||||
window is now refused by the read-only `Check` before the store read and
|
||||
the secret decrypt, so rotating IPs cannot drive that work past the cap;
|
||||
the atomic `Allow` still records after the read (OC-0377 intact) —
|
||||
`api/totp_cap_before_store_test.go`, RED `status = 500, want 429`. (2) A
|
||||
verify whose claim loses at `Consume` releases the code it marked, so a
|
||||
winner mid-recovery (`Restore`) is not left with a live token behind a
|
||||
dead code — `service/auth_lost_claim_test.go` forces the interleaving
|
||||
through the store's `GetUserByID`, RED `the losing claim left its code
|
||||
marked as used`. Codex's security review was refused by its usage limit
|
||||
(re-requested once).
|
||||
- **Frozen set.** `auth_characterization_test.go` changed at exactly the
|
||||
three `// known:` rows (OC-0376 ~:452, OC-0377 ~:780, OC-0378 ~:812); no
|
||||
fourth row moved. `auth_handler_test.go`, `totp_handler_test.go`,
|
||||
`auth_handler_delete_broadcast_test.go`: `git diff 75d64dd4 -- <file>`
|
||||
empty.
|
||||
- **Ledger diff** (`8fdb51ed`): OC-0345, OC-0346, OC-0376, OC-0377,
|
||||
OC-0378 `open` → `fixed` with `fix.commit`, `fix.test`,
|
||||
`fix.revertProof: pass`; totals **315 fixed / 59 open** → **320 fixed /
|
||||
54 open** (3 declined, 1 duplicate, 378). The four count-carrying
|
||||
documents (`docs/plans/README.md`, `hp-0-scorecard`, `issue-register`,
|
||||
`b0-baseline`) re-derived around every number: 54 open = 1 high, 12
|
||||
medium, 41 low; hunts 27 / 26 / 1 (the three `b3-1` records closed);
|
||||
53 of the 54 resolve (OC-0323 still the exception); Client 33 / Server 21.
|
||||
Issue-register rows OC-0345 and OC-0346 marked fixed with this PR.
|
||||
- **Coverage** (statements; cover-profile blocks merged per file by range
|
||||
with max count, then summed — the B3-2 method): `-coverpkg=./api/,./service/
|
||||
./api/ ./service/` at `1be1eea4`:
|
||||
|
||||
| File | B3-2 (`fe1d11b8`) | B3-9 (`1be1eea4`) |
|
||||
| --------------------- | ------------------- | ------------------- |
|
||||
| `api/auth_handler.go` | 98/114 = 86.0% | 98/114 = 86.0% |
|
||||
| `api/totp_handler.go` | 54/60 = 90.0% | 54/60 = 90.0% |
|
||||
| `service/auth.go` | 240/253 = 94.9% | 253/266 = 95.1% |
|
||||
| **slice** | **392/427 = 91.8%** | **405/440 = 92.0%** |
|
||||
|
||||
At `85d86dc7` the slice measured 398/434 = 91.7%: OC-0376 gave `Register`
|
||||
its own `auth.GenerateToken` failure branch — unreachable (crypto/rand),
|
||||
and a duplicate of the one `issueSession` already carried — so one new
|
||||
statement was uncovered while covered statements rose 392 → 398. Commit
|
||||
`4b304f6e` folds token generation into one `newSessionToken`
|
||||
helper that both `issueSession` and `Register` persist through (behaviour
|
||||
identical: the characterization file green before and after), which is
|
||||
the row above. Measured again at `4b304f6e`: 402/437 = 92.0%, `service/auth.go` 250/263 = 95.1%; the Codex round's two branches (`Check`, the lost-claim `Unmark`) are covered by their own tests, giving the table's figures. The handler files are untouched by B3-9 and keep B3-2's numbers.
|
||||
|
||||
## Exit gate
|
||||
|
||||
The roadmap's six conditions, with the evidence each maps to:
|
||||
|
||||
@@ -63,29 +63,31 @@ Nothing here is a B1 blocker.
|
||||
|
||||
**No confirmed issue blocks B1.**
|
||||
|
||||
Open ledger, re-verified 2026-08-29:
|
||||
Open ledger, re-verified 2026-08-29; counts re-derived 2026-08-30 after B3-9
|
||||
(PR #1454) closed `OC-0345`, `OC-0346`, `OC-0376`, `OC-0377`, `OC-0378`:
|
||||
|
||||
| Status | Count |
|
||||
| --------- | ------- |
|
||||
| fixed | 315 |
|
||||
| open | **59** |
|
||||
| fixed | 320 |
|
||||
| open | **54** |
|
||||
| declined | 3 |
|
||||
| duplicate | 1 |
|
||||
| **total** | **378** |
|
||||
|
||||
Of the 59 open records:
|
||||
Of the 54 open records:
|
||||
|
||||
- **1 high, 12 medium, 46 low. Zero critical.** The high is `OC-0350`, an
|
||||
- **1 high, 12 medium, 41 low. Zero critical.** The high is `OC-0350`, an
|
||||
admin-panel login defect raised by the 2026-08-29 hunt and not yet phased.
|
||||
- Four hunts: 29 from `general-2026-08-22-b`, 26 from `general-2026-08-29`,
|
||||
3 from `b3-1-auth-characterization-2026-08-29` (defects the auth
|
||||
characterization rows pin as-is, assigned to B3-9), 1 from
|
||||
`b2-1-fixture-capture-2026-08-28`.
|
||||
- **58 of the 59 resolve to a live `file:line`** — 0 dead paths across all 378
|
||||
records, re-checked 2026-08-29. `OC-0323` is the exception: its line drifted
|
||||
past end of file when B2 work shortened `Server/service/channel.go`, so it
|
||||
needs re-pointing before it is fixed.
|
||||
- 33 sit under `Client/`, 26 under `Server/`.
|
||||
- Three hunts: 27 from `general-2026-08-22-b`, 26 from `general-2026-08-29`,
|
||||
1 from `b2-1-fixture-capture-2026-08-28`. The three
|
||||
`b3-1-auth-characterization-2026-08-29` records (defects the auth
|
||||
characterization rows pinned as-is) were fixed in B3-9 on 2026-08-30.
|
||||
- **53 of the 54 resolve to a live `file:line`** — 0 dead paths across all 378
|
||||
records, re-checked 2026-08-29; the five B3-9 closed were live then and are
|
||||
fixed now. `OC-0323` is the exception: its line drifted past end of file
|
||||
when B2 work shortened `Server/service/channel.go`, so it needs re-pointing
|
||||
before it is fixed (B3-8, with the message/read-state family).
|
||||
- 33 sit under `Client/`, 21 under `Server/`.
|
||||
- **None of the 2026-08-22 records is assigned to B1**; their register phases
|
||||
span B2–B10. The 2026-08-29 records are not yet phased in the register.
|
||||
|
||||
|
||||
@@ -76,14 +76,15 @@ together as if each row were a unique defect:
|
||||
|
||||
| Status | Count |
|
||||
| --------- | ------: |
|
||||
| Fixed | 315 |
|
||||
| Open | 59 |
|
||||
| Fixed | 320 |
|
||||
| Open | 54 |
|
||||
| Declined | 3 |
|
||||
| Duplicate | 1 |
|
||||
| **Total** | **378** |
|
||||
|
||||
The rows below cover `OC-0311`–`OC-0348` from the 2026-08-22 hunt, nine of
|
||||
which have since been fixed; `OC-0349`–`OC-0375` are recorded in the ledger and
|
||||
The rows below cover `OC-0311`–`OC-0348` from the 2026-08-22 hunt, eleven of
|
||||
which have since been fixed (the last two, `OC-0345` and `OC-0346`, by B3-9 in
|
||||
PR #1454); `OC-0349`–`OC-0375` are recorded in the ledger and
|
||||
are not yet enumerated here. Closing a planning row does not close an
|
||||
`OC-*` record: the implementation, regression test, focused verification, full
|
||||
required gates, and ledger update must land together.
|
||||
@@ -102,46 +103,46 @@ required gates, and ledger update must land together.
|
||||
The wording below is intentionally concise. The ledger contains the detailed
|
||||
evidence, reproduction, and suggested fix for each record.
|
||||
|
||||
| ID | Sev | Area | Public-safe defect summary | Phase | Required closure evidence |
|
||||
| ------- | ------ | ----------------------- | --------------------------------------------------------------------------------------------------------------------------- | ------ | ------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| OC-0311 | Medium | Client voice/E2EE | A leave event from another readable voice channel can mutate the active call's peer-key state. | B2/B7 | Scope leave handling to the active channel and cover reordered leave/join/replay sequences. |
|
||||
| OC-0312 | Medium | Client PTT | Binding push-to-talk during a call can clear mute ownership before the deferred mute applies, leaving PTT unusable. | B7 | Preserve the PTT ownership transition atomically and test mid-call binding/restart. |
|
||||
| OC-0313 | Medium | Client profiles | Legacy per-user volume fallback is repeatedly copied across server profiles instead of being consumed once. | B4/B7 | One-time scoped migration, legacy-key removal, and cross-server isolation tests. |
|
||||
| OC-0314 | Medium | Client identity | The client discards the server's partial-success warning when a credential change succeeds but session revocation does not. | B4/B9 | Surface warnings for password/TOTP changes with an action to review sessions; test all affected endpoints. |
|
||||
| OC-0315 | Medium | Client replay | Replay-gate timestamps mix naive UTC server values with local wall-clock parsing. | B2/B7 | One UTC parsing contract and timezone-varied replay boundary tests. |
|
||||
| OC-0316 | Medium | Server/client E2EE | WebSocket resume restores peer public keys but not a room key rotated during the outage. | B2/B7 | Resume re-establishes the current room key and the security indicator cannot claim success prematurely; rotation/outage test passes. |
|
||||
| OC-0317 | Medium | Client DM state | The replay path can regress a DM's `lastMessageId`, undermining duplicate-count protection. | B2/B7 | Monotonic last-message updates with duplicate, out-of-order, and reconnect tests. |
|
||||
| OC-0318 | Medium | Server plugins | Install-time and restart-time plugin manifest precedence differs between JSON and TOML. | B2 | One canonical manifest contract or explicit ambiguity rejection; install/restart parity test. |
|
||||
| OC-0319 | Medium | Client accessibility | The Large Font preference is overridden by a higher-priority inline font-size value. | B9 | Verified text-scale change across restart, zoom, responsive layouts, and accessibility checks. |
|
||||
| OC-0320 | Medium | Server updater | Server self-update selects Linux AMD64 independently of the running architecture. | B6/B10 | Architecture-aware manifest selection and signed update/rollback smoke on every supported server target. |
|
||||
| OC-0321 | Medium | Server TOTP | A TOTP key-file read failure can be treated as absence and lead to key replacement. | B4 | Generate only on confirmed non-existence; all other read errors fail closed without modifying the file. |
|
||||
| OC-0322 | Low | Client connection | TypeScript host validation accepts a hostname form rejected by the native proxy. | B2/B7 | Shared validation corpus produces identical browser, desktop, and Rust decisions. |
|
||||
| OC-0323 | Low | Server unread state | Mark-read/channel-focus can overwrite a mention count from a newer message using a stale snapshot. | B3/B5 | Atomic/monotonic read-state update with concurrent-message regression coverage. |
|
||||
| OC-0324 | Low | Server auth | Login rate-limit identity folding differs from SQLite account lookup semantics. | B4 | Account lookup and limiter use one tested canonical identity rule, including Unicode collision cases. |
|
||||
| OC-0325 | Low | Client search | Search results parse naive UTC timestamps as local time. | B7/B9 | Shared UTC parser and timezone/day-boundary rendering tests. |
|
||||
| OC-0326 | Low | Client pins | Pinned-message timestamps parse naive UTC values as local time. | B7/B9 | Shared UTC parser and timezone/day-boundary rendering tests. |
|
||||
| OC-0327 | Low | Server voice moderation | Server mute/deafen also affects screen-share audio contrary to the product contract. | B5 | Effective moderation applies only to intended media sources; SFU and client-policy tests agree. |
|
||||
| OC-0328 | Low | Client unread state | Channel badges lack the message-ID replay guard already used by DMs. | B2/B7 | Monotonic channel replay guard with duplicate/out-of-order/reconnect tests. |
|
||||
| OC-0329 | Low | Client privacy | Legacy DM profile notes fall back across servers indefinitely. | B4/B7 | One-time server-scoped migration, old-key removal, and cross-server privacy test. |
|
||||
| OC-0330 | Low | Client pins | Pinned messages discard author identity and therefore cannot resolve nicknames. | B7/B9 | Preserve author ID and render the same display identity as ordinary messages. |
|
||||
| OC-0331 | Low | Server admin UI | API-token Created/Last Used values parse naive UTC timestamps as local time. | B6/B9 | Shared UTC contract and timezone/day-boundary admin tests. |
|
||||
| OC-0332 | Low | Client updater | Bare IPv6 server addresses produce an invalid updater URL. | B6/B10 | Central URL builder brackets IPv6 literals and passes domain/IPv4/IPv6/update smoke tests. |
|
||||
| OC-0333 | Low | Client voice UI | Voice-roster render identity does not change when a participant is renamed mid-call. | B7/B9 | Reactive identity signature and rename-in-call test. |
|
||||
| OC-0334 | Low | Client PTT | Escape closes Settings and can simultaneously be saved as the captured PTT key. | B7/B9 | Escape cancels capture without persistence; teardown and timeout paths are tested. |
|
||||
| OC-0335 | Low | Client lifecycle | Each Add Server modal retains listeners and its removed subtree for the connect-page lifetime. | B7 | Modal-owned abort lifecycle; repeated open/close instrumentation shows no accumulation. |
|
||||
| OC-0336 | Low | Client lifecycle | Server-profile rows re-register page-lifetime listeners on every render. | B7 | Row/render ownership prevents accumulation under repeated updates and teardown. |
|
||||
| OC-0337 | Low | Server replay | Cold-tier voice replay truncation can discard the newest events and reconstruct the wrong roster. | B2/B3 | Ordered, bounded replay retains the correct window; boundary/resume tests reconstruct the authoritative roster. |
|
||||
| OC-0338 | Low | Server plugins | TOML plugin manifests can omit configured memory and CPU resource limits. | B2/B3 | Explicit TOML mapping and JSON/TOML resource-limit parity tests. |
|
||||
| OC-0339 | Low | Server config | A valid but empty configuration section is reported as an unknown ineffective key. | B6 | Empty known sections are accepted; true unknown keys remain actionable and tested. |
|
||||
| OC-0340 | Low | Server CLI | A negative API-token expiry can create a token that never expires. | B4/B6 | CLI and HTTP share positive-expiry validation; negative/zero/boundary tests fail safely. |
|
||||
| OC-0341 | Low | Server CLI | A numeric token label cannot be revoked because parsing commits to the ID path. | B4/B6 | Unambiguous ID/label selection or safe fallback with numeric-label regression tests. |
|
||||
| OC-0342 | Low | Client voice UI | Voice avatar letter/color derives from username while the adjacent label may be a nickname. | B9 | Avatar and label consistently derive from the displayed identity. |
|
||||
| OC-0343 | Low | Desktop shell | Clicking the tray icon can hide a minimized window instead of restoring it. | B7/B9 | Minimized windows unminimize and focus; only visible, non-minimized windows toggle hidden. |
|
||||
| OC-0344 | Low | Server TLS | Automatic HTTP-to-HTTPS redirect assumes port 443 instead of the configured HTTPS endpoint. | B6 | Redirect derives the configured public origin/port and passes default/custom/domain/IP tests. |
|
||||
| OC-0345 | Low | Server owner auth | Owner middleware repeats a role read and maps a transient read failure to forbidden. | B3/B4 | Reuse the authenticated context and preserve correct unavailable/unauthorized distinctions in failure tests. |
|
||||
| OC-0346 | Low | Server telemetry | Panic recovery reads trace context before tracing middleware creates it. | B3/B6 | Middleware order gives recoveries the active trace ID; panic-path structured-log test passes. |
|
||||
| OC-0347 | Low | Client DM voice UI | A DM call label reads but does not subscribe to DM state, so it remains stale. | B7/B9 | Subscribe to the owning state and test mid-call rename/update. |
|
||||
| OC-0348 | Low | Client presence | The online-count header includes the local invisible user while the member list presents that user as offline. | B7/B9 | Count and list share one visibility policy with invisible-status regression tests. |
|
||||
| ID | Sev | Area | Public-safe defect summary | Phase | Required closure evidence |
|
||||
| ------- | ------ | ----------------------- | --------------------------------------------------------------------------------------------------------------------------- | ------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| OC-0311 | Medium | Client voice/E2EE | A leave event from another readable voice channel can mutate the active call's peer-key state. | B2/B7 | Scope leave handling to the active channel and cover reordered leave/join/replay sequences. |
|
||||
| OC-0312 | Medium | Client PTT | Binding push-to-talk during a call can clear mute ownership before the deferred mute applies, leaving PTT unusable. | B7 | Preserve the PTT ownership transition atomically and test mid-call binding/restart. |
|
||||
| OC-0313 | Medium | Client profiles | Legacy per-user volume fallback is repeatedly copied across server profiles instead of being consumed once. | B4/B7 | One-time scoped migration, legacy-key removal, and cross-server isolation tests. |
|
||||
| OC-0314 | Medium | Client identity | The client discards the server's partial-success warning when a credential change succeeds but session revocation does not. | B4/B9 | Surface warnings for password/TOTP changes with an action to review sessions; test all affected endpoints. |
|
||||
| OC-0315 | Medium | Client replay | Replay-gate timestamps mix naive UTC server values with local wall-clock parsing. | B2/B7 | One UTC parsing contract and timezone-varied replay boundary tests. |
|
||||
| OC-0316 | Medium | Server/client E2EE | WebSocket resume restores peer public keys but not a room key rotated during the outage. | B2/B7 | Resume re-establishes the current room key and the security indicator cannot claim success prematurely; rotation/outage test passes. |
|
||||
| OC-0317 | Medium | Client DM state | The replay path can regress a DM's `lastMessageId`, undermining duplicate-count protection. | B2/B7 | Monotonic last-message updates with duplicate, out-of-order, and reconnect tests. |
|
||||
| OC-0318 | Medium | Server plugins | Install-time and restart-time plugin manifest precedence differs between JSON and TOML. | B2 | One canonical manifest contract or explicit ambiguity rejection; install/restart parity test. |
|
||||
| OC-0319 | Medium | Client accessibility | The Large Font preference is overridden by a higher-priority inline font-size value. | B9 | Verified text-scale change across restart, zoom, responsive layouts, and accessibility checks. |
|
||||
| OC-0320 | Medium | Server updater | Server self-update selects Linux AMD64 independently of the running architecture. | B6/B10 | Architecture-aware manifest selection and signed update/rollback smoke on every supported server target. |
|
||||
| OC-0321 | Medium | Server TOTP | A TOTP key-file read failure can be treated as absence and lead to key replacement. | B4 | Generate only on confirmed non-existence; all other read errors fail closed without modifying the file. |
|
||||
| OC-0322 | Low | Client connection | TypeScript host validation accepts a hostname form rejected by the native proxy. | B2/B7 | Shared validation corpus produces identical browser, desktop, and Rust decisions. |
|
||||
| OC-0323 | Low | Server unread state | Mark-read/channel-focus can overwrite a mention count from a newer message using a stale snapshot. | B3/B5 | Atomic/monotonic read-state update with concurrent-message regression coverage. |
|
||||
| OC-0324 | Low | Server auth | Login rate-limit identity folding differs from SQLite account lookup semantics. | B4 | Account lookup and limiter use one tested canonical identity rule, including Unicode collision cases. |
|
||||
| OC-0325 | Low | Client search | Search results parse naive UTC timestamps as local time. | B7/B9 | Shared UTC parser and timezone/day-boundary rendering tests. |
|
||||
| OC-0326 | Low | Client pins | Pinned-message timestamps parse naive UTC values as local time. | B7/B9 | Shared UTC parser and timezone/day-boundary rendering tests. |
|
||||
| OC-0327 | Low | Server voice moderation | Server mute/deafen also affects screen-share audio contrary to the product contract. | B5 | Effective moderation applies only to intended media sources; SFU and client-policy tests agree. |
|
||||
| OC-0328 | Low | Client unread state | Channel badges lack the message-ID replay guard already used by DMs. | B2/B7 | Monotonic channel replay guard with duplicate/out-of-order/reconnect tests. |
|
||||
| OC-0329 | Low | Client privacy | Legacy DM profile notes fall back across servers indefinitely. | B4/B7 | One-time server-scoped migration, old-key removal, and cross-server privacy test. |
|
||||
| OC-0330 | Low | Client pins | Pinned messages discard author identity and therefore cannot resolve nicknames. | B7/B9 | Preserve author ID and render the same display identity as ordinary messages. |
|
||||
| OC-0331 | Low | Server admin UI | API-token Created/Last Used values parse naive UTC timestamps as local time. | B6/B9 | Shared UTC contract and timezone/day-boundary admin tests. |
|
||||
| OC-0332 | Low | Client updater | Bare IPv6 server addresses produce an invalid updater URL. | B6/B10 | Central URL builder brackets IPv6 literals and passes domain/IPv4/IPv6/update smoke tests. |
|
||||
| OC-0333 | Low | Client voice UI | Voice-roster render identity does not change when a participant is renamed mid-call. | B7/B9 | Reactive identity signature and rename-in-call test. |
|
||||
| OC-0334 | Low | Client PTT | Escape closes Settings and can simultaneously be saved as the captured PTT key. | B7/B9 | Escape cancels capture without persistence; teardown and timeout paths are tested. |
|
||||
| OC-0335 | Low | Client lifecycle | Each Add Server modal retains listeners and its removed subtree for the connect-page lifetime. | B7 | Modal-owned abort lifecycle; repeated open/close instrumentation shows no accumulation. |
|
||||
| OC-0336 | Low | Client lifecycle | Server-profile rows re-register page-lifetime listeners on every render. | B7 | Row/render ownership prevents accumulation under repeated updates and teardown. |
|
||||
| OC-0337 | Low | Server replay | Cold-tier voice replay truncation can discard the newest events and reconstruct the wrong roster. | B2/B3 | Ordered, bounded replay retains the correct window; boundary/resume tests reconstruct the authoritative roster. |
|
||||
| OC-0338 | Low | Server plugins | TOML plugin manifests can omit configured memory and CPU resource limits. | B2/B3 | Explicit TOML mapping and JSON/TOML resource-limit parity tests. |
|
||||
| OC-0339 | Low | Server config | A valid but empty configuration section is reported as an unknown ineffective key. | B6 | Empty known sections are accepted; true unknown keys remain actionable and tested. |
|
||||
| OC-0340 | Low | Server CLI | A negative API-token expiry can create a token that never expires. | B4/B6 | CLI and HTTP share positive-expiry validation; negative/zero/boundary tests fail safely. |
|
||||
| OC-0341 | Low | Server CLI | A numeric token label cannot be revoked because parsing commits to the ID path. | B4/B6 | Unambiguous ID/label selection or safe fallback with numeric-label regression tests. |
|
||||
| OC-0342 | Low | Client voice UI | Voice avatar letter/color derives from username while the adjacent label may be a nickname. | B9 | Avatar and label consistently derive from the displayed identity. |
|
||||
| OC-0343 | Low | Desktop shell | Clicking the tray icon can hide a minimized window instead of restoring it. | B7/B9 | Minimized windows unminimize and focus; only visible, non-minimized windows toggle hidden. |
|
||||
| OC-0344 | Low | Server TLS | Automatic HTTP-to-HTTPS redirect assumes port 443 instead of the configured HTTPS endpoint. | B6 | Redirect derives the configured public origin/port and passes default/custom/domain/IP tests. |
|
||||
| OC-0345 | Low | Server owner auth | Owner middleware repeats a role read and maps a transient read failure to forbidden. | B3/B4 | Reuse the authenticated context and preserve correct unavailable/unauthorized distinctions in failure tests. **Fixed 2026-08-30, PR #1454 (B3-9):** a role read fault answers 503, a missing role still 403. |
|
||||
| OC-0346 | Low | Server telemetry | Panic recovery reads trace context before tracing middleware creates it. | B3/B6 | Middleware order gives recoveries the active trace ID; panic-path structured-log test passes. **Fixed 2026-08-30, PR #1454 (B3-9):** tracing mounted ahead of recovery; the otel-tagged panic-log test passes. |
|
||||
| OC-0347 | Low | Client DM voice UI | A DM call label reads but does not subscribe to DM state, so it remains stale. | B7/B9 | Subscribe to the owning state and test mid-call rename/update. |
|
||||
| OC-0348 | Low | Client presence | The online-count header includes the local invisible user while the member list presents that user as offline. | B7/B9 | Count and list share one visibility policy with invisible-status regression tests. |
|
||||
|
||||
## Public-safe security remediation
|
||||
|
||||
|
||||
Reference in New Issue
Block a user