Skip to content

Fix rendering <a> without href when scheme unsupported#13040

Merged
Gargron merged 1 commit intomasterfrom
fix-hrefless-anchors
Feb 8, 2020
Merged

Fix rendering <a> without href when scheme unsupported#13040
Gargron merged 1 commit intomasterfrom
fix-hrefless-anchors

Conversation

@Gargron
Copy link
Copy Markdown
Member

@Gargron Gargron commented Feb 4, 2020

Close #13037

This replaces an <a> that has no href with its children. But maybe we don't want any children elements like <span> to stick around?

Copy link
Copy Markdown
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

If the link has Mastodon-style formatting, it will hide the invisible parts and apply ellipsis, making the link unusable. So there needs to be some extra processing, or the CSS needs to be reworked.

Comment thread app/lib/sanitize_config.rb
@ClearlyClaire
Copy link
Copy Markdown
Contributor

I'm not opposed to removing the <a> tag entirely, but it seems more complicated and error-prone than the purely-CSS solution.

Also, a <a> tag with no href attribute is semantically fine according to the HTML 5.1 specs (it says it must not have a target attribute though).

@Gargron Gargron force-pushed the fix-hrefless-anchors branch from c49dbd2 to d0ba3fa Compare February 7, 2020 14:54
@Gargron Gargron requested a review from ClearlyClaire February 7, 2020 14:56
Copy link
Copy Markdown
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

seems good overall, except for the inlined nitpicks

also, I'm not sure it makes sense to allow all the supported protocols (which shouldn't be called HTTP_PROTOCOLS anymore) in embed and iframe tags

Comment thread app/lib/sanitize_config.rb
end
rescue Addressable::URI::InvalidURIError
nil
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This whole thing is done without fully parsing URLs in Sanitize
cf. https://github.com/rgrove/sanitize/blob/master/lib/sanitize/transformers/clean_element.rb#L143

It's probably safer (in terms of possible changed behaviors) and more CPU-efficient to do it the same way.

Btw, I think we should either reject URLs or rewrite them to be relative to the original status (an option I prefer, but is maybe not worth it).

@Gargron Gargron force-pushed the fix-hrefless-anchors branch 2 times, most recently from 8160ace to 543551e Compare February 8, 2020 15:36
- Disallow links with relative paths
- Disallow iframes with non-http protocols and relative paths

Close #13037
@Gargron Gargron force-pushed the fix-hrefless-anchors branch from 543551e to 1b5ca22 Compare February 8, 2020 16:16
@Gargron Gargron requested a review from ClearlyClaire February 8, 2020 16:30
@Gargron Gargron merged commit b134934 into master Feb 8, 2020
@Gargron Gargron deleted the fix-hrefless-anchors branch February 8, 2020 20:22
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.

2 participants