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)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: darin.moz, Assigned: darin.moz)
References
Details
Attachments
(1 file, 2 obsolete files)
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.
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Updated•18 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•18 years ago
|
||
Attachment #215930 -
Flags: superreview?(bzbarsky)
Attachment #215930 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 2•18 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•18 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 4•18 years ago
|
||
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+
Comment 5•18 years ago
|
||
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•18 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•18 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•18 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•18 years ago
|
||
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•18 years ago
|
Attachment #216066 -
Flags: review?(bugs)
Comment 10•18 years ago
|
||
(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•18 years ago
|
||
Yeah, necko in particular is designed to work properly without a pref system.
Comment 12•18 years ago
|
||
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•18 years ago
|
||
Will do, thanks Ben!
Assignee | ||
Comment 14•18 years ago
|
||
Attachment #216066 -
Attachment is obsolete: true
Attachment #216076 -
Flags: superreview?(bzbarsky)
Attachment #216066 -
Flags: superreview?(bzbarsky)
Comment 15•18 years ago
|
||
(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•18 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 17•18 years ago
|
||
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•18 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?
Comment 19•18 years ago
|
||
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•18 years ago
|
||
fixed-on-trunk with additional commentry
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
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.
Description
•