Skip to content

Commit

Permalink
fix: anchor tag safety (via swagger-api#4789)
Browse files Browse the repository at this point in the history
* v3.17.6

* release(3.17.6): rebuild dist

* add failing tests

* fix Link component

* fix OnlineValidatorBadge component

* switch from <a> to <Link> in operation components

* make Markdown inputs safe

* use Link component in Info block, for target safety

* add eslint rule for unsafe `target` usage
  • Loading branch information
shockey authored Aug 4, 2018
1 parent fe5b234 commit dd3afdc
Show file tree
Hide file tree
Showing 15 changed files with 354 additions and 31 deletions.
8 changes: 8 additions & 0 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@
"import"
],

"settings": {
"react": {
"pragma": "React",
"version": "15.0"
}
},

"rules": {
"semi": [2, "never"],
"strict": 0,
Expand All @@ -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]
Expand Down
43 changes: 38 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
46 changes: 30 additions & 16 deletions src/core/components/info.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<div>
{ url && <div><a href={ sanitizeUrl(url) } target="_blank">{ name } - Website</a></div> }
{ url && <div><Link href={ sanitizeUrl(url) } target="_blank">{ name } - Website</Link></div> }
{ email &&
<a href={sanitizeUrl(`mailto:${email}`)}>
<Link href={sanitizeUrl(`mailto:${email}`)}>
{ url ? `Send email to ${name}` : `Contact ${name}`}
</a>
</Link>
}
</div>
)
Expand All @@ -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 (
<div>
{
url ? <a target="_blank" href={ sanitizeUrl(url) }>{ name }</a>
url ? <Link target="_blank" href={ sanitizeUrl(url) }>{ name }</Link>
: <span>{ name }</span>
}
</div>
Expand All @@ -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 <a target="_blank" href={ sanitizeUrl(url) }><span className="url"> { url } </span></a>
const { url, getComponent } = this.props

const Link = getComponent("Link")

return <Link target="_blank" href={ sanitizeUrl(url) }><span className="url"> { url } </span></Link>
}
}

Expand All @@ -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")
Expand All @@ -111,7 +125,7 @@ export default class Info extends React.Component {
{ version && <VersionStamp version={version}></VersionStamp> }
</h2>
{ host || basePath ? <InfoBasePath host={ host } basePath={ basePath } /> : null }
{ url && <InfoUrl url={url} /> }
{ url && <InfoUrl getComponent={getComponent} url={url} /> }
</hgroup>

<div className="description">
Expand All @@ -120,14 +134,14 @@ export default class Info extends React.Component {

{
termsOfService && <div>
<a target="_blank" href={ sanitizeUrl(termsOfService) }>Terms of service</a>
<Link target="_blank" href={ sanitizeUrl(termsOfService) }>Terms of service</Link>
</div>
}

{ contact && contact.size ? <Contact data={ contact } /> : null }
{ license && license.size ? <License license={ license } /> : null }
{contact && contact.size ? <Contact getComponent={getComponent} data={ contact } /> : null }
{license && license.size ? <License getComponent={getComponent} license={ license } /> : null }
{ externalDocsUrl ?
<a target="_blank" href={sanitizeUrl(externalDocsUrl)}>{externalDocsDescription || externalDocsUrl}</a>
<Link target="_blank" href={sanitizeUrl(externalDocsUrl)}>{externalDocsDescription || externalDocsUrl}</Link>
: null }

</div>
Expand Down
2 changes: 1 addition & 1 deletion src/core/components/layout-utils.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ export class Select extends React.Component {
export class Link extends React.Component {

render() {
return <a {...this.props} className={xclass(this.props.className, "link")}/>
return <a {...this.props} rel="noopener noreferrer" className={xclass(this.props.className, "link")}/>
}

}
Expand Down
2 changes: 1 addition & 1 deletion src/core/components/online-validator-badge.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export default class OnlineValidatorBadge extends React.Component {
}

return (<span style={{ float: "right"}}>
<a target="_blank" href={`${ sanitizedValidatorUrl }/debug?url=${ encodeURIComponent(this.state.url) }`}>
<a target="_blank" rel="noopener noreferrer" href={`${ sanitizedValidatorUrl }/debug?url=${ encodeURIComponent(this.state.url) }`}>
<ValidatorImage src={`${ sanitizedValidatorUrl }?url=${ encodeURIComponent(this.state.url) }`} alt="Online validator badge"/>
</a>
</span>)
Expand Down
7 changes: 4 additions & 3 deletions src/core/components/operation-tag.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
Expand Down Expand Up @@ -78,11 +79,11 @@ export default class OperationTag extends React.Component {
{ tagExternalDocsDescription }
{ tagExternalDocsUrl ? ": " : null }
{ tagExternalDocsUrl ?
<a
<Link
href={sanitizeUrl(tagExternalDocsUrl)}
onClick={(e) => e.stopPropagation()}
target={"_blank"}
>{tagExternalDocsUrl}</a> : null
target="_blank"
>{tagExternalDocsUrl}</Link> : null
}
</small>
}
Expand Down
3 changes: 2 additions & 1 deletion src/core/components/operation.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -134,7 +135,7 @@ export default class Operation extends PureComponent {
<span className="opblock-external-docs__description">
<Markdown source={ externalDocs.description } />
</span>
<a target="_blank" className="opblock-external-docs__link" href={ sanitizeUrl(externalDocs.url) }>{ externalDocs.url }</a>
<Link target="_blank" className="opblock-external-docs__link" href={sanitizeUrl(externalDocs.url)}>{externalDocs.url}</Link>
</div>
</div> : null
}
Expand Down
18 changes: 16 additions & 2 deletions src/core/components/providers/markdown.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -15,13 +26,16 @@ function Markdown({ source, className = "" }) {
{source}
</div>
}
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 ) {
Expand Down
2 changes: 2 additions & 0 deletions src/core/plugins/oas3/wrap-components/markdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion test/components/markdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe("Markdown component", function() {
it("allows links", function() {
const str = `[Link](https://example.com/)`
const el = render(<Markdown source={str} />)
expect(el.html()).toEqual(`<div class="markdown"><p><a target="_blank" href="https://example.com/">Link</a></p>\n</div>`)
expect(el.html()).toEqual(`<div class="markdown"><p><a rel="noopener noreferrer" target="_blank" href="https://example.com/">Link</a></p>\n</div>`)
})
})

Expand Down
Loading

0 comments on commit dd3afdc

Please sign in to comment.