Benchmark PR 2

[celmis-codereviewer]

Benchmark reproduction of ai-code-review-evaluation#2

[celmis-codereviewer]

❌ CHANGES REQUESTED — blocking findings

Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.

[celmis-codereviewer]

💬 COMMENT — findings to consider

Full findings and scope are in the review summary — one persistent comment, updated in place on every run.

[celmis-codereviewer]

❌ CHANGES REQUESTED — blocking findings

Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.

[celmis-codereviewer]

Why: tu on line 106 can be nil if no TopicUser record exists, or tu.notification_level can be nil for default topic states; line 108 then calls tu.notification_level > ... which raises NoMethodError or ArgumentError when nil is compared with an integer.

🔴 Nil dereference / invalid comparison on nil notification_level

When TopicUser.find_by returns nil (if no TopicUser record exists for the user and topic) or when tu.notification_level is nil (the default database value before a user explicitly sets a notification level), tu.notification_level > TopicUser.notification_levels[:regular] will raise either a NoMethodError or an ArgumentError (comparison of NilClass with Fixnum failed).

Suggested change

	tu.notification_level = TopicUser.notification_levels[:regular]
	tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id]) || TopicUser.new(user_id: current_user.id, topic_id: params[:topic_id])
	level = tu.notification_level || TopicUser.notification_levels[:regular]
	if level > TopicUser.notification_levels[:regular]
	tu.notification_level = TopicUser.notification_levels[:regular]
	else
	tu.notification_level = TopicUser.notification_levels[:muted]
	end

_{agent: defect · rule: defect.nil-dereference · confidence: 0.95}

[celmis-codereviewer]

Why: title is retrieved from component properties on line 27 and concatenated directly into the HTML string buffer on line 29 without HTML escaping, resulting in cross-site scripting (XSS).

🟠 Unescaped component property in string buffer leads to XSS

The title property is directly interpolated into raw HTML string buffering without escaping HTML entities. If title contains user-supplied text or standard HTML special characters, an attacker can inject arbitrary HTML/JavaScript into the document.

Suggested change

	buffer.push("<h4 class='title'>" + title + "</h4>");
	buffer.push("<h4 class='title'>" + Ember.Handlebars.Utils.escapeExpression(title) + "</h4>");

_{agent: security · rule: sec.cwe-79 · confidence: 0.95}

[celmis-codereviewer]

Why: stopNotificiationsText contains user-controlled topic title content and is rendered using triple curlies {{{...}}} on line 3, bypassing Handlebars HTML escaping and leading to stored XSS.

🟠 Unescaped Handlebars interpolation allows stored XSS

Using triple curlies {{{stopNotificiationsText}}} instructs Handlebars to render the string as raw unescaped HTML. Because stopNotificiationsText incorporates model.fancyTitle (which comes from topic titles created by users), any malicious script inserted into a topic title will be executed in the context of users viewing the unsubscribe page.

Suggested change

	{{{stopNotificiationsText}}}
	{{stopNotificiationsText}}

_{agent: security · rule: sec.cwe-79 · confidence: 0.95}

[celmis-codereviewer]

Why: unsubscribe changes user notification state in the database on line 113 via an HTTP GET request, leaving the action vulnerable to Cross-Site Request Forgery (CSRF).

🟠 State-changing action executed via HTTP GET request (CSRF)

The unsubscribe controller method modifies database state by toggling TopicUser#notification_level and saving the record. Because this action is routed via HTTP GET (/t/:topic_id/unsubscribe) and skips XHR checks, an attacker can embed an image tag or send a link (e.g., <img src="/t/123/unsubscribe">) to silently alter notification settings for logged-in users.

Suggested change

	def unsubscribe
	# Change route to use POST/PUT and ensure CSRF token verification is enforced for state changes.

_{agent: security · rule: sec.cwe-352 · confidence: 0.95}

[celmis-codereviewer]

Why: When an email is generated with add_unsubscribe_link: true without providing an unsubscribe_url option (such as digest emails or system notifications), I18n.t('unsubscribe_link', ...) raises I18n::MissingInterpolationArgument because %{unsubscribe_url} on line 1855 is required in the translation template.

🟠 Missing interpolation argument %{unsubscribe_url} breaks emails that use add_unsubscribe_link

The updated unsubscribe_link locale key now references %{unsubscribe_url}. Email::MessageBuilder does not include unsubscribe_url in its default @template_args, and unsubscribe_url is only passed in UserNotifications.send_notification_email. Any other email configured with add_unsubscribe_link: true (e.g. digest emails or account notifications) will raise I18n::MissingInterpolationArgument when rendering the email footer.

Suggested change

	To stop receiving notifications about this particular topic, [click here](%{unsubscribe_url}).
	unsubscribe_link: |
	To unsubscribe from these emails, visit your [user preferences](%{user_preferences_url}).
	topic_unsubscribe_link: |
	To stop receiving notifications about this particular topic, [click here](%{unsubscribe_url}).

_{agent: defect · rule: defect.missing-argument · confidence: 0.95}

[celmis-codereviewer]

Why: template_args on line 68 is referenced without the @ prefix, raising a NameError (undefined local variable or method 'template_args') when html_part renders the unsubscribe link.

🟠 Undefined local variable template_args missing @ prefix

On line 68 of lib/email/message_builder.rb, template_args is referenced as a local variable instead of using the instance variable @template_args (which is initialized on line 24 and used on line 63). When html_part runs with @opts[:add_unsubscribe_link] enabled, executing this line will raise NameError: undefined local variable or method 'template_args'.

Suggested change

	html_override.gsub!("%{unsubscribe_link}", unsubscribe_link)
	unsubscribe_link = PrettyText.cook(I18n.t('unsubscribe_link', @template_args)).html_safe

_{agent: defect · rule: defect.undefined-variable · confidence: 0.95}

[celmis-codereviewer]

Why: tu can be nil on line 105 if the user has no existing TopicUser record, causing an unhandled NoMethodError when tu.notification_level is dereferenced on line 107.

🟠 Unhandled null object reference when TopicUser record is absent

TopicUser.find_by returns nil if the current user has not previously interacted with or tracked the specified topic. Calling tu.notification_level directly assumes tu is present, causing an unhandled NoMethodError and 500 server error when a user accesses the unsubscribe URL for a topic without an existing TopicUser record.

Suggested change

	if tu.notification_level > TopicUser.notification_levels[:regular]
	tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
	if tu
	if tu.notification_level > TopicUser.notification_levels[:regular]
	tu.notification_level = TopicUser.notification_levels[:regular]
	else
	tu.notification_level = TopicUser.notification_levels[:muted]
	end
	tu.save!
	end

_{agent: security · rule: sec.cwe-476 · confidence: 0.90}

[celmis-codereviewer]

🤖 Code Review for PR #12

⚙ ADJUSTED — graph context partial (17 of 18 changed files): 1 of 18 changed files have no symbols in the index; 1 of them is not in that checkout at all (spec/components/email/message_builder_spec.rb) — this PR's base is older than the indexed revision, so those files were renamed or deleted before it and no re-index can bring them back; there is nothing to fix.

❌ CHANGES REQUESTED — blocking findings

Findings

• 🔴 Critical: 1
• 🟠 Error: 6

Scope

• Files changed: 18
• Lines: +156 / -69

Performance

• Analysis time: 205.3s · agents: structural, cve, contract, security, defect · tokens: 35,587/29,432

---

_{Powered by Code Analyzer · context: tree-sitter graph + structural, cve, contract, security, defect}