908260 - HTTP error 450 should result in a download blocked by parental controls

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[:Paolo Amadini]

HTTP error 450 (Blocked by parental control proxies) should result in a
DownloadError object with the becauseBlockedByParentalControls property set:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#3013

Comment 1

[David E. Ross]

What is the basis for making HTTP error 450 signify anything?  Neither RFC 2616, its errata, nor its update RFCs mention an error 450.  

Has an RFC for this been submitted to the IETF?

Comment 2

[:Paolo Amadini]

I didn't get that this was a Windows-specific feature from the referenced code,
thanks for pointing that out. We should do this to complete the integration with
parental controls that we already put in place, and update the comments with the
correct status description "Blocked by Windows Parental Controls".

Comment 3

[Neil Deakin]

Attachment: Initial patch to implement this

I think this is what is intended here. Two issues:

- I'm not sure how to get the blocked error to propagate properly. Right now the test just fails because it thinks no error occurs.
- do we need to ensure aCancelable.cancel() is called?

Comment 4

[:Paolo Amadini]

Comment on attachment 803158 [details] [diff] [review]
Initial patch to implement this

The exception should be thrown by the DownloadSaver.execute method, after the target file has been closed, or before it is opened. This must work for both DownloadCopySaver and DownloadLegacySaver.

One way to do this would be to set a state variable indicating the problem, cancel the ongoing request, and then read the state variable when handling the request failure.

Comment 5

[Neil Deakin]

Attachment: Raise an DownloadError on 450

The only thing I'm not sure about is which additional actions need to happen on cancel:

- cancelable.cancel()
- if the part file needs to be deleted or if that already happens

Comment 6

[:Paolo Amadini]

I'll review this patch later, when we have handled the other bugs blocking the Downloads API release.

Comment 7

[:Paolo Amadini]

Comment on attachment 804940 [details] [diff] [review]
Raise an DownloadError on 450

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

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +407,5 @@
>          let error = new DownloadError(Cr.NS_ERROR_FAILURE, "Download blocked.");
>          error.becauseBlocked = true;
>          error.becauseBlockedByParentalControls = true;
> +        return error;
> +      }

We've been talking about changing the DownloadError constructor to look like "new DownloadError({ ...properties... })", this would make this helper function unneeded (note that Cr.NS_ERROR_FAILURE would be the default "result" property).

Can you make the change in this patch?

@@ +428,5 @@
> +        // An HTTP 450 error code can also block the download, however this is
> +        // not included in the check above as it requires the request to start.
> +        if (this._blockedByParentalControls) {
> +          throw createParentalBlockedError();
> +        }

We should make sure that the "error" property is set. We may do this by changing the "ex" property instead of throwing again. This should not happen for cancellation instead.

Actually, the old parental controls code may be buggy in that regard. If it makes sense to move some processing to another function to simplify the code or make the logic clearer, feel free to do some refactoring.

::: toolkit/components/jsdownloads/src/DownloadLegacy.js
@@ +95,5 @@
> +        // If the HTTP status is 450, then Windows Parental Controls have
> +        // blocked this download.
> +        if (aRequest instanceof Ci.nsIHttpChannel &&
> +            aRequest.responseStatus == 450) {
> +          download._blockedByParentalControls = true;

Let's do this inside onTransferStarted.

Actually, we'll need to do the same for DownloadCopySaver as well, so we may define a function there and call it from DownloadLegacySaver.

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +1442,5 @@
> +      download = yield promiseStartLegacyDownload(httpUrl("parentalblocked.zip"));
> +      yield promiseDownloadStopped(download);
> +      do_throw("The download should have blocked.");
> +    } catch (ex if ex instanceof Downloads.Error && ex.becauseBlocked) {
> +      do_check_true(ex.becauseBlockedByParentalControls);

So, we should also check the "error" property here, and maybe in the other parental controls tests as well.

Comment 8

[Neil Deakin]

> We should make sure that the "error" property is set. We may do this by
> changing the "ex" property instead of throwing again. This should not happen
> for cancellation instead.
> 
> Actually, the old parental controls code may be buggy in that regard. If it
> makes sense to move some processing to another function to simplify the code
> or make the logic clearer, feel free to do some refactoring.
> 

Paolo, can you elaborate here? Do you mean that existing parental blocking code should set the error property, or that there are additional bugs?

Comment 9

[Neil Deakin]

Attachment: Part 1 - convert DownloadError to take a properties object

Comment 10

[Neil Deakin]

Attachment: Part 2 - block download on 450 error

Comment 11

[:Paolo Amadini]

(In reply to Neil Deakin from comment #8)
> Paolo, can you elaborate here? Do you mean that existing parental blocking
> code should set the error property, or that there are additional bugs?

The first one :-)

Comment 12

[:Paolo Amadini]

Comment on attachment 808790 [details] [diff] [review]
Part 1 - convert DownloadError to take a properties object

The idea is that DownloadError could accept "because" properties in the constructor as well. Example:

"new DownloadError({ becauseBlockedByParentalControls: true })"

In this case, we could set the becauseBlocked property as well. Or, if you
prefer to have less behind-the-scenes magic:

"new DownloadError({ becauseBlocked: true,
                     becauseBlockedByParentalControls: true })"

The message would be determined automatically.

Comment 13

[Neil Deakin]

Attachment: Part 1.2 - convert DownloadError to take a properties object

Comment 14

[Neil Deakin]

Attachment: Part 2.2 - block download on 450 error

Comment 15

[:Paolo Amadini]

Comment on attachment 809203 [details] [diff] [review]
Part 1.2 - convert DownloadError to take a properties object

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

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +338,5 @@
>      // While shutting down or disposing of this object, we prevent the download
>      // from returning to be in progress.
>      if (this._finalized) {
> +      return Promise.reject(new DownloadError({  
> +                                message: "Cannot start after finalization."}));

nit: whitespace at end of line

@@ +1160,5 @@
> + *                        This is useful when the error is received by a
> + *                        component that handles both aspects of the download.
> + *          }
> + * The properties object may also contain any of the DownloadError's because
> + * properties, which will be set accordingly in the error object.

nit: align with parameter documentation, empty line before @param

@@ +1167,5 @@
>  {
>    const NS_ERROR_MODULE_BASE_OFFSET = 0x45;
>    const NS_ERROR_MODULE_NETWORK = 6;
>    const NS_ERROR_MODULE_FILES = 13;
> + 

whitespace

@@ +1474,5 @@
>                } else {
>                  // Infer the origin of the error from the failure code, because
>                  // BackgroundFileSaver does not provide more specific data.
> +                let error = { result: aStatus, inferCause: true };
> +                deferSaveComplete.reject(new DownloadError(error));

nit: let properties =

(also below)

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +19,5 @@
>  
>  let scriptFile = do_get_file("common_test_Download.js");
>  Services.scriptloader.loadSubScript(NetUtil.newURI(scriptFile).spec);
>  
> +add_task(function test_DownloadError()

nit: add a "//// Tests" section.

Thanks for writing the tests for the expected behavior!

Comment 16

[:Paolo Amadini]

Comment on attachment 809204 [details] [diff] [review]
Part 2.2 - block download on 450 error

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

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +1354,5 @@
>      }
>      do_throw("The download should have blocked.");
>    } catch (ex if ex instanceof Downloads.Error && ex.becauseBlocked) {
>      do_check_true(ex.becauseBlockedByParentalControls);
> +    do_check_eq(ex.message, "Download blocked.");

I don't think we should verify the message here. Instead, we should check "download.error.becauseBlockedByParentalControls".

@@ +1382,5 @@
> +    }
> +    do_throw("The download should have blocked.");
> +  } catch (ex if ex instanceof Downloads.Error && ex.becauseBlocked) {
> +    do_check_true(ex.becauseBlockedByParentalControls);
> +    do_check_eq(ex.message, "Download blocked.");

The same here. I'm not sure we set the property correctly yet.

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +775,5 @@
>  
> +  // This URL will emulate being blocked by Windows Parental controls
> +  gHttpServer.registerPathHandler("/parentalblocked.zip",
> +    function (aRequest, aResponse) {
> +      aResponse.setStatusLine(aRequest.httpVersion, 450, "Parental Blocked");

Let's use the status message "Blocked by Windows Parental Controls".

Comment 17

[Neil Deakin]

Attachment: Part 2.3 - block download on 450 error

Should the first parental controls block (DownloadIntegration.shouldBlockForParentalControls) be within the try block? I notice that when this error occurs, download.stopped is not set to false, so I didn't add a check to the test.

Comment 18

[:Paolo Amadini]

Comment on attachment 809363 [details] [diff] [review]
Part 2.3 - block download on 450 error

(In reply to Neil Deakin from comment #17)
> Should the first parental controls block
> (DownloadIntegration.shouldBlockForParentalControls) be within the try
> block? I notice that when this error occurs, download.stopped is not set to
> false, so I didn't add a check to the test.

Uh, absolutely! That was more buggy than I expected :-)

r+ if we throw the exception within the "try" block.

Comment 19

[Neil Deakin]

Attachment: Part 2.4 - block download on 450 error

I also had to change download.start() to return currentAttempt instead of this._currentAttempt as this._currentAttempt is cleared during the finally block.

Comment 20

[David E. Ross]

Not seeing any response to my comment #1, I did some research.  

According to Wikipedia at <http://en.wikipedia.org/wiki/List_of_HTTP_status_codes#cite_note-20>, HTTP error code 450 is a Microsoft "extension" of RFC 2616.  This raises the question whether Chrome, Opera, or any other browser will also implement Microsoft's definition of HTTP error code 450.  It also raises the question as to what Mozilla will do if Microsoft changes how it defines HTTP error code 450.  

The W3C at <http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html> does not define any HTTP error codes in the 4xx series beyond 417.  However, that Web page has not been updated since 2004.  

The Internet Assigned Numbers Authority (IANA) at <http://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml> says that HTTP error codes 430 and 432-499 are "Unassigned".  That Web page was updated less than a year ago.  

I strongly suggest that, before further work is done on this RFE, Mozilla should submit a draft RFC to the Internet Engineering Task Force (IETF) to stabilize the definition of HTTP error code 450.  After all, Mozilla does claim to create standards-compliant applications.

Comment 21

[Neil Deakin]

We already implement this error code, and have done since 2007. This bug is about implementing it in the new download manager api.

Comment 22

[:Paolo Amadini]

(In reply to David E. Ross from comment #20)
> Not seeing any response to my comment #1, I did some research.  

Thank you for doing this research. I'm sorry my answer wasn't as exhaustive as it could be, you've done a better job at explaining the background information.

> I strongly suggest that, before further work is done on this RFE, Mozilla
> should submit a draft RFC to the Internet Engineering Task Force (IETF) to
> stabilize the definition of HTTP error code 450.  After all, Mozilla does
> claim to create standards-compliant applications.

The misunderstanding is that the "parental control proxy" we're talking about is a local piece of software installed on a Windows machine, designed to filter requests initiated by applications. This HTTP error code is not meant to be significant over the Internet, as far as we're concerned.

If we ignore the effects of this Windows API, the result is that locally blocked downloads in Firefox will fail for apparently mysterious reasons. If we fix this, the user sees a message saying the download was blocked by parental controls, which is a far better experience.

We could have made this code Windows-only, but I see little value in doing that, with the downside of differentiating a set of tests that can be currently run uniformly across all platforms.

Comment 23

[:Paolo Amadini]

Comment on attachment 809963 [details] [diff] [review]
Part 2.4 - block download on 450 error

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

r+ with the changes below, including making clearer in the code documentation what this feature is about.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +412,5 @@
> +        // Disallow download if parental controls service restricts it.
> +        if (yield DownloadIntegration.shouldBlockForParentalControls(this)) {
> +          this.error = new DownloadError({
> +            becauseBlockedByParentalControls: true });
> +          throw this.error;

You may just "throw new DownloadError".

@@ +432,5 @@
>            throw new DownloadError({ message: "Download canceled." });
>          }
>  
> +        // An HTTP 450 error code can also block the download, however this is
> +        // not included in the check above as it requires the request to start.

nit: It's not clear what the "check above" refers to, I was thinking of the previous line of code.

@@ +479,5 @@
>      }.bind(this)));
>  
>      // Notify the new download state before returning.
>      this._notifyChange();
> +    return currentAttempt;

Thanks for finding and fixing this!

@@ +1340,5 @@
>    },
>  
>    /**
> +   * Return true if the request's response has been blocked by parental controls
> +   * with an HTTP 450 error code.

Given the comments in the bug, I'd make it more explicit in the code comment here that this check is just for a Windows API.

@@ +1566,1 @@
>                backgroundFileSaver.onStartRequest(aRequest, aContext);

I've realized that to be technically correct we should still unconditionally call onStartRequest on BackgroundFileSaver before we do the check, because we call onStopRequest on it later.

(OnStartRequest in BackgroundFileSaver does nothing, by the way.)

Comment 24

[Neil Deakin]

https://hg.mozilla.org/integration/fx-team/rev/1292ead729c9
https://hg.mozilla.org/integration/fx-team/rev/c0ba6ce1d34d

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1292ead729c9
https://hg.mozilla.org/mozilla-central/rev/c0ba6ce1d34d

Comment 26

[David E. Ross]

What would happen if the W3C specifications or the Internet RFCs adopted a different meaning to HTTP error code 450?  Lacking any action by Mozilla or Microsoft towards locking down HTTP error code 450, this could indeed happen.