Repository navigation
chore(auth): drop the unused User.Salt field and stop reading or writing users.salt - #1514
Conversation
…salt User.Salt predates bcrypt (which embeds its own salt in the hash). Every writer set it to "" and nothing read it, but the store still SELECTed, INSERTed and UPDATEd the users.salt column, and the bootstrap admin INSERTs in migrate.go named it too. Remove the field and every reference to the column from application code: the scanUser SELECT lists, CreateUser, CreateAdminIfNone, UpdateUserCredentials, CompletePasswordReset and both ensureAdminUser INSERTs. The column stays (NOT NULL DEFAULT ''), so rows inserted without it remain valid and code still running from the previous release keeps working during a rolling deploy. Dropping the column is left to a later migration once no deployed release references it. New integration tests drop the column from a migrated database and drive every one of those statements, so they fail on main with `column "salt" does not exist`. Refs LeanerCloud#313 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 15 billable files and costs up to $3.75. Or wait 59 minutes for your next included review. View limit details
Comment |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rF6. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rF8. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
VERDICT: APPROVE a748143 Findings (none blocking):
Attacked:
|
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 3 approving independent review(s) on |
|
Independent review 2 (claude-fleet, cold): APPROVE a748143 Findings:
Attacked:
Verified locally (GOTOOLCHAIN=go1.26.10, no creds, heavy runs under lockf):
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
User.Saltis a leftover from before bcrypt, which stores its own salt inside the hash. Every writer set it to""and no code read it. The store still selected, inserted and updated theusers.saltcolumn on every user read and credential write, and the bootstrap admin INSERTs inmigrate.gonamed the column too. This PR removes the field and every reference to the column from application code. It does not drop the column. That is deferred on purpose, as explained below.Refs #313
Root cause and fix
#1271 left
User.Saltfor later: the issue says to delete the field "once the column is dropped", and the store still read and wrote the column. This PR removes:internal/auth/types.go: theSaltfield.internal/auth/service_user.goandservice_password.go: the fiveSalt: ""/user.Salt = ""assignments.internal/auth/store_postgres.go:saltfrom the fourscanUserSELECTs (GetUserByID,GetUserByEmail,ListUsers,GetUserByResetToken) and from thescanUserdestinations.saltfrom theCreateUserINSERT and theCreateAdminIfNoneconditional INSERT. The placeholders are renumbered, and the NOT EXISTS binding moves from$20to$19.internal/auth/store_postgres_credentials.go:salt = $nfromUpdateUserCredentialsandCompletePasswordReset, with the placeholders renumbered. The compare-and-setWHEREclauses are unchanged.internal/database/postgres/migrations/migrate.go:saltfrom bothensureAdminUserandensureAdminUserWithPasswordINSERTs.The column keeps its schema,
salt VARCHAR(255) NOT NULL DEFAULT ''(000001), so INSERTs that omit it still produce valid rows. Existing values are no longer rewritten. Legacy rows keep whatever they held, which was always''for rows written by any bcrypt-era code, and nothing reads them.Why the column is not dropped here
The brief allowed dropping the column only "if nothing reads or writes it and dropping is safe". After this PR nothing reads or writes it, but dropping it in the same release is not safe:
internal/server/app.gorunMigrationsFunc: migrations.RunMigrations).SELECT ... salt ...) would then fail withcolumn "salt" does not exist. That breaks login, session validation and admin user pages until the old instances drain.So the drop belongs in a follow-up PR once a release containing this change is everywhere. Migration
000148was reserved for this work and is not used by this PR. The follow-up drop migration should take whatever number is next when it lands. That PR also has to update the raw-SQL test fixtures that still insertsalton a fully migrated database:internal/api/handler_plans_concurrency_integration_test.gointernal/api/combined_plan_purchase_integration_test.gointernal/api/handler_purchases_run_update_any_integration_test.gointernal/api/handler_purchases_retry_submit_race_integration_test.gointernal/api/handler_purchases_submit_dedupe_integration_test.goMigration tests that insert at their own older version (000057, 000059, 000064, 000065, 000096, 000099, 000103, 000130, 000138) are unaffected.
#313 status after this PR
UpdateLastUsed,ResolveGCPCredentials,SendWelcomeEmailUser.SaltfieldloadUserrouting,DeleteUser/checkLastAdminConstraint,roleSessionNameredactEmail×3 plusredactEmailLocalMatchesAccountvsAccountScope.AllowspermissionCoveredBycarry-throughTestGrantCeiling_CarvedOutMayBeKeptNotWidened). See #1271.Note for #104 (maintainer action; #104's body is the maintainer's, so I haven't edited it)
The "Verified sound" section of #104 says that
ValidateSession,validateResetTokenandValidateUserAPIKey"all add asubtle.ConstantTimeCompareafter the SQL fetch". It lists this as a timing defense. Suggested replacement for that sentence:Verification
Unit, mock and local-Postgres evidence on branch head
a748143d, which sits on main3bfc2fe5. Run withAWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.New regression tests, both
integration-tagged against a real migrated Postgres. Each dropsusers.saltand then drives the statements that used to name it:internal/auth/store_postgres_no_salt_db_test.go,TestIntegration_UserStoreWorksWithoutSaltColumnruns, in order:CreateAdminIfNone,CreateUser,GetUserByID,GetUserByEmail,ListUsersUpdateUserCredentials,SetPasswordResetToken,GetUserByResetToken,CompletePasswordResetinternal/database/postgres/migrations/ensure_admin_user_no_salt_test.go,TestEnsureAdminUser_WithoutSaltColumn: rerunsRunMigrationswith an admin email on both bootstrap paths (no password, and with password). It asserts the admin row exists with the Administrators group and the expectedactive.Fail before, pass after:
go test -tags integration -count=1 -timeout 20m -run 'TestIntegration_UserStoreWorksWithoutSaltColumn' ./internal/auth/fails at the first statement:ERROR: column "salt" of relation "users" does not exist (SQLSTATE 42703).migrate.goreverted,go test -tags integration -count=1 -timeout 20m -run 'TestEnsureAdminUser_WithoutSaltColumn' ./internal/database/postgres/migrations/fails both subtests (user row for ... must exist).RunMigrationsonly logsWARNING: admin bootstrap failed ... column "salt" ... does not existand returns nil, so the missing row is what shows the failure.-v.Existing integration tests I edited (their
Saltreferences were removed), run narrowly withgo test -tags integration -count=1 -timeout 20m -run '^(TestIntegration_LoginPreservesSecurityChanges|TestIntegration_CredentialWritesPreserveConcurrentMFA|TestIntegration_ResetConfirmPreservesConcurrentMFA|TestIntegration_CreateAdminIfNone_.*|TestIntegration_UserStoreWorksWithoutSaltColumn)$' ./internal/auth/: all PASS, including the concurrent bootstrap and stress variants. With the column still present they also show the new SQL works against today's schema.Checks on the branch:
go build ./...,go vet ./...andgo vet -tags integration ./...are clean.go test -count=1passes for./internal/auth/,./internal/database/postgres/migrations/,./internal/mocks/,./internal/server/and./internal/api/, run one package at a time. The pgxmock column list and argument expectations and the positional fake-row helper instore_postgres_test.gowere updated to the 20-column scan.golangci-lintv2.10.1run --allow-parallel-runnerson./internal/auth/ ./internal/database/postgres/migrations/, with and without--build-tags integration, reports 0 issues.gocyclo -over 10 -ignore "_test\.go"on the six changed non-test files reports nothing.gh pr list --state open --json files).Combined-tree verification at local queue commit
2be43e1e= origin/mainb9271e88+fix/issue-363-next,fix/issue-177-next,fix/issue-919-next,fix/issue-313-next,fix/issue-532-nextandfix/issue-316-nextmerged in that order. Passed:go build ./...;go vetwith and without-tags integration;go test -race -short -count=1 ./...; golangci-lint v2.10.1 with and without the integration tag; gocyclo -over 10;go test -race -tags integrationoninternal/api,internal/auth,internal/config,internal/database/postgres/migrations,internal/purchase,internal/serverandinternal/testutil; the git-secrets scan andscripts/test-git-secrets-allowlist.sh.Concurrency
No new read-then-write is introduced. The credential compare-and-set statements keep their
WHEREguards and only lose an unrelated SET column.CreateAdminIfNonekeeps its advisory lock and NOT EXISTS predicate (the concurrent bootstrap tests above still pass).Out of scope
users.salt(follow-up migration, see above).redactEmailconsolidation (owner decision) andMatchesAccount(tracked with fix(auth): bind group account scope entries to account IDs #1116).json:"-"and never sent.Maintainer: please mirror the issue's triage labels onto this PR:
triaged,priority/p3,severity/low,urgency/eventually,impact/internal,effort/m,type/chore.🤖 Generated with Claude Code