Skip to content

Conversation

@atcastle
Copy link
Contributor

This PR makes a small change to NgOptimizedImage to properly allow inputs of the safeUrl type for ngSrc in NgOptimizedImage. This is purely for compatibility/migration concerns, as NgOptimizedImage does not enforce sanitization on the src url, as that is not an xss vector in modern browsers.

The change is made using a transform which automatically unwraps provided safeUrl values, so the rest of the NgOptimizedImage still treats the ngSrc as always being a string.

CC: @AndrewKushnir @kara

@pullapprove pullapprove bot requested a review from AndrewKushnir August 14, 2023 16:47
@atcastle atcastle force-pushed the safeurl-ngoptimizedimage branch 3 times, most recently from d2f4cbe to 61c1d1a Compare August 14, 2023 17:00
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package target: patch This PR is targeted for the next patch release common: image directive labels Aug 14, 2023
@ngbot ngbot bot modified the milestone: Backlog Aug 14, 2023
@atcastle atcastle force-pushed the safeurl-ngoptimizedimage branch from 61c1d1a to a18c6dc Compare August 14, 2023 17:50
@AndrewKushnir AndrewKushnir 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 action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 15, 2023
@AndrewKushnir
Copy link
Contributor

@atcastle thanks for addressing the feedback! The change looks great 👍

Could you please take a look at the failing test CI job (it looks like it has a legit failure)? Note: the aio-local one is unrelated and can be fixed by rebase on top of the most recent main branch.

@atcastle atcastle force-pushed the safeurl-ngoptimizedimage branch from a18c6dc to b8f1db2 Compare August 15, 2023 18:10
@angular-robot angular-robot bot requested a review from AndrewKushnir August 15, 2023 18:10
@atcastle atcastle force-pushed the safeurl-ngoptimizedimage branch from b8f1db2 to 8e1841a Compare August 15, 2023 18:12
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

Allow safeUrl and add transformer to immediately convert ngSrc to string
@atcastle atcastle force-pushed the safeurl-ngoptimizedimage branch from 8e1841a to 70bcee7 Compare August 15, 2023 19:02
@pullapprove pullapprove bot requested review from alxhub and jessicajaniuk August 15, 2023 19:06
Copy link
Contributor

@jessicajaniuk jessicajaniuk 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

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: public-api

@pkozlowski-opensource pkozlowski-opensource added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Aug 16, 2023
@AndrewKushnir AndrewKushnir removed the request for review from alxhub August 16, 2023 03:52
@pkozlowski-opensource pkozlowski-opensource added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Aug 16, 2023
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit d910bf8.

AndrewKushnir pushed a commit that referenced this pull request Aug 17, 2023
Allow safeUrl and add transformer to immediately convert ngSrc to string

PR Close #51351
@angular-automatic-lock-bot
Copy link

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 Sep 17, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
Allow safeUrl and add transformer to immediately convert ngSrc to string

PR Close angular#51351
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package common: image directive target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants