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 documentation rendering issues #2264

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Aug 12, 2024

Replace < and > in documentation comments with &lt and &gt so
that they won't be interpreted as HTML tags and will render as expected.

In one place where we show Dart code with angle bracket I've replaced
the wrapping [ and ] with backticks.

Fixes CI.

@mkustermann
Copy link
Member

... that they won't be interpreted as HTML tags and will render as expected.

I would assume that api docs would html-escape any text it's trying to render and not just blindly take the text and paste it in a <div> (or other) element. The comments are not HTML markup.

@osa1
Copy link
Member Author

osa1 commented Aug 12, 2024

I think it's Markdown semantics that tags are copied directly. You can try it with an online Markdown renderer, e.g. try entering <foo>test</foo> in https://markdownlivepreview.com/ or https://stackedit.io/app#.

I'm not an expert, but there aren't a fixed set of tags that are valid in HTML, see e.g. https://stackoverflow.com/questions/2802687/is-there-a-way-to-create-your-own-html-tag-in-html5.

@mkustermann
Copy link
Member

If it's supposed to work like that, lgtm (I can see a single place in package:flutter docs where they also do this)

I think it's Markdown semantics that tags are copied directly.

At least <script>...</script> tags don't seem to be copied (which would be pretty bad - imagine malicious users put <script ....> links into dart comments, publish package to pub and then other users go to api docs and suddenly 3rd party JavaScript runs when viewing API docs)

/cc @srawlins

@osa1
Copy link
Member Author

osa1 commented Aug 12, 2024

AFAIU HTML sanitization is left to the user, see https://pub.dev/documentation/markdown/latest (search for "HTML sanitization").

This code just runs the script when you navigate to the documentation page:

/// Test <script>alert('hi');</script> test.
int calculate() {
  return 6 * 7;
}

In elsewhere where you get Markdown input from the user you would have to check the tags yourself.

@osa1 osa1 merged commit 6bfe0d6 into dart-lang:master Aug 12, 2024
37 checks passed
@osa1 osa1 deleted the fix_doc_issues branch August 12, 2024 09:19
@mkustermann
Copy link
Member

This code just runs the script when you navigate to the documentation page:

/// Test <script>alert('hi');</script> test.
int calculate() {
  return 6 * 7;
}

/cc @jonasfj Do we allow 3rd party JavaScript to run on the API docs we host? Is this expected?

@jonasfj
Copy link
Member

jonasfj commented Aug 12, 2024

On pub.dev we run rendered markdown through:
https://pub.dev/packages/sanitize_html

In dartdoc there is a --sanitize-html option, which triggers:
https://github.com/dart-lang/dartdoc/blob/ce098154b16255bbc9ddfa89e3f6141262645513/lib/src/render/documentation_renderer.dart#L94

that's effectively an embedded fork of package:sanitize_html, which is then used to sanitize HTML tags.


In summary:

  • <script>alert('hi');</script> will not be embedded in the HTML pages served on pub.dev.
    • Non-allowlisted tags are stripped through HTML sanitization.
  • Should disallowed tags make it through HTML sanitization somehow, we have CSP headers in place which will:
    • Prevent execution of embedded <script> and <style> blocks, and,
    • Specify an allowlist of the domains from which external JS and CSS can be loaded.

On topic:

  • Yes, markdown allows embedded HTML tags (including <script>).
  • Documentation authors should avoid accidental creation of HTML tags, by:
    • Using &lt; and &gt; instead of < and > (respectively); or;

    • Use backticks around inline code snippets:

      /// Test `<script>alert('hi');</script>` test.
    • Use a code block

      ```html
      <script>alert('hi');</script>
      ```
      

Obviously, in this case, just use backticks around the inline code snippet.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 13, 2024
Revisions updated by `dart tools/rev_sdk_deps.dart`.

ecosystem (https://github.com/dart-lang/ecosystem/compare/f977423..2719d0c):
  2719d0c  2024-08-08  Devon Carew  add invalid_runtime_check_with_js_interop_types, unintended_html_in_doc_comment (dart-lang/ecosystem#285)

http (https://github.com/dart-lang/http/compare/73fce77..76512c4):
  76512c4  2024-08-07  Kate  test(http_client_conformance_tests): Remove old skips (dart-lang/http#1284)
  d7ae256  2024-08-08  Anikate De  [docs] sort pkg list in ascending order (dart-lang/http#1287)
  b82d88c  2024-08-06  Anikate De  [docs] Add ok_http entry to readme (dart-lang/http#1285)

package_config (https://github.com/dart-lang/package_config/compare/f0b7256..76934c2):
  76934c2  2024-08-06  Kevin Moore  Latest lints, require Dart 3.4 (dart-lang/package_config#157)

sync_http (https://github.com/dart-lang/sync_http/compare/ab8377e..91c0dd5):
  91c0dd5  2024-08-12  dependabot[bot]  Bump actions/checkout from 4.1.6 to 4.1.7 (google/sync_http.dart#49)

test (https://github.com/dart-lang/test/compare/9fbbfdb..8be3c94):
  8be3c948  2024-08-12  Ömer Sinan Ağacan  Run dart2wasm integration test on Windows (dart-lang/test#2265)
  e656e5a9  2024-08-12  Yaroslav Vorobev  fix: use `toFilePath` in package config Uri (dart-lang/test#2262)
  6bfe0d62  2024-08-12  Ömer Sinan Ağacan  Fix documentation rendering issues (dart-lang/test#2264)

tools (https://github.com/dart-lang/tools/compare/55dbd6e..d563c38):
  d563c38  2024-08-13  Moritz  Add health workflow (dart-lang/tools#292)
  8ac5509  2024-08-12  Devon Carew  Update CODEOWNERS for package:unified_analytics (dart-lang/tools#289)

web (https://github.com/dart-lang/web/compare/e89fe49..4996dc2):
  4996dc2  2024-08-12  Srujan Gaddam  Ignore unintended_html_in_doc_comment (dart-lang/web#278)

Change-Id: I808778af5fb9a1f6885ae847614ffb660fcb8662
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/380204
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Devon Carew <devoncarew@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.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.

3 participants