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

Stop using setAttribute for ARIA #3378

Closed
joeyparrish opened this issue Apr 29, 2021 · 1 comment
Closed

Stop using setAttribute for ARIA #3378

joeyparrish opened this issue Apr 29, 2021 · 1 comment
Labels
component: UI The issue involves the Shaka Player UI status: archived Archived and locked; will not be updated type: code health A code health issue
Milestone

Comments

@joeyparrish
Copy link
Member

Instead of using e.g. foo.setAttribute('aria-label', 'foo'), we should be using foo.ariaLabel = 'foo'. The liberal use of setAttribute confuses an internal Google tool that hunts for potential XSS vulnerabilities.

The checks we are having issues with are meant to find an attacker-controllable way to set the 'src' attribute on a <script> tag, but in cases where the compiler doesn't know what key we're using or doesn't know the type of the element, it will throw errors and refuse to build our source code.

@joeyparrish joeyparrish added type: code health A code health issue component: UI The issue involves the Shaka Player UI labels Apr 29, 2021
@joeyparrish joeyparrish added this to the v3.2 milestone Apr 29, 2021
joeyparrish added a commit that referenced this issue Apr 30, 2021
Some internal Google tools that are meant to find XSS vulnerabilities
complain about some parts of Shaka Player.  In particular:

 - Using setAttribute instead of a src property
 - Using setAttribute with a variable key
 - Using URL.createObjectURL

This resolves these internal XSS checks.  As far as I can tell, there
are no serious XSS risks from any of these, as the complaining checks
are meant to catch things that could be executed as scripts.  None of
these user-controlled inputs are treated as such, but it seems that
the compiler involved can't tell that.

This goes part of the way toward solving #3378.

Change-Id: I302b9eb56a374854c9b8525b21960ef64fd386f1
shaka-bot pushed a commit that referenced this issue Apr 30, 2021
Some internal Google tools that are meant to find XSS vulnerabilities
complain about some parts of Shaka Player.  In particular:

 - Using setAttribute instead of a src property
 - Using setAttribute with a variable key
 - Using URL.createObjectURL

This resolves these internal XSS checks.  As far as I can tell, there
are no serious XSS risks from any of these, as the complaining checks
are meant to catch things that could be executed as scripts.  None of
these user-controlled inputs are treated as such, but it seems that
the compiler involved can't tell that.

This goes part of the way toward solving #3378.

Change-Id: I302b9eb56a374854c9b8525b21960ef64fd386f1
@joeyparrish joeyparrish modified the milestones: v3.2, v3.3 Jul 7, 2021
shaka-bot pushed a commit that referenced this issue Jul 7, 2021
The ARIAMixin interface mixin lets users apply ARIA attributes to
DOM elements without having to rely on setAttribute and getAttribute.
It is one of our goals to switch to using that interface.
Unfortunately, it is not implemented on Firefox, so a polyfill is
necessary for that platform, before we can start using it.

Issue #3378

Change-Id: Ia878900d75c7c2c04613360baacb4524774ac746
@theodab
Copy link
Collaborator

theodab commented Jul 7, 2021

With Alvaro's PR (#3489) merged, the only ARIA property we are assigning with setAttribute is now aria-describedby, which is set in the demo. Sadly, that attribute does not have a variable equivalent in the ARIAMixin interface mixin, so we can't replace it.
So this is as good as we are going to get, I think.

@theodab theodab closed this as completed Jul 7, 2021
@joeyparrish joeyparrish modified the milestones: v3.3, v3.2 Jul 8, 2021
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Sep 5, 2021
@shaka-project shaka-project locked and limited conversation to collaborators Sep 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: UI The issue involves the Shaka Player UI status: archived Archived and locked; will not be updated type: code health A code health issue
Projects
None yet
Development

No branches or pull requests

3 participants