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

feat(utils): markdown callouts #2298

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

feat(utils): markdown callouts #2298

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
709ed41
feat(utils): markdown callouts
Erb3 Aug 25, 2024
fa3e22b
fix(utils): add back xss class filtering for p and svg
Erb3 Aug 25, 2024
088920e
Merge branch 'main' into github-style-markdown-callouts
Erb3 Aug 26, 2024
3039f06
fix(web): lucide icons
Erb3 Aug 27, 2024
0cc96c9
Merge branch 'main' into github-style-markdown-callouts
Erb3 Aug 27, 2024
e22e90d
chore: attempt to fix conflict
Erb3 Aug 28, 2024
66c7af1
Merge branch 'main' into github-style-markdown-callouts
Erb3 Aug 28, 2024
c45da4a
refactor(web): markdown alerts scss
Erb3 Aug 28, 2024
552dce6
refactor(web): use pseudo-elements for markdown callouts
Erb3 Aug 28, 2024
15f0b9f
refactor(web): position callouts correctly
Erb3 Aug 28, 2024
389bc2a
Merge branch 'main' into github-style-markdown-callouts
Erb3 Aug 28, 2024
38d6de4
refactor: add callout icons to assets
Erb3 Sep 15, 2024
1072b8d
Merge branch 'main' into github-style-markdown-callouts
Erb3 Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions packages/assets/styles/classes.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1183,3 +1183,97 @@ select {
border-top-left-radius: var(--radius-md) !important;
border-top-right-radius: var(--radius-md) !important;
}

// Callouts

.markdown-alert {
padding: 0.5rem 1rem;
margin-bottom: 16px;
color: inherit;

&::after {
position: absolute;
display: block;
content: '';
background-color: red;
width: 0.25em;
height: 15%;
Erb3 marked this conversation as resolved.
Show resolved Hide resolved
left: 1.5rem;
transform: translateY(-90%);
border-radius: var(--radius-xl);
}

:first-child {
margin-top: 0;
}

:last-child {
margin-bottom: 0;
}

& .markdown-alert-title {
display: flex;
font-weight: 500;
align-items: center;
line-height: 1;
}

& .markdown-alert-title > svg {
margin-right: 0.5rem;
display: inline-block;
overflow: visible !important;
vertical-align: text-bottom;
height: 1.2em;
width: 1.2em;
}

&.markdown-alert-note {
& .markdown-alert-title {
color: var(--color-blue);
}

&::after {
background-color: var(--color-blue);
}
}

&.markdown-alert-tip {
& .markdown-alert-title {
color: var(--color-green);
}

&::after {
background-color: var(--color-green);
}
}

&.markdown-alert-important {
& .markdown-alert-title {
color: var(--color-purple);
}

&::after {
background-color: var(--color-purple);
}
}

&.markdown-alert-warning {
& .markdown-alert-title {
color: var(--color-orange);
}

&::after {
background-color: var(--color-orange);
}
}

&.markdown-alert-caution {
& .markdown-alert-title {
color: var(--color-red);
}

&::after {
background-color: var(--color-red);
}
}
}
1 change: 1 addition & 0 deletions packages/utils/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"dayjs": "^1.11.10",
"highlight.js": "^11.9.0",
"markdown-it": "^14.1.0",
"markdown-it-github-alerts": "^0.3.0",
"xss": "^1.0.14"
}
}
49 changes: 49 additions & 0 deletions packages/utils/parse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import MarkdownIt from 'markdown-it'
import MarkdownItGitHubAlerts from 'markdown-it-github-alerts'
import { escapeAttrValue, FilterXSS, safeAttrValue, whiteList } from 'xss'

export const configuredXss = new FilterXSS({
Expand All @@ -24,6 +25,19 @@ export const configuredXss = new FilterXSS({
source: ['media', 'sizes', 'src', 'srcset', 'type'],
p: [...(whiteList.p || []), 'align'],
div: [...(whiteList.p || []), 'align'],
svg: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has me a little concerned as I know there's a bunch of random things you can do with svgs. it doesn't necessarily look like this allows anything bad but I don't know if I am knowledgable enough about svg vulnerabilities to approve this confidently.

it also seems to me like the elements added as part of the markdown renderer should not be subject to the xss/whitelisted elements as the raw html input. I wonder if there is a different way to do this?

Copy link
Contributor Author

@Erb3 Erb3 Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also a concern for me, but I frankly found no other way to do it. From my testing the jsxss was executed based on the output from markdown-it, not the description itself. We can probably change it to be pre-html, but that sounds insecure.

'aria-hidden',
'width',
'height',
'viewBox',
'fill',
'stroke',
'stroke-width',
'stroke-linecap',
'stroke-linejoin',
],
path: ['d'],
circle: ['cx', 'cy', 'r'],
},
css: {
whiteList: {
Expand Down Expand Up @@ -75,6 +89,28 @@ export const configuredXss = new FilterXSS({
}
return `${name}="${escapeAttrValue(allowedClasses.join(' '))}"`
}

// For markdown callouts
if (name === 'class' && ['div', 'p'].includes(tag)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again it seems weird that this is being whitelisted within the input, as someone could put these classes on their own html elements and have weird styles happen

const classWhitelist = [
'markdown-alert',
'markdown-alert-note',
'markdown-alert-tip',
'markdown-alert-warning',
'markdown-alert-important',
'markdown-alert-caution',
'markdown-alert-title',
]

const allowed: string[] = []
for (const className of value.split(/\s/g)) {
if (classWhitelist.includes(className)) {
allowed.push(className)
}
}

return `${name}="${escapeAttrValue(allowed.join(' '))}"`
}
},
safeAttrValue(tag, name, value, cssFilter) {
if (tag === 'img' && name === 'src' && !value.startsWith('data:')) {
Expand Down Expand Up @@ -129,6 +165,19 @@ export const md = (options = {}) => {
...options,
})

md.use(MarkdownItGitHubAlerts, {
icons: {
note: '<svg xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-info"><circle cx="12" cy="12" r="10"/><path d="M12 16v-4"/><path d="M12 8h.01"/></svg>',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be possible to import the relevant icons from @modrinth/assets instead of hardcoding them here? A couple of them might need to be added to the library though

Copy link
Contributor Author

@Erb3 Erb3 Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added them to the modrinth/assets locally, however how would I go about using these? Markdown-it-github-alerts requires a string, can't take a component afaik. I guess I could import and export it as a string in assets, but it feels a bit jank

tip: '<svg xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-lightbulb"><path d="M15 14c.2-1 .7-1.7 1.5-2.5 1-.9 1.5-2.2 1.5-3.5A6 6 0 0 0 6 8c0 1 .2 2.2 1.5 3.5.7.7 1.3 1.5 1.5 2.5"/><path d="M9 18h6"/><path d="M10 22h4"/></svg>',
important:
'<svg xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-message-square-warning"><path d="M21 15a2 2 0 0 1-2 2H7l-4 4V5a2 2 0 0 1 2-2h14a2 2 0 0 1 2 2z"/><path d="M12 7v2"/><path d="M12 13h.01"/></svg>',
warning:
'<svg xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-triangle-alert"><path d="m21.73 18-8-14a2 2 0 0 0-3.48 0l-8 14A2 2 0 0 0 4 21h16a2 2 0 0 0 1.73-3"/><path d="M12 9v4"/><path d="M12 17h.01"/></svg>',
caution:
'<svg xmlns="http://www.w3.org/2000/svg" width="48" height="48" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-octagon-alert"><path d="M12 16h.01"/><path d="M12 8v4"/><path d="M15.312 2a2 2 0 0 1 1.414.586l4.688 4.688A2 2 0 0 1 22 8.688v6.624a2 2 0 0 1-.586 1.414l-4.688 4.688a2 2 0 0 1-1.414.586H8.688a2 2 0 0 1-1.414-.586l-4.688-4.688A2 2 0 0 1 2 15.312V8.688a2 2 0 0 1 .586-1.414l4.688-4.688A2 2 0 0 1 8.688 2z"/></svg>',
},
})

const defaultLinkOpenRenderer =
md.renderer.rules.link_open ||
function (tokens, idx, options, _env, self) {
Expand Down
14 changes: 14 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.