Enhance embed URL handling and validation system

[ShashankFC]

Test 4nn---nReplicated from ai-code-review-evaluation/discourse-greptile#4

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 2m 9s —— View job

---

Code Review Complete

I've reviewed the PR that adds embeddable Discourse comments functionality with RSS feed polling. Here's my comprehensive analysis:

Todo List

• Check for repository-specific guidelines (CLAUDE.md)
• Understand PR changes and scope
• Review controller implementations
• Review model implementations
• Review job implementations
• Review specs and test coverage
• Review frontend code (JavaScript/CSS)
• Post comprehensive review feedback

---

Critical Security Issues

1. SSRF Vulnerability in TopicEmbed.import_remote - HIGH SEVERITY

Location: app/models/topic_embed.rb:48

The open(url).read call is a critical security vulnerability that allows Server-Side Request Forgery (SSRF) attacks. An attacker could:

• Access internal network resources (metadata services, internal APIs)
• Port scan internal networks
• Read local files using file:// protocol
• Execute denial-of-service attacks

doc = Readability::Document.new(open(url).read, ...)  # DANGEROUS!

Recommendation: Use a safe HTTP client with:

• Protocol whitelist (only http/https)
• IP address blacklist (block private ranges: 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 127.0.0.0/8, 169.254.0.0/16)
• Timeout limits
• Redirect following limits

Fix this →

2. SSRF Vulnerability in PollFeed - HIGH SEVERITY

Location: app/jobs/scheduled/poll_feed.rb:29

Same issue - open(SiteSetting.feed_polling_url) allows SSRF attacks even though it's an admin-configured setting.

rss = SimpleRSS.parse open(SiteSetting.feed_polling_url)  # DANGEROUS!

Fix this →

3. XSS Vulnerability via Unescaped HTML - HIGH SEVERITY

Location: app/models/topic_embed.rb:13,22,36

The code injects unsanitized URLs directly into HTML and uses raw_html cook method, bypassing Discourse's security:

contents << "\n<hr>\n<small>#{I18n.t('embed.imported_from', link: "<a href='#{url}'>#{url}</a>")}</small>\n"

If url contains malicious content like javascript:alert(1) or " onclick="alert(1), this creates XSS vulnerabilities.

Additionally, the raw_html cook method (line 22, 36) bypasses all sanitization, allowing any JavaScript/HTML from external sources to execute in users' browsers.

Fix this →

4. Weak Origin Validation in JavaScript - MEDIUM SEVERITY

Location: app/assets/javascripts/embed.js:17

if (discourseUrl.indexOf(e.origin) === -1) { return; }

This validation is insufficient. indexOf checks if e.origin is a substring of discourseUrl, which could be bypassed:

• If discourseUrl = "http://example.com", then e.origin = "http://example.com.evil.com" would pass
• Should use exact origin comparison instead

Fix this →

---

Security Concerns

5. X-Frame-Options Set to ALLOWALL

Location: app/controllers/embed_controller.rb:28

response.headers['X-Frame-Options'] = "ALLOWALL"

This is intentional for embedding but:

• ALLOWALL is not a valid value (should be ALLOW-FROM or omit the header)
• Consider using Content-Security-Policy: frame-ancestors instead for better control
• This allows any site to frame your content, not just the configured embeddable_host

Fix this →

6. Insufficient Referer Validation

Location: app/controllers/embed_controller.rb:26

raise Discourse::InvalidAccess.new('invalid referer host') if URI(request.referer || '').host != SiteSetting.embeddable_host

Problems:

• Referer headers can be spoofed or missing
• Only checks exact host match, doesn't handle subdomains
• No validation that the embed_url matches the referer

---

Code Quality Issues

7. Race Condition in Topic Creation

Location: lib/topic_retriever.rb:36-37

return if TopicEmbed.where(embed_url: @embed_url).exists?

There's a check-then-act race condition. Between checking and creating, another process could create the embed. The transaction in TopicEmbed.import helps, but consider using database-level uniqueness constraints.

8. Missing Error Handling

Location: app/jobs/scheduled/poll_feed.rb:29-36

No error handling for:

• Network failures when fetching the feed
• Invalid RSS/XML parsing
• Individual item processing failures (one bad item breaks the entire feed)

Should wrap in begin/rescue and log errors without breaking the job.

Fix this →

9. Inefficient Query Pattern

Location: app/models/topic_embed.rb:78-79

def self.topic_id_for_embed(embed_url)
  TopicEmbed.where(embed_url: embed_url).pluck(:topic_id).first
end

Should use .limit(1).pluck(:topic_id).first or just .where(...).first.try(:topic_id) to avoid loading unnecessary rows.

10. Missing Validation in Controller

Location: app/controllers/embed_controller.rb:9-10

embed_url = params.require(:embed_url)
topic_id = TopicEmbed.topic_id_for_embed(embed_url)

No validation that embed_url is actually a valid URL format before using it. Should validate format and potentially that it matches the allowed embeddable_host.

---

Testing Gaps

11. Incomplete Controller Tests

Location: spec/controllers/embed_controller_spec.rb:40-46

The test mocks out the actual job enqueueing:

retriever = mock
TopicRetriever.expects(:new).returns(retriever)
retriever.expects(:retrieve)

But the actual code does:

Jobs.enqueue(:retrieve_topic, user_id: current_user.try(:id), embed_url: embed_url)

The test doesn't verify the job is enqueued correctly.

12. Missing Integration Tests

No tests for:

• The complete embed flow from RSS to post creation
• URL absolutization edge cases (relative URLs with query params, fragments, etc.)
• Content-SHA1 update detection
• Concurrent access scenarios

---

Minor Issues

13. Hardcoded Cache TTL

Location: app/controllers/embed_controller.rb:19

discourse_expires_in 1.minute

Consider making this configurable via site settings.

14. Inconsistent nil Handling

Location: app/jobs/scheduled/poll_feed.rb:33

url = i.link
url = i.id if url.blank? || url !~ /^https?\:\/\//

If both i.link and i.id are invalid URLs, this will pass a non-URL string to TopicEmbed.import, which will silently fail (line 11 of topic_embed.rb checks the pattern).

15. Missing Index

Location: db/migrate/20131217174004_create_topic_embeds.rb

The migration adds an index on embed_url but not on topic_id, which is used in joins and lookups. Consider adding an index on topic_id.

---

Summary

Critical Issues: 4 high-severity security vulnerabilities (SSRF, XSS, origin validation)
Must Fix Before Merge:

• SSRF vulnerabilities (FEATURE: automatically downsize large images #1, FEATURE: per-topic unsubscribe option in emails #2)
• XSS vulnerability (Add comprehensive email validation for blocked users #3)
• Origin validation (Enhance embed URL handling and validation system #4)

Recommended Fixes:

• Frame options and CSP headers (Optimize header layout performance with flexbox mixins #5)
• Referer validation improvements (UX: show complete URL path if website domain is same as instance domain #6)
• Error handling in feed polling (FIX: proper handling of group memberships #8)

The feature implementation is solid, but the security issues are blocking. The SSRF and XSS vulnerabilities could lead to serious exploits and must be addressed before this can be merged safely.

---