Enhance embed URL handling and validation system#4
Conversation
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bugbot free trial expires on August 11, 2025
Learn more in the Cursor dashboard.
| url = i.link | ||
| url = i.id if url.blank? || url !~ /^https?\:\/\// | ||
|
|
||
| content = CGI.unescapeHTML(i.content.scrub) |
There was a problem hiding this comment.
Bug: Feed Polling Job Fails and Vulnerable to SSRF
The feed polling job is affected by two issues: a NoMethodError occurs when i.content is nil because .scrub is called directly on it, causing the job to fail; and an SSRF vulnerability exists due to the use of open(SiteSetting.feed_polling_url), potentially allowing access to internal resources or local files via malicious URLs.
Locations (1)
| src = a['src'] | ||
| if src.present? && src.start_with?('/') | ||
| a['src'] = "#{prefix}/#{src.sub(/^\/+/, '')}" | ||
| end |
There was a problem hiding this comment.
Bug: URL Absolutization and Port Handling Errors
The absolutize_urls method contains two bugs:
- It incorrectly absolutizes protocol-relative URLs (e.g.,
//example.com/path), treating them as relative paths and prepending the full scheme/host, resulting in malformed URLs like{prefix}/example.com/path. - The port inclusion logic is flawed, adding the port unless it's 80 or 443, without considering the URL's scheme. This incorrectly excludes ports for HTTPS URLs on port 80 and HTTP URLs on port 443.
Locations (1)
| 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.
Bug: Import Method Error and XSS Vulnerability
The TopicEmbed.import method is susceptible to a NoMethodError if the contents parameter is nil when attempting to append a string, and an XSS vulnerability due to unescaped url interpolation in the generated HTML.
Locations (1)
|
|
||
| function postMessageReceived(e) { | ||
| if (!e) { return; } | ||
| if (discourseUrl.indexOf(e.origin) === -1) { return; } |
There was a problem hiding this comment.
Bug: Origin Validation Vulnerable to Subdomain Attacks
The origin validation check discourseUrl.indexOf(e.origin) === -1 is vulnerable to subdomain attacks. Using substring matching, it allows malicious origins (e.g., https://discourse.example for https://discourse.example.com) to bypass the security check if they are a substring of discourseUrl. This enables them to send postMessage events to resize the iframe. A more precise origin comparison (e.g., exact equality) is required.
Locations (1)
|
This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days. |
3 participants
Test 4