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

Sanitizer vs ARIA #245

Open
otherdaniel opened this issue Dec 17, 2024 · 4 comments
Open

Sanitizer vs ARIA #245

otherdaniel opened this issue Dec 17, 2024 · 4 comments

Comments

@otherdaniel
Copy link
Collaborator

I'm no longer sure how to handle aria-* attributes. I had contacted our ARIA folks, with the intent of verifying the state of some newly defined attributes, but the question came up of whether aria-* support in Sanitizer might be harmful:

For example, aria-owns effectively re-writes the element hierarchy as seen from a screen reader, so user-generated content that allows aria-owns could essentially "grab" parts of page by declaring its ownership of it. E.g. on a site with review comments, a comment could try to "own" the login box; or if a site displays a purchase price + frees, maybe a user comment could "own" the element with the fees and thus hide them (for ARIA-based readers).

Generally, the aria-* attributes influence how the screen reader (or other ARIA-based tool) builds up its version of the render tree, and we couldn't find a legitimate use case for doing this from user generated content. At least not without explicit page opt-in.


Mapping this to the Sanitizer API threat model, this clearly isn't XSS. So I don't think it violates the "baseline" guarantees the "safe" versions wish to uphold. But I do find it questionable whether we want to allow aria-* in the "safe defaults".

I guess one way one could argue is that e.g. aria-owns creates a capability that regular HTML / MathML / SVG doesn't; you'd have to use scripts to achieve a similar effect; even though aria is purely declarative.

The person I consulted with suggested to flat-out block any aria-* attributes in Sanitizer, until a legimitate use case emerges.

We're unaware of any real-world attack that has worked this way.

@mozfreddyb
Copy link
Collaborator

I think we should do the work and review the capability of ARIA attributes. I think you're making a good case why aria-owns should be disallowed by default (but not within the XSS-prevented-only safety guarantees).

I'm not sure what the other ARIA attributes are about. I have read conflicting takes where either ARIA is not required if your markup is sensible (which would make it OK to be removed by default) or ARIA is required to make web browsing bearable.

I would love to understand what this "if your markup is sensible" would mean specifically and whether it might align with the anti-patterans that the Sanitizer is removing anyway. (An example of bad a11y that comes to mind as obvious overlap would be "using a div as a button", which is not something the Sanitizer would allow anyway because that would require scripts)

@annevk
Copy link
Collaborator

annevk commented Dec 18, 2024

Didn't we already kinda decide that for "default" we would err on the side of caution and just not include things we are not sure about? People can always make a case to add to "default" in the future and it should be (mostly) compatible to do so. Restricting things in the future is much harder.

Especially since ARIA features can have global repercussions, such as with aria-labelledby, I'm inclined to agree with Daniel's colleague to not add these to the list for now.

@mozfreddyb
Copy link
Collaborator

So, the plan for evolving "default" is to

  • be a bit more strict at first and rely on people complaining
  • use these complaints to review & add things
  • ensure (hope?) people don't rely on it being a fixed set that they have once reviewed

I can live with that.

@annevk
Copy link
Collaborator

annevk commented Dec 18, 2024

Yeah. And potentially/ideally we can bucket these complaints in a set of use cases to create other built-in policies that are useful to people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants