-
Notifications
You must be signed in to change notification settings - Fork 0
Enhance embed URL handling and validation system #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: embed-url-handling-pre
Are you sure you want to change the base?
Conversation
| return if user.blank? | ||
|
|
||
| require 'simple-rss' | ||
| rss = SimpleRSS.parse open(SiteSetting.feed_polling_url) |
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.
[Security] Server-Side Request Forgery (SSRF) in poll_feed
- Problem: The
open(SiteSetting.feed_polling_url)call lacks URL validation, making it vulnerable to SSRF if the URL is attacker-controlled. - Fix: Implement strict URL validation (whitelist schemes, restrict public IP ranges, prevent internal network access) before calling
open.
| def cook(*args) | ||
| # For some posts, for example those imported via RSS, we support raw HTML. In that | ||
| # case we can skip the rendering pipeline. | ||
| return raw if cook_method == Post.cook_methods[:raw_html] |
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.
[Security] Cross-Site Scripting (XSS) via raw_html cook method
- Problem: The
raw_htmlcook method bypasses sanitization, allowing malicious HTML/JavaScript from imported content to be rendered directly, leading to XSS. - Fix: Apply robust HTML sanitization (e.g.,
Sanitizegem) to all externalcontentsbefore saving and rendering, even whencook_methodisraw_html.
| require 'ruby-readability' | ||
|
|
||
| opts = opts || {} | ||
| doc = Readability::Document.new(open(url).read, |
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.
[Security] Server-Side Request Forgery (SSRF) in import_remote
- Problem: The
open(url).readcall inimport_remotelacks URL validation, making it vulnerable to SSRF ifurlis attacker-controlled. - Fix: Implement strict URL validation (whitelist schemes, restrict public IP ranges, prevent internal network access) before calling
open.
| <img src='<%= post.user.small_avatar_url %>'> | ||
| <h3><%= post.user.username %></h3> | ||
| </div> | ||
| <div class='cooked'><%= raw post.cooked %></div> |
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.
[Security] Cross-Site Scripting (XSS) via raw post.cooked rendering
- Problem: Directly rendering
raw post.cookedfrom untrusted external sources allows malicious HTML/JavaScript to execute, leading to XSS. - Fix: Ensure all
raw_htmlcontent is thoroughly sanitized against XSS before storage or rendering, or apply a strict sanitization filter at the rendering stage.
|
|
||
| function postMessageReceived(e) { | ||
| if (!e) { return; } | ||
| if (discourseUrl.indexOf(e.origin) === -1) { return; } |
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.
[Security] Weak postMessage Origin Validation
- Problem: The
indexOfcheck fore.originis insufficient, allowing attackers to bypass validation with crafted domains and receive sensitivepostMessagedata. - Fix: Implement robust origin validation using exact equality (
e.origin === discourseUrl) or by parsing and comparingURL.hostproperties.
| validates_presence_of :content_sha1 | ||
|
|
||
| # Import an article from a source (RSS/Atom/Other) | ||
| def self.import(user, url, title, contents) |
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.
[Architecture] Service Logic in TopicEmbed Model
- Problem:
TopicEmbed.importandTopicEmbed.import_remotecontain extensive business logic, violating the Single Responsibility Principle and making the model bloated and less maintainable. - Fix: Extract complex service-like logic (content fetching, parsing, orchestration) into dedicated service objects or modules.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Security] Weak postMessage Target Origin
- Problem: Using
request.refererastargetOriginforpostMessageis insecure, asReferercan be spoofed, leading to information leakage to malicious origins. - Fix: Set
targetOriginto a fixed, known, and trusted origin (e.g.,SiteSetting.embeddable_host) instead of relying onrequest.referer.
| end | ||
|
|
||
| def cook(*args) | ||
| # For some posts, for example those imported via RSS, we support raw HTML. In that |
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.
[Architecture] Divergent Post Processing Pipeline for raw_html
- Problem:
raw_htmlposts bypass the standardPlugin::Filter.apply(:after_post_cook)step, creating inconsistencies and potential missed plugin functionality. - Fix: Re-evaluate the
raw_htmlcook method to ensure it integrates with or explicitly handles the standard post-cooking pipeline, or document the divergence clearly.
|
|
||
| # First check RSS if that is enabled | ||
| if SiteSetting.feed_polling_enabled? | ||
| Jobs::PollFeed.new.execute({}) |
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.
[Architecture] Synchronous Job Execution in TopicRetriever
- Problem:
TopicRetrieverdirectly callsJobs::PollFeed.new.execute({}), bypassing the asynchronous job queue and potentially blocking the retriever. - Fix: Enqueue
Jobs::PollFeedproperly for background execution to leverage asynchronous processing and avoid blockingTopicRetriever.
Test 4