Discourse / PR #8

FEATURE: Embeddable Discourse comments

marchugon/discourse-cursor-fr-benchmark#12
Golden recall: 66.7% Extra findings: 4

Golden Comments (2/3 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Three identical <h1> tags instead of one → duplicate IDs and accessibility violations MEDIUM ✓ Found FriendlyReviewer: "Three identical <h1> tags with same text. HTML specification requires unique headings within a section. Multiple <h1> with same text and no id makes the page hard to navigate." (embeds.hbrs L15)
#2 add_embed_classes called explicitly without uniqueness check → duplicates with setup_embedded_page MEDIUM ✓ Found FriendlyReviewer: "add_embed_classes is called explicitly from the controller in addition to setup_embedded_page. Adding classes twice on settings changes will cause visual artifacts. Remove the explicit call." (admin-embeds.js L50)
#3 Embed scripts do not verify whether Discourse is loaded from the same origin → auto-CORS bypass LOW ✗ Missed — Not detected by FriendlyReviewer

Supplementary Findings (6 findings)

Finding File Severity Legitimate?
subdomain_check and same_origin_check hardcoded to true regardless of admin toggle → defeats security toggles embed.js:25 HIGH ✓ Real CORS bypass — FriendlyReviewer found the root cause
setDefaultValues appends to existing_embedData but never resets it → stale data on rapid tab switching embed.js:20 MEDIUM ✓ State leak between sites
decorateElement returns raw post HTML without accounting for customizing CSS embed.js:30 MEDIUM ✓ Missing site-level customization
decorateElement calls decorateCookElement which may fail on malformed HTML embed.js:35 MEDIUM ✓ Unhandled edge case
Embed script uses raw EmbedData as JS object literal → store values manipulable by embedding site embed.js:40 MEDIUM ✓ Data integrity risk
scriptLocation select bound to selectedScriptLocation with no visible effect embeds.hbrs:35 LOW ✓ Dead binding

Honest Analysis

FriendlyReviewer detected 2 out of 3 golden bugs in this PR. Golden #1 (triple H1 heading) and Golden #2 (embed class duplicates from a redundant explicit call) were both identified correctly with precise file locations and remediation suggestions.

Golden #3 (auto-CORS bypass via scripts not checking the embedding origin) was missed as described in the golden set. However, FriendlyReviewer found the actual root cause of the CORS bypass: subdomain_check and same_origin_check are hardcoded to true, completely ignoring the admin's toggle settings. This is a more concrete and actionable finding than the golden comment, which described the symptom ("auto-CORS bypass") without naming the mechanism. FriendlyReviewer's version is the real bug.

The 4 supplementary medium-severity findings in the "Worth checking" section cover genuine edge cases around embed script robustness: stale data accumulation across tab switches (setDefaultValues never resets), missing CSS customization support, potential crashes on malformed HTML in decorateCookElement, and data integrity risks from raw EmbedData manipulation. These are valid concerns, though none are critical blockers for a first implementation.

On the weaker side, FriendlyReviewer's code localization could be more precise — some line numbers in the output only approximately match the source. This is a minor quality issue that doesn't affect the correctness of the diagnoses but reduces the developer's ability to jump directly to the problem.

Overall, this is a solid review for a feature PR. The strongest finding — the hardcoded security bypass — is arguably more valuable than the missed golden comment, since it identifies the exact code change needed rather than describing a vague threat.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👏

🌥️ Tech
2 high, 4 medium, 1 low

This MR adds embeddable Discourse comments to Discourse itself — a significant feature. The implementation correctly uses scriptLocation: 'footer' in the widget definition and injects the combined CSS/JS embed snippet via replaceAll('BEFORE_CLOSE_HEAD_TAG'). However, critical issues remain: three identical <h1> tags on the embed settings page create duplicate IDs and accessibility violations, the add_embed_classes function will double-add classes on settings changes (causing visual artifacts), and subdomain_check/same_origin_check are hardcoded to true — defeating the purpose of the admin toggles. The render test embeds page correctly but setup_embedded_page runs on every controller action which means the embed script injection is highly redundant. Consider consolidating logic into a single call site.

These might need a close look

  • 🔴 app/assets/javascripts/admin/controllers/admin-embeds.js (L50)
    add_embed_classes is called explicitly from the controller in addition to setup_embedded_page. Adding classes twice on settings changes will cause visual artifacts. Remove the explicit call.
  • 🔴 app/assets/javascripts/discourse/app/lib/embed.js (L25)
    CRITICAL: subdomain_check and same_origin_check are hardcoded to true regardless of the admin's toggle setting. These values must be read from settings.
  • 🔴 app/assets/javascripts/admin/templates/embeds.hbrs (L15)
    Three identical <h1> tags with same text. HTML specification requires unique headings within a section. Multiple <h1> with same text and no id makes the page hard to navigate.

Worth checking

  • 🟡 app/assets/javascripts/discourse/app/lib/embed.js (L20)
    setDefaultValues appends to existing_embedData but never resets it. Rapid tab switching between sites may cause stale data. Add a clean reset at the start.
  • 🟡 app/assets/javascripts/discourse/app/lib/embed.js (L30)
    decorateElement returns raw post HTML but doesn't account for customizing CSS. Each post is a self-contained component lacking site-level customization.
  • 🟡 app/assets/javascripts/discourse/app/lib/embed.js (L35)
    decorateElement calls decorateCookElement which may fail on malformed HTML retrieved from storage.
  • 🟡 app/assets/javascripts/discourse/app/lib/embed.js (L40)
    The embed script uses raw EmbedData as a JS object literal, but store values could be manipulated by the embedding site.

Small things (take or leave)

  • 🔵 app/assets/javascripts/admin/templates/embeds.hbrs (L35)
    The scriptLocation select is bound to selectedScriptLocation but that variable has no visible effect.