Skip to content

Benchmark PR 4 - #14

Open
celmis-codereviewer wants to merge 1 commit into
cr-base-4from
cr-pr-4
Open

Benchmark PR 4#14
celmis-codereviewer wants to merge 1 commit into
cr-base-4from
cr-pr-4

Conversation

@celmis-codereviewer

Copy link
Copy Markdown

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

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

@celmis-codereviewer celmis-codereviewer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

<%= link_to(I18n.t('embed.title'), @topic_view.topic.url, class: 'button', target: '_blank') %>
<%- else %>
<%= link_to(I18n.t('embed.start_discussion'), @topic_view.topic.url, class: 'button', target: '_blank') %>
<%- end if %>

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: Line 6 contains <%- end if %>, which generates invalid Ruby code (end if) that raises a SyntaxError when ActionView renders app/views/embed/best.html.erb.

🔴 Syntax error in ERB template (end if)

The ERB block on line 6 finishes with <%- end if %> instead of <%- end %>. In Ruby, end if without a condition following if is invalid syntax and will raise a SyntaxError whenever this view is rendered.

Suggested change
<%- end if %>
<%- end %>

agent: defect · rule: defect.syntax · confidence: 1.00

return if user.blank?

require 'simple-rss'
rss = SimpleRSS.parse open(SiteSetting.feed_polling_url)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: Line 29 passes SiteSetting.feed_polling_url directly to Kernel open, allowing an attacker who configures a URL prefixed with a pipe symbol to execute arbitrary shell commands.

🔴 Command injection via Kernel.open on feed_polling_url

In Ruby, open() from Kernel executes shell commands if the passed string starts with a pipe (|). If SiteSetting.feed_polling_url is user-controlled or configured to start with |, arbitrary system commands will be executed on the server.

Suggested change
rss = SimpleRSS.parse open(SiteSetting.feed_polling_url)
rss = SimpleRSS.parse URI.open(SiteSetting.feed_polling_url)

agent: security · rule: sec.cwe-78 · confidence: 0.98

Comment thread app/models/topic_embed.rb
require 'ruby-readability'

opts = opts || {}
doc = Readability::Document.new(open(url).read,

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: Line 48 passes the untrusted url string directly to Kernel open, enabling arbitrary command execution if the URL starts with a pipe character.

🔴 Command injection via Kernel.open on user-controlled embed URL

Calling open(url) in Ruby uses Kernel#open, which evaluates input starting with a pipe character (|) as a subshell command. Because url originates from query parameters supplied to the embed endpoint, an attacker can trigger OS command execution on the host.

Suggested change
doc = Readability::Document.new(open(url).read,
doc = Readability::Document.new(URI.open(url).read,

agent: security · rule: sec.cwe-78 · confidence: 0.98

url = i.link
url = i.id if url.blank? || url !~ /^https?\:\/\//

content = CGI.unescapeHTML(i.content.scrub)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: i.content can be nil on line 35 when an RSS feed item lacks a tag; calling .scrub on it without a check raises NoMethodError.

🟠 Unchecked nil call on i.content raises NoMethodError

In RSS 2.0 or other feed formats where <content> is absent, SimpleRSS returns nil for i.content. Calling i.content.scrub without checking if i.content is present raises a NoMethodError: undefined method 'scrub' for nil:NilClass and crashes the PollFeed job.

Suggested change
content = CGI.unescapeHTML(i.content.scrub)
content = CGI.unescapeHTML((i.content || i.description || '').scrub)

agent: defect · rule: defect.nil-dereference · confidence: 0.95

Comment thread app/models/post.rb
def cook(*args)
# For some posts, for example those imported via RSS, we support raw HTML. In that
# case we can skip the rendering pipeline.
return raw if cook_method == Post.cook_methods[:raw_html]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: Line 133 bypasses Discourse's post cooking pipeline and HTML sanitization when cook_method is raw_html, allowing untrusted HTML content imported from feeds or remote URLs to execute stored XSS.

🟠 Stored XSS via raw_html cook_method bypassing post sanitization

When cook_method == Post.cook_methods[:raw_html], the post content (raw) is returned verbatim without passing through the standard post cooking and HTML sanitization pipeline. Any script tags or malicious HTML attributes present in imported feed or remote content will be rendered directly in users' browsers.

Suggested change
return raw if cook_method == Post.cook_methods[:raw_html]
return Sanitize.fragment(raw, Sanitize::Config::RELAXED) if cook_method == Post.cook_methods[:raw_html]

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

window.onload = function() {
if (parent) {
// Send a post message with our loaded height
parent.postMessage({type: 'discourse-resize', height: document['body'].offsetHeight}, '<%= request.referer %>');

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: Line 11 interpolates the HTTP request.referer header directly into an inline JavaScript snippet inside an ERB template, allowing an attacker to execute arbitrary JavaScript by sending a crafted Referer header.

🟠 Reflected Cross-Site Scripting (XSS) via request.referer interpolation

The ERB view template interpolates <%= request.referer %> directly inside a single-quoted JavaScript string within a <script> tag without HTML or JS escaping. An attacker supplying a Referer header containing quotes or HTML script tags (e.g. http://example.com/';alert(1)//) can inject and execute arbitrary JavaScript.

Suggested change
parent.postMessage({type: 'discourse-resize', height: document['body'].offsetHeight}, '<%= request.referer %>');
parent.postMessage({type: 'discourse-resize', height: document['body'].offsetHeight}, <%= request.referer.to_json %>);

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

iframe.id = 'discourse-embed-frame';
iframe.width = "100%";
iframe.frameBorder = "0";
iframe.scrolling = "no";

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: comments can be null on line 5 if the target element #discourse-comments is missing from the document; calling comments.appendChild on line 11 raises a TypeError.

🟠 Unchecked null reference when accessing discourse-comments element

If the host page embedding Discourse does not have an element with id="discourse-comments", document.getElementById('discourse-comments') evaluates to null. Calling comments.appendChild(iframe) without a null check throws an unhandled TypeError and halts script execution.

Suggested change
iframe.scrolling = "no";
if (comments) {
comments.appendChild(iframe);
}

agent: defect · rule: defect.nil-dereference · confidence: 0.90

raise Discourse::InvalidAccess.new('embeddable host not set') if SiteSetting.embeddable_host.blank?
raise Discourse::InvalidAccess.new('invalid referer host') if URI(request.referer || '').host != SiteSetting.embeddable_host

response.headers['X-Frame-Options'] = "ALLOWALL"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: Line 28 sets the X-Frame-Options header to ALLOWALL, disabling standard framing restrictions and exposing the endpoint to clickjacking attacks.

🟡 Weak X-Frame-Options response header set to ALLOWALL

Setting X-Frame-Options to ALLOWALL allows any third-party domain to embed this Discourse response within an <iframe>, weakening clickjacking protection.

Suggested change
response.headers['X-Frame-Options'] = "ALLOWALL"
response.headers['X-Frame-Options'] = "SAMEORIGIN"

agent: security · rule: sec.cwe-1021 · confidence: 0.95

@celmis-codereviewer

Copy link
Copy Markdown
Author

🤖 Code Review for PR #14

CHANGES REQUESTED — blocking findings

Findings

  • 🔴 Critical: 3
  • 🟠 Error: 4
  • 🟡 Warning: 1

Scope

  • Files changed: 27
  • Lines: +646 / -13

Performance

  • Analysis time: 239.4s · agents: structural, cve, contract, security, defect · tokens: 45,543/30,610

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

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.

2 participants