174628 - using the window.open(<uri>,''), '&' character gets replaced by '%26 ' string

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Rudolf Krist]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2b) Gecko/20021001 Phoenix/0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1) Gecko/20020826

I use a javascript function to open a new window. The method
window.open(<uri>,'') replaces all special characters like one of {ä,ü,ö,ß...} 
by their %<hex-value> form, in the <uri> String. But all following '&', '=' are 
replaced too. Because of that I cannot use the uri-request-arguments to 
transport data from one page to another one.

example:
tr_id=151&mem_id=17572&mem_name=crew|&reason=Sprühen&val=13&wind_type=modify
gets
tr_id=151&mem_id=17572&mem_name=crew|&reason=Spr%FChen%26val%3D13%26wind_type
%3Dmodify

The Javascript function I used:
function modTA(tr_id, memb_id, memb_name, reason, val) {
    var mf = window.open("clanfinance/taWindow.php?tr_id="+tr_id+"&mem_id="
            +memb_id+"&mem_name="+memb_name+"&reason="+reason+"&val="+val
            +"&wind_type=modify", 
            "", "height=220,width=265,resizeable=0,status=0");
}

The same happens when I use the Phoenix Browser

Reproducible: Always

Steps to Reproduce:
1. go to the url i described above
2. navigate: -> ClanKasse -> Clankasse
3. select a row, that contains one of the characters {ö, ü, ä} like that with 
     TrID=64
4. -> ändern

... -> use the 'Zurück' button to close the window
Actual Results:  
the "Zweck" edit field contains the uri-request-arguments which follow the
argument, which contains the data, that should be displayed in the edit field.


Expected Results:  
Mozilla should only display the data, which the argument &reason contains.

Notices:
You should allow cookies to access that web page
You should not modify too much on that web page

Comment 1

[Phil Schwartau]

Browser, not engine ---> Form Submission for further triage.

Rudolf: I get a "404 - Not Found" error when I try to go to the URL
you gave above: http://thecrew.ngz-server.de/rudi/clanfinanzen_test_site/

Is there a typo?

Comment 2

[Phil Schwartau]

Not Found
The requested URL /rudi/clanfinanzen_test_site/ was not found on this server.

Apache/1.3.26 Server at thecrew.ngz-server.de Port 80

Comment 3

[Rudolf Krist]

sorry, i modified the url

Comment 4

[Alexandru Savulov]

I see what the problem is, but is not form submission, is the window.open method
that is doing the escaping magic in the URI string. The &'s in front of the
first occurence of an german umlaut character do not get escaped. This is not
form submission afaict, however the form submission code does use the nsEscape
function to escape occurences of chars not in the US-ASCII set. If that is the
problem, it should be repaired there. I'm not familiar to the javascript part
though. I will try to find out to whom to reassign this one unless Phil knows. 

Comment 5

[Phil Schwartau]

Attachment: Reduced HTML testcase

Comment 6

[Phil Schwartau]

Alex is right; I misassigned this.

The reduced testcase calls window.open() on this non-existent URL: 

              "AAA=crew&BBB=Sprühen&CCC=modify"

Mozilla raises an alert saying this URL can't be found. In the alert,
we can see the bug:

              The 'ü' is escaped to '%FC'        <--- GOOD

But then things go wrong after this (Moz 20021014xx WinNT):

              The next '&' is escaped to '%26'   <--- BAD!
              The next '=' is escaped to '%3D'   <--- BAD!
 

Reassigning to DOM Level 0, although perhaps they are relying on
other code in order to form this escaped URL...

Comment 7

[Phil Schwartau]

UPDATE: both the testcase and the original URL above WORKFORME
using trunk builds from today, 2002-10-15, on WinNT and Linux.

Rudolf: could you try the latest build and see if the bug is fixed?
If so, you can resolve this bug as WORKSFORME. Thanks -

Comment 8

[Phil Schwartau]

CORRECTION: the bug is NOT fixed.


I. The reduced testcase: 

a) When run off the Bugzilla server, we do not see the bug.
   No alert comes up, and the new window opens with this message:

  "The requested URL /AAA=crew&BBB=Sprühen&CCC=modify was not found on this    
  server."

   Since no escaping is done, we can't see the bug.


b) But when saved as a local file, however, we get the bug I described
   in Comment #6 above. We do see this bug in the 2002-10-15 builds, too.



II. The given URL:   
    http://thecrew.ngz-server.de/rudi/clanfinanzen_test_site_moz_error/

    I misunderstood what to look for in the "Zweck" textbox.
    If you click the "ändern" link in the "Sprühen" row, the "Zweck"
    textbox in the new window should contain only "Sprühen", as in IE6.

    Mozilla is showing "Sprühen&val=100&wind_type=modify" instead.
    We see this bug in the 2002-10-15 builds, too.

Comment 9

[Rudolf Krist]

I can reproduce the bug also on a linux system with Netscape 7.0,
Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.0.1) Gecko/20020823 Netscape/7.0.

Comment 10

[Alexandru Savulov]

Attachment: testcase that demo'es the diff between GET submission and window.open

there is a difference between the way the url is encoded in the GET form
submission and in window.open(). see the
nsFSURLEncoded::URLEncode(nsFormSubmission.cpp) method where we use nsEscape
only on the buffers that contain the names or the values but not the = and &
signs. i do think bug 174615 is a dup of this one

Comment 11

[Alexandru Savulov]

sorry i attached a wrong testcase, will attach a corrected one

Comment 12

[Alexandru Savulov]

Attachment: testcase that demo'es the diff between GET submission and window.open

the correct testcase

Comment 13

[Vladimir Ermakov]

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

Comment 14

[Phil Schwartau]

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

Comment 15

[Phil Schwartau]

From the duplicate bug 176417:

------- Additional Comment_ #8 From [e-mail] 2002-10-24 11:30 ------- 

... The string, with the '&' intact, gets through the JS engine just fine
and finally gets hosed by 'nsEscapeCount' in xpcom/io/nsEscape.cpp -
called by GlobalWindowImpl::Escape.

Comment 16

[Katsuhiko Momoi]

Marking this topembed. 
Due to this bug, #1 chat site in China is not working.
Cannot find any existing chat rooms as URL to a room include
"&gender=male/female" following a Chinese character.

Comment 17

[Jaime Rodriguez, Jr.]

Marking this as topembed+/nsbeta1+ as this is effecting the top site in China.

Comment 18

[Rudolf Krist]

the project under the previous url 
http://thecrew.ngz-server.de/...
became status productive, so i created the same site at another provider, to 
demonstrate the bug.
the new url is
http://mitglied.lycos.de/meph6666/clanfinanzen_test_site/

Comment 19

[Kai Ensenbach]

I wanted to ask when the target milestone will be released, because I cannot
find anything about that release on your actual roadmap.

Comment 20

[Phil Schwartau]

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

Comment 21

[Andreas Lange]

Another version of this bug appears for example with
window.open("pageö.html?id=5") which will try to open "page%F6.html%3Fid%3D5".
Neither the '?' nor the '=' should be encoded. I don't think the encoding of '?'
has been mentioned previously? This breaks at least one rather major Swedish
website.

Comment 22

[Nisheeth Ranjan]

Taking...

Comment 23

[Nisheeth Ranjan]

Attachment: First try at fix

This patch adds a new bit, url_QueryChars, to nsEscapeMask.  nsGlobalWindow ORs
this bit into the mask parameter when it calls Escape().  If this bit is set,
the three characters '?', '&', and '=' are not escaped in the url string.

I've tested the whichever test cases are live on this bug and its duplicates. 
They all work.

I'm concerned, though, that we might still be missing some cases.  Darin, can
you think of other characters that we shouldn't escape?  How about the
following example:

window.open("http://savuÄlov.com:8080/return.php?name1=value1");

Should we or should we not escape the ':' in the url's host name?  With the
current patch, the ':' is escaped.  Will that confuse Necko?

Comment 24

[Nisheeth Ranjan]

Comment on attachment 111812 [details] [diff] [review]
First try at fix

Heikki, would you please review this?  Its a small, one page patch for the
topembed+ bug that I snagged from jst last week.

Johnny, if you could super-review it, that would be great!

Comment 25

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The patch is about URL handling and I don't feel comfortable reviewing this. I
think this should be reviewed by darin, andreas or dougt, or all of them.

The first thought that came to me about this was that we should probably do that
special escaping only for the query part of the URL, not the whole URL. But I
just don't know this code well enough to know what works and what might cause
regressions.

Comment 26

[Andreas Otte]

I can't believe that old nsEscape stuff is still in use ... can someone please
point me to the code that uses it. 

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 111812 [details] [diff] [review]
First try at fix

- In nsEscape.cpp:

     {	  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,	/* 0x */
		 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,	/* 1x */
-		 0,0,0,0,0,0,0,0,0,0,7,4,0,7,7,4,	/* 2x	!"#$%&'()*+,-./
 */
-	  7,7,7,7,7,7,7,7,7,7,0,0,0,0,0,0,	/* 3x  0123456789:;<=>?  */
+		 0,0,0,0,0,0,8,0,0,0,7,4,0,7,7,4,	/* 2x	!"#$%&'()*+,-./
 */
+	  7,7,7,7,7,7,7,7,7,7,0,0,0,8,0,8,	/* 3x  0123456789:;<=>?  */
	     0,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,	/* 4x  @ABCDEFGHIJKLMNO  */

Clean up the indentation of this code while you're at it, I smell evil tabs!
:-) Same thing in nsEscape.h.

sr=jst

Comment 28

[Andreas Otte]

Okay, found it in nsGlobalWindow. Why does this escape happen at all? I think it
is not necessary, by doing so you destroy the url syntax and then you get what
you ask for. Let the escaping be done by the code that actually creates the url.
I don't think the problem should be fixed with the attached patch.

Comment 29

[Andreas Otte]

If you want to escape here then you should possibly use NS_EscapeURL with the
esc_OnlyNonASCII mask, but then why escape at all?

Comment 30

[Darin Fisher]

Comment on attachment 111812 [details] [diff] [review]
First try at fix

>Index: nsEscape.cpp

>     {    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,	/* 0x */
> 		 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,	/* 1x */
>+		 0,0,0,0,0,0,8,0,0,0,7,4,0,7,7,4,	/* 2x   !"#$%&'()*+,-./	 */
>+         7,7,7,7,7,7,7,7,7,7,0,0,0,8,0,8,	/* 3x  0123456789:;<=>?	 */
> 	     0,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,	/* 4x  @ABCDEFGHIJKLMNO  */

there's some definite indentation problems here.  can you please clean up
the indentation along with this patch?	thx!



>Index: nsEscape.h
> ,	url_Path		= PR_BIT(2)
>+, url_QueryChars = PR_BIT(3)

again, indentation probs.

r=darin

Comment 31

[Darin Fisher]

Comment on attachment 111812 [details] [diff] [review]
First try at fix

revoking my r= until andreas's comments are addressed (i jumped the gun). 
generally, the rule is to let necko do the escaping.  otherwise, we lose out on
knowledge of non-ASCII chars.  if at possible try to leverage the escaping
built into nsStandardURL.

Comment 32

[Nisheeth Ranjan]

Attachment: Patch that incorporates comments from jst, andreas, and darin

I've broken out the string encoding code from Escape() into a new protected
method called Convert() in nsGlobalWindow.  Escape() now calls Convert() and
then escapes the string using the old nsEscape() call, like so:

  nsEscape(dest.get(), nsEscapeMask(url_XAlphas | url_XPAlphas | url_Path));

OpenInternal(), however, calls Convert() and then escapes the url using
NS_EscapeURL(), like so:

  NS_EscapeURL(dest, esc_AlwaysCopy | esc_OnlyNonASCII, rightCStr);

I've also cleaned up the code in OpenInternal() that was doing a bunch of
unnecessary string copies.

Comment 33

[Nisheeth Ranjan]

Comment on attachment 112354 [details] [diff] [review]
Patch that incorporates comments from jst, andreas, and darin

Darin and Johnny,

Would you please r and sr this patch?  It fixes url escaping problems in
nsGlobalWindow::Open() which is a topembed+ bug right now.

Thanks!

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 112354 [details] [diff] [review]
Patch that incorporates comments from jst, andreas, and darin

+GlobalWindowImpl::Convert(const nsAString& aStr,
+			   char** aDest)

Maybe this would be better named doCharsetConversion() or somesuch?

- In GlobalWindowImpl::Escape():

+  if (NS_SUCCEEDED(result = Convert(aStr, getter_Copies(dest)))) {

Please pull this call to Convert() out before the if, and then just check
NS_SUCCEEDED(result) in the if (and rename 'result' to 'rv' while you're at
it).

- In GlobalWindowImpl::OpenInternal():

+    // if the URL contains non ASCII, escape all non ASCII characters starting
+    // from the first non ASCII char.

I wonder if this is really worth it. We save passing some characters to the
charset converter, but is it worth the additional code size and added
complexity? Couldn't this just pass the whole URL to Convert() and let it deal
with the fact that some of the first characters don't need converting?

I'm fine with this either way, but I just don't see much of a benefit of doing
what we do here.

+      if (NS_SUCCEEDED(Convert(Substring(aUrl, nonAsciiPos, len -
nonAsciiPos), 
+	 getter_Copies(dest)))) {

Indent the getter_Copies() so that it lines up with the other argument to
Convert().

sr=jst

Comment 35

[Nisheeth Ranjan]

Attachment: Patch that addresses jst's comments

This patch includes all the changes suggested by jst.  His sr can carry forward
to this patch.	Will ask Darin for a review so that another pair of eyes see
this final (hopefully!	:-) ) patch.

Comment 36

[Nisheeth Ranjan]

Comment on attachment 112466 [details] [diff] [review]
Patch that addresses jst's comments

Darin, here's the patch I just AIM'd you about.  Please take a look.  

The patch is described in comment 32 and includes the changes suggestd by jst
in comment 34.

Thanks!

Comment 37

[Darin Fisher]

Comment on attachment 112466 [details] [diff] [review]
Patch that addresses jst's comments

r=darin (there's a ton of string copies in this code!)

Comment 38

[Johnny Stenback (:jst)]

Comment on attachment 112466 [details] [diff] [review]
Patch that addresses jst's comments

sr=jst

Comment 39

[Nisheeth Ranjan]

Comment on attachment 112466 [details] [diff] [review]
Patch that addresses jst's comments

This is a topembed+ bug that breaks China's top chat site among others.  The
problem was that window.open() was escaping ASCII characters like '&', '=', and
'?' in the url.  Now, it only escapes non-ASCII characters.

darin and andreas have reviewed and jst has super-reviewed.

I've tested the old test case in bug 35076 and all the test cases on this bug
and its duplicates (the ones that were still accessible).  I've also completed
pre-checkin tests.

I'm not sure how important topembed+ bugs are to the 1.3 beta release.	If they
 are, please let me check in immediately before the 1.3 beta branch is created.

If you want to get wider testing for this internally, please grant me approval
to check in on the trunk after the 1.3 beta branch is created.

As far as I know, Netscape project management is happy if this patch makes
either 1.3 beta or 1.3 final.

Thanks!

Comment 40

[Asa Dotzler [:asa]]

Comment on attachment 112466 [details] [diff] [review]
Patch that addresses jst's comments

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 41

[Nisheeth Ranjan]

Fix checked into trunk...