Restrict pings to same host and limit to no more than one per anchor

RESOLVED FIXED in mozilla1.9alpha1

Status

()

enhancement
P2
normal
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: darin.moz, Assigned: darin.moz)

Tracking

Trunk
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
Restrict pings to same origin and limit to no more than one per anchor.

My goal with this change is to introduce some preferences that can be used to tweak the behavior of <a ping>.  For example, I feel that a lot of the criticisms against <a ping> would be alieviated if they were restricted to same origin and restricted to no more than one per link click.
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

13 years ago
Priority: -- → P2
(Assignee)

Comment 1

13 years ago
Posted patch v1 patch (obsolete) — Splinter Review
Attachment #215930 - Flags: superreview?(bzbarsky)
Attachment #215930 - Flags: review?(cbiesinger)
(Assignee)

Comment 2

13 years ago
The attached patch implements support for limiting the number of pings per link and the limiting pings to the same host as the referring document.  It is possible that we might prefer to implement a same-origin (scheme://host:port) restriction, but I'm not sure that there is much value in that.

Also, I chose to use prefs to apply the same-host restriction instead of trying to leverage the capability system since I'm not sure how to use the capability system for something like this without hacking the capability system directly.
(Assignee)

Comment 3

13 years ago
Comment on attachment 215930 [details] [diff] [review]
v1 patch

Requesting review from Ben on the Firefox link properties (metaData.js) changes.
Attachment #215930 - Flags: review?(bugs)
Comment on attachment 215930 [details] [diff] [review]
v1 patch

nsWebShell.cpp
+nsPingListener::GetInterface(const nsIID &iid, void **result)
+{
+  return QueryInterface(iid, result);

I'd prefer it if this just checked for the single IID it wants to give out. I don't think it makes sense to give out, for example, an nsIRequestObserver here.

   chan->AsyncOpen(listener, nsnull);
 
+  // Count this as a successful ping since failure from this point forward does
+  // not mean the ping wasn't sent.

Strictly speaking, asyncOpen could have failed, and that would mean that the ping was not sent :-) but, that's an edge case, and probably doesn't really matter.

+  if (!PingsEnabled(&info.maxPings, &info.requireSameHost))
+    return;
+  if (info.numPings == 0)

Shouldn't the if check maxPings rather than numPings (which is uninitialized at this point)?

Is there a specific reason not to just use the first N URLs listed in the attribute, but making it dependent on whether they are on the same host?

browser/base/content/metaData.js
+      Components.classes["@mozilla.org/preferences-service;1"].
+      getService(Components.interfaces.nsIPrefBranch);

not sure whether that's the usual indentation for the getService line but I'll let ben comment on that :)

this function will throw an exception if one of the prefs isn't set, shouldn't it return an empty array instead or something?

+  var docURI = ios.newURI(elem.ownerDocument.documentURI, null, null);

maybe pass an origin charset here? (elem.ownerDocument.characterSet)

+      var uri = ios.newURI(pings[i], null, null);

what about relative URIs? (and character sets)



>Also, I chose to use prefs to apply the same-host restriction instead of trying
>to leverage the capability system since I'm not sure how to use the capability
>system for something like this without hacking the capability system directly.

Well, what you could do is call checkSameOriginURI: http://lxr.mozilla.org/seamonkey/source/caps/idl/nsIScriptSecurityManager.idl

(or maybe even the principal version, which may be more correct, but requires having a target principal)

although, the interface does sound like scheme and port have to match as well, so that'd seem like the wrong thing here.
Attachment #215930 - Flags: review?(cbiesinger) → review+
hm, ignore the last part of my comment about same origin, I should've read the first paragraph of comment 2 before :)
(Assignee)

Comment 6

13 years ago
> Is there a specific reason not to just use the first N URLs listed in the
> attribute, but making it dependent on whether they are on the same host?

Well, it seemed best to me.  Consider this example:

http://foo.com/
  <html>
  <body>
  <a ping="http://bar.com/ http://foo.com/" href="...">...</a>
  ...

Now imagine if max_per_link=1 and require_same_host=true.  If we only looked at the first link, then we would send no pings.  If the pings were re-ordered, then we would send one ping.  That seems bad to me.  I prefer applying the require_same_host restriction first.
(Assignee)

Comment 7

13 years ago
> this function will throw an exception if one of the prefs isn't set, 
> shouldn't it return an empty array instead or something?

I think Firefox should always define the prefs, so I'd like to see exceptions to tell me when a pref wasn't defined, so I can ensure that it is defined.  Doesn't that seem like a good idea?


>+  var docURI = ios.newURI(elem.ownerDocument.documentURI, null, null);
>
>maybe pass an origin charset here? (elem.ownerDocument.characterSet)
>
>+      var uri = ios.newURI(pings[i], null, null);
>
>what about relative URIs? (and character sets)

Well, I'm only interested in the host portion of the URLs and origin charset does not affect that portion of the URL string.  Of course, I suppose that is relying on an implementation detail.  OK, I'll pass the origin charset.
(Assignee)

Comment 8

13 years ago
> >what about relative URIs? (and character sets)

The value returned from "elem.ping" is an absolute URI, much like the value returned from "elem.href" for an anchor tag.
(Assignee)

Comment 9

13 years ago
Posted patch v1.1 patch (obsolete) — Splinter Review
revised patch
Attachment #215930 - Attachment is obsolete: true
Attachment #216066 - Flags: superreview?(bzbarsky)
Attachment #215930 - Flags: superreview?(bzbarsky)
Attachment #215930 - Flags: review?(bugs)
(Assignee)

Updated

13 years ago
Attachment #216066 - Flags: review?(bugs)
(In reply to comment #7)
> I think Firefox should always define the prefs, so I'd like to see exceptions
> to tell me when a pref wasn't defined, so I can ensure that it is defined. 
> Doesn't that seem like a good idea?

well, it kinda depends if you want to assume that or not. C++ code at least tends to be more tolerant about nonexistant prefs... well, I don't really mind either way.
(Assignee)

Comment 11

13 years ago
Yeah, necko in particular is designed to work properly without a pref system.
Comment on attachment 216066 [details] [diff] [review]
v1.1 patch

the UI piece looks ok to me, although: 

nit: 

>Index: browser/base/content/metaData.js

>+  var pings = elem.ping.split(' ');

This file uses double quotes " " around strings, you should do that here, too. 

And you should add some documentation to this function saying what its purpose is.
Attachment #216066 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

13 years ago
Will do, thanks Ben!
(Assignee)

Comment 14

13 years ago
Posted patch v1.2 patchSplinter Review
Attachment #216066 - Attachment is obsolete: true
Attachment #216076 - Flags: superreview?(bzbarsky)
Attachment #216066 - Flags: superreview?(bzbarsky)
(In reply to comment #6)
> the first link, then we would send no pings.  If the pings were re-ordered,
> then we would send one ping.  That seems bad to me.  I prefer applying the
> require_same_host restriction first.

I do want to note, though, that with your patch you can't tell what URI will be pinged by an ping="http://foo http://bar" attribute alone, because it makes it depend on which host the HTML is stored on. (which isn't necessarily a bad thing, but I wanted to mention it)

Another thought, though... should pings to subdomains be allowed? should setting document.domain affect the allowed hosts?
(Assignee)

Comment 16

13 years ago
> Another thought, though... should pings to subdomains be allowed? should
> setting document.domain affect the allowed hosts?

Maybe.  I'd like to leave that for later.
Comment on attachment 216076 [details] [diff] [review]
v1.2 patch

I'd really prefer CheckSameOriginPrincipal checks instead of IsSameHost.  Otherwise we're just waiting for people to file bugs about their privileged code not working.

Other than that, this looks ok.  sr=bzbarsky with the principal change made.
Attachment #216076 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 18

13 years ago
Boris: note, I'm interested in allowing pings to other protocols on other ports provided they are to the same host.  Is there a way to do that with the current principal checking functions?  Also, what's the best example to follow for doing this principal checking?
Hmm... Principal-checking will not allow cross-protocol pings, no.  I guess this is ok, then, with some comments added about why we're just comparing hosts...
(Assignee)

Comment 20

13 years ago
fixed-on-trunk with additional commentry
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Depends on: 370021
Summary: Restrict pings to same origin and limit to no more than one per anchor → Restrict pings to same host and limit to no more than one per anchor
You need to log in before you can comment on or make changes to this bug.