FEATURE: per-topic unsubscribe option in emails - #2
Conversation
mfeuerstein
left a comment
mfeuerstein
left a comment
There was a problem hiding this comment.
PR Review — approved
Reviewed 18 files. 0 high-severity issues found. Verdict: approved.
app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs (low)
- Reviewed app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs — looks good
app/assets/javascripts/discourse/routes/topic-from-params.js.es6 (medium)
- Reviewed app/assets/javascripts/discourse/routes/topic-from-params.js.es6 — looks good
app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 (low)
- Reviewed app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 — looks good
app/assets/javascripts/discourse/components/dropdown-button.js.es6 (low)
- Reviewed app/assets/javascripts/discourse/components/dropdown-button.js.es6 — looks good
app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 (low)
- Reviewed app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 — looks good
app/assets/javascripts/discourse/routes/app-route-map.js.es6 (low)
- Reviewed app/assets/javascripts/discourse/routes/app-route-map.js.es6 — looks good
app/mailers/user_notifications.rb (low)
- Reviewed app/mailers/user_notifications.rb — looks good
app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6 (low)
- Reviewed app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6 — looks good
app/models/topic_user.rb (low)
- Reviewed app/models/topic_user.rb — looks good
app/assets/stylesheets/common/base/topic.scss (low)
- Reviewed app/assets/stylesheets/common/base/topic.scss — looks good
app/views/email/notification.html.erb (low)
- Reviewed app/views/email/notification.html.erb — looks good
config/routes.rb (low)
- Reviewed config/routes.rb — looks good
app/controllers/topics_controller.rb (low)
- Reviewed app/controllers/topics_controller.rb — looks good
app/models/topic.rb (low)
- Reviewed app/models/topic.rb — looks good
config/locales/server.en.yml (low)
- Reviewed config/locales/server.en.yml — looks good
spec/components/email/message_builder_spec.rb (low)
- Reviewed spec/components/email/message_builder_spec.rb — looks good
config/locales/client.en.yml (low)
- Reviewed config/locales/client.en.yml — looks good
lib/email/message_builder.rb (low)
- Reviewed lib/email/message_builder.rb — looks good
zach-source
left a comment
zach-source
left a comment
There was a problem hiding this comment.
Two nil crashes in unsubscribe logic, CSRF vulnerability on GET routes — see inline.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Test 2
Problem
The PR description is empty ("Test 2"), so the goal is inferred from the diff: add a per-topic unsubscribe option to notification emails — an unsubscribe link in the email body that lands the user on a topic unsubscribe page which mutes/normalizes their notification level for that topic.
Solution Reviewed
- New
Topic#unsubscribe_url("#{url}/unsubscribe") threaded intoUserNotifications#send_notification_emailand theunsubscribe_linki18n string. - New
TopicsController#unsubscribeaction (GET route) that toggles the user'sTopicUser#notification_level(watching/tracking -> regular, otherwise -> muted), then renders the topic show page. - Ember client:
topic-unsubscriberoute/controller/view/template +unsubscribe.hbsconfirmation screen, plus a guard indropdown-buttonto omit the title<h4>when absent. - Email template/locales and a small
message_builder_specupdate; assorted ES6/Ruby style refactors.
Summary
The feature is wired end-to-end, but the core server endpoint has two blocking defects: a nil crash on users without a TopicUser record (a common path for mentioned/category-watching users), and a state-mutating GET route that is a CSRF vector and silently toggles state on prefetch/refresh. These should be fixed before merge.
Files Reviewed
app/controllers/topics_controller.rb— deeply reviewed (unsubscribe action, perform_show_response render change)config/routes.rb— deeply reviewed (route ordering, new unsubscribe routes)app/models/topic.rb,app/models/topic_user.rb— reviewed (unsubscribe_url; stylistic refactor + track_visit! scope)app/mailers/user_notifications.rb,lib/email/message_builder.rb— deeply reviewed (unsubscribe_url plumbing, unsubscribe_link interpolation contract)app/views/email/notification.html.erb— reviewed (template restructure, placeholder substitution)config/locales/client.en.yml,config/locales/server.en.yml— reviewed (new keys, unsubscribe_link block scalar)- Ember JS (route/controller/view/template + app-route-map, topic-from-params, dropdown-button) — reviewed
app/assets/stylesheets/common/base/topic.scss— lightly reviewed (unsubscribe styles)spec/components/email/message_builder_spec.rb— reviewed (assertion coverage gap)
Verification
bundle exec rspec/ Rails test suite — skipped: no Ruby toolchain available in this environment (ruby/bundlenot installed). Static review only.
Issues Found
See inline comments. In addition, one non-blocking suggestion that has no single diff line:
lib/email/message_builder.rb(header_args) — theList-Unsubscribeheader still points atuser_preferences_url(generic preferences page) while the email body now advertises a one-click per-topic unsubscribe URL. For RFC 8058 one-click-unsubscribe compliance and consistency, consider pointing the header attemplate_args[:unsubscribe_url]when present.
Verdict
Recommend changes before merge — fix the nil deref and the state-mutating GET before this ships.
| return redirect_to @topic_view.topic.unsubscribe_url, status: 301 | ||
| end | ||
|
|
||
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) |
There was a problem hiding this comment.
Blocking — nil deref crashes the unsubscribe link for users without a TopicUser record. TopicUser.find_by returns nil when the user has no per-topic record (e.g. they were only @mentioned, are watching via a group/category, or hit the URL directly). The very next line (tu.notification_level) then raises NoMethodError -> 500 on the unsubscribe page — the exact action the email link triggers. The canonical pattern here is TopicUser.change (topic_user.rb:77), which creates the row if missing and sets notifications_changed_at/notifications_reason_id. Use TopicUser.change(current_user.id, params[:topic_id], notification_level: ...) (or guard tu with find_or_initialize_by) instead of raw find_by + save!.
| get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/, post_number: /\d+/} | ||
| get "t/:slug/:topic_id/moderator-liked" => "topics#moderator_liked", constraints: {topic_id: /\d+/} | ||
| get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/} | ||
| get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} |
There was a problem hiding this comment.
Blocking — state-mutating GET route is a CSRF vector and is non-idempotent. unsubscribe is registered as get, but TopicsController#unsubscribe calls tu.save! to change notification_level. Rails CSRF protection does not cover GET requests, so any logged-in user can be silently made to mute/track topics by a cross-origin <img src="/t/:slug/:id/unsubscribe"> embedded anywhere they browse. Additionally the action toggles (watching/tracking -> regular, else -> muted), so link pre-fetching by browsers/email clients (Apple Mail, Outlook) and accidental refreshes silently mutate notification settings. Switch to a POST/DELETE route with a CSRF token (or a signed single-use token), and make the target a stable level rather than a toggle.
| tu.notification_level = TopicUser.notification_levels[:muted] | ||
| end | ||
|
|
||
| tu.save! |
There was a problem hiding this comment.
find_by + save! bypasses TopicUser.change: it sets no notifications_changed_at / notifications_reason_id, publishes no MessageBus /topic/:id notification_level_change event (so other open tabs / the topic page won't reflect the change in real time), and the find->mutate->save has no transaction or locking — a double-click or two concurrent requests both read the same level and both write, collapsing two intended toggles into one. Prefer TopicUser.change, which wraps this in a transaction with a RecordNotUnique rescue.
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
There was a problem hiding this comment.
{{{stopNotificiationsText}}} renders raw HTML. The i18n string embeds <strong>{{title}}</strong> and title is interpolated from model.fancyTitle (user-controlled topic title). It is safe today only because fancy_title is server-escaped; triple-braces bypass Handlebars escaping, so any future change to fancyTitle sanitization becomes a stored-XSS sink. Prefer double-braces and restructure the i18n key to avoid embedded HTML, or sanitize fancyTitle in the controller before interpolation.
| html_override.gsub!("%{respond_instructions}", respond_instructions) | ||
| end | ||
|
|
||
| unsubscribe_link = PrettyText.cook(I18n.t('unsubscribe_link', template_args)).html_safe |
There was a problem hiding this comment.
I18n.t('unsubscribe_link', template_args) (here in html_part, and identically in body) now interpolates %{unsubscribe_url}, which only exists in template_args when the caller passes unsubscribe_url via @opts. Today only UserNotifications#send_notification_email does, so nothing breaks — but the unsubscribe_link translation contract is now tighter and undocumented; any other/future caller setting add_unsubscribe_link: true without unsubscribe_url will raise I18n::MissingInterpolationArgument at send time. Either default unsubscribe_url in the builder or document the requirement.
| raise ex | ||
| end | ||
|
|
||
| def unsubscribe |
There was a problem hiding this comment.
No controller tests exist for the new unsubscribe action (spec/controllers/topics_controller_spec.rb has no unsubscribe context). This is a state-mutating, auth-gated, security-sensitive endpoint — please add coverage for: the notification_level mutation, the slugs_do_not_match/blank-slug 301 redirect, the authenticated-access requirement, and the perform_show_response render path.
| body: 'hello world', | ||
| add_unsubscribe_link: true) } | ||
| add_unsubscribe_link: true, | ||
| unsubscribe_url: "/t/1234/unsubscribe") } |
There was a problem hiding this comment.
The add_unsubscribe_link: true context now passes unsubscribe_url: "/t/1234/unsubscribe", but the assertions only check the List-Unsubscribe header and that user_preferences_url is in the body — they never assert the per-topic unsubscribe URL actually renders. The core feature this PR adds (the per-topic link in the email body) is therefore untested. Add something like expect(message_with_unsubscribe.body).to match("/t/1234/unsubscribe").
| export default Discourse.Route.extend({ | ||
| model(params) { | ||
| const topic = this.store.createRecord("topic", { id: params.id }); | ||
| return PostStream.loadTopicView(params.id).then(json => { |
There was a problem hiding this comment.
The model hook returns PostStream.loadTopicView(params.id).then(...) with no .catch. After the server-rendered page loads, a 404/403/network failure rejects the promise and falls through to Ember's generic error handler — the user sees a generic 'Oops' page with no context that the unsubscribe action failed. Add rejection handling that surfaces an unsubscribe-specific message.
|
|
||
| export default ObjectController.extend({ | ||
|
|
||
| stopNotificiationsText: function() { |
There was a problem hiding this comment.
Typo in the computed property name: stopNotificiationsText -> stopNotificationsText (extra i, missing a). The template at topic/unsubscribe.hbs:3 references the same misspelled name so it works today, but the misspelling will trip up future maintainers/translators. Fix in both places together.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Test 2
Problem
The PR description ("Test 2") is empty, so the goal is inferred from the diff: add a per-topic unsubscribe option to notification emails — a link in the email body that lands the user on an in-app topic unsubscribe page which sets their notification level for that topic.
Solution Reviewed
Topic#unsubscribe_url("#{url}/unsubscribe") threaded intoUserNotifications#send_notification_emailand theunsubscribe_linki18n string.- New
TopicsController#unsubscribeGET action: canonicalizes the slug via 301, then toggles the current user'sTopicUser#notification_level(watching/tracking -> regular, else -> muted) viafind_by+save!, then renders the topic show view. - Ember client route/controller/view/template (
topicUnsubscribeat/t/:slug/:id/unsubscribe) using newclient.en.ymltopic.unsubscribe.*keys;dropdown-buttonguards an empty title. notification.html.erb,message_builder.rb, andserver.en.ymlrestructured;message_builder_spec.rbupdated to passunsubscribe_url.
Summary
The feature is wired end-to-end, but the server unsubscribe action has two blocking defects: a nil crash on the exact email-link path for users without a TopicUser row, and a state-mutating, tokenless GET route that is a CSRF vector and non-idempotent. Several non-blocking issues remain around real-time sync, error handling, raw-HTML rendering, a tightened i18n contract, and missing test coverage. Recommend changes before merge.
Files Reviewed
app/controllers/topics_controller.rb— deeply reviewed (high risk: new state-mutating action)config/routes.rb— deeply reviewed (GET route mapped to a mutating action)app/models/topic.rb,app/models/topic_user.rb— reviewed forunsubscribe_urland theTopicUser.changecontractapp/mailers/user_notifications.rb,lib/email/message_builder.rb— reviewed for the i18n contractapp/views/email/notification.html.erb,config/locales/server.en.yml— reviewed for email renderingapp/assets/javascripts/discourse/**(route/controller/template/view, dropdown-button, topic-from-params) — reviewed; Ember deps (ObjectController,PostStream.loadTopicView,topic-notifications-button) verified presentconfig/locales/client.en.yml— i18n keys consistent with usagespec/components/email/message_builder_spec.rb— reviewed; coverage gap found
Verification
- Static source-context verification: confirmed
PostStream.loadTopicViewexists and is the default export; confirmedObjectControllerexists; confirmedfancy_titleserver-escapes the title; confirmedsend_notification_emailis the only in-repo caller settingadd_unsubscribe_link: trueand that it passesunsubscribe_url; confirmed routes match between Rails and Ember. - Ruby syntax check / spec suite: skipped (no Ruby/bundler environment available in the review sandbox).
Issues Found
🔴 Blocking
config/routes.rb:440— state-mutating, tokenless GETunsubscriberoute: CSRF vector + non-idempotent toggle (see inline).app/controllers/topics_controller.rb:105—TopicUser.find_byreturns nil ->tu.notification_levelraisesNoMethodError-> 500 on the email-link path (see inline).
🟡 Non-blocking
app/controllers/topics_controller.rb:113—save!bypassesTopicUser.change: no MessageBus event / audit (see inline).app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs:3— triple-brace raw-HTML render is a latent XSS sink (see inline).app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6:6— model promise has no rejection handler (see inline).lib/email/message_builder.rb:67—unsubscribe_linki18n now requires%{unsubscribe_url}with no default (see inline).app/controllers/topics_controller.rb:98— no controller spec for the new security-sensitive action (see inline).spec/components/email/message_builder_spec.rb:172— assertions don't verify the per-topic URL renders (see inline).
💡 Suggestions
app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6:5—stopNotificiationsTexttypo (see inline).
Verdict
Recommend changes before merge — fix the nil crash and the CSRF/non-idempotent GET route (switch to a token-gated POST/DELETE or a signed single-use token with a fixed target level) before this ships.
| get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/, post_number: /\d+/} | ||
| get "t/:slug/:topic_id/moderator-liked" => "topics#moderator_liked", constraints: {topic_id: /\d+/} | ||
| get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/} | ||
| get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} |
There was a problem hiding this comment.
Blocking — state-mutating, tokenless GET route is a CSRF vector and is non-idempotent. unsubscribe is registered as get, but TopicsController#unsubscribe calls tu.save! to change notification_level. Rails protect_from_forgery does not cover GET requests, and check_xhr is explicitly skipped for unsubscribe, so a cross-origin <img src="/t/:slug/:id/unsubscribe"> embedded anywhere a logged-in user browses silently mutes their topics. The action toggles (watching/tracking -> regular, else -> muted), so mail-client/browser pre-fetching (Apple Mail, Outlook) and an accidental refresh re-flip the level. The existing secure pattern is EmailController's token-gated email/unsubscribe/:key (DigestUnsubscribeKey.user_for_key).
Suggested fix: use a POST/DELETE route with a CSRF token (or a signed single-use token in the URL), and set a fixed target level instead of toggling:
# routes.rb
post "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: { topic_id: /\d+/ }# topics_controller.rb — set a stable level, don't toggle
TopicUser.change(current_user.id, params[:topic_id], notification_level: TopicUser.notification_levels[:muted])| return redirect_to @topic_view.topic.unsubscribe_url, status: 301 | ||
| end | ||
|
|
||
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) |
There was a problem hiding this comment.
Blocking — nil deref crashes the unsubscribe link for users without a TopicUser record. TopicUser.find_by returns nil when the user has no per-topic row (e.g. they were only @mentioned, are watching via a group/category, or hit the URL directly without ever visiting). The very next line (tu.notification_level) then raises NoMethodError -> 500 on the unsubscribe page — the exact action the email link triggers. The canonical pattern is TopicUser.change (topic_user.rb), which upserts the row if missing and sets notifications_changed_at/notifications_reason_id.
target_level = ->(tu) { tu&.notification_level.to_i > TopicUser.notification_levels[:regular] ? :regular : :muted }
TopicUser.change(current_user.id, params[:topic_id], notification_level: TopicUser.notification_levels[target_level.call(tu)])| tu.notification_level = TopicUser.notification_levels[:muted] | ||
| end | ||
|
|
||
| tu.save! |
There was a problem hiding this comment.
find_by + save! bypasses TopicUser.change, so even when the row exists: (1) no MessageBus.publish("/topic/#{id}", {notification_level_change: ...}) is emitted, so other open tabs / the topic page won't reflect the change in real time; (2) no notifications_changed_at / notifications_reason_id audit is set; (3) the read-modify-write has no transaction or row locking, so a double-click or two concurrent requests both read the same level and collapse two intended toggles into one. TopicUser.change wraps this in a transaction with a RecordNotUnique rescue and publishes the MessageBus event — prefer it over raw find_by + save!.
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
There was a problem hiding this comment.
{{{stopNotificiationsText}}} renders raw HTML. The i18n string embeds <strong>{{title}}</strong> and title is interpolated from model.fancyTitle (user-controlled topic title). It is safe today only because Topic#fancy_title server-escapes via ERB::Util.html_escape before client emoji-unescaping; triple-braces bypass Handlebars escaping, so any future change to fancyTitle sanitization becomes a stored-XSS sink. Prefer double-braces and move the <strong> formatting into the template / a computed that escapes, or restructure the i18n key to avoid embedding HTML:
stopNotificationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),| export default Discourse.Route.extend({ | ||
| model(params) { | ||
| const topic = this.store.createRecord("topic", { id: params.id }); | ||
| return PostStream.loadTopicView(params.id).then(json => { |
There was a problem hiding this comment.
The model hook returns PostStream.loadTopicView(params.id).then(...) with no .catch. After the server-rendered page loads, a 404/403/network failure rejects the promise and falls through to Ember's generic error handler — the user sees a generic 'Oops' page with no indication the unsubscribe action failed. Add rejection handling (or an actions.error hook) that surfaces an unsubscribe-specific message:
return PostStream.loadTopicView(params.id).then(json => {
topic.updateFromJson(json);
return topic;
}).catch(() => {
this.controllerFor("application").set("showFooter", true);
// surface an unsubscribe-specific error to the user
});| html_override.gsub!("%{respond_instructions}", respond_instructions) | ||
| end | ||
|
|
||
| unsubscribe_link = PrettyText.cook(I18n.t('unsubscribe_link', template_args)).html_safe |
There was a problem hiding this comment.
The unsubscribe_link i18n contract is now tighter and undocumented. I18n.t('unsubscribe_link', template_args) (here in html_part, and identically in body) now interpolates %{unsubscribe_url}, which only exists in template_args when the caller passes unsubscribe_url via @opts. Today only UserNotifications#send_notification_email does, so nothing breaks — but any future/plugin caller setting add_unsubscribe_link: true without unsubscribe_url will raise I18n::MissingInterpolationArgument at send time, and non-English locales missing the new line silently omit the per-topic link. Either default unsubscribe_url in the builder or document the requirement:
@template_args = {
site_name: SiteSetting.email_prefix.presence || SiteSetting.title,
base_url: Discourse.base_url,
user_preferences_url: "#{Discourse.base_url}/my/preferences",
unsubscribe_url: "",
}.merge!(@opts)| raise ex | ||
| end | ||
|
|
||
| def unsubscribe |
There was a problem hiding this comment.
No controller tests exist for the new unsubscribe action (spec/controllers/topics_controller_spec.rb has no unsubscribe context). This is a state-mutating, auth-gated, security-sensitive endpoint — please add coverage for: the notification_level mutation, the slugs_do_not_match/blank-slug 301 redirect, the authenticated-access requirement (ensure_logged_in), and the perform_show_response render path (including the failure case where no TopicUser row exists).
| body: 'hello world', | ||
| add_unsubscribe_link: true) } | ||
| add_unsubscribe_link: true, | ||
| unsubscribe_url: "/t/1234/unsubscribe") } |
There was a problem hiding this comment.
The core feature this PR adds (the per-topic unsubscribe link in the email body) is untested. The builder is now constructed with unsubscribe_url: "/t/1234/unsubscribe", but the assertions only check the List-Unsubscribe header and that user_preferences_url is in the body — they never assert the per-topic URL renders. Add:
it "includes the per-topic unsubscribe url in the body" do
expect(message_with_unsubscribe.body).to match("/t/1234/unsubscribe")
end|
|
||
| export default ObjectController.extend({ | ||
|
|
||
| stopNotificiationsText: function() { |
There was a problem hiding this comment.
Typo in the computed property name: stopNotificiationsText -> stopNotificationsText (extra i, missing a). The template at topic/unsubscribe.hbs:3 references the same misspelled name so it works today, but the misspelling will trip up future maintainers/translators. Fix in both places together:
stopNotificationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Test 2
Problem
The PR description ("Test 2") is empty, so the goal is inferred from the diff: add a per-topic unsubscribe option to notification emails — an unsubscribe link in the email body that lands the user on an in-app topic page which changes their notification level for that topic.
Solution Reviewed
- New
Topic#unsubscribe_url("#{url}/unsubscribe") threaded intoUserNotifications#send_notification_emailand theunsubscribe_linki18n string. - New
TopicsController#unsubscribeGET action that toggles the current user'sTopicUser#notification_level(watching/tracking → regular, otherwise → muted) viafind_by+save!, then renders the topic show view. - Ember client:
topic-unsubscriberoute/controller/template/view,app-route-mapentry,dropdown-buttonnull-guard,topic-from-paramsES6 cleanup, locales, and SCSS.
Summary
The feature is wired end-to-end (routes, mailer, builder, i18n, email template, Ember), but the new unsubscribe controller action has two blocking defects: a nil-deref crash on a reachable path and a state-mutating, CSRF-vulnerable, non-idempotent GET route. Several test-coverage gaps for the new state-mutating endpoint round out the review. Recommend changes before merge.
Files Reviewed
app/controllers/topics_controller.rb— deeply reviewed (newunsubscribeaction: nil-deref, bypassesTopicUser.change, CSRF via GET)config/routes.rb— deeply reviewed (GET unsubscribe route = CSRF vector)app/models/topic.rb— deeply reviewed (unsubscribe_urlis correct)app/models/topic_user.rb— deeply reviewed (verifiedchangecontract thatunsubscribebypasses)lib/email/message_builder.rb— deeply reviewed (i18n interpolation path; in-tree callers all wireunsubscribe_url)app/mailers/user_notifications.rb— deeply reviewedapp/views/email/notification.html.erb— lightly reviewed (template/formatting only)app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6— deeply reviewed (missing promise rejection handling)app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6— deeply reviewed (typo)app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs— lightly reviewed (triple-brace safe:fancy_titleis server-escaped)app/assets/javascripts/discourse/components/dropdown-button.js.es6— lightly reviewed (null-guard is correct)app/assets/javascripts/discourse/routes/topic-from-params.js.es6— lightly reviewed (ES6 refactor, no behavior change)app/assets/javascripts/discourse/routes/app-route-map.js.es6— lightly reviewed (route entry correct)app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6— lightly reviewed (trivial)app/assets/stylesheets/common/base/topic.scss— lightly reviewed (CSS only)config/locales/client.en.yml,config/locales/server.en.yml— lightly reviewed (i18n keys consistent)spec/components/email/message_builder_spec.rb— deeply reviewed (assertion gap)
Verification
- Ruby syntax/typecheck — skipped: no Ruby toolchain in the review environment.
- Specialist passes: security (blind), data/edge-case+state, error-handling+integration, tests/intent — all run.
Verdict
Recommend changes before merge — the unsubscribe action crashes on a reachable path and its GET route is a CSRF vector; both are straightforward to fix with TopicUser.change and a token-gated POST/PUT.
|
|
||
| tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) | ||
|
|
||
| if tu.notification_level > TopicUser.notification_levels[:regular] |
There was a problem hiding this comment.
Blocking — nil deref crashes the unsubscribe link for users without a TopicUser row. TopicUser.find_by (line 105) returns nil whenever the user has no per-topic record (mentioned-only, group/category watch, or a direct URL hit), and this line dereferences tu.notification_level, raising NoMethodError → 500 on the exact page the email link opens. Even when the row exists, raw find_by + save! bypasses TopicUser.change (topic_user.rb:77), which wraps the write in a transaction, sets notifications_changed_at/notifications_reason_id, publishes the /topic/:id MessageBus notification_level_change event (so other tabs update live), and rescues RecordNotUnique. Replace the whole block with the canonical upsert:
target = tu&.notification_level.to_i > TopicUser.notification_levels[:regular] ? :regular : :muted
TopicUser.change(current_user.id, params[:topic_id], notification_level: target)| get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/, post_number: /\d+/} | ||
| get "t/:slug/:topic_id/moderator-liked" => "topics#moderator_liked", constraints: {topic_id: /\d+/} | ||
| get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/} | ||
| get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/} |
There was a problem hiding this comment.
Blocking — state-mutating GET route is a CSRF vector and is non-idempotent. unsubscribe is registered as get but the action calls tu.save! to change notification_level. Rails CSRF protection does not cover GET, check_xhr is explicitly skipped for unsubscribe, and ensure_logged_in only checks the session cookie — which browsers attach automatically to cross-origin <img src="/t/123/unsubscribe"> requests — so any page a logged-in user visits can silently flip their notification level on arbitrary topics. The action also toggles (watching/tracking → regular, else → muted), so mail-client/browser pre-fetching or an accidental refresh re-flips the level (unsubscribe → resubscribe). The sibling mute/unmute routes below use put for exactly this reason — switch to a token-gated POST/PUT (or a signed single-use token in the URL) and make the operation idempotent (always set :muted, don't toggle).
| export default Discourse.Route.extend({ | ||
| model(params) { | ||
| const topic = this.store.createRecord("topic", { id: params.id }); | ||
| return PostStream.loadTopicView(params.id).then(json => { |
There was a problem hiding this comment.
PostStream.loadTopicView(...) rejects on 404 (deleted topic), 403 (no access), or network failure, but the model hook has no .catch and afterModel then calls topic.set("details.notificationReasonText", null) on a topic that never loaded — the user sees a generic Ember error page with no indication the unsubscribe failed. Add rejection handling (or an actions.error hook) that surfaces an unsubscribe-specific message:
return PostStream.loadTopicView(params.id).then(json => {
topic.updateFromJson(json);
return topic;
}).catch(() => {
this.controllerFor("application").set("showFooter", true);
// surface an unsubscribe-specific error to the user
});| raise ex | ||
| end | ||
|
|
||
| def unsubscribe |
There was a problem hiding this comment.
The new unsubscribe action is state-mutating, auth-gated, and non-idempotent, but spec/controllers/topics_controller_spec.rb has no unsubscribe context (the file has extensive contexts for sibling actions like show). Please add coverage for: the notification_level toggle, the slugs_do_not_match/blank-slug 301 redirect, the authenticated-access requirement (ensure_logged_in), and the missing-TopicUser-row crash case.
| body: 'hello world', | ||
| add_unsubscribe_link: true) } | ||
| add_unsubscribe_link: true, | ||
| unsubscribe_url: "/t/1234/unsubscribe") } |
There was a problem hiding this comment.
The builder is now constructed with unsubscribe_url: "/t/1234/unsubscribe", but the existing tests only assert the List-Unsubscribe header and that user_preferences_url appears in the body — they never assert the per-topic unsubscribe URL renders. The core feature this PR adds (the per-topic link in the email body) is therefore untested. Add:
it "includes the per-topic unsubscribe url in the body" do
expect(message_with_unsubscribe.body).to match("/t/1234/unsubscribe")
end|
|
||
| export default ObjectController.extend({ | ||
|
|
||
| stopNotificiationsText: function() { |
There was a problem hiding this comment.
Typo in the computed property name: stopNotificiationsText has an extra i (should be stopNotificationsText). The template at topic/unsubscribe.hbs:3 references the same misspelled name so it works today, but fix both together to avoid confusing future maintainers:
stopNotificationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),| context: context, | ||
| username: username, | ||
| add_unsubscribe_link: true, | ||
| unsubscribe_url: post.topic.unsubscribe_url, |
There was a problem hiding this comment.
send_notification_email now passes unsubscribe_url: post.topic.unsubscribe_url, but spec/mailers/user_notifications_spec.rb only asserts has_key(:add_unsubscribe_link) and never that unsubscribe_url is forwarded. If this opt were dropped, the unsubscribe_link i18n string (which now requires %{unsubscribe_url}) would raise I18n::MissingInterpolationArgument only at email-send time, with no spec catching it. Add expects_build_with(has_key(:unsubscribe_url)) beside the existing assertions.
5 participants
Test 2