Bugzilla

Actually something like this:

```
diff --git a/uriloader/exthandler/nsExternalProtocolHandler.cpp b/uriloader/exthandler/nsExternalProtocolHandler.cpp
--- a/uriloader/exthandler/nsExternalProtocolHandler.cpp
+++ b/uriloader/exthandler/nsExternalProtocolHandler.cpp
@@ -159,17 +159,22 @@ nsresult nsExtProtocolChannel::OpenURL()
 #endif

     RefPtr<mozilla::dom::BrowsingContext> ctx;
     rv = mLoadInfo->GetTargetBrowsingContext(getter_AddRefs(ctx));
     if (NS_FAILED(rv)) {
       goto finish;
     }

-    RefPtr<nsIPrincipal> principal = mLoadInfo->TriggeringPrincipal();
+    RefPtr<nsIPrincipal> principal;
+    if (mLoadInfo->RedirectChainIncludingInternalRedirects().IsEmpty()) {
+      principal = mLoadInfo->TriggeringPrincipal();
+    } else {
+      mLoadInfo->RedirectChainIncludingInternalRedirects().LastElement()->GetPrincipal(getter_AddRefs(principal));
+    }
     rv = extProtService->LoadURI(mUrl, principal, ctx,
                                  mLoadInfo->GetLoadTriggeredFromExternal());

     if (NS_SUCCEEDED(rv) && mListener) {
       mStatus = NS_ERROR_NO_CONTENT;

       RefPtr<nsExtProtocolChannel> self = this;
       nsCOMPtr<nsIStreamListener> listener = mListener;
```

looks like it "works" - except that then, hardly any of these exploits open a dialog at all -- because the x-domain spoofing mitigation we (okay, I) implemented in bug 1606797 notice that principal A is trying to load an external protocol handler in a browsing context that is currently under the control of principal B, and we block that... I could move the check but that feels like a footgun if we end up adding consumers, or I could pass 2 principals which feels like a recipe for confusion. Passing the loadinfo around would help but would be a bit heavyweight, per Nika's feedback in https://phabricator.services.mozilla.com/D120879 .

Nika or Christoph, do you have thoughts on how to proceed? I think removing the x-frame checks is likely to regress bug 1636654...

Actually something like this:

```diff
diff --git a/uriloader/exthandler/nsExternalProtocolHandler.cpp b/uriloader/exthandler/nsExternalProtocolHandler.cpp
--- a/uriloader/exthandler/nsExternalProtocolHandler.cpp
+++ b/uriloader/exthandler/nsExternalProtocolHandler.cpp
@@ -159,17 +159,22 @@ nsresult nsExtProtocolChannel::OpenURL()
 #endif

     RefPtr<mozilla::dom::BrowsingContext> ctx;
     rv = mLoadInfo->GetTargetBrowsingContext(getter_AddRefs(ctx));
     if (NS_FAILED(rv)) {
       goto finish;
     }

-    RefPtr<nsIPrincipal> principal = mLoadInfo->TriggeringPrincipal();
+    RefPtr<nsIPrincipal> principal;
+    if (mLoadInfo->RedirectChainIncludingInternalRedirects().IsEmpty()) {
+      principal = mLoadInfo->TriggeringPrincipal();
+    } else {
+      mLoadInfo->RedirectChainIncludingInternalRedirects().LastElement()->GetPrincipal(getter_AddRefs(principal));
+    }
     rv = extProtService->LoadURI(mUrl, principal, ctx,
                                  mLoadInfo->GetLoadTriggeredFromExternal());

     if (NS_SUCCEEDED(rv) && mListener) {
       mStatus = NS_ERROR_NO_CONTENT;

       RefPtr<nsExtProtocolChannel> self = this;
       nsCOMPtr<nsIStreamListener> listener = mListener;
```

looks like it "works" - except that then, hardly any of these exploits open a dialog at all -- because the x-domain spoofing mitigation we (okay, I) implemented in bug 1606797 notice that principal A is trying to load an external protocol handler in a browsing context that is currently under the control of principal B, and we block that... I could move the check but that feels like a footgun if we end up adding consumers, or I could pass 2 principals which feels like a recipe for confusion. Passing the loadinfo around would help but would be a bit heavyweight, per Nika's feedback in https://phabricator.services.mozilla.com/D120879 .

Nika or Christoph, do you have thoughts on how to proceed? I think removing the x-frame checks is likely to regress bug 1636654...