Closed Bug 136188 Opened 21 years ago Closed 21 years ago

Fix WWW_OpenURL and add WWW_GetWindowInfo DDE commands

Categories

(SeaMonkey :: UI Design, enhancement)

Product:
SeaMonkey
Component:
UI Design
Platform:
x86
All
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: support, Assigned: law)

References

Details

Attachments

(1 file, 4 obsolete files)

OK, merge of the dueling patches, ready for review
14.42 KB, patch
roc
: review+
roc
: superreview+
jesup
: approval+
Details | Diff | Splinter Review 
From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
BuildID:    20020408

The WWW_OpenUrl command does not look at the third argument of the DDE data.  
The third argument is the window id the URL should be opened in and is used to 
determine if a new window should be created to open the URL or not.  Currently 
Mozilla always defaults to using the current window to open URLs through DDE.

The WWW_GetWindowInfo DDE command is not supported.  This command just passes 
the current URL and Window title for the top-most browser window.  It is needed 
for third party plug-ins, such as bookmark managers, to determine the location 
of the browser.

Reproducible: Always
Steps to Reproduce:
1.Send a WWW_OpenURL DDE command to the browser with the new window argument 
set to "0".  This should cause the browser to open the link in a new window.
2.Send a WWW_GetWindowInfo command to the browser
3.

Actual Results:  For the WWW_OpenURL command - the URL is Opened in the current 
window.

For the WWW_GetWindowInfo command nothing happens.

Expected Results:  WWW_OpenURL - Mozilla should have started a new window and 
opened the link in that window.

WWW_GetWindowInfo - Mozilla should have returned the current URL and Page Title 
per DDE spec
Confirmed; drivers want this work.  John, please attach the diffs for your fix
to this bug; preferred in -u format, but in any format is better than none.  See
"Create a New Attachment" link above.

Reassigning to Bill Law.
Assignee: sgehani → law
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: mozilla1.0
Target Milestone: --- → mozilla1.0
This code sample correctly parses the DDD WWW_OpenURL command looking at the
third argument to determine whether a new window should be used or not.  The
prior version always defaulted to the current window.  This does not match the
spec at:
http://developer.netscape.com/docs/manuals/communicator/DDE/ddevb.htm#www_openurl


This patch also includes support for WWW_GetWindowInfo DDE command.
I can be your super-reviewer.

I know you didn't start the "hope there aren't commas in quotes" thing, but
sometimes there are commas in URLs. Here, for example:
http://www.heraldsun.news.com.au
Would it be too much to ask to fix this in the argument parser while we're here,
assuming such URLs hork your product? Or are the URLs quoted so this isn't an
issue? (I don't have Windows so I can't test.)

Similarly, could you, should you be quoting the strings you send back from
GetWindowInfo? It looks like horrible things could happen if the page URL or
title contains a double quote character. This is disturbing since this could be
under the control of people we don't trust.

I think your psuedo-loop is wrong. The cleanest way would be to break that
code out into a helper function with many returns. Otherwise you can just remove
the loop and the breaks will drop you out of the switch statement. As it is, if
there's a failure the breaks will drop you out of the loop and you'll fall
through to the "activate" case ... ouch.
The first point, fixing the parser to handle a comma in a URL, should be 
addressed.  Right now, ParseDDEArg is only called from 3 locations:
1) topicOpenUrl needs it to read the URL string.
2) topicOpenUrl needs it to determine if a new window should be used to open 
the URL.
3) topicActivate needs it to determine is the user is trying to active a new or 
current window.

Item number 1 is the only data retrieved with the function that is surrounded 
in quotes.  How about adding another parameter (BOOL) to the ParseDDEArg 
function to indicate if the argument we are searching for is surrounded by 
quotes?  That could close the loophole that you bring up while not radically 
changing the parsing algorithm.  


As for the return data of WWW_GetWindowInfo, each data item should be 
surrounded in quotes.  This is how other browsers DDE intefaces work along with 
the old Netscape DDE interface.  It should be in the format "URL","Title",""  

If a URL and title both contained a comma, concievably it could be impossible 
to parse.  However, I do not believe a quote can appear in a URL.  The burden 
of parsing this information correctly falls on the application.  Parsing of the 
data should be done in this manner, for example if the sample return was:
"http://www.kaylon.com","Tools for Navigating Information",""
^^                    ^^^^                               ^^^^
12                    3456                               789A

- URL: this is from the 2nd character (2) until the next quote (3) in the string
- Title: this string is from end of the URL/Title separating characters (6) 
until until the end of the overall string minus the ","" (789A) found at the 
end. Therefore, if commas, or anything else for that matter, appear between (6) 
and (7) in the title, the application will get the data as expected.

I agree that this will be available to people we do not trust, but wouldn’t the 
page title already have potentially "bad" character combinations parsed out 
since it is also displayed across the title bar of the window?

A break statement at the end of the topicGetWindowInfo is missing.  I will 
include this when I post a new patch pending your agreement/suggestion about 
how to fix the parser when quotes appear in the URL.


John

double-quotes are illegal in URI's.  Commas may appear in URI's depending on the
exact usage.  However, if URI's are enclosed in quotes, as you said, it's not an
issue.

Titles CAN have quotes in them I believe by use of """.  If the encoding is
maintained, or the title is run through an encoder, then you're ok (so long as
the things processing the title know to run it through an un-encoder).

> How about adding another parameter (BOOL) to the ParseDDEArg 
> function to indicate if the argument we are searching for is surrounded by
> quotes?

When you extract the window argument (argument #3), it's argument #1 that you
have to worry about containing commas. So I don't think this parameter is
exactly what you need.

I think it would be better to fix ParseDDEArg so that it scans forward for the
Nth parameter, where a parameter is either an unquoted string not containing a
comma, or a quoted string not containing a quote, returning an unquoted string
in either case.

> double-quotes are illegal in URI's

Does this mean they should not appear in URIs, or Mozilla *guarantees* they will
never appear in the URI here? There's a huge difference. I think we should be
paranoid and scan through the URI for rogue double quotes; if we find one, we
can abort.

> Title: this string is from end of the URL/Title separating characters (6) 
> until until the end of the overall string minus the ","" (789A) found at the 
> end. Therefore, if commas, or anything else for that matter, appear between
> (6) and (7) in the title, the application will get the data as expected.

It violates the principle of least surprise to send back title strings
containing embedded double quotes and expect the client to strip off the ",""
from the end of the string to get the correct title. Does DDE not specify a way
to escape double quotes in a double-quoted string? If it does, we should use
that. If it doesn't, we should sanitize the title by, for example, replacing all
double quotes in the title with single quotes.

> A break statement at the end of the topicGetWindowInfo is missing.

Can we just get rid of the do loop? I find it a bit ugly, and I've never seen
this trick used anywhere else in the code. Having the breaks drop directly out
of the switch statement is fine.
First the Do loop will be removed.  I had cut and pasted this from another 
location in this module, I just figured that is how things were done. 

>When you extract the window argument (argument #3), it's argument #1 that you
>have to worry about containing commas. So I don't think this parameter is
>exactly what you need.

>I think it would be better to fix ParseDDEArg so that it scans forward for the
>Nth parameter, where a parameter is either an unquoted string not containing a
>comma, or a quoted string not containing a quote, returning an unquoted string
>in either case.

Since the scope of the ParseDDEArg function is only to a few calls, I was 
thinking about customizing it by passing in an enumerated value instead of the 
index.  The enum would be: { OpenUrl_URL, OpenUrl_NewWindow, Activate_Window }

Then in the parse function, it would know that OpenURL_URL is a double quoted 
string that is the first item in the string being passed.  The 
OpenUrl_NewWindow item is the 3rd item in a string and is not contained in 
quotes, but it comes after a double quoted string that was the first argument.  
Activete_Window would know to look at the first parameter and it would not be 
in double quotes. 

The upside to this is that the ParseDDEArgs function would be correct, the 
downside is that if more DDE commands are added, the ParseDDEArgs function has 
to change accordingly.  What do you think?

>Does this mean they should not appear in URIs, or Mozilla *guarantees* they 
>will never appear in the URI here? There's a huge difference. I think we 
>should be paranoid and scan through the URI for rogue double quotes; if we 
>find one, we can abort.

They should not appear for the item to be a valid URL, but you can get one to 
be returned as the current URL of the browser.  For example, typing in 
http://www.yahoo.com/"  will take you to a Yahoo Page that tells you the page 
is not found.  From the browser's perspective, this appears as valid.

The correct thing to do is the escape each quote out of the string, so it would 
appear as "http://www.yahoo.com/\"" in a WWW_OpenURL command.   For the 
GetWindowInfo command it would be returned in the same manner.  This is how 
other DDE interfaces work in this scenario.

>Does DDE not specify a way to escape double quotes in a double-quoted string? 
>If it does, we should use that. If it doesn't, we should sanitize the title 
>by, for example, replacing all double quotes in the title with single quotes.

Yes it does, they should be escaped with a slash character '\'.  

Depending on whether you are in agreement that the ParseDDEArgs function should 
be customized passing in an enum value rather than a generic index, I will 
implement this. 



> I had cut and pasted this from another location in this module, I just
> figured that is how things were done.

It is already there? Hmm. Well, it shouldn't be :-).

> What do you think?

Sounds hacky. I'd prefer that you just write the general function as I
described. It shouldn't be much code and it will probably be easier to understand.

Thanks!
>> I had cut and pasted this from another location in this module, I just
>> figured that is how things were done.

>It is already there? Hmm. Well, it shouldn't be :-).

Did you look at the code?
Attached patch alternative patch (obsolete) — — Splinter Review 
I reviewed the original patch and came up with this modified one.

- fixed a fair number of stylistic issues (e.g., using nsCAutoString, naming
variables, moving declarations closer to initializations, etc.),

- inserted a break; statement at the end of the new topicGetWindowInfo: case
(this was the only actual bug I found),

- rearranted the code for the new call to DdeCreateDataHandle so that it shares
code with the other calls to that API,

- simplified the rewrite of ParseDDEData (instead of nested if/for loop, I just
use a while loop with no new variables),

- modifie some comments to make them more accurate.

The problem with commas in WWW_OpenURL urls is bug 84277.  That bug was
nowhere's near being on anybody's urgent mozilla1.0 list prior to now.
This patch includes both of the prior proposed attachement suggestions.  

In addition, the return data for WWW_GetWindowInfo is being parsed and escape
characters '\' are being inserted in front of quotes that appear within the
page title or url.

The ParseDDEArgs function has also been modified to correctly parse items that
are surrounded by quotes.  We used to search from comma to comma for an
arguemnt. This was a problem if an agument that was surrounded in quotes
contained a comma (i.e. the URL data in WWW_OpenURL command).

Now, when we start processing an argument, the new routine checks to see if the
argument starts with a quote.  If it does begin with a quote, we search until
the next quote that doesn't have an escape character in front of it.  Once we
find that other quote, we know where our string begins and ends and it can
contain a comma.

If there is no quote, we just look for the next comma as usual.  

The routine will also correctly parse items other than the first argument. 
That was a problem that is present in the current code.  Only the first
argument can be read.

John
+                    // Add escape Characters in front of any quotes in the URL
+                    PRInt32 offset = -1;
+
+                    while( 1 ) {
+                        offset = url.FindChar( '"', ++offset );
+                        if ( offset == kNotFound ) {
+                            // No more quotes, exit.
+                            break;
+                        }
+                        else {
+                            url.Insert( PRUnichar('\\'), offset );
+                            //Increment offset because we just inserted a slash
+                            offset++;
+                        }
+                    }

This bit of code should be factored out into a static utility function and 
called from the two places it occurs.

Also, your ParseDDEArgs rewrite is more complicated than it needs to be, 
intuitively, at least.  The only "enhancement" that would need to be made to 
the simpler version (in my patch) is to change the advancement to the next 
comma: 
   +            offset = temp.FindChar( ',', ++offset );

Instead, we need to examine the character at ++offset and if it's a '"' then 
advance to the next non-escaped '"' before looking for the ','.  I think that 
can be done with a simple loop right there without overhauling the entire 
function.

I'll try to code that up and post back here in a few minutes.

Oh, one other thing.  One of the "break;" statements isn't indented quite far 
enough (got to make sure we fix those :-)!
Attached patch alternative patch, v2 (obsolete) — — Splinter Review 
This patch fixes ParseDDEArg the way I suggested.  It turns out that I need the
"advance to end of quoted string" logic two places (also when advancing to the
end of the requested arg) so I pulled that out into a separate function:

+// Utility function to advance to end of quoted string.
+// p+offset must point to the comma preceding the arg on entry.
+// On return, p+result points to the closing '"' (or end of the string
+// if the closing '"' is missing) if the arg is quoted.  If the arg
+// is not quoted, then p+result will point to the first character
+// of the arg.
+static PRInt32 advanceToEndOfQuotedArg( const char *p, PRInt32 offset, PRInt32

len ) {
+    // Check whether the current arg is quoted.
+    if ( p[++offset] == '"' ) {
+	 // Advance past the closing quote.
+	 while ( offset < len && p[++offset] != '"' ) {
+	     // If the current character is a backslash, then the
+	     // next character can't be a *real* '"', so skip it.
+	     if ( p[offset] == '\\' ) {
+		 offset++;
+	     }
+	 }
+    }
+    return offset;
+}
Attachment #78246 - Attachment is obsolete: true
Attachment #78506 - Attachment is obsolete: true
Bill,

Is there any chance you can add this code to your patch?  It is the code to sub 
in escape characters in front quotes that could occur in a page title?  This is 
just to make sure the response string from WWW_GetWindowInfo is correct.

Otherwise, the suggested patch works for me.

Thanks,

John

+                    PRInt32 offset = -1;
+
+                    while( 1 ) {
+                        offset = title.FindChar( '"', ++offset );
+                        if ( offset == kNotFound ) {
+                            // No more quotes, exit.
+                            break;
+                        }
+                        else {
+                            title.Insert( PRUnichar('\\'), offset );
+                            //Increment offset because we just inserted a slash
+                            offset++;
+                        }
+                    }

You suckered me into it, John.	I put your code into an escapeQuotes function.

So now it's my patch; can you review it, then?	I presume you'll run your suite
of tests.
Attachment #78767 - Attachment is obsolete: true
Attachment #78785 - Attachment is obsolete: true
Bill,

Thanks for adding the parse routine.  I have tried the patch and it works for 
our test suite. Here are the main points that our test covers:

- Opening a URL
- Opening a URL in a new window
- Opening a URL containing a comma
- Opening a URL containing a comma in a new window
- Getting page information (URL, Title) with no commas or quotes
- Getting page info for a URL with a comma
- Getting page info for a Title containing a quote and having it escaped 
propertly
- Getting page title for a Title that contains a quote and comman and having 
the data returned escaped. 

I think all the work is done on this bug.  From my review, I think the code is 
ready.

This fix will also close out bug 84277.

What are the steps to getting this reviewed and checked in? 

Thanks,

John
I just wanted to follow up, I did not recieve confirmation that an email was 
sent out to everyone on the review board. 

I think the proposed patch fixes the bug.  What do we have to do to get it 
reviewed/checked in?

Thanks,

John
Looks great but I have two more comments --- sorry :-)

1) I think escapeQuotedString should also escape any backslashes found in the
string. Otherwise the DDE client might treat them as escape characters and
unescape them which would be wrong and potentially dangerous. E.g. if the title
is blahblahblah\nono, we should return "blahblah\\nono"

2) The lossy UCS2 conversion should be applied BEFORE escapeQuotedString.
Otherwise high UCS2 characters could be downmapped to backslashes and
doublequotes which would mess up our quoting again.
>1) I think escapeQuotedString should also escape any backslashes found in the
>string. Otherwise the DDE client might treat them as escape characters and
>unescape them which would be wrong and potentially dangerous. E.g. if the title
>is blahblahblah\nono, we should return "blahblah\\nono"

I disagree on the point above.  Escaping a quote character is not specified in 
the DDE interface.  It only says that the quote should be escaped.  As far as 
other browsers:

If a sample page title contains a '\' character Opera, IE, and classic Netscape 
do not escape the character.  Perhaps it might be a good idea, but I think 
maintaining a uniform DDE interface is important.  Especially from an 
application standpoint a consistent interface is best. 


So then the titles
foo\"bar
and
foo"bar

will both be quoted to foo\"bar and then unquoted to foo"bar ... well, if
everyone is broken this way, I guess we have to be too.
Robert:

re: 2) converting before escaping

nsAString::FindChar takes a PRUnichar argument, so '"' is promoted to PRUnichar 
and will only match legitimate PRUnichar double-quotes.  Or at least that's 
what jag just told me.

re: escaping backslashes (your most recent comment)

I believe that we'll convert the title foo\"bar to foo\\"bar, and foo"bar to 
foo\"bar, no?

There *is* a problem with the titles that end in backslashes, though.  E.g., 
foo\ comes out (fully quoted) 
as "http://www.foobar.com/whatever.html","foo\","".  That's a bit of a 
challenge for clients to parse, I suppose, but not impossible.

John, do you guys handle that OK?

P.S. John, to expedite things, click on the Edit link next to the attachment 
and check "reviewed" and type r=support@kaylong.com in the comment box.  That 
patch needs its reviewed, super-reviewed, and approved checkboxes checked (the 
latter for the branch).

Once you and Robert put the reviewed/super-reviewed checks in place, I'll email 
the powers-that-be to get approval.  Then I'll check it in.  Nothing to it.
>There *is* a problem with the titles that end in backslashes, though.  E.g., 
>foo\ comes out (fully quoted) 
>as "http://www.foobar.com/whatever.html","foo\","".  That's a bit of a 
>challenge for clients to parse, I suppose, but not impossible.

>John, do you guys handle that OK?

Bill, 

Our code handles that correctly, because if we fail to parse the string 
correctly starting from the beginning and working till the end, we will start 
over from the end and work our way backwards to try and recover the data.

In the even that we can't extract the title correctly, we let the user know and 
they can enter it in manually.

Quite honestly, in one of our testing databases of over a million bookmarks, I 
could not find a page title that would cause this problem. It could present a 
problem, but the fix is to modify the DDE standard, then our code patch.

I think application programmers should be aware of this potential shortcoming 
in the spec and code accordingly.

John


I am trying to follow Bill's directions for review:

>P.S. John, to expedite things, click on the Edit link next to the attachment 
>and check "reviewed" and type r=support@kaylong.com in the comment box.  That 
>patch needs its reviewed, super-reviewed, and approved checkboxes checked (the 
>latter for the branch).

However, when I try to submit my review I get a message that says "You are not 
authorized to edit attachments."

Any ideas how I can get past this? 

Thanks,
John
Comment on attachment 78815 [details] [diff] [review]
OK, merge of the dueling patches, ready for review

OK, I'm happy. sr=roc+moz

I'll also attach John's review.

Now you just need to email drivers explaining why this needs to be in 1.0.
Include a link to the bug and the patch.
Attachment #78815 - Flags: superreview+
Attachment #78815 - Flags: review+
Have I done the right things to get approval on this bug?  I sent an email to 
drivers@mozilla.com and never heard anything.

What can I do to get approval so this can be incorporated?

Thanks,

John
I don't see any mail from you to drivers since April 8 when you first discussed
this patch.

Send another email to drivers@mozilla.org. Let them know that you now have
review and super-review (and who you got it from) and that you now need approval
for trunk and also the 1.0 branch. Include the bugzilla URL and patch URL.
Mention why you think this should be in 1.0 (you could just include the previous
email you sent).
Comment on attachment 78815 [details] [diff] [review]
OK, merge of the dueling patches, ready for review

a=rjesup@wgate.com for branch checkin post-RC1
Attachment #78815 - Flags: approval+
Blocks: 31343
QA Contact: paw → tpreston
Bill, please check this in ASAP!
fixed on branch & trunk
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
adding branch resolution keyword.
Keywords: fixed1.0.0
Verified checked into lxr.mozilla.org and bonsai.mozilla.org
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.0 → verified1.0.0
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.