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

security: anchor tag safety #4789

Merged
merged 10 commits into from
Aug 4, 2018
Merged

security: anchor tag safety #4789

merged 10 commits into from
Aug 4, 2018

Conversation

shockey
Copy link
Contributor

@shockey shockey commented Aug 4, 2018

Description

Setting target="_blank" on anchor tags is unsafe unless used in conjunction with rel="noopener".

This PR fixes our internal usage, adapts our Markdown parser to protect against from vulnerable user input, and adds a linter rule to prevent future introductions of this vulnerability into Swagger UI.

Motivation and Context

Fixes an issue reported through our disclosure channel (email, [email protected]).

How Has This Been Tested?

XSS tests have been extended to cover these changes.

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@shockey shockey merged commit dd3afdc into master Aug 4, 2018
@shockey shockey deleted the security/anchor-target-safety branch October 8, 2018 21:42
@ganeshgore
Copy link

Does anchor tag safety also means ignoring hrefs with non standard port.
I am trying to add anchor tag with non standard protocol like (vscode://filename) in description part of swagger document. It seems to ignore that . Is it possible to add such URLs?

You can quick check by adding
[TestLink](vscode://file/c:/temp.txt)
in description part of https://editor.swagger.io/ example. It removes href while rendering.

@shockey
Copy link
Contributor Author

shockey commented Nov 26, 2018

@ganeshgore it does, currently we only allow http/https. please open a feature request if you'd like to have support for vscode:// urls 😄

bajtos added a commit to bajtos/swagger-ui that referenced this pull request Jun 21, 2019
Setting target="_blank" on anchor tags is unsafe unless used in
conjunction with rel="noopener".

This is a back-port of dd3afdc (swagger-api#4789) to fix the problem in
version 2.x.

Signed-off-by: Miroslav Bajtoš <[email protected]>
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.

2 participants