From dd3afdc45656bda2a64ae6a7f9bdad006ea98149 Mon Sep 17 00:00:00 2001 From: kyle Date: Sat, 4 Aug 2018 00:54:03 -0700 Subject: [PATCH] fix: anchor tag safety (via #4789) * v3.17.6 * release(3.17.6): rebuild dist * add failing tests * fix Link component * fix OnlineValidatorBadge component * switch from to in operation components * make Markdown inputs safe * use Link component in Info block, for target safety * add eslint rule for unsafe `target` usage --- .eslintrc | 8 ++ package-lock.json | 43 ++++++- package.json | 2 +- src/core/components/info.jsx | 46 +++++--- src/core/components/layout-utils.jsx | 2 +- .../components/online-validator-badge.jsx | 2 +- src/core/components/operation-tag.jsx | 7 +- src/core/components/operation.jsx | 3 +- src/core/components/providers/markdown.jsx | 18 ++- .../plugins/oas3/wrap-components/markdown.js | 2 + test/components/markdown.js | 2 +- test/xss/anchor-target-rel/info.js | 108 ++++++++++++++++++ test/xss/anchor-target-rel/link.js | 44 +++++++ test/xss/anchor-target-rel/markdown.js | 66 +++++++++++ .../online-validator-badge.js | 32 ++++++ 15 files changed, 354 insertions(+), 31 deletions(-) create mode 100644 test/xss/anchor-target-rel/info.js create mode 100644 test/xss/anchor-target-rel/link.js create mode 100644 test/xss/anchor-target-rel/markdown.js create mode 100644 test/xss/anchor-target-rel/online-validator-badge.js diff --git a/.eslintrc b/.eslintrc index da46603b..d409e0eb 100644 --- a/.eslintrc +++ b/.eslintrc @@ -24,6 +24,13 @@ "import" ], + "settings": { + "react": { + "pragma": "React", + "version": "15.0" + } + }, + "rules": { "semi": [2, "never"], "strict": 0, @@ -37,6 +44,7 @@ "comma-dangle": 0, "no-console": ["error", { allow: ["warn", "error"] }], "react/jsx-no-bind": 1, + "react/jsx-no-target-blank": 2, "react/display-name": 0, "mocha/no-exclusive-tests": "error", "import/no-extraneous-dependencies": [2] diff --git a/package-lock.json b/package-lock.json index 2cc2d90e..be5d9e07 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4612,15 +4612,36 @@ "dev": true }, "eslint-plugin-react": { - "version": "7.7.0", - "resolved": "https://registry.npmjs.org/eslint-plugin-react/-/eslint-plugin-react-7.7.0.tgz", - "integrity": "sha512-KC7Snr4YsWZD5flu6A5c0AcIZidzW3Exbqp7OT67OaD2AppJtlBr/GuPrW/vaQM/yfZotEvKAdrxrO+v8vwYJA==", + "version": "7.10.0", + "resolved": "https://registry.npmjs.org/eslint-plugin-react/-/eslint-plugin-react-7.10.0.tgz", + "integrity": "sha512-18rzWn4AtbSUxFKKM7aCVcj5LXOhOKdwBino3KKWy4psxfPW0YtIbE8WNRDUdyHFL50BeLb6qFd4vpvNYyp7hw==", "dev": true, "requires": { "doctrine": "2.1.0", - "has": "1.0.1", + "has": "1.0.3", "jsx-ast-utils": "2.0.1", - "prop-types": "15.6.1" + "prop-types": "15.6.2" + }, + "dependencies": { + "has": { + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/has/-/has-1.0.3.tgz", + "integrity": "sha512-f2dvO0VU6Oej7RkWJGrehjbzMAjFp5/VKPp5tTpWIV4JHHZK1/BxbFRtf/siA2SWTe09caDmVtYYzWEIbBS4zw==", + "dev": true, + "requires": { + "function-bind": "1.1.1" + } + }, + "prop-types": { + "version": "15.6.2", + "resolved": "https://registry.npmjs.org/prop-types/-/prop-types-15.6.2.tgz", + "integrity": "sha512-3pboPvLiWD7dkI3qf3KbUe6hKFKa52w+AE0VCqECtf+QHAKgOL37tTaNCnuX1nAAQ4ZhyP+kYVKf8rLmJ/feDQ==", + "dev": true, + "requires": { + "loose-envify": "1.3.1", + "object-assign": "4.1.1" + } + } } }, "eslint-plugin-standard": { @@ -17778,6 +17799,18 @@ } } }, + "eslint-plugin-react": { + "version": "7.7.0", + "resolved": "https://registry.npmjs.org/eslint-plugin-react/-/eslint-plugin-react-7.7.0.tgz", + "integrity": "sha512-KC7Snr4YsWZD5flu6A5c0AcIZidzW3Exbqp7OT67OaD2AppJtlBr/GuPrW/vaQM/yfZotEvKAdrxrO+v8vwYJA==", + "dev": true, + "requires": { + "doctrine": "2.1.0", + "has": "1.0.1", + "jsx-ast-utils": "2.0.1", + "prop-types": "15.6.1" + } + }, "globals": { "version": "11.5.0", "resolved": "https://registry.npmjs.org/globals/-/globals-11.5.0.tgz", diff --git a/package.json b/package.json index aabe928b..2fe6b06d 100644 --- a/package.json +++ b/package.json @@ -100,7 +100,7 @@ "eslint": "^4.1.1", "eslint-plugin-import": "^2.13.0", "eslint-plugin-mocha": "^4.11.0", - "eslint-plugin-react": "~7.7.0", + "eslint-plugin-react": "^7.10.0", "expect": "^1.20.2", "extract-text-webpack-plugin": "^3.0.2", "file-loader": "^1.1.11", diff --git a/src/core/components/info.jsx b/src/core/components/info.jsx index 8ff2f05e..676e0e4b 100644 --- a/src/core/components/info.jsx +++ b/src/core/components/info.jsx @@ -25,22 +25,25 @@ export class InfoBasePath extends React.Component { class Contact extends React.Component { static propTypes = { - data: PropTypes.object + data: PropTypes.object, + getComponent: PropTypes.func.isRequired } render(){ - let { data } = this.props + let { data, getComponent } = this.props let name = data.get("name") || "the developer" let url = data.get("url") let email = data.get("email") + const Link = getComponent("Link") + return (
- { url &&
{ name } - Website
} + { url &&
{ name } - Website
} { email && - + { url ? `Send email to ${name}` : `Contact ${name}`} - + }
) @@ -49,18 +52,23 @@ class Contact extends React.Component { class License extends React.Component { static propTypes = { - license: PropTypes.object + license: PropTypes.object, + getComponent: PropTypes.func.isRequired + } render(){ - let { license } = this.props + let { license, getComponent } = this.props + + const Link = getComponent("Link") + let name = license.get("name") || "License" let url = license.get("url") return (
{ - url ? { name } + url ? { name } : { name } }
@@ -70,12 +78,17 @@ class License extends React.Component { export class InfoUrl extends React.PureComponent { static propTypes = { - url: PropTypes.string.isRequired + url: PropTypes.string.isRequired, + getComponent: PropTypes.func.isRequired } + render() { - const { url } = this.props - return { url } + const { url, getComponent } = this.props + + const Link = getComponent("Link") + + return { url } } } @@ -100,6 +113,7 @@ export default class Info extends React.Component { const { url:externalDocsUrl, description:externalDocsDescription } = (externalDocs || fromJS({})).toJS() const Markdown = getComponent("Markdown") + const Link = getComponent("Link") const VersionStamp = getComponent("VersionStamp") const InfoUrl = getComponent("InfoUrl") const InfoBasePath = getComponent("InfoBasePath") @@ -111,7 +125,7 @@ export default class Info extends React.Component { { version && } { host || basePath ? : null } - { url && } + { url && }
@@ -120,14 +134,14 @@ export default class Info extends React.Component { { termsOfService &&
- Terms of service + Terms of service
} - { contact && contact.size ? : null } - { license && license.size ? : null } + {contact && contact.size ? : null } + {license && license.size ? : null } { externalDocsUrl ? - {externalDocsDescription || externalDocsUrl} + {externalDocsDescription || externalDocsUrl} : null }
diff --git a/src/core/components/layout-utils.jsx b/src/core/components/layout-utils.jsx index 9f27bb3d..6dfc4316 100644 --- a/src/core/components/layout-utils.jsx +++ b/src/core/components/layout-utils.jsx @@ -196,7 +196,7 @@ export class Select extends React.Component { export class Link extends React.Component { render() { - return + return } } diff --git a/src/core/components/online-validator-badge.jsx b/src/core/components/online-validator-badge.jsx index 69b6ba6f..8f0f2f14 100644 --- a/src/core/components/online-validator-badge.jsx +++ b/src/core/components/online-validator-badge.jsx @@ -54,7 +54,7 @@ export default class OnlineValidatorBadge extends React.Component { } return ( - + ) diff --git a/src/core/components/operation-tag.jsx b/src/core/components/operation-tag.jsx index 0c4dc8b6..ce45a171 100644 --- a/src/core/components/operation-tag.jsx +++ b/src/core/components/operation-tag.jsx @@ -46,6 +46,7 @@ export default class OperationTag extends React.Component { const Collapse = getComponent("Collapse") const Markdown = getComponent("Markdown") const DeepLink = getComponent("DeepLink") + const Link = getComponent("Link") let tagDescription = tagObj.getIn(["tagDetails", "description"], null) let tagExternalDocsDescription = tagObj.getIn(["tagDetails", "externalDocs", "description"]) @@ -78,11 +79,11 @@ export default class OperationTag extends React.Component { { tagExternalDocsDescription } { tagExternalDocsUrl ? ": " : null } { tagExternalDocsUrl ? - e.stopPropagation()} - target={"_blank"} - >{tagExternalDocsUrl} : null + target="_blank" + >{tagExternalDocsUrl} : null } } diff --git a/src/core/components/operation.jsx b/src/core/components/operation.jsx index f784e816..554b5fbe 100644 --- a/src/core/components/operation.jsx +++ b/src/core/components/operation.jsx @@ -99,6 +99,7 @@ export default class Operation extends PureComponent { const OperationServers = getComponent( "OperationServers" ) const OperationExt = getComponent( "OperationExt" ) const OperationSummary = getComponent( "OperationSummary" ) + const Link = getComponent( "Link" ) const { showExtensions } = getConfigs() @@ -134,7 +135,7 @@ export default class Operation extends PureComponent { - { externalDocs.url } + {externalDocs.url} : null } diff --git a/src/core/components/providers/markdown.jsx b/src/core/components/providers/markdown.jsx index aac227fa..65b94d9d 100644 --- a/src/core/components/providers/markdown.jsx +++ b/src/core/components/providers/markdown.jsx @@ -4,6 +4,17 @@ import Remarkable from "remarkable" import DomPurify from "dompurify" import cx from "classnames" +DomPurify.addHook("beforeSanitizeElements", function (current, ) { + // Attach safe `rel` values to all elements that contain an `href`, + // i.e. all anchors that are links. + // We _could_ just look for elements that have a non-self target, + // but applying it more broadly shouldn't hurt anything, and is safer. + if (current.href) { + current.setAttribute("rel", "noopener noreferrer") + } + return current +}) + // eslint-disable-next-line no-useless-escape const isPlainText = (str) => /^[A-Z\s0-9!?\.]+$/gi.test(str) @@ -15,13 +26,16 @@ function Markdown({ source, className = "" }) { {source} } - const html = new Remarkable({ + + const md = new Remarkable({ html: true, typographer: true, breaks: true, linkify: true, linkTarget: "_blank" - }).render(source) + }) + + const html = md.render(source) const sanitized = sanitizer(html) if ( !source || !html || !sanitized ) { diff --git a/src/core/plugins/oas3/wrap-components/markdown.js b/src/core/plugins/oas3/wrap-components/markdown.js index f78cfdbf..c0ce7d8a 100644 --- a/src/core/plugins/oas3/wrap-components/markdown.js +++ b/src/core/plugins/oas3/wrap-components/markdown.js @@ -7,6 +7,8 @@ import { sanitizer } from "core/components/providers/markdown" const parser = new Remarkable("commonmark") +parser.set({ linkTarget: "_blank" }) + export const Markdown = ({ source, className = "" }) => { if ( source ) { const html = parser.render(source) diff --git a/test/components/markdown.js b/test/components/markdown.js index d1a3ad3b..0585c676 100644 --- a/test/components/markdown.js +++ b/test/components/markdown.js @@ -52,7 +52,7 @@ describe("Markdown component", function() { it("allows links", function() { const str = `[Link](https://example.com/)` const el = render() - expect(el.html()).toEqual(`

Link

\n
`) + expect(el.html()).toEqual(`

Link

\n
`) }) }) diff --git a/test/xss/anchor-target-rel/info.js b/test/xss/anchor-target-rel/info.js new file mode 100644 index 00000000..6a685a86 --- /dev/null +++ b/test/xss/anchor-target-rel/info.js @@ -0,0 +1,108 @@ +/* eslint-env mocha */ +import React from "react" +import expect from "expect" +import { render } from "enzyme" +import { fromJS } from "immutable" +import Info, { InfoUrl } from "components/info" +import { Link } from "components/layout-utils" +import Markdown from "components/providers/markdown" + +describe(" Anchor Target Safety", function(){ + const dummyComponent = () => null + const components = { + Markdown, + InfoUrl, + Link + } + const baseProps = { + getComponent: c => components[c] || dummyComponent, + host: "example.test", + basePath: "/api", + info: fromJS({ + title: "Hello World" + }) + } + + it("renders externalDocs links with safe `rel` attributes", function () { + const props = { + ...baseProps, + externalDocs: fromJS({ + url: "http://google.com/" + }) + } + let wrapper = render() + const anchor = wrapper.find("a") + + expect(anchor.html()).toEqual("http://google.com/") + expect(anchor.attr("target")).toEqual("_blank") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) + + it("renders Contact links with safe `rel` attributes", function () { + const props = { + ...baseProps, + info: fromJS({ + contact: { + url: "http://google.com/", + name: "My Site" + } + }) + } + let wrapper = render() + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://google.com/") + expect(anchor.attr("target")).toEqual("_blank") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) + + it("renders License links with safe `rel` attributes", function () { + const props = { + ...baseProps, + info: fromJS({ + license: { + url: "http://mit.edu/" + } + }) + } + let wrapper = render() + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://mit.edu/") + expect(anchor.attr("target")).toEqual("_blank") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) + + it("renders termsOfService links with safe `rel` attributes", function () { + const props = { + ...baseProps, + info: fromJS({ + termsOfService: "http://smartbear.com/" + }) + } + let wrapper = render() + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://smartbear.com/") + expect(anchor.attr("target")).toEqual("_blank") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) + + it("renders definition URL links with safe `rel` attributes", function () { + const props = { + ...baseProps, + url: "http://petstore.swagger.io/v2/petstore.json" + } + let wrapper = render() + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://petstore.swagger.io/v2/petstore.json") + expect(anchor.attr("target")).toEqual("_blank") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) +}) diff --git a/test/xss/anchor-target-rel/link.js b/test/xss/anchor-target-rel/link.js new file mode 100644 index 00000000..bc082f20 --- /dev/null +++ b/test/xss/anchor-target-rel/link.js @@ -0,0 +1,44 @@ +/* eslint-env mocha */ +import React from "react" +import expect from "expect" +import { render } from "enzyme" +import { fromJS } from "immutable" +import { Link } from "components/layout-utils" + +describe(" Anchor Target Safety", function () { + const dummyComponent = () => null + const components = { + Link + } + const baseProps = { + getComponent: c => components[c] || dummyComponent + } + + it("renders regular links with `noreferrer` and `noopener`", function () { + const props = { + ...baseProps, + href: "http://google.com/" + } + let wrapper = render() + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://google.com/") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) + + it("enforces `noreferrer` and `noopener` on target=_blank links", function () { + const props = { + ...baseProps, + href: "http://google.com/", + target: "_blank" + } + let wrapper = render() + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://google.com/") + expect(anchor.attr("target")).toEqual("_blank") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) +}) diff --git a/test/xss/anchor-target-rel/markdown.js b/test/xss/anchor-target-rel/markdown.js new file mode 100644 index 00000000..0e30accc --- /dev/null +++ b/test/xss/anchor-target-rel/markdown.js @@ -0,0 +1,66 @@ +/* eslint-env mocha */ +import React from "react" +import expect from "expect" +import { render } from "enzyme" +import Markdown from "components/providers/markdown" +import { Markdown as OAS3Markdown } from "corePlugins/oas3/wrap-components/markdown.js" + +describe("Markdown Link Anchor Safety", function () { + describe("Swagger 2.0", function () { + it("sanitizes Markdown links", function () { + const str = `Hello, [here](http://google.com/) is my link` + const wrapper = render() + + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://google.com/") + expect(anchor.attr("target")).toEqual("_blank") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) + + it("sanitizes raw HTML links", function () { + const str = `Hello, here is my link` + const wrapper = render() + + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://google.com/") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) + }) + + describe("OAS 3", function () { + it("sanitizes Markdown links", function () { + const str = `Hello, [here](http://google.com/) is my link` + const wrapper = render() + + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://google.com/") + expect(anchor.attr("target")).toEqual("_blank") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) + + it("sanitizes raw HTML links", function () { + const str = `Hello, here is my link` + const wrapper = render() + + const anchor = wrapper.find("a") + + expect(anchor.attr("href")).toEqual("http://google.com/") + expect(anchor.attr("rel") || "").toInclude("noopener") + expect(anchor.attr("rel") || "").toInclude("noreferrer") + }) + }) +}) + +function withMarkdownWrapper(str, { isOAS3 = false } = {}) { + if(isOAS3) { + return `

${str}

` + } + + return `

${str}

\n
` +} \ No newline at end of file diff --git a/test/xss/anchor-target-rel/online-validator-badge.js b/test/xss/anchor-target-rel/online-validator-badge.js new file mode 100644 index 00000000..7b6b5736 --- /dev/null +++ b/test/xss/anchor-target-rel/online-validator-badge.js @@ -0,0 +1,32 @@ +/* eslint-env mocha */ +import React from "react" +import expect, { createSpy } from "expect" +import { mount } from "enzyme" +import { fromJS, Map } from "immutable" +import OnlineValidatorBadge from "components/online-validator-badge" + +describe(" Anchor Target Safety", function () { + it("should render a validator link with safe `rel` attributes", function () { + // When + const props = { + getConfigs: () => ({}), + getComponent: () => null, + specSelectors: { + url: () => "swagger.json" + } + } + const wrapper = mount( + + ) + + const anchor = wrapper.find("a") + + // Then + expect(anchor.props().href).toEqual( + "https://online.swagger.io/validator/debug?url=swagger.json" + ) + expect(anchor.props().target).toEqual("_blank") + expect(anchor.props().rel || "").toInclude("noopener") + expect(anchor.props().rel || "").toInclude("noreferrer") + }) +})