Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement webRequest auth handler

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
WebExtensions: Request Handling
P3
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: billm, Assigned: mixedpuppy, Mentored)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

34 Branch
mozilla54
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [webRequest]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
https://developer.chrome.com/extensions/webRequest#event-onAuthRequired
https://developer.chrome.com/extensions/webRequest#event-onBeforeRedirect
(Reporter)

Updated

2 years ago
Priority: -- → P1
(Reporter)

Updated

2 years ago
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
(Reporter)

Updated

2 years ago
Mentor: wmccloskey@mozilla.com
Keywords: dev-doc-needed

Updated

2 years ago
Whiteboard: [webRequest]

Updated

2 years ago
Blocks: 1214433
Priority: P1 → P2

Updated

2 years ago
Flags: blocking-webextensions+

Updated

2 years ago
Blocks: 1213483

Updated

2 years ago
Whiteboard: [webRequest] → [webRequest]triaged
(Reporter)

Updated

2 years ago
Summary: Implement webRequest auth and redirect handlers → Implement webRequest auth handler

Updated

2 years ago
Assignee: nobody → g.maone
Priority: P2 → P3
Summary: Implement webRequest auth handler → Implement webRequest auth and redirect handlers
Half of this bug is actually Bug 1215197, which I fixed.
Morphing accordingly.
Summary: Implement webRequest auth and redirect handlers → Implement webRequest auth handler

Updated

10 months ago
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Flags: blocking-webextensions+
(Assignee)

Comment 2

9 months ago
Girogio, still working on this?
Flags: needinfo?(g.maone)

Comment 3

9 months ago
Actually not, because I don't know directly about extensions which actually depend on it (I fixed the half I needed for mine).
I could in future, after finishing NoScript as a WebExtension, so if it's a priority feel free to take it.
Assignee: g.maone → nobody
Flags: needinfo?(g.maone)

Comment 4

8 months ago
Hi,
we are planning to port our produxt's firefox plugin to webextension.
We are blocked on this API. Is there an ETA on when the API would be available.
Another reason we want to port the plugin to the webextension mode is due to the E10s feature.
(Assignee)

Comment 5

7 months ago
Created attachment 8819484 [details] [diff] [review]
WIP authRequired

WIP patch for onAuthRequired.  Looking for feedback on the way I've hooked into notificationCallbacks and modified runChannelListener.  This works, need to write tests, but it has one difference from the chromium implementation.

Per https://developer.chrome.com/extensions/webRequest

The flow after onAuthRequired goes back to onBeforeSendHeaders, however our implementation goes back to onBeforeRequest.  Haven't looked into how to identify this easily to skip that call, also not sure if it's necessary.
Assignee: nobody → mixedpuppy
Attachment #8819484 - Flags: feedback?(kmaglione+bmo)

Comment 6

7 months ago
Comment on attachment 8819484 [details] [diff] [review]
WIP authRequired

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

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html
@@ +28,5 @@
> +  // SelfSupport has a tendency to fire when running this test alone, without
> +  // a good way to turn it off we just set the url to ""
> +  yield SpecialPowers.pushPrefEnv({
> +    set: [["browser.selfsupport.url", ""]],
> +  });

Please move this to the head_webrequest file.

@@ +50,5 @@
> +  yield extension.awaitMessage("continue");
> +
> +  yield testXHR(SimpleTest.getTestFileURL("auth.sjs"));
> +
> +  yield extension.awaitMessage("done");

We need to test the behavior with cached auth credentials, fallback to the default auth prompt, multiple listeners, ...

::: toolkit/modules/addons/WebRequest.jsm
@@ +540,5 @@
> +      //  "ONLY_PASSWORD":8,"PREVIOUS_FAILED":16,"CROSS_ORIGIN_SUB_RESOURCE":32}
> +      let data = {
> +        scheme: authInfo.authenticationScheme,
> +        realm: authInfo.realm,
> +        isProxy: authInfo.flags & authInfo.AUTH_PROXY,

This needs to be a boolean.

@@ +550,5 @@
> +      this.runChannelListener(channel, this.getLoadContext(channel),
> +                              "authRequired", data, (result) => {
> +                                dump(`authRequired callback gets ${JSON.stringify(result)}\n`);
> +                                if (!result) {
> +                                  callback.onAuthCancelled(context, true);

We need to fall back to default auth handlers here, and correctly handle multiple authRequired listeners.

@@ +560,5 @@
> +                                }
> +                              });
> +    } catch (e) {
> +      dump(`---- asyncPromptAuth failure ${e.message}\n`);
> +    }

We need to return an nsICancelable here.

@@ +710,5 @@
>  
>      return Object.assign(data, extraData);
>    },
>  
> +  runChannelListener(channel, loadContext = null, kind, extraData = null, channelCallback = null) {

I'd rather we just have `runChannelListeners` return the listener results for authRequired listeners than pass a callback and add all these special cases.

@@ +888,5 @@
>      this.runChannelListener(channel, loadContext, "headersReceived");
> +
> +    if (this.listeners.authRequired.size) {
> +      dump(`=== set notificationCallbacks in examine, TODO better place to do this?\n`);
> +      channel.notificationCallbacks = this;

If we're going to replace channel notificationCallbacks, we need to only do it if there are auth listeners that actually match this request, and to forward interfaces that we don't support to the original callbacks, and to forward to the original auth handler when a listener doesn't provide credentials.

But actually finding the correct auth handler to forward to is non-trivial, and could easily get out of sync with necko's logic, so I'd much rather we come up with a cleaner way to add global auth listeners, similar to what we do for "net-channel-event-sinks" category entries.
Attachment #8819484 - Flags: feedback?(kmaglione+bmo)

Updated

7 months ago
webextensions: --- → +
(Assignee)

Comment 7

7 months ago
> If we're going to replace channel notificationCallbacks, we need to only do
> it if there are auth listeners that actually match this request, and to
> forward interfaces that we don't support to the original callbacks, and to
> forward to the original auth handler when a listener doesn't provide
> credentials.
> 
> But actually finding the correct auth handler to forward to is non-trivial,
> and could easily get out of sync with necko's logic, so I'd much rather we
> come up with a cleaner way to add global auth listeners, similar to what we
> do for "net-channel-event-sinks" category entries.

I've opted to continue with notificationCallbacks for now as there are a bunch of paths using it to prompt for authorization.  I think a followup would be to fix/replace notificationCallbacks or at least provide an alternative to it.
(Assignee)

Comment 8

7 months ago
Created attachment 8821259 [details] [diff] [review]
WIP authRequired V2

This improves on the previous and should be handling the comments you made before.  I still have to add tests for proxy auth, but would like to see if you have further comments on the direction of the patch.
Attachment #8819484 - Attachment is obsolete: true
Attachment #8821259 - Flags: feedback?(kmaglione+bmo)

Comment 9

7 months ago
Comment on attachment 8821259 [details] [diff] [review]
WIP authRequired V2

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

If we're going to go this route, please file a follow-up to handle this in necko without having to wrap notification callbacks on every channel it affects.

::: toolkit/components/extensions/test/mochitest/mochitest.ini
@@ +1,4 @@
>  [DEFAULT]
>  tags = webextensions in-process-webextensions
>  
> +

Not necessary.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html
@@ +14,5 @@
> +
> +let baseUrl = "http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/authenticate.sjs";
> +function testXHR(url) {
> +  return new Promise(resolve => {
> +    let xhr = new XMLHttpRequest();

Please use fetch instead.

@@ +87,5 @@
> +  // expecting origin == undefined
> +  extension.sendMessage("set-expected", {expect, origin: location.href});
> +  yield extension.awaitMessage("continue");
> +
> +  yield testXHR(baseUrl + `?realm=webRequest_auth&user=${authInfo.username}&pass=${authInfo.password}`);

`${baseUrl}?realm=...`

::: toolkit/modules/addons/WebRequest.jsm
@@ +437,5 @@
> +    throw Cr.NS_ERROR_NO_INTERFACE;
> +  },
> +
> +  promptAuth(channel, level, authInfo) {
> +    throw Cr.NS_ERROR_NOT_IMPLEMENTED;

We should fallback to the previous callbacks here.

@@ +456,5 @@
> +          port: uri.port,
> +        },
> +      };
> +
> +      channel.notificationCallbacks = this.notificationCallbacks;

This is going to cause problems if some other caller tries to chain callbacks the same way that we're doing.

@@ +463,5 @@
> +      // In the case that no listener provides credentials, we fallback to the
> +      // previously set callback class for authentication.
> +      channelData.authPromptForward = () => {
> +        let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL;
> +        let provider = channel.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider);

We should probably stick to our saved callbacks here. And we need to handle the other cases that necko handles:

http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#362-383

@@ +471,5 @@
> +      channelData.authPromptCallback = (authCredentials) => {
> +        // The API allows for canceling the request, providing credentials or
> +        // doing nothing, so we do not provide a way to call onAuthCanceled.
> +        // Canceling the request will result in canceling the authentication.
> +        if (authCredentials) {

We need to verify that username and password are valid string properties.

@@ +478,5 @@
> +
> +          callback.onAuthAvailable(context, authInfo);
> +          // At least one addon has responded, so we wont forward to the regular
> +          // prompt handlers.
> +          delete channelData.authPromptForward;

Set to null instead so we don't change the shape of the object.

We also need to clear the authPromptCallback.

@@ +488,5 @@
> +
> +      return {
> +        QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]),
> +        callback: callback,
> +        context: context,

callback,
    context,
    cancel() {
      ...;
    }

However, given that this is already a closure, I'm not sure why we need the `context` and `callback` member properties.

@@ +893,5 @@
>          if (opts.responseHeaders && result.responseHeaders && responseHeaders) {
>            responseHeaders.applyChanges(result.responseHeaders);
>          }
> +
> +        if (opts.blocking && result.authCredentials) {

We should only be doing this for authRequired listeners.

@@ +949,3 @@
>      }
>  
> +    if (this.listeners.authRequired.size) {

We need to check whether these listeners actually apply to this channel. Same for headersReceived.

@@ +1008,5 @@
>  
>  var onBeforeSendHeaders = new HttpEvent("modify", ["requestHeaders", "blocking"]);
>  var onSendHeaders = new HttpEvent("afterModify", ["requestHeaders"]);
>  var onHeadersReceived = new HttpEvent("headersReceived", ["blocking", "responseHeaders"]);
> +var onAuthRequired = new HttpEvent("authRequired", ["blocking", "responseHeaders"]); // TODO asyncBlocking

We already handle async blocking for all listeners. No need to treat this any differently.
Attachment #8821259 - Flags: feedback?(kmaglione+bmo) → feedback+
(Assignee)

Comment 10

7 months ago
(In reply to Kris Maglione [:kmag] from comment #9)

> @@ +949,3 @@
> >      }
> >  
> > +    if (this.listeners.authRequired.size) {
> 
> We need to check whether these listeners actually apply to this channel.
> Same for headersReceived.

runChannelListener does that, do you feel we should additionally check before that?  That seems to make sense for auth since we're hooking into notificationCallbacks, but I'm not sure it makes sense to do for headersReceived.

> @@ +1008,5 @@
> >  
> >  var onBeforeSendHeaders = new HttpEvent("modify", ["requestHeaders", "blocking"]);
> >  var onSendHeaders = new HttpEvent("afterModify", ["requestHeaders"]);
> >  var onHeadersReceived = new HttpEvent("headersReceived", ["blocking", "responseHeaders"]);
> > +var onAuthRequired = new HttpEvent("authRequired", ["blocking", "responseHeaders"]); // TODO asyncBlocking
> 
> We already handle async blocking for all listeners. No need to treat this
> any differently.

I'm not clear about your comment.  "asyncBlocking" is an option documented for onAuthRequired that can be used instead of "blocking", it's the only webrequest listener with that option.  If you use that you are given a callback to use in the listener.  While we achieve the same thing by returning a promise from the listener, we'd still want to support asyncBlocking if we want to get to full compatibility.
(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> (In reply to Kris Maglione [:kmag] from comment #9)
> 
> > @@ +949,3 @@
> > >      }
> > >  
> > > +    if (this.listeners.authRequired.size) {
> > 
> > We need to check whether these listeners actually apply to this channel.
> > Same for headersReceived.
> 
> runChannelListener does that, do you feel we should additionally check
> before that?  That seems to make sense for auth since we're hooking into
> notificationCallbacks, but I'm not sure it makes sense to do for
> headersReceived.

Yes. My point is that we shouldn't be replacing the channel's notificationCallbacks if there are no listeners that require it.

> > @@ +1008,5 @@
> > >  
> > >  var onBeforeSendHeaders = new HttpEvent("modify", ["requestHeaders", "blocking"]);
> > >  var onSendHeaders = new HttpEvent("afterModify", ["requestHeaders"]);
> > >  var onHeadersReceived = new HttpEvent("headersReceived", ["blocking", "responseHeaders"]);
> > > +var onAuthRequired = new HttpEvent("authRequired", ["blocking", "responseHeaders"]); // TODO asyncBlocking
> > 
> > We already handle async blocking for all listeners. No need to treat this
> > any differently.
> 
> I'm not clear about your comment.  "asyncBlocking" is an option documented
> for onAuthRequired that can be used instead of "blocking", it's the only
> webrequest listener with that option.  If you use that you are given a
> callback to use in the listener.  While we achieve the same thing by
> returning a promise from the listener, we'd still want to support
> asyncBlocking if we want to get to full compatibility.

I'm not worried about 100% compatibility. If at some point we decide we need to support that convention in the `chrome` namespace, we can add a wrapper at the binding layer.
(Assignee)

Comment 12

7 months ago
(In reply to Kris Maglione [:kmag] from comment #9)
> :::
> toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html
> @@ +14,5 @@
> > +
> > +let baseUrl = "http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/authenticate.sjs";
> > +function testXHR(url) {
> > +  return new Promise(resolve => {
> > +    let xhr = new XMLHttpRequest();
> 
> Please use fetch instead.

Fetch doesn't support authentication.  Bug 1120722.
https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#289
Comment hidden (mozreview-request)
(Assignee)

Comment 14

7 months ago
Comment on attachment 8821259 [details] [diff] [review]
WIP authRequired V2

I believe I have everything covered now, so pushed to review.
Attachment #8821259 - Attachment is obsolete: true

Comment 15

6 months ago
mozreview-review
Comment on attachment 8821372 [details]
Bug 1190689 implement onAuthRequired for WebRequest,

https://reviewboard.mozilla.org/r/100680/#review104758

Looks good for the most part. I'm mainly just worried that we have insufficient tests for some corner cases at this point.

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:185
(Diff revision 1)
>          expectedEvent = expected.optional_events.includes(name);
>        }
>        browser.test.assertTrue(expectedEvent, `received ${name}`);
>        browser.test.assertEq(expected.type, details.type, "resource type is correct");
> +      // ignore origin test for generated background page
> +      if (details.originUrl.split("/").pop() != "_generated_background_page.html") {

if (!details.originUrl.endsWith("/_generated_background_page.html"))

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:23
(Diff revision 1)
> +    xhr.onabort = () => {
> +      resolve();
> +    };
> +    xhr.onerror = () => {
> +      resolve();
> +    };

Shouldn't we reject in these cases?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:43
(Diff revision 1)
> +    }, {urls: ["<all_urls>"]}, ["blocking"]);
> +    browser.webRequest.onErrorOccurred.addListener((details) => {
> +      browser.test.succeed("request canceled");
> +      browser.test.sendMessage("done");
> +    }, {urls: ["<all_urls>"]});
> +    browser.test.sendMessage("started");

No need for this.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:143
(Diff revision 1)
> +    browser.webRequest.onAuthRequired.addListener((details) => {
> +      browser.test.log(`handlingExt called with ${details.requestId} ${details.url}`);
> +      browser.test.sendMessage("done");
> +      return {authCredentials: {username: "foobar", password: "testpass"}};
> +    }, {urls: ["<all_urls>"]}, ["blocking"]);
> +    browser.test.sendMessage("started");

Not necessary.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:207
(Diff revision 1)
> +  function background() {
> +    browser.webRequest.onAuthRequired.addListener((details) => {
> +      browser.test.succeed(`handlingExt onAuthRequired called with ${details.requestId} ${details.url}`);
> +      browser.test.sendMessage("done");
> +    }, {urls: ["<all_urls>"]}, ["blocking"]);
> +    browser.test.sendMessage("started");

Not necessary.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:233
(Diff revision 1)
> +
> +  yield handlingExt.awaitMessage("done");
> +  yield handlingExt.unload();
> +  yield canceler.awaitMessage("done");
> +  yield canceler.unload();
> +});

I think we're still missing the case where there are two blocking listeners, where the first doesn't return any credentials and the second does. Also, ideally separately for cases where it returns an empty object and where it returns undefined.

We should also probably have listeners that return no credentials that are called after, and two listeners that both return credentials for the same request.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:249
(Diff revision 1)
> +    const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +    Cu.import("resource://gre/modules/Services.jsm");
> +    Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +    let observer = channel => {
> +      if (channel instanceof Ci.nsIHttpChannel && channel.URI.host === "mochi.test") {

Can we return early here instead just to save a level of indentation?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:267
(Diff revision 1)
> +          getInterface(iid) {
> +            if (iid.equals(Ci.nsIAuthPromptProvider) || iid.equals(Ci.nsIAuthPrompt2)) {
> +              return this;
> +            }
> +            return callbacks.getInterface(iid);
> +          },

Please make sure to test this separately with the nsIAuthPromptProvider and nsIAuthPrompt2 cases, and make sure that code gets complete coverage in coverage tests.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:275
(Diff revision 1)
> +          },
> +          getAuthPrompt(reason, iid) {
> +            return this;
> +          },
> +          asyncPromptAuth(channel, callback, context, level, authInfo) {
> +            channel.notificationCallbacks = callbacks;

Why?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:298
(Diff revision 1)
> +    }, {urls: ["<all_urls>"]}, ["blocking"]);
> +    browser.webRequest.onErrorOccurred.addListener((details) => {
> +      browser.test.succeed("request canceled");
> +      browser.test.sendMessage("done");
> +    }, {urls: ["<all_urls>"]});
> +    browser.test.sendMessage("started");

Not necessary.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:337
(Diff revision 1)
> +        return {authCredentials: {username: "puser", password: "ppass"}};
> +      }
> +      browser.test.assertTrue(proxyOk, "providing www authorization after proxy auth");
> +      browser.test.sendMessage("done");
> +      return {authCredentials: {username: "auser", password: "apass"}};
> +    }, {urls: ["<all_urls>"]}, ["blocking"]);

We should probably use a better URL filter in all of these.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:338
(Diff revision 1)
> +      }
> +      browser.test.assertTrue(proxyOk, "providing www authorization after proxy auth");
> +      browser.test.sendMessage("done");
> +      return {authCredentials: {username: "auser", password: "apass"}};
> +    }, {urls: ["<all_urls>"]}, ["blocking"]);
> +    browser.test.sendMessage("started");

Not necessary.

::: toolkit/modules/addons/WebRequest.jsm:404
(Diff revision 1)
>  };
>  
>  ChannelEventSink.init();
>  
> +// nsIAuthPrompt2 implementation for onAuthRequired
> +function AuthRequestor(notificationCallbacks, httpObserver) {

Please use an ES6 class.

::: toolkit/modules/addons/WebRequest.jsm:440
(Diff revision 1)
> +    // This should never get called without getInterface having been called first.
> +    if (iid.equals(Ci.nsIAuthPrompt2)) {
> +      return this;
> +    }
> +    let provider = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider);
> +    if (provider) {

No need for this. `getInterface` will throw if the interface isn't available.

::: toolkit/modules/addons/WebRequest.jsm:449
(Diff revision 1)
> +    if (!prompt) {
> +      throw Cr.NS_ERROR_NO_INTERFACE;
> +    }

Not necessary. `getInterface` will throw if it doesn't provide this interface.

::: toolkit/modules/addons/WebRequest.jsm:472
(Diff revision 1)
> +
> +    let channelData = getData(channel);
> +    // In the case that no listener provides credentials, we fallback to the
> +    // previously set callback class for authentication.
> +    channelData.authPromptForward = () => {
> +      let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL;

Please move this to just before the `getAuthPrompt` call.

::: toolkit/modules/addons/WebRequest.jsm:475
(Diff revision 1)
> +    // previously set callback class for authentication.
> +    channelData.authPromptForward = () => {
> +      let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL;
> +      let provider = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider);
> +      let prompt;
> +      if (provider) {

`getInterface` will throw if the interface isn't supported, so this needs to be a try-catch rather than an if.

::: toolkit/modules/addons/WebRequest.jsm:495
(Diff revision 1)
> +        authInfo.password = authCredentials.password;
> +
> +        callback.onAuthAvailable(context, authInfo);
> +        // At least one addon has responded, so we wont forward to the regular
> +        // prompt handlers.
> +        channelData.authPromptForward = null;

We also probably need to null out `authPromptCallback` here.

::: toolkit/modules/addons/WebRequest.jsm:505
(Diff revision 1)
> +    this.httpObserver.runChannelListener(channel, loadContext, "authRequired", data);
> +
> +    return {
> +      QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]),
> +      cancel() {
> +        callback.onAuthCancelled(this.context, false);

s/this.//

::: toolkit/modules/addons/WebRequest.jsm:505
(Diff revision 1)
> +    this.httpObserver.runChannelListener(channel, loadContext, "authRequired", data);
> +
> +    return {
> +      QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]),
> +      cancel() {
> +        callback.onAuthCancelled(this.context, false);

We also probably need to null things out here.

::: toolkit/modules/addons/WebRequest.jsm:935
(Diff revision 1)
> +    if (listener.size < 1) {
> +      return false;
> +    }

This is probably an unnecessary optimization. If you keep it, though, please use `listener.size == 0`
Attachment #8821372 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 16

6 months ago
mozreview-review-reply
Comment on attachment 8821372 [details]
Bug 1190689 implement onAuthRequired for WebRequest,

https://reviewboard.mozilla.org/r/100680/#review104758

> I think we're still missing the case where there are two blocking listeners, where the first doesn't return any credentials and the second does. Also, ideally separately for cases where it returns an empty object and where it returns undefined.
> 
> We should also probably have listeners that return no credentials that are called after, and two listeners that both return credentials for the same request.

The first modification is supposed to win, so subsequent listeners can only cancel a request.
Comment hidden (mozreview-request)
(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> The first modification is supposed to win, so subsequent listeners can only
> cancel a request.

Right, the first modification is supposed to win, but we still need to test multiple handlers trying to return auth data for the same request behave sanely. If at some point, we wind up making a change that causes the auth callback to be called multiple times in that case, for instance, and that causes the browser to crash, that's something I'd like to know before it makes it to production.

Comment 19

6 months ago
mozreview-review
Comment on attachment 8821372 [details]
Bug 1190689 implement onAuthRequired for WebRequest,

https://reviewboard.mozilla.org/r/100680/#review106412

Looks good so far. Leaving r? set until you finish addressing the issues from the last review.

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:167
(Diff revisions 1 - 2)
> -  function getListener(name) {
> -    return details => {
> +  let listeners = {
> +    onBeforeRequest(expected, details, result) {

In the future, can you try to put as much as these kinds of refactorings as possible into a separate patches?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:178
(Diff revisions 1 - 2)
> -  };
> +  yield testXHR(`${baseUrl}?realm=test_webRequest_auth_cancelled&user=${authCredentials.username}&pass=${authCredentials.password}`)
> +              .then(() => {
> +                ok(false, "xhr succeeded");
> +              })
> +              .catch(() => {
> +                ok(true, "caught rejected xhr");
> +              });

yield Assert.rejects(testXHR(...), ...);

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:235
(Diff revisions 1 - 2)
> -  function background() {
> -    browser.webRequest.onAuthRequired.addListener((details) => {
> -      browser.test.succeed(`handlingExt onAuthRequired called with ${details.requestId} ${details.url}`);
> -      browser.test.sendMessage("done");
> -    }, {urls: ["<all_urls>"]}, ["blocking"]);
> -    browser.test.sendMessage("started");
> +  let ext = getAuthHandler();
> +  yield ext.startup();
> +  let canceler = getAuthHandler({cancel: true});
> +  yield canceler.startup();
> +
> +  yield testXHR(`${baseUrl}?realm=auth_blocking_noreturn&user=auth_blocking_noreturn&pass=auth_blocking_noreturn`)

yield Assert.rejects(testXHR(...), ...);

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:267
(Diff revisions 1 - 2)
>      const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>      Cu.import("resource://gre/modules/Services.jsm");
>      Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>      let observer = channel => {
> -      if (channel instanceof Ci.nsIHttpChannel && channel.URI.host === "mochi.test") {
> +      if (!(channel instanceof Ci.nsIHttpChannel && channel.URI.host === "mochi.test")) {

I think I'd prefer `!(channel instanceof nsIHttpChannel) || host !== "mochi.test"`, but I don't have that strong an opinion on it.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:313
(Diff revisions 1 - 2)
> -  });
> -
>    yield handlingExt.startup();
> -  yield handlingExt.awaitMessage("started");
>  
> -  yield testXHR(`${baseUrl}?realm=auth_nonblocking_forwardAuth&user=auth_nonblocking_forwardAuth&pass=auth_nonblocking_forwardAuth`);
> +  yield testXHR(`${baseUrl}?realm=auth_nonblocking_forwardAuth&user=auth_nonblocking_forwardAuth&pass=auth_nonblocking_forwardAuth`)

yield Assert.rejects(textXHR(...), ...);

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:402
(Diff revisions 1 - 2)
> +              .then(() => {
> +                ok(true, "xhr succeeded");
> +              })
> +              .catch(() => {
> +                ok(false, "caught rejected xhr");
> +              });

If we need this handler, it should be a single `.then()` handler with success and error callbacks. But I don't think we need it. The rejected promise on its own should cause the test to fail.

Same for the other instances.

::: toolkit/modules/addons/WebRequest.jsm:468
(Diff revisions 1 - 2)
>      channelData.authPromptForward = () => {
> -      let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL;
> -      let provider = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider);
>        let prompt;
> -      if (provider) {
> -        prompt = provider.getAuthPrompt(reason, Ci.nsIAuthPrompt2);
> -      } else {
> +      try {
> +        let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL;
> +        prompt = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider)
> +                                           .getAuthPrompt(reason, Ci.nsIAuthPrompt2);
> +      } catch (e) {
>          prompt = this.notificationCallbacks.getInterface(Ci.nsIAuthPrompt2);
>        }
>        prompt.asyncPromptAuth(channel, callback, context, level, authInfo);
>      };

We should probably put a try-catch around this entire body, in case the second `getInterface` or `asyncPromptAuth` throw.

::: toolkit/modules/addons/WebRequest.jsm:489
(Diff revisions 1 - 2)
>            typeof authCredentials.username === "string" &&
>            typeof authCredentials.password === "string") {
>          authInfo.username = authCredentials.username;
>          authInfo.password = authCredentials.password;
>  
>          callback.onAuthAvailable(context, authInfo);

We should also probably put a try-catch around this.
(Assignee)

Comment 20

6 months ago
(In reply to Kris Maglione [:kmag] from comment #18)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> > The first modification is supposed to win, so subsequent listeners can only
> > cancel a request.
> 
> Right, the first modification is supposed to win, but we still need to test
> multiple handlers trying to return auth data for the same request behave
> sanely. If at some point, we wind up making a change that causes the auth
> callback to be called multiple times in that case, for instance, and that
> causes the browser to crash, that's something I'd like to know before it
> makes it to production.

Yep, that last update added tests for several scenarios including this.
Comment hidden (mozreview-request)
Blocks: 1332408

Comment 22

6 months ago
mozreview-review
Comment on attachment 8821372 [details]
Bug 1190689 implement onAuthRequired for WebRequest,

https://reviewboard.mozilla.org/r/100680/#review107150

r=me with issues addressed. Thanks!

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:17
(Diff revisions 2 - 3)
> +    return promise.then(() => {
> +      ok(false, msg);
> +    }).catch(() => {
> +      ok(true, msg);
> +    });

One `.then` clause with success and failure handlers, please.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:260
(Diff revisions 2 - 3)
>          QueryInterface(iid) {
>            if (iid.equals(Ci.nsISupports) ||
>                iid.equals(Ci.nsIInterfaceRequestor) ||
>                iid.equals(Ci.nsIAuthPromptProvider) ||
>                iid.equals(Ci.nsIAuthPrompt2)) {
>              return this;
>            }
> -          return callbacks.QueryInterface(iid);
> +          throw Cr.NS_ERROR_NO_INTERFACE;
>          },

`XPCOMUtils.generateQI`, please.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:269
(Diff revisions 2 - 3)
>          getInterface(iid) {
>            if (iid.equals(Ci.nsIAuthPromptProvider) || iid.equals(Ci.nsIAuthPrompt2)) {
>              return this;
>            }
> -          return callbacks.getInterface(iid);
> +          throw Cr.NS_ERROR_NO_INTERFACE;
>          },

This can also use `generateQI`.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:330
(Diff revisions 2 - 3)
> +        QueryInterface(iid) {
> +          if (iid.equals(Ci.nsISupports) ||
> +              iid.equals(Ci.nsIInterfaceRequestor) ||
> +              iid.equals(Ci.nsIAuthPrompt2)) {
> +            return this;
> +          }
> +          throw Cr.NS_ERROR_NO_INTERFACE;
> +        },

`XPCOMUtils.generateQI` please.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_auth.html:351
(Diff revisions 2 - 3)
> +        },
> +        asyncPromptAuth(channel, callback, context, level, authInfo) {
> +          // We just cancel here, we're only ensuring that non-webrequest
> +          // notificationcallbacks get called if webrequest doesn't handle it.
> +          Promise.resolve().then(() => {
> +            channel.cancel(Cr.NS_BINDING_ABORTED);

I'm pretty sure we still need to call the auth prompt callback here, even if we cancel the request.

::: toolkit/modules/addons/WebRequest.jsm:406
(Diff revisions 2 - 3)
> -    this.notificationCallbacks = notificationCallbacks;
> +    this.notificationCallbacks = channel.notificationCallbacks;
> +    this.loadGroupCallbacks = channel.loadGroup && channel.loadGroup.notificationCallbacks;

We should null both of these out when we're done, just to be safe.

::: toolkit/modules/addons/WebRequest.jsm:440
(Diff revisions 2 - 3)
>    getInterface(iid) {
>      if (iid.equals(Ci.nsIAuthPromptProvider) || iid.equals(Ci.nsIAuthPrompt2)) {
>        return this;
>      }
> -    if (this.notificationCallbacks) {
> -      return this.notificationCallbacks.getInterface(iid);
> +    try {
> +      return this._getForwardedInterface(iid);

I think we only want to forward to the channel `notificationCallbacks` here, not the load group. Same for QI.

::: toolkit/modules/addons/WebRequest.jsm:480
(Diff revisions 2 - 3)
> -        let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL;
> +          let reason = data.isProxy ? Ci.nsIAuthPromptProvider.PROMPT_PROXY : Ci.nsIAuthPromptProvider.PROMPT_NORMAL;
> -        prompt = this.notificationCallbacks.getInterface(Ci.nsIAuthPromptProvider)
> +          prompt = this._getForwardedInterface(Ci.nsIAuthPromptProvider).getAuthPrompt(reason, Ci.nsIAuthPrompt2);
> -                                           .getAuthPrompt(reason, Ci.nsIAuthPrompt2);
> -      } catch (e) {
> +        } catch (e) {
> -        prompt = this.notificationCallbacks.getInterface(Ci.nsIAuthPrompt2);
> +          prompt = this._getForwardedInterface(Ci.nsIAuthPrompt2);
> -      }
> +        }

This is slightly wrong. We need to check the channel's notifcationCallbacks for nsIAuthPromptProvider and nsIAuthPrompt2 before we fall back to checking the loadGroup for either.

::: toolkit/modules/addons/WebRequest.jsm:486
(Diff revisions 2 - 3)
> +      } catch (e) {
> +        Cu.reportError(`webRequest asyncPromptAuth failure ${e}`);
> +      }

If we fail to get a prompt above, we need to call onAuthCanceled ourselves. And I think we also need to null out the `authPromptForward` and `authPromptCallback` callbacks here. Channels aren't cycle collected, so if these aren't cleared, the channel will leak.
Attachment #8821372 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 23

6 months ago
mozreview-review-reply
Comment on attachment 8821372 [details]
Bug 1190689 implement onAuthRequired for WebRequest,

https://reviewboard.mozilla.org/r/100680/#review107150

> We should null both of these out when we're done, just to be safe.

Discussed on irc.  There's no good way to do this and it shouldn't be an issue, but we'll revisit if it is.
Comment hidden (mozreview-request)
(Assignee)

Comment 25

6 months ago
An additional try run with a webrequest addon that is loaded for all tests.  Doing this to verify this wouldn't break other existing tests for http auth.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1f5f06f3d8de5d6ee895811f230fdd6dcee3efa
(Assignee)

Comment 26

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3144bf5ed09c
(Assignee)

Comment 27

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5287d33467e
Comment hidden (mozreview-request)
(Assignee)

Comment 29

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb5e05817f45
Comment hidden (mozreview-request)

Comment 31

6 months ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8b0fc745a65e
implement onAuthRequired for WebRequest, r=kmag

Comment 32

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b0fc745a65e
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onAuthRequired to be promise-y and remove references to asyncBlocking, and updated the compat data.

Please let me know if we need anything else!
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 34

3 months ago
Docs LGTM
(Assignee)

Updated

3 months ago
Flags: needinfo?(mixedpuppy)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.