Skip to content

Commit b3d35e6

Browse files
committed
test(frontend): pin the unrecognised-permission-value escaping (F2)
Adversarial review of this PR confirmed the escaping in buildActionOptions/buildResourceOptions is correct today, but nothing guarded it going forward. Both functions interpolate a stored permission's raw action/resource string into an HTML attribute (value="...") and a text label ("not recognized by this form...") whenever the value falls outside ALL_ACTIONS/ALL_RESOURCES -- the same "API string reaching innerHTML" pattern #1727 fixed twice elsewhere in this codebase a few hours earlier. Adds a test.each over five breakout payloads (element injection, attribute injection onto the <option> tag itself, a raw <script>, a single-quote-context breakout, and a closing-tag option injection). Each case asserts the strong properties rather than that the string appears somewhere: no element was parsed out of the payload, the selected <option> gained no attributes beyond value/selected, its label has zero child elements, and the value survives byte-identical through the DOM and through an actual save. Verified the tests have teeth: temporarily stripped escapeHtml from the action-side fallback option and reran -- 4 of 5 cases failed with the exact predicted symptoms (extra attributes leaking through on the attribute-injection payload, an extra <option> from the closing-tag payload, etc). The fifth (single-quote breakout) correctly does not fail either way, since the surrounding attribute is double-quoted and a lone "'" cannot close it regardless of escaping -- kept as a defense-in-depth case rather than dropped, since it still asserts the escaping holds under that payload shape. Reverted the temporary change; all 10 tests in this file pass against the real code.
1 parent 4149f6b commit b3d35e6

1 file changed

Lines changed: 81 additions & 0 deletions

File tree

‎frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ jest.mock('../confirmDialog', () => ({
5252
import * as api from '../api';
5353
import * as groupState from '../groups/state';
5454
import * as groupModals from '../groups/groupModals';
55+
import { ALL_ACTIONS, ALL_RESOURCES } from '../permissions';
5556

5657
// The exact seeded Purchaser group permission set (PURCHASER_PERMS in
5758
// permissions.generated.ts), reproduced here as a fixture so the test
@@ -202,4 +203,84 @@ describe('regression: group edit no longer drops/widens unrepresentable permissi
202203
expect(actionSelect.selectedOptions[0]?.textContent).toContain('not recognized');
203204
expect(resourceSelect.selectedOptions[0]?.textContent).toContain('not recognized');
204205
});
206+
207+
// F2 (adversarial review of this PR): the escaping above is correct today
208+
// but nothing was pinning it. buildActionOptions/buildResourceOptions in
209+
// groupModals.ts interpolate the raw stored permission value into both an
210+
// HTML attribute (`value="..."`) and a text label (`⚠ ... (not
211+
// recognized...)`) for any value outside ALL_ACTIONS/ALL_RESOURCES --
212+
// exactly the "API string reaching innerHTML" pattern #1727 fixed twice
213+
// elsewhere in this codebase a few hours before this file was written.
214+
// These five breakout payloads (from the reviewer) pin the strong
215+
// properties rather than just "the string appears somewhere": no element
216+
// was parsed out of the payload, the selected <option> gained no
217+
// attributes beyond value/selected, its label has zero child elements,
218+
// and the value is preserved byte-for-byte through the DOM and through
219+
// an actual save.
220+
describe('F2: unrecognised-value option escaping holds under XSS breakout payloads', () => {
221+
const BREAKOUT_PAYLOADS: Array<[string, string]> = [
222+
['element injection', '"><img src=x onerror=alert(1)>'],
223+
['attribute injection onto the <option> tag', '" autofocus onfocus=alert(1) x="'],
224+
['raw script tag', '<script>alert(1)</script>'],
225+
['single-quote context breakout', "'><svg onload=alert(1)>"],
226+
['closing-tag option injection', '</option><option value="x" selected>evil</option>'],
227+
];
228+
229+
test.each(BREAKOUT_PAYLOADS)('%s does not inject markup and round-trips unchanged', async (_label, payload) => {
230+
const group: api.APIGroup = {
231+
id: 'breakout-group-id',
232+
name: 'Breakout',
233+
description: 'Old description',
234+
permissions: [{ action: payload, resource: payload }],
235+
created_at: '2024-01-01T00:00:00Z',
236+
};
237+
238+
(api.getGroup as jest.Mock).mockResolvedValue(group);
239+
await groupModals.openEditGroupModal(group.id);
240+
241+
const actionSelect = document.querySelector('.perm-action') as HTMLSelectElement;
242+
const resourceSelect = document.querySelector('.perm-resource') as HTMLSelectElement;
243+
244+
// No breakout payload parsed as a live element anywhere in the modal.
245+
expect(document.querySelectorAll('img, script, svg, iframe, style').length).toBe(0);
246+
247+
// No extra <option> was smuggled in via the closing-tag breakout: the
248+
// action select is ALL_ACTIONS + the empty placeholder + the one
249+
// fallback option for this unrecognised value; the resource select is
250+
// ALL_RESOURCES + the one fallback (it has no placeholder).
251+
expect(actionSelect.options.length).toBe(ALL_ACTIONS.length + 1 + 1);
252+
expect(resourceSelect.options.length).toBe(ALL_RESOURCES.length + 1);
253+
254+
const selectedAction = actionSelect.selectedOptions[0];
255+
const selectedResource = resourceSelect.selectedOptions[0];
256+
expect(selectedAction).toBeDefined();
257+
expect(selectedResource).toBeDefined();
258+
259+
// The attribute-injection payload gained no new attributes on the
260+
// <option> element itself: exactly value + selected, nothing else
261+
// (no autofocus/onfocus/etc smuggled in).
262+
expect(Array.from(selectedAction!.attributes).map(a => a.name).sort()).toEqual(['selected', 'value']);
263+
expect(Array.from(selectedResource!.attributes).map(a => a.name).sort()).toEqual(['selected', 'value']);
264+
265+
// The label rendered as text, not markup: zero child elements.
266+
expect(selectedAction!.children.length).toBe(0);
267+
expect(selectedResource!.children.length).toBe(0);
268+
269+
// The value round-trips byte-identically through the DOM.
270+
expect(actionSelect.value).toBe(payload);
271+
expect(resourceSelect.value).toBe(payload);
272+
273+
// ...and through an actual save: the exact payload reaches the API
274+
// call, neither dropped nor mangled by an unrelated description edit.
275+
(document.getElementById('group-description') as HTMLTextAreaElement).value = 'New description';
276+
const event = { preventDefault: jest.fn() } as unknown as Event;
277+
await groupModals.saveGroup(event);
278+
279+
expect(api.updateGroup).toHaveBeenCalledWith('breakout-group-id', {
280+
name: 'Breakout',
281+
description: 'New description',
282+
permissions: [{ action: payload, resource: payload }],
283+
});
284+
});
285+
});
205286
});

0 commit comments

Comments
 (0)