Enhance embed URL handling and validation system

[everettbu]

Test 4

[github-actions]

This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days.

[mfeuerstein]

PR Review — approved

Reviewed 28 files. 0 high-severity issues found. Verdict: approved.

app/models/post.rb (low)

• Reviewed app/models/post.rb — looks good

app/jobs/regular/retrieve_topic.rb (low)

• Reviewed app/jobs/regular/retrieve_topic.rb — looks good

Gemfile_rails4.lock (low)

• Reviewed Gemfile_rails4.lock — looks good

app/assets/javascripts/embed.js (low)

• Reviewed app/assets/javascripts/embed.js — looks good

app/controllers/embed_controller.rb (low)

• Reviewed app/controllers/embed_controller.rb — looks good

app/jobs/scheduled/poll_feed.rb (low)

• Reviewed app/jobs/scheduled/poll_feed.rb — looks good

app/views/layouts/embed.html.erb (low)

• Reviewed app/views/layouts/embed.html.erb — looks good

app/views/embed/loading.html.erb (low)

• Reviewed app/views/embed/loading.html.erb — looks good

app/views/embed/best.html.erb (low)

• Reviewed app/views/embed/best.html.erb — looks good

app/models/topic_embed.rb (low)

• Reviewed app/models/topic_embed.rb — looks good

config/routes.rb (low)

• Reviewed config/routes.rb — looks good

Gemfile (low)

• Reviewed Gemfile — looks good

config/locales/client.en.yml (low)

• Reviewed config/locales/client.en.yml — looks good

app/assets/stylesheets/embed.css.scss (low)

• Reviewed app/assets/stylesheets/embed.css.scss — looks good

db/migrate/20131217174004_create_topic_embeds.rb (low)

• Reviewed db/migrate/20131217174004_create_topic_embeds.rb — looks good

config/locales/server.en.yml (low)

• Reviewed config/locales/server.en.yml — looks good

config/site_settings.yml (low)

• Reviewed config/site_settings.yml — looks good

lib/topic_retriever.rb (low)

• Reviewed lib/topic_retriever.rb — looks good

spec/components/topic_retriever_spec.rb (low)

• Reviewed spec/components/topic_retriever_spec.rb — looks good

lib/post_creator.rb (low)

• Reviewed lib/post_creator.rb — looks good

lib/tasks/disqus.thor (low)

• Reviewed lib/tasks/disqus.thor — looks good

db/migrate/20131210181901_migrate_word_counts.rb (low)

• Reviewed db/migrate/20131210181901_migrate_word_counts.rb — looks good

spec/jobs/poll_feed_spec.rb (low)

• Reviewed spec/jobs/poll_feed_spec.rb — looks good

db/migrate/20131219203905_add_cook_method_to_posts.rb (low)

• Reviewed db/migrate/20131219203905_add_cook_method_to_posts.rb — looks good

spec/controllers/embed_controller_spec.rb (low)

• Reviewed spec/controllers/embed_controller_spec.rb — looks good

db/migrate/20131223171005_create_top_topics.rb (low)

• Reviewed db/migrate/20131223171005_create_top_topics.rb — looks good

lib/post_revisor.rb (low)

• Reviewed lib/post_revisor.rb — looks good

spec/models/topic_embed_spec.rb (low)

• Reviewed spec/models/topic_embed_spec.rb — looks good

[ron-x5labs]

Code Review: FEATURE: Embeddable Discourse comments, now with simple-rss instead of feedzirra

Problem

Adds embeddable Discourse comments: a site owner includes embed.js, which iframes embed/best?embed_url=.... The controller either renders a TopicView (best 5 posts) or enqueues a RetrieveTopic job that fetches the source page (via ruby-readability) and stores it as a cook_method=raw_html post whose cooked == raw. A scheduled PollFeed job imports an RSS/Atom feed the same way.

Solution Reviewed

New EmbedController (referer-gated, X-Frame-Options: ALLOWALL), TopicEmbed model (import/create/revise + absolutize_urls), TopicRetriever (Redis-throttled), RetrieveTopic/PollFeed jobs, Post#cook_methods enum (raw_html bypasses the cook pipeline), migrations (topic_embeds, posts.cook_method), embed views/layout/JS/CSS, site settings, and locales. Disqus thor rewired through import_remote.

Summary

The feature is well-structured and the core wiring (enum default, PostCreator opt, PostRevisor skip_validations, routes, settings, locales) is consistent. However it ships with multiple stored-XSS sinks (imported HTML rendered raw, including on the main forum), command-injection via Kernel#open, a nil-crash that aborts the entire feed poll, a transaction-boundary violation that can orphan enqueued jobs, and an infinite reload loop in the loading view. Recommend changes before merge.

Files Reviewed

• app/controllers/embed_controller.rb — deeply reviewed
• app/models/topic_embed.rb — deeply reviewed
• app/models/post.rb — deeply reviewed
• app/jobs/scheduled/poll_feed.rb — deeply reviewed
• app/jobs/regular/retrieve_topic.rb — deeply reviewed
• lib/topic_retriever.rb — deeply reviewed
• lib/post_creator.rb / lib/post_revisor.rb — deeply reviewed (existing, touched)
• app/assets/javascripts/embed.js — deeply reviewed
• app/views/embed/best.html.erb / loading.html.erb / layouts/embed.html.erb — deeply reviewed
• db/migrate/* (3 files) — deeply reviewed
• lib/tasks/disqus.thor — deeply reviewed
• config/routes.rb, config/site_settings.yml, config/locales/*.en.yml — deeply reviewed
• Gemfile, Gemfile_rails4.lock — deeply reviewed (Gemfile.lock/Gemfile_rails_master.lock not updated — see notes)
• spec files (4) — lightly reviewed (coverage noted)
• app/assets/stylesheets/embed.css.scss — lightly reviewed (styles only)

Verification

• No Ruby/JS toolchain available in the review environment — typecheck/tests/build skipped. Findings are grounded in static analysis of the checked-out worktree source (post_creator.rb:56/85/288-294 confirm the enqueue-after-transaction contract; enum.rb confirms cook_methods[:raw_html]==2, migration default 1=regular).

Issues Found

See inline comments. Highlights:

🔴 Blocking

• best.html.erb:19 — <%= raw post.cooked %> renders imported RSS/Readability HTML unescaped; cook_method=raw_html makes cooked==raw (post.rb:133), bypassing sanitization entirely. This is stored XSS served not only in the embed iframe but on the main Discourse topic view (Ember renders the same cooked).
• topic_embed.rb:13 — url is interpolated unescaped into <a href='#{url}'>; the ^https?:// regex has no end anchor and no URI.parse, so '/</> in the URL break out of the single-quoted attribute into stored raw_html. Reaches the main forum via cooked.
• poll_feed.rb:35 — i.content.scrub raises NoMethodError when a feed item has no content element (common — many feeds only carry description). Aborts the whole rss.items.each loop; with retry: false the hourly poll is silently lost.
• poll_feed.rb:29 — open(feed_polling_url) + SimpleRSS.parse have no rescue and retry: false; a transient network/parse error silently drops the entire hourly poll with no logging.
• topic_embed.rb:48 — import_remote calls open(url).read before import's ^https?:// check, and uses Kernel#open (executes |-prefixed strings as shell commands, reads file://). The disqus thor path passes t[:link] from an XML export straight to open(url) — a malicious/malformed export yields command injection / local file read. No timeout, no scheme allowlist.
• topic_embed.rb:21 — TOCTOU + transaction-boundary violation: embed is read outside any transaction (line 16); concurrent retrieve/poll callers both enter the create branch and the loser's TopicEmbed.create! raises RecordNotUnique unhandled. Wrapping PostCreator.create (which enqueues :process_post/:feature_topic_users after its own transaction, post_creator.rb:288-294) in an outer Topic.transaction means those jobs fire before the outer commit; if TopicEmbed.create! fails they run against rolled-back ids → orphan jobs.
• topic_embed.rb:23 — creator.create returns the post even when the save rolled back (errors present, nil id); post.present? is still true, so TopicEmbed.create! runs with nil topic_id/post_id and hits the NOT NULL constraints.
• loading.html.erb:8 — unconditional 30s setTimeout(reload) with no error state or retry cap. If the enqueued retrieve_topic job failed or was throttled (60s retrieved_recently? guard), the reload re-enters embed#best, finds no topic_id, re-enqueues another job, and re-renders loading — an infinite loop that always shows "Loading Discussion...".
• create_top_topics.rb:3 — a previously-shipped migration is modified to add force: true; on any environment that already ran it (or a re-run) this drops and recreates top_topics, destroying data. Unrelated to the feature.
• disqus.thor:148 — breaking CLI change: the documented --category/-c option is removed and the PostCreator call (with created_at and [Permalink] raw) is replaced by TopicEmbed.import_remote; callers passing --category now fail and topic created_at is no longer honored.

🟡 Non-blocking

• embed.js:17 — discourseUrl.indexOf(e.origin) === -1 is a substring containment check, not exact origin equality; non-default-port/substring origins pass. Compare e.origin === new URL(discourseUrl).origin.
• topic_retriever.rb:27 — $redis.setnx + $redis.expire are two round-trips; a crash between them leaves the key with no TTL, permanently blocking that embed_url. Use atomic SET key 1 EX 60 NX.
• topic_retriever.rb:41 — Jobs::PollFeed.new.execute({}) runs the full feed poll synchronously inside the retrieve worker (blocking, and concurrent with the scheduled PollFeed — amplifies the TOCTOU race).
• topic_embed.rb:64 — absolutize_urls only rewrites start_with?('/') hrefs/srcs; protocol-relative (//cdn.host/x) is mangled to http://embedhost.com/cdn.host/x, and bare-relative links are left relative.
• create_topic_embeds.rb:3 — force: true on a new migration drops the table if it exists on re-run after partial state.
• embed layout:11 — postMessage targetOrigin is request.referer (attacker-influenced, empty when referer absent → resize never delivered); should be a fixed expected origin.
• embed_controller.rb:24-26 — authorization rests solely on the forgeable Referer header; denied/abusive embed attempts are not logged.
• Stale lockfiles: Gemfile.lock and Gemfile_rails_master.lock were not regenerated (only Gemfile_rails4.lock); CI (bundle install --deployment) will fail or silently regenerate on the default lockfile.
• feed_key (poll_feed.rb:20) is dead code — the intended modified-since throttle is never wired; the full feed is re-fetched/re-iterated hourly.
• embed.js:5 — no null guard on document.getElementById('discourse-comments'); a page missing the div throws before the iframe is created.
• Test coverage gaps: no tests for import_remote/Readability, the real poll_feed body, XSS/raw_html sanitization, absolutize_urls port/protocol-relative cases, or perform_retrieve branches.

💡 Suggestions

• best.html.erb:6 — <%- end if %> is a confusing leftover (Ruby parses it as a modifier if whose condition is the next emitted buffer-append, so it renders identically to plain end); simplify to <%- end %>.

Verdict

Recommend changes before merge — multiple stored-XSS sinks and a command-injection vector are security-blockers; the feed-poll nil-crash, transaction-boundary violation, and infinite loading loop are correctness-blockers.

[ron-x5labs]

🔴 Stored XSS. <%= raw post.cooked %> renders imported RSS/Readability HTML unescaped. cook_method=raw_html makes cooked == raw (post.rb:133), bypassing the cook/sanitization pipeline entirely. This is served not only in the embed iframe but also on the main Discourse topic view (Ember renders the same cooked), so imported <script>/event-handlers execute on the Discourse origin for every reader. Sanitize imported HTML (e.g. via Sanitize/CookedPostProcessor) before storing as raw_html, or cook through the normal pipeline with a permissive allowlist.

[ron-x5labs]

🔴 Stored XSS via attribute breakout. url is interpolated unescaped into <a href='#{url}'>#{url}</a>. The guard at line 11 (url =~ /^https?\:\/\//) only anchors the start — there is no URI.parse and no end anchor, so ', <, > (or spaces) in the URL break out of the single-quoted attribute into the stored raw_html payload. Since cooked==raw is rendered unescaped (best.html.erb:19) and on the main forum, this is exploitable from a crafted RSS/disqus link. Parse the URL with URI.parse/Addressable and CGI.escapeHTML/attribute-escape it, or build the anchor via Nokogiri rather than string interpolation.

[ron-x5labs]

🔴 Nil crash aborts the entire feed poll. i.content.scrub assumes every feed item has a content element. Many RSS/Atom feeds only carry description/summary, for which SimpleRSS returns nil → NoMethodError. Combined with no per-item rescue and retry: false (line 12), a single content-less item silently drops the whole hourly poll. Guard for nil (fall back to i.description/i.summary or '') and wrap each item in a rescue.

[ron-x5labs]

🔴 No error handling + retry: false. open(SiteSetting.feed_polling_url) and SimpleRSS.parse raise on transient network/DNS/SSL/HTTP errors and malformed feeds. With sidekiq_options retry: false and no rescue/logging, a single blip silently skips the entire hourly poll with no application-level signal. Wrap in begin/rescue, pass read_timeout: to open, log failures, and either enable retries or record a Sidekiq failure with feed context.

[ron-x5labs]

🔴 Command injection / local file read via Kernel#open. import_remote calls open(url).read before import's ^https?:// check (line 11). Kernel#open executes |-prefixed strings as shell commands and reads file:// URIs. The disqus thor path passes t[:link] from an XML export straight to open(url) (disqus.thor:148), so a malicious/malformed export yields command injection or local file disclosure. No timeout, no scheme allowlist, no size cap. Use URI.open (or Net::HTTP/Faraday) with an http(s)-only scheme check, a read timeout, and a redirect cap.

[ron-x5labs]

🔴 Breaking CLI change. The documented --category/-c option was removed and the PostCreator call (which honored created_at and used a [Permalink] raw) is replaced by TopicEmbed.import_remote. Existing callers passing --category now fail, the original created_at is no longer honored for the topic post, and the permalink semantics changed. If the rewire is intentional, keep --category (or document its removal) and preserve created_at through import_remote's opts.

[ron-x5labs]

🟡 Substring origin check. discourseUrl.indexOf(e.origin) === -1 is a containment test, not exact origin equality; non-default-port configs or any origin that serializes to a substring of discourseUrl pass. Compare exactly, e.g. e.origin === discourseUrl.replace(/\/$/, '') (or new URL(discourseUrl).origin).

[ron-x5labs]

🟡 Non-atomic Redis throttle. $redis.setnx(key, '1') + $redis.expire(key, 60) are two round-trips; a crash/kill between them leaves the key with no TTL, permanently blocking retrieval for that embed_url. Use the atomic SET key 1 EX 60 NX form.

[ron-x5labs]

🟡 Synchronous nested job. Jobs::PollFeed.new.execute({}) runs the entire feed poll inline inside the retrieve_topic worker — blocking the worker for the full network fetch+parse+imports and running concurrently with the scheduled hourly PollFeed (amplifying the TopicEmbed.import TOCTOU race). Enqueue :poll_feed or guard with a distributed lock instead of calling execute directly.

[ron-x5labs]

🟡 absolutize_urls mishandles protocol-relative URLs. Only start_with?('/') hrefs/srcs are rewritten, so //cdn.host/x (which also starts with /) becomes http://embedhost.com/cdn.host/x, breaking the link. Bare-relative links are left relative. Handle //-prefixed (protocol-relative) separately and consider resolving a[href]/img[src] via URI.join(prefix, href).

[ron-x5labs]

Code Review: FEATURE: Embeddable Discourse comments, now with simple-rss instead of feedzirra

Problem

Adds embeddable Discourse comments: a third-party site includes embed.js, which iframes embed/best?embed_url=.... The controller (referer-gated) renders a TopicView of the best 5 posts, or enqueues a RetrieveTopic job that fetches the source page via ruby-readability and stores it as a cook_method=raw_html post whose cooked == raw (bypasses the cook/sanitization pipeline). A scheduled PollFeed job imports an RSS/Atom feed the same way.

Solution Reviewed

New EmbedController (referer-gated, X-Frame-Options: ALLOWALL), TopicEmbed model (import/create/revise + absolutize_urls), TopicRetriever (Redis-throttled), RetrieveTopic/PollFeed jobs, Post#cook_methods enum (raw_html short-circuits cook), migrations (topic_embeds, posts.cook_method), embed views/layout/JS/CSS, site settings, locales. Disqus thor rewired through import_remote.

Summary

The feature is well-structured and the core wiring (enum default=1=regular, PostCreator opt, PostRevisor skip_validations, routes, settings, locales) is internally consistent. However it ships with two stored-XSS chains that reach the main forum (not just the embed iframe), a command-injection/local-file-read vector via Kernel#open, a nil-crash that breaks feed import for common feeds, a transaction-boundary violation that can orphan enqueued jobs, an infinite reload loop, and a data-loss migration edit. Recommend changes before merge.

Files Reviewed

• app/models/post.rb — deeply reviewed (cook override)
• app/models/topic_embed.rb — deeply reviewed (import/import_remote/absolutize_urls)
• app/controllers/embed_controller.rb — deeply reviewed
• app/jobs/scheduled/poll_feed.rb — deeply reviewed
• app/jobs/regular/retrieve_topic.rb — deeply reviewed
• lib/topic_retriever.rb — deeply reviewed
• lib/post_creator.rb / lib/post_revisor.rb — deeply reviewed (touched)
• lib/tasks/disqus.thor — deeply reviewed
• app/assets/javascripts/embed.js — deeply reviewed
• app/views/embed/best.html.erb / loading.html.erb / layouts/embed.html.erb — deeply reviewed
• db/migrate/* (4 files) — deeply reviewed
• config/routes.rb, config/site_settings.yml, config/locales/*.en.yml — deeply reviewed
• Gemfile, Gemfile_rails4.lock — deeply reviewed (note: Gemfile.lock/Gemfile_rails_master.lock not updated)
• spec files (4) — lightly reviewed (coverage gaps noted)
• app/assets/stylesheets/embed.css.scss — lightly reviewed (styles only)

Verification

• No Ruby/JS toolchain available in the review environment — typecheck/lint/tests/build skipped. Findings grounded in static analysis of the checked-out PR source (confirmed: Enum.new(:regular, :raw_html) => regular=1, raw_html=2, migration default=1; post.js.handlebars:70 renders {{{cooked}}} raw on the main forum; topic_view.rb:300 raises Discourse::NotFound for missing topic; post_creator.rb:288-294 enqueues jobs after its inner transaction; anonymous_cache.rb:14 cache key excludes Referer).

Issues Found

See inline comments. Highlights:

Blocking

• post.rb:133 — cook_method=raw_html returns raw unsanitized; cooked is rendered raw in the embed (best.html.erb:19) AND on the main forum (post.js.handlebars:70 {{{cooked}}}). Imported RSS/Readability HTML bypasses PrettyText sanitization entirely → stored XSS.
• topic_embed.rb:13 — url interpolated unescaped into <a href='#{url}'>; the ^https?:// regex has no end anchor / no URI.parse, so '/</> in the URL break out of the single-quoted attribute → stored XSS via the raw_html cooked column.
• topic_embed.rb:48 — import_remote calls open(url).read before import's scheme guard; Kernel#open executes |-prefixed strings as shell commands and reads file://. The disqus thor path passes t[:link] from an XML export straight to open(url) → command injection / local file read.
• poll_feed.rb:35 — i.content.scrub raises NoMethodError for feed items lacking a content element (common — many feeds only carry description); aborts the whole rss.items.each loop. With retry: false the hourly poll is silently lost.
• loading.html.erb:9 — unconditional 30s setTimeout(reload) with no error/retry cap; if the retrieve job failed or was throttled (60s guard), reload re-enters embed#best, finds no topic_id, re-enqueues, re-renders loading → infinite loop.
• create_top_topics.rb:3 — a previously-shipped migration is edited to add force: true; on re-run (db:migrate:redo or a flaky deploy) this drops and recreates top_topics, destroying data. Unrelated to this feature.
• topic_embed.rb:21 — embed read outside the transaction (line 15); concurrent retrieve/poll callers both enter the create branch and the loser's TopicEmbed.create! raises unhandled RecordNotUnique. Wrapping PostCreator.create (which enqueues :process_post/:feature_topic_users after its own transaction, post_creator.rb:288-294) in an outer Topic.transaction means those jobs fire before the outer commit; if TopicEmbed.create! fails they run against rolled-back ids → orphan jobs.
• disqus.thor:148 — breaking CLI change: the documented --category/-c option is removed, and the thread's original created_at is no longer honored for the imported topic's first post (replies keep theirs), producing topics dated after their own replies.

Non-blocking

• topic_retriever.rb:27 — $redis.setnx + $redis.expire are two round-trips; a crash between them leaves the key with no TTL, permanently blocking that embed_url. Use atomic SET key 1 EX 60 NX.
• topic_retriever.rb:41 — Jobs::PollFeed.new.execute({}) runs the full feed poll synchronously inside the retrieve worker (blocking, duplicates the scheduled PollFeed, amplifies the TOCTOU race).
• embed.js:17 — discourseUrl.indexOf(e.origin) === -1 is substring containment, not exact origin equality; compare e.origin === new URL(discourseUrl).origin.
• layouts/embed.html.erb:11 — postMessage targetOrigin is request.referer (attacker-influenced, and a full URL with path ≠ origin, so the resize message is silently dropped in the common case). Should be a fixed expected origin.
• embed_controller.rb:26 — authorization rests solely on the forgeable Referer header; embed_url itself is never host-checked in the controller, and denied/abusive embed attempts are not logged.
• topic_embed.rb:59 — uri.port != 80 && uri.port != 443 is scheme-blind; https://host:80 and http://host:443 lose their explicit port and resolve to the wrong endpoint. Use uri.port == uri.default_port.
• topic_embed.rb:64 — absolutize_urls only rewrites start_with?('/') hrefs/srcs; protocol-relative //host/x is mangled to an embed-host path, and bare-relative links are left relative (resolve against Discourse, not source). Use URI.join(url, href).
• topic_embed.rb:24 — post.present? is true even when PostCreator.create rolled back (errors set, nil id/topic_id); TopicEmbed.create! then runs with nil ids and violates NOT NULL.
• poll_feed.rb:29 — open(SiteSetting.feed_polling_url) + SimpleRSS.parse have no rescue and retry: false; a transient network/parse error silently drops the entire hourly poll with no logging.
• embed_controller.rb:13 — TopicView.new(topic_id, ...) raises Discourse::NotFound when the TopicEmbed row is stale (topic deleted/inaccessible); the embed endpoint errors out instead of falling back to loading/retrieve.
• Stale lockfiles: Gemfile.lock and Gemfile_rails_master.lock were not regenerated (only Gemfile_rails4.lock); bundle install --deployment will fail or silently regenerate on the default lockfile.

Suggestions

• poll_feed.rb:20 — feed_key is dead code (never referenced); the intended modified-since throttle is never wired, so the full feed is re-fetched/re-iterated hourly.
• Gemfile:210 — ruby-readability and simple-rss are added without version constraints and both are long-unmaintained; the repo pins most other gems, so these break convention. Pin with ~> 0.5.7 / ~> 1.3.1.
• Test coverage gaps: no tests for import_remote/Readability, the real poll_feed body, XSS/raw_html sanitization, absolutize_urls port/protocol-relative cases, or perform_retrieve/retrieved_recently? bodies (specs stub them out).

Verdict

Recommend changes before merge — the stored-XSS chains and command-injection vector are security-blockers; the feed-poll nil-crash, transaction-boundary violation, infinite loading loop, and data-loss migration edit are correctness-blockers.

[ron-x5labs]

Stored XSS via raw_html cook bypass. cook_method == raw_html returns raw unchanged, skipping the PrettyText.cook sanitization/whitelist pipeline that normal posts pass through. The resulting cooked column is rendered raw in the embed iframe (best.html.erb:19 <%= raw post.cooked %>) and on the main Discourse forum (post.js.handlebars:70 {{{cooked}}}). Imported RSS/Readability HTML flows straight into this column with skip_validations: true → stored XSS that reaches the main forum, not just the embed. Since raw_html is meant to be an internal-only fast path, the imported content must still be sanitized before persistence (e.g. run it through PrettyText/Sanitize or a whitelist).

def cook(*args)
  if cook_method == Post.cook_methods[:raw_html]
    # still sanitize imported HTML
    return Sanitize.clean(raw, Sanitize::Config::RELAXED)
  end
  Plugin::Filter.apply(:after_post_cook, self, post_analyzer.cook(*args))
end

[ron-x5labs]

Stored XSS via attribute breakout. url is interpolated unescaped into <a href='#{url}'>#{url}</a>. The /^https?:/// guard on line 11 has no end anchor and no URI.parse, so a valid-URI payload like http://evil.com/'onclick='alert(1) passes the regex ('/(/) are RFC3986 sub-delims) and breaks out of the single-quoted href attribute. The built string is then stored as raw_html cooked content (skip_validations: true) and rendered raw. Escape the URL and validate it fully:

return unless url =~ /^https?:///\z/
uri = URI.parse(url)
raise Discourse::InvalidParameters if uri.host.blank?
safe_url = ERB::Util.html_escape(url)
contents << "\n<hr>\n<small>#{I18n.t('embed.imported_from', link: \"<a href='#{safe_url}'>#{safe_url}</a>\")}</small>\n"

[ron-x5labs]

Command injection / local file read via Kernel#open. import_remote calls open(url).read before import's ^https?:// scheme guard runs, and TopicRetriever#invalid_host? only compares URI(url).host (no scheme check). Kernel#open executes strings beginning with | as shell commands and honors file:// URIs. The disqus thor task passes t[:link] from an XML export straight into open(url), so a malicious/malformed export yields RCE / local file read. open-uri also follows redirects with no limit and no timeout (SSRF to internal hosts). Validate scheme+host before fetching and use a bounded fetch:

def self.import_remote(user, url, opts=nil)
  raise Discourse::InvalidParameters if url !~ /^https?:///\z/
  uri = URI.parse(url)
  raise Discourse::InvalidParameters if uri.host != SiteSetting.embeddable_host
  require 'ruby-readability'
  doc = Readability::Document.new(open(url, read_timeout: 10, open_timeout: 10, redirect: false).read,
    tags: %w[div p code pre h1 h2 h3 b em i strong a img], attributes: %w[href src])
  TopicEmbed.import(user, url, opts[:title] || doc.title, doc.content)
rescue OpenURI::HTTPError, SocketError, Timeout::Error => e
  Rails.logger.warn("TopicEmbed#import_remote failed for #{url}: #{e.message}")
  nil
end

[ron-x5labs]

i.content.scrub crashes for items lacking a content element. SimpleRSS returns nil/raises for tags absent from a feed item; many RSS 2.0 feeds only carry <description>. nil.scrub raises NoMethodError, and because the each loop has no per-item rescue, one such item aborts the whole batch. With sidekiq_options retry: false that hour's feed import is silently lost. Guard nil and isolate each item:

rss.items.each do |i|
  begin
    url = i.link
    url = i.id if url.blank? || url !~ /^https?:///\z/
    content = (i.content || i.description || '').to_s.scrub
    TopicEmbed.import(user, url, i.title, CGI.unescapeHTML(content))
  rescue => e
    Rails.logger.warn("PollFeed: skipping item #{i.try(:link)}: #{e.message}")
  end
end

[ron-x5labs]

Infinite reload loop. The unconditional 30s setTimeout(reload) has no error state or retry cap. If the enqueued RetrieveTopic job failed, was throttled (60s retrieved_recently? guard), or the user is blank, the reload re-enters embed#best, finds no topic_id, re-enqueues another job, and re-renders loading — an infinite loop that always shows "Loading Discussion..." and hammers the server. Track attempt count and show a terminal failure state after N tries:

(function() {
  var attempts = +(sessionStorage.getItem('discourse-embed-retries') || 0);
  if (attempts >= 5) { return; } // stop, leave the loading message
  sessionStorage.setItem('discourse-embed-retries', attempts + 1);
  setTimeout(function() { document.location.reload(); }, 30000);
})();

[ron-x5labs]

post.present? is true after a rolled-back create. PostCreator.create returns the in-memory @post even when the save rolled back (errors set, id/topic_id nil) — e.g. on rollback_if_host_spam_detected. post.present? is still true, so TopicEmbed.create! runs with nil topic_id/post_id and hits the NOT NULL constraints. Check persistence instead:

if post&.id.present? && post.errors.empty?
  TopicEmbed.create!(topic_id: post.topic_id, embed_url: url, content_sha1: content_sha1, post_id: post.id)
end

[ron-x5labs]

No error handling + retry: false silently drops the hourly poll. open(feed_polling_url) and SimpleRSS.parse have no rescue; a transient network/DNS/parse error aborts the job, and with retry: false it never self-heals — no application-level log, no alert. Wrap the fetch/parse in a rescue that logs with the feed URL:

begin
  require 'simple-rss'
  rss = SimpleRSS.parse(open(SiteSetting.feed_polling_url, read_timeout: 10, open_timeout: 10))
rescue => e
  Rails.logger.warn("PollFeed failed for #{SiteSetting.feed_polling_url}: #{e.class} #{e.message}")
  raise
end

[ron-x5labs]

TopicView.new raises NotFound for a stale embed. When topic_id_for_embed returns an id whose topic was deleted or is inaccessible, TopicView#check_and_raise_exceptions raises Discourse::NotFound (topic_view.rb:300), so the embed iframe shows an error page instead of falling back to the loading/retrieve path. Wrap and fall back:

begin
  @topic_view = TopicView.new(topic_id, current_user, {best: 5})
rescue Discourse::NotFound
  @topic_view = nil
end
if @topic_view.nil?
  Jobs.enqueue(:retrieve_topic, user_id: current_user.try(:id), embed_url: embed_url)
  render 'loading'
end

[ron-x5labs]

feed_key is dead code. It is defined but never referenced anywhere in the codebase, implying an intended modified-since dedup that was never wired. As a result the full feed is re-fetched and re-iterated every hour. Either wire it (e.g. cache the last-modified/etag and skip unchanged feeds) or remove it to avoid confusion.

[ron-x5labs]

Gems added without version constraints and unmaintained. ruby-readability and simple-rss have no version pin (only Gemfile_rails4.lock pins 0.5.7/1.3.1); the repo pins most other gems (sidekiq, '2.15.1', pg, '0.15.1'), so these break convention. Both gems are long-unmaintained (simple-rss ~2009), widening the unvetted dependency surface (ruby-readability also pulls in guess_html_encoding). Pin them: gem 'ruby-readability', '~> 0.5.7' / gem 'simple-rss', '~> 1.3.1'.

[ron-x5labs]

Code Review: FEATURE: Embeddable Discourse comments, now with simple-rss instead of feedzirra

Problem

Adds embeddable Discourse comments: a third-party site includes embed.js, which iframes embed/best?embed_url=.... The controller (referer-gated) renders a TopicView of the best 5 posts, or enqueues a RetrieveTopic job that fetches the source page via ruby-readability and stores it as a cook_method=raw_html post whose cooked == raw. A scheduled PollFeed job imports an RSS/Atom feed the same way.

Solution Reviewed

New EmbedController (referer-gated, X-Frame-Options: ALLOWALL), TopicEmbed model (import/create/revise + absolutize_urls), TopicRetriever (Redis-throttled), RetrieveTopic/PollFeed jobs, Post#cook_methods enum + Post#cook short-circuit, embed views/layout/JS/CSS, site settings, locales, route, two migrations, and a Disqus-thor refactor.

Summary

The feature's skeleton is coherent, but the import path is unsanitized end-to-end and the feed poller is broken on the documented runtime. There are several blocking security (stored XSS, command-injection/SSRF via Kernel#open) and correctness (TOCTOU race, post.present? after rollback, force: true on a shipped migration, broken loading→best flow, String#scrub on Ruby 2.0.0) issues that should be fixed before merge.

Files Reviewed

• app/views/embed/best.html.erb — deep (XSS sink)
• app/models/topic_embed.rb — deep (import/absolutize_urls — XSS, TOCTOU, open, rollback guard)
• app/models/post.rb — deep (cook bypass)
• app/jobs/scheduled/poll_feed.rb — deep (scrub crash, retry:false, dead feed_key)
• lib/topic_retriever.rb — deep (throttle, nested job)
• app/controllers/embed_controller.rb — deep (stale topic, enqueue)
• app/views/embed/loading.html.erb — deep (reload loop/gate)
• db/migrate/20131223171005_create_top_topics.rb — deep (force:true)
• db/migrate/20131217174004_create_topic_embeds.rb — light (force:true on new table)
• db/migrate/20131219203905_add_cook_method_to_posts.rb — light (default 1 == :regular, sound)
• lib/tasks/disqus.thor — deep (breaking CLI / created_at)
• lib/post_creator.rb, lib/post_revisor.rb — light (opts wiring, verified grounding)
• Gemfile / Gemfile_rails4.lock — deep (unpinned + lockfile sync)
• config/site_settings.yml, config/routes.rb, locales — light
• app/assets/javascripts/embed.js, embed.css.scss, layouts/embed.html.erb — light
• specs — light (coverage gaps noted inline where relevant)
• db/migrate/20131210181901_migrate_word_counts.rb — skipped (whitespace-only change)

Verification

• Static grounding (run): confirmed PostCreator#create returns @post after the transaction block (rollback leaves a non-nil object) — grounds the post.present? finding.
• Confirmed Post#cook returns raw for raw_html and post.cooked ||= post.cook(...) assigns it — grounds the XSS bypass.
• Confirmed .travis.yml pins Ruby 2.0.0 and Gemfile.lock/Gemfile_rails_master.lock lack the new gems — grounds the scrub + lockfile findings.
• Full rspec suite: skipped — requires DB/Redis provisioning and is out of scope for review; the new specs themselves stub away the parsing/throttle/referer paths (noted inline).

Issues Found

8 blocking + 7 non-blocking + 2 suggestions, posted as inline comments below. Headline blockers: stored XSS via the raw_html cook bypass (best.html.erb:19/post.rb:133), unescaped URL interpolation (topic_embed.rb:13), Kernel#open command/SSRF (topic_embed.rb:48), TOCTOU race (topic_embed.rb:15), String#scrub crash on Ruby 2.0.0 (poll_feed.rb:35), broken loading→best reload (loading.html.erb:9), force: true on a shipped migration (create_top_topics.rb:3), and post.present? after rollback (topic_embed.rb:24).

Verdict

Recommend changes before merge — the unsanitized import path is an account-takeover-class XSS and the feed poller is non-functional on the CI runtime; the remaining blockers are data-loss/race/correctness issues.

[ron-x5labs]

🔴 Stored XSS. <%= raw post.cooked %> renders imported HTML unescaped. cook_method=raw_html makes Post#cook return raw unchanged (post.rb:133), bypassing the PrettyText.cook sanitize pipeline, so cooked is the raw RSS/scraped HTML — including any <script>/onerror= from a malicious or compromised feed. The iframe is served from the Discourse origin, so this is same-origin script execution against the viewer's session (and cooked is also rendered raw in the normal topic/email views). Sanitize imported content before storing (route it through PrettyText.cook/Sanitize.clean, or a strict Nokogiri allowlist) and don't short-circuit Post#cook to raw for raw_html posts.

[ron-x5labs]

🔴 Stored XSS via attribute breakout. url is interpolated unescaped into <a href='#{url}'>#{url}</a>. The guard at line 11 (url =~ /^https?://) only anchors the start, so a URL like http://x' onclick='alert(1)// closes the single-quoted attribute and injects a handler; since the post is raw_html the markup is stored verbatim and rendered via raw. url is attacker-influenced (RSS i.link/i.id, or params[:embed_url]). Validate with URI.parse + scheme check and escape on output:

return unless (u = URI.parse(url) rescue nil) && %w[http https].include?(u.scheme)
# in the appended markup:
esc = ERB::Util.method(:html_escape)
contents << "\n<hr>\n<small>#{I18n.t('embed.imported_from', link: "<a href='#{esc.call(url)}'>#{esc.call(url)}</a>")}</small>\n"

[ron-x5labs]

🔴 Command injection / local-file read via Kernel#open. open(url).read runs before import's ^https?:// scheme check (that check is inside import, line 11, not here). Kernel#open treats |cmd as a shell command and file: as local-file access; import_remote is reached from disqus.thor with t[:link] that is not host-checked, and open-uri follows HTTP redirects without re-validating the host (SSRF to internal services such as cloud metadata). Validate the scheme first and use URI#read:

u = URI.parse(url)
raise Discourse::InvalidAccess unless %w[http https].include?(u.scheme)
doc = Readability::Document.new(u.read, tags: ..., attributes: ...)

Apply the same to poll_feed.rb:29 (open(SiteSetting.feed_polling_url)).

[ron-x5labs]

🔴 TOCTOU race. embed is read outside the transaction (line 15); TopicEmbed.create! happens inside it (line 25) against the unique index on embed_url. Concurrent callers — EmbedController#best enqueues RetrieveTopic while the hourly PollFeed iterates the same URLs on separate workers — both observe embed.blank?, both create, and the loser raises RecordNotUnique, which propagates unhandled and aborts the rest of the poll_feed item loop. Move the lookup inside the transaction, or rescue ActiveRecord::RecordNotUnique and re-read the embed to take the update branch.

[ron-x5labs]

🔴 PollFeed is broken on the documented runtime and on common feeds. i.content.scrub raises NoMethodError two ways: String#scrub was added in Ruby 2.1.0 but .travis.yml pins Ruby 2.0.0 (no string-scrub backport in any lockfile), and i.content is nil for any RSS item lacking <content:encoded> (most RSS 2.0 feeds expose only <description>). With sidekiq_options retry: false (line 12) the entire hourly import silently fails. Nil-guard and fall back to description:

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

(and require/drop scrub to support Ruby 2.0.0).

[ron-x5labs]

🟡 Stale/orphaned embed → permanent 404. When an imported topic is deleted, topic_id_for_embed still returns its id (no dependent: :destroy on the Topic side), so TopicView.new raises Discourse::NotFound/InvalidAccess, rendered as a raw error page inside the iframe; the else re-import branch is only taken when topic_id is falsy. Rescue/nil-check the TopicView and fall through to retrieval, or add dependent: :destroy so deleting the topic removes the topic_embeds row.

[ron-x5labs]

🟡 absolutize_urls mangles protocol-relative URLs. //cdn.example.com/x satisfies start_with?('/') and sub(/^\/+/, '') strips both leading slashes, yielding http://<embedhost>/cdn.example.com/x (wrong host). Exclude protocol-relative URLs (and note :uri.port != 80 && != 443 at line 59 ignores scheme — http://host:443/https://host:80 lose their explicit port; prefer uri.port != uri.default_port):

if href.present? && href.start_with?('/') && !href.start_with?('//')

[ron-x5labs]

🟡 Unpinned gems + only one lockfile regenerated. ruby-readability/simple-rss have no version constraint (the repo pins most gems), so a future resolve can pull an unmaintained/typosquat release. And the diff only updates Gemfile_rails4.lock — Gemfile.lock and Gemfile_rails_master.lock are missing these gems (and transitive guess_html_encoding), so rails3/master bundle check is out of sync. Pin versions (gem 'ruby-readability', '0.5.7') and regenerate all three lockfiles.

[ron-x5labs]

💡 feed_key is dead code. Defined but never referenced anywhere — it looks like an intended Last-Modified/ETag conditional-GET throttle that was never wired up, so every hourly tick re-fetches and re-imports the whole feed with no idempotency guard. Either wire it up or remove it.

[ron-x5labs]

💡 embed.permalink is defined but no view/controller/model references it. Remove it or add the consuming UI.