Closed Bug 210560 Opened 21 years ago Closed 21 years ago

popup windows from form submit are not blocked

Categories

(SeaMonkey :: General, defect)

Product:
SeaMonkey
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.5alpha

People

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

References

(
URL
)

Details

Attachments

(1 file, 1 obsolete file)

check for popup abuse also in the internal Open method
6.32 KB, patch
jst
: review+
brendan
: superreview+
Details | Diff | Splinter Review 
For the record, Ray Trent found this one. This bug is a split from bug 187255.

Popup windows opened using a form submit with a new window as its target aren't
blocked. A simplified version of the test page would be:

<html><head><script>
function loadpopup() {
  document.flauncher.submit();
}
</script></head>
<body onload='loadpopup()'>
The onload handler submits a form with the target _blank
<form name='flauncher' target='_blank' action='about:blank' method='post'>
</form>
</body></html>

Our popup checking code currently checks only the public Open API method. But
the form submission goes through an internal Open method which bypasses the check.
This fixes the problem, but wants testing and analysis. For instance,
nsDocShell::InternalLoad will throw an assertion because it couldn't get a
window to open into. I can't fault it for behaving that way though in this case
there's no real problem. But this patch wants care.
Comment on attachment 126405 [details] [diff] [review]
check for popup window abuse also in the internal Open method

As far as jag and I can tell, this internal Open method is called only by
nsDocShell::InternalLoad. So the side effects of this patch are understood.
Calling for reviews.

This patch looks large but really all it does is factor out the abuse code into
two new methods and add a call to those methods in the internal Open method
used by DocShell.
Attachment #126405 - Flags: superreview?(jst)
Attachment #126405 - Flags: review?(brendan)
Comment on attachment 126405 [details] [diff] [review]
check for popup window abuse also in the internal Open method

Shouldn't this be r? jst and sr? me?  I'll wait till jst has had a look.

/be
Attachment #126405 - Flags: superreview?(jst)
Attachment #126405 - Flags: superreview?(brendan)
Attachment #126405 - Flags: review?(brendan)
Attachment #126405 - Flags: review+
While I'm waiting for review and adjusting the r/sr labels to make it look like
I pay attention to these things, I've taken the opportunity to adjust the API.
Same code as before but packaged slightly differently.
Attachment #126405 - Attachment is obsolete: true
Attachment #126494 - Flags: superreview?(brendan)
Attachment #126494 - Flags: review?(jst)
Oh damnit jst had already reviewed my previous patch. Well, he'll like this
version too. I've just cleaned it up a little bit. Brendan?
Comment on attachment 126494 [details] [diff] [review]
check for popup abuse also in the internal Open method

r=jst, my previous comment still applies.
Attachment #126494 - Flags: review?(jst) → review+
Comment on attachment 126405 [details] [diff] [review]
check for popup window abuse also in the internal Open method

>+PRBool GlobalWindowImpl::OpenAbuseCheck(const nsAString &aName,
>+                                        PRBool *aAbusePoint)
>+{
>+  PRBool allowWindow = PR_TRUE;
>+  *aAbusePoint = CheckForAbusePoint();
>+  
>+  if (*aAbusePoint && IsPopupBlocked(mDocument)) {
>+    allowWindow = PR_FALSE;

Nit, how about an empty line here?

>+    // However it might still not be blocked.
>+    // Special case items that don't actually open new windows.
>+    nsAutoString name(aName);
>+    if (!name.IsEmpty() &&
>+        !name.EqualsIgnoreCase("_top") &&
>+        !name.EqualsIgnoreCase("_self") &&
>+        !name.EqualsIgnoreCase("_content")) {

Why copy into an nsAutoString here?  Use aName.isEmpty() and if Equals("_top",
nsCaseInsensitiveStringComparator), etc.

sr=me with the better string-fu.

/be
Attachment #126405 - Flags: superreview?(brendan) → superreview+
Comment on attachment 126494 [details] [diff] [review]
check for popup abuse also in the internal Open method

Oops, marked the old patch by following a link from old bugmail.

/be
Attachment #126494 - Flags: superreview?(brendan) → superreview+
Patch is in (jst: there was a previous comment? I was expecting one. You've
never not had something to say about a patch of mine before :)). I've negotiated
with Brendan (comment 7) to make window name comparisons in unicode into a
separate bug 210706.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.5alpha
Isn't this the same as bug 144726 which is still open?

I get the misbehavior in 2003071815, but since I have
browser.block.target_new_window enabled it opens in the same window, so I can't
be sure.
The test page in bug 144726 uses the same code, even identical variable names
and similar structure, as the test page in this bug. Obviously they both were
derived from the same original source. I'd say they're duplicates, these two bugs.

However I still say this one's fixed. The 144726 test page opens a popup on me
without my permission in a recent build of the 1.4 branch (which doesn't have
this fix) but doesn't open a popup in a recent build of the trunk (which does
have this fix).
*** Bug 144726 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
Under Tools/Options/Content I check the box to block pop-ups.  AND it does.  BUT the next time I start up FF it is NOT checked and I get tons of pop-ups.  Why doesn't this box stay checked each time I open up FF?
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: