Enhance embed URL handling and validation system

[akshayutture-augment]

Test 4

---

Replicated from ai-code-review-evaluation/discourse-greptile#4

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review Complete ✓

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

---

🔴 Critical Security Issues

1. XSS Vulnerability in embed.js (app/assets/javascripts/embed.js:17)

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

Issue: This validation is flawed. indexOf will match any substring, allowing malicious domains to bypass the check.

Example: If discourseUrl = "https://example.com", then e.origin = "https://notexample.com" would pass because indexOf returns a position >= 0.

Fix: Use proper origin validation:

if (e.origin !== discourseUrl.replace(/\/$/, '')) { return; }

2. XSS Vulnerability in embed layout (app/views/layouts/embed.html.erb:11)

parent.postMessage({...}, '<%= request.referer %>');

Issue: request.referer is user-controlled and inserted directly into JavaScript without sanitization. An attacker could inject malicious code.

Fix: Use JSON encoding or validate/sanitize the referer:

parent.postMessage({...}, <%= request.referer.to_json %>);

3. Potential XSS in embed view (app/views/embed/best.html.erb:19)

<div class='cooked'><%= raw post.cooked %></div>

Issue: Using raw bypasses HTML escaping. While post.cooked should already be sanitized in the normal flow, posts with cook_method: :raw_html explicitly bypass sanitization (app/models/post.rb:131-133), creating an XSS vector for RSS-imported content.

Recommendation: Ensure RSS content is properly sanitized before storage, or add sanitization at render time for embed views.

4. Open Redirect Vulnerability (lib/topic_retriever.rb:52 & app/models/topic_embed.rb:48)

TopicEmbed.import_remote(user, @embed_url)
# which calls:
open(url).read

Issue: open() with user-controlled URLs can access local files (file://) or internal network resources, leading to SSRF attacks.

Fix: Validate URLs and use a safer HTTP library:

return unless url =~ /^https?:\/\//
require 'open-uri'
open(url, redirect: false, open_timeout: 10, read_timeout: 30) do |f|
  # process
end

---

🟡 High Priority Issues

5. Missing CSRF Protection (app/controllers/embed_controller.rb:2)

skip_before_filter :check_xhr

Issue: Skipping XHR checks for embed controller removes CSRF protection. While necessary for embedding, this should be documented and carefully validated.

Recommendation: Add comprehensive origin validation to compensate.

6. Race Condition in TopicEmbed.import (app/models/topic_embed.rb:20-30)

Issue: The transaction doesn't prevent race conditions. Multiple simultaneous requests with the same URL could create duplicate topics.

Fix: Add database-level locking:

embed = TopicEmbed.where(embed_url: url).lock.first

7. SQL Injection Risk (app/jobs/scheduled/poll_feed.rb:35)

content = CGI.unescapeHTML(i.content.scrub)
TopicEmbed.import(user, url, i.title, content)

Issue: While not direct SQL injection, the RSS content is imported with skip_validations: true and cook_method: :raw_html, bypassing security checks.

Recommendation: Add content validation/sanitization even for raw HTML imports.

8. Referer Validation is Too Strict (app/controllers/embed_controller.rb:26)

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

Issue: Comparing only the host is insufficient - should also validate the scheme. Also, this breaks when referer has subdomains.

Better approach:

referer_uri = URI.parse(request.referer || '')
allowed_host = SiteSetting.embeddable_host
raise Discourse::InvalidAccess.new('invalid referer host') unless referer_uri.host == allowed_host || referer_uri.host&.end_with?(".#{allowed_host}")

---

🟢 Code Quality & Best Practices

9. Inconsistent Error Handling (lib/topic_retriever.rb:15-18)

The invalid_host? method silently catches URI::InvalidURIError but the controller raises an exception. Consider consistent error handling across the application.

10. Missing Index on content_sha1 (db/migrate/20131217174004_create_topic_embeds.rb:11)

Issue: The content_sha1 field is used for change detection but lacks an index, which could cause performance issues with many embeds.

Recommendation: Add index if this field is queried frequently.

11. Magic Number (app/controllers/embed_controller.rb:13)

@topic_view = TopicView.new(topic_id, current_user, {best: 5})

Issue: The hardcoded 5 should be a configurable site setting for flexibility.

12. Missing Validation in Controller (app/controllers/embed_controller.rb:9)

embed_url = params.require(:embed_url)

Issue: While require ensures presence, there's no validation that it's a valid URL format before passing to TopicEmbed.topic_id_for_embed.

13. Resource Exhaustion Risk (lib/topic_retriever.rb:27-28)

$redis.expire(retrieved_key, 60)

Issue: 60-second throttle might be too permissive for public endpoints. Consider rate limiting by IP or session as well.

14. Job Enqueueing Without User Context (app/controllers/embed_controller.rb:15)

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

Issue: Anonymous users can trigger background jobs. While throttled, this could still be abused.

Recommendation: Consider additional rate limiting or requiring authentication for first-time embeds.

---

📋 Test Coverage Analysis

Good coverage includes:

• ✅ Controller specs testing authentication and authorization
• ✅ Model specs for TopicEmbed import functionality
• ✅ Component specs for TopicRetriever throttling

Missing test coverage:

• ❌ No tests for the poll_feed_spec.rb job error handling
• ❌ No tests for the absolutize_urls edge cases (malformed URLs, ports)
• ❌ No tests for concurrent embed creation (race conditions)
• ❌ No tests for XSS/security scenarios
• ❌ No tests for SSRF attack vectors in URL fetching
• ❌ Missing test for the RetrieveTopic job

---

🔧 Additional Recommendations

15. Add Content-Security-Policy Headers: For the embed layout to prevent XSS

16. Add URL Whitelist/Blacklist: Beyond host validation, consider blocking private IP ranges and localhost

17. Add Logging: Log all embed URL fetches for security auditing

18. Consider Adding Retry Logic: For failed RSS/HTTP fetches in production

19. Documentation: Add inline documentation explaining the security model for embedding

20. Migration Concerns:

    • The migrations modify unrelated files (db/migrate/20131210181901_migrate_word_counts.rb:17) - consider if this is intentional
    • Migration 20131223171005_create_top_topics.rb also shows modifications

---

Summary

This PR implements a solid foundation for embeddable comments, but has critical security vulnerabilities that must be addressed before merging:

1. Must Fix: XSS vulnerabilities in embed.js and embed layout
2. Must Fix: SSRF vulnerability via open()
3. Must Fix: Insufficient origin validation
4. Should Fix: Race conditions, missing validations, rate limiting improvements

The test coverage is reasonable but should be expanded to include security test cases.

Recommendation: ⚠️ Do not merge until critical security issues are resolved.

---