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
[manpage] don't emit OSC 8 hyperlinks for anchor references (#12108) #12260
Conversation
I still want to restrict those protocols to avoid unexpected actions if you click on them (see https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml). I bet any URI scheme can be insecure but I would prefer restricting to the "well-known" schemes and the most common ones. On the other hand, if you want to handle more protocols, I think we can make it a configuration value. |
It's fair enough that clickable links should be restricted to safe ones, especially since we're running in a different security context than a browser. I'll do that. |
That's a sound argument. So maybe you could store the allowed protocols at the class level and users could simply override that field in extensions (without needing a configuration value). |
…oc#12108) A reference like ":ref:`Some other page <some-other-page>`" results in a refuri "#some-other-page". This does not seem useful to readers of the man page. It is especially unhelpful when using a terminal that implements a hint mode for selecting links -- the extra links add noise, making it harder to select the interesting ones. Don't emit OSC 8 for those. Also don't emit it for URLs that might be unsafe (see sphinx-doc#12260 (comment)) I don't know one that would be unsafe but I'm sure there are some. OTOH, "man_show_urls" doesn't seem unsafe because it shows the exact URL that will be opened, so enable that for all URLs (except for relative ones). Follow up to sphinx-doc#12108 I also confirmed that even with docutils 0.21 we do not need to override depart_reference because we already skip reference nodes.
4d82101
to
829c390
Compare
sphinx/writers/manpage.py
Outdated
@@ -319,7 +320,7 @@ def visit_reference(self, node: Element) -> None: | |||
self.visit_Text(node) | |||
self.body.append(self.defs['reference'][1]) | |||
|
|||
if uri.startswith(('mailto:', 'http:', 'https:', 'ftp:')): | |||
if not uri.startswith('#'): |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I agree. That being said, it's the existing behavior (however weird it is) and I don't have a concrete motivation to want to change it. So this is fine by me, can revisit this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it's dumb from me. The if
would be equivalent to is_safe_to_click
, so applying my suggesting is the same as not having your PR at all.
Now, the if-body is doing some processing for mailto links and cuts the URI. After, we check if the URi is still empty or not and emit OSC if needed. I'd suggest doing something different and perhaps more flexible:
- Check whether you are using an URI scheme or not: if so, just keep the current behaviour. In particular, you cannot be an achor if you are using an URI scheme.
- For anchor references, do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure what you mean, do you think it's fine that arbitrary URIs remain clickable?
I don't have a strong opinion on that one. But better safe than sorry I guess, which is why this PR disables them.
I mainly care about fixing anchor-references (by not showing them at all), and making man_show_urls
show all non-anchor URIs as text
Here are the cases, starred are the ones fixed by this current PR
refuri | clickable | shown with man_show_urls |
---|---|---|
https:///example.com | yes | yes |
gopher:///example.com | no* | yes* |
#local-ref | no* | no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok, your approach is better. If this is how this is rendered, then I'm fine ! (it was just hard to imagine the things since I don't see a visual example).
…oc#12108) A reference like ":ref:`Some other page <some-other-page>`" results in a refuri "#some-other-page". This does not seem useful to readers of the man page. It is especially unhelpful when using a terminal that implements a hint mode for selecting links -- the extra links add noise, making it harder to select the interesting ones. Don't emit OSC 8 for those. Also don't emit it for URLs that might be unsafe (see sphinx-doc#12260 (comment)) I don't know one that would be unsafe but I'm sure there are some. OTOH, "man_show_urls" doesn't seem unsafe because it shows the exact URL that will be opened, so enable that for all URLs (except for relative ones). Follow up to sphinx-doc#12108 I also confirmed that even with docutils 0.21 we do not need to override depart_reference because we already skip reference nodes.
Fixed a problem with empty URLs and man_show_urls. Give file links like |
829c390
to
32e589e
Compare
A reference like
:ref:`Some other page <some-other-page>`
resultsin a refuri "#some-other-page". This does not seem useful to readers
of the man page. It is especially unhelpful when using a terminals
that implements a hint mode for selecting links -- the extra links
add noise, making it harder to select the interesting ones.
While at it, relax the restriction that "man_show_urls" is limited
to mailto/http/ftp URLs; I don't see why other protocols shouldn't
be allowed.
Follow up to #12108
I also confirmed that even with docutils 0.21 we do not need to
override depart_reference because we already skip reference nodes.