Skip to content

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

Closed
physicx237 wants to merge 1 commit intoangular:mainfrom
physicx237:my-fix-branch
Closed

fix(compiler): adding the inert property to the "SCHEMA" array#53148
physicx237 wants to merge 1 commit intoangular:mainfrom
physicx237:my-fix-branch

Conversation

@physicx237
Copy link
Contributor

@physicx237 physicx237 commented Nov 24, 2023 •
edited
Loading

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:

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
Copy link
Member

JeanMeche commented Nov 24, 2023

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.

JoostK
JoostK reviewed Nov 24, 2023
View reviewed changes
@physicx237 physicx237 requested a review from JoostK November 24, 2023 14:17
JoostK
JoostK reviewed Nov 24, 2023
View reviewed changes
packages/compiler/src/schema/dom_security_schema.ts Outdated
Copy link
Member

@JoostK JoostK Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bjarkler bjarkler Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, this should be removed.

@physicx237
Copy link
Contributor Author

physicx237 commented Nov 24, 2023 •
edited
Loading

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?

@physicx237 physicx237 requested a review from JoostK November 24, 2023 15:01
@JoostK
Copy link
Member

JoostK commented Nov 24, 2023

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.

@JoostK 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 pkozlowski-opensource added the area: compiler Issues related to `ngc`, Angular's template compiler label Nov 27, 2023
@ngbot ngbot bot added this to the Backlog milestone Nov 27, 2023
@physicx237 physicx237 closed this Dec 8, 2023
@physicx237 physicx237 reopened this Dec 8, 2023
@physicx237
Copy link
Contributor Author

physicx237 commented Dec 13, 2023 •
edited
Loading

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 jelbourn requested a review from bjarkler December 13, 2023 18:03
@jelbourn
Copy link
Contributor

jelbourn commented Dec 13, 2023

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
Copy link
Contributor

bjarkler commented Dec 13, 2023

LGTM, thanks!

@physicx237
Copy link
Contributor Author

physicx237 commented Dec 13, 2023

Thanks for your feedback!

@JoostK 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
@JoostK
Copy link
Member

JoostK commented Dec 13, 2023

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

@physicx237
Copy link
Contributor Author

physicx237 commented Dec 13, 2023

Yes, sure

@JoostK
Copy link
Member

JoostK commented Dec 13, 2023

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

@physicx237
fix(compiler): adding the inert property to the "SCHEMA" array
ad4bad5 
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 b637b7ce646e8bab2f585339028a84018e8ea982.

This commit is being reverted because the inert property is safe as a boolean attribute
JoostK
JoostK approved these changes Dec 14, 2023
View reviewed changes
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

@JoostK 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
@physicx237
Copy link
Contributor Author

physicx237 commented Dec 14, 2023

And thank you!

jelbourn
jelbourn approved these changes Dec 16, 2023
View reviewed changes
Copy link
Contributor

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Reviewed-for: fw-security

pkozlowski-opensource
pkozlowski-opensource approved these changes Feb 19, 2024
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Reviewed-for: fw-security

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

alxhub commented Feb 21, 2024

This PR was merged into the repository by commit dba3e0b.

@alxhub alxhub closed this in dba3e0b Feb 21, 2024
alxhub pushed a commit that referenced this pull request Feb 21, 2024
@physicx237 @alxhub
fix(compiler): adding the inert property to the "SCHEMA" array (#53148)
6447c0e 
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 b637b7ce646e8bab2f585339028a84018e8ea982.

This commit is being reverted because the inert property is safe as a boolean attribute

PR Close #53148
@physicx237 physicx237 deleted the my-fix-branch branch February 22, 2024 05:50
@angular-automatic-lock-bot
Copy link

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

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoostK JoostK JoostK approved these changes

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

+2 more reviewers

@bjarkler bjarkler bjarkler left review comments

@jelbourn jelbourn jelbourn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

Template binding does not recognize the 'inert' property.

8 participants

@physicx237 @JeanMeche @JoostK @jelbourn @bjarkler @alxhub @pkozlowski-opensource @AndrewKushnir