348360 - jar:file: urls should not trigger a lookup

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

In bug 341946, we turned on url checks for all jar: urls.  Most jar: urls are actually jar:file: urls when viewing a xul document.  These shouldn't result in a lookup.

Comment 1

[Tony Chang (Google)]

Attachment: v1: check inner uri

It would be nice to use nsINestedURI, but that's not on branch, so just do it manually.

Comment 2

[Tony Chang (Google)]

on trunk

Comment 3

[Mike Connor [:mconnor]]

Not going to block, but will take a patch that's well-baked.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233257 [details] [diff] [review]
v1: check inner uri

a=drivers for the mozilla181 branch

Comment 5

[Tony Chang (Google)]

on branch

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233257 [details] [diff] [review]
v1: check inner uri

Er... this patch is wrong.  You can't use nsINestedURI on the branch, but you _CAN_ use nsIJARURI.  And should.

For example, the code as currently written will break on something simple like:

jar:jar:http://...

Please see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/caps/src/nsScriptSecurityManager.cpp&rev=1.266.2.14&mark=263-268#262 for the right way to do this.

Also, I would like to see a followup bug for doing this using nsINestedURI on trunk.

Comment 7

[Tony Chang (Google)]

(In reply to comment #6)
> (From update of attachment 233257 [details] [diff] [review] [edit])
> Er... this patch is wrong.  You can't use nsINestedURI on the branch, but you
> _CAN_ use nsIJARURI.  And should.

Ah, didn't know about that.  Will post a fixed patch today.

> For example, the code as currently written will break on something simple like:
> 
> jar:jar:http://...

This should work with the current patch since it recurses (I tested this case as well).

> Also, I would like to see a followup bug for doing this using nsINestedURI on
> trunk.

Ok, will do this also.

Comment 8

[Tony Chang (Google)]

Attachment: use nsIJARURI instead of GetPath

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233908 [details] [diff] [review]
use nsIJARURI instead of GetPath

Looks good.  Thanks!

Comment 10

[Tony Chang (Google)]

Attachment: use nsINestedURI (for trunk)

Comment 11

[Tony Chang (Google)]

Attachment: v2: use nsINestedURI (for trunk)

Oops, failed to remove the old return in the last patch.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233922 [details] [diff] [review]
v2: use nsINestedURI (for trunk)

You don't want the check for "jar" here, do you?

Comment 13

[Tony Chang (Google)]

Attachment: v3: any nested URI

Check any URI with a nested URI (except those that already pass our filter, like about: URIs).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233944 [details] [diff] [review]
v3: any nested URI

>+    nsCOMPtr<nsINestedURI> nestedURI;

Put the do_QI there.

>+    if (nestedURI = do_QueryInterface(aURI)) {

Because this will warn...

sr=bzbarsky with that.

Comment 15

[Tony Chang (Google)]

Attachment: v4: any nested URI

Comment 16

[Tony Chang (Google)]

v4 on trunk

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 233908 [details] [diff] [review]
use nsIJARURI instead of GetPath

a=dbaron on behalf of drivers.  Please land on MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword once you have done so.

Comment 18

[Tony Chang (Google)]

patch "use nsIJARURI instead of GetPath" on branch.