1305567 - V4 updates always fail with a 400 status code

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[François Marier [:francois]]

My profile somehow got into a bad state and V4 updates always fail:

listmanager: 14:38:32 GMT-0700 (PDT): update request: {
  "tableList": "googpub-phish-proto,goog-malware-proto,goog-unwanted-proto",
  "tableNames": {
    "googpub-phish-proto": true,
    "goog-malware-proto": true,
    "goog-unwanted-proto": true
  },
  "requestPayload": "ChUKE25hdmNsaWVudC1hdXRvLWZmb3gaJggCEAIaGgoNCAIQBhgBIgMwMDEwARC0dRoCGAf84fIgIgIgASgBGiYIARACGhoKDQgBEAYYASIDMDAxMAEQ23YaAhgHZ1/B+CICIAEoARoKCAMQAiICIAEoAQ==",
  "isPostRequest": false
}

listmanager: 14:38:32 GMT-0700 (PDT): makeUpdateRequestForEntry_: requestPayload ChUKE25hdmNsaWVudC1hdXRvLWZmb3gaJggCEAIaGgoNCAIQBhgBIgMwMDEwARC0dRoCGAf84fIgIgIgASgBGiYIARACGhoKDQgBEAYYASIDMDAxMAEQ23YaAhgHZ1/B+CICIAEoARoKCAMQAiICIAEoAQ== update: https://safebrowsing.googleapis.com/v4/threatListUpdates:fetch?$ct=application/x-protobuf&key=[trimmed-google-api-key] tablelist: googpub-phish-proto,goog-malware-proto,goog-unwanted-proto

listmanager: 14:38:32 GMT-0700 (PDT): download error for googpub-phish-proto,goog-malware-proto,goog-unwanted-proto: 400


Here are the prefs I have changed:

urlclassifier.malwareTable;goog-malware-proto,goog-unwanted-proto,goog-malware-shavar,goog-unwanted-shavar,test-malware-simple,test-unwanted-simple
urlclassifier.phishTable;googpub-phish-proto,goog-phish-shavar,test-phish-simple

and here are the only states that are in the prefs:

browser.safebrowsing.provider.google4.state.goog-malware-proto;Cg0IARAGGAEiAzAwMTABENt2GgIYB2dfwfg=
browser.safebrowsing.provider.google4.state.googpub-phish-proto;Cg0IAhAGGAEiAzAwMTABELR1GgIYB/zh8iA=

Comment 1

[François Marier [:francois]]

Henry found that it has to do with the base64 encoding of the request containing non-URL safe characters.

The spec we got from Google isn't clear. It says "base64" but it should ideally be base64url otherwise we need to URL-escape the base64-encoding string before adding it to the URL.

Comment 2

[Henry Chang [:hchang]]

Attachment: Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. .

Review commit: https://reviewboard.mozilla.org/r/81288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81288/

Comment 3

[Henry Chang [:hchang]]

Comment on attachment 8795162 [details]
Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81288/diff/1-2/

Comment 4

[Henry Chang [:hchang]]

Attachment: Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. .

Review commit: https://reviewboard.mozilla.org/r/81470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81470/

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8795162 [details]
Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81288/diff/2-3/

Comment 6

[François Marier [:francois]]

Comment on attachment 8795162 [details]
Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. .

https://reviewboard.mozilla.org/r/81288/#review80408

It's unfortunate we have to fake success to get to the body, but I think the error message is worth it. Great job!

::: toolkit/components/url-classifier/content/listmanager.js:417
(Diff revision 3)
>      let urlUtils = Cc["@mozilla.org/url-classifier/utils;1"]
>                       .getService(Ci.nsIUrlClassifierUtils);
> -    let requestPayload =  urlUtils.makeUpdateRequestV4(tableArray,
> +
> +    streamerMap.requestPayload = urlUtils.makeUpdateRequestV4(tableArray,
> -                                                       stateArray,
> +                                                              stateArray,
> -                                                       tableArray.length);
> +                                                              tableArray.length);;

nit: stray ";"

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:684
(Diff revision 3)
>        mDownloadErrorCallback->HandleEvent(strStatus);
>      }
>  
>      mDownloadError = true;
> -    status = NS_ERROR_ABORT;
> +
> +    // Continue to receive error message only when log is enabled.

To explain better what is going on, I would say something like: "We can't return an error just yet because we need to read the body in order to see the error message. We'll return an error in OnStopRequest() instead."

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:685
(Diff revision 3)
>      }
>  
>      mDownloadError = true;
> -    status = NS_ERROR_ABORT;
> +
> +    // Continue to receive error message only when log is enabled.
> +    status = LOG_ENABLED() ? NS_OK : NS_ERROR_ABORT;

I'm a little worried that this way of using `LOG_ENABLED()` might fail silently in the future.

The usual way I've seen it used is:

```
status = NS_ERROR_ABORT;
if (LOG_ENABLED()) {
  // Comment here
  status = NS_OK;
}
```

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:706
(Diff revision 3)
>                                                nsISupports* context,
>                                                nsIInputStream *aIStream,
>                                                uint64_t aSourceOffset,
>                                                uint32_t aLength)
>  {
> -  if (!mDBService)
> +  if (!mDBService && !mDownloadError)

nit: since we're changing that line, let's add the missing braces to the if statement

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:741
(Diff revision 3)
>  nsUrlClassifierStreamUpdater::OnStopRequest(nsIRequest *request, nsISupports* context,
>                                              nsresult aStatus)
>  {
> +  if (mDownloadError) {
> +    LOG(("Download error message: %s", mDownloadErrorMessage.get()));
> +    return NS_OK;

Maybe we can return an error here to indicate that this request really failed.

We would return `NS_ABORT_REQUEST` if we didn't care about the error message in the body, but that's not really the right error message once we're inside `OnStopRequest()` since the request has been fully received.

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8795162 [details]
Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81288/diff/3-4/

Comment 8

[François Marier [:francois]]

Comment on attachment 8795162 [details]
Bug 1305567 - Use base64url encoding, avoid cutting the state and dump download error message. .

https://reviewboard.mozilla.org/r/81288/#review80708

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d46155cc719e
Use base64url encoding, avoid cutting the state and dump download error message. r=francois.

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7aa4e7bb71e
Backed out changeset d46155cc719e for various failures in browser-chrome, devtools, and xpcshell (e.g. test_streamupdater.js) tests. r=backout on a CLOSED TREE

Comment 11

[Henry Chang [:hchang]]

Attachment: Bug1305567.patch

Comment 12

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec93d2ffb4eb

Comment 13

[Henry Chang [:hchang]]

Attachment: Bug1305567.patch

Comment 14

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ab0589fe4bf

Comment 15

[Henry Chang [:hchang]]

Attachment: Bug1305567.patch

Comment 16

[François Marier [:francois]]

Comment on attachment 8796704 [details] [diff] [review]
Bug1305567.patch

Review of attachment 8796704 [details] [diff] [review]:
-----------------------------------------------------------------

It's too bad we can't add the extra logging, but it would make the code very fragile.

Comment 17

[Henry Chang [:hchang]]

Due to the following error when I was trying to push to reviewboard after backout:

"One or more fields had errors (HTTP 400, API Error 105); identifier: Parent review request is submitted or discarded"

I can no longer use review board to land my commit. Use 'checkin-needed' instead.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4d883b4212
Use base64url encoding and avoid cutting the state. r=francois

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d4d883b4212