fix(27428): fix if we type enter anything followed by a \ in settings search #27432

DDDDDanica · 2024-09-26T17:26:01Z

In JavaScript, backslashes are used as escape characters in strings and regular expressions, which can cause issues when an unescaped backslash is present in a user input. To handle this, we'll need to escape any special characters in the user input before constructing the regular expression.

Description

Related issues

Fixes: #27428, #26945

Manual testing steps

Go to settings
type foo\
press enter should not crash

Screenshots/Recordings

Before

After

regex-search.mov

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-09-26T17:26:20Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

metamaskbot · 2024-09-26T18:20:22Z

Builds ready [9f20d4f]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1816 ± 90 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	387	2304	1688	445	214
		domContentLoaded	1601	2245	1794	167	80
		load	1608	2309	1816	187	90
		domInteractive	24	196	51	38	18

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 92 Bytes (0.00%)
common: 0 Bytes (0.00%)

NiranjanaBinoy

LGTM

metamaskbot · 2024-09-26T21:39:37Z

Builds ready [1f6301e]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1884 ± 125 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	260	2608	1749	526	253
		domContentLoaded	1598	2449	1839	232	112
		load	1618	2543	1884	260	125
		domInteractive	13	149	50	31	15

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 106 Bytes (0.00%)
common: 0 Bytes (0.00%)

davidmurdoch · 2024-09-26T22:53:17Z

Should we just use String#replaceAll (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll) instead of using a regular expression in the relevant string replacement step in colorText?

DDDDDanica · 2024-09-27T17:20:48Z

Should we just use String#replaceAll (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll) instead of using a regular expression in the relevant string replacement step in colorText?

hey @davidmurdoch do you mean
elemText = elemText.replaceAll('&', '&'); instead of elemText = elemText.replace('&', '&');?

davidmurdoch · 2024-09-28T00:30:48Z

Should we just use String#replaceAll (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll) instead of using a regular expression in the relevant string replacement step in colorText?

hey @davidmurdoch do you mean elemText = elemText.replaceAll('&', '&'); instead of elemText = elemText.replace('&', '&');?

No. In the colorText function we have:

export function colorText(menuElement, regex) {

   // ... omitted code ...
   
  menuElement.innerHTML = elemText.replace(
      regex,
      '<span class="settings-page__header__search__list__item__highlight">$&</span>',
  );
}

used as:

colorText(menuTabElement, searchRegex);

but this could be simplified to something like:

export function colorText(menuElement, textToColor) {

   // ... omitted code ...
   
  menuElement.innerHTML = elemText.replaceAll(
      textToColor,
      `<span class="settings-page__header__search__list__item__highlight">${textToColor}</span>`,
  );
}

used as:

colorText(menuTabElement, searchElem.value);

I don't think we even need to create a regular expression here.

DDDDDanica · 2024-09-28T19:06:19Z

@davidmurdoch good suggestions ! However, i believe const searchRegex = new RegExp(escapeRegExp(searchElem.value), 'gi'); this regex is to make the search case-insensitive. we could do something like but it seems more complex

export function colorText(menuElement, textToColor) {
  if (menuElement !== null) {
    let elemText = menuElement.innerHTML;
    elemText = elemText.replace('&amp;', '&');
    elemText = elemText.replace(
      /(<span class="settings-page__header__search__list__item__highlight">|<\/span>)/gim,
      '',
    );

    const lowerCaseElemText = elemText.toLowerCase();
    const lowerCaseTextToColor = textToColor.toLowerCase();

    if (lowerCaseElemText.includes(lowerCaseTextToColor)) {
      const index = lowerCaseElemText.indexOf(lowerCaseTextToColor);
      const originalText = elemText.substring(index, index + textToColor.length);
      
      menuElement.innerHTML = elemText. replaceAll(
        originalText,
        `<span class="settings-page__header__search__list__item__highlight">${originalText}</span>`,
      );
    }
  }
}

metamaskbot · 2024-10-01T19:20:03Z

Builds ready [3535156]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1796 ± 69 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	276	2151	1716	363	174
		domContentLoaded	1565	2084	1769	142	68
		load	1573	2097	1796	144	69
		domInteractive	14	203	59	52	25

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 106 Bytes (0.00%)
common: 0 Bytes (0.00%)

… search

metamaskbot · 2024-10-02T14:40:26Z

Builds ready [9417681]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1715 ± 84 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	385	2231	1630	338	162
		domContentLoaded	1489	2170	1682	174	83
		load	1497	2180	1715	174	84
		domInteractive	16	91	39	20	9

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 106 Bytes (0.00%)
common: 0 Bytes (0.00%)

sonarcloud · 2024-10-02T14:56:52Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

DDDDDanica added the team-extension-platform label Sep 26, 2024

DDDDDanica self-assigned this Sep 26, 2024

DDDDDanica requested a review from a team as a code owner September 26, 2024 17:26

DDDDDanica mentioned this pull request Sep 26, 2024

[Bug]: Multiple backslashes are breaking the settings search. #26945

Closed

DDDDDanica force-pushed the fix/27428-search-regex branch from 8e04cf2 to 9f20d4f Compare September 26, 2024 17:45

NiranjanaBinoy previously approved these changes Sep 26, 2024

View reviewed changes

DDDDDanica dismissed NiranjanaBinoy’s stale review via 0bb8922 September 26, 2024 20:19

DDDDDanica force-pushed the fix/27428-search-regex branch 2 times, most recently from 4c36e9f to 1f6301e Compare September 26, 2024 21:08

DDDDDanica force-pushed the fix/27428-search-regex branch from 1f6301e to 3535156 Compare October 1, 2024 18:48

NiranjanaBinoy previously approved these changes Oct 1, 2024

View reviewed changes

DDDDDanica added 2 commits October 2, 2024 15:07

fix(27428): fix if we type enter anything followed by a \ in settings…

90e7d4a

… search

fix(27428): Attach more tests to pass sonar quality gate

9417681

DDDDDanica dismissed NiranjanaBinoy’s stale review via 9417681 October 2, 2024 14:08

DDDDDanica force-pushed the fix/27428-search-regex branch from 3535156 to 9417681 Compare October 2, 2024 14:08

NiranjanaBinoy approved these changes Oct 2, 2024

View reviewed changes

DDDDDanica enabled auto-merge October 2, 2024 21:23

davidmurdoch approved these changes Oct 2, 2024

View reviewed changes

DDDDDanica added this pull request to the merge queue Oct 2, 2024

Merged via the queue into develop with commit e76a149 Oct 2, 2024
77 checks passed

DDDDDanica deleted the fix/27428-search-regex branch October 2, 2024 22:05

github-actions bot locked and limited conversation to collaborators Oct 2, 2024

metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(27428): fix if we type enter anything followed by a \ in settings search #27432

fix(27428): fix if we type enter anything followed by a \ in settings search #27432

DDDDDanica commented Sep 26, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Sep 26, 2024

metamaskbot commented Sep 26, 2024

NiranjanaBinoy left a comment

metamaskbot commented Sep 26, 2024

davidmurdoch commented Sep 26, 2024

DDDDDanica commented Sep 27, 2024

davidmurdoch commented Sep 28, 2024

DDDDDanica commented Sep 28, 2024 •

edited

Loading

metamaskbot commented Oct 1, 2024

metamaskbot commented Oct 2, 2024

sonarcloud bot commented Oct 2, 2024

fix(27428): fix if we type enter anything followed by a \ in settings search #27432

fix(27428): fix if we type enter anything followed by a \ in settings search #27432

Conversation

DDDDDanica commented Sep 26, 2024 • edited by github-actions bot Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Sep 26, 2024

metamaskbot commented Sep 26, 2024

NiranjanaBinoy left a comment

Choose a reason for hiding this comment

metamaskbot commented Sep 26, 2024

davidmurdoch commented Sep 26, 2024

DDDDDanica commented Sep 27, 2024

davidmurdoch commented Sep 28, 2024

DDDDDanica commented Sep 28, 2024 • edited Loading

metamaskbot commented Oct 1, 2024

metamaskbot commented Oct 2, 2024

sonarcloud bot commented Oct 2, 2024

Quality Gate passed

DDDDDanica commented Sep 26, 2024 •

edited by github-actions bot

Loading

DDDDDanica commented Sep 28, 2024 •

edited

Loading