Skip to content

Enhance embed URL handling and validation system#4

Open
ShashankFC wants to merge 1 commit into
embed-url-handling-prefrom
embed-url-handling-post
Open

Enhance embed URL handling and validation system#4
ShashankFC wants to merge 1 commit into
embed-url-handling-prefrom
embed-url-handling-post

Conversation

@ShashankFC

Copy link
Copy Markdown
Collaborator

@GitHoobar

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f8aed295a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/models/topic_embed.rb
Comment on lines +45 to +49
require 'ruby-readability'

opts = opts || {}
doc = Readability::Document.new(open(url).read,
tags: %w[div p code pre h1 h2 h3 b em i strong a img],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require open-uri before calling open(url)

TopicEmbed.import_remote calls open(url).read without requiring open-uri. In a Sidekiq worker or task that hasn't already loaded open-uri, this will invoke Kernel#open on a string like http://... and raise Errno::ENOENT, so embed retrieval and Disqus imports can fail immediately depending on load order. Adding require 'open-uri' (or using URI.open) in this method would make the fetch reliable regardless of what other code has been loaded.

Useful? React with 👍 / 👎.

Comment thread app/models/topic_embed.rb
Comment on lines +62 to +66
fragment.css('a').each do |a|
href = a['href']
if href.present? && href.start_with?('/')
a['href'] = "#{prefix}/#{href.sub(/^\/+/, '')}"
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle non-root relative URLs in import

absolutize_urls only rewrites URLs that start with /, so relative links like href="images/foo.png" or src="../img.png" remain unchanged. When the post is rendered on the Discourse domain, those URLs resolve against the forum host instead of the source site, breaking links/images for many RSS/HTML feeds that use document-relative paths. Consider using URI.join with the source URL for all relative paths, not just root-relative ones.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants