Skip to content

Commit 761ac9f

Browse files
sethherrclaude
andcommitted
Bind the registration confirmation token to the address it was mailed to
owner_email stays editable after the confirmation link goes out, and clean_params deep-merges, so the token survived an address change - which let a registrant point their own link at someone else's account and be signed in as them. The token now records the address it was minted for and reads as absent for any other, so the next send mints a fresh one. Also from the review: - Confirm an existing unconfirmed account, like sign_in_with_magic_link does. Without it the auth cookie was set for a user current_user won't resolve, so the registrant got "Logged in!" and then please_confirm_email, with the single-use token already spent. - flash[:notice] rather than flash[:info], which isn't a UI::Alerts::Base::Component kind (it raised when rendered). - bin/rake prepare_translations, for the i18n normalization spec. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0934a3f commit 761ac9f

6 files changed

Lines changed: 81 additions & 15 deletions

File tree

app/components/emails/register_confirmation/component.en.yml

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,13 @@ en:
33
components:
44
emails:
55
register_confirmation:
6-
click_below_to_pick_up_where_you_left_off: >-
7-
Click below to confirm this email address and pick up where you left off.
6+
click_below_to_pick_up_where_you_left_off: Click below to confirm this email
7+
address and pick up where you left off.
88
confirm_and_continue: Confirm my email
99
confirm_your_email: Confirm your email
1010
link_signs_you_in: >-
1111
This link signs you in to Bike Index, so don't forward it. It works for
1212
%{days} days.
1313
we_saved_your_registration_html: >-
14-
We saved your registration for a <strong>%{color_and_brand}</strong> on the
15-
world's most comprehensive bike registry.
14+
We saved your registration for a <strong>%{color_and_brand}</strong> on
15+
the world's most comprehensive bike registry.

app/controllers/register_controller.rb

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ def confirm_email
124124
# Someone else's session stays theirs - the registration is still finished for the
125125
# address that was emailed, it just isn't that account's own
126126
if current_user.present?
127-
flash[:info] = translation(:signed_in_as_other, email: current_user.email) unless @b_param.self_made?(current_user)
127+
flash[:notice] = translation(:signed_in_as_other, email: current_user.email) unless @b_param.self_made?(current_user)
128128
elsif sign_in_confirmed_user.blank?
129129
return redirect_to_current_step
130130
end
@@ -166,6 +166,8 @@ def sign_in_confirmed_user
166166
return nil
167167
end
168168

169+
# The link proved the address, so an account that had never confirmed it now has
170+
user.confirm(user.confirmation_token) unless user.confirmed?
169171
sign_in_user(user)
170172
set_sign_in_flash(user, signed_up)
171173
@current_user = user

app/models/b_param.rb

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -455,14 +455,20 @@ def confirm_email!(creator_id: nil)
455455

456456
update(creator_id: self.creator_id || creator_id,
457457
params: params.merge("email_confirmed_at" => Time.current)
458-
.except("email_confirmation_token"))
458+
.except("email_confirmation_token", "email_confirmation_email"))
459459
end
460460

461461
# The emailed link's credential. Distinct from id_token, which is in the registrant's
462462
# own URL before any email goes out - only a secret that lived solely in the email
463-
# proves the address received it
463+
# proves the address received it. Nil once owner_email is edited to a different
464+
# address, since all the token proves is that the one it was mailed to received it
464465
def email_confirmation_token
465-
params["email_confirmation_token"]
466+
params["email_confirmation_token"] if email_confirmation_email == EmailNormalizer.normalize(owner_email)
467+
end
468+
469+
# The address the token in hand was mailed to
470+
def email_confirmation_email
471+
params["email_confirmation_email"]
466472
end
467473

468474
def email_confirmation_token_matches?(token)
@@ -481,6 +487,7 @@ def email_confirmation_token_expired?
481487
def generate_email_confirmation_token!
482488
token = email_confirmation_token_expired? ? SecurityTokenizer.new_token : email_confirmation_token
483489
update(params: params.merge("email_confirmation_token" => token,
490+
"email_confirmation_email" => EmailNormalizer.normalize(owner_email),
484491
"email_confirmation_sent_at" => Time.current))
485492
token
486493
end

config/locales/en.yml

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1921,8 +1921,8 @@ en:
19211921
acknowledge:
19221922
acknowledge_everything: Check every box to agree to these rules
19231923
confirm_email:
1924-
confirmation_link_expired: >-
1925-
That confirmation link has expired — check your email for a new one
1924+
confirmation_link_expired: That confirmation link has expired — check your
1925+
email for a new one
19261926
signed_in_as_other: >-
19271927
You're signed in as %{email}, so we kept you in that account. The registration
19281928
is still saved to the address we emailed.
@@ -1931,10 +1931,11 @@ en:
19311931
manufacturer_required: Manufacturer is required to register
19321932
unable_to_save: We're unable to save that registration, please try again
19331933
find_b_param:
1934-
registration_not_found: We couldn't find that registration — start a new one below
1934+
registration_not_found: We couldn't find that registration — start a new one
1935+
below
19351936
sign_in_confirmed_user:
1936-
unable_to_sign_in: >-
1937-
We confirmed your email, but couldn't sign you in to that account
1937+
unable_to_sign_in: We confirmed your email, but couldn't sign you in to that
1938+
account
19381939
update:
19391940
name_required: Owner name is required to register
19401941
search:

spec/models/b_param_spec.rb

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -906,7 +906,10 @@ def acparams(hash)
906906

907907
context "expired token" do
908908
let(:expired_token) { SecurityTokenizer.new_token(Time.current - BParam::TOKEN_EXPIRATION - 1.day) }
909-
before { b_param.update(params: b_param.params.merge("email_confirmation_token" => expired_token)) }
909+
before do
910+
b_param.update(params: b_param.params.merge("email_confirmation_token" => expired_token,
911+
"email_confirmation_email" => b_param.owner_email))
912+
end
910913

911914
it "doesn't match, and mints a new token" do
912915
expect(b_param.email_confirmation_token_expired?).to be_truthy
@@ -916,5 +919,24 @@ def acparams(hash)
916919
expect(b_param.email_confirmation_token_expired?).to be_falsey
917920
end
918921
end
922+
923+
context "owner_email edited after the link went out" do
924+
let!(:token) { b_param.generate_email_confirmation_token! }
925+
926+
it "drops the token - it only proves the address it was mailed to" do
927+
b_param.clean_params({bike: {owner_email: "someone-else@example.com"}}.as_json)
928+
b_param.save!
929+
expect(b_param.reload.email_confirmation_token_matches?(token)).to be_falsey
930+
931+
# A link for the new address is a new token
932+
expect(b_param.generate_email_confirmation_token!).to_not eq token
933+
end
934+
935+
it "keeps the token when only the address's casing changes" do
936+
b_param.clean_params({bike: {owner_email: "Owner@Example.com"}}.as_json)
937+
b_param.save!
938+
expect(b_param.reload.email_confirmation_token_matches?(token)).to be_truthy
939+
end
940+
end
919941
end
920942
end

spec/requests/register_request_spec.rb

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1028,10 +1028,44 @@ def address_street_field
10281028
# No account for the confirmed address - they're still signed in as themselves
10291029
expect(User.count).to eq 1
10301030
expect(Bike.last).to have_attributes(owner_email:, creator_id: current_user.id)
1031-
expect(flash[:info]).to be_present
10321031
expect(response).to redirect_to step_path.call("finished")
1032+
follow_redirect!
1033+
expect(response.body).to include "signed in as #{current_user.email}"
10331034
end
10341035
end
10351036
end
1037+
1038+
context "owner_email edited after the link went out" do
1039+
let!(:other_user) { FactoryBot.create(:user_confirmed, email: "someone-else@example.com") }
1040+
let(:step_1_params) { {b_param: {manufacturer_id: "Trek", cycle_type: "cargo", owner_email: other_user.email}} }
1041+
1042+
it "doesn't confirm the address the token was never mailed to" do
1043+
post base_url, params: step_1_params.merge(b_param_token: b_param.id_token)
1044+
expect(b_param.reload.owner_email).to eq other_user.email
1045+
1046+
expect { post "#{base_url}/confirm_email", params: confirm_params }
1047+
.to_not change(User, :count)
1048+
expect(b_param.reload.email_confirmed?).to be_falsey
1049+
expect(flash[:error]).to be_present
1050+
# Nobody signed in - the link only ever proved the address it was mailed to
1051+
get "/my_account"
1052+
expect(response.status).to eq 302
1053+
end
1054+
end
1055+
1056+
context "unconfirmed account for the address" do
1057+
let!(:unconfirmed_user) { FactoryBot.create(:user, email: owner_email) }
1058+
1059+
it "confirms it, since the link proved the address" do
1060+
expect { post "#{base_url}/confirm_email", params: confirm_params }
1061+
.to_not change(User, :count)
1062+
expect(unconfirmed_user.reload.confirmed).to be_truthy
1063+
expect(response).to redirect_to step_path.call("2")
1064+
1065+
# Actually signed in, rather than bounced to please_confirm_email
1066+
get "/my_account"
1067+
expect(response.status).to eq 200
1068+
end
1069+
end
10361070
end
10371071
end

0 commit comments

Comments
 (0)