Repository navigation
Keep madmin from writing blank shopkeeper tokens - #158
Merged
Merged
Conversation
Ported from myturntagapi, where saving a shopkeeper in madmin returned 500 in production: PG::UniqueViolation on index_shopkeepers_on_reset_password_token, "Key (reset_password_token)=() already exists". The edit form sends every field, so an empty token arrives as "", and once one shopkeeper had "" stored, the next one with no token collided with it. The template has the same form fields and unique indexes. Shopkeeper normalizes blank reset_password_token and confirmation_token (both unique) to NULL, and a migration clears the "" already stored. The madmin test submits the edit form with every field, as a browser does, while another shopkeeper holds "" tokens. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3Tt4YrVgQRxh5MCpiiric
The previous commit normalized blank reset_password_token and confirmation_token to NULL. Normalization also rewrites finder values, so where(reset_password_token: "") became WHERE reset_password_token IS NULL and matched every shopkeeper without a token. Ported from myturntagapi #219. Drop the normalization and keep the two tokens out of madmin's shopkeeper form (form: false), the only writer that sent "". The migration that clears stored "" values stays. ShopkeeperTest checks that a blank token finds no shopkeeper (it failed with the normalization); the madmin form test still fails with the token fields back in the form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3Tt4YrVgQRxh5MCpiiric
dadachi
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Port of myturntagapi #218 and #219. There, saving a shopkeeper in
/madminreturned 500 in production:PG::UniqueViolationonindex_shopkeepers_on_reset_password_token, "Key (reset_password_token)=() already exists". madmin's edit form submits every field, so empty token fields arrive as""; once one shopkeeper had""stored, the next save of a shopkeeper with no token collided with it. The template has the same setup (ShopkeeperResourceexposedreset_password_tokenandconfirmation_tokenin the form, both with unique indexes).ShopkeeperResource:reset_password_tokenandconfirmation_tokenareform: false(still shown read-only on the show page), so madmin never writes""into them20261017000000_nullify_blank_shopkeeper_tokensclears""already storedMadminTest"saves the shopkeeper edit form as the browser submits it": GETs the edit page, parses the form with Nokogiri and PATCHes every field like a browser, while another shopkeeper holds""tokensShopkeeperTest"a blank token finds no shopkeeper":where(reset_password_token: "")/where(confirmation_token: "")match nothing andwith_reset_password_token("")is nilThe first commit normalized blank tokens to NULL in
Shopkeeper(normalizes ... token.presence). The second commit removes that: Rails also normalizes finder values, sowhere(reset_password_token: "")becameIS NULLand matched every shopkeeper without a token. A comment inShopkeeperexplains why the token columns are not normalized.Other unique string columns exposed in madmin were checked:
accounts_invitations.token,roles.tag,permissions.tag,admin_users.email,action_push_native_devices.tokenetc. are allNOT NULLand always filled, so blank-vs-NULL cannot collide there; only the shopkeeper tokens are nullable.Test evidence
PG::UniqueViolation ... index_shopkeepers_on_confirmation_token ... Key (confirmation_token)=() already exists. With the token fields put back in the form (no normalization), it errors the same way againShopkeeperTest"a blank token finds no shopkeeper" with the normalization in place: fails (Expected true to be nil or false, thewhere(reset_password_token: "").exists?assertion)shopkeeper_test.rb+madmin_test.rb34 runs, 0 failuresbin/rubocop -f githubclean;bin/cipassed (626 runs, 0 failures, 0 errors, 1 skip)🤖 Generated with Claude Code
https://claude.ai/code/session_01Y3Tt4YrVgQRxh5MCpiiric