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(security): allow calc and gradient functions. #13943

Closed

Conversation

mprobst
Copy link
Contributor

@mprobst mprobst commented Jan 16, 2017

Also includes support for # color notation in function arguments (common
in gradient functions).

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

Also includes support for # color notation in function arguments (common
in gradient functions).
@mprobst mprobst added area: security Issues related to built-in security features, such as HTML sanitation feature Issue that requests a new feature action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 16, 2017
@mprobst mprobst requested review from rjamet and vicb January 16, 2017 08:44
Copy link
Contributor

@rjamet rjamet left a comment

Choose a reason for hiding this comment

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

LGTM on the security side.

const SAFE_STYLE_VALUE =
new RegExp(`^(${VALUES}|(?:${TRANSFORMATION_FNS}|${COLOR_FNS})${FN_ARGS})$`, 'g');
const GRADIENTS = '(?:repeating-)?(?:linear|radial)-gradient';
const CSS3_FNS = '(?:calc|attr)';
Copy link
Contributor

Choose a reason for hiding this comment

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

attr is a fun one: imagine something like <input type="password" value="secret" style="{{x}}"/>, do you think you're able to exfiltrate or at least display the value ?

I don't think you can for now on Chrome or Firefox, but https://developer.mozilla.org/en-US/docs/Web/CSS/attr says that "support for properties other than content is experimental". That means that very hypothetically, you could have:
<input type="embarassing" value="information" [style.background-image]="userSpecified"> with attr(value url), which is okayish according to MDN and leaks some information.

Realistically, and with the limitations of attr, it looks fine. Unless planets are wrongfully aligned, I don't see this causing issues. The web moving forward too fast might break these assumptions, so look out for CSS string concatenations if they ever happen :/

@mprobst mprobst added action: merge The PR is ready for merge by the caretaker pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 16, 2017
@mhevery
Copy link
Contributor

mhevery commented Jan 17, 2017

@mprobst please fix lint errors so that it can be merged.

@mhevery mhevery closed this in e19bf70 Jan 17, 2017
mhevery pushed a commit that referenced this pull request Jan 19, 2017
PR Close #13943

Also includes support for # color notation in function arguments (common
in gradient functions).
wKoza pushed a commit to wKoza/angular that referenced this pull request Feb 12, 2017
PR Close angular#13943

Also includes support for # color notation in function arguments (common
in gradient functions).
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
PR Close angular#13943

Also includes support for # color notation in function arguments (common
in gradient functions).
@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 10, 2019
idlechara pushed a commit to idlechara/angular that referenced this pull request Apr 27, 2021
PR Close angular#13943

Also includes support for # color notation in function arguments (common
in gradient functions).
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: security Issues related to built-in security features, such as HTML sanitation cla: yes feature Issue that requests a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants