Discourse / PR #4

Enhance embed URL handling and validation system

marchugon/discourse-cursor-fr-benchmark#8
Golden recall: 100% Extra findings: 12

Golden Comments (6/6 found)

# Comment Severity Status FriendlyReviewer Detail
#1 SSRF via open(url).read in TopicEmbed.import_remote — user-supplied URL passed directly to open() without validation HIGH ✓ Found FriendlyReviewer: "Kernel#open risk: user-supplied URL may execute pipe commands. Use OpenURI.open_uri with explicit timeouts. Also missing error handling for network failures." (topic_embed.rb:40)
#2 Origin validation bypass: discourseUrl.indexOf(e.origin) — malicious subdomain passes the check MEDIUM ✓ Found FriendlyReviewer: "Origin validation uses substring match via indexOf; attacker could craft origin that contains legitimate URL as substring. Use new URL(discourseUrl).origin for exact comparison." (embed.js:20)
#3 postMessage targetOrigin: full referer URL passed instead of just the origin MEDIUM ✓ Found FriendlyReviewer: "targetOrigin receives full referer URL instead of just origin. Use URI(request.referer).origin to parse scheme+host+port." (embed.html.erb:10)
#4 X-Frame-Options: ALLOWALL disables anti-clickjacking protection MEDIUM ✓ Found FriendlyReviewer: "Non-standard X-Frame-Options: ALLOWALL not recognized by modern browsers. Use Content-Security-Policy: frame-ancestors <embeddable_host> instead." (embed_controller.rb:28)
#5 TopicEmbed.import: contents << ... mutates caller's string, and XSS via unescaped URL interpolation MEDIUM ◈ Partial FriendlyReviewer found the string mutation via << (L9) and the TOCTOU race condition between where / create! (L12). Missed NoMethodError on nil contents and XSS via <a href='#{url}'>#{url}</a>.
#6 ERB syntax error: end if instead of end in best.html.erb MEDIUM ✓ Found FriendlyReviewer: "ERB syntax error: <%- end if %> is invalid. Change to <% end %>." (best.html.erb:6)

Supplementary Findings (14 findings)

Finding File Severity Legitimate?
XSS risk: cook_method == :raw_html returns raw unsanitized HTML as cooked post.rb:131 HIGH ✓ Stored XSS through bypass of all sanitization
Missing require 'cgi' for CGI.unescapeHTML — relies on brittle Rails autoloading poll_feed.rb:37 HIGH ✓ Could fail in production depending on load order
force: true added to existing migration will drop and recreate top_topics table top_topics.rb:3 HIGH ✓ Destroys existing data on migration
Mock for TopicView incomplete — will crash during view rendering (lacks stubs for .posts, .topic, .url) embed_controller_spec.rb:49 HIGH ✓ Test would fail at runtime
EditRateLimiter.performed! (Redis) called inside DB transaction — no rollback if transaction fails post_revisor.rb:49 MEDIUM ✓ Rate limit irreversibly consumed on failure
Unchecked return value of @post.save — validation failure continues execution silently post_revisor.rb:83 MEDIUM ✓ Silent failure with side effects
No per-item error handling in poll loop — exception in one item aborts entire loop poll_feed.rb:31 MEDIUM ✓ Single malformed feed blocks all subsequent feeds
Missing foreign key constraints on topic_id and post_id topic_embeds.rb:4 MEDIUM ✓ Orphaned records possible
Infinite retry loop in loading template with no max limit loading.html.erb:7 MEDIUM ✓ Browser resource exhaustion
Assertions in after block are fragile — move into each it block embed_controller_spec.rb:35 MEDIUM ✓ Test reliability concern
if (parent) is always truthy — should check window !== window.parent embed.html.erb:7 LOW ✓ Unnecessary postMessage when not embedded
Orphaned translation key embed.permalink defined but never used server.en.yml:37 LOW ◈ Acceptable noise
Grammatical error: "a RSS/ATOM feed" should be "an RSS/ATOM feed" server.en.yml:769 LOW ◈ Acceptable noise
Missing as: name on new embed route — consider as: :embed_best routes.rb:244 LOW ◈ Acceptable noise

Honest Analysis

FriendlyReviewer detected 5 of the 6 golden comments in this PR, with the remaining golden comment partially identified — a strong result. The security coverage is particularly impressive: SSRF via Kernel#open, origin validation bypass via substring matching, postMessage targetOrigin leaking the full referer URL, and non-standard X-Frame-Options: ALLOWALL were all correctly diagnosed with precise remediation suggestions. These are four distinct security issues across two languages (Ruby and JavaScript), and FriendlyReviewer identified every one.

Golden comment #5 (TopicEmbed.import vulnerabilities) was only partially found. FriendlyReviewer correctly identified the string mutation via << (L9) and the TOCTOU race condition between where and create! (L12). However, it missed the NoMethodError that would occur when contents is nil (no guard before calling <<), and the stored XSS via unescaped URL interpolation in the generated HTML anchor tag. These two sub-bugs are more subtle — the nil case requires reasoning about the full call chain, and the XSS depends on understanding that #{url} is not HTML-escaped in the template context.

Beyond the golden set, FriendlyReviewer uncovered several serious bugs. The raw_html cook method in Post bypasses all sanitization and would allow stored XSS through any embedding flow — a critical finding that no golden comment covers. The force: true flag on an existing migration would silently drop the top_topics table and destroy all production data on deploy. The missing require 'cgi' is a brittle dependency on Rails autoloading that could fail in production depending on load order. The incomplete mock for TopicView would cause a runtime crash during test execution, not just a false positive — the tests as written cannot pass.

The medium-severity findings are also legitimate: EditRateLimiter.performed! inside a DB transaction creates an irreversible Redis side effect if the transaction rolls back; the unchecked @post.save return value silently continues execution past a validation failure; the missing foreign key constraints on topic_embeds allow orphaned records; and the infinite retry loop in loading.html.erb could exhaust browser resources.

A small amount of noise is present: the orphaned embed.permalink translation key, the grammatical "a RSS" → "an RSS" error, and the missing route as: name are all trivial. This level of noise is acceptable — it is the trade-off for a reviewer that casts a wide net and finds real bugs that no human benchmark would credit.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
11 high, 6 medium, 4 low

This merge request introduces an embed feature for Discourse, enabling external sites to display Discourse topics/comments in iframes, with RSS/Atom feed polling and automatic topic creation. The review identified several critical security, data integrity, and reliability issues that must be addressed before deployment.

Security: Multiple XSS vectors and origin validation weaknesses. The raw_html cook method bypasses all sanitization (stored XSS). postMessage origin validation uses substring matching, and targetOrigin passes full URL instead of origin. The X-Frame-Options header uses non-standard ALLOWALL. Kernel#open risks command injection.

Data Integrity: TopicEmbed.import mutates caller's string via <<. Idempotency race condition in embed creation. Missing FK constraints on topic_embeds and top_topics tables. Migration force: true on existing table will drop data.

Code Quality: ERB syntax error in best.html.erb. Missing require 'cgi' in poll_feed.rb. EditRateLimiter.performed! inside DB transaction without rollback. Unchecked save return value in PostRevisor. Missing trailing newlines.

Testing: Assertions in after block, incomplete mocks causing runtime failures, and significant coverage gaps (malformed referer, loading template, import_remote, idempotency).

Configuration: Orphaned translation key, grammatical error, and missing route name.

Recommendation: Address all high-criticality findings before merging. Many issues stem from insufficient input validation and error handling.

These might need a close look

  • 🔴 app/models/post.rb (L131)
    XSS risk: cook_method == :raw_html returns raw unsanitized HTML as cooked. Run through an HTML sanitizer (e.g., Loofah) to strip malicious tags and attributes.
  • 🔴 app/controllers/embed_controller.rb (L28)
    Non-standard X-Frame-Options: ALLOWALL not recognized by modern browsers. Use Content-Security-Policy: frame-ancestors <embeddable_host> instead.
  • 🔴 app/assets/javascripts/embed.js (L20)
    Origin validation uses substring match indexOf; attacker could craft origin that contains legitimate URL as substring. Use exact origin comparison via new URL(discourseUrl).origin.
  • 🔴 app/views/layouts/embed.html.erb (L10)
    postMessage targetOrigin receives full referer URL instead of just origin. Use URI(request.referer).origin to parse scheme+host+port.
  • 🔴 app/models/topic_embed.rb (L9)
    contents << "..." mutates caller's string. Use contents + "..." to avoid corrupting feed content data.
  • 🔴 app/models/topic_embed.rb (L12)
    TOCTOU race condition between where and create!. Wrap in rescue for ActiveRecord::RecordNotUnique or use find_or_create_by.
  • 🔴 app/models/topic_embed.rb (L40)
    Kernel#open risk: user-supplied URL may execute pipe commands. Use OpenURI.open_uri with explicit timeouts. Also missing error handling for network failures.
  • 🔴 app/jobs/scheduled/poll_feed.rb (L37)
    Missing require 'cgi' for CGI.unescapeHTML. Add require to avoid brittle dependency on Rails autoloading.
  • 🔴 db/migrate/20131223171005_create_top_topics.rb (L3)
    force: true added to existing migration will drop and recreate top_topics table, destroying existing data. Use new migration for schema changes.
  • 🔴 app/views/embed/best.html.erb (L6)
    ERB syntax error: <%- end if %> is invalid. Change to <% end %>.
  • 🔴 spec/controllers/embed_controller_spec.rb (L49)
    Mock for TopicView incomplete; will crash during view rendering as it lacks stubs for .posts, .topic, .url, etc. Use real object or full stub.

Worth checking

  • 🟡 lib/post_revisor.rb (L49)
    EditRateLimiter.performed! (Redis) called inside DB transaction. If transaction rolls back, rate limit is irreversibly consumed. Move outside transaction or add rollback.
  • 🟡 lib/post_revisor.rb (L83)
    Unchecked return value of @post.save. Validation failure continues execution, consuming rate limit and potentially bumping topic. Add guard to return false on failure.
  • 🟡 app/jobs/scheduled/poll_feed.rb (L31)
    No per-item error handling; exception in one item aborts entire loop. Wrap loop body in begin/rescue to isolate failures.
  • 🟡 db/migrate/20131217174004_create_topic_embeds.rb (L4)
    Missing foreign key constraints on topic_id and post_id. Add foreign_key: true to prevent orphaned records.
  • 🟡 app/views/embed/loading.html.erb (L7)
    Infinite retry loop with no max limit. Implement max retries using sessionStorage or query parameter.
  • 🟡 spec/controllers/embed_controller_spec.rb (L35)
    Assertions in after block are fragile; move into each it block.

Small things (take or leave)

  • 🔵 app/views/layouts/embed.html.erb (L7)
    if (parent) is always truthy; should check window !== window.parent to avoid sending postMessage when not embedded.
  • 🔵 config/locales/server.en.yml (L37)
    Orphaned translation key embed.permalink defined but never used. Remove or wire to view.
  • 🔵 config/locales/server.en.yml (L769)
    Grammatical error: "a RSS/ATOM feed" should be "an RSS/ATOM feed".
  • 🔵 config/routes.rb (L244)
    Missing as: name on new embed route; consider adding as: :embed_best for consistency.