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.

Sign up for GitHub

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

Jump to bottom

Notice field removes target="_blank" attribute in <a> tags #11477

Closed
3 tasks done
Pachat opened this issue Feb 7, 2022 · 3 comments · Fixed by #11787
Closed
3 tasks done

Notice field removes target="_blank" attribute in <a> tags #11477

Pachat opened this issue Feb 7, 2022 · 3 comments · Fixed by #11787
Labels
🍰 Improvement

Comments

@Pachat
Copy link

Pachat commented Feb 7, 2022

Preflight Checklist

Describe the Bug

Is there a reason not to render target="_blank"

To Reproduce

  1. In a Notice field, set <a href="my.pdf" target="_blank">Please click here</a>
  2. Save and show the item. The console shows: <a href="my.pdf">Please click here</a>

This is strange as the recorded content is <a href=\"my.pdf\" target=\"_blank\">Please click here</a>

Errors Shown

Does not open in a new tab.

What version of Directus are you using?

9.5.1

What version of Node.js are you using?

14.17.1

What database are you using?

SQLite version 3.37.0

What browser are you using?

Firefox

What operating system are you using?

Windows_NT 10.0.19043

How are you deploying Directus?

locally
@azrikahar
Copy link
Contributor

azrikahar commented Feb 7, 2022 •
edited

This seems to be because notice interface passes the content via v-md directive:

directus/app/src/interfaces/presentation-notice/presentation-notice.vue

Line 4 in b8c4317

<div v-md="text" />

And the target attribute is still intact after processed by marked. However dompurify's sanitize function strips it out over here:

directus/app/src/utils/md.ts

Lines 1 to 9 in b8c4317

import { marked } from 'marked';
import dompurify from 'dompurify';
/**
* Render and sanitize a markdown string
*/
export function md(str: string): string {
return dompurify.sanitize(marked(str));
}

This is also a known issue in dompurify: cure53/DOMPurify#317

As seen in the follow up comments in that thread, this can be resolved with dompurify's hooks, but this comment also does mention the security concern with this as well: cure53/DOMPurify#317 (comment). @rijkvanzanten thoughts on this?

@azrikahar azrikahar added the App label Feb 7, 2022
@azrikahar azrikahar changed the title Notice accepts <a href="">...</a> but removes target="_blank" attribute Notice field removes target="_blank" attribute in <a> tags Feb 7, 2022
@Pachat
Copy link
Author

Pachat commented Feb 7, 2022 •
edited

About the security concern, the Links to cross-origin destinations are unsafe gives the solution:
Adding rel="noopener" or rel="noreferrer" to your target="_blank" links avoids these issues.

And this is exactly what Directus already does when we add a new external URL to the modules menu:
<a class="button x-large align-center icon tile normal" type="button" disabled="false" href="https://directus.io/" target="_blank" rel="noopener noreferrer">
which is fine there.

Unfortunately in a Notice, after sanitization, target="_blank" is removed and only rel="noopener noreferrer" is kept.

@azrikahar
Copy link
Contributor

Thanks for the elaboration. Technically the target and rel attribute for external URL added to modules menu are "manually" added in the code as seen here, which is why it's fine:

directus/app/src/components/v-button/v-button.vue

Lines 28 to 29 in 748ee97

:target="component === 'a' ? '_blank' : undefined"
:rel="component === 'a' ? 'noopener noreferrer' : undefined"

but yea in the Notice interface case, dompurify ended up being the one sanitizing/removing target by default but not rel as seen in the existing issue in reported over there.

@azrikahar azrikahar mentioned this issue Feb 22, 2022
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
🍰 Improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow target="_blank" in markdown sanitization directus/directus
2 participants
@Pachat @azrikahar