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

VirtualNode props getter throws in an AngularJS v1 edge case with hidden input elements #4316

Closed
dbjorge opened this issue Jan 31, 2024 · 0 comments · Fixed by #4317
Closed
Labels
fix Bug fixes

Comments

@dbjorge
Copy link
Contributor

dbjorge commented Jan 31, 2024

We debugged a customer issue today involving an axe-core crash when running against a specific customer page. The page in question was using AngularJS 1.7.8 and included a DOM snippet with a structure similar to the following:

<div id="outer">
  <div class="hidden"></div>
  <input type="hidden" id="field_1" value="0" autocomplete="off">





  <input type="hidden" id="field_2" value="1.2.3.4 (value=)" autocomplete="off">
</div>

AngularJS v1.7.8 includes this hacky workaround that is intended to be applied to <input type="hidden"> elements to override the way their value property getters work. However, in the customer page, for some reason this overriden value getter ended up applied not just to the input elements, but also to some of the text Nodes representing the whitespace in between the two hidden inputs.

This causes axe-core to throw an error in /lib/core/base/virtual-node/virtual-node.js when it attempts to read the value of the text nodes in question - it expects that this operation should just harmlessly return undefined for a text node, but the AngularJS override ends up trying to invoke theTextNode.getAttribute('value'), which throws because it's a text node.

Unfortunately, we were unable to extract a minimal/standalone repro for this issue - the above code is structurally similar to what we saw in the customer's page, but not enough on its own to trigger the issue. We know that the override in question isn't present in newer/supported versions of angular, so we think it's probably not a good use of time to further debug the root cause of the override being applied to the unexpected nodes; it's unlikely that the issue would be fixed even if we identified the root cause. Instead, we can mitigate the customer impact by just being more defensive about when we read the value property - it (and a few other props) shouldn't need to be read at all for non-input elements.

@dbjorge dbjorge added the fix Bug fixes label Jan 31, 2024
dbjorge added a commit that referenced this issue Feb 1, 2024
…de types (#4317)

Updates VirtualNode's `props` initialization path to avoid reading
properties that we know based on `nodeType` won't be present anyway.
This should mitigate #4316 by avoiding reading the problematic `value`
prop present on certain text nodes.

I also cleaned up some missing test coverage in the impacted code.

Closes: #4316
straker pushed a commit that referenced this issue Feb 5, 2024
…de types (#4317)

Updates VirtualNode's `props` initialization path to avoid reading
properties that we know based on `nodeType` won't be present anyway.
This should mitigate #4316 by avoiding reading the problematic `value`
prop present on certain text nodes.

I also cleaned up some missing test coverage in the impacted code.

Closes: #4316
WilcoFiers added a commit that referenced this issue Mar 25, 2024
##
[4.9.0](v4.8.4...v4.9.0)
(2024-03-25)

### Features

- adding the wcag131 tag to the aria-hidden-body rule
([#4349](#4349))
([dd4c3c3](dd4c3c3)),
closes [#4315](#4315)
- **checks:** deprecate aria-busy check
([#4356](#4356))
([be0b555](be0b555)),
closes [#4347](#4347)
[#4340](#4340)
- **color:** add color channel values and luminosity, saturation, clip
functions ([#4366](#4366))
([9e70199](9e70199)),
closes
[/github.com//pull/4365/files#r1517706612](https://github.com/dequelabs//github.com/dequelabs/axe-core/pull/4365/files/issues/r1517706612)
- **i18n:** add Greek Translations
([#3836](#3836))
([3ea9a48](3ea9a48))
- **i18n:** Add Italian translation
([#4344](#4344))
([de1baa9](de1baa9))
- **i18n:** Add Simplified Chinese translation
([#4379](#4379))
([bda7c8d](bda7c8d))
- **i18n:** Add Taiwanese Mandarin translation
([#4299](#4299))
([c5e11de](c5e11de))

### Bug Fixes

- Add LICENSE-3RD-PARTY.txt file
([#4304](#4304))
([daa0fe6](daa0fe6))
- add Object.values polyfill for node <=6
([#4274](#4274))
([5eb867b](5eb867b))
- **aria-required-children:** avoid confusing aria-busy message in
failures ([#4347](#4347))
([591607d](591607d)),
closes [#fail13](https://github.com/dequelabs/axe-core/issues/fail13)
[#4340](#4340)
- avoid reading element-specific node properties of non-element node
types ([#4317](#4317))
([b853b18](b853b18)),
closes [#4316](#4316)
[#4316](#4316)
- **color-contrast:** handle text that is outside `overflow: hidden`
ancestor ([#4357](#4357))
([bdb7300](bdb7300)),
closes [#4253](#4253)
- **color-contrast:** support color blend modes hue, saturation, color,
luminosity ([#4365](#4365))
([7ae4761](7ae4761))
- **d.ts:** RawNodesResult issues
([#4229](#4229))
([d660518](d660518))
- **d.ts:** RunOptions.reporter can be any string
([#4218](#4218))
([e53f5c5](e53f5c5))
- **i18n:** update Italian translations
([#4377](#4377))
([4d65d4b](4d65d4b))
- **listitem:** clarify roleNotValid message
([#4374](#4374))
([0f8a9af](0f8a9af))
- **scrollable-region-focusable:** missing wcag213 tag
([#4201](#4201))
([0080a72](0080a72))
- **target-size:** always pass 10x targets (avoid perf bottleneck)
([#4376](#4376))
([be327c4](be327c4))
- **target-size:** do not crash for nodes with many overlapping widgets
([#4373](#4373))
([1dbea83](1dbea83)),
closes [#4359](#4359)
[#4359](#4359)
[#4360](#4360)
- **utils/get-selector:** ignore 'xmlns' attribute when generating a
selector ([#4303](#4303))
([938b411](938b411))

This PR was opened by a robot 🤖 🎉
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fixes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant