Release v0.6.1 #32
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "release/0.6.1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Release v0.6.1
PR review followup release — addresses all findings from the v0.6.0 automated PR review.
Changes
Security Fixes
Bug Fixes
Refactoring
Testing
Checklist
References
Version: v0.6.1 — PR Review Followup (Gravity PM ID: 122)
PR Review
Verdict: Not Acceptable
This review was conducted from a fresh context against the current branch state (release/0.6.1 → main, 57 files, 7717 diff lines). First review cycle — no prior review comments on this PR.
Findings
Test Results
internal/crypto: 83.9% — excellent for new security packageinternal/sse: 100.0%internal/auth: 42.8%internal/middleware: 50.2%internal/handlers: 26.7%internal/services: 30.0% — newProviderServiceandUserServiceat 0%Human Review Items
2 items flagged for human review — see items marked with HUMAN REVIEW.
Key Findings
p.position— will fail on any real database upgrade —internal/database/migrations/00030_dashboard_transition.sql:7internal/database/migrations/00023_auth_local_support.sqlcomputeLoadTestStatsaccesseslatencies[0]andlatencies[len-1]without length guard —internal/handlers/demo.go:743-749ProviderServiceandUserServicehave 0% test coverage despite handling security-sensitive operationscomputeLoadTestStats—rpscalculation divides bydurationMs/1000.0with no guard for sub-millisecond elapsed timeinternal/database/migrations/00030_dashboard_transition.sql
[BLOCKING] Line 7: Column
p.positiondoes not exist — migration will fail on any database upgradeThe data migration in the UP block added by OP#1707 references a non-existent column:
The
pagestable was created withsort_order(see00001_initial_schema.sql), notposition. PostgreSQL validates column references at parse time regardless of row count, so this migration will fail withERROR: column p.position does not existon any deployment running this upgrade.This was introduced in this PR as part of the OP#1707 migration reversibility refactor. The tests do not catch this because all tests use
pgxmock— no test applies real migrations against PostgreSQL.Suggested fix:
internal/database/migrations/00023_auth_local_support.sql
[IMPORTANT] Lines 1–49: Migration combines two distinct concerns — partial violation of OP#1707 single-purpose requirement
The migration mixes: (1) altering the
userstable for local auth columns (password_hash,auth_provider,email_verified,email_verified_at, and convertingoidc_subjectto nullable with a partial index), and (2) creating the entirely separateauth_tokenstable.The
auth_tokensmechanism is used across multiple flows (confirmation, reset, magic link, invite) and could logically be independent of the local auth user columns. The DOWN migration correctly drops both in the right order, but cannot be applied partially.HUMAN REVIEW: The OP#1707 intent was "single-purpose and fully reversible." Whether this combination is acceptable depends on whether the project considers these two changes to be one logical unit of work or two. The migration is fully reversible. If maintaining the current combined migration is preferred, adding a clear comment explaining the intent (
-- auth_tokens table: supports confirmation, reset, magic link, and invite flows) would satisfy the spirit of the requirement.[IMPORTANT] Lines 39–42: DOWN migration creates synthetic data for existing local users
The comment documents the intent, but the effect is that a rollback on a production database with existing local-auth users permanently writes fabricated
oidc_subjectvalues (removed-<uuid>) into the users table. These would not be recognized as valid OIDC subjects by any identity provider, effectively locking out those users.HUMAN REVIEW: This is the only viable rollback strategy for this schema change, but it means rolling back is destructive if local users exist. This should be clearly documented in the migration file with an explicit warning that rollback after local-auth users are created will require manual data cleanup.
internal/handlers/demo.go
[IMPORTANT] Lines 743–749:
computeLoadTestStatsaccesseslatencies[0]andlatencies[len-1]without a length guardIn the current code,
totalReqs == 0is prevented in practice by enforcingiterations >= 1andconcurrency >= 1. However,computeLoadTestStatsis a standalone function whose signature acceptslatencies []float64with no length guarantee. If called with an empty slice (e.g. if all goroutines error before any append — which can't happen with the current goroutine logic, but could with future changes), this panics.Suggested fix:
internal/services/provider.go + internal/services/user.go
[MINOR] New services have 0% test coverage
ProviderService(15 methods, handles OIDC client_secret encryption/decryption) andUserService(7 methods, handles user creation, email lookup, password hashing) both have 0% coverage. These are security-sensitive operations introduced in this PR.The critical paths that should be tested:
As an admin, I can save an OIDC provider.
secrets != nil)Create()is called with a provider having aClientSecretenc:prefixGet()should return the decrypted plaintextAs a new user, I can register.
CheckUsernameExistsreturns false andCheckEmailExistsreturns falseCreateLocalUser()is calledbcrypt-hashed password should be inserted[MINOR] Lines 743–744:
rpsdivision bydurationMs/1000.0— no guard for sub-millisecond durationdurationMsis computed fromelapsed.Milliseconds()which returns 0 for durations under 1ms. If a load test completes in under 1ms (only possible in trivially small tests),rpswould be+Inf. ThetotalReqs == 0guard suggested above would cover this.internal/auth/local.go
[NITPICK] Line 159: Email verification message reveals account existence after correct password
This message is only returned when: (1) the email exists, AND (2) the password is correct, AND (3) the account is unverified. An attacker who probes with a known email+password combination can distinguish "unverified account" from "invalid credentials."
OP#1720 fixed enumeration via timing and response message for non-existent accounts. This is a different, weaker form — requires the attacker to already know the correct password. Given the UX value (guiding users to check their email), this trade-off is common in practice.
HUMAN REVIEW: Decide whether to preserve this message for UX reasons or normalize to
"Invalid email or password"to fully eliminate enumeration surface. A middle path: return theNeedConfirm: trueflag (triggering the resend-confirmation UI) without a message that confirms account+password validity.internal/middleware/csrf.go
[NITPICK] Sec-Fetch-Site approach is stateless and correct, but diverges from PR description
The PR body says "Add CSRF tokens to fetch() POST calls in edit-mode.js" but the implementation uses
X-Requested-With: fetchheaders +SecFetchProtectmiddleware (Sec-Fetch-Site / Origin / X-Requested-With fallback chain), not explicit server-generated CSRF tokens.This is a valid and well-understood CSRF mitigation strategy. All fetch() POST calls in
edit-mode.jscorrectly include"X-Requested-With": "fetch", satisfying the middleware. No issue with the implementation — the PR description is just slightly imprecise about the mechanism.Positive Observations
The PR delivers a significant amount of security and quality improvement:
ValidateAndConsumeis a correct atomicUPDATE … WHERE used_at IS NULL AND expires_at > now() RETURNING— no race condition possible.unauthorized()response.gzipResponseWriter.Flush()correctly delegates to bothgz.Flush()and the underlyinghttp.Flusher.SSEStatuscorrectly short-circuit viareturn.ForgotPassword,SendMagicLink, andResendConfirmationcorrectly return identical messages regardless of account existence.enc:prefix for migration compatibility, and passthrough for plaintext existing values — solid implementation.CreateTestPageandScaleTestPagenow use proper transactions withdefer tx.Rollback().🤖 Generated by Gravity Bot (pr-review agent) · gashy v0.6.1 · 57 files, 7717 lines reviewed
PR Review — Supplemental Findings
Deep code analysis surfaced additional findings not covered in the initial review. All are advisory given the PR is already Not Acceptable on the blocking migration bug — but these warrant attention in the fix pass.
internal/handlers/setup.go — SoC violations (OP#1704/OP#1707 partially unmet)
[IMPORTANT]
handleOIDCSetupcontains direct SQL againstpoolthat bypassesProviderServiceLines ~205–246:
handleOIDCSetuprunsINSERT INTO oidc_providersandUPDATE app_settingsdirectly via thepoolfield, duplicating upsert logic that also exists inOIDCManager.Init(manager.go:56–68) andProviderService.Create/Update. Three separate places now maintain the same SQL for the same table. This is an incomplete OP#1704 extraction — the handler was not refactored to callProviderService.[IMPORTANT]
handleLocalSetupcontains a directINSERT INTO usersthat bypassesUserService.CreateLocalUserLine ~314–318: The setup wizard creates the initial admin user with an inline
INSERT INTO users, whileRegisterHandlerusesUserService.CreateLocalUser. The two paths diverge in column coverage (setup omitsgroups) and will drift further asUserServiceevolves. This is inconsistent with OP#1704's goal of a single canonical user-creation path.Suggested fix for both: Route
handleOIDCSetupthroughProviderService.Create/UpdateandhandleLocalSetupthroughUserService.CreateLocalUser(withIsAdmin=true,EmailVerified=true).[IMPORTANT]
LocalAuthHandler.Logincallspool.Execdirectly forlast_login_atauth/local.go:166:_, _ = h.db.Exec(ctx,UPDATE users SET last_login_at = now() WHERE id = $1, user.ID)—UserService.UpdateLastLoginalready exists (user.go:109–112) for exactly this purpose. The direct call bypasses the service layer inconsistently with OP#1704.internal/handlers/demo.go — Demo write endpoints exposed in guest mode
[IMPORTANT] Line 112 in
middleware/mode.go:isDemoWritePathgates demo API mutations out ofisPublicPath, which prevents unauthenticated access inoauth_displayandfullmodes. However, inguestmode theEnforceModeswitch falls through tonext.ServeHTTP(w, r)unconditionally, meaningPOST /demo/api/test-page,POST /demo/api/test-page/{id}/scale, andPOST /demo/api/load-testare fully accessible without any authentication. TheRequireAdmin(demoMux)wrapping in main.go protects the demo read endpoints butisDemoWritePathroutes make the write endpoints accessible.Specifically in main.go the write endpoints are registered as:
So
RequireAdminDOES protect these routes — this finding may be a false positive ifRequireAdminreturns 403 for unauthenticated users (not just non-admins).HUMAN REVIEW: Verify whether
RequireAdminrejects unauthenticated requests (no user in context) with 403 in guest mode, or whether it only checksuser.IsAdmin. If it checksuser != nil && user.IsAdmin, it is safe. If it only checksuser.IsAdminwithout a nil guard, guest-mode requests with no user would panic or bypass.internal/handlers/reset.go — Silent auth bypass in MagicLogin
[IMPORTANT] Lines ~330–340:
MagicLoginperforms a runtime type assertion to get aSessionCreatorfromh.sessions:If the type assertion fails (e.g.
h.sessionsis wired with a type that satisfiesSessionDestroyerbut notSessionCreator), no session is created and the user is redirected as if login succeeded — but with no session cookie, they are immediately redirected back to login. This is not an auth bypass (no session = no access) but it's a silent, confusing failure mode. The correct fix is to makeResetHandlerdepend on an interface that exposes both methods, so a wiring mistake is caught at compile time.Suggested fix: Define
type SessionManager interface { SessionCreator; SessionDestroyer }and changeResetHandler.sessionstoSessionManager.internal/services/auth_token.go — Missing test coverage for exported functions
[MINOR]
ListTokensByType(used byInviteHandler) andInvalidateTokens(used byResendConfirmation) have 0% coverage. Both are exported and in active use.Suggested test cases:
As the system, it can list pending invites.
ListTokensByType(ctx, "invite")is calledcreated_at DESCAs a user resending confirmation, prior tokens are invalidated.
InvalidateTokens(ctx, userID, "confirmation")is calledused_atset andValidateTokenreturns an error for bothinternal/auth/manager.go — No migration path for existing plaintext client secrets
[MINOR] When
OIDC_ENCRYPT_KEYis introduced to an existing deployment, anyoidc_providers.client_secretvalues written before encryption was configured will remain as plaintext.Decryptpasses them through unchanged (correct), but there is no admin UI or startup warning to indicate that existing secrets are unencrypted. An operator who addsOIDC_ENCRYPT_KEYexpecting at-rest protection will have it only for newly saved secrets.Suggested fix: On startup, if
secrets != nil, queryoidc_providersfor anyclient_secretvalues that do NOT have theenc:prefix and log a warning:"OIDC_ENCRYPT_KEY is set but N provider(s) have unencrypted client_secret values — re-save them in the admin UI to encrypt".🤖 Generated by Gravity Bot (pr-review agent) · supplemental analysis · gashy v0.6.1
@ai wrote in #32 (comment):
@ai wrote in #32 (comment):
guest mode should not be allowed to see this. only a demo mode should bypass auth for the demo page and features.
PR Followup Report — PR#32 (Release v0.6.1)
Triage Summary
Parsed 12 findings from the initial review + supplemental review comments, plus 4 human review flags.
Fixes Applied (v0.6.1 — 8/8 tasks closed)
802418fp.position→p.sort_orderin migration 0003062d90adcb76964computeLoadTestStatsb696b4e8a8728elast_login_atthrough UserService1b85daa/demofrom public paths2fe01ebSessionManagerinterface to prevent silent MagicLogin failurea268be4Deferred to v0.7.0
config.gostruct (reducemain.goenv sprawl)AdminHandlerCRUD flowsHuman Review Items (Not Auto-Fixable)
oidc_subjectvalues on rollback; document operational recovery procedureTest Results
All existing + new tests pass:
go test ./internal/handlers/...— PASS (5.0s)go test ./internal/services/...— PASS (0.9s)golangci-lint run ./...— cleanCoverage Delta
internal/services/provider.gointernal/services/user.goAutomated followup by Gravity Bot — 8 fixes committed, 2 items deferred, 4 flagged for human review.