Closed Bug 1412824 Opened 2 years ago Closed 2 years ago

Refactor MaybeCloseWindow to allow closing the window if toplevel data URI navigation is blocked

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

Copying over the comment from bz from Bug 1403814#c36:

> Can we even fix that within nsDSURIContentListener?

Yes.  We could factor out the logic from http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/uriloader/exthandler/nsExternalHelperAppService.cpp#1591-1598,1608-1619 and http://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2526-2560 into a helper "close this window async and return its opener if it says it was just opened" thing...
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
See Also: → 1403814
Hey Smaug, I refactored the window closing functionality from nsExternalHelperAppService. At the moment I just dumped the refactored class "nsMaybeCloseWindowService" within nsExternalHelperAppService, which on the one hand makes it easier for you to review but on the other hand it's not the right place.

Where should we put the classe nsMaybeCloseWindowService?

Other than that, the code is ready for review. I manually verified that it's working correctly and closes the blocked window.
Attachment #8924536 - Flags: feedback?(bugs)
Comment on attachment 8924536 [details] [diff] [review]
bug_1412824_refactor_maybeclosewindow.patch

>+nsIInterfaceRequestor* nsMaybeCloseWindowService::MaybeCloseWindow()
> {
I don't understand why this method returns nsIInterfaceRequestor*

>+class nsMaybeCloseWindowService final : public nsITimerCallback
Oddly named class. This isn't any service, there can be multiple instances of the class.
And drop ns-prefix.
Perhaps MAybeCloseWindowHelper

>+{
>+public:
>+  NS_DECL_THREADSAFE_ISUPPORTS
Why threadsafe refcounting?

The new class could be close to nsDocShell I think.


(IMO most of uriloader/* could be moved to closer to docshell, but others may have other opinions.)
Attachment #8924536 - Flags: feedback?(bugs) → feedback+
Really appreciate the quick turnaround time -thanks.

(In reply to Olli Pettay [:smaug] from comment #2)
> >+nsIInterfaceRequestor* nsMaybeCloseWindowService::MaybeCloseWindow()
> > {
> I don't understand why this method returns nsIInterfaceRequestor*

I tried to capture the original implementation as precisely as possible, see [1]. So, we are closing the provided window or return it's opener if the window was just opened. I added a comment explaining the behavior above MaybeCloseWindow().

[1] https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2537

> The new class could be close to nsDocShell I think.

I don't think it's worth adding a new file, but I totally agree that it should be close to nsDocShell. Since the class is now named 'MaybeCloseWindowHelper', I think it makes sense that nsDSURIContentListener hosts that class.
Attachment #8924536 - Attachment is obsolete: true
Attachment #8924690 - Flags: review?(bugs)
Comment on attachment 8924690 [details] [diff] [review]
bug_1412824_refactor_maybeclosewindow.patch

>+void MaybeCloseWindowHelper::SetShouldCloseWindow(bool aShouldCloseWindow)
Nit, void should be on its own line

>+nsIInterfaceRequestor* MaybeCloseWindowHelper::MaybeCloseWindow()
As should other return types.

> nsDSURIContentListener::~nsDSURIContentListener()
>@@ -88,16 +148,26 @@ nsDSURIContentListener::DoContent(const 
>   if (aOpenedChannel) {
>     aOpenedChannel->GetLoadFlags(&loadFlags);
> 
>     // block top-level data URI navigations if triggered by the web
>     if (!nsContentSecurityManager::AllowTopLevelNavigationToDataURI(aOpenedChannel)) {
>       // logging to console happens within AllowTopLevelNavigationToDataURI
>       aRequest->Cancel(NS_ERROR_DOM_BAD_URI);
>       *aAbortProcess = true;
>+      // close the window since the navigation to a data URI was blocked
>+      if (mDocShell) {
>+        nsCOMPtr<nsIInterfaceRequestor> contentContext =
>+          do_QueryInterface(mDocShell->GetWindow());
>+        if (contentContext) {
>+          mMaybeCloseWindowHelper = new MaybeCloseWindowHelper(contentContext);
>+          mMaybeCloseWindowHelper->SetShouldCloseWindow(true);
>+          mMaybeCloseWindowHelper->MaybeCloseWindow();

Why we need mMaybeCloseWindowHelper member variable on nsDSURIContentListener?
Just having a local RefPtr<MaybeCloseWindowHelper> 
and then calling MaybeCloseWindow() should be enough.
Attachment #8924690 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #4)
> >+          mMaybeCloseWindowHelper = new MaybeCloseWindowHelper(contentContext);
> >+          mMaybeCloseWindowHelper->SetShouldCloseWindow(true);
> >+          mMaybeCloseWindowHelper->MaybeCloseWindow();
> 
> Why we need mMaybeCloseWindowHelper member variable on
> nsDSURIContentListener?
> Just having a local RefPtr<MaybeCloseWindowHelper> 
> and then calling MaybeCloseWindow() should be enough.

Ha, indeed :-)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb29f6c93a3
Refactor MaybeCloseWindow and allow to reuse the window close code from within nsExternalHelperAppService as well as nsDSURIContentListener. r=smaug
https://hg.mozilla.org/mozilla-central/rev/9eb29f6c93a3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.