fix(compactSelect): Escape quotes inside option values (#51007)

`CompactSelect` crashes when there's a quote inside any option's value
string, e.g. `{value: 'hello"', label: …}`


https://github.com/getsentry/sentry/assets/44172267/970ebbeb-be0a-46ad-9638-7d9043779ac9

This happens because `react-aria`'s `useOverlay()` hook query-selects
option containers using a CSS selector string that contains the option's
value:
<img width="413" alt="image"
src="https://github.com/getsentry/sentry/assets/44172267/26ece092-5420-420f-bdb3-eac19da53a52">

To fix this, we can escape each option's value string ([using
`CSS.escape()`](https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_static)),
making them safe for query selection. The escaped version is only used
internally. The `onChange()` callback will still return options with
their initial, unescaped values.

After the fix, quoted option values no longer cause a crash:


https://github.com/getsentry/sentry/assets/44172267/a53a4d1c-583b-4f20-a719-7fc7666e1615

static/app/components/compactSelect/composite.tsx

@@ -10,6 +10,7 @@ import {Control, ControlProps} from './control';
 import {List, MultipleListProps, SingleListProps} from './list';
 import {EmptyMessage} from './styles';
 import {SelectOption} from './types';
+import {getItemsWithKeys} from './utils';
 
 interface BaseCompositeSelectRegion<Value extends React.Key> {
   options: SelectOption<Value>[];
@@ -162,16 +163,13 @@ function Region<Value extends React.Key>({
     };
   }, [multiple, value, defaultValue, onChange, closeOnSelect]);
 
-  const optionsWithKey = useMemo<SelectOption<Value>[]>(
-    () => options.map(item => ({...item, key: item.value})),
-    [options]
-  );
+  const itemsWithKey = useMemo(() => getItemsWithKeys(options), [options]);
 
   return (
     <List
       {...props}
       {...listProps}
-      items={optionsWithKey}
+      items={itemsWithKey}
       disallowEmptySelection={disallowEmptySelection}
       isOptionDisabled={isOptionDisabled}
       shouldFocusWrap={false}

static/app/components/compactSelect/index.spec.tsx

@@ -140,6 +140,32 @@ describe('CompactSelect', function () {
       expect(screen.getByRole('button', {name: 'Option One +1'})).toBeInTheDocument();
     });
 
+    it('can select options with values containing quotes', async function () {
+      const mock = jest.fn();
+      render(
+        <CompactSelect
+          multiple
+          options={[
+            {value: '"opt_one"', label: 'Option One'},
+            {value: '"opt_two"', label: 'Option Two'},
+          ]}
+          onChange={mock}
+        />
+      );
+
+      // click on the trigger button
+      await userEvent.click(screen.getByRole('button'));
+
+      // select Option One & Option Two
+      await userEvent.click(screen.getByRole('option', {name: 'Option One'}));
+      await userEvent.click(screen.getByRole('option', {name: 'Option Two'}));
+
+      expect(mock).toHaveBeenCalledWith([
+        {value: '"opt_one"', label: 'Option One'},
+        {value: '"opt_two"', label: 'Option Two'},
+      ]);
+    });
+
     it('displays trigger button with prefix', function () {
       render(
         <CompactSelect
@@ -272,8 +298,8 @@ describe('CompactSelect', function () {
           label: 'Section 1',
           showToggleAllButton: true,
           options: [
-            {value: 'opt_one', label: 'Option One'},
-            {value: 'opt_two', label: 'Option Two'},
+            {key: 'opt_one', value: 'opt_one', label: 'Option One'},
+            {key: 'opt_two', value: 'opt_two', label: 'Option Two'},
           ],
         },
         'select'
@@ -295,8 +321,8 @@ describe('CompactSelect', function () {
           label: 'Section 1',
           showToggleAllButton: true,
           options: [
-            {value: 'opt_one', label: 'Option One'},
-            {value: 'opt_two', label: 'Option Two'},
+            {key: 'opt_one', value: 'opt_one', label: 'Option One'},
+            {key: 'opt_two', value: 'opt_two', label: 'Option Two'},
           ],
         },
         'unselect'
@@ -320,8 +346,8 @@ describe('CompactSelect', function () {
           label: 'Section 2',
           showToggleAllButton: true,
           options: [
-            {value: 'opt_three', label: 'Option Three'},
-            {value: 'opt_four', label: 'Option Four'},
+            {key: 'opt_three', value: 'opt_three', label: 'Option Three'},
+            {key: 'opt_four', value: 'opt_four', label: 'Option Four'},
           ],
         },
         'select'
@@ -402,6 +428,33 @@ describe('CompactSelect', function () {
       expect(screen.getByRole('button', {name: 'Option One +1'})).toBeInTheDocument();
     });
 
+    it('can select options with values containing quotes', async function () {
+      const mock = jest.fn();
+      render(
+        <CompactSelect
+          grid
+          multiple
+          options={[
+            {value: '"opt_one"', label: 'Option One'},
+            {value: '"opt_two"', label: 'Option Two'},
+          ]}
+          onChange={mock}
+        />
+      );
+
+      // click on the trigger button
+      await userEvent.click(screen.getByRole('button'));
+
+      // select Option One & Option Two
+      await userEvent.click(screen.getByRole('row', {name: 'Option One'}));
+      await userEvent.click(screen.getByRole('row', {name: 'Option Two'}));
+
+      expect(mock).toHaveBeenCalledWith([
+        {value: '"opt_one"', label: 'Option One'},
+        {value: '"opt_two"', label: 'Option Two'},
+      ]);
+    });
+
     it('displays trigger button with prefix', function () {
       render(
         <CompactSelect
@@ -536,8 +589,8 @@ describe('CompactSelect', function () {
           label: 'Section 1',
           showToggleAllButton: true,
           options: [
-            {value: 'opt_one', label: 'Option One'},
-            {value: 'opt_two', label: 'Option Two'},
+            {key: 'opt_one', value: 'opt_one', label: 'Option One'},
+            {key: 'opt_two', value: 'opt_two', label: 'Option Two'},
           ],
         },
         'select'
@@ -559,8 +612,8 @@ describe('CompactSelect', function () {
           label: 'Section 1',
           showToggleAllButton: true,
           options: [
-            {value: 'opt_one', label: 'Option One'},
-            {value: 'opt_two', label: 'Option Two'},
+            {key: 'opt_one', value: 'opt_one', label: 'Option One'},
+            {key: 'opt_two', value: 'opt_two', label: 'Option Two'},
           ],
         },
         'unselect'
@@ -584,8 +637,8 @@ describe('CompactSelect', function () {
           label: 'Section 2',
           showToggleAllButton: true,
           options: [
-            {value: 'opt_three', label: 'Option Three'},
-            {value: 'opt_four', label: 'Option Four'},
+            {key: 'opt_three', value: 'opt_three', label: 'Option Three'},
+            {key: 'opt_four', value: 'opt_four', label: 'Option Four'},
           ],
         },
         'select'

static/app/components/compactSelect/index.tsx

@@ -13,6 +13,7 @@ import type {
   SelectOptionOrSectionWithKey,
   SelectSection,
 } from './types';
+import {getItemsWithKeys} from './utils';
 
 export type {SelectOption, SelectOptionOrSection, SelectSection};
 
@@ -80,17 +81,7 @@ function CompactSelect<Value extends React.Key>({
     return {multiple, value, defaultValue, onChange, closeOnSelect, grid};
   }, [multiple, value, defaultValue, onChange, closeOnSelect, grid]);
 
-  const optionsWithKey = useMemo<SelectOptionOrSectionWithKey<Value>[]>(
-    () =>
-      options.map((item, i) => ({
-        ...item,
-        // options key has to be unique to the current list of options,
-        // else we risk of a duplicate key error and end up confusing react
-        // which ultimately fails to call the correct item handlers
-        key: 'options' in item ? item.key ?? `options-${i}` : item.value,
-      })),
-    [options]
-  );
+  const itemsWithKey = useMemo(() => getItemsWithKeys(options), [options]);
 
   const controlDisabled = useMemo(
     () => disabled ?? options?.length === 0,
@@ -107,7 +98,7 @@ function CompactSelect<Value extends React.Key>({
     >
       <List
         {...listProps}
-        items={optionsWithKey}
+        items={itemsWithKey}
         onSectionToggle={onSectionToggle}
         disallowEmptySelection={disallowEmptySelection}
         isOptionDisabled={isOptionDisabled}
@@ -116,12 +107,12 @@ function CompactSelect<Value extends React.Key>({
         sizeLimitMessage={sizeLimitMessage}
         aria-labelledby={triggerId}
       >
-        {(item: SelectOptionOrSection<Value>) => {
+        {(item: SelectOptionOrSectionWithKey<Value>) => {
           if ('options' in item) {
             return (
               <Section key={item.key} title={item.label}>
                 {item.options.map(opt => (
-                  <Item key={opt.value} {...opt}>
+                  <Item {...opt} key={opt.key}>
                     {opt.label}
                   </Item>
                 ))}
@@ -129,11 +120,7 @@ function CompactSelect<Value extends React.Key>({
             );
           }
 
-          return (
-            <Item key={item.value} {...item}>
-              {item.label}
-            </Item>
-          );
+          return <Item {...item}>{item.label}</Item>;
         }}
       </List>
 

static/app/components/compactSelect/list.tsx

@@ -14,6 +14,7 @@ import {ListBox} from './listBox';
 import {SelectOption, SelectOptionOrSectionWithKey, SelectSection} from './types';
 import {
   getDisabledOptions,
+  getEscapedKey,
   getHiddenOptions,
   getSelectedOptions,
   HiddenSectionToggle,
@@ -138,15 +139,15 @@ function List<Value extends React.Key>({
     const disabledKeys = [
       ...getDisabledOptions(items, isOptionDisabled),
       ...hiddenOptions,
-    ].map(String);
+    ].map(getEscapedKey);
 
     if (multiple) {
       return {
         selectionMode: 'multiple',
         disabledKeys,
         // react-aria turns all keys into strings
-        selectedKeys: value?.map(String),
-        defaultSelectedKeys: defaultValue?.map(String),
+        selectedKeys: value?.map(getEscapedKey),
+        defaultSelectedKeys: defaultValue?.map(getEscapedKey),
         disallowEmptySelection,
         allowDuplicateSelectionEvents: true,
         onSelectionChange: selection => {
@@ -171,8 +172,10 @@ function List<Value extends React.Key>({
       selectionMode: 'single',
       disabledKeys,
       // react-aria turns all keys into strings
-      selectedKeys: defined(value) ? [String(value)] : undefined,
-      defaultSelectedKeys: defined(defaultValue) ? [String(defaultValue)] : undefined,
+      selectedKeys: defined(value) ? [getEscapedKey(value)] : undefined,
+      defaultSelectedKeys: defined(defaultValue)
+        ? [getEscapedKey(defaultValue)]
+        : undefined,
       disallowEmptySelection: disallowEmptySelection ?? true,
       allowDuplicateSelectionEvents: true,
       onSelectionChange: selection => {

static/app/components/compactSelect/types.tsx

@@ -36,9 +36,29 @@ export type SelectOptionOrSection<Value extends React.Key> =
   | SelectOption<Value>
   | SelectSection<Value>;
 
+export interface SelectOptionWithKey<Value extends React.Key>
+  extends SelectOption<Value> {
+  /**
+   * Key to identify this section. If not specified, the section's index will
+   * be used.
+   */
+  key: React.Key;
+}
+
+export interface SelectSectionWithKey<Value extends React.Key>
+  extends SelectSection<Value> {
+  /**
+   * Key to identify this section. If not specified, the section's index will
+   * be used.
+   */
+  key: React.Key;
+  options: SelectOptionWithKey<Value>[];
+}
+
 /**
  * Select option/section type used for internal selection manager. DO NOT import in
  * other components. Import `SelectOptionOrSection` instead.
  */
 export type SelectOptionOrSectionWithKey<Value extends React.Key> =
-  SelectOptionOrSection<Value> & {key?: React.Key};
+  | SelectOptionWithKey<Value>
+  | SelectSectionWithKey<Value>;

static/app/components/compactSelect/utils.tsx

@@ -13,9 +13,36 @@ import {
   SelectOption,
   SelectOptionOrSection,
   SelectOptionOrSectionWithKey,
+  SelectOptionWithKey,
   SelectSection,
 } from './types';
 
+export function getEscapedKey<Value extends React.Key | undefined>(value: Value): string {
+  return CSS.escape(String(value));
+}
+
+export function getItemsWithKeys<Value extends React.Key>(
+  options: SelectOption<Value>[]
+): SelectOptionWithKey<Value>[];
+export function getItemsWithKeys<Value extends React.Key>(
+  options: SelectOptionOrSection<Value>[]
+): SelectOptionOrSectionWithKey<Value>[];
+export function getItemsWithKeys<Value extends React.Key>(
+  options: SelectOptionOrSection<Value>[]
+): SelectOptionOrSectionWithKey<Value>[] {
+  return options.map((item, i) => {
+    if ('options' in item) {
+      return {
+        ...item,
+        key: item.key ?? `options-${i}`,
+        options: getItemsWithKeys(item.options),
+      };
+    }
+
+    return {...item, key: getEscapedKey(item.value)};
+  });
+}
+
 /**
  * Recursively finds the selected option(s) from an options array. Useful for
  * non-flat arrays that contain sections (groups of options).
@@ -31,7 +58,7 @@ export function getSelectedOptions<Value extends React.Key>(
     }
 
     // If this is an option
-    if (selection === 'all' || selection.has(String(cur.value))) {
+    if (selection === 'all' || selection.has(getEscapedKey(cur.value))) {
       const {key: _key, ...opt} = cur;
       return acc.concat(opt);
     }
@@ -154,15 +181,15 @@ export function getHiddenOptions<Value extends React.Key>(
  * then this function unselects all of them.
  */
 export function toggleOptions<Value extends React.Key>(
-  optionValues: Value[],
+  optionKeys: Value[],
   selectionManager: SelectionManager
 ) {
   const {selectedKeys} = selectionManager;
   const newSelectedKeys = new Set(selectedKeys);
 
-  const allOptionsSelected = optionValues.every(val => selectionManager.isSelected(val));
+  const allOptionsSelected = optionKeys.every(val => selectionManager.isSelected(val));
 
-  optionValues.forEach(val =>
+  optionKeys.forEach(val =>
     allOptionsSelected ? newSelectedKeys.delete(val) : newSelectedKeys.add(val)
   );
 

tests/acceptance/test_organization_events_v2.py

@@ -550,7 +550,7 @@ class OrganizationEventsV2Test(AcceptanceTestCase, SnubaTestCase):
             self.browser.elements('[aria-label="Filter by operation"]')[0].click()
 
             # select django.middleware
-            self.browser.elements('[data-test-id="django.middleware"]')[0].click()
+            self.browser.elements('[data-test-id="django\\\\.middleware"]')[0].click()
 
             self.browser.snapshot("events-v2 - transactions event detail view - ops filtering")