-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: embed-url-handling-post #319
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| /* global discourseUrl */ | ||
| /* global discourseEmbedUrl */ | ||
| (function() { | ||
|
|
||
| var comments = document.getElementById('discourse-comments'), | ||
| iframe = document.createElement('iframe'); | ||
| iframe.src = discourseUrl + "embed/best?embed_url=" + encodeURIComponent(discourseEmbedUrl); | ||
| iframe.id = 'discourse-embed-frame'; | ||
| iframe.width = "100%"; | ||
| iframe.frameBorder = "0"; | ||
| iframe.scrolling = "no"; | ||
| comments.appendChild(iframe); | ||
|
|
||
|
|
||
| function postMessageReceived(e) { | ||
| if (!e) { return; } | ||
| if (discourseUrl.indexOf(e.origin) === -1) { return; } | ||
|
|
||
| if (e.data) { | ||
| if (e.data.type === 'discourse-resize' && e.data.height) { | ||
| iframe.height = e.data.height + "px"; | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+15
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Origin validation is vulnerable to bypass attacks. The 🔒 Proposed fix for stricter origin validation function postMessageReceived(e) {
if (!e) { return; }
- if (discourseUrl.indexOf(e.origin) === -1) { return; }
+ // Extract origin from discourseUrl for strict comparison
+ var defined = document.createElement('a');
+ defined.href = discourseUrl;
+ var expectedOrigin = defined.protocol + '//' + defined.host;
+ if (e.origin !== expectedOrigin) { return; }
if (e.data) {🤖 Prompt for AI Agents |
||
| window.addEventListener('message', postMessageReceived, false); | ||
|
|
||
| })(); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| //= require ./vendor/normalize | ||
| //= require ./common/foundation/base | ||
|
|
||
| article.post { | ||
| border-bottom: 1px solid #ddd; | ||
|
|
||
| .post-date { | ||
| float: right; | ||
| color: #aaa; | ||
| font-size: 12px; | ||
| margin: 4px 4px 0 0; | ||
| } | ||
|
|
||
| .author { | ||
| padding: 20px 0; | ||
| width: 92px; | ||
| float: left; | ||
|
|
||
| text-align: center; | ||
|
|
||
| h3 { | ||
| text-align: center; | ||
| color: #4a6b82; | ||
| font-size: 13px; | ||
| margin: 0; | ||
| } | ||
| } | ||
|
|
||
| .cooked { | ||
| padding: 20px 0; | ||
| margin-left: 92px; | ||
|
|
||
| p { | ||
| margin: 0 0 1em 0; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| header { | ||
| padding: 10px 10px 20px 10px; | ||
|
|
||
| font-size: 18px; | ||
|
|
||
| border-bottom: 1px solid #ddd; | ||
| } | ||
|
|
||
| footer { | ||
| font-size: 18px; | ||
|
|
||
| .logo { | ||
| margin-right: 10px; | ||
| margin-top: 10px; | ||
| } | ||
|
|
||
| a[href].button { | ||
| margin: 10px 0 0 10px; | ||
| } | ||
| } | ||
|
|
||
| .logo { | ||
| float: right; | ||
| max-height: 30px; | ||
| } | ||
|
|
||
| a[href].button { | ||
| background-color: #eee; | ||
| padding: 5px; | ||
| display: inline-block; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| class EmbedController < ApplicationController | ||
| skip_before_filter :check_xhr | ||
| skip_before_filter :preload_json | ||
| before_filter :ensure_embeddable | ||
|
|
||
| layout 'embed' | ||
|
|
||
| def best | ||
| embed_url = params.require(:embed_url) | ||
| topic_id = TopicEmbed.topic_id_for_embed(embed_url) | ||
|
|
||
| if topic_id | ||
| @topic_view = TopicView.new(topic_id, current_user, {best: 5}) | ||
| else | ||
| Jobs.enqueue(:retrieve_topic, user_id: current_user.try(:id), embed_url: embed_url) | ||
| render 'loading' | ||
| end | ||
|
|
||
| discourse_expires_in 1.minute | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def ensure_embeddable | ||
| 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" | ||
| rescue URI::InvalidURIError | ||
| raise Discourse::InvalidAccess.new('invalid referer host') | ||
| end | ||
|
|
||
|
|
||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| require_dependency 'email/sender' | ||
| require_dependency 'topic_retriever' | ||
|
|
||
| module Jobs | ||
|
|
||
| # Asynchronously retrieve a topic from an embedded site | ||
| class RetrieveTopic < Jobs::Base | ||
|
|
||
| def execute(args) | ||
| raise Discourse::InvalidParameters.new(:embed_url) unless args[:embed_url].present? | ||
|
|
||
| user = nil | ||
| if args[:user_id] | ||
| user = User.where(id: args[:user_id]).first | ||
| end | ||
|
|
||
| TopicRetriever.new(args[:embed_url], no_throttle: user.try(:staff?)).retrieve | ||
| end | ||
|
|
||
| end | ||
|
|
||
| end | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # | ||
| # Creates and Updates Topics based on an RSS or ATOM feed. | ||
| # | ||
| require 'digest/sha1' | ||
| require_dependency 'post_creator' | ||
| require_dependency 'post_revisor' | ||
| require 'open-uri' | ||
|
|
||
| module Jobs | ||
| class PollFeed < Jobs::Scheduled | ||
| recurrence { hourly } | ||
| sidekiq_options retry: false | ||
|
|
||
| def execute(args) | ||
| poll_feed if SiteSetting.feed_polling_enabled? && | ||
| SiteSetting.feed_polling_url.present? && | ||
| SiteSetting.embed_by_username.present? | ||
| end | ||
|
|
||
| def feed_key | ||
| @feed_key ||= "feed-modified:#{Digest::SHA1.hexdigest(SiteSetting.feed_polling_url)}" | ||
| end | ||
|
|
||
| def poll_feed | ||
| user = User.where(username_lower: SiteSetting.embed_by_username.downcase).first | ||
| return if user.blank? | ||
|
|
||
| require 'simple-rss' | ||
| rss = SimpleRSS.parse open(SiteSetting.feed_polling_url) | ||
|
|
||
| rss.items.each do |i| | ||
| url = i.link | ||
| url = i.id if url.blank? || url !~ /^https?\:\/\// | ||
|
|
||
| content = CGI.unescapeHTML(i.content.scrub) | ||
| TopicEmbed.import(user, url, i.title, content) | ||
| end | ||
| end | ||
|
Comment on lines
+24
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing error handling and potential NoMethodError on nil content. The 🐛 Proposed fix with error handling and nil safety def poll_feed
user = User.where(username_lower: SiteSetting.embed_by_username.downcase).first
return if user.blank?
- require 'simple-rss'
- rss = SimpleRSS.parse open(SiteSetting.feed_polling_url)
-
- rss.items.each do |i|
- url = i.link
- url = i.id if url.blank? || url !~ /^https?\:\/\//
-
- content = CGI.unescapeHTML(i.content.scrub)
- TopicEmbed.import(user, url, i.title, content)
+ begin
+ require 'simple-rss'
+ feed_response = URI.open(SiteSetting.feed_polling_url, read_timeout: 30, open_timeout: 10)
+ rss = SimpleRSS.parse(feed_response)
+
+ rss.items.each do |i|
+ url = i.link
+ url = i.id if url.blank? || url !~ /^https?\:\/\//
+
+ raw_content = i.content || i.description || ''
+ content = CGI.unescapeHTML(raw_content.to_s.scrub)
+ TopicEmbed.import(user, url, i.title, content)
+ end
+ rescue StandardError => e
+ Rails.logger.error("PollFeed failed: #{e.message}")
end
end🧰 Tools🪛 Brakeman (8.0.1)[medium] 29-29: Model attribute used in file name (File Access) 🤖 Prompt for AI Agents |
||
|
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,82 @@ | ||||||||||||||||||||||
| require_dependency 'nokogiri' | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| class TopicEmbed < ActiveRecord::Base | ||||||||||||||||||||||
| belongs_to :topic | ||||||||||||||||||||||
| belongs_to :post | ||||||||||||||||||||||
| validates_presence_of :embed_url | ||||||||||||||||||||||
| validates_presence_of :content_sha1 | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Import an article from a source (RSS/Atom/Other) | ||||||||||||||||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential XSS: URL is interpolated into HTML without escaping. The 🛡️ Proposed fix using ERB::Util for escaping+ require 'erb'
+
# Import an article from a source (RSS/Atom/Other)
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"
+ escaped_url = ERB::Util.html_escape(url)
+ contents << "\n<hr>\n<small>#{I18n.t('embed.imported_from', link: "<a href='#{escaped_url}'>#{escaped_url}</a>")}</small>\n"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| embed = TopicEmbed.where(embed_url: url).first | ||||||||||||||||||||||
| content_sha1 = Digest::SHA1.hexdigest(contents) | ||||||||||||||||||||||
| post = nil | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # If there is no embed, create a topic, post and the embed. | ||||||||||||||||||||||
| if embed.blank? | ||||||||||||||||||||||
| Topic.transaction do | ||||||||||||||||||||||
| creator = PostCreator.new(user, title: title, raw: absolutize_urls(url, contents), skip_validations: true, cook_method: Post.cook_methods[:raw_html]) | ||||||||||||||||||||||
| post = creator.create | ||||||||||||||||||||||
| if post.present? | ||||||||||||||||||||||
| TopicEmbed.create!(topic_id: post.topic_id, | ||||||||||||||||||||||
| embed_url: url, | ||||||||||||||||||||||
| content_sha1: content_sha1, | ||||||||||||||||||||||
| post_id: post.id) | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
| else | ||||||||||||||||||||||
| post = embed.post | ||||||||||||||||||||||
| # Update the topic if it changed | ||||||||||||||||||||||
| if content_sha1 != embed.content_sha1 | ||||||||||||||||||||||
| revisor = PostRevisor.new(post) | ||||||||||||||||||||||
| revisor.revise!(user, absolutize_urls(url, contents), skip_validations: true, bypass_rate_limiter: true) | ||||||||||||||||||||||
| embed.update_column(:content_sha1, content_sha1) | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| post | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def self.import_remote(user, url, opts=nil) | ||||||||||||||||||||||
| require 'ruby-readability' | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| opts = opts || {} | ||||||||||||||||||||||
| doc = Readability::Document.new(open(url).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) | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
|
Comment on lines
+44
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: SSRF vulnerability and missing error handling in Using 🔒 Proposed fix with validation and error handling def self.import_remote(user, url, opts=nil)
require 'ruby-readability'
opts = opts || {}
+
+ begin
+ uri = URI.parse(url)
+ raise "Invalid URL scheme" unless %w[http https].include?(uri.scheme)
+
+ # Add timeout and redirect limits
+ response = uri.open(read_timeout: 30, open_timeout: 10)
+ html_content = response.read(1.megabyte) # Limit content size
+ rescue StandardError => e
+ Rails.logger.error("Failed to fetch embed URL #{url}: #{e.message}")
+ return nil
+ end
+
- doc = Readability::Document.new(open(url).read,
+ doc = Readability::Document.new(html_content,
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)
end🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Convert any relative URLs to absolute. RSS is annoying for this. | ||||||||||||||||||||||
| def self.absolutize_urls(url, contents) | ||||||||||||||||||||||
| uri = URI(url) | ||||||||||||||||||||||
| prefix = "#{uri.scheme}://#{uri.host}" | ||||||||||||||||||||||
| prefix << ":#{uri.port}" if uri.port != 80 && uri.port != 443 | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fragment = Nokogiri::HTML.fragment(contents) | ||||||||||||||||||||||
| fragment.css('a').each do |a| | ||||||||||||||||||||||
| href = a['href'] | ||||||||||||||||||||||
| if href.present? && href.start_with?('/') | ||||||||||||||||||||||
| a['href'] = "#{prefix}/#{href.sub(/^\/+/, '')}" | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
| fragment.css('img').each do |a| | ||||||||||||||||||||||
| src = a['src'] | ||||||||||||||||||||||
| if src.present? && src.start_with?('/') | ||||||||||||||||||||||
| a['src'] = "#{prefix}/#{src.sub(/^\/+/, '')}" | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| fragment.to_html | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def self.topic_id_for_embed(embed_url) | ||||||||||||||||||||||
| TopicEmbed.where(embed_url: embed_url).pluck(:topic_id).first | ||||||||||||||||||||||
| end | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| end | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,30 @@ | ||||||||||
| <header> | ||||||||||
| <%- if @topic_view.posts.present? %> | ||||||||||
| <%= 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') %> | ||||||||||
|
Comment on lines
+3
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add Prevent reverse‑tabnabbing by adding 🔒 Proposed fix- <%= link_to(I18n.t('embed.title'), `@topic_view.topic.url`, class: 'button', target: '_blank') %>
+ <%= link_to(I18n.t('embed.title'), `@topic_view.topic.url`, class: 'button', target: '_blank', rel: 'noopener noreferrer') %>
...
- <%= link_to(I18n.t('embed.start_discussion'), `@topic_view.topic.url`, class: 'button', target: '_blank') %>
+ <%= link_to(I18n.t('embed.start_discussion'), `@topic_view.topic.url`, class: 'button', target: '_blank', rel: 'noopener noreferrer') %>
...
- <%= link_to post.created_at.strftime("%e %b %Y"), post.url, class: 'post-date', target: "_blank" %>
+ <%= link_to post.created_at.strftime("%e %b %Y"), post.url, class: 'post-date', target: "_blank", rel: 'noopener noreferrer' %>
...
- <%= link_to(I18n.t('embed.continue'), `@topic_view.topic.url`, class: 'button', target: '_blank') %>
+ <%= link_to(I18n.t('embed.continue'), `@topic_view.topic.url`, class: 'button', target: '_blank', rel: 'noopener noreferrer') %>Also applies to: 14-14, 25-25 🤖 Prompt for AI Agents |
||||||||||
| <%- end if %> | ||||||||||
|
|
||||||||||
| <%= link_to(image_tag(SiteSetting.logo_url, class: 'logo'), Discourse.base_url) %> | ||||||||||
| </header> | ||||||||||
|
|
||||||||||
| <%- if @topic_view.posts.present? %> | ||||||||||
| <%- @topic_view.posts.each do |post| %> | ||||||||||
| <article class='post'> | ||||||||||
| <%= link_to post.created_at.strftime("%e %b %Y"), post.url, class: 'post-date', target: "_blank" %> | ||||||||||
| <div class='author'> | ||||||||||
| <img src='<%= post.user.small_avatar_url %>'> | ||||||||||
| <h3><%= post.user.username %></h3> | ||||||||||
|
Comment on lines
+16
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add alt text to the avatar image. This improves accessibility for screen‑reader users. ♿ Proposed fix- <img src='<%= post.user.small_avatar_url %>'>
+ <%= image_tag(post.user.small_avatar_url, alt: post.user.username, class: "avatar") %>📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| </div> | ||||||||||
| <div class='cooked'><%= raw post.cooked %></div> | ||||||||||
| <div style='clear: both'></div> | ||||||||||
| </article> | ||||||||||
| <%- end %> | ||||||||||
|
|
||||||||||
| <footer> | ||||||||||
| <%= link_to(I18n.t('embed.continue'), @topic_view.topic.url, class: 'button', target: '_blank') %> | ||||||||||
| <%= link_to(image_tag(SiteSetting.logo_url, class: 'logo'), Discourse.base_url) %> | ||||||||||
| </footer> | ||||||||||
|
|
||||||||||
| <% end %> | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add null check for the target DOM element.
If the
discourse-commentselement doesn't exist on the page,comments.appendChild(iframe)will throw an error. Consider adding defensive validation.🛡️ Proposed fix
var comments = document.getElementById('discourse-comments'), iframe = document.createElement('iframe'); +if (!comments) { return; } iframe.src = discourseUrl + "embed/best?embed_url=" + encodeURIComponent(discourseEmbedUrl);📝 Committable suggestion
🤖 Prompt for AI Agents