fix(compiler): adding the inert property to the "SCHEMA" array #53148

physicx237 · 2023-11-24T12:05:50Z

This change allows template binding "inert" attribute with the following syntax: [inert]="isInert"

Fixes #51879

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #51879

What is the new behavior?

Possibility of template binding of the inert attribute with the following syntax: [inert]="isInert"

Does this PR introduce a breaking change?

Yes
No

Other information

JeanMeche · 2023-11-24T12:42:24Z

Cf the file header

// DO NOT EDIT THIS DOM SCHEMA WITHOUT A SECURITY REVIEW!
//
// Newly added properties must be security reviewed and assigned an appropriate SecurityContext in
// dom_security_schema.ts. Reach out to mprobst & rjamet for details.

I'm marking it as blocked until this is done.

packages/compiler/src/schema/dom_element_schema_registry.ts

JoostK · 2023-11-24T14:25:19Z

packages/compiler/src/schema/dom_security_schema.ts

@@ -31,6 +31,7 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {
      'iframe|srcdoc',
      '*|innerHTML',
      '*|outerHTML',
+      '*|inert',


I don't think this is needed; inert is not a property that needs an HTML security context.

+1, this should be removed.

physicx237 · 2023-11-24T14:58:30Z

JeanMeche said:
"See file header

// DO NOT EDIT THIS HOUSE DIAGRAM WITHOUT A SECURITY CHECK!
//
// Newly added properties must be security checked and match the corresponding SecurityContext in
// dom_security_schema.ts. More information on the mprost & rjamet website.

I mark it as blocked until this is done."

Should I leave the "inert" attribute in the security context or is it not needed?
What's the best way to proceed?

JoostK · 2023-11-24T15:18:55Z

The intent of that comment is to make people aware of potential security implications of the schema definitions. For example, Angular must sanitize data when binding to the innerHTML property, but not when binding to the inert property since that will only be interpreted as boolean value, not rendered as HTML.

Hence, changes to the schema require a security review to avoid mistakenly introducing a vulnerability in the framework. For inert though, I cannot think of a reason why it would be security sensitive.

physicx237 · 2023-12-13T14:45:18Z

Cf the file header

// DO NOT EDIT THIS DOM SCHEMA WITHOUT A SECURITY REVIEW!
//
// Newly added properties must be security reviewed and assigned an appropriate SecurityContext in
// dom_security_schema.ts. Reach out to mprobst & rjamet for details.

I'm marking it as blocked until this is done.

How to reach out to mbprost & rjamet for details?

jelbourn · 2023-12-13T18:05:49Z

Ah, that comment is out of date (mbprost & rjamet aren't involved with Angular any longer). @bjarkler I believe inert should be completely safe here as a boolean attribute, but I'll add you for FYI anyway since it's part of the process for changing the schema.

LGTM insofar as adding inert in general, just need to address the comments from @JoostK

bjarkler · 2023-12-13T18:36:43Z

LGTM, thanks!

physicx237 · 2023-12-13T18:57:21Z

Thanks for your feedback!

JoostK · 2023-12-13T21:57:17Z

Could you please squash the commits? The revert won't be auto squashed otherwise

physicx237 · 2023-12-13T21:58:03Z

Yes, sure

JoostK · 2023-12-13T22:33:59Z

Looks like something went wrong, there's now five commits.

This change allows template binding "inert" attribute with the following syntax: [inert]="isInert" Fixes #51879 fixup! fix(compiler): adding the inert property to the "SCHEMA" array revert: "fixup! fix(compiler): adding the inert property to the "SCHEMA" array" This reverts commit b637b7c. This commit is being reverted because the inert property is safe as a boolean attribute

JoostK

Thanks for the updates!

physicx237 · 2023-12-14T07:09:36Z

And thank you!

jelbourn

LGTM

Reviewed-for: fw-security

pkozlowski-opensource

LGTM

Reviewed-for: fw-security

alxhub · 2024-02-21T23:14:04Z

This PR was merged into the repository by commit dba3e0b.

This change allows template binding "inert" attribute with the following syntax: [inert]="isInert" Fixes #51879 fixup! fix(compiler): adding the inert property to the "SCHEMA" array revert: "fixup! fix(compiler): adding the inert property to the "SCHEMA" array" This reverts commit b637b7c. This commit is being reverted because the inert property is safe as a boolean attribute PR Close #53148

angular-automatic-lock-bot · 2024-03-24T00:10:24Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pullapprove bot requested review from alxhub, jelbourn, josephperrott and pkozlowski-opensource November 24, 2023 12:06

JeanMeche added the state: blocked label Nov 24, 2023

JoostK reviewed Nov 24, 2023

View reviewed changes

packages/compiler/src/schema/dom_element_schema_registry.ts Show resolved Hide resolved

physicx237 requested a review from JoostK November 24, 2023 14:17

JoostK reviewed Nov 24, 2023

View reviewed changes

physicx237 requested a review from JoostK November 24, 2023 15:01

JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Nov 25, 2023

pkozlowski-opensource added the area: compiler Issues related to `ngc`, Angular's template compiler label Nov 27, 2023

ngbot bot added this to the Backlog milestone Nov 27, 2023

physicx237 closed this Dec 8, 2023

physicx237 reopened this Dec 8, 2023

jelbourn requested a review from bjarkler December 13, 2023 18:03

JoostK added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed state: blocked action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 13, 2023

physicx237 force-pushed the my-fix-branch branch from 21b3336 to db9a4ad Compare December 14, 2023 00:10

physicx237 force-pushed the my-fix-branch branch from db9a4ad to ad4bad5 Compare December 14, 2023 00:25

JoostK approved these changes Dec 14, 2023

View reviewed changes

JoostK removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 14, 2023

jelbourn approved these changes Dec 16, 2023

View reviewed changes

physicx237 force-pushed the my-fix-branch branch from 39522b0 to ad4bad5 Compare December 17, 2023 19:24

AndrewKushnir approved these changes Jan 18, 2024

View reviewed changes

pkozlowski-opensource approved these changes Feb 19, 2024

View reviewed changes

pkozlowski-opensource removed request for alxhub, josephperrott and bjarkler February 19, 2024 15:33

pkozlowski-opensource added the action: merge The PR is ready for merge by the caretaker label Feb 19, 2024

alxhub closed this in dba3e0b Feb 21, 2024

physicx237 deleted the my-fix-branch branch February 22, 2024 05:50

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler): adding the inert property to the "SCHEMA" array #53148

fix(compiler): adding the inert property to the "SCHEMA" array #53148

physicx237 commented Nov 24, 2023 •

edited

JeanMeche commented Nov 24, 2023

JoostK Nov 24, 2023

bjarkler Dec 13, 2023

physicx237 commented Nov 24, 2023 •

edited

JoostK commented Nov 24, 2023

physicx237 commented Dec 13, 2023 •

edited

jelbourn commented Dec 13, 2023

bjarkler commented Dec 13, 2023

physicx237 commented Dec 13, 2023

JoostK commented Dec 13, 2023

physicx237 commented Dec 13, 2023

JoostK commented Dec 13, 2023

JoostK left a comment

physicx237 commented Dec 14, 2023

jelbourn left a comment

pkozlowski-opensource left a comment

alxhub commented Feb 21, 2024

angular-automatic-lock-bot bot commented Mar 24, 2024

fix(compiler): adding the inert property to the "SCHEMA" array #53148

fix(compiler): adding the inert property to the "SCHEMA" array #53148

Conversation

physicx237 commented Nov 24, 2023 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

JeanMeche commented Nov 24, 2023

JoostK Nov 24, 2023

Choose a reason for hiding this comment

bjarkler Dec 13, 2023

Choose a reason for hiding this comment

physicx237 commented Nov 24, 2023 • edited

JoostK commented Nov 24, 2023

physicx237 commented Dec 13, 2023 • edited

jelbourn commented Dec 13, 2023

bjarkler commented Dec 13, 2023

physicx237 commented Dec 13, 2023

JoostK commented Dec 13, 2023

physicx237 commented Dec 13, 2023

JoostK commented Dec 13, 2023

JoostK left a comment

Choose a reason for hiding this comment

physicx237 commented Dec 14, 2023

jelbourn left a comment

Choose a reason for hiding this comment

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

alxhub commented Feb 21, 2024

angular-automatic-lock-bot bot commented Mar 24, 2024

physicx237 commented Nov 24, 2023 •

edited

physicx237 commented Nov 24, 2023 •

edited

physicx237 commented Dec 13, 2023 •

edited