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

Markdown: whitelist aria-label for curly brackets #4095

Merged
merged 4 commits into from
Mar 2, 2022

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Dec 14, 2021

Fixes #3165. Fixes #4094.

Changelog Entry

Fixed

  • Fixes #3165 and #4094. Allowlist aria-label for links in Markdown and skip unrecognized attributes or invalid curly brackets, by @compulim, in PR #4095

Description

We should whitelist aria-label (as curly brackets) in Markdown and skip unrecognized or invalid form. The aria-label attribute is added by PR #3022.

This will continue to enable aria-label in [Link](https://.../){aria-label="This is a link"}.

But it will skip Hello {1}. As {1} could be used for text-templating (related to #3165).

Design

Curly brackets pattern

I tested few undocumented behavior of markdown-it-attrs:

  • [Link](...){aria-label} or [Link](...){aria-label=} will add aria-label attribute without value (boolean true)
  • [Link](...){aria-label=abc} will add aria-label="abc"
  • [Link](...){aria-label ="abc"} will add aria-label attribute without value (note the space before/after equal sign)
  • [Link](...){ aria-label="abc" } will add aria-label="abc"
  • No escape character is supported when processing the value, i.e. double-quote is (almost) not settable via markdown-it-attrs

As we only allowlist aria-label, to solve the {1} case, I come up with these as the solely supported patterns for curly brackets:

  • {aria-label}
  • {aria-label=ABC}
  • {aria-label="ABC"}

Other patterns will be considered as invalid, such as including unsupported attributes.

Invalid pattern should be skipped by markdown-it-attrs and left untouched.

Controlling what to process (or skip)

markdown-it-attrs will process all curly brackets and remove them from content, even though they are invalid or not specified by the whitelist attributes.

There are very few options we can use to control the behavior markdown-it-attrs. Thus, we used allowlist pattern by using regular expression to convert the curly brackets {} into a rarely-used set of characters ⟬⟭ (white tortoise shell brackets).

Then, we ask markdown-it-attrs to only look at these shell brackets.

This effectively allow us to control what the markdown-it-attrs could see.

Specific Changes

  • Modified renderMarkdown.ts
    • Allowlist aria-label
    • Use tortoise shell brackets to control what markdown-it-attrs could see
  • Added new tests
  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

@compulim compulim marked this pull request as ready for review December 14, 2021 21:39
@compulim compulim added the p1 Painful if we don't fix, won't block releasing label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 Painful if we don't fix, won't block releasing
Projects
None yet
2 participants