Enhance embed URL handling and validation system#8
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive embed URL handling and validation system that enables external sites to embed Discourse forum content. The implementation includes URL validation, RSS/ATOM feed polling for automated content import, and a complete embedding infrastructure with frontend and backend components.
Key changes:
- Adds TopicEmbed model with URL validation and content import capabilities
- Implements RSS/ATOM feed polling job for automated topic creation
- Creates EmbedController with validation and topic retrieval functionality
Reviewed Changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/models/topic_embed.rb | Core model implementing embed URL validation and content import logic |
| app/models/post.rb | Adds cook_methods enum to support raw HTML rendering for imported posts |
| app/controllers/embed_controller.rb | Controller handling embed requests with host and referer validation |
| app/jobs/scheduled/poll_feed.rb | Scheduled job for polling and importing RSS/ATOM feeds |
| app/jobs/regular/retrieve_topic.rb | Async job for retrieving topics from embedded sites |
| lib/topic_retriever.rb | Service handling topic retrieval with URL validation and throttling |
| lib/post_creator.rb | Enhanced to support custom cook_method for raw HTML posts |
| lib/post_revisor.rb | Updated to support skip_validations option |
| lib/tasks/disqus.thor | Refactored to use TopicEmbed.import_remote instead of direct PostCreator |
| db/migrate/20131217174004_create_topic_embeds.rb | Migration creating topic_embeds table |
| db/migrate/20131219203905_add_cook_method_to_posts.rb | Migration adding cook_method column to posts |
| db/migrate/20131223171005_create_top_topics.rb | Migration updated with force: true option |
| config/site_settings.yml | Adds embedding configuration settings |
| config/routes.rb | Adds embed/best route |
| config/locales/server.en.yml | Adds server-side embed translations |
| config/locales/client.en.yml | Adds client-side embedding label |
| app/views/layouts/embed.html.erb | Layout for embedded content with iframe resizing |
| app/views/embed/best.html.erb | View displaying best posts for embedded topics |
| app/views/embed/loading.html.erb | Loading view with auto-refresh |
| app/assets/stylesheets/embed.css.scss | Styles for embedded content |
| app/assets/javascripts/embed.js | Client-side iframe integration script |
| Gemfile | Adds ruby-readability and simple-rss dependencies |
| spec/models/topic_embed_spec.rb | Tests for TopicEmbed model functionality |
| spec/controllers/embed_controller_spec.rb | Tests for EmbedController validation and behavior |
| spec/jobs/poll_feed_spec.rb | Tests for PollFeed job execution |
| spec/components/topic_retriever_spec.rb | Tests for TopicRetriever service |
|
|
||
| private | ||
|
|
||
| def invalid_host? |
There was a problem hiding this comment.
This comparison could fail when embeddable_host is nil. The method should check for nil before the host comparison, or combine the nil check into this condition.
| def invalid_host? | |
| def invalid_host? | |
| return true if SiteSetting.embeddable_host.nil? |
| require 'ruby-readability' | ||
|
|
||
| opts = opts || {} | ||
| doc = Readability::Document.new(open(url).read, |
There was a problem hiding this comment.
Using open(url).read with user-provided URLs creates a Server-Side Request Forgery (SSRF) vulnerability. The URL should be validated to prevent access to internal network resources, or use a safer HTTP client with configurable restrictions.
| 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') |
There was a problem hiding this comment.
The referer validation is insufficient as it can be empty (nil coalesced to empty string), which would cause URI parsing to return nil for the host, potentially bypassing the check. Additionally, referer headers can be spoofed. Consider adding explicit nil checks and additional validation.
| 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') | |
| referer = request.referer | |
| if referer.blank? | |
| raise Discourse::InvalidAccess.new('missing referer') | |
| end | |
| begin | |
| referer_uri = URI.parse(referer) | |
| rescue URI::InvalidURIError | |
| raise Discourse::InvalidAccess.new('invalid referer host') | |
| end | |
| referer_host = referer_uri.host | |
| if referer_host.blank? || referer_host.downcase != SiteSetting.embeddable_host.downcase | |
| raise Discourse::InvalidAccess.new('invalid referer host') | |
| end | |
| response.headers['X-Frame-Options'] = "ALLOWALL" |
Test 4
Replicated from ai-code-review-evaluation/discourse-copilot#4