Closed Bug 1236940 Opened 8 years ago Closed 8 years ago

Include the property "ip" in callbacks for WebRequest.onCompleted and .onResponseStarted

Categories

(WebExtensions :: Untriaged, defect, P2)

43 Branch
defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: regseb, Assigned: aswan)

References

Details

(Whiteboard: [good first bug] triaged)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151208100201

Steps to reproduce:

In background script of a WebExtensions: add a listener on event chrome.webRequest.onCompleted.


Actual results:

The parameter 'ip' is not provide.


Expected results:

The paramter 'ip' contains the server IP adress.
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Flags: blocking-webextensions?
Priority: -- → P2
Whiteboard: triaged
Assignee: nobody → aswan
Chatting to Giorgio on IRC, you need to expose the remoteAddress property of https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannelInternal.
not certain who uses - either security or corp network.  do not believe it's a 1.0 blocker based on timing - after connection made.  giorgio is looking into DNS API
Flags: blocking-webextensions? → blocking-webextensions-
Whiteboard: triaged → [good first bug] triaged
Attachment #8715152 - Flags: review?(wmccloskey)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8715152 [details] [diff] [review]
Add ip property to chrome.webRequest.onCompleted callback

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

Thanks Andrew. You might try using eslint to get a hang of our style rules. There are instructions here:
https://wiki.mozilla.org/DevTools/CodingStandards

Normally I would r+ this, but I'd like to have a second look at the style stuff. I'll get to it more quickly the next time.

::: toolkit/components/extensions/ext-webRequest.js
@@ +40,5 @@
>          frameId: ExtensionManagement.getFrameId(data.windowId),
>          parentFrameId: ExtensionManagement.getParentFrameId(data.parentWindowId, data.windowId),
>        };
>  
> +      if ('ip' in data) {

We always use double quotes for uniformity.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
@@ +191,5 @@
>      return {};
>    }
>  
> +  let completedUrls = new Set();
> +  function onCompleted(details) {

Any reason you only tested in onCompleted? It looks like there are a couple other methods that support the ip property.

@@ +195,5 @@
> +  function onCompleted(details) {
> +    onRecord("completed", details);
> +
> +    // when resources are cached, the ip property is not present,
> +    // so only check for the ip property the first time around

Capitalize this and end with a period please.

@@ +221,5 @@
> +    if (msg == "skipCompleted") {
> +      checkCompleted = false;
> +      browser.test.sendMessage("ackSkipCompleted");
> +    }
> +    else

The else should go on the same line as the previous brace. Also, you should use braces around the else portion.

::: toolkit/modules/addons/WebRequest.jsm
@@ +337,5 @@
> +      let httpChannel = channel.QueryInterface(Ci.nsIHttpChannelInternal);
> +      try {
> +        data.ip = httpChannel.remoteAddress;
> +      }
> +      catch (e) {

This should go on the same line as the previous brace.

@@ +339,5 @@
> +        data.ip = httpChannel.remoteAddress;
> +      }
> +      catch (e) {
> +        // the remoteAddress getter throws if the address is unavailable.
> +        // but ip is an optional property so just ignore the exception.

Please capitalize sentences in comments.

@@ +341,5 @@
> +      catch (e) {
> +        // the remoteAddress getter throws if the address is unavailable.
> +        // but ip is an optional property so just ignore the exception.
> +      }
> +      

There's some trailing whitespace here. Please remove.
Attachment #8715152 - Flags: review?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #4) 
> ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
> @@ +191,5 @@
> >      return {};
> >    }
> >  
> > +  let completedUrls = new Set();
> > +  function onCompleted(details) {
> 
> Any reason you only tested in onCompleted? It looks like there are a couple
> other methods that support the ip property.

Ah, I had meant to circle back to this, thanks for the reminder.
The events for which it is documented are
onBeforeRedirect
onResponseStarted
onCompleted
onErrorOccurred

We don't (yet?) support onErrorOccurred.
For onBeforeRedirect, empirically it looks like the remoteAddress property isn't set on the HTTP channel when we get the event (in any case it's not clear from the documentation, but i guess it is meant to be the address of the server that sent the redirect?)

We certainly can and should handle onResponseStarted though, I'll add that along with the style fixes.
Summary: Webrequest onCompleted not provide IP → Include the property "ip" in callbacks for WebRequest.onCompleted and .onResponseStarted
Attachment #8715152 - Attachment is obsolete: true
Attachment #8717615 - Flags: review?(wmccloskey) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c45fef870db0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: