Skip to content

Conversation

@hila-f-qodo
Copy link

@greptile-apps
Copy link

greptile-apps bot commented Dec 23, 2025

Greptile Summary

This PR adds per-topic unsubscribe functionality allowing users to unsubscribe from individual topics via email links. Users receive topic-specific unsubscribe links in notification emails that toggle their notification level between regular and muted.

Major changes:

  • Added /t/:slug/:id/unsubscribe route and controller action to handle topic-level unsubscription
  • Created Ember.js route, controller, view, and template for the unsubscribe UI
  • Updated email templates to include topic-specific unsubscribe links alongside global preference links
  • Added defensive null check to dropdown-button component

Critical issues found:

  • TopicUser.find_by can return nil in topics_controller.rb:105, causing NoMethodError when no TopicUser record exists
  • XSS vulnerability in template using triple-brace {{{ which allows unescaped HTML from topic titles
  • Consistent typo: stopNotificiationsText (missing 'a') in both controller and template

Confidence Score: 1/5

  • This PR has critical bugs that will cause runtime failures and security vulnerabilities
  • The nil-pointer bug will crash the application when users who haven't interacted with a topic try to unsubscribe. The XSS vulnerability could allow malicious topic titles to execute JavaScript. Both issues must be fixed before merge.
  • app/controllers/topics_controller.rb and app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs require immediate fixes for the nil-pointer and XSS issues

Important Files Changed

Filename Overview
app/controllers/topics_controller.rb Added unsubscribe action with critical nil-pointer bug when TopicUser doesn't exist
app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs Created unsubscribe template with XSS vulnerability and typo in property name
app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 Created controller with typo in property name (stopNotificiations)
app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 Created route to load topic data for unsubscribe page, looks clean
app/models/topic.rb Added unsubscribe_url helper method
config/routes.rb Added unsubscribe routes for topics with and without slug

Sequence Diagram

sequenceDiagram
    participant User
    participant Email
    participant Browser
    participant TopicsController
    participant TopicUser
    participant TopicView
    
    User->>Email: Receives notification email
    Email->>User: Shows unsubscribe link
    User->>Browser: Clicks topic unsubscribe link
    Browser->>TopicsController: GET /t/:slug/:id/unsubscribe
    TopicsController->>TopicView: Create TopicView
    TopicView-->>TopicsController: Topic data
    TopicsController->>TopicUser: find_by(user_id, topic_id)
    TopicUser-->>TopicsController: TopicUser record (or nil)
    alt TopicUser exists
        TopicsController->>TopicUser: Update notification_level
        TopicUser-->>TopicsController: Saved
        TopicsController->>Browser: Render topic with notification controls
    else TopicUser is nil
        TopicsController->>Browser: Error (NoMethodError)
    end
    Browser->>User: Shows unsubscribe page
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

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])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: find_by can return nil if no TopicUser record exists for this user and topic. The subsequent lines will raise NoMethodError when trying to call methods on nil.

Suggested change
tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) || TopicUser.create(user_id: current_user.id, topic_id: params[:topic_id])
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/topics_controller.rb
Line: 105:105

Comment:
**logic:** `find_by` can return `nil` if no `TopicUser` record exists for this user and topic. The subsequent lines will raise `NoMethodError` when trying to call methods on `nil`.

```suggestion
    tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) || TopicUser.create(user_id: current_user.id, topic_id: params[:topic_id])
```

How can I resolve this? If you propose a fix, please make it concise.

@@ -0,0 +1,8 @@
<div class="container">
<p>
{{{stopNotificiationsText}}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Using triple-brace {{{ allows unescaped HTML which creates XSS vulnerability. The fancyTitle could contain malicious HTML/JavaScript.

Suggested change
{{{stopNotificiationsText}}}
{{stopNotificiationsText}}
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs
Line: 3:3

Comment:
**logic:** Using triple-brace `{{{` allows unescaped HTML which creates XSS vulnerability. The `fancyTitle` could contain malicious HTML/JavaScript.

```suggestion
    {{stopNotificiationsText}}
```

How can I resolve this? If you propose a fix, please make it concise.


export default ObjectController.extend({

stopNotificiationsText: function() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: stopNotificiationsText is misspelled (should be stopNotificationsText with 'a'). This typo is referenced in the template.

Suggested change
stopNotificiationsText: function() {
stopNotificationsText: function() {
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6
Line: 5:5

Comment:
**syntax:** `stopNotificiationsText` is misspelled (should be `stopNotificationsText` with 'a'). This typo is referenced in the template.

```suggestion
  stopNotificationsText: function() {
```

How can I resolve this? If you propose a fix, please make it concise.

@@ -0,0 +1,8 @@
<div class="container">
<p>
{{{stopNotificiationsText}}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: stopNotificiationsText is misspelled (should be stopNotificationsText with 'a').

Suggested change
{{{stopNotificiationsText}}}
{{stopNotificationsText}}
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs
Line: 3:3

Comment:
**syntax:** `stopNotificiationsText` is misspelled (should be `stopNotificationsText` with 'a').

```suggestion
    {{stopNotificationsText}}
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants