javascript: URL cant submit because of %20 instead of SPACE

VERIFIED FIXED in mozilla1.0

Status

()

Core
Networking
P1
normal
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: Henrik Gemal, Assigned: Andreas Otte)

Tracking

({helpwanted})

Trunk
mozilla1.0
x86
Windows 2000
helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

17 years ago
If you go to:
http://film.jubii.dk/rh/
and in the "Nyhedsbrev" input field to the left enter a mailadress and presses
"Tilmeld"
nothing happens! IE Submits just fine.
The offending line is:
HREF="javascript:document.forms['tilmeld'].mail_status.value='frameld';%20document.forms['tilmeld'].submit();"
which has %20 instead of a space.


anything we can do?
(Reporter)

Comment 1

17 years ago
Created attachment 52938 [details]
simple test case
Um.... sounds like just a syntax error in the JS to me.  We can evangelize. :)

Comment 3

17 years ago
Not sure of correct component to decide this; but not JS Engine.
Reassigning to Parser for consideration - is it this component
which decides what to do with %20 ? 
Assignee: rogerl → harishd
Component: Javascript Engine → Parser
QA Contact: pschwartau → moied

Comment 4

17 years ago
Phil: This has nothing to do with the parser. Giving bug to layout.
Assignee: harishd → attinasi
Component: Parser → Layout
QA Contact: moied → petersen

Updated

17 years ago
Target Milestone: --- → mozilla1.0.1
(Reporter)

Updated

17 years ago
Depends on: 94902

Comment 5

17 years ago
Moving Mozilla 1.01 bugs to 'future' milestone with priority P1

I will be pulling bugs from 'future' milestones when scheduling later work.
Priority: -- → P1
Target Milestone: mozilla1.0.1 → Future
*** Bug 130035 has been marked as a duplicate of this bug. ***
Nominating for nsbeta1, mozilla1.0.

javascript:foo is a URI and we should follow RFC 2396 here.  It appears that
MSIE already follows the RFC.

This IMO is critical for 1.0 so we don't "create" a new standard that developers
expect to continue.

For reference, the RFC can be found here: http://www.faqs.org/rfcs/rfc2396.html
Keywords: mozilla1.0, nsbeta1

Comment 8

16 years ago
->javascript. The test case gives the JavaScript error:
javascript:document.forms['tilmeld'].mail_status.value='frameld';%20document.for
ms['tilmeld'].submit(); line 1: syntax error
Assignee: attinasi → rogerl
Component: Layout → JavaScript Engine
QA Contact: petersen → pschwartau
Target Milestone: Future → ---

Comment 9

16 years ago
Again: this is not JS Engine. What component parses javascript: URLs
before they are passed to JS Engine? Will try Networking component -

The question is, what should we do if a javascript: URL has a %20 instead
of a space? In particular, see Comment #7 From Christopher -
Assignee: rogerl → new-network-bugs
Component: JavaScript Engine → Networking
QA Contact: pschwartau → benc

Comment 10

16 years ago
+helpwanted

As I understand it, each URL scheme is responsible for parsing their URI.

Bringing some URL authority...
Keywords: helpwanted
(Assignee)

Comment 11

16 years ago
The %20 is absolutly correct in the url, every space has to be escaped this way.
Are you sure the %20 is the cause of this? I'm no expert in form submits, but I
remember some talk about a IE special version of form submit that does not work
with mozilla.

In any case, I think everything in the javascript url after the scheme (the
path) has to be unescaped before given to the JS engine.

This should be done in mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp. Can't find
it there. Darin?
Andreas, yes since we don't unescape it, we parse the % as a modulo operator of
javascript.  Since |%20| is invalid (being a binary operator), we throw an error
and script execution halts.
(Assignee)

Comment 13

16 years ago
Hmmm ... okay, this is settled then. We have to do the unescaping, I can only
hope the modulo operater gets well escaped in javascript urls ... otherwise we
will have another batch of syntax errors ...
Well, I don't think that we will see anything negative from fixing this bug.  It
appears from the reporter of this bug and 130035 that MSIE already escapes
javascript: URIs properly.  So if we fix this, anything that would break us
would also break MSIE.
(Assignee)

Comment 15

16 years ago
Created attachment 74596 [details] [diff] [review]
patch to unescape the script

With this patch the script gets unescaped before getting evaluated. Evaluation
still returns an error ...

Comment 16

16 years ago
i think you want:

NS_UnescapeURL(script);

instead.  otherwise, script.Length() might be incorrect.

Comment 17

16 years ago
andreas: do you want to own this bug?
(Assignee)

Comment 18

16 years ago
Created attachment 74768 [details] [diff] [review]
patch with darins fix

... and suddenly the submissions seems to work, thanks darin!

All others: please try the fix ...
Attachment #74596 - Attachment is obsolete: true
(Assignee)

Comment 19

16 years ago
taking ...
Target Milestone: --- → mozilla1.0
Patch works great.  :)
Assignee: new-network-bugs → andreas.otte

Comment 21

16 years ago
Comment on attachment 74768 [details] [diff] [review]
patch with darins fix

sr=darin
Attachment #74768 - Flags: superreview+
(Assignee)

Comment 22

16 years ago
Anyone want to give an r=?

Comment 23

16 years ago
+making test:

If this is purely a URL syntax question, I can add it to the URL parsing tests
we do, but if there are larger javascipt aspects of this, I'd like to have the
javascript people handle that.
Summary: cant submit because of %20 instead of SPACE → javascript: URLcant submit because of %20 instead of SPACE

Comment 24

16 years ago
It is purely a URL parsing problem. If the JS Engine gets a valid
program, it runs it correctly; nothing here indicates otherwise -
(Assignee)

Comment 25

16 years ago
jst, this is a networking problem under dom. Want to give r=?
Comment on attachment 74768 [details] [diff] [review]
patch with darins fix

r=jst
Attachment #74768 - Flags: review+

Comment 27

16 years ago
Comment on attachment 74768 [details] [diff] [review]
patch with darins fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74768 - Flags: approval+
(Assignee)

Comment 28

16 years ago
fix checked in 
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 29

16 years ago
*** Bug 107524 has been marked as a duplicate of this bug. ***
FYI, this fix was not quite right, it introduced bug 144429, and it doesn't do
the right thing with non ASCII characters.
(Assignee)

Comment 31

16 years ago
It seems we have to be smarter with unescaping in this case, don't unescape
stuff that is part of a string in javascript, leave it as it is.

Comment 32

16 years ago
Errr, surely it's quite simple: when handing a javascript: URL, just URL-
unescape the part after the scheme (all of it), and pass the result to the 
script engine.

This means that multiple levels of encodings are often used: here's a 
convoluted example.

<a href="javascript:alert(&#37;27O&#37;5cx4b&#37;27)">Click me</a>

The "href" attribute is subject to HTML encoding, so we HTML-unencode it and 
get:
    javascript:alert(%27O%5cx4b%27)

That's a javascript: URL, so we URL-unescape the rest and get:
    alert('O\x4b')

The string constant argument to "alert" is, as a Javascript string, subject to 
its own escape sequences, so we process those and get:
    OK

So clicking on the original <a> tag we started with should alert: OK

AFAICT there's no special issue with non-ASCII characters; just make sure that 
at each level of encapsulation you do the right level of encoding:

- always properly build your Javascript source, including escape sequences in 
strings
- always properly URL-encode that javascript if you're then packing it into a 
javascript: URL
- always properly encode any URL if you're creating an HTML attribute, 
e.g. "href".

Comment 33

16 years ago
Do we want to mark this fixed and then get a new bug for the second-layer of
escaping problems?
Summary: javascript: URLcant submit because of %20 instead of SPACE → javascript: URL cant submit because of %20 instead of SPACE

Comment 34

16 years ago
Re Comment #32: 

There may be a W3C standard for this; I don't know. But I can't believe
users currently follow, or would follow, the suggestion in Comment #32:

> - always properly URL-encode that javascript if you're then packing it
> into a javascript: URL

I would argue against that point. I should think the javascript:
protocol means what it says: that everything under its scope is
a JavaScript program, and only JavaScript escaping should appear.

Thus, in my view, the example in Comment #32 should be parsed from
the inside out, not the outside in. Again, without knowing the
standard, I just can't imagine users writing javascript: URLs while 
taking three different escapings into account.

Comment 35

16 years ago
No-one said it would be easy ;-)

I think there can only be two sensible options:

1. the part after "javascript:" is NOT url-encoded (at all).  Pros: simple; 
compatible with Opera 6.  Cons: limits what can be included (?); not compatible 
with IE.

2. the part after "javascript:" IS url-encoded (completely).  Pros/cons: the 
opposite of the above.

From what other people have said, there is no "standard" as to which approach 
to take - but there is precedent.  I would argue there's a strong motive to do 
the same thing as IE here.

As for complexity - well if you want to work in an integrated environment (of 
sorts) - where HTML can contain URLs, and those URLs can contain Javascript - 
then I'm afraid the multiple encodings issue just goes with the territory.

Comment 36

16 years ago
Well, I think #2 is the only option. The RFC's pretty much say that you encode.
(Assignee)

Comment 37

16 years ago
Yes, RFC 2396 pretty much defines the standard for all uris. That includes the
allowed characters and the special meaning of %. The only option I see too is to
encode it properly, for example if you want to send an already encoded string
then you have to double encode it to get the right result after unescaping.

Comment 38

16 years ago
VERIFIED: mozilla 1.1b, netscape 7.0.

I've got a brief testcase that has some escaped characters and it seems to work
correctly.
Status: RESOLVED → VERIFIED

Comment 39

16 years ago
*** Bug 144429 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.