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

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

Merged
merged 1 commit into from Feb 8, 2020

Conversation

@Gargron
Copy link
Member

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
Collaborator

ThibG left a comment

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.

@ThibG

This comment has been minimized.

Copy link
Collaborator

ThibG commented Feb 4, 2020

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 Feb 7, 2020
@Gargron Gargron requested a review from ThibG Feb 7, 2020
@ThibG
ThibG approved these changes Feb 7, 2020
Copy link
Collaborator

ThibG left a comment

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

app/lib/sanitize_config.rb Show resolved Hide resolved
end
rescue Addressable::URI::InvalidURIError
nil
end

This comment has been minimized.

Copy link
@ThibG

ThibG Feb 7, 2020

Collaborator

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 Feb 8, 2020
- 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 Feb 8, 2020
@Gargron Gargron requested a review from ThibG Feb 8, 2020
@ThibG
ThibG approved these changes Feb 8, 2020
@Gargron Gargron merged commit b134934 into master Feb 8, 2020
2 checks passed
2 checks passed
build-and-test Workflow: build-and-test
Details
codeclimate All good!
Details
@Gargron Gargron deleted the fix-hrefless-anchors branch Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.