Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid problems from element IDs that exist on object prototype #4060

Merged
merged 3 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions lib/commons/aria/get-accessible-refs.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,26 @@ function cacheIdRefs(node, idRefs, refAttrs) {
if (node.hasAttribute) {
if (node.nodeName.toUpperCase() === 'LABEL' && node.hasAttribute('for')) {
const id = node.getAttribute('for');
idRefs[id] = idRefs[id] || [];
idRefs[id].push(node);
if (!idRefs.has(id)) {
idRefs.set(id, [node]);
} else {
idRefs.get(id).push(node);
}
}

for (let i = 0; i < refAttrs.length; ++i) {
const attr = refAttrs[i];
const attrValue = sanitize(node.getAttribute(attr) || '');

if (!attrValue) {
continue;
}

const tokens = tokenList(attrValue);
for (let k = 0; k < tokens.length; ++k) {
idRefs[tokens[k]] = idRefs[tokens[k]] || [];
idRefs[tokens[k]].push(node);
for (const token of tokenList(attrValue)) {
if (!idRefs.has(token)) {
idRefs.set(token, [node]);
} else {
idRefs.get(token).push(node);
}
}
}
}
Expand All @@ -50,22 +54,21 @@ function getAccessibleRefs(node) {
let root = getRootNode(node);
root = root.documentElement || root; // account for shadow roots

const idRefsByRoot = cache.get('idRefsByRoot', () => new WeakMap());
const idRefsByRoot = cache.get('idRefsByRoot', () => new Map());

let idRefs = idRefsByRoot.get(root);
if (!idRefs) {
idRefs = {};
idRefs = new Map();
idRefsByRoot.set(root, idRefs);

const refAttrs = Object.keys(standards.ariaAttrs).filter(attr => {
const { type } = standards.ariaAttrs[attr];
return idRefsRegex.test(type);
});

cacheIdRefs(root, idRefs, refAttrs);
}

return idRefs[node.id] || [];
return idRefs.get(node.id) ?? [];
}

export default getAccessibleRefs;
8 changes: 7 additions & 1 deletion lib/core/utils/find-by.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@
*/
function findBy(array, key, value) {
if (Array.isArray(array)) {
return array.find(obj => typeof obj === 'object' && obj[key] === value);
return array.find(
obj =>
obj !== null &&
typeof obj === 'object' &&
Object.hasOwn(obj, key) &&
obj[key] === value
);
}
}

Expand Down
13 changes: 9 additions & 4 deletions lib/core/utils/selector-cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,12 @@ function findMatchingNodes(expression, selectorMap, shadowId) {
nodes = selectorMap['*'];
} else {
if (exp.id) {
// a selector must match all parts, otherwise we can just exit
// early
if (!selectorMap[idsKey] || !selectorMap[idsKey][exp.id]?.length) {
// a selector must match all parts, otherwise we can just exit early
if (
!selectorMap[idsKey] ||
!Object.hasOwn(selectorMap[idsKey], exp.id) ||
!selectorMap[idsKey][exp.id]?.length
) {
return;
}

Expand Down Expand Up @@ -176,7 +179,9 @@ function getSharedValues(a, b) {
* @param {Object} map
*/
function cacheSelector(key, vNode, map) {
map[key] = map[key] || [];
if (!Object.hasOwn(map, key)) {
map[key] = [];
}
map[key].push(vNode);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/standards/html-elms.js
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ const htmlElms = {
'menuitem',
'menuitemcheckbox',
'menuitemradio',
'meter',
'meter',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??? How the heck did that happen

'option',
'progressbar',
'radio',
Expand Down
28 changes: 28 additions & 0 deletions test/commons/aria/get-accessible-refs.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,34 @@ describe('aria.getAccessibleRefs', function () {
assert.deepEqual(getAccessibleRefs(node), [ref]);
});

describe('when JavaScript object names are used as IDs', function () {
const ids = [
'prototype',
'constructor',
'__proto__',
'Element',
'nodeName',
'valueOf',
'toString'
];
for (const id of ids) {
it(`does not break with id="${id}"`, function () {
setLookup({ 'aria-bar': { type: 'idrefs' } });
fixture.innerHTML = `<div id="ref" aria-bar="${ids.join(
' '
)}"></div><i id="${id}"></i></b>`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
)}"></div><i id="${id}"></i></b>`;
)}"></div><i id="${id}"></i>`;


var node = document.getElementById(id);
var ref = document.getElementById('ref');
assert.deepEqual(
getAccessibleRefs(node),
[ref],
`Not equal for ID ${id}`
);
});
}
});

(shadowSupport ? it : xit)('works inside shadow DOM', function () {
setLookup({ 'aria-bar': { type: 'idref' } });
fixture.innerHTML = '<div id="foo"></div>';
Expand Down
32 changes: 32 additions & 0 deletions test/core/utils/find-by.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,36 @@ describe('axe.utils.findBy', function () {
it('should not throw if passed falsey first parameter', function () {
assert.isUndefined(axe.utils.findBy(null, 'id', 'macaque'));
});

it('ignores any non-object elements in the array', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes even without your changes, might need a different test / setup.

var array = [
{
id: 'monkeys',
foo: 'bar'
},
'bananas',
true,
null,
123
];

assert.equal(axe.utils.findBy(array, 'id', 'monkeys'), array[0]);
});

it('only looks at owned properties', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test also passes without your changes. For this test to work you'll need to create an object whos prototype parent defines the property in question. Object properties won't work since you can't find them by value.

Something like this:

const parent = {
  value: 'bananas'
}
const child = Object.create(parent);
var array = [
  child,
  {
    id: 'monkeys',
    value: 'bananas'
  }
];

assert.deepEqual(axe.utils.findBy(array, 'value', 'bananas'), {
  id: 'monkeys',
  value: 'bananas'
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, yeah that sure wasn't my best work ever!

var array = [
{
id: 'monkeys',
Constructor: 'monkeys'
},
{
id: 'bananas'
}
];

assert.deepEqual(axe.utils.findBy(array, 'Constructor', 'monkeys'), {
id: 'monkeys',
Constructor: 'monkeys'
});
});
});
23 changes: 23 additions & 0 deletions test/core/utils/selector-cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,29 @@ describe('utils.selector-cache', function () {

assert.lengthOf(Object.keys(map), 0);
});

describe('with javascripty attribute selectors', function () {
const terms = [
'prototype',
'constructor',
'__proto__',
'Element',
'nodeName',
'valueOf',
'toString'
];
for (const term of terms) {
it(`works with ${term}`, function () {
fixture.innerHTML = `<div id="${term}" class="${term}" aria-label="${term}"></div>`;
const vNode = new axe.VirtualNode(fixture.firstChild);
const map = {};
cacheNodeSelectors(vNode, map);
assert.deepEqual(map['[id]'], [vNode]);
assert.deepEqual(map['[class]'], [vNode]);
assert.deepEqual(map['[aria-label]'], [vNode]);
});
}
});
});

describe('getNodesMatchingExpression', function () {
Expand Down
6 changes: 3 additions & 3 deletions test/integration/full/all-rules/all-rules.html
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@
>
<banner></banner>
<main>
<div accesskey="B"></div>
<div accesskey="B" id="__proto__"></div>
<map>
<area href="#" id="pass1" alt="monkeys" />
</map>
<div aria-label="foo">Foo</div>
<div aria-label="foo" id="constructor">Foo</div>
<div role="contentinfo"></div>
<div role="link">Home</div>
<div role="dialog" aria-label="Cookies"></div>
Expand All @@ -51,7 +51,7 @@
<div role="treeitem">Item</div>
</div>
<audio id="caption"><track kind="captions" /></audio>
<input autocomplete="username" />
<input autocomplete="username" id="toString" />
<p id="fail1" style="line-height: 1.5 !important">Banana error</p>
<p><blink>text</blink></p>
<button id="text">Name</button>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,15 @@ <h1 id="pass-h1-valid-role" role="none"></h1>
<div id="fail-dpub-4" role="doc-noteref">ok</div>
<!-- images -->
<div id="fail-dpub-5" role="doc-cover">ok</div>
<img src="#" role="meter" aria-valuenow="0" aria-valuemin="0" aria-valuemax="100" alt="test" id="pass-img-valid-role-meter">
<img
src="#"
role="meter"
aria-valuenow="0"
aria-valuemin="0"
aria-valuemax="100"
alt="test"
id="pass-img-valid-role-meter"
/>
<img role="doc-cover" aria-label="foo" id="pass-img-valid-role-aria-label" />
<img role="menuitem" title="bar" id="pass-img-valid-role-title" />
<div id="image-baz">hazaar</div>
Expand Down