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

rustdoc should escape <> #18419

Closed
tbu- opened this issue Oct 29, 2014 · 13 comments
Closed

rustdoc should escape <> #18419

tbu- opened this issue Oct 29, 2014 · 13 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@tbu-
Copy link
Contributor

tbu- commented Oct 29, 2014

For an example where this is necessary, see the docs for StrInterner: The generated HTML code looks like this:

<p>A StrInterner differs from Interner<String> in that it accepts &amp;str rather than RcStr, resulting in less allocation.</p>

@huonw
Copy link
Member

huonw commented Oct 29, 2014

I don't think this is a good idea, including inline HTML is legitimate. Maybe a rustdoc warning might be good, but just escaping everything entirely seems unfortunate.

@huonw huonw added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 29, 2014
@ftxqxd
Copy link
Contributor

ftxqxd commented Oct 29, 2014

In this sort of case, Interner<String> should be surrounded by backticks (i.e., Interner<String>), thus avoiding the whole problem.

@Thiez
Copy link
Contributor

Thiez commented Oct 29, 2014

@huonw should we really assume Rustdoc will only even support outputting html? It already supports outputting json for other tools, what if such tools do not support html?

@huonw
Copy link
Member

huonw commented Oct 29, 2014

Inline HTML is part of "the" markdown spec (to be specific, the commonmark spec), so as long as rustdoc uses markdown it seems that it should support this. I'm not too fussed either way, but it's how markdown is.

(rustdoc doesn't currently use that spec, but there is interest in switching to it.)

@nwin
Copy link
Contributor

nwin commented Feb 28, 2015

I don’t think that producing arbitrary or invalid html is a good idea. This could easily backfire if one would decide to include some more sophisticated documentation on crates.io or elsewhere. I’m just saying <script>. I’m actually a bit shocked about the commonmark spec…

@huonw
Copy link
Member

huonw commented Feb 28, 2015

Even if rustdoc protected against this specific instance, I wouldn't rely on it being trustworthy to avoid XSS (etc.) in the face of a malicious attacker, meaning a doc hosting service should always serve the rendered docs on a credential-less domain. (e.g. like the github.com vs. github.io split.)

@tbu-
Copy link
Contributor Author

tbu- commented Mar 1, 2015

@huonw It should still be addressed.

@huonw
Copy link
Member

huonw commented Mar 1, 2015

I don't personally think rustdoc should be compulsorily opinionated about this aspect of markdown. There's use-cases for manually writing some HTML to get the docs to render exactly how an author wants (and not just render: to be more accessible via better semantic markup), and if this stops a library's markdown docs being rendered to anything other than HTML it is the fault (and responsibility) of that library's author, not of our tooling (IMO, only way one might fault it would be that rustdoc only supports markdown input at the moment, but that's somewhat orthogonal).

Improvements we could possibly make (off the top of my head) are:

  • implementing some warnings to try to catch cases of accidental/incorrect HTML (like the example in the original issue)
  • providing an option to make rustdoc disallow inline HTML all together
  • having a blacklist/whitelist HTML filtering stage for a user to chose to allow specific tags/functionality of HTML (opt-in; an extension to the previous one)

I guess these may be quite hard (doing it properly would give rustdoc a dependency on e.g. html5ever), although from a glance the markdown library (hoedown) we use does support some options for controlling how HTML is handling, so at least having a way to error/disable inline HTML totally might not be so hard, although I haven't looked into in detail.

I'm willing to be convinced that rustdoc should completely ignore this aspect of the commonmark spec, but, it would take some convincing (more than the objections to it mentioned so far :) )... at the very least, supporting no HTML ever does diverge from many of the most popular installations of markdown, e.g. github and stackexchange both support subsets of inline HTML.

@tbu-
Copy link
Contributor Author

tbu- commented Mar 1, 2015

One more objection would be the backward-compatiblity Rust wants to achieve. Make a sensible restriction for some subsets of HTML (such as <sub>, <sup>, <i>) that can be represented by any reasonable presentation format, then extend later, rather than having an allow-all policy in place that cannot be changed so easily.

@steveklabnik
Copy link
Member

Triage: no change. still unsure if this is a thing we'll actually do, but maybe.

@tbu-
Copy link
Contributor Author

tbu- commented Mar 24, 2016

Today it probably runs into the backward-compatiblity problems described above.

@steveklabnik steveklabnik added T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. and removed T-tools labels May 18, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 22, 2017
@kzys
Copy link
Contributor

kzys commented Sep 1, 2018

Then maybe resolving the issue? I personally prefer to keep Rustdoc CommonMark-compatible.

@GuillaumeGomez
Copy link
Member

Indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants