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

feat(utils): markdown callouts #2298

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
7 changes: 7 additions & 0 deletions packages/assets/icons/message-square-warning.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 8 additions & 0 deletions packages/assets/icons/octogon-alert.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 7 additions & 0 deletions packages/assets/icons/triangle-alert.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 6 additions & 0 deletions packages/assets/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@
import _XCircleIcon from './icons/x-circle.svg?component'
import _ZoomInIcon from './icons/zoom-in.svg?component'
import _ZoomOutIcon from './icons/zoom-out.svg?component'
import _MessageSquareWarning from "./icons/message-square-warning.svg?component"

Check failure on line 163 in packages/assets/index.ts

View workflow job for this annotation

GitHub Actions / Build, Test, and Lint

Replace `"./icons/message-square-warning.svg?component"` with `'./icons/message-square-warning.svg?component'`
import _OctogonAlert from "./icons/octogon-alert.svg?component"

Check failure on line 164 in packages/assets/index.ts

View workflow job for this annotation

GitHub Actions / Build, Test, and Lint

Replace `"./icons/octogon-alert.svg?component"` with `'./icons/octogon-alert.svg?component'`
import _TriangleAlert from "./icons/triangle-alert.svg?component"

Check failure on line 165 in packages/assets/index.ts

View workflow job for this annotation

GitHub Actions / Build, Test, and Lint

Replace `"./icons/triangle-alert.svg?component"` with `'./icons/triangle-alert.svg?component'`

// Editor Icons
import _BoldIcon from './icons/bold.svg?component'
Expand Down Expand Up @@ -341,3 +344,6 @@
export const Heading3Icon = _Heading3Icon
export const CardIcon = _CardIcon
export const SparklesIcon = _SparklesIcon
export const MessageSquareWarningIcon = _MessageSquareWarning
export const OctogonAlertIcon = _OctogonAlert
export const TriangleAlertIcon = _TriangleAlert
93 changes: 93 additions & 0 deletions packages/assets/styles/classes.scss
Original file line number Diff line number Diff line change
Expand Up @@ -1182,3 +1182,96 @@ 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;
position: relative;

&::after {
position: absolute;
top: 0;
left: 0;
bottom: 0;
content: '';
width: 0.25em;
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);
}
}
}
2 changes: 2 additions & 0 deletions packages/utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
"@codemirror/state": "^6.3.2",
"@codemirror/view": "^6.22.1",
"@types/markdown-it": "^14.1.1",
"@modrinth/assets": "workspace:*",
"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"
}
}
53 changes: 53 additions & 0 deletions packages/utils/parse.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
import {
InfoIcon,
LightBulbIcon,
MessageSquareWarningIcon,
OctogonAlertIcon,
TriangleAlertIcon,
} from '@modrinth/assets'
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 +32,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

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 +96,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 (
Expand Down Expand Up @@ -133,6 +176,16 @@ export const md = (options = {}) => {
...options,
})

md.use(MarkdownItGitHubAlerts, {
icons: {
note: InfoIcon,
tip: LightBulbIcon,
important: MessageSquareWarningIcon,
warning: TriangleAlertIcon,
caution: OctogonAlertIcon,
},
})

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

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

Loading