Commit 6900d26c authored by Mark Florian's avatar Mark Florian

Preserve sibling elements in settings search

Before, any elements that were siblings of matching text nodes got
destroyed, replaced with their normalised `textContent`. That is, given
this DOM:

```html
<p>
  Foo <i>bar</i> qux.
</p>
```

If you searched for `foo`, then the above would be transformed into
something like:

```html
<p>
  <mark>Foo</mark> bar qux.
</p>

<!-- But the correct DOM would be: -->
<p>
  <mark>Foo</mark> <i>bar</i> qux.
</p>
```

And then if you cleared the search, it would become:

```html
<p>
  Foo bar qux.
</p>

<!-- But the correct DOM would be the same as before the search: -->
<p>
  Foo <i>bar</i> qux.
</p>
```

The crux of the problem was that the *highlighting* algorithm wasn't the
inverse of the *highlight clearing* algorithm. Now, it is, and only text
nodes are modified. Matching text nodes are split into non-matching text
nodes and `mark` elements containing the matched search text, and that
collection of nodes replaces the original text node.

Addresses https://gitlab.com/gitlab-org/gitlab/-/issues/350494.

Changelog: fixed
parent 71accc21
<script>
import { GlSearchBoxByType } from '@gitlab/ui';
import { uniq, escapeRegExp } from 'lodash';
import { escapeRegExp } from 'lodash';
import {
EXCLUDED_NODES,
HIDE_CLASS,
......@@ -60,41 +60,42 @@ const hideSectionsExcept = (sectionSelector, visibleSections) => {
});
};
const transformMatchElement = (element, searchTerm) => {
const textStr = element.textContent;
const highlightTextNode = (textNode, searchTerm) => {
const escapedSearchTerm = new RegExp(`(${escapeRegExp(searchTerm)})`, 'gi');
const textList = textNode.data.split(escapedSearchTerm);
return textList.reduce((documentFragment, text) => {
let addElement;
const textList = textStr.split(escapedSearchTerm);
const replaceFragment = document.createDocumentFragment();
textList.forEach((text) => {
let addElement = document.createTextNode(text);
if (escapedSearchTerm.test(text)) {
addElement = document.createElement('mark');
addElement.className = `${HIGHLIGHT_CLASS} ${NONE_PADDING_CLASS}`;
addElement.textContent = text;
escapedSearchTerm.lastIndex = 0;
} else {
addElement = document.createTextNode(text);
}
replaceFragment.appendChild(addElement);
});
return replaceFragment;
documentFragment.appendChild(addElement);
return documentFragment;
}, document.createDocumentFragment());
};
const highlightElements = (elements = [], searchTerm) => {
elements.forEach((element) => {
const replaceFragment = transformMatchElement(element, searchTerm);
element.innerHTML = '';
element.appendChild(replaceFragment);
const highlightText = (textNodes = [], searchTerm) => {
textNodes.forEach((textNode) => {
const fragmentWithHighlights = highlightTextNode(textNode, searchTerm);
textNode.parentElement.replaceChild(fragmentWithHighlights, textNode);
});
};
const displayResults = ({ sectionSelector, expandSection, searchTerm }, matches) => {
const elements = matches.map((match) => match.parentElement);
const sections = uniq(elements.map((element) => findSettingsSection(sectionSelector, element)));
const displayResults = ({ sectionSelector, expandSection, searchTerm }, matchingTextNodes) => {
const sections = Array.from(
new Set(matchingTextNodes.map((node) => findSettingsSection(sectionSelector, node))),
);
hideSectionsExcept(sectionSelector, sections);
sections.forEach(expandSection);
highlightElements(elements, searchTerm);
highlightText(matchingTextNodes, searchTerm);
};
const clearResults = (params) => {
......@@ -114,13 +115,13 @@ const search = (root, searchTerm) => {
: NodeFilter.FILTER_REJECT;
},
});
const results = [];
const textNodes = [];
for (let currentNode = iterator.nextNode(); currentNode; currentNode = iterator.nextNode()) {
results.push(currentNode);
textNodes.push(currentNode);
}
return results;
return textNodes;
};
export default {
......
......@@ -12,7 +12,8 @@ describe('search_settings/components/search_settings.vue', () => {
const GENERAL_SETTINGS_ID = 'js-general-settings';
const ADVANCED_SETTINGS_ID = 'js-advanced-settings';
const EXTRA_SETTINGS_ID = 'js-extra-settings';
const TEXT_CONTAIN_SEARCH_TERM = `This text contain ${SEARCH_TERM} and <script>alert("111")</script> others.`;
const TEXT_CONTAIN_SEARCH_TERM = `This text contain ${SEARCH_TERM}.`;
const TEXT_WITH_SIBLING_ELEMENTS = `${SEARCH_TERM} <a data-testid="sibling" href="#">Learn more</a>.`;
let wrapper;
......@@ -43,13 +44,7 @@ describe('search_settings/components/search_settings.vue', () => {
});
};
const matchParentElement = () => {
const highlightedList = Array.from(document.querySelectorAll(`.${HIGHLIGHT_CLASS}`));
return highlightedList.map((element) => {
return element.parentNode;
});
};
const findMatchSiblingElement = () => document.querySelector(`[data-testid="sibling"]`);
const findSearchBox = () => wrapper.find(GlSearchBoxByType);
const search = (term) => {
findSearchBox().vm.$emit('input', term);
......@@ -70,6 +65,7 @@ describe('search_settings/components/search_settings.vue', () => {
<section id="${EXTRA_SETTINGS_ID}" class="settings">
<span>${SEARCH_TERM}</span>
<span>${TEXT_CONTAIN_SEARCH_TERM}</span>
<span>${TEXT_WITH_SIBLING_ELEMENTS}</span>
</section>
</div>
</div>
......@@ -100,7 +96,7 @@ describe('search_settings/components/search_settings.vue', () => {
it('highlight elements that match the search term', () => {
search(SEARCH_TERM);
expect(highlightedElementsCount()).toBe(2);
expect(highlightedElementsCount()).toBe(3);
});
it('highlight only search term and not the whole line', () => {
......@@ -109,14 +105,26 @@ describe('search_settings/components/search_settings.vue', () => {
expect(highlightedTextNodes()).toBe(true);
});
it('prevents search xss', () => {
// Regression test for https://gitlab.com/gitlab-org/gitlab/-/issues/350494
it('preserves elements that are siblings of matches', () => {
const snapshot = `
<a
data-testid="sibling"
href="#"
>
Learn more
</a>
`;
expect(findMatchSiblingElement()).toMatchInlineSnapshot(snapshot);
search(SEARCH_TERM);
const parentNodeList = matchParentElement();
parentNodeList.forEach((element) => {
const scriptElement = element.getElementsByTagName('script');
expect(scriptElement.length).toBe(0);
});
expect(findMatchSiblingElement()).toMatchInlineSnapshot(snapshot);
clearSearch();
expect(findMatchSiblingElement()).toMatchInlineSnapshot(snapshot);
});
describe('default', () => {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment