489455 - Update URLClassifier to follow Safebrowsing spec

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Shawn Wilsher :sdwilsh]

The spec has changed, and we need to remove a special case from Safebrowsing URL canonicalization where "%00" is changed to "%01".

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This makes the requested change.  I also added a test for this behavior, as well as updated the C++ test to follow the format of the rest of the test harness (TEST-PASS on success, TEST-UNEXPECTED-FAIL on failure).

Not sure if Tony or dcamp should be the one reviewing this, so requesting from both and assuming one of them will tell me what's up with that.

Comment 2

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

Blocks 3.5 final, if we can get it done before b4 code freeze, we'll take it for that milestone, too!

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

It was suggested on irc to use ScopedXPCOM in the test as well so we get leak checking.

Comment 4

[Tony Chang (Google)]

Comment on attachment 373942 [details] [diff] [review]
v1.1

r+

Comment 5

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #0)
> The spec has changed, and we need to remove a special case from Safebrowsing
> URL canonicalization where "%00" is changed to "%01".

Which spec has changed? I don't see any changes recently here: http://code.google.com/p/google-safe-browsing/wiki/Protocolv2Spec
But that's because this spec omits this detail completely... Oh well... Wouldn't it be more honest to say that this change is needed, because Google has changed software on their servers?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Which spec has changed? I don't see any changes recently here:
> http://code.google.com/p/google-safe-browsing/wiki/Protocolv2Spec
> But that's because this spec omits this detail completely... Oh well...
> Wouldn't it be more honest to say that this change is needed, because Google
> has changed software on their servers?
More accurately, this substitution happens on the server side and in Firefox, but is not actually part of the protocol spec.  Google would like their servers to not do this, and are waiting for us to update our code.

I'd love to be able to land this today, but the tree is all sorts of colors but green, so we'll have to wait and see.

Comment 7

[Shawn Wilsher :sdwilsh]

I was wrong about the tree.  Assuming things cycle green, I'll land on 1.9.1 tonight as well.

http://hg.mozilla.org/mozilla-central/rev/83b59076910c

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 373942 [details] [diff] [review]
v1.1

Like to try and take this for 1.9.0.10, but would understand if drivers feel it hasn't baked long enough yet.

Comment 9

[Shawn Wilsher :sdwilsh]

and 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/89318e970c75

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 373942 [details] [diff] [review]
v1.1

Approved for 1.9.0.10, a=dveditz for release-drivers

Today is code-freeze for 1.9.0.10, uncomfortably late. If this breaks safebrowsing for millions of people you owe us some expensive yummy spirits.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> Today is code-freeze for 1.9.0.10, uncomfortably late. If this breaks
> safebrowsing for millions of people you owe us some expensive yummy spirits.
Hey, this is why we have tests ;)

I had to modify the test changes slightly for it all to work on 1.9.0, but otherwise this patch applied cleanly.

Checking in toolkit/components/url-classifier/src/nsUrlClassifierUtils.cpp;
new revision: 1.11; previous revision: 1.10
Checking in toolkit/components/url-classifier/tests/TestUrlClassifierUtils.cpp;
new revision: 1.8; previous revision: 1.7