Closed
Bug 211599
Opened 22 years ago
Closed 22 years ago
Can't open link with javascript and linefeed in the href
Categories
(Core :: Layout, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.5alpha
People
(Reporter: wolruf, Assigned: bzbarsky)
References
()
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
10.22 KB,
patch
|
dougt
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•22 years ago
|
||
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?
![]() |
Assignee | |
Comment 2•22 years ago
|
||
In any case, taking.
Assignee: other → bzbarsky
Keywords: regression
Priority: -- → P1
Target Milestone: --- → mozilla1.5alpha
Updated•22 years ago
|
OS: Windows 2000 → All
Comment 3•22 years ago
|
||
bz: hmm... yeah, nsSimpleURI::SetSpec should probably do the same filtering as
nsStandardURL::SetSpec.
![]() |
Assignee | |
Comment 4•22 years ago
|
||
Please ignore the nsHTMLUtils stuff -- that's for another bug, and I will make
sure to separate it out before checking in.
![]() |
Assignee | |
Updated•22 years ago
|
Attachment #127188 -
Flags: superreview?(darin)
Attachment #127188 -
Flags: review?(dougt)
Comment 5•22 years ago
|
||
andreas, can you look at this?
Comment 6•22 years ago
|
||
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?
Comment 7•22 years ago
|
||
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.
Comment 8•22 years ago
|
||
We should not filter simple urls by default, remember data urls for example,
would have ugly results ...
![]() |
Assignee | |
Comment 9•22 years ago
|
||
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.
Comment 10•22 years ago
|
||
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 11•22 years ago
|
||
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-
![]() |
Assignee | |
Comment 12•22 years ago
|
||
Attachment #127188 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•22 years ago
|
Attachment #127188 -
Attachment is obsolete: false
Attachment #127188 -
Flags: review?(dougt)
![]() |
Assignee | |
Updated•22 years ago
|
Attachment #127200 -
Flags: superreview?(darin)
Attachment #127200 -
Flags: review?(andreas.otte)
Comment 13•22 years ago
|
||
Okay, agreed, on rereading rfc 2397 it is now clear to me that "unsafe"
characters will be encoded, either hex or base64.
Comment 14•22 years ago
|
||
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 15•22 years ago
|
||
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-
![]() |
Assignee | |
Comment 16•22 years ago
|
||
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
![]() |
Assignee | |
Updated•22 years ago
|
Attachment #127200 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•22 years ago
|
Attachment #127206 -
Flags: superreview?(darin)
Attachment #127206 -
Flags: review?(andreas.otte)
Comment 17•22 years ago
|
||
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+
![]() |
Assignee | |
Updated•22 years ago
|
Attachment #127206 -
Flags: review?(andreas.otte) → review?(dougt)
Comment 18•22 years ago
|
||
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+
![]() |
Assignee | |
Comment 19•22 years ago
|
||
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: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•