Skip to content

Commit 977cc27

Browse files
committed
Fix XSS vulnerability on search results page
Pages that are indexed in search results have their entire contents indexed, including any HTML code snippets. These HTML snippets would appear in the search results unsanitised, so it was possible to render arbitrary HTML or run arbitrary scripts: > ![script being invoked](https://user-images.githubusercontent.com/5111927/230888935-0367b598-eda7-4f67-afb5-799b41684ee3.png) > ![HTML being rendered](https://user-images.githubusercontent.com/5111927/230888939-f0056edc-6955-4f10-8aee-c93414b1cb69.png) This is a largely theoretical security issue; to exploit it, an attacker would need to find a way of committing malicious code to a page indexed by a site that uses tech-docs-gem (which are typically not editable by untrusted users). Their code would also be limited by the relatively short length that's rendered in the corresponding search result. Nevertheless, the XSS would then be triggerable by visiting a pre-constructed URL (`/search/index.html?q=some+search+term`), which users could be tricked into clicking on through social engineering. This commit sanitises the HTML before rendering it to the page. It does so whilst retaining the `<mark data-markjs="true">` behaviour that highlights the search term in the result: > ![sanitised HTML with highlights](https://user-images.githubusercontent.com/5111927/230888944-9aaf4920-cddd-43f9-8ef5-17f15785af73.png) I've used jQuery's `text()` function for sanitisation, as that is the approach used elsewhere in the project ([1]). I did consider using native JavaScript (using the same approach as in Mustache [2]) to avoid the jQuery dependency, but this itself may contain bugs and would lead to having two sanitisation approaches to maintain, so I opted against it. For future reference, the code in this commit can be swapped out with: ```js var entityMap = { '&': '&amp;', '<': '&lt;', '>': '&gt;', '"': '&quot;', "'": '&#39;', '/': '&#x2F;', '`': '&#x60;', '=': '&#x3D;' }; var sanitizedContent = String(content).replace(/[&<>"'`=\/]/g, function (s) { return entityMap[s]; }); ``` [1]: https://github.com/alphagov/tech-docs-gem/blob/66cc7ab0a06dc2f1fe89de8cba2270fcf46f6466/lib/assets/javascripts/_modules/search.js#L202-L204 [2]: https://github.com/janl/mustache.js/blob/972fd2b27a036888acfcb60d6119317744fac7ee/mustache.js#L60-L75
1 parent 7c29396 commit 977cc27

File tree

2 files changed

+8
-2
lines changed

2 files changed

+8
-2
lines changed

lib/assets/javascripts/_modules/search.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,8 +169,8 @@
169169

170170
this.processContent = function processContent (content, query) {
171171
var output
172-
content = '<div>' + content + '</div>'
173-
content = $(content).mark(query)
172+
var sanitizedContent = $('<div></div>').text(content).html()
173+
content = $('<div>' + sanitizedContent + '</div>').mark(query)
174174

175175
// Split content by sentence.
176176
var sentences = content.html().replace(/(\.+|:|!|\?|\r|\n)("*|'*|\)*|}*|]*)/gm, '|').split('|')

spec/javascripts/search-spec.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,5 +99,11 @@ describe('Search', function () {
9999
var expectedResults = ' … This is <mark data-markjs="true">test</mark> sentence one … This is <mark data-markjs="true">test</mark> sentence two … This is <mark data-markjs="true">test</mark> sentence three … This is <mark data-markjs="true">test</mark> sentence four … This is <mark data-markjs="true">test</mark> sentence five … '
100100
expect(processedContent).toEqual(expectedResults)
101101
})
102+
103+
it('sanitises HTML in the search results', function () {
104+
processedContent = module.processContent('It will render multiple `<input>` `<script>alert("uhoh")</script>` and its accompanying suggestions and `aria-live` region.', 'multi region')
105+
var expectedResults = ' … It will render <mark data-markjs="true">multi</mark>ple `&lt;input&gt;` `&lt;script&gt;alert("uhoh")&lt;/script&gt;` and its accompanying suggestions and `aria-live` <mark data-markjs="true">region</mark> … '
106+
expect(processedContent).toEqual(expectedResults)
107+
})
102108
})
103109
})

0 commit comments

Comments
 (0)