Discourse / PR #26

FEATURE: per-topic unsubscribe option in emails

discourse/discourse@6669a2d9
Golden recall: 50% Extra findings: 13

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 logic: Potential nil pointer exception - if no TopicUser record exists, tu will be nil and calling methods on it will crash HIGH ✓ Found "TopicUser.find_by(user_id: ..., topic_id: ...) can legitimately return nil (e.g. forwarded email, topic never visited, regular notification), and if tu.notification_level > ... then raises NoMethodError. Prefer TopicUser.change(...) (which upserts the row) or guard tu explicitly" (app/controllers/topics_controller.rb L105)
#2 Typo in property name: 'stopNotificiationsText' should be 'stopNotificationsText' (missing 'n' in 'Notifications') LOW ✗ Not found No candidate mentions the typo. The review's 8 low findings all target other concerns — the List-Unsubscribe header dropping the per-topic URL, the 301 on a mutating GET, the unguarded TopicView.new, the cosmetic topic_user.rb diff, the structural template change, the mobile CSS inconsistency, and two locale strings — but none of them names stopNotificiationsText. The typo lives in client-side code the review did not inspect closely. See app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6

Supplementary Findings (13 findings)

Finding File Severity Legitimate?
unsubscribe is a state-changing GET that skips check_xhr; Rails CSRF verification only applies to non-GET by default, so a malicious page can force a logged-in victim to unsubscribe/mute from arbitrary topics via an image/link app/controllers/topics_controller.rb:98 HIGH ✓ Real: a mutating GET with no CSRF protection is a genuine security issue on a logged-in action
%{unsubscribe_url} becomes an implicit required argument of the unsubscribe_link locale string — Rails raises I18n::MissingInterpolationArgument if a caller omits it; the contract is fragile and undocumented lib/email/message_builder.rb:90 MEDIUM ✓ Real: a latent breaking contract; nothing is broken today only because the single caller always passes the value
unsubscribe_url returns a plain topic URL with no token/key, so the per-topic email link only works for a logged-in, currently-authenticated user — inconsistent with the app's established key-based unsubscribe mechanism app/models/topic.rb:719 MEDIUM ✓ Real: a design gap for the feature's core use case (clicking from a mail client without a session)
The new per-topic unsubscribe mechanism is not asserted by the spec: there is no check that the body/html contains the supplied unsubscribe_url (/t/1234/unsubscribe), so the feature this MR adds is untested spec/components/email/message_builder_spec.rb:178 MEDIUM ✓ Real: the feature is delivered without test coverage, and the existing assertion mixes two builder instances
The action writes tu.notification_level directly and calls tu.save!, bypassing TopicUser.change which sets notifications_changed_at, creates the row when absent, and publishes the MessageBus notification_level_change event — so the open topic page keeps a stale badge app/controllers/topics_controller.rb:107 MEDIUM ✓ Real: bypassing the canonical API means the client never learns of the change; stale UI state
The List-Unsubscribe header always points at the global user_preferences_url and drops the per-topic unsubscribe_url entirely; the new spec test only checks .to be_present, not the value lib/email/message_builder.rb:107 LOW ✓ Real: may be intentional (RFC list-level header) but undocumented and arguably at odds with the feature title
redirect_to @topic_view.topic.unsubscribe_url, status: 301 issues Moved Permanently for a URL whose effect is a state change; 301 is cacheable by browsers/intermediaries and semantically wrong for a mutating GET app/controllers/topics_controller.rb:102 LOW ✓ Real: a 301 on a state-changing endpoint can be cached and replayed; 302/303 is correct
TopicView.new(params[:topic_id], current_user) is not guarded like the show action's begin/rescue Discourse::NotFound; a non-existent or unviewable topic raises instead of a clean 404 app/controllers/topics_controller.rb:99 LOW ✓ Real: inconsistent error handling with the sibling show action
The diff to topic_user.rb is purely cosmetic (whitespace/formatting) with no functional change, adding diff noise to a feature MR whose blast radius is large app/models/topic_user.rb:10 LOW ✓ Borderline: cosmetic reformatting noise, a legitimate review nit for a feature MR
The template change is structural/formatting only; the per-topic link still depends on the %{unsubscribe_link} placeholder. Pre-existing quirk: when context_posts.present?, %{respond_instructions} is emitted twice (both gsub'd, so not a rendering bug) app/views/email/notification.html.erb:17 LOW ✓ Borderline: awareness-only, pre-existing duplication — not introduced by this PR
The new rule sets .topic-unsubscribe .notification-options { display: inline-block; } but does not neutralize the mobile base rule (clear: both; float: left); display and float are independent, so the control is floated on mobile vs inline-block on desktop app/assets/stylesheets/common/base/topic.scss:69 LOW ✓ Real: a cross-viewport inconsistency; the override leaves the mobile float in place
change_notification_state omits the trailing period that its sibling stop_notifications string has config/locales/client.en.yml:986 LOW ✓ Borderline: cosmetic consistency fix only
The new plain-text line is appended verbatim (un-cooked markdown [click here](...)) to the body, doubling the raw-markdown-in-text exposure (mirrors the pre-existing user-preferences line); may also break hardcoded email fixtures that assert the old single-line text config/locales/server.en.yml:1852 LOW ✓ Real: raw markdown in a plain-text email, plus a plausible fixture breakage the review calls out

Honest Analysis

FriendlyReviewer found 1 of the 2 golden comments — recall 50%. The golden it caught matters: the nil TopicUser dereference (HIGH) is found precisely at topics_controller.rb L105, with the failure modes enumerated (forwarded email, never-visited topic, regular notification) and the right fix suggested (TopicUser.change, which upserts). That is the crash-in-production bug of this PR, and the review treats it as such, ranking it among its "close look" items.

The missed golden is the typo stopNotificiationsText. None of the 13 findings the review did produce mentions it — the eight low findings are all elsewhere: the List-Unsubscribe header, the 301, the unguarded TopicView, a cosmetic diff, a template restructure, mobile CSS, and two locale strings. This is a characteristic miss: the review reasons about behavior and contracts, while a typo in a client-side property name is a string-level defect that only shows up by comparing the diff token by token. The golden is LOW severity, so the recall loss here is cheap, but it is a real category gap — a mechanical error in the very file the feature centers on (topic-unsubscribe.js.es6) went entirely unexamined.

The supplementary findings are the strongest part of this run. The CSRF-exposed state-changing GET (HIGH) is arguably more important than the whole golden set combined: it lets any page force a logged-in victim to unsubscribe from arbitrary topics. The MessageBus finding (direct write instead of TopicUser.change means the open topic page keeps a stale badge) shows cross-file reasoning — the review knows what the client depends on and checks whether the new endpoint feeds it. The untested-spec finding and the unkeyed-URL design gap are both legitimate feature-level objections. Only the two locale items and the cosmetic-diff item are genuinely minor.

The review also disclosed, in its own words, that one section (mailer/model/view wiring) was delivered in partial form and should be re-confirmed — an honest flag, and relevant here: the missed golden sits precisely in that under-confirmed area. The verdict ("should not be merged as-is") is justified by the CSRF and nil-pointer issues regardless of the typo.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌥️ Feat
2 high, 4 medium, 8 low 1 issue, 1 mitigated

The MR implements a per-topic unsubscribe option in notification emails: Topic#unsubscribe_url is threaded through MessageBuilder#template_args (via the unsubscribe_url opt) and rendered as a per-topic link in both the HTML and plain-text bodies, with matching routes (t/:topic_id/unsubscribe and slug variant), a new topics#unsubscribe controller action, client/server locale strings, and some scoped unsubscribe-page CSS. Routing, locale YAML validity, and the CSS specificity for desktop are correct.

However, the review found several blocking concerns, concentrated in the new controller endpoint: it can raise NoMethodError when no TopicUser row exists, it mutates the notification level directly instead of going through TopicUser.change (so the MessageBus event the client relies on is never published and the open topic page keeps a stale badge), and it performs the state change via a CSRF-exposed GET. In addition, the per-topic unsubscribe_url carries no signed key and requires an authenticated session, which is inconsistent with the app's other (key-based) unsubscribe links. The spec does not actually assert the new per-topic link is rendered, and the unsubscribe_link locale string now implicitly hard-requires %{unsubscribe_url} for any caller enabling add_unsubscribe_link.

Note: one section of the review (mailer/model/view wiring, incl. user_notifications.rb, topic.rb, topic_user.rb, and notification.html.erb) was delivered in partial form and may be less complete/verified than the rest; its findings are included but should be re-confirmed.

Because the controller endpoint has blocking correctness and security issues, this MR should not be merged as-is.

These might need a close look

  • 🔴 app/controllers/topics_controller.rb (L105)
    TopicUser.find_by(user_id: ..., topic_id: ...) can legitimately return nil (e.g. forwarded email, topic never visited, regular notification), and if tu.notification_level > ... then raises NoMethodError. Prefer TopicUser.change(...) (which upserts the row) or guard tu explicitly.
  • 🔴 app/controllers/topics_controller.rb (L98)
    unsubscribe is a state-changing GET that skips check_xhr; Rails CSRF verification only applies to non-GET by default, so a malicious page can force a logged-in victim to unsubscribe/mute from arbitrary topics via an image/link. Switch to POST/PUT (matching set_notifications/mute) with an authenticity token, or at minimum require check_xhr / an explicit token.

Worth checking

  • 🟡 lib/email/message_builder.rb (L90)
    The unsubscribe_link locale string now contains %{unsubscribe_url}, and MessageBuilder interpolates it in both body (line 90) and html_part (line 67) whenever add_unsubscribe_link is true. %{unsubscribe_url} thus becomes an implicit required argument — Rails raises I18n::MissingInterpolationArgument if a caller omits it. Today only user_notifications.rb sets this opt and always passes the value, so nothing is currently broken, but the contract is fragile and undocumented. Guard the interpolation (only render the per-topic sentence when unsubscribe_url is present) or split the locale string into a global prefs string and a per-topic supplement, and cover the add_unsubscribe_link: true without unsubscribe_url case in the spec.
  • 🟡 app/models/topic.rb (L719)
    unsubscribe_url returns a plain topic URL with no token/key, so the per-topic email link only works for a logged-in, currently-authenticated user; clicking from a mail client without a session bounces to login and loses the unsubscribe context. This is inconsistent with the app's established unsubscribe mechanism (e.g. digest uses email_unsubscribe_url(key: ...)). Confirm the design and document it, or emit a signed key and handle the non-authenticated / no-TopicUser state gracefully.
  • 🟡 spec/components/email/message_builder_spec.rb (L178)
    The new per-topic unsubscribe mechanism is not asserted by the spec: there is no check that the body/html contains the supplied unsubscribe_url (/t/1234/unsubscribe), so the feature this MR adds is untested. Additionally, expect(message_with_unsubscribe.body).to match(builder.template_args[:user_preferences_url]) mixes two builder instances; use message_with_unsubscribe.template_args[:user_preferences_url] for clarity (it only passes today because user_preferences_url is a constant derived from Discourse.base_url).
  • 🟡 app/controllers/topics_controller.rb (L107)
    The action writes tu.notification_level directly and calls tu.save!, bypassing the canonical TopicUser.change API which (a) sets notifications_changed_at / notifications_reason_id, (b) creates the row when absent, and (c) publishes the MessageBus /topic/#{topic_id} notification_level_change event — so the open topic page keeps a stale badge/state. Use TopicUser.change(current_user, params[:topic_id], notification_level: new_level). Also note the toggle semantics (a tracking user is only dropped to regular) may not match the email copy promising to stop notifications.
Small things (take or leave)
  • 🔵 lib/email/message_builder.rb (L107)
    The List-Unsubscribe header always points at the global user_preferences_url and drops the per-topic unsubscribe_url entirely; the new spec test only checks .to be_present, not the value. This may be intentional (RFC list-level header), but it should be documented or made per-topic given the feature title.
  • 🔵 app/controllers/topics_controller.rb (L102)
    redirect_to @topic_view.topic.unsubscribe_url, status: 301 issues Moved Permanently for a URL whose effect is a state change. 301 is cacheable by browsers/intermediaries and is semantically wrong for a mutating GET; 302/303 See Other is more appropriate.
  • 🔵 app/controllers/topics_controller.rb (L99)
    TopicView.new(params[:topic_id], current_user) is not guarded like the show action's begin/rescue Discourse::NotFound; for a non-existent or unviewable topic it raises Discourse::NotFound/Discourse::InvalidAccess and relies on the global handler instead of a clean 404/invalid-access response.
  • 🔵 app/models/topic_user.rb (L10)
    The diff to topic_user.rb is purely cosmetic (whitespace/formatting) with no functional change, adding diff noise to a feature MR whose blast radius is large. Recommend keeping such reformatting out of feature MRs or splitting it into a separate commit.
  • 🔵 app/views/email/notification.html.erb (L17)
    The template change is structural/formatting only; the per-topic link still depends on the %{unsubscribe_link} placeholder substituted in MessageBuilder#html_part. Pre-existing quirk (not introduced here): when context_posts.present?, %{respond_instructions} is emitted twice — both are gsub'd so not a rendering bug, but it is duplicated content. Awareness only.
  • 🔵 app/assets/stylesheets/common/base/topic.scss (L69)
    The new rule sets .topic-unsubscribe .notification-options { display: inline-block; } but does not neutralize the mobile base rule (.notification-options { clear: both; float: left; margin-top: 0; padding-top: 1px; } in mobile/topic.scss:39). display and float are independent, so the control is floated on mobile vs inline-block on desktop — a cross-viewport inconsistency. Reset float: none; clear: none; within the scope or document the intent.
  • 🔵 config/locales/client.en.yml (L986)
    change_notification_state omits the trailing period that its sibling stop_notifications string has; cosmetic consistency fix only.
  • 🔵 config/locales/server.en.yml (L1852)
    The new To stop receiving notifications about this particular topic, [click here](%{unsubscribe_url}). line is appended verbatim (un-cooked markdown [click here](...)) to the plain-text body, doubling the raw-markdown-in-text exposure (mirrors the pre-existing user-preferences line). Also note this locale change may break hardcoded email fixtures (e.g. spec/fixtures/emails/*.eml, receiver_spec.rb) that assert the old single-line text.

✅ Feature-level checklist

Not there yet

  • ❌ The new per-topic unsubscribe mechanism is covered by automated tests.
    The spec does not assert that the per-topic link is rendered — it only checks the pre-existing global user_preferences_url body line, so the new mechanism itself is untested.

Partially covered

  • ⚠️ Per-topic unsubscribe option is provided in notification emails (link rendered with an actionable destination).
    The per-topic unsubscribe_url is threaded through MessageBuilder and rendered in both HTML and plain-text bodies, with routing and a controller action implemented. However the link is unauthenticated/unkeyed (requires a logged-in session, inconsistent with the app's key-based unsubscribes) and the controller endpoint that processes it has correctness and CSRF issues, so the feature as delivered is only partially satisfactory.