Closed Bug 51355 Opened 24 years ago Closed 23 years ago

moz escapes javascript: urls containing non-ascii chars

Categories

(Core :: DOM: HTML Parser, defect, P2)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.2

People

(Reporter: mikko.rantalainen, Assigned: nhottanscp)

References

Details

(Keywords: highrisk, intl, Whiteboard: [PDT+]wait for tree open to check in)

Attachments

(11 files)

Attaching the reporter's HTML test case here -
220 bytes, text/html
Details
nbsp html entity in url
229 bytes, text/html
Details
extended testcase
1.13 KB, text/html
Details
Patch, use UTF-8 if the URI shcheme is 'javascript', changed files where ASCII is assumened for URI to UTF-8.
4.68 KB, patch
Details | Diff | Splinter Review
Patch, updated with jst's comment (diff -uw).
2.27 KB, patch
Details | Diff | Splinter Review
Patch, updated with jst's comment (diff -uw), ignore the last patch, I put a wrong one.
4.50 KB, patch
Details | Diff | Splinter Review
Patch, updated with the 2nd jst's comment (diff -uw).
4.55 KB, patch
Details | Diff | Splinter Review
Patch, removed "pos != -1" check.
4.52 KB, patch
Details | Diff | Splinter Review
Patch, localized change to only modify nsHTMLUtils.cpp.
1.65 KB, patch
Details | Diff | Splinter Review
Patch, updated with ftang's suggestion.
1.64 KB, patch
Details | Diff | Splinter Review
Patch, updated with the suggestions (diff -uw)
1.69 KB, patch
Details | Diff | Splinter Review
Any "special" character in href causes javascript to fail because browser escapes all "special" characters. Following code works in Netscape Communicator 4.73 but two last fail under mozilla (build ID:2000090421 - quite probably others builds too). <html> <a href='javascript:alert("Hello World!")'>Hello World!</a><br> <a href='javascript:alert("Hello Wörld!")'>Hello Wörld!</a><br> <a href='javascript:alert("Hello W&#246;rld!")'>Hello W&#246;rld!</a><br> </html> As seen in the status bar mozilla translates second and third href as 'javascript:alert(%22Hello%20W%F6rld!%22)' which doesn't work. I'm not sure whether the second href should work or not but IMHO third one should because I have escaped &ouml; character. (Escaping " as %22 doesn't make any difference either). IMO correct solution would be to use string between " or ' characters in whole with no other translation but *unescaping* before scripting.
Even NN4.73, if you type this into the URL bar: javascript:alert("Hello W&#246;rld!"); it doesn't work. There seems to be browser code that escapes the characters if they are part of HTML. Therefore I'm sending this bug over to the Parser component for further triage, as it doesn't seem to be a JS Engine issue -
Assignee: rogerl → rickg
Status: UNCONFIRMED → NEW
Component: Javascript Engine → Parser
Ever confirmed: true
QA Contact: pschwartau → janc
I don't believe we plan to support javascript in attributes. Vidur?
marking wont fix, since I don't think we're supporting js attributes.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → WONTFIX
javascript: in a href is not the same as JavaScript entities. We're not supporting the latter. The former should and does work. The entities and Unicode characters in the attributes in the examples shown should be correctly preserved.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Vidur's right, my mistake. Note that the parsing engine is passing the href's along unchanged. I suspect the culprit is either the link handling code or the sink. I'll dig a bit more.
Status: REOPENED → ASSIGNED
I've now confirmed that the javascript attributes are being parsed and stored as written. I now suspect that the anchor handling code is converting these before showing them.
Reassigning to trudele for triage. I think the content model is well formed, and that the problem is in the link handling code.
Assignee: rickg → trudelle
Status: ASSIGNED → NEW
Keywords: nsbeta3
Priority: P3 → P2
Thanks to billlaw, I've tracked this down to nsIOService::Escape(). The valid nsString is being converted to a whacky C-String (hence the %20's). Please take a look.
Assignee: trudelle → warren
Note: this may also be the solution to bug 40469 (-?)
The problem specifically is the use of Escape here: http://lxr.mozilla.org/seamonkey/source/layout/html/content/src/nsHTMLUtils.cpp# 150 We are converting all questionable characters in the string to their escaped values. Probably what we should do is construct the appropriate kind of url for the protocol in question (here, javascript:), and let the protocol's url parser deal with the escaping (if necessary). For javascript: we'll want to escape the things inside the quotes (I think), but not the entire url string. For "standard URLs" (most of the other protocols) we'll escape according to their rules.
Not working on any platform: 2000-10-05-09-MN6 : Windows 2000-10-05-13-MN6 : Mac 2000-10-05-09-MN6 : Linux Changing platform/os to all/all Nominating for rtm
Keywords: rtm
OS: Linux → All
Hardware: PC → All
Warren, is there a trivial fix for this? This bug has languished almost a month in the rtm nomination state. If we don't need this for rtm, please mark it rtm-.
Whiteboard: [need info]
rtm-, no activity. please update the bug if you're actually working on it.
Whiteboard: [need info] → [rtm-]
Even a classic non breakable space html entity gives same wrong behavior. Try this (attached next) : <html><head><title>non breakable space</title></head><body> <a href='javascript:alert("Hello&nbsp;World!")'>Hello&nbsp;World!</a><br> or<br> <a href="javascript:alert('Hello&nbsp;World!')">Hello&nbsp;World!</a> </body></html>
updated qa contact.
QA Contact: janc → bsharma
*** Bug 75542 has been marked as a duplicate of this bug. ***
The escapes are part of the URL syntax, so it should be just as valid if the page contained the escaped characters. Isn't the problem really that something needs to unescape them?
Reassigning to component's default owner. Warren, hope you don't mind.
Assignee: warren → harishd
The problem is either in the JS engine or in layout. Reassigning bug to waterson. ccing attinasi & rogerl
Assignee: harishd → waterson
*** Bug 77043 has been marked as a duplicate of this bug. ***
*** Bug 60576 has been marked as a duplicate of this bug. ***
Attached file extended testcase —
*** Bug 55468 has been marked as a duplicate of this bug. ***
*** Bug 63314 has been marked as a duplicate of this bug. ***
*** Bug 66055 has been marked as a duplicate of this bug. ***
cc jst@netscape.com,ftang@netscape.com since I'm copying their comments from duplicate bugs. (pschwartau@netscape.com and jag@tty.nl are already cc'ed here.) From bug 55468: ------- Additional Comments From Johnny Stenback 2000-11-21 02:01 ------- Mozilla's javascript: URL handling is a mess, we escape the non-ASCII characters in their original charset but we never unescape them - also, the escaped URL has no charset info in it so even if we did unescape the JS URL before executing it there's no way to know what charset the URL is encoded as... oh my From bug 66055: ------- Additional Comments From Frank Tang 2001-01-30 11:32 ------- We probably don't care if this only a issue in alert. But I think the alert here is just use as a simplified test case. The real problem is these JavaScript is inside the HREF. We escape them since other URL need to be escaped but we didn't consider the case for JavaScript. What we should do is find out the code between the layout engine and the javaScript code and unescape it back by using the document charset. I think this bug is in the border line of fixing or not fixing. Let's keep this as future untill we find breakage in the real world case. From bug 63314: ------- Additional Comments From Peter ``jag'' Annema 2000-12-19 12:29 ------- Actually, this should be escaped (it's supposed to be an URI[0]), so I guess it should be unescaped before handing it to the JS engine. I strongly suspect this to be a dupe of an existing bug. [0] http://www.w3.org/TR/html4/struct/links.html#adef-href ------- Additional Comments From Phil Schwartau 2001-04-26 11:06 ------- This WW3 link may be of interest: http://www.w3.org/TR/html4/appendix/notes.html#non-ascii-chars
Keywords: 4xp, intl
Summary: "javascript:code" doesn't work in hrefs properly → moz escapes javascript: urls containing non-ascii chars
cc jst@netscape.com,ftang@netscape.com since I just copied their comments from duplicate bugs.
My attached testcase doesn't cover: - %hh in javascript: URLs that contain non-ascii characters - %hh in javascript: URLs that do not contain non-ascii characters - unescaping with the correct charset (?) (from bug 55468)
Keywords: mozilla1.0
*** Bug 69043 has been marked as a duplicate of this bug. ***
Keywords: mostfreq
*** Bug 79602 has been marked as a duplicate of this bug. ***
How did I get this bug?
Assignee: waterson → harishd
The problem seems to be around line: 150 in nsHTMLUtils.cpp ( what rickg had pointed out ). http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLUtils.cpp #150 Back to waterson :-)
Assignee: harishd → waterson
This should probably go to ftang.
Assignee: waterson → ftang
nhotta- can you help this one?
Assignee: ftang → nhotta
I tried additional cases. <a href='javascript:alert("Hello W\u00F6rld!")'>Hello W&#246;rld!</a><br> this works. <a href='javascript:alert("Hello W%F6rld!")'>Hello W&#246;rld!</a><br> this works but does not unescape "%F6". Not sure which spec the string should follow, URI, HTML, JS or all? Is there a spec for the case, href=javascript? Only \uxxxx seems to be working now.
nhotta: try this <a href="Hello W&#246;rld">Hello W&#246;rld</a>
I tried that (it was in the original report) and it did not work. I assume "&#246;" is decoded to unicode and if that is escaped to "%F6", it still should works even if that is not unescaped (as my second example). I am going to look at the actual value in nsHTMLUtils.cpp. But I would like to know what is really allowed in that string (%xx, \uxxxx, &#xxx;).
I looked at the value, that was "%22Hello%20W%F6rld!%22" as mentioned in the original report. So it has to be unescaped before passed to JavaScript. The problem is when unescape and convert to unicode, you don't know about the charset. There is a proposal of adding a hint_charset field to nsIURI. It also proposes to use UTF-8, so the current conversion and escape code may not be needed, you just need to set the hint_charset and call NS_ConvertUCS2toUTF8. It was posted on 5/11/2001 to n.p.m.netlib, cc to people from that post. Is there a bug for that proposal?
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
Filed bug 84032 for adding "hint_charset" to nsIURI.
Asked jst and rpotts for reviews.
Should charset conversion apply to all new protocols by default? CF bug 81717 (converting \ to / in links should happen for only http,https,ftp rather than everything except javascript).
I had a look at the attached patch and there's a bunch of issues with this patch: In nsGenericHTMLElement.cpp: nsAutoString href; - href.AssignWithConversion(hrefCStr); + href = NS_ConvertUTF8toUCS2(hrefCStr); If you'd make this: - nsAutoString href; - href.AssignWithConversion(hrefCStr); + NS_ConvertUTF8toUCS2 href(hrefCStr); you'd save a string copy, no? The changes in nsHTMLUtils.cpp seem like major overkill to me, we convert a unicode string into a utf8 cstring (and this conversion is very likely to end up doing an malloc since URL's are often longer than what fits in nsAutoString's internal buffer) and then create a new URI object and parse the whole URI string only to find out if the URI string starts with "javascript:"? Wouldn't simple string manipulation be more efficient (and less code) here? Oh, is the attached patch a cvs diff -w? If not you better fix the indentation in nsHTMLUtils.cpp: + if (bIsJavascript) { + spec = utf8Spec; + } + else { // Otherwise, we'll need to use aDocument to cough up a character // set converter, and re-encode the relative portion of the URL as ... ^ |____ indent! nsInputStreamChannel.cpp, can this get more inefficient? :-) nsString name; - name.AppendWithConversion(urlStr); + name.Append(NS_ConvertUTF8toUCS2(urlStr)); *result = name.ToNewUnicode(); How about: - nsString name; - name.AppendWithConversion(urlStr); - *result = name.ToNewUnicode(); + *result = ToNewUnicode(NS_ConvertUTF8toUCS2(urlStr)); in nsSimpleURI::SetSpec(): nsAutoString spec; - spec.AssignWithConversion(aSpec); + spec = NS_ConvertUTF8toUCS2(aSpec); Any reason to not make 'spec' be of type NS_ConvertUTF8toUCS2 in stead of being of type nsAutoString? nsJSProtocolHandler.cpp: nsAutoString scriptString; - scriptString.AssignWithConversion(script); + scriptString.Assign(NS_ConvertUTF8toUCS2(script)); rv = scriptContext->EvaluateString(scriptString, Why not: - nsAutoString scriptString; - scriptString.AssignWithConversion(script); + NS_ConvertUTF8toUCS2 scriptString(script); rv = scriptContext->EvaluateString(scriptString, Fix those issues and I'll have one more look...
Jesse, I am not clear about your question. Charset conversion does not convert \ to /, that unrelated.
Much better! One more thing, in nsHTMLUtils.cpp: + PRInt32 pos = aSpec.Find(":"); + nsAutoString scheme; + if ((pos != -1) && + (aSpec.Left(scheme, pos) != -1) && + scheme.EqualsIgnoreCase("javascript")) { + spec.Assign(NS_ConvertUCS2toUTF8(aSpec.GetUnicode())); + } This code will end up doing aSpec.Left() and scheme.EqualsIgnoreCase() for every URL when we could limit the cases to where the ':' is found at the right place, i.e. if ((pos == nsCRT::strlen("javascript")) && ...), or something like that. Oh, and use aSpec.FindChar(':') when looking for a single character. Make those changes and you'll have sr=jst
That part only executed if the URI contains non ASCII characters. Anyway, faster is better, I will update the patch.
I don't see a reason for the pos != -1 check here, if you're worried about the performance hit from nsCRT::strlen("javascript") in the cases where a ':' isn't found in the URI then you could make the check be simply: if (pos == 10 /* strlen("javascript") */ && ... which would be even faster and even less code, and you could actually optimize this even further by never searching for the ':' character, in stead you could simply check if the 10th character is a ':' (after making sure the string is longer than 10 characters) and then do the check if the data before the ':' was "javascript" or something else, but since this is not performance critical or anything, I'm ok with this as is, except for the "pos != -1" check. sr=jst if you remove that check...
sr on the nsSimpleURI and nsInputStreamChannel string changes. I wonder if there are other places in necko that we should be doing this?.
Under mozilla/netwerk, there are 59 of ToNewCstring, 43 of AssignWithConversion, 11 of NS_ConvertASCIItoUCS2 Changing them to ToNewUTF8String, NS_ConvertUTF8toUCS2 would not break ASCII (UTF-8 is a superset of ASCII), though I am not sure about performance difference.
remove the N6 time [rtm-] since it cause confusion.
Whiteboard: [rtm-]
Blocks: 83989
The current patch makes it so javascript: is the only protocol where the URL isn't subject to charset conversion. Is this what we want to do, or do we want to list the protocols that *do* get converted? (I'm bringing this up because in bug 81717, another URL conversion, converting \ to /, is being applied to everything except javascript: and causing data: URLs to break.)
>I wonder if there are other places in necko that we should be doing this?. Let's sperate that into different bug. Let's deal with known issue for now and unknown issue later.
Whiteboard: r/sr=jst+dougt, need a=
Jesse, I think it's fine to add other protocols if they also want UTF-8 string. If there is a specific example, please file a separate bug. Note that protocol handlers may also need change in order to accept UTF-8 URI.
Testing information: I ran http://www.mozilla.org/quality/precheckin-tests.html which include http and ftp. Tested links in 4.01 spec http://www.w3.org/TR/html4/. Also tried non ASCII cases http://www.fcenter.ru/wn/hn02082000.htm to make sure non javascript case is not affected.
Filed separately as bug 85310 about the remaining ToNewCString in necko.
pdt+ base on 6/11 pdt meeting.
Whiteboard: r/sr=jst+dougt, need a= → [PDT+]r/sr=jst+dougt, need a=
I review the 06/08/01 10:02 patch, I feel the change is right but have high risk except the nsHTMLUtils.cpp part. I afraid this may cause some unknown regression since some changs is in the low leverl code. In this late cycle, I suggest we hold this patch and ask naoki to work on a work around changes which only in nsHTMLUtils.cpp. In the case of JavaScript protocol, instead of convert from Unicode to UTF8, we could convert to the javascript escape \u for non ASCII data. In this way, we can gunarantee it only impact JavaScript protocol but not other code. We should definitely consider the rest of the 06/08/01 10:02 patch after we branch off from moz.0.9.3. It is safer to test that kind of change in the early development cycle.
Adding highrisk and patch keywords based on ftang's comments.
Keywords: highrisk, patch
Frank reviewed the patch. His comments, * use nsCRT::IsAscii * check ECMA script spec for \uxxxx format
From the ECMA script spec, http://www.ecma.ch/ecma1/stand/ecma-262.htm > UnicodeEscapeSequence :: See 7.6 > \u HexDigit HexDigit HexDigit HexDigit So it looks like it expects four digits although I didn't find a place which says it had to be four digits. I found that the current mozilla does not accept non four digits like \uF6, it has to be \u00F6.
r=ftang
I would've put the \u as part of the format string to PR_snprintf() (and made sure there's room for that too in buf) in stead of first appending \u to the string and then doing the printf and then appending the result of printf to the string, that would've been less code, but both ways work. sr=jst
+ nsAutoString scheme; + if ((pos == (PRInt32) nsCRT::strlen("javascript")) && + (aSpec.Left(scheme, pos) != -1) && + scheme.EqualsIgnoreCase("javascript")) { FYI, here's a way to consolidate the string constant and avoid the strlen of that constant, without wiring in 10 (although 10 is well-known to be the length of "javascript"!): + static const char kJavaScript[] = "javascript"; + nsAutoString scheme; + if ((pos == (PRInt32)(sizeof kJavaScript - 1)) && + (aSpec.Left(scheme, pos) != -1) && + scheme.EqualsIgnoreCase(kJavaScript")) { I agree with what jst said. Instead of: + char buf[5]; + spec.Truncate(0); + for (const PRUnichar* uch = aSpec.GetUnicode(); *uch; ++uch) { + if (!nsCRT::IsAscii(*uch)) { + spec.Append("\\u"); + PR_snprintf(buf, sizeof(buf), "%.4x", *uch); + spec.Append(buf); Why not + char buf[6+1]; // space for \uXXXX plus a NUL at the end + spec.Truncate(0); + for (const PRUnichar* uch = aSpec.GetUnicode(); *uch; ++uch) { + if (!nsCRT::IsAscii(*uch)) { + PR_snprintf(buf, sizeof(buf), "\\u%.4x", *uch); + spec.Append(buf); /be
Whiteboard: [PDT+]r/sr=jst+dougt, need a= → [PDT+]need sr=
*** Bug 86092 has been marked as a duplicate of this bug. ***
sr=jst
Whiteboard: [PDT+]need sr= → [PDT+]need a=
a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)
Whiteboard: [PDT+]need a= → [PDT+]wait for tree open to check in
checked in
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
I checked now. It work! Thanx! from Japan
verified win2k 2001061905 macos 2001061908 linux 2001061914
Status: RESOLVED → VERIFIED
*** Bug 88220 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.

Attachment

General

Creator:
Created:
Updated:
Size: