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

Back-port "fix: anchor tag safety" to 2.x #5419

Closed
wants to merge 1 commit into from

Conversation

bajtos
Copy link

@bajtos bajtos commented 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 (#4789) to fix the problem in version 2.x.

@shockey @JonathanParrilla PTAL

Description

I ran grep -r target src/main/template to find all places where we are creating links to external URLs and then fixed these places by adding rel="noopener noreferrer" attribute.

Motivation and Context

In LoopBack, we are depending on swagger-ui version 2.x (see https:/strongloop/loopback-component-explorer). We tried to upgrade to 3.x, but found that such upgrade requires too much effort, and thus decided to stick with 2.x.

Now we are seeing a security vulnerability reported for our module. I believe we are not affected by the vulnerability, because the possibly-malicious URLs are fully under control of the person running swagger-ui, but it still look bad that security checkers are reporting a vulnerability.

Can we back-port the fix from 3.x to 2.x please?

How Has This Been Tested?

I tried to run npm test, unfortunately it failed with a cryptic error:

> [email protected] pretest /Users/bajtos/src/swagger-ui
> npm run jshint


> [email protected] prejshint /Users/bajtos/src/swagger-ui
> gulp

fs.js:27
const { Math, Object, Reflect } = primordials;
                                  ^

ReferenceError: primordials is not defined
    at fs.js:27:35
    at req_ (/Users/bajtos/src/swagger-ui/node_modules/natives/index.js:143:24)
    at Object.req [as require] (/Users/bajtos/src/swagger-ui/node_modules/natives/index.js:55:10)
    at Object.<anonymous> (/Users/bajtos/src/swagger-ui/node_modules/vinyl-fs/node_modules/graceful-fs/fs.js:1:37)
    at Module._compile (internal/modules/cjs/loader.js:759:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
    at Module.load (internal/modules/cjs/loader.js:628:32)
    at Function.Module._load (internal/modules/cjs/loader.js:555:12)
    at Module.require (internal/modules/cjs/loader.js:666:19)
    at require (internal/modules/cjs/helpers.js:16:16)

I also try to run mocha directly, unfortunately that failed too in the test should have "Swagger UI" in title with no diagnostic messages :(

Could you please advise how can/should I test this change?

Screenshots (if appropriate):

n/a

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.

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]>
Copy link

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

I am not familiar with this codebase, but upon reading https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/, LGTM 👍

@shockey
Copy link
Contributor

shockey commented Jun 21, 2019

@bajtos, just acknowledging that I've seen this -- I've kicked off an internal discussion.

@bajtos
Copy link
Author

bajtos commented Jun 21, 2019

@shockey thanks! ❤️

While you are discussing my pull request internally, could you please discuss back-port of #5190 too? If you are ok to accept back-ports of security fixes from 3.x to 2.x, then I'd like to contribute back-port of #5190 too.

@shockey

This comment has been minimized.

@shockey
Copy link
Contributor

shockey commented Jun 21, 2019

we've decided to pass on this PR, along with any other requests for updates to swagger-ui@2.

the last version of swagger-ui@2 was released 895 days ago, and was deprecated with the release of [email protected], which happened 824 days ago.

the effort required to confidently push a security-related fix for a codebase that:

  • has not been touched in years
  • this team hasn't worked on before
  • needs build tools that don't work in current (i.e., safe) versions of Node
  • may have first-party security issues that have been addressed in more current versions of Swagger UI
  • definitely has a few third-party security issues, because the dependencies are so out of date
  • isn't otherwise supported or maintained (yet merging this would give the illusion of some level of ongoing security)

..outweighs the benefit of pushing out a fix.

for your part @bajtos, I understand the predicament you're in -- I see that LoopBack has a legacy/LTS version that is pretty well stuck with swagger-ui@2 due to heavy customization and integration.

since there's no easy out for you until your current version goes to EOL, feel free to fork our 2.x branch and create a forked module that includes these changes. I've had to do this with some of Swagger UI's dependencies for various reasons.

lastly, thanks for the PR. I do want to merge it, but I don't think it's the right thing to do given that it opens a Pandora's box of expectations for 2.x security updates going forward.

@shockey shockey closed this Jun 21, 2019
@bajtos
Copy link
Author

bajtos commented Jun 24, 2019

@shockey thank you for a detailed explanation, I appreciate you took the time to discuss this matter in depth and write down your reasoning. I understand your situation. The decision you made makes sense, even if the consequences are not convenient to me :)

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.

3 participants