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

Markdown fixes #1632

Closed
wants to merge 2 commits into from
Closed

Conversation

acappiello
Copy link

This fixes the issues with markdown that I detailed in #1622. This should also close #1332 and #1491.

The first is a fix for markdown code blocks that contain xml or html. Inserting the raw markdown into the DOM treats this xml/html as part of the document, leading to problems. My fix is to let handlebars escape the markdown and then unescape it when calling marked. Using html outside of code blocks still works as intended.

The second is to only call marked within the specified dom_id. This is important when loading multiple SwaggerUI instances on the same page, as the current code will call marked multiple times on the same content.

@gmanfunky
Copy link

This is mostly better than the PR i sent at #1633
to fix some XSS vulnerability cases.

But i notice it also an incomplete fix as it doesn't fix including untrusted data via {{{signature}}}.

@acappiello
Copy link
Author

I'm not sure that I'm actually addressing your XSS concerns, because this fix requires the content being passed to marked to be unescaped first.

I wasn't directly considering XSS here, but it's certainly a related issue. As a result, I only changed handlebars expressions inside markdown classes, since at the time I wasn't sure whether things like {{{signature}}} were that way for a reason.

@webron
Copy link
Contributor

webron commented Jun 8, 2017

As our focus is on 3.0, this is no longer needed. Thanks for taking the time to submit this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In fenced blocks of text in descriptions, XML tag names are forced to lowercase
3 participants