Off-by-one error and unchecked parameter in aggregateChecks.js

[zkf]

https://github.com/dequelabs/axe-core/blob/a3fd5f79f0b6fcde310eb46a73cd91e8e76e0aef/lib/core/utils/aggregateChecks.js#L25-L34

On line 32, check.priority is assigned the value 4 if it was 0. AFAICT, valid priority values are in the range 0-3, as they are indices into the axe.constants.results table. https://github.com/dequelabs/axe-core/blob/a3fd5f79f0b6fcde310eb46a73cd91e8e76e0aef/lib/core/utils/aggregateChecks.js#L71

And the reason why I started looking at this module:

Line 26: Array.indexOf() with an undefined argument is unfortunate, as some polyfill libraries do not implement this correctly (For example, in one implementation, [,].indexOf(undefined) === 0 instead of -1). Personally, I also think it isn’t obvious what value to expect from [,].indexOf(undefined).

I suggest adding an additional check for if check.result is valid; and if it is not, set i to -1 directly without using undefined as a lookup key.

I.e., let i = typeof check.result === 'undefined' ? -1 : checkMap.indexOf(check.result);

[zkf]

BTW, these kinds of API issues are bound to pop up because Axe depends on the Javascript API on the page under test, but that API may have been modified by the same page. Have you considered using something like https://github.com/perry-mitchell/archetype which creates an iframe and grabs the original browser API methods from there?

[WilcoFiers]

@zkf Thanks for the report. Would you mind putting together a PR with your fix?

We occasionally see these things come up. It's rare enough that we usually just fix the particular problem and move on. Archetype sounds interesting, I'll put it on my list of technologies to look into.

[zkf]

@WilcoFiers RE: PR. Will do.

I had a closer look at what I first thought was an off-by-one error. Changing 4 - check.priority; into 3 - check.priority; produced a heap of errors :tired_face:

If I understand it correctly, the problem is actually a case of using integer arithmetic where it shouldn’t be used. It seems that the point of “reversing the outcome” for the none type is actually to swap FAIL and PASS. Thus, the arithmetic breaks if the outcome is NA (integer value: 0).

NA: 0 PASS: 1 CANTTELL: 2 FAIL: 3

Thus; 4 - FAIL = PASS, 4 - CANTTELL = CANTTELL, 4 - PASS = FAIL, 4 - NA = 4, which is an invalid priority value.

If I am correct, I would suggest changing the code at lines 30–33 into

        if (type === 'none') {
            // For none, reverse pass and fail outcomes
            if (check.priority === axe.constants.PASS_PRIO) {
                check.priority = axe.constants.FAIL_PRIO
            } else if (check.priority === axe.constants.FAIL_PRIO) {
                check.priority = axe.constants.PASS_PRIO
            }
        }

In addition, maybe the comment should explain exactly why the outcomes have to be reversed?

[WilcoFiers]

Whoops, closed prematurely. Yes I think you're right, would be better to swap instead of do the reverse thingy. That's a good point.

[marcysutton]

Looks like this was fixed with #970, so I'm going to close it.