From 72b34e4394b105590b47451c4fdecc7de44b943f Mon Sep 17 00:00:00 2001 From: dadachi Date: Sat, 10 Oct 2026 17:16:36 +0900 Subject: [PATCH 1/3] Stop failed sign-ins writing current_platform; fix resending orphaned invites devise_token_auth looks the shopkeeper up by email before checking the password, and SessionsController#create saved the `source` header whenever that lookup succeeded. Anyone who knew an email could set the shopkeeper's current_platform to any string, skipping validation; the record then failed its ios/android inclusion check, so that shopkeeper's profile updates returned 422. The platform is now written from render_create_success, which runs only after a successful sign-in, and only for "ios" or "android". Deleting an inviter nullifies accounts_invitations.invited_by_id, and resending such an invitation raised NoMethodError on nil.name in the mail job. The invitation mail now names the app (mobile_app.name) instead. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WyUzNEPWyhjjWrBcSaXSW4 --- .../shopkeeper_auth/sessions_controller.rb | 25 +++++++------- app/mailers/shopkeeper/notification_mailer.rb | 8 +++-- app/models/shopkeeper.rb | 4 ++- .../notification_mailer/invited.html.erb | 2 +- .../notification_mailer/invited.text.erb | 2 +- .../sessions_controller_test.rb | 33 +++++++++++++++++++ .../shopkeeper/notification_mailer_test.rb | 19 +++++++++++ 7 files changed, 76 insertions(+), 17 deletions(-) diff --git a/app/controllers/shopkeeper_auth/sessions_controller.rb b/app/controllers/shopkeeper_auth/sessions_controller.rb index 76cc772..8b4ae29 100644 --- a/app/controllers/shopkeeper_auth/sessions_controller.rb +++ b/app/controllers/shopkeeper_auth/sessions_controller.rb @@ -15,20 +15,13 @@ class ShopkeeperAuth::SessionsController < DeviseTokenAuth::SessionsController if: -> { params[:email].present? }, with: RENDER_LOGIN_THROTTLED - def create - super - return if @resource.blank? - - source = request.headers["source"] - return if source.blank? - - @resource.current_platform = source - @resource.save!(validate: false) - end - protected + # devise_token_auth calls this only after the password checks out, so a + # failed sign-in never writes the platform def render_create_success + update_current_platform + @resource.token = @token.token @resource.client = @token.client @resource.expiry = @token.expiry @@ -52,4 +45,14 @@ def render_destroy_success def render_destroy_error render json: {code: 404, error_message: I18n.t("devise_token_auth.sessions.user_not_found")}, status: :not_found end + + private + + # The apps send "ios" or "android"; anything else keeps the stored value + def update_current_platform + source = request.headers["source"] + return unless Shopkeeper::CURRENT_PLATFORMS.include?(source) + + @resource.update_column(:current_platform, source) + end end diff --git a/app/mailers/shopkeeper/notification_mailer.rb b/app/mailers/shopkeeper/notification_mailer.rb index 1d50784..2995e8c 100644 --- a/app/mailers/shopkeeper/notification_mailer.rb +++ b/app/mailers/shopkeeper/notification_mailer.rb @@ -2,15 +2,17 @@ class Shopkeeper::NotificationMailer < ShopkeeperMailer def invited @accounts_invitation = params[:accounts_invitation] @account = @accounts_invitation.account - @invited_by = @accounts_invitation.invited_by + # invited_by is nullified when the inviter is deleted; the invitation can + # still be resent, so name the app instead + @inviter_name = @accounts_invitation.invited_by&.name || ConfigSettings.mobile_app.name name = @accounts_invitation.name email = @accounts_invitation.email mail( to: email_address_with_name(email, name), - from: email_address_with_name(ConfigSettings.email.default_from, @invited_by.name), - subject: I18n.t("shopkeeper.notification_mailer.invited.subject", inviter: @invited_by.name, account: @account.name) + from: email_address_with_name(ConfigSettings.email.default_from, @inviter_name), + subject: I18n.t("shopkeeper.notification_mailer.invited.subject", inviter: @inviter_name, account: @account.name) ) end diff --git a/app/models/shopkeeper.rb b/app/models/shopkeeper.rb index 0994f85..c108459 100644 --- a/app/models/shopkeeper.rb +++ b/app/models/shopkeeper.rb @@ -1,4 +1,6 @@ class Shopkeeper < ApplicationRecord + CURRENT_PLATFORMS = %w[ios android].freeze + extend Devise::Models # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable and :omniauthable @@ -25,7 +27,7 @@ class Shopkeeper < ApplicationRecord validates :current_platform, presence: true, - inclusion: {in: %w[ios android]} + inclusion: {in: CURRENT_PLATFORMS} # override devise method to include additional info as opts hash def send_confirmation_instructions(opts = {}) diff --git a/app/views/shopkeeper/notification_mailer/invited.html.erb b/app/views/shopkeeper/notification_mailer/invited.html.erb index dcf4057..ba86449 100644 --- a/app/views/shopkeeper/notification_mailer/invited.html.erb +++ b/app/views/shopkeeper/notification_mailer/invited.html.erb @@ -1,5 +1,5 @@

- <%= t(".header_html", inviter: @invited_by.name, account: @account.name) %> + <%= t(".header_html", inviter: @inviter_name, account: @account.name) %>

diff --git a/app/views/shopkeeper/notification_mailer/invited.text.erb b/app/views/shopkeeper/notification_mailer/invited.text.erb index 24f3a4d..7b6a0b0 100644 --- a/app/views/shopkeeper/notification_mailer/invited.text.erb +++ b/app/views/shopkeeper/notification_mailer/invited.text.erb @@ -1,4 +1,4 @@ -<%= strip_tags t(".header_html", inviter: @invited_by.name, account: @account.name) %> +<%= strip_tags t(".header_html", inviter: @inviter_name, account: @account.name) %> <%= t(".you_can_accept_or_decline") %> diff --git a/test/controllers/shopkeeper_auth/sessions_controller_test.rb b/test/controllers/shopkeeper_auth/sessions_controller_test.rb index 5509d84..f991a96 100644 --- a/test/controllers/shopkeeper_auth/sessions_controller_test.rb +++ b/test/controllers/shopkeeper_auth/sessions_controller_test.rb @@ -53,4 +53,37 @@ class ShopkeeperAuth::SessionsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_equal "ios", shopkeeper.reload.current_platform end + + # Bug: devise_token_auth finds the shopkeeper by email before checking the + # password, so the source header used to be saved on a failed sign-in too. + # Anyone knowing an email could set current_platform to any string, which + # then failed the inclusion validation and broke that shopkeeper's updates. + test "failed sign-in does not change 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: "wrong-password"}, + headers: {source: "android"} + + assert_response :unauthorized + assert_equal "ios", shopkeeper.reload.current_platform + end + + # Shopkeeper validates current_platform against ios/android + # (Shopkeeper::CURRENT_PLATFORMS); a sign-in must not store anything else. + test "successful sign-in with an unsupported 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"}, + headers: {source: "hacked"} + + assert_response :success + assert_equal "ios", shopkeeper.reload.current_platform + assert shopkeeper.valid?, shopkeeper.errors.full_messages.to_sentence + end end diff --git a/test/mailers/shopkeeper/notification_mailer_test.rb b/test/mailers/shopkeeper/notification_mailer_test.rb index d9fe870..ea40599 100644 --- a/test/mailers/shopkeeper/notification_mailer_test.rb +++ b/test/mailers/shopkeeper/notification_mailer_test.rb @@ -29,6 +29,25 @@ class Shopkeeper::NotificationMailerTest < ActionMailer::TestCase assert_match @account.name, mail.body.encoded end + # Bug: deleting the inviter nullifies invited_by, and resending such an + # invitation raised NoMethodError on nil.name inside the mail job. The mail + # now names the app instead (config/settings.yml mobile_app.name). + test "invited names the app when the inviter has been deleted" do + @invitation.update!(invited_by: nil) + + mail = Shopkeeper::NotificationMailer.with(accounts_invitation: @invitation).invited + + expected_subject = I18n.t( + "shopkeeper.notification_mailer.invited.subject", + inviter: ConfigSettings.mobile_app.name, + account: @account.name + ) + assert_equal expected_subject, mail.subject + assert_equal [@invitation.email], mail.to + assert_match @invitation.token, mail.body.encoded + assert_match "#{ConfigSettings.mobile_app.name} has invited you", mail.text_part.body.decoded + end + test "confirmation_instructions renders the expected subject, recipient, and body" do mail = Shopkeeper::NotificationMailer.with( resource: @shopkeeper, From 4acc809e5e15483ac8adca7a3789ea5bc480415e Mon Sep 17 00:00:00 2001 From: dadachi Date: Sat, 10 Oct 2026 17:20:28 +0900 Subject: [PATCH 2/3] Clear the RuboCop and erb_lint config warnings Drop the Style/DoubleCopDisableDirective entry (RuboCop removed the cop) and rename .erb-lint.yml to .erb_lint.yml, the name erb_lint now expects. Both tools printed a warning on every run; the lint results are unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WyUzNEPWyhjjWrBcSaXSW4 --- .erb-lint.yml => .erb_lint.yml | 0 .rubocop.yml | 3 --- 2 files changed, 3 deletions(-) rename .erb-lint.yml => .erb_lint.yml (100%) diff --git a/.erb-lint.yml b/.erb_lint.yml similarity index 100% rename from .erb-lint.yml rename to .erb_lint.yml diff --git a/.rubocop.yml b/.rubocop.yml index 59459c1..19eb709 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1126,9 +1126,6 @@ Style/Documentation: Style/DocumentationMethod: Enabled: false -Style/DoubleCopDisableDirective: - Enabled: false - Style/DoubleNegation: Enabled: false From bd528ab105890986aa1f99e9bc4a02b814cbbef4 Mon Sep 17 00:00:00 2001 From: dadachi Date: Sat, 10 Oct 2026 17:20:29 +0900 Subject: [PATCH 3/3] Add CHANGELOG entry for #149 and refresh the test count Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WyUzNEPWyhjjWrBcSaXSW4 --- AGENTS.md | 2 +- CHANGELOG.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6796c55..e5718cc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -103,7 +103,7 @@ Cross-platform push via the `noticed` (v3) and `action_push_native` gems. APNs f - `json_response` for parsing JSON API responses - `create_new_auth_token` for generating auth headers (Devise Token Auth) - Fixtures in test/fixtures/ and seed data in db/fixtures/test/ -- Run tests: `bin/rails test` (537 tests, 1331 assertions) +- Run tests: `bin/rails test` (540 tests, 1342 assertions) ### Development Server Configuration - `HOST` (Wi-Fi IP) and `PORT` are required in `.env`; `Procfile.dev` uses `${HOST:?...}` so Rails fails loudly if unset, and `development.rb` uses `ENV.fetch("HOST")` for `action_mailer.default_url_options`. Must match `NATIVEAPPTEMPLATE_API_DOMAIN` in the iOS scheme and Android `gradle.properties`. Never `127.0.0.1`, `localhost`, or `0.0.0.0`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 26b1103..e7ce51d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## [Unreleased] +- Fix a failed sign-in saving the `source` header as `current_platform`: anyone knowing an email could set it to any string, after which that shopkeeper's profile updates returned 422. Only successful sign-ins write it, and only `ios` or `android`. Fix resending an invitation whose inviter was deleted (the mail job raised on `nil.name`); the mail names the app instead. Rename `.erb-lint.yml` to `.erb_lint.yml` and drop a removed RuboCop cop from `.rubocop.yml` (#149) - Deploy with Kamal 2 to GitHub Container Registry instead of Render: add `Dockerfile`, `.dockerignore`, `config/deploy.yml` (kamal-proxy serving a Cloudflare Origin Certificate with `forward_headers: true` and `ssl_redirect: false`, Postgres 18 as the `db` accessory on the private Docker network, Solid Queue in Puma, Active Storage volume, amd64 builder), `.kamal/secrets`, `bin/kamal` and `bin/thrust`. Add the missing `GET /up` health check route and `assume_ssl = true` so kamal-proxy's health check passes. Add Cloudflare's ranges to `trusted_proxies` (`config/cloudflare_ips.yml`) so `rate_limit` keys on the caller rather than the edge. Remove `render.yaml`, `bin/render-*.sh` and the staging environment, which only served Render PR previews. New tests: `test/config/deploy_config_test.rb`, `test/integration/health_check_test.rb`, `test/integration/trusted_proxies_test.rb` (#144) - Point the `SOLID_QUEUE_IN_PUMA` comment in `.env.sample` at `config/deploy.yml`; it still referred to the removed `render.yaml` (#147) - Require `devise_token_auth ~> 1.3` in the Gemfile (the lockfile already resolved 1.3.0) so `bundle update` cannot step back to 1.2.x (#142) @@ -23,7 +24,7 @@ - API change: members and invitations must have at least one role (422 "Roles can't be blank"). A role-less member made `GET /permissions` raise (#121) - API change: password reset and confirmation `redirect_url` must be on the API's own host. Other hosts get 422 for resets and fall back to the default page for confirmations (#120) - Harden admin sign-in: rate limit by IP and email, `reset_session` on sign-in and sign-out, and Madmin now rejects a deleted admin's session (#119) -- AGENTS.md: correct the CORS and test-worker notes, and keep the test count current (#118, #127, #132, #147) +- AGENTS.md: correct the CORS and test-worker notes, and keep the test count current (#118, #127, #132, #147, #149) - Clear finished Solid Queue jobs every hour via `config/recurring.yml` (#117) - API change: `item_tags#update` no longer accepts `state`. Completion goes through the `complete`/`idle` events, and an unknown state no longer returns 500 (#116) - API change: API errors are always JSON `{code, error_message}`, including missing records (404), missing parameters (400), unknown API routes and 500s; before, they were HTML unless the request sent a JSON Content-Type (#115)