Update URLClassifier to follow Safebrowsing spec

RESOLVED FIXED in Firefox 3.6a1

Status

()

Toolkit
Safe Browsing
P2
normal
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

({fixed1.9.0.11, fixed1.9.1})

Trunk
Firefox 3.6a1
fixed1.9.0.11, fixed1.9.1
Points:
---
Bug Flags:
blocking-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.76 KB, patch
Tony Chang (Google)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

9 years ago
Created attachment 373941 [details] [diff] [review]
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.
Attachment #373941 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #373941 - Flags: review?(tony)
Attachment #373941 - Flags: review?(dcamp)
Attachment #373941 - Flags: review?
(Assignee)

Updated

9 years ago
Whiteboard: [needs review safebrowsing peer]
Blocks 3.5 final, if we can get it done before b4 code freeze, we'll take it for that milestone, too!
Flags: blocking-firefox3.5+
Priority: -- → P2
(Assignee)

Comment 3

9 years ago
Created attachment 373942 [details] [diff] [review]
v1.1

It was suggested on irc to use ScopedXPCOM in the test as well so we get leak checking.
Attachment #373941 - Attachment is obsolete: true
Attachment #373942 - Flags: review?(tony)
Attachment #373942 - Flags: review?(dcamp)
Attachment #373941 - Flags: review?(tony)
Attachment #373941 - Flags: review?(dcamp)

Comment 4

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

r+
Attachment #373942 - Flags: review?(tony) → review+
(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?
(Assignee)

Updated

9 years ago
Attachment #373942 - Flags: review?(dcamp)
(Assignee)

Comment 6

9 years ago
(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.
Whiteboard: [needs review safebrowsing peer] → [can land]
(Assignee)

Comment 7

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Summary: Update URLClassifier per new Safebrowsing spec → Update URLClassifier to follow Safebrowsing spec
Whiteboard: [can land]
Target Milestone: --- → Firefox 3.6a1
(Assignee)

Comment 8

9 years ago
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.
Attachment #373942 - Flags: approval1.9.0.10?
Keywords: fixed1.9.1
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.
Attachment #373942 - Flags: approval1.9.0.10? → approval1.9.0.10+
(Assignee)

Comment 11

9 years ago
(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
Keywords: fixed1.9.0.10
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.