Skip to content
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

Prevent links in PR description that link to redirect.redirect.github.com #7190

Merged
merged 1 commit into from
Jul 16, 2023

Conversation

stefangr
Copy link
Contributor

The different sections of the metadata go through the LinkAndMentionSanitizer twice.

Once per sub section and once as the complete message.

In case of source providers that support HTML that is not a problem. The Markdown is converted to HTML and after that left alone.

In case of a source provider that does not support HTML (like bitbucket) there is a problem.
The links in the markdown are converted from github.com to redirect.github.com. And in the second pass the links are converted from redirect.github.com to redirect.redirect.github.com.

This PR contains a fix so only the second pass will convert the links in case the source provider does not support HTML.

@stefangr stefangr requested a review from a team as a code owner April 28, 2023 11:41
@stefangr stefangr changed the title Prevent double conversion of github.com to redirect.github.com Prevent links in PR description that link to redirect.redirect.github.com May 13, 2023
Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

What you're trying to do makes complete sense. 👍

Unfortunately, this solution is tracking state across function calls, and that can be hard to reason about and gets brittle quickly.

From a quick look at LinkAndMentionSanitizer, it looks like the github_redirection_service is the only URL that gets rewritten.

Would it be possible to instead modify the method that does the rewrite so that if the URL already matches the rewritten URL that it won't attempt another rewrite?

def replace_github_host(text)
text.gsub(
/(www\.)?github.com/, github_redirection_service || "github.com"
)
end

That would be a bit more robust and keep all the "rewriting" logic constrained within that method.

…anitizer twice.

Once per sub section and once as the complete message.

In case of source providers that support HTML that is not a problem.
The Markdown is converted to HTML and after that left alone.

In case of a source provider that does not support HTML there is a problem.
The links in the markdown are converted from github.com to redirect.github.com.
And in the second pass the links are converted from redirect.github.com to redirect.redirect.github.com.
@stefangr stefangr force-pushed the fix-double-redirect-conversion branch from c2109be to 781ecb7 Compare July 16, 2023 13:32
Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

This looks so much more maintainable, thanks!

@jeffwidman jeffwidman merged commit 2530fd0 into dependabot:main Jul 16, 2023
102 checks passed
@stefangr stefangr deleted the fix-double-redirect-conversion branch July 16, 2023 16:42
brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
…anitizer twice. (dependabot#7190)

Once per sub section and once as the complete message.

In case of source providers that support HTML that is not a problem.
The Markdown is converted to HTML and after that left alone.

In case of a source provider that does not support HTML there is a problem.
The links in the markdown are converted from github.com to redirect.github.com.
And in the second pass the links are converted from redirect.github.com to redirect.redirect.github.com.
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.

None yet

2 participants