Skip to content

fix(core): Error on invalid APP_ID#63252

Closed
JeanMeche wants to merge 1 commit intoangular:mainfrom
JeanMeche:escape-appid
Closed

fix(core): Error on invalid APP_ID#63252
JeanMeche wants to merge 1 commit intoangular:mainfrom
JeanMeche:escape-appid

Conversation

@JeanMeche
Copy link
Copy Markdown
Member

@JeanMeche JeanMeche commented Aug 19, 2025 •
edited
Loading

Unsanitized appIds could be responsible to generating broken CSS selectors. (eg : is an example for a character that breaks a selector by being a separator for pseudo-selectors.)

fixes #63251

@pullapprove pullapprove bot requested a review from AndrewKushnir August 19, 2025 20:36
@JeanMeche JeanMeche added the action: global presubmit The PR is in need of a google3 global presubmit label Aug 19, 2025
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Aug 19, 2025
@ngbot ngbot bot added this to the Backlog milestone Aug 19, 2025
@JeanMeche
Copy link
Copy Markdown
Member Author

JeanMeche commented Aug 19, 2025

We'll need a TGP on this to see if this is a breaking change.

@JeanMeche JeanMeche marked this pull request as draft August 19, 2025 20:53
@JeanMeche JeanMeche marked this pull request as ready for review September 16, 2025 18:15
@JeanMeche JeanMeche changed the title refactor(platform-browser): escape appID in Emulated DOM renderer. refactor(platform-browser): Sanitize appID in Emulated DOM renderer. Sep 16, 2025
@thePunderWoman
Copy link
Copy Markdown
Contributor

thePunderWoman commented Sep 16, 2025

TGP

AndrewKushnir
AndrewKushnir reviewed Sep 16, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Shared some feedback with @JeanMeche via chat. Key points:

  • The check should happen sooner (e.g. at the time we create an application injector, using the ENVIRONMENT_INITIALIZER callback).
  • We should throw an error if the APP_ID value is "unsafe", so that we don't need to keep sanitizing it in the codebase at the time when we need to use it (easy to forget adding sanitization at that point).

@pullapprove pullapprove bot requested a review from AndrewKushnir September 17, 2025 15:53
AndrewKushnir
AndrewKushnir reviewed Sep 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @JeanMeche!

A couple of comments:

  • It'd be great to add some tests to verify this behavior
  • We should consider adding a similar check to the NgModule-based bootstrap logic as well

@pullapprove pullapprove bot requested a review from AndrewKushnir September 17, 2025 15:59
@AndrewKushnir AndrewKushnir added target: major This PR is targeted for the next major release breaking changes labels Sep 17, 2025
@thePunderWoman
Copy link
Copy Markdown
Contributor

thePunderWoman commented Sep 17, 2025

@JeanMeche This has conflicts that need to be resolved.

@thePunderWoman thePunderWoman added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 17, 2025
@JeanMeche JeanMeche force-pushed the escape-appid branch 2 times, most recently from e3d0726 to 5ea4456 Compare September 17, 2025 16:34
@pullapprove pullapprove bot requested a review from AndrewKushnir September 17, 2025 19:36
AndrewKushnir
AndrewKushnir approved these changes Sep 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@JeanMeche JeanMeche force-pushed the escape-appid branch 2 times, most recently from e586446 to ca37c83 Compare September 17, 2025 20:18
@thePunderWoman thePunderWoman 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 Sep 17, 2025
jelbourn
jelbourn reviewed Sep 17, 2025
View reviewed changes
@JeanMeche
fix(core): Error on invalid APP_ID
57dba94 
An invalid APP_ID could be responsible to generating broken CSS selectors. (eg `:` is an example for a character that breaks a selector by being a separator for pseudo-selectors.)
We now throw an error if the provided value is not alphanumerical
@JeanMeche JeanMeche changed the title refactor(platform-browser): Sanitize appID in Emulated DOM renderer. fix(core): Error on invalid APP_ID Sep 17, 2025
@thePunderWoman
Copy link
Copy Markdown
Contributor

thePunderWoman commented Sep 18, 2025

TGP

@thePunderWoman
Copy link
Copy Markdown
Contributor

thePunderWoman commented Sep 18, 2025

There are a few failures, but I'm pretty sure they're entirely unrelated to this based on the traces. I think this is safe to merge.

@thePunderWoman thePunderWoman added action: merge The PR is ready for merge by the caretaker and removed action: global presubmit The PR is in need of a google3 global presubmit labels Sep 19, 2025
@ngbot
Copy link
Copy Markdown

ngbot bot commented Sep 19, 2025

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing
    pending 1 pending code review

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 main, please try rebasing to main 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.

thePunderWoman
thePunderWoman approved these changes Sep 19, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

LGTM

@thePunderWoman
Copy link
Copy Markdown
Contributor

thePunderWoman commented Sep 19, 2025

This PR was merged into the repository. The changes were merged into the following branches:

thePunderWoman added a commit to thePunderWoman/angular that referenced this pull request Sep 19, 2025
@thePunderWoman
Revert "fix(core): Error on invalid APP_ID (angular#63252)"
b05c047 
This reverts commit fec7c28.
@JeanMeche JeanMeche deleted the escape-appid branch September 19, 2025 21:50
@JeanMeche JeanMeche mentioned this pull request Sep 19, 2025
Closed
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Oct 20, 2025

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 Oct 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@thePunderWoman thePunderWoman thePunderWoman approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

+1 more reviewer

@jelbourn jelbourn jelbourn left review comments

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: core Issues related to the framework runtime breaking changes target: major This PR is targeted for the next major release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

Escape APP_ID for ViewEncapsulation.Emulated

4 participants

@JeanMeche @thePunderWoman @jelbourn @AndrewKushnir