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

feat(core): add missing ARIA attributes to html sanitizer #29685

Closed
wants to merge 3 commits into from

Conversation

@MartinMa
Copy link
Contributor

commented Apr 3, 2019

Allow ARIA attributes from the WAI-ARIA 1.1 spec, which were stripped by the htmlSanitizer.

Closes #26815

PR Checklist

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

Aria attributes like aria-label are being stripped by the html sanitizer as unsafe html.

Issue Number: #26815

What is the new behavior?

aria-* attributes are treated as safe html and are not being stripped by the html sanitizer (for example when used within [innerHTML]).

Does this PR introduce a breaking change?

  • No

@MartinMa MartinMa requested review from angular/fw-core as code owners Apr 3, 2019

@googlebot googlebot added the cla: yes label Apr 3, 2019

@MartinMa MartinMa force-pushed the MartinMa:security-fix-mm branch from 1f4e2be to 33877c6 Apr 3, 2019

@ngbot ngbot bot added this to the needsTriage milestone Apr 4, 2019

@mhevery mhevery self-assigned this Apr 11, 2019

@ngbot

This comment has been minimized.

Copy link

commented Apr 11, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: integration_test" is failing
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@MartinMa

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Hi, the "ci/circleci: integration_test" is failing.

This seems to be the problem:

Commit 33877c6 uncompressed bundle exceeded expected size by >1% (expected: 177585, actual: 179726).

Is there anything I can do to fix this?

Edit: I tried to restart the CI job, but I don't seem to have permission.

@IgorMinar
Copy link
Member

left a comment

@rjamet do you have any security concerns about any of these attributes? thanks!

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@MartinMa the tests are failing because you added too much "weight" to the framework which has a negative impact on the payload size.

We'll need to discuss whether this is just something we have to swallow (and update the limit in the test), or if there are other measures that could allow these attributes while not increase the size. One to consider is allowing any "aria-" prefixed attribute without listing them specifically (this seems like a bad idea to me but @rjamet should weigh in on that option as well).

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@MartinMa you brought up an interesting issue that has competing security, accessibility, and performance requirements. It's going to be interesting to see how we resolve it. 😄

@rjamet

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

No concern for security: I think the worse effect you'd get is something phishing-like, which is expected with a sanitizer, or confusing navigation/interactions (especially aria-activedescendant), which is also reasonable to expect from adversarial sanitized HTML.

@MartinMa

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@IgorMinar Interesting considerations.

A regex solution could be more lightweight. I think you'd probably have to change this line:

if (!VALID_ATTRS.hasOwnProperty(lower)) {

to this:

if (!(VALID_ATTRS.hasOwnProperty(lower) || ARIA_REGEX.test(lower))) {

while ARIA_REGEX could be one of the following

const ARIA_REGEX = /^aria-[a-z]*$/; // 1
const ARIA_REGEX = /^aria-[a-z]+$/; // 2
const ARIA_REGEX = /^aria-[a-z]{4,16}$/; // 3
const ARIA_REGEX = /^aria-(a(ctivedescendant|tomic|utocomplete)|busy|c(hecked|o(l(count|index|span)|ntrols)|urrent)|d(e(scribedby|tails)|isabled|ropeffect)|e(rrormessage|xpanded)|flowto|grabbed|h(aspopup|idden)|invalid|keyshortcuts|l(abel(|ledby)|evel|ive)|m(odal|ulti(line|selectable))|o(rientation|wns)|p(laceholder|osinset|ressed)|r(e(adonly|levant|quired)|oledescription|ow(count|index|span))|s(e(lected|tsize)|ort)|value(max|min|now|text))$/; // 4

I think something like 4 is a nightmare. 3 would cover the longest keyword aria-activedescendant, the short ones like aria-busy and everything in between.

Personally however, I am a fan of extending VALID_ATTRS as layed out in this PR and being strict about the attributes. But I could change the code to use a regex if desired?

If i understand correctly. Security is no concern with both approaches (strict list vs. relaxed regex)

Another question: Does the metric "uncompressed bundle size" count comments in?

@benlesh

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@MartinMa ... This PR is really close to getting merged. Do you think you can figure out what is going wrong with the failing test in CI? It might be that it needs a rebase, I'm not sure, I didn't dig into it.

👍 Good luck

@MartinMa MartinMa force-pushed the MartinMa:security-fix-mm branch from 33877c6 to c10ca12 Apr 23, 2019

@MartinMa

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@benlesh I rebased the commit on top of master but the integration test is still failing because of:

Commit c10ca12 uncompressed bundle exceeded expected size by >1% (expected: 177585, actual: 179825).
If this is a desired change, please update the size limits in file '/home/circleci/ng/integration/../integration/_payload-limits.json'.

We were discussing if the payload limits in this tests should be adjusted accordingly ... (see the earlier comments).

Please tell me if there is anything else I can do.

@benlesh benlesh requested a review from IgorMinar Apr 23, 2019

@benlesh

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Paging @IgorMinar for review... Should he update the payload size?

@IgorMinar
Copy link
Member

left a comment

Alright.. let's update the payload size limit and get this merged. We will need to save some code somewhere to make up for the size increase, but we shouldn't be sacrificing accessibility to keep Angular small.

Let's keep the code as is for readability.

perf musings: We might want to try to improve upon it in a separate PR by constructing the array and adding the duplicated "aria-" prefix to all the strings via a map (e.g. 'multiselectable,orientation,owns'.split(',').map(s => aria-${s})). I'm not even sure if that would be a clear perf win because of the extra eval cost, but since we are evaling these strings anyway it seems plausible that it would be faster - we'd need to test on https://www.webpagetest.org/easy to verify but doing so accurately will be hard. (keep in mind that even though gzip/brotli optimizes away the duplication the js parser still has to process the uncompressed payload so that's the part we are trying to optimize and it's not clear whether we should trade of js parser time or js eval time here.

Thanks @rjamet for the security review.

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

merge-assistance: ben can you please update the payload size yourself and run a presubmit to expedite the merge? thanks

MartinMa and others added some commits Apr 3, 2019

feat(core): add missing ARIA attributes to html sanitizer
Allow ARIA attributes from the WAI-ARIA 1.1 spec which were stripped by the htmlSanitizer.

Closes #26815

@IgorMinar IgorMinar force-pushed the MartinMa:security-fix-mm branch from c10ca12 to 09b24e4 Apr 25, 2019

@MartinMa MartinMa requested a review from angular/fw-integration as a code owner Apr 25, 2019

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Apr 25, 2019

@IgorMinar IgorMinar added cla: yes and removed cla: no labels Apr 25, 2019

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Apr 25, 2019

@IgorMinar IgorMinar added cla: yes and removed cla: no labels Apr 25, 2019

@googlebot

This comment has been minimized.

Copy link

commented Apr 25, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

feat(core): add missing ARIA attributes to html sanitizer (angular#29685
)

Allow ARIA attributes from the WAI-ARIA 1.1 spec which were stripped by the htmlSanitizer.

Closes angular#26815

PR Close angular#29685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.