Closed Bug 211599 Opened 21 years ago Closed 21 years ago

Can't open link with javascript and linefeed in the href

Categories

(Core :: Layout, defect, P1)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.5alpha

People

(Reporter: wolruf, Assigned: bzbarsky)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

build ID: 2003070107 (trunk) on Win2k & Linux.

Steps to reproduce:
1. Load URL,
2. Click on 'help' in right column ("if you need tip on searching"),
3. Window doesn't popup.

JavaScript console shows:
Error: unterminated string literal
Source File:
javascript:sitewide_toolkit_window('http://www.cisco.com/ciscodotcom/toolkit/searchbasic.html
');
Line: 1, Column: 24
Source Code:
sitewide_toolkit_window('http://www.cisco.com/ciscodotcom/toolkit/searchbasic.html
[...]

the relevant source is:
[...]
 </span><a
href="javascript:sitewide_toolkit_window('http://www.cisco.com/ciscodotcom/toolkit/searchbasic.html
');" class="modulecaptionlink">Help
[...]

There's a carriage return in the <a href , which is allowed according to RFC2396
(section 1.6, "whitespaces should be ignored"), but here the "\n" is in a
javascript statement.

IE6 and NS4.79 behave different from Mozilla: the window pops up ok.

Apologies if this is a dupe, I had a headache trying to understand if my
particular case (JS within CDATA in Quirks mode) applies to bug 47048 or bug 42287.
If the linefeed is converted in whitespace, it'll fail anyway. The real fix on
the server would be to suppress the linefeed but as NS4.79 behaves different, I
thought I'd file a bug report to double check.
Darin, the issue here is that SetSpec on an nsSimpleURI does not strip out
newline chars.  Should it?  Or should I go back to stripping out \r and \n in
the DOM when constructing the href URI for an anchor?
In any case, taking.
Assignee: other → bzbarsky
Keywords: regression
Priority: -- → P1
Target Milestone: --- → mozilla1.5alpha
OS: Windows 2000 → All
bz: hmm... yeah, nsSimpleURI::SetSpec should probably do the same filtering as
nsStandardURL::SetSpec.
Attached patch Proposed patch (obsolete) — Splinter Review
Please ignore the nsHTMLUtils stuff -- that's for another bug, and I will make
sure to separate it out before checking in.
Attachment #127188 - Flags: superreview?(darin)
Attachment #127188 - Flags: review?(dougt)
andreas, can you look at this?
Comment on attachment 127188 [details] [diff] [review]
Proposed patch

can you move the helper function into nsURLHelper to avoid code duplication? 
maybe add a flag for the embedded \t issue?  actually, why don't you want to
strip out embedded tabs?
more to the point: why should nsStandardURL and nsSimpleURI differ at all in how
they filter unacceptable characters?  it seems to me that we should try to
utilize the same filtering code for consistency.
We should not filter simple urls by default, remember data urls for example,
would have ugly results ...
data: urls and the like are why I didn't want to filter tabs... but that said,
whitespace in data: urls should be encoded, no?

I'm fine with filtering tabs out of the simple URI, fwiw.
andreas: do you really think stripping "\r\n\t" from a data: URL is likely to
cause problems?  who creates data: URLs with intentional tabs?
Comment on attachment 127188 [details] [diff] [review]
Proposed patch

creating a "net_FilterURIString" function is definitely the way to go.	maybe
we just want a flag to control filtering within the URI or whatever, but i
definitely think it'd be good to not duplicate code here.
Attachment #127188 - Flags: superreview?(darin) → superreview-
Attachment #127188 - Attachment is obsolete: false
Attachment #127188 - Flags: review?(dougt)
Attachment #127200 - Flags: superreview?(darin)
Attachment #127200 - Flags: review?(andreas.otte)
Okay, agreed, on rereading rfc 2397 it is now clear to me that "unsafe"
characters will be encoded, either hex or base64.   
Comment on attachment 127200 [details] [diff] [review]
True.  On second thought, I have no problem with stripping \t from all urls

>Index: layout/html/base/src/nsObjectFrame.cpp

>-  return NS_NewURI(aFullURI, aSrc, originCharset.get(),
>-                   aBaseURI, nsHTMLUtils::IOService);
>+  return NS_NewURI(aFullURI, aSrc, originCharset.get(), aBaseURI);

hmm... are you sure you want to lose the cached IO service?  will
this be costly?


>Index: netwerk/base/src/nsSimpleURI.cpp

> nsSimpleURI::SetSpec(const nsACString &aSpec)
> {
>     if (aSpec.IsEmpty()) {
>         mScheme.Truncate();
>         mPath.Truncate();
>         return NS_OK;
>     }
> 
>+    const nsAFlatCString& flat = PromiseFlatCString(aSpec);
>+    const char* specPtr = flat.get();
>+
>+    NS_ASSERTION(specPtr && *specPtr, "IsEmpty() lied");

do we really need this assertion?  IsEmpty isn't going to lie ;-)
also, what if we filter out \n\r\t and then have an empty string?
maybe the IsEmpty check should follow (as well as preceed?)
filtering.


>+    // filter out unexpected chars "\r\n\t" if necessary
>+    nsCAutoString filteredSpec;
>+    PRBool filtered = net_FilterURIString(specPtr, filteredSpec);
>+
>     // nsSimpleURI currently restricts the charset to US-ASCII
>+    nsCAutoString spec;
>+    if (filtered) {
>+        NS_EscapeURL(filteredSpec, esc_OnlyNonASCII|esc_AlwaysCopy, spec);
>+    } else {
>+        NS_EscapeURL(flat, esc_OnlyNonASCII|esc_AlwaysCopy, spec);
>+    }

hmm... (with my codesighs hat on:) can you do it with only one call
to NS_EscapeURL?  something like this might be nice:

  nsCAutoString spec, filteredSpec;
  PRUint32 specLen;  
  if (net_FilterURIString(specPtr, filteredSpec)) {
      specPtr = filteredSpec.get();    // inlines to member var access
      specLen = filteredSpec.Length(); // inlines to member var access
  } else
      specLen = flat.Length();
  NS_EscapeURL(specPtr, specLen, esc_OnlyNonASCII|esc_AlwaysCopy, spec);


>Index: netwerk/base/src/nsURLHelper.h

>+/**
>+ * Filter out whitespace from a URI string.  The input is the |str|
>+ * pointer. |result| is written to if and only if there is whitespace that has
>+ * to be filtered out.  The return value is true if and only if |result| is
>+ * written to.

maybe document what is filtered.
Attachment #127200 - Flags: superreview?(darin) → superreview-
Comment on attachment 127200 [details] [diff] [review]
True.  On second thought, I have no problem with stripping \t from all urls

I think there is a second instance of FilterString usage in
nsStandardUrl::Resolve, was unable to compile.

The NS_PRECONDITION in nsURLHelper.cpp net_FilterURIString is not terminated,
also does not compile.
Attachment #127200 - Flags: review?(andreas.otte) → review-
I thought I'd compiled, but clearly I had not...

> are you sure you want to lose the cached IO service?

That's the "separate bug" part.... The part of that hunk that belongs to this
bug is the removal of whitespace-stripping.

> maybe the IsEmpty check should follow (as well as preceed?)

I did it both before and after in this version of the patch.

Addressed the other comments.
Attachment #127188 - Attachment is obsolete: true
Attachment #127200 - Attachment is obsolete: true
Attachment #127206 - Flags: superreview?(darin)
Attachment #127206 - Flags: review?(andreas.otte)
Comment on attachment 127206 [details] [diff] [review]
Fix all the comments.

i'm going to nit-pick once more (sorry!)... it seems that if we assume specPtr
can never be null valued (i think that is a safe assumption), then we could
just have one IsEmpty check just before calling NS_EscapeURL.

sr=darin with or without that change.
Attachment #127206 - Flags: superreview?(darin) → superreview+
Attachment #127206 - Flags: review?(andreas.otte) → review?(dougt)
Comment on attachment 127206 [details] [diff] [review]
Fix all the comments.

do we have to worry about net_FilterURIString accepting a string that contains
a null before the ws?

> for (; *p; ++p) {

style nit: please make this a while loop.

fix the nit and comment about the safety filterURIString.  r=dougt
Attachment #127206 - Flags: review?(dougt) → review+
The string being passed in is UTF-8, so it will not contain embedded nulls.

Made the other changes, and checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: