FEATURE: per-topic unsubscribe option in emails

[joelachance]

Benchmark PR recreated from ai-code-review-evaluation/discourse-graphite for Code Review Bench. Upstream: ai-code-review-evaluation#2

[joelachance]

Unsubscribe action mutates state on GET request

The unsubscribe action is routed via get in config/routes.rb, yet it changes the user's notification level and saves. GET requests must be safe/idempotent per HTTP semantics. This means pre-fetchers, browser link previews, and crawlers will trigger the unsubscribe side-effect simply by following the link.

Prevents accidental unsubscription triggered by email clients that pre-fetch links (e.g., Outlook Safe Links, Apple Mail Privacy Protection), bots, or browser prefetch.

Split the action: keep the GET route to render the unsubscribe page (showing current state and a confirmation button), and add a PUT/POST route that performs the actual notification-level change. Update the Ember route's client-side model hook to issue the mutation only on explicit user action. Verify by confirming a GET to the URL does not alter the topic_users row.

[joelachance]

NullPointerError when TopicUser record does not exist

The unsubscribe controller action calls TopicUser.find_by(...) which returns nil when no record exists (e.g., the user never interacted with the topic). The code then immediately calls tu.notification_level on a potentially nil object, which raises a NoMethodError in production.

Prevents a 500 error for any user who clicks the unsubscribe link but has no TopicUser record for that topic.

In app/controllers/topics_controller.rb at the unsubscribe method, guard against tu being nil. Either create a TopicUser record with the desired level, or return an appropriate response. For example: tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) followed by if tu.nil? then TopicUser.change(current_user.id, params[:topic_id], notification_level: TopicUser.notification_levels[:muted]) else the existing logic. Verify with a test where a user who has never visited the topic clicks the unsubscribe link.

[joelachance]

XSS in dropdown-button when title contains user-controlled content

The renderString method in dropdown-button.js.es6 concatenates this.get('title') directly into an HTML string without escaping: buffer.push("<h4 class='title'>" + title + "</h4>"). If a caller passes a title that contains user-controlled content (e.g., a topic title), this is a DOM-based XSS. The NotificationsButton component (R37) sets title: '' but subclasses or other usages could pass unsafe values.

Prevents XSS if any caller passes unescaped user content as the title property.

In app/assets/javascripts/discourse/components/dropdown-button.js.es6 line 27, escape the title before concatenation using Handlebars.Utils.escapeExpression(title) or Discourse's equivalent escaping utility. Verify by passing a title containing <script>alert(1)</script> and confirming it renders as text.

[joelachance]

Unsubscribe URL in emails is not authenticated — any recipient can unsubscribe another user

The unsubscribe_url generated by Topic#unsubscribe_url is simply {topic_url}/unsubscribe with no token or HMAC. It relies solely on current_user (session authentication). However, the URL is placed in email bodies (unsubscribe_link locale). If the email is forwarded or the link shared, any logged-in user visiting it will have their own notification level changed (not necessarily the intended user's). More critically, the List-Unsubscribe header containing this URL may be processed by email clients that do not carry session cookies, potentially resulting in a redirect loop or error rather than the expected unsubscribe.

Ensures that the unsubscribe mechanism works reliably from email clients and that authentication is handled appropriately for one-click unsubscribe flows.

Consider adding a signed token (HMAC of user_id + topic_id + expiry) to the unsubscribe URL so it can work without an active session, similar to the existing DigestUnsubscribeKey pattern in EmailController. At minimum, document that the current implementation requires an active session and will not work from email clients that strip cookies. Verify by clicking the List-Unsubscribe link from an email client without an active Discourse session and confirming appropriate behavior.

[joelachance]

XSS via triple-brace rendering of user-controlled topic title

The Handlebars template unsubscribe.hbs uses {{{stopNotificiationsText}}} (triple braces = unescaped HTML). The computed property interpolates model.fancyTitle into the i18n string. fancyTitle may contain HTML entities from the Discourse fancy_title pipeline, but if a topic title contains injected HTML (admin-created or via a bug), it will be rendered unescaped in the browser.

Eliminates a stored XSS vector where a malicious topic title could execute JavaScript in the context of users visiting the unsubscribe page.

In app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs, replace {{{stopNotificiationsText}}} with a safe approach: either use double-braces {{stopNotificiationsText}} and handle the bold formatting differently (e.g., separate the title into its own escaped binding), or sanitize the title before interpolation in the controller. Verify by creating a topic with a title containing <img src=x onerror=alert(1)> and confirming the script does not execute on the unsubscribe page.

[joelachance]

Handle missing TopicUser record in unsubscribe action to avoid 500s

TopicUser.find_by(...) can return nil (e.g., user never visited the topic or record not created yet). The code immediately dereferences tu.notification_level, which will raise and 500 the request. This is realistic on a link reached from email notifications (user may not have prior TopicUser row).

Prevents a production 500 on a common user journey (email → unsubscribe link), improving reliability and supportability.

In app/controllers/topics_controller.rb#unsubscribe, replace find_by with a creation path when absent. For example: tu = TopicUser.get(params[:topic_id], current_user) || TopicUser.create!(user_id: current_user.id, topic_id: params[:topic_id], notification_level: TopicUser.notification_levels[:regular]) (or use existing helper that ensures a row exists—there appears to be logic in TopicUser for ensuring consistency). Then apply the toggle and save. Verify with a controller/request spec: user with no topic_users row can hit /t/:id/unsubscribe and gets 200/redirect with notification_level updated/created.

[joelachance]

Prevent HTML injection in topic unsubscribe page by removing triple-stash rendering

The unsubscribe template renders stopNotificiationsText via triple-stash ({{{...}}}), which bypasses escaping. That text includes {{title}} interpolated from model.fancyTitle, which is derived from topic title and can be user-controlled. This is a reachable XSS path when a user clicks the unsubscribe link.

Eliminates a direct, user-triggerable XSS on a route that is explicitly linked from emails and thus likely to be visited from untrusted contexts.

In app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs, change {{{stopNotificiationsText}}} to {{stopNotificiationsText}} and update config/locales/client.en.yml to avoid embedding raw <strong> tags in the translation. If bolding is required, use a safe pattern: split into separate keys and wrap the title in a <strong> in the template using escaped title, or use a framework-sanctioned i18n HTML helper that sanitizes interpolations. Verify by setting a topic title containing <script>/<img onerror> and confirming it is escaped on /t/:id/unsubscribe.

[joelachance]

Avoid GET side effects on /t/:id/unsubscribe (CSRF-able notification level change)

TopicsController#unsubscribe mutates TopicUser.notification_level on a GET request. Even if the attacker can't read the response due to SOP, they can still cause the victim’s browser to issue the GET (img tag, link prefetch, etc.), changing the victim’s notification settings without intent.

Prevents a concrete CSRF-style state change that can silently alter user preferences, improving integrity of notification settings.

In app/controllers/topics_controller.rb and config/routes.rb, change the mutating action to a non-GET verb (POST/PUT) and enforce CSRF protection (Rails authenticity token) for HTML flow. If email-click needs to work without CSRF token, use a signed, single-purpose token (e.g., per-user/per-topic signed key) included in the email URL and validate it server-side before changing state. Verification: add/request spec covering that GET does not mutate, and POST with valid token does; and confirm the email link includes the token or routes to a confirmation page before applying changes.

[joelachance]

Fix XSS in dropdown-button title rendering

The dropdown-button component now concatenates title into an HTML string and pushes it directly into the render buffer. If title can contain user-controlled content (or HTML entities), this creates a direct injection path into the DOM.

Prevents a concrete client-side XSS vector in a widely used UI building block (dropdown headers), reducing risk of account takeover via script injection.

In app/assets/javascripts/discourse/components/dropdown-button.js.es6, escape title before concatenating into HTML. Concretely: import an escape helper used elsewhere in the codebase (e.g., Ember/Handlebars escaping utility) and do buffer.push("<h4 class='title'>" + escapeExpression(title) + "</h4>"), or stop constructing raw HTML strings and render the title via a template / bound property that auto-escapes. Verify by adding a minimal JS/QUnit test (if present in repo) or a manual repro: set title to "<img src=x onerror=alert(1)>" and confirm it is rendered as text, not executed.