Skip to content

Commit

Permalink
Additional checks on Result component (#481)
Browse files Browse the repository at this point in the history
  • Loading branch information
JasonStoltz authored Apr 14, 2020
1 parent 9147036 commit e4ab783
Show file tree
Hide file tree
Showing 7 changed files with 199 additions and 2 deletions.
1 change: 1 addition & 0 deletions packages/react-search-ui-views/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
"@storybook/react": "^4.1.18",
"babel-loader": "^8.0.6",
"chokidar-cli": "^1.2.2",
"core-js": "^3.6.5",
"enzyme": "^3.10.0",
"enzyme-adapter-react-16": "^1.14.0",
"enzyme-to-json": "^3.3.5",
Expand Down
4 changes: 2 additions & 2 deletions packages/react-search-ui-views/src/Result.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import PropTypes from "prop-types";
import React from "react";

import { appendClassName } from "./view-helpers";
import { appendClassName, getUrlSanitizer } from "./view-helpers";
import { isFieldValueWrapper } from "./types/FieldValueWrapper";

function getFieldType(result, field, type) {
Expand Down Expand Up @@ -59,7 +59,7 @@ function Result({
}) {
const fields = getEscapedFields(result);
const title = getEscapedField(result, titleField);
const url = getRaw(result, urlField);
const url = getUrlSanitizer(URL, location)(getRaw(result, urlField));

return (
<li className={appendClassName("sui-result", className)} {...rest}>
Expand Down
16 changes: 16 additions & 0 deletions packages/react-search-ui-views/src/__tests__/Result.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,19 @@ it("renders with className prop applied", () => {
const { className } = wrapper.props();
expect(className).toEqual("sui-result test-class");
});

it("renders correctly when there is a malicious URL", () => {
const wrapper = shallow(
<Result
{...{
...requiredProps,
result: {
...requiredProps.result,
[URL_FIELD]: { raw: "javascript://test%0aalert(document.domain)" }
},
urlField: URL_FIELD
}}
/>
);
expect(wrapper).toMatchSnapshot();
});
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,60 @@ exports[`renders correctly when there is a URL 1`] = `
</li>
`;

exports[`renders correctly when there is a malicious URL 1`] = `
<li
className="sui-result"
>
<div
className="sui-result__header"
/>
<div
className="sui-result__body"
>
<ul
className="sui-result__details"
>
<li
key="field"
>
<span
className="sui-result__key"
>
field
</span>
<span
className="sui-result__value"
dangerouslySetInnerHTML={
Object {
"__html": "value",
}
}
/>
</li>
<li
key="url"
>
<span
className="sui-result__key"
>
url
</span>
<span
className="sui-result__value"
dangerouslySetInnerHTML={
Object {
"__html": "javascript://test%0aalert(document.domain)",
}
}
/>
</li>
</ul>
</div>
</li>
`;

exports[`renders correctly when there is a title 1`] = `
<li
className="sui-result"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import { getUrlSanitizer } from "../../view-helpers";
import URL from "core-js-pure/features/url";

describe("getUrlSanitizer", () => {
let url;
let currentLocation;

beforeEach(() => {
url = "";
currentLocation = "";
});

function subject() {
return getUrlSanitizer(URL, currentLocation)(url);
}

describe("when valid url with http", () => {
beforeEach(() => {
url = "http://www.example.com/";
currentLocation = "http://www.mysite.com";
});

it("should allow it", () => {
expect(subject()).toEqual(url);
});
});

describe("when valid url with https", () => {
beforeEach(() => {
url = "https://www.example.com/";
currentLocation = "http://www.mysite.com";
});

it("should allow it", () => {
expect(subject()).toEqual(url);
});
});

describe("when relative url", () => {
beforeEach(() => {
url = "/item/1234";
currentLocation = "http://www.mysite.com";
});

it("should allow it", () => {
expect(subject()).toEqual(url);
});
});

describe("when the protocol is javascript", () => {
beforeEach(() => {
url = "javascript://test%0aalert(document.domain)";
currentLocation = "http://www.mysite.com";
});

it("should disallow it", () => {
expect(subject()).toEqual("");
});
});

describe("when the protocol is javascript with spaces in it", () => {
beforeEach(() => {
url = " javascript://test%0aalert(document.domain)";
currentLocation = "http://www.mysite.com";
});

it("should disallow it", () => {
expect(subject()).toEqual("");
});
});

describe("when the url is invalid", () => {
beforeEach(() => {
url = "<div>My bad URL</div>";
currentLocation = "http://www.mysite.com";
});

it("treats it as a relative url, which should still be safe", () => {
expect(subject()).toEqual(url);
});
});

describe("when the protocol is not whitelisted", () => {
beforeEach(() => {
url = "mailto://user@example.com";
currentLocation = "http://www.mysite.com";
});

it("should disallow it", () => {
expect(subject()).toEqual("");
});
});

describe("when the url is protocol-less", () => {
beforeEach(() => {
url = "//www.example.com/";
currentLocation = "https://www.mysite.com";
});

it("uses the protocol from the current location", () => {
expect(subject()).toEqual(url);
});
});
});
21 changes: 21 additions & 0 deletions packages/react-search-ui-views/src/view-helpers/getUrlSanitizer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const VALID_PROTOCOLS = ["http:", "https:"];

/**
*
* @param {URL} URLParser URL interface provided by browser https://developer.mozilla.org/en-US/docs/Web/API/URL
* @param {String} currentLocation String representation of the browser's current location
*/
export default function getUrlSanitizer(URLParser, currentLocation) {
// This function is curried so that dependencies can be injected and don't need to be mocked in tests.
return url => {
let parsedUrl = {};

try {
// Attempts to parse a URL as relative
parsedUrl = new URLParser(url, currentLocation);
// eslint-disable-next-line no-empty
} catch (e) {}

return VALID_PROTOCOLS.includes(parsedUrl.protocol) ? url : "";
};
}
1 change: 1 addition & 0 deletions packages/react-search-ui-views/src/view-helpers/index.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export { default as getFilterValueDisplay } from "./getFilterValueDisplay";
export { default as appendClassName } from "./appendClassName";
export { default as getUrlSanitizer } from "./getUrlSanitizer";

0 comments on commit e4ab783

Please sign in to comment.