Enhance embed URL handling and validation system - #4
Conversation
| def self.import(user, url, title, contents) | ||
| return unless url =~ /^https?\:\/\// | ||
|
|
||
| contents << "\n<hr>\n<small>#{I18n.t('embed.imported_from', link: "<a href='#{url}'>#{url}</a>")}</small>\n" |
There was a problem hiding this comment.
Stored XSS via unsanitized URL interpolation in imported_from_html
In TopicEmbed.import, the url string is interpolated directly into an HTML anchor tag: "<a href='#{url}'>#{url}</a>". If a URL contains a single-quote (valid in URLs, e.g. http://evil.com/x'onmouseover='alert(1)) it breaks out of the href attribute. The resulting HTML is stored as raw post content (cook_method = raw_html) and rendered without further sanitization to all viewers of that topic and to users of the embed iframe.
Eliminates a stored XSS vector affecting every reader of an embedded topic, including admins.
In app/models/topic_embed.rb line 13, use ERB::Util.html_escape(url) (or CGI.escapeHTML) for both the href attribute value and the link text: "<a href='#{ERB::Util.html_escape(url)}'>#{ERB::Util.html_escape(url)}</a>". Verify with a test that includes a URL with ', ", <, and > characters.
| require 'ruby-readability' | ||
|
|
||
| opts = opts || {} | ||
| doc = Readability::Document.new(open(url).read, |
There was a problem hiding this comment.
Server-Side Request Forgery (SSRF) via open() on user-controlled URLs
Both TopicEmbed.import_remote and Jobs::PollFeed#poll_feed call open(url) (Kernel#open / open-uri) on URLs ultimately derived from user input (embed_url param, RSS feed links, or feed_polling_url site setting). open-uri's open will follow redirects and can hit internal network addresses (e.g. http://169.254.169.254/... for cloud metadata, or file:// if a pipe character is used in older Rubies). There is no allowlist of schemes, no resolution check against private IP ranges, and no timeout.
Prevents an attacker (or a malicious RSS feed) from reading cloud instance metadata, probing internal services, or causing long-running connections that exhaust workers.
Replace bare open(url).read in app/models/topic_embed.rb (import_remote) and app/jobs/scheduled/poll_feed.rb (poll_feed) with a dedicated HTTP client (e.g. Net::HTTP or Excon) configured with: (1) explicit https/http scheme check, (2) DNS resolution guard rejecting RFC-1918 and link-local IPs, (3) a connect/read timeout (e.g. 10s), (4) redirect limit. Wrap in a helper like FinalDestination (already present in Discourse later) that the two call sites share.
| window.onload = function() { | ||
| if (parent) { | ||
| // Send a post message with our loaded height | ||
| parent.postMessage({type: 'discourse-resize', height: document['body'].offsetHeight}, '<%= request.referer %>'); |
There was a problem hiding this comment.
Unescaped request.referer in embed layout enables reflected XSS
In app/views/layouts/embed.html.erb, request.referer is interpolated directly into a <script> block as the postMessage target origin: parent.postMessage({...}, '<%= request.referer %>');. The Referer header is attacker-controlled. A crafted referer like ');alert(document.cookie);// would break out of the string and execute arbitrary JavaScript in the context of the Discourse domain inside the iframe.
Closes a reflected XSS that executes in the Discourse origin, potentially leaking CSRF tokens or session data.
In app/views/layouts/embed.html.erb line 11, escape the referer for safe inclusion in JavaScript: use <%= escape_javascript(request.referer) %> (or j() helper) and wrap in quotes. Better yet, compute the target origin server-side as URI(request.referer).tap{|u| u.path = ''; u.query = nil}.to_s and pass that escaped value.
|
|
||
| if topic_id | ||
| @topic_view = TopicView.new(topic_id, current_user, {best: 5}) | ||
| else |
There was a problem hiding this comment.
No authentication or rate-limiting on embed topic creation endpoint
The EmbedController#best action, when no topic exists for an embed_url, enqueues a RetrieveTopic job with no authentication required (controller skips check_xhr and doesn't require login). An unauthenticated visitor can trigger HTTP fetches to arbitrary URLs matching the embeddable_host setting, creating topics and consuming server resources. The Redis-based throttle in TopicRetriever is per-URL, so an attacker can fan out across many distinct URLs.
Prevents unauthenticated users from triggering unbounded topic creation and SSRF-like HTTP fetches against the embeddable host's pages.
Add a global rate limit in TopicRetriever#retrieved_recently? that throttles by IP (or globally) in addition to per-URL throttling. For example: $redis.setnx("retrieve-global:#{request.ip}", 1) with a 10-second expiry. Also consider requiring at least a valid embed_url format before enqueuing.
|
|
||
| # First check RSS if that is enabled | ||
| if SiteSetting.feed_polling_enabled? | ||
| Jobs::PollFeed.new.execute({}) |
There was a problem hiding this comment.
Prevent job storms / synchronous scheduled job execution from TopicRetriever
TopicRetriever.perform_retrieve calls Jobs::PollFeed.new.execute({}) directly when feed polling is enabled. That runs the scheduled job inline in the request/job context that triggered retrieval, potentially doing network IO and iterating many feed items. This can amplify load or create cascading failures if many embeds miss at once (even with redis throttling per embed_url, different embed URLs can still trigger it).
Improves isolation and locality: embed retrieval remains bounded and does not unexpectedly perform global feed imports, reducing tail latency and avoiding accidental DoS during embed spikes.
In lib/topic_retriever.rb, remove the inline Jobs::PollFeed.new.execute({}) call. Instead enqueue the scheduled job (or a dedicated regular job) once per interval using a redis key (e.g., feed_poll:last_run) or rely on the scheduler exclusively. Verify behavior by adding a spec in spec/components/topic_retriever_spec.rb asserting PollFeed is enqueued rather than executed inline when feed_polling_enabled? is true.
| private | ||
|
|
||
| def ensure_embeddable | ||
| raise Discourse::InvalidAccess.new('embeddable host not set') if SiteSetting.embeddable_host.blank? |
There was a problem hiding this comment.
Use EmbeddableHost allowlist in EmbedController instead of single-host referer equality check
The new EmbedController uses SiteSetting.embeddable_host and requires URI(request.referer).host == SiteSetting.embeddable_host. Elsewhere in the repo, embedding is handled via EmbeddableHost.host_allowed?(referer), supporting multiple allowed hosts (and optional paths) and avoiding brittle equality checks. The new controller’s logic is both stricter than intended (breaks multi-host) and weaker in some ways (no path support, no development/admin exception behavior shown in the existing controller).
Avoids regressions for existing installations using multiple embeddable hosts and keeps the security policy consistent across embed entrypoints.
In app/controllers/embed_controller.rb (new best action controller), replace the referer check with the existing allowlist helper: raise ... unless EmbeddableHost.host_allowed?(request.referer) (or EmbeddableHost.record_for_host if you need category). Mirror the existing development/admin exception behavior from the current embed controller. Add/update spec in spec/controllers/embed_controller_spec.rb to cover multiple allowed hosts against the new best endpoint, similar to the existing comments endpoint coverage.
2 participants
Benchmark PR recreated from ai-code-review-evaluation/discourse-graphite for Code Review Bench. Upstream: ai-code-review-evaluation#4