feat: html blocks! #875
feat: html blocks! #875
Conversation
| it('runs user scripts in compat mode', () => { | ||
| render(<HTMLBlock runScripts={true}>{`<script>mockFn()</script>`}</HTMLBlock>); | ||
| expect(global.mockFn).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("doesn't run user scripts by default", () => { | ||
| render(<HTMLBlock>{`<script>mockFn()</script>`}</HTMLBlock>); | ||
| expect(global.mockFn).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| it("doesn't render user scripts by default", () => { | ||
| render(<HTMLBlock>{`<script>mockFn()</script>`}</HTMLBlock>); | ||
| expect(screen.queryByText('mockFn()')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("doesn't render user scripts with weird endings", () => { | ||
| render(<HTMLBlock>{`<script>mockFn()</script foo='bar'>`}</HTMLBlock>); | ||
| expect(screen.queryByText('mockFn()')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("doesn't render user scripts with a malicious string", () => { | ||
| render(<HTMLBlock>{`<scrip<script></script>t>mockFn()</s<script></script>cript>`}</HTMLBlock>); | ||
| expect(screen.queryByText('mockFn()')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("doesn't run scripts on the server (even in compat mode)", () => { | ||
| const html = ` | ||
| <h1>Hello World</h1> | ||
| <script>mockFn()</script> | ||
| `; | ||
| const elem = <HTMLBlock runScripts={true}>{html}</HTMLBlock>; | ||
| const view = renderToString(elem); | ||
| expect(elem.props.runScripts).toBe(true); | ||
| expect(view.indexOf('<script>')).toBeLessThan(0); | ||
| expect(view.indexOf('<h1>')).toBeGreaterThanOrEqual(0); | ||
| }); | ||
|
|
||
| it('renders the html in a `<pre>` tag if safeMode={true}', async () => { | ||
| const md = '<HTMLBlock safeMode={true}>{`<button onload="alert(\'gotcha!\')"/>`}</HTMLBlock>'; | ||
| const code = compile(md); | ||
| const Component = await run(code); | ||
| expect(renderToStaticMarkup(<Component />)).toMatchInlineSnapshot( | ||
| '"<pre class="html-unsafe"><code><button onload="alert('gotcha!')"/></code></pre>"', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I think we can scrap all these safety features now that all of the doc is executable?
There was a problem hiding this comment.
lol honestly all these tests helped me dial the component in. i didn't realize i had swapped the "cleaned" vs "dirty" html until i got to the veeeery last test
There was a problem hiding this comment.
Keep whatever tests, but I think we can wholesale remove safeMode and runScripts
There was a problem hiding this comment.
i was actually going to ask you about that -- runScripts is a component prop, but safeMode and lazyImages are both processor options/settings for the user. we still need to support these in a similar way, yeah? i tried making the components basically HOC and passing args that way, but useMDXComponents was Not Having It
There was a problem hiding this comment.
Instead of HOC's we should probably create a context for the options?
There was a problem hiding this comment.
Really disappointed in myself that I didn't do that 2 years ago.
| scripts.push(match[1]); | ||
| } | ||
| const cleaned = html.replace(MATCH_SCRIPT_TAGS, ''); | ||
| return [cleaned, () => scripts.map(js => window.eval(js))]; |
There was a problem hiding this comment.
Like, there's no reason to strip script tags if the whole thing is getting executed?
There was a problem hiding this comment.
it only gets executed if you pass runScripts=true? we should probably leave it to the user to decide whether or not they wanna get h4x0rd
There was a problem hiding this comment.
HTML blocks were originally a way to say, hey this code is dangerous and we know it. All other 'inlined' html would get tags and attributes sanitized. But I don't see how we could even begin to sanitize MDX.
The only thing I can think of, is if some existing page with an html block has an unknown malicious script in it, and all of sudden we gave it life?
| if (typeof window !== 'undefined' && typeof runScripts === 'boolean' && runScripts) exec(); | ||
| }, [runScripts, exec]); | ||
|
|
||
| if (safeMode) { |
There was a problem hiding this comment.
safeMode was my silly attempt to make suggested edits safer. Suggested edits could be submitted by anonymous users. And it features a preview mode, so if the submitter wasn't an admin, we wouldn't render the HTML. 🤷🏼
There was a problem hiding this comment.
that was the usage for us sure, but i'm wondering about the 1000 or downloads a week of @readme/markdown? is this new improved renderer going to be internal only or are we improving on the current one?
There was a problem hiding this comment.
okay, we can figure out the final details in another PR since it's a bit out of scope for this one 😅
|
This PR was released!🚀 Changes included in v6.75.0-beta.31 |
🧰 Changes
HTML for everyone!
🧬 QA & Testing