From 2e93213e78e8952d1959b966745d49e3cc707109 Mon Sep 17 00:00:00 2001 From: dadachi Date: Wed, 29 Apr 2026 17:44:47 +0900 Subject: [PATCH 1/4] Fix two latent bugs in permissions and sign-in flows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PermissionsController#index compared `confirmed_*_version < current_*_version`, where `current_version` returns nil if no row is flagged current. `string < nil` raises ArgumentError → 500. Add a `version_outdated?` helper that treats a missing current version as "nothing to update". ShopkeeperAuth::SessionsController#create unconditionally assigned `request.headers["source"]` to current_platform and called `save!(validate: false)`. Sign-ins without the source header overwrote the user's stored platform with nil, bypassing the presence/inclusion validation. Now skip the assignment when the header is blank. Adds regression tests for both paths. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../v1/shopkeeper/permissions_controller.rb | 13 ++++++++-- .../shopkeeper_auth/sessions_controller.rb | 5 +++- .../shopkeeper/permissions_controller_test.rb | 11 ++++++++ .../sessions_controller_test.rb | 25 +++++++++++++++++++ 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/v1/shopkeeper/permissions_controller.rb b/app/controllers/api/v1/shopkeeper/permissions_controller.rb index 568a063..3cff3e2 100644 --- a/app/controllers/api/v1/shopkeeper/permissions_controller.rb +++ b/app/controllers/api/v1/shopkeeper/permissions_controller.rb @@ -8,8 +8,8 @@ def index current_privacy_version = PrivacyVersion.current_version current_terms_version = TermsVersion.current_version - should_update_privacy = current_shopkeeper.confirmed_privacy_version < current_privacy_version - should_update_terms = current_shopkeeper.confirmed_terms_version < current_terms_version + should_update_privacy = version_outdated?(current_shopkeeper.confirmed_privacy_version, current_privacy_version) + should_update_terms = version_outdated?(current_shopkeeper.confirmed_terms_version, current_terms_version) options = {} options[:meta] = { @@ -25,4 +25,13 @@ def index permissions = current_accounts_shopkeeper.permissions render json: PermissionSerializer.new(permissions, options).serializable_hash end + + private + + # nil current → nothing published yet, nothing to update. + def version_outdated?(confirmed, current) + return false if current.nil? + + confirmed < current + end end diff --git a/app/controllers/shopkeeper_auth/sessions_controller.rb b/app/controllers/shopkeeper_auth/sessions_controller.rb index 7758ddc..4739587 100644 --- a/app/controllers/shopkeeper_auth/sessions_controller.rb +++ b/app/controllers/shopkeeper_auth/sessions_controller.rb @@ -3,7 +3,10 @@ def create super return if @resource.blank? - @resource.current_platform = request.headers["source"] + source = request.headers["source"] + return if source.blank? + + @resource.current_platform = source @resource.save!(validate: false) end diff --git a/test/controllers/api/v1/shopkeeper/permissions_controller_test.rb b/test/controllers/api/v1/shopkeeper/permissions_controller_test.rb index 7ef3d92..148b75b 100644 --- a/test/controllers/api/v1/shopkeeper/permissions_controller_test.rb +++ b/test/controllers/api/v1/shopkeeper/permissions_controller_test.rb @@ -64,4 +64,15 @@ class Api::V1::Shopkeeper::PermissionsControllerTest < ActionDispatch::Integrati assert_response :unauthorized end + + test "index does not crash when there is no current published privacy or terms version" do + PrivacyVersion.update_all(current_type: PrivacyVersion.current_types[:uncurrent]) + TermsVersion.update_all(current_type: TermsVersion.current_types[:uncurrent]) + + get api_v1_shopkeeper_permissions_url, headers: @shopkeeper.create_new_auth_token + + assert_response :success + assert_equal false, response.parsed_body["meta"]["should_update_privacy"] + assert_equal false, response.parsed_body["meta"]["should_update_terms"] + end end diff --git a/test/controllers/shopkeeper_auth/sessions_controller_test.rb b/test/controllers/shopkeeper_auth/sessions_controller_test.rb index 4e9067a..5509d84 100644 --- a/test/controllers/shopkeeper_auth/sessions_controller_test.rb +++ b/test/controllers/shopkeeper_auth/sessions_controller_test.rb @@ -28,4 +28,29 @@ class ShopkeeperAuth::SessionsControllerTest < ActionDispatch::IntegrationTest delete destroy_shopkeeper_session_url, headers: shopkeeper.create_new_auth_token assert_response :success end + + test "successful sign-in updates current_platform to the source header value" do + shopkeeper = shopkeepers(:one) + shopkeeper.create_default_account + shopkeeper.update_column(:current_platform, "ios") + + post shopkeeper_session_url, + params: {email: shopkeeper.email, password: "password"}, + headers: {source: "android"} + + assert_response :success + assert_equal "android", shopkeeper.reload.current_platform + end + + test "successful sign-in without source header preserves the existing current_platform" do + shopkeeper = shopkeepers(:one) + shopkeeper.create_default_account + shopkeeper.update_column(:current_platform, "ios") + + post shopkeeper_session_url, + params: {email: shopkeeper.email, password: "password"} + + assert_response :success + assert_equal "ios", shopkeeper.reload.current_platform + end end From b90febb78ec68c57d53c68f54bb41895222bb539 Mon Sep 17 00:00:00 2001 From: dadachi Date: Wed, 29 Apr 2026 18:00:48 +0900 Subject: [PATCH 2/4] Revert PermissionsController nil-version guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review: a missing current PrivacyVersion/TermsVersion is a server-side data integrity problem (no row published as current). Crashing loud is preferable to silently telling the client "you're up to date" — the latter would mask the data issue and mislead clients. Keeps the SessionsController fix. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../api/v1/shopkeeper/permissions_controller.rb | 13 ++----------- .../v1/shopkeeper/permissions_controller_test.rb | 11 ----------- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/app/controllers/api/v1/shopkeeper/permissions_controller.rb b/app/controllers/api/v1/shopkeeper/permissions_controller.rb index 3cff3e2..568a063 100644 --- a/app/controllers/api/v1/shopkeeper/permissions_controller.rb +++ b/app/controllers/api/v1/shopkeeper/permissions_controller.rb @@ -8,8 +8,8 @@ def index current_privacy_version = PrivacyVersion.current_version current_terms_version = TermsVersion.current_version - should_update_privacy = version_outdated?(current_shopkeeper.confirmed_privacy_version, current_privacy_version) - should_update_terms = version_outdated?(current_shopkeeper.confirmed_terms_version, current_terms_version) + should_update_privacy = current_shopkeeper.confirmed_privacy_version < current_privacy_version + should_update_terms = current_shopkeeper.confirmed_terms_version < current_terms_version options = {} options[:meta] = { @@ -25,13 +25,4 @@ def index permissions = current_accounts_shopkeeper.permissions render json: PermissionSerializer.new(permissions, options).serializable_hash end - - private - - # nil current → nothing published yet, nothing to update. - def version_outdated?(confirmed, current) - return false if current.nil? - - confirmed < current - end end diff --git a/test/controllers/api/v1/shopkeeper/permissions_controller_test.rb b/test/controllers/api/v1/shopkeeper/permissions_controller_test.rb index 148b75b..7ef3d92 100644 --- a/test/controllers/api/v1/shopkeeper/permissions_controller_test.rb +++ b/test/controllers/api/v1/shopkeeper/permissions_controller_test.rb @@ -64,15 +64,4 @@ class Api::V1::Shopkeeper::PermissionsControllerTest < ActionDispatch::Integrati assert_response :unauthorized end - - test "index does not crash when there is no current published privacy or terms version" do - PrivacyVersion.update_all(current_type: PrivacyVersion.current_types[:uncurrent]) - TermsVersion.update_all(current_type: TermsVersion.current_types[:uncurrent]) - - get api_v1_shopkeeper_permissions_url, headers: @shopkeeper.create_new_auth_token - - assert_response :success - assert_equal false, response.parsed_body["meta"]["should_update_privacy"] - assert_equal false, response.parsed_body["meta"]["should_update_terms"] - end end From 0de6fe44dfde33d200bb8eaece9b40bb5f6ca470 Mon Sep 17 00:00:00 2001 From: dadachi Date: Wed, 29 Apr 2026 18:03:31 +0900 Subject: [PATCH 3/4] Require 'source' header on shopkeeper sign-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per direction: the source header (ios/android) is mandatory. Reject the request with 401 when missing instead of skipping the current_platform update — silently signing the user in without the header would let API tools accumulate sessions that lack platform attribution and bypass the presence/inclusion validation on Shopkeeper. Adds the locale key and updates the regression test for the blank-header path. Updates the existing "no params" test to send the header so the bad_credentials assertion remains the path under test. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/controllers/shopkeeper_auth/sessions_controller.rb | 8 +++++--- config/locales/en.yml | 1 + .../shopkeeper_auth/sessions_controller_test.rb | 7 ++++--- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/controllers/shopkeeper_auth/sessions_controller.rb b/app/controllers/shopkeeper_auth/sessions_controller.rb index 4739587..9216b20 100644 --- a/app/controllers/shopkeeper_auth/sessions_controller.rb +++ b/app/controllers/shopkeeper_auth/sessions_controller.rb @@ -1,11 +1,13 @@ class ShopkeeperAuth::SessionsController < DeviseTokenAuth::SessionsController def create + source = request.headers["source"] + if source.blank? + return render json: {code: 401, error_message: I18n.t("devise_token_auth.sessions.missing_source")}, status: :unauthorized + end + super return if @resource.blank? - source = request.headers["source"] - return if source.blank? - @resource.current_platform = source @resource.save!(validate: false) end diff --git a/config/locales/en.yml b/config/locales/en.yml index 626b6d1..5d053f1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -88,6 +88,7 @@ en: not_confirmed: "A confirmation email was sent to your account at '%{email}'. You must follow the instructions in the email before your account can be activated." bad_credentials: "Invalid email or password. Please try again." user_not_found: "User was not found or was not signed in." + missing_source: "Missing 'source' header." passwords: missing_email: "You must provide an email address." missing_redirect_url: "Missing redirect URL." diff --git a/test/controllers/shopkeeper_auth/sessions_controller_test.rb b/test/controllers/shopkeeper_auth/sessions_controller_test.rb index 5509d84..fd58b59 100644 --- a/test/controllers/shopkeeper_auth/sessions_controller_test.rb +++ b/test/controllers/shopkeeper_auth/sessions_controller_test.rb @@ -2,7 +2,7 @@ class ShopkeeperAuth::SessionsControllerTest < ActionDispatch::IntegrationTest test "returns unauthorized if shopkeeper not valid" do - post shopkeeper_session_url + post shopkeeper_session_url, headers: {source: "ios"} assert_response :unauthorized assert response.parsed_body["error_message"] assert_equal I18n.t("devise_token_auth.sessions.bad_credentials"), response.parsed_body["error_message"] @@ -42,7 +42,7 @@ class ShopkeeperAuth::SessionsControllerTest < ActionDispatch::IntegrationTest assert_equal "android", shopkeeper.reload.current_platform end - test "successful sign-in without source header preserves the existing current_platform" do + test "sign-in without source header is rejected with 401" do shopkeeper = shopkeepers(:one) shopkeeper.create_default_account shopkeeper.update_column(:current_platform, "ios") @@ -50,7 +50,8 @@ class ShopkeeperAuth::SessionsControllerTest < ActionDispatch::IntegrationTest post shopkeeper_session_url, params: {email: shopkeeper.email, password: "password"} - assert_response :success + assert_response :unauthorized + assert_equal I18n.t("devise_token_auth.sessions.missing_source"), response.parsed_body["error_message"] assert_equal "ios", shopkeeper.reload.current_platform end end From 57d0e72a6728af677b6a112a19e29526b01b0510 Mon Sep 17 00:00:00 2001 From: dadachi Date: Thu, 30 Apr 2026 06:49:22 +0900 Subject: [PATCH 4/4] Make 'source' header optional on shopkeeper sign-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the earlier "require source header" form. Anti-mass-signup is now handled at the right layer by the sign-up rate_limit introduced in PR #50, so the sign-in header has no security job left. current_platform is informational metadata; rejecting sign-ins on missing metadata is too aggressive — it breaks non-mobile callers (curl, CI, integration tools, future web client) without a real benefit. Skip the current_platform update when the header is blank: the existing stored value is preserved (instead of being nuked to nil by the original buggy code path). Drop the missing_source locale key. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/controllers/shopkeeper_auth/sessions_controller.rb | 8 +++----- config/locales/en.yml | 1 - .../shopkeeper_auth/sessions_controller_test.rb | 7 +++---- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/app/controllers/shopkeeper_auth/sessions_controller.rb b/app/controllers/shopkeeper_auth/sessions_controller.rb index 9216b20..4739587 100644 --- a/app/controllers/shopkeeper_auth/sessions_controller.rb +++ b/app/controllers/shopkeeper_auth/sessions_controller.rb @@ -1,13 +1,11 @@ class ShopkeeperAuth::SessionsController < DeviseTokenAuth::SessionsController def create - source = request.headers["source"] - if source.blank? - return render json: {code: 401, error_message: I18n.t("devise_token_auth.sessions.missing_source")}, status: :unauthorized - end - super return if @resource.blank? + source = request.headers["source"] + return if source.blank? + @resource.current_platform = source @resource.save!(validate: false) end diff --git a/config/locales/en.yml b/config/locales/en.yml index 5d053f1..626b6d1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -88,7 +88,6 @@ en: not_confirmed: "A confirmation email was sent to your account at '%{email}'. You must follow the instructions in the email before your account can be activated." bad_credentials: "Invalid email or password. Please try again." user_not_found: "User was not found or was not signed in." - missing_source: "Missing 'source' header." passwords: missing_email: "You must provide an email address." missing_redirect_url: "Missing redirect URL." diff --git a/test/controllers/shopkeeper_auth/sessions_controller_test.rb b/test/controllers/shopkeeper_auth/sessions_controller_test.rb index fd58b59..5509d84 100644 --- a/test/controllers/shopkeeper_auth/sessions_controller_test.rb +++ b/test/controllers/shopkeeper_auth/sessions_controller_test.rb @@ -2,7 +2,7 @@ class ShopkeeperAuth::SessionsControllerTest < ActionDispatch::IntegrationTest test "returns unauthorized if shopkeeper not valid" do - post shopkeeper_session_url, headers: {source: "ios"} + post shopkeeper_session_url assert_response :unauthorized assert response.parsed_body["error_message"] assert_equal I18n.t("devise_token_auth.sessions.bad_credentials"), response.parsed_body["error_message"] @@ -42,7 +42,7 @@ class ShopkeeperAuth::SessionsControllerTest < ActionDispatch::IntegrationTest assert_equal "android", shopkeeper.reload.current_platform end - test "sign-in without source header is rejected with 401" do + test "successful sign-in without source header preserves the existing current_platform" do shopkeeper = shopkeepers(:one) shopkeeper.create_default_account shopkeeper.update_column(:current_platform, "ios") @@ -50,8 +50,7 @@ class ShopkeeperAuth::SessionsControllerTest < ActionDispatch::IntegrationTest post shopkeeper_session_url, params: {email: shopkeeper.email, password: "password"} - assert_response :unauthorized - assert_equal I18n.t("devise_token_auth.sessions.missing_source"), response.parsed_body["error_message"] + assert_response :success assert_equal "ios", shopkeeper.reload.current_platform end end