Make IsAdmin strict, gate wizard upload on IsWizardMode#530
Conversation
Previously IsAdmin returned true when admin_users was empty, so the wizard could upload images during setup. But that branch was reached on every request, not just wizard endpoints — any deployment that provisioned .env from configuration management would skip the wizard, leave admin_users empty, and grant admin authority to anonymous visitors on all routes guarded by IsAdmin. Split the concern: IsAdmin now returns false when no admin user exists. IsWizardMode is a new predicate (true iff admin_users is empty) used only by UploadFile and UpdateSettings — the two endpoints the wizard hits before the admin user is created. Refs #529 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a security flaw where auth.IsAdmin() could return true when admin_users was empty (fresh DB), unintentionally granting admin privileges to anonymous users. It makes IsAdmin strict and introduces IsWizardMode to allow only the install-wizard-required pre-admin endpoints to proceed before an admin exists.
Changes:
- Make
auth.IsAdmin()returnfalsewhen no admin user exists. - Add
IAuth.IsWizardMode()/Auth.IsWizardMode()to detect “no admin exists yet”. - Gate
UploadFileandUpdateSettingswithIsAdmin || IsWizardMode, and extend tests/mocks accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
auth/auth.go |
Tightens IsAdmin and adds IsWizardMode predicate used by wizard-exempt handlers. |
admin/admin.go |
Allows wizard-mode access to UploadFile and UpdateSettings while keeping other admin routes strict. |
auth/auth_test.go |
Adds direct tests for strict IsAdmin and IsWizardMode semantics with SQLite in-memory DB. |
admin/admin_test.go |
Updates auth mock and adds a wizard-mode upload test case to verify auth gate behavior. |
blog/blog_test.go |
Updates auth mock to satisfy the expanded IAuth interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (a *Auth) IsWizardMode(c *gin.Context) bool { | ||
| var adminUser AdminUser | ||
| err := (*a.db).First(&adminUser).Error | ||
| return err != nil |
There was a problem hiding this comment.
IsWizardMode currently returns true for any error from First(&adminUser), not just the expected "record not found" case. That means DB failures (e.g. missing table, connection issues) would be treated as wizard mode and could unintentionally allow unauthenticated access to the wizard-exempt endpoints. Consider returning true only when errors.Is(err, gorm.ErrRecordNotFound), and otherwise returning false (optionally logging the unexpected error).
| return err != nil | |
| if err == nil { | |
| return false | |
| } | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| return true | |
| } | |
| log.Printf("IsWizardMode: unexpected error querying admin user: %v", err) | |
| return false |
Summary
Fixes #529.
auth.IsAdmin()previously returnedtruewhenever theadmin_userstable was empty, on the rationale that "the wizard needs to upload images." That branch was reached on every request, so any deployment that provisioned.envfrom configuration management (Ansible, Compose env files, Helm, etc.) would skip the install wizard, leaveadmin_usersempty, and silently grant admin authority to anonymous visitors on every route guarded byIsAdmin— including/api/v1/postsPOST/PATCH/DELETE.This PR splits the concern:
IsAdminis now strict: returnsfalsewhen no admin user exists.IsWizardModeis a new predicate onIAuth:trueiffadmin_usersis empty.UploadFileandUpdateSettings— the only two admin endpoints the install wizard actually hits before an admin user exists — accept the wizard exemption:if !IsAdmin && !IsWizardMode.Every other admin handler stays strictly admin-only. The wizard's setup flow is unchanged for the operator.
Note on the upstream issue
This PR addresses fix (2) from #529 only. Fix (1) —
rootHandlershould also route to the wizard whenadmin_usersis empty, so automated deploys that pre-populate.envcan't bypass setup — is a separate change and not included here. With this PR alone, a pre-populated.envstill bypasses the wizard, but the resulting state is no longer dangerous becauseIsAdmincorrectly returnsfalsefor everyone.Tests
auth/auth_test.gocovers the strictIsAdminandIsWizardModesemantics directly against an in-memory SQLite DB.IsWizardMode=false, plus a new test case asserts wizard-mode pass-through (request reaches the handler body and fails on missing form file with 400, not on auth with 401).admin/admin_test.goandblog/blog_test.goextended to satisfy the expanded interface.```
ok goblog/admin 0.032s
ok goblog/auth 0.006s
ok goblog/blog 0.028s
```
🤖 Generated with Claude Code