323924 - Call CheckLoadURI before issuing pings

Status: RESOLVED FIXED

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

Description

[Darin Fisher]

Call CheckLoadURI before issuing pings

We should restrict pings as we restrict redirects.

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

I think it should also check with content policy (::ShouldProcess?), so content policy can selectively veto the ping.

Comment 2

[Darin Fisher]

Yeah, dveditz made the same comment to me over IM.  I plan to include that in the forthcoming patch.

Comment 3

[Darin Fisher]

*** Bug 323959 has been marked as a duplicate of this bug. ***

Comment 4

[Darin Fisher]

My patch in bug 324642 limits pings to HTTP and HTTPS only, so that should "fix" this bug as well.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

that won't fix the content policy part of this bug.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Actually, we should still call CheckLoadURI...  There's no reason we won't fix things so some sites can't link to others (eg via capabilities) at some point.

Comment 7

[Darin Fisher]

ok

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This should do it

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch v2

jst says it's ok to depend on the scriptsecuritymanager to be available.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 246722 [details] [diff] [review]
Patch to fix

r+sr=jst

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ugh, missed the comments at the top of this bug, there's some more work to be done here.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: This one's better

This one adds all needed checks both on initial load and on redirect.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 246862 [details] [diff] [review]
This one's better

>Index: base/nsWebShell.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/docshell/base/nsWebShell.cpp,v
>retrieving revision 1.681
>diff -u -8 -p -r1.681 nsWebShell.cpp
>--- base/nsWebShell.cpp	28 Nov 2006 20:47:10 -0000	1.681
>+++ base/nsWebShell.cpp	28 Nov 2006 23:24:42 -0000
>@@ -117,16 +117,17 @@
> #include "nsCExternalHandlerService.h"
> 
> #include "nsIIDNService.h"
> 
> #include "nsIPrefBranch.h"
> #include "nsIPrefService.h"
> #include "nsITimer.h"
> #include "nsIScriptSecurityManager.h"
>+#include "nsContentPolicyUtils.h"
> 
> #ifdef NS_DEBUG
> /**
>  * Note: the log module is created during initialization which
>  * means that you cannot perform logging before then.
>  */
> static PRLogModuleInfo* gLogModule = PR_NewLogModule("webshell");
> #endif
>@@ -187,16 +188,60 @@ PingsEnabled(PRInt32 *maxPerLink, PRBool
>       prefs->GetIntPref(PREF_PINGS_MAX_PER_LINK, maxPerLink);
>       prefs->GetBoolPref(PREF_PINGS_REQUIRE_SAME_HOST, requireSameHost);
>     }
>   }
> 
>   return allow;
> }
> 
>+static PRBool
- In CheckPingURI():

...
+  if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {
+    return PR_FALSE;
+  }
+}

Missing return at the end of the function. Maybe just return NS_SUCCEEDED(rv) && !NS_CP_REJECTED(shouldLoad) and avoid the explicit if check?

r+sr=jst with that fixed.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in with that fixed