Display test context code without rendering HTML

[irvinlim]

Is it possible for the test context content to display without rendering as HTML? I noticed that in code-snippet.jsx, the CodeSnippet component uses dangerouslySetInnerHTML.

Is there a reason for this? I am displaying HTML code in the test context and I believe it shouldn't be rendering (since it's a CodeSnippet after all). Plus, this might open up XSS risks (I'm personally using this to generate dynamic test reports).

I would be happy to create a PR to fix this if neeeded.

[adamgruber]

Can you show me an example of the context you are passing, how it is rendering and what you expect the output to be? Also I wouldn't think there's a ton of risk with XSS here since the reports are static files. Do you have an example of how you think the report could be used for XSS attacks?

[irvinlim]

I'm actually passing in a raw HTTP response body in the context. This would contain HTTP headers as well as the actual HTML content, so I was kinda surprised that the HTML was actually rendering.

This wouldn't be a problem, except when you have intentionally malformed HTML tags, which might cut off the rest of the content, or if you intended to have HTML tags appear as code in your context.

This is an example of how I would expect it to render (except the syntax highlighting, but that's another issue):

Instead, this is what I'm getting:

Notice the "malformed" script tag in the HTTP request context (actually URL encoded), which caused the rest of the contents of the context <div> to be hidden away. Also, I would wanted to have seen the HTML source in the HTTP response context, instead of the rendering of the HTML.

I am actually using Mocha to write XSS test cases, which might explain how I find that doing dangerouslySetInnerHTML might make this report generator vulnerable to XSS. What I'm doing might be a bit of an obscure use case, but all the same I believe that you don't need to use dangerouslySetInnerHTML in the first place if you only wished to append some HTML to the CodeSnippet.

Hence, I just wanted to check what was your rationale for using dangerouslySetInnerHTML in the first place. If you are agreeable, I will submit a PR to fix this to remove the use of this property.

---

I think that a further enhancement to contexts in general would be to allow a third, explicit context type parameter, rather than guessing whether it is meant to be code or meant to be a string. It's a little ugly for sentences to have way-off syntax highlighting applied to it, and could be better displayed with <p> rather than <code>.

Furthermore, we can bring it one step further to allow the user to pass in the exact syntax highlighter to use (like how GitHub-flavoured Markdown allows you to set the language of a code block), though it's highly likely most users will only require JS syntax highlighting.

---

For completion's sake, this is the exact HTML that is being rendered in Google Chrome (which probably automatically added a closing </script> tag):

<code>GET http:<span class="hljs-comment">//REDACTED/dvwa/vulnerabilities/xss_r/?name=</span><script><span class="hljs-comment">alErt%281%29&lt;%2FscrIpt&gt; HTTP/1.1</span>
HOST: REDACTED
Connection: keep-alive
Accept: *<span class="hljs-comment">/*

</span></script></code>

[adamgruber]

@irvinlim Thanks for the detailed response! This is a good find and looking back I'm not entirely sure why I decided to use dangerouslySetInnerHTML. I did some quick testing and I think it would be safe to remove that. If you want to work up a PR to address this that'd be great. Just be sure to add proper test cases.

Regarding the context enhancements, I think this need a little more thought. While I agree that the current implementation is a bit limited, I hesitate to start adding options. I wanted the API for adding context to be very simple and straightforward and to just work without configuration required.

I agree it would be nice to have better syntax highlighting but this would come at a cost. Each additional language increases the overall bundle size. Plus, where do we draw the line on what languages are included? Currently the report includes javascript and diff. In your example it would be useful to add xml/html. Others may have a use for something different.

[irvinlim]

Great! I will be submitting a PR to remove the use of dangerouslySetInnerHTML in this particular location. Should I run npm run dist before submitting the PR, or do I leave it to you, since the transpiled/webpacked files are checked in into Git?

Agreed with your views on syntax highlighting. It might help if there was a way for me to disable syntax highlighting for a particular context, rather than enable syntax highlighting for additional languages when requested. This way, it would solve both problems of both HTML and plaintext from being highlighted incorrectly.

I agree there isn't a simple way about this, we can leave that for a later time to solve.

[adamgruber]

Don't worry about creating the dist files, I'll take care of that. At some point I'll update the repo so those aren't checked in and instead are created on publish.

---

For syntax highlighting, addContext could take an optional third argument with options. For example:

addContext(this, 'something', { highlight: false };

This would probably be the simplest case and allow for future options like an explicit syntax option. But let's leave this for a separate PR.

[irvinlim]

Alright! Just submitted a PR for the dangerouslySetInnerHTML issue.

If you are keen, I could implement your suggestion for the options argument sometime next week.

I'll look forward to seeing the changes published on NPM since I'll be using this at work!