Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($compile) : Add base#href to the list of RESOURCE_URL context attrs #15597

Closed
wants to merge 1 commit into from

Conversation

rjamet
Copy link
Contributor

@rjamet rjamet commented Jan 11, 2017

This will put the same security constraints to bindings on base#ref as on iframes or script srcs, since it changes the behavior of relative URLs across the page. Currently, they don't have a $sce context. Also, it's more generally a good idea in Angular itself, as since #15144 and its fix #15145 we'll consider that baseURI as a trusted origin.

Does this PR introduce a breaking change?
Yup: something like <base href="{{myUrl}}"/> will send myUrl to the $sce's RESOURCE_URL checks. By default, it will break if myUrl isn't same-origin. Also, concatenation in trusted contexts is disabled: "/{{myPathComponent}}/something" won't work at all.

This follows the discussion in #15537 and the initial issue in #15144, fixed in #15145.

…ributes.

This will put the same security constraints to bindings on base#ref as
on iframes or script srcs, since it changes the behavior of relative
URLs across the page.
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM, but the commit message needs a proper breaking change notice.

gkalpak added a commit that referenced this pull request Jan 12, 2017
This reverts commit 5e28b6e.
Reverting while investigating security implications of 5e28b6e without #15597
(which is possibly a breaking change, thus not suitable for this branch).
gkalpak added a commit that referenced this pull request Jan 12, 2017
This reverts commit cce98ff.
Reverting while investigating security implications of cce98ff without #15597
(which is possibly a breaking change, thus not suitable for this branch).
@gkalpak
Copy link
Member

gkalpak commented Jan 12, 2017

@rjamet, what is your take on #15145 (comment)? Is it OK to release #15145 without this change, or should they rather go together (or not at all)?

@petebacondarwin
Copy link
Contributor

For anyone wondering this question was answered in #15145 (comment)

both PRs at the same time would be the safest.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM

@gkalpak gkalpak closed this in 1cf728e Jan 18, 2017
ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017
…attributes

Previously, `base[href]` didn't have an SCE context. This was not ideal, because
`base[href]` affects the behavior of all relative URLs across the page.
Furthermore, since angular#15145, `base[href]` also affects which URLs are considered
trusted under the 'self' policy for white- or black-listed resource URLs.

This commit tightens the security of Angular apps, by adding `base[href]` to the
list of RESOURCE_URL context attributes, essentially putting the same
constraints on bindings to `base[href]` as on iframe or script sources.

Refer to the
[`$sce` API docs](https://code.angularjs.org/snapshot/docs/api/ng/service/$sce)
for more info on SCE trusted contexts.

Closes angular#15597

BREAKING CHANGE:

Previously, `<base href="{{ $ctrl.baseUrl }}" />` would not require `baseUrl` to
be trusted as a RESOURCE_URL. Now, `baseUrl` will be sent to `$sce`'s
RESOURCE_URL checks. By default, it will break unless `baseUrl` is of the same
origin as the application document.

Refer to the
[`$sce` API docs](https://code.angularjs.org/snapshot/docs/api/ng/service/$sce)
for more info on how to trust a value in a RESOURCE_URL context.

Also, concatenation in trusted contexts is not allowed, which means that the
following won't work: `<base href="/something/{{ $ctrl.partialPath }}" />`.

Either construct complex values in a controller (recommended):

```js
this.baseUrl = '/something/' + this.partialPath;
```
```html
<base href="{{ $ctrl.baseUrl }}" />
```

Or use string concatenation in the interpolation expression (not recommended
except for the simplest of cases):

```html
<base href="{{ '/something/' + $ctrl.partialPath }}" />
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants