Closed Bug 326155 Opened 19 years ago Closed 18 years ago

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

Categories

(Core :: DOM: Navigation, enhancement, P2)

Product:
Core
Component:
DOM: Navigation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

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

References

Details

Attachments

(1 file, 2 obsolete files)

v1.2 patch
11.28 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review 
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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha
Priority: -- → P2
Attached patch v1 patch (obsolete) — — Splinter Review 

        Attachment #215930 -
        Flags: superreview?(bzbarsky)

        Attachment #215930 -
        Flags: review?(cbiesinger)

    
    

    
  
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.

    
    

    
  
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 :)

    
    

    
  
> 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.

    
    

    
  
> 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.

    
    

    
  
> >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.

    
    

    
  

      
      
      
      

        Attached 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)

    
  

        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.

    
    

    
  
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+

    
    

    
  
Will do, thanks Ben!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.2 patch
        — 
          — Splinter 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?

    
    

    
  
> 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+

    
    

    
  
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...

    
    

    
  
fixed-on-trunk with additional commentry
Status: ASSIGNED → RESOLVED
Closed: 18 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: