Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2944373002:
CSP: Remove wrong DCHECK in CSPSourceList (Closed)
|
Created:
3 years, 6 months ago by arthursonzogni Modified:
3 years, 6 months ago Reviewers:
Mike West CC:
battre, chromium-reviews, darin-cc_chromium.org, jam Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCSP: Remove wrong DCHECK in CSPSourceList
Removing DCHECK :
```
// When the '*' source is used, it must be the only one.
DCHECK(!allow_star || (!allow_self && sources.empty()));
```
This is obviously wrong. That is the 'none' source that cannot be
present with other sources.
This was introduced by mistake in
https://crrev.com/2937503002/
BUG=735049
Review-Url: https://codereview.chromium.org/2944373002
Cr-Commit-Position: refs/heads/master@{#481138}
Committed: https://chromium.googlesource.com/chromium/src/+/5174a7f227b37a6f0bc93f56da97c49e22a88f7a
Patch Set 1 #Patch Set 2 : Add test #
Messages
Total messages: 23 (13 generated)
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
arthursonzogni@chromium.org changed reviewers: + mkwst@chromium.org
Hi Mike, Could you please review this. That's a DCHECK I introduced by mistake. In addition to being wrong, it was not related to the original CL. Sorry about this. The issue was reported in https://crbug.com/692449.
Description was changed from ========== CSP: Remove wrong DCHECK in CSPSourceList Removing DCHECK : ``` // When the '*' source is used, it must be the only one. DCHECK(!allow_star || (!allow_self && sources.empty())); ``` This is obviously wrong. That is the 'none' source that cannot be present with other sources. This was introduced by mistake in https://crrev.com/2937503002/ ========== to ========== CSP: Remove wrong DCHECK in CSPSourceList Removing DCHECK : ``` // When the '*' source is used, it must be the only one. DCHECK(!allow_star || (!allow_self && sources.empty())); ``` This is obviously wrong. That is the 'none' source that cannot be present with other sources. This was introduced by mistake in https://crrev.com/2937503002/ BUG=735049 ==========
Removing the DCHECK looks reasonable. Can you add a test to verify that we handle policies with `*` correctly (e.g. without DCHECKing)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/20 16:04:42, Mike West (OOO until 19th) wrote: > Removing the DCHECK looks reasonable. Can you add a test to verify that we > handle policies with `*` correctly (e.g. without DCHECKing)? I remember now why I introduced this DCHECK in the original check. It was intentional. The behavior of CSPSourceList::Allow has changed a little bit. Before the original CL, when |allow_star| was true, the checks with |allow_self| didn't happens. Now both are checked. The behavior would be different if I can find two urls (self_url, target_url) such that the CSPSourceList < 'self' > allow the request and < * > block the request. In that case the CSPSourceList < 'self' * > would block the request before the CL and allow it after. It is probably not possible to find those two url thought. I will try to make a test based on this if it is possible. The DCHECK was here to ensure this CL doesn't introduce a change.
On 2017/06/20 at 17:04:11, arthursonzogni wrote: > On 2017/06/20 16:04:42, Mike West (OOO until 19th) wrote: > > Removing the DCHECK looks reasonable. Can you add a test to verify that we > > handle policies with `*` correctly (e.g. without DCHECKing)? > > I remember now why I introduced this DCHECK in the original check. It was intentional. > The behavior of CSPSourceList::Allow has changed a little bit. Before the original CL, when |allow_star| was true, the checks with |allow_self| didn't happens. > Now both are checked. > > The behavior would be different if I can find two urls (self_url, target_url) such that the CSPSourceList < 'self' > allow the request and < * > block the request. > In that case the CSPSourceList < 'self' * > would block the request before the CL and allow it after. > It is probably not possible to find those two url thought. I will try to make a test based on this if it is possible. `data:`, `blob:`, and `filesystem:` should all meet that criteria.
On 2017/06/20 17:24:34, Mike West (OOO until 19th) wrote:
> On 2017/06/20 at 17:04:11, arthursonzogni wrote:
> > On 2017/06/20 16:04:42, Mike West (OOO until 19th) wrote:
> > > Removing the DCHECK looks reasonable. Can you add a test to verify that we
> > > handle policies with `*` correctly (e.g. without DCHECKing)?
> >
> > I remember now why I introduced this DCHECK in the original check. It was
> intentional.
> > The behavior of CSPSourceList::Allow has changed a little bit. Before the
> original CL, when |allow_star| was true, the checks with |allow_self| didn't
> happens.
> > Now both are checked.
> >
> > The behavior would be different if I can find two urls (self_url,
target_url)
> such that the CSPSourceList < 'self' > allow the request and < * > block the
> request.
> > In that case the CSPSourceList < 'self' * > would block the request before
the
> CL and allow it after.
> > It is probably not possible to find those two url thought. I will try to
make
> a test based on this if it is possible.
>
> `data:`, `blob:`, and `filesystem:` should all meet that criteria.
I was looking in this direction.
The thing is that if |allow_star|=true then the request is allowed When the
url's scheme is in {http, https, http-so, https-so, ws, wss, ftp} **or** is the
same as the scheme of 'self'.
So to meet this criteria, we need self_url and target_url to both have a scheme
different from the ones listed above and also to be different.
In this case 'self' can't match with target_url. The function SourceAllowScheme
would return 'NotMatching'.
So I think it is impossible.
I added the test. I was not able to test a request allowed by 'self' and not by '*' (impossible). Instead I tested a request allowed by '*' and not by 'self'.
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/20 19:23:33, Mike West (OOO until 19th) wrote: > LGTM, thanks! Thanks!
The CQ bit was checked by arthursonzogni@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498030590849910,
"parent_rev": "240efbe686c1a0ff29dd3453ba5d048a979266c5", "commit_rev":
"5174a7f227b37a6f0bc93f56da97c49e22a88f7a"}
Message was sent while issue was closed.
Description was changed from ========== CSP: Remove wrong DCHECK in CSPSourceList Removing DCHECK : ``` // When the '*' source is used, it must be the only one. DCHECK(!allow_star || (!allow_self && sources.empty())); ``` This is obviously wrong. That is the 'none' source that cannot be present with other sources. This was introduced by mistake in https://crrev.com/2937503002/ BUG=735049 ========== to ========== CSP: Remove wrong DCHECK in CSPSourceList Removing DCHECK : ``` // When the '*' source is used, it must be the only one. DCHECK(!allow_star || (!allow_self && sources.empty())); ``` This is obviously wrong. That is the 'none' source that cannot be present with other sources. This was introduced by mistake in https://crrev.com/2937503002/ BUG=735049 Review-Url: https://codereview.chromium.org/2944373002 Cr-Commit-Position: refs/heads/master@{#481138} Committed: https://chromium.googlesource.com/chromium/src/+/5174a7f227b37a6f0bc93f56da97... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/5174a7f227b37a6f0bc93f56da97... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
