Skip to content

fix(auth): serialize initial administrator setup - #798

Open
Quick104 wants to merge 1 commit into
mainfrom
codex/fix-bootstrap-admin-race
Open

fix(auth): serialize initial administrator setup#798
Quick104 wants to merge 1 commit into
mainfrom
codex/fix-bootstrap-admin-race

Conversation

@Quick104

Copy link
Copy Markdown
Contributor

Problem

Related issue: N/A — audit-validated security fix

POST /api/v1/auth/setup checked whether any users existed and created the first administrator in separate autocommit operations. Two requests could both observe an empty users table, then create different administrator accounts and login sessions.

The finding was reproduced on fresh origin/main at 8164fd594b9fdd8c1944bb0b6251f2d00e4a24ca: two synchronized setup requests both returned 201, leaving two administrators and two sessions.

Approach

Add a repository operation dedicated to first-user creation. It opens a PostgreSQL transaction, takes a SHARE ROW EXCLUSIVE lock on users, rechecks that the table is empty, inserts through the same transaction, and commits. Competing setup requests and ordinary user writers therefore serialize on the database connection that owns the transaction. If the connection is lost, PostgreSQL rolls the transaction back and releases the lock.

The normal account-provisioning path is unchanged. Initial setup reuses its profile creation and cleanup flow but selects the database-enforced first-user operation. No migration or API shape change is required.

The regression test uses two independent one-connection pools. A query tracer holds both setup attempts after their initial zero-user checks and releases them together. It asserts one token-pair success, one ErrSetupAlreadyComplete, and exactly one user, administrator, and session.

Validation

Focused and database-backed checks:

  • go test ./internal/auth ./internal/database/pglock — pass
  • go test -race ./internal/auth ./internal/database/pglock — pass
  • go vet ./internal/auth ./internal/database/pglock — pass
  • go test ./internal/api/handlers -run '^TestAuthProviders' -count=1 — pass
  • SILO_TEST_DATABASE_URL=<isolated-empty-database> go test ./internal/auth -run '^TestSetupInitialUserConcurrentAcrossPoolsDB$' -count=20 — pass
  • SILO_TEST_DATABASE_URL=<isolated-empty-database> go test -race ./internal/auth -run '^TestSetupInitialUserConcurrentAcrossPoolsDB$' -count=5 — pass

Full repository gate:

  • make embed-stub — pass
  • go build ./... — pass
  • gofmt -l . — pass, no output
  • go vet ./... — pass
  • golangci-lint run --new-from-merge-base="origin/main" ./... — pass, 0 issues; it printed one warning about an unrelated stale worktree path
  • make test-go on macOS — not passing because of repeatable failures in untouched packages: internal/httpstream/TestReadFromRollsDeadlineUnderProductionStep and internal/jellycompat/TestBeginWebOperationRecoversDeadProcessLock / TestBeginWebOperationRejectsLiveProcessLock
  • go test ./... against the exact candidate source on the Linux dev-builder — pass, including internal/httpstream and internal/jellycompat
  • pnpm install --frozen-lockfile — pass
  • pnpm run lint — pass with 167 inherited warnings and 0 errors
  • pnpm run format:check — pass
  • pnpm run build — pass with existing asset/chunk-size warnings
  • make test-web — pass: 294 files, 2,184 tests
  • make verify-settings-bindings-all — pass
  • make verify-playback-fixtures — pass
  • make verify-local-paths — pass

Isolated dev-builder validation in audit-bootstrap-admin:

  • controller orientation and doctor passed before and after validation
  • a normal first setup returned 201 and produced one administrator, one user, and one session
  • after resetting only the synthetic test identity, synchronized competing setup requests returned 201 and 401 setup_complete
  • the competing run produced one administrator, one user, and one session

Risks

The table lock briefly blocks writes to users while first-user creation hashes the password and inserts the row. This happens only while the database has no users. Setup-status reads remain available. There is no migration, stored-data rewrite, client contract change, or Jellyfin compatibility change.

Rollback is a revert of this commit; no schema or data rollback is needed.

AI Disclosure

  • Tool(s): OpenAI Codex
  • Model(s): n/a (the tool did not report a model identifier)
  • Involvement: AI-assisted
  • Adversarial review: A fresh read-only investigator independently reproduced the count-then-create race. A separate bypass reviewer rejected an earlier advisory-lock design because a one-connection pool could deadlock and lock-session loss could release the fence before side effects completed; it also found an over-broad test interface. The implementation was replaced with a same-transaction PostgreSQL table lock and emptiness check, and the interface was narrowed. The reviewer then rechecked the replacement for concurrent setup, ordinary user writers, connection loss, pool exhaustion, and API regressions and found no remaining concrete bypass or regression.

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 52 minutes.

View limit details

Limit details: You’ve used the included review currently available. Your 93 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5fb99bae-f9c1-4eff-8e90-985fb1fdb1b1

📥 Commits

Reviewing files that changed from the base of the PR and between 8164fd5 and 3e09fd1.

📒 Files selected for processing (5)
  • internal/auth/account_provisioner.go
  • internal/auth/account_provisioner_test.go
  • internal/auth/repository.go
  • internal/auth/service.go
  • internal/auth/service_setup_test.go

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3e09fd14f1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +60 to +62
select {
case <-release:
case <-ctx.Done():

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Give the query barrier an independent timeout

When SILO_TEST_DATABASE_URL is set but only one traced pool reaches the count query—for example, because the database has too few connection slots for the control pool plus both traced pools—that goroutine waits on release forever. The fallback cannot help because ctx comes from t.Context(), which is canceled only when the test is already finishing, while the test cannot finish because wg.Wait() is blocked on this goroutine. Use a timeout or cancellation source that can fire while the test is running so a connection failure does not stall the package until the global go test timeout.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant