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

Fixes swagger-editor/#1502. #3712

Merged
merged 3 commits into from Sep 29, 2017
Merged

Fixes swagger-editor/#1502. #3712

merged 3 commits into from Sep 29, 2017

Conversation

owenconti
Copy link
Contributor

@owenconti owenconti commented Sep 28, 2017

Fixes swagger-api/swagger-editor#1502.
Fixes #3385.
Fixes #2700.

Change logic for markdown rendering to:

  1. Convert source markdown to HTML
  2. Sanitize HTML
  3. Send sanitized HTML to markdown renderer

Works with the following test specs:

https://gist.github.com/tbarsballe/bb135cd6f65b1341bd4184f003ca8f4d
https://gist.github.com/shockey/e65144c103f4cf3cd501d2c2d1c63b18
https://gist.github.com/shockey/7346f22b0d31b39ae65b2b5fe39a8f51

Change logic for markdown rendering to:

1. Convert source markdown to HTML
2. Sanitize HTML
3. Send sanitized HTML to markdown renderer
@owenconti
Copy link
Contributor Author

I'm going to add various XSS cases to the existing sanitization tests.

Copy link
Contributor

@shockey shockey left a comment

Choose a reason for hiding this comment

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

This looks good! I had one question, left it as a comment.

I'll hold off on approval/merge until you get around to adding tests.

@@ -72,6 +73,7 @@
"redux": "^3.x.x",
"redux-immutable": "3.0.8",
"redux-logger": "*",
"remarkable": "^1.7.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're using Remarkable directly, do we need react-remarkable anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I think we can remove it. I also want to try removing ReactMarkdown from the OAS3 component and replace it with dangerouslySetInnerHTML like in the 2.0 component.

@shockey
Copy link
Contributor

shockey commented Sep 29, 2017

Git merge = 💥

@shockey
Copy link
Contributor

shockey commented Sep 29, 2017

Okay, got it working again 😄

@shockey
Copy link
Contributor

shockey commented Sep 29, 2017

This fixes #3385 as well.

Copy link
Contributor

@shockey shockey left a comment

Choose a reason for hiding this comment

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

Looks good - I confirmed that the XSS test definitions didn't trigger anything.

@shockey shockey mentioned this pull request Sep 29, 2017
@shockey shockey merged commit 356dc81 into swagger-api:master Sep 29, 2017
@shockey
Copy link
Contributor

shockey commented Sep 29, 2017

Merged so we can release this fix.

I've opened a new issue for the tests that you mentioned, @owenconti.

@ettaramesh
Copy link

Its been mentioned that #3385 is being fixed as part of this issue. Could you please let me know which version of swagger-ui has this fix available. I have tested with 3.33.0 and the issue still exists.

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