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.

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?

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 :-)!

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; +}

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.

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.

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

Bill, please check this in ASAP!

fixed on branch & trunk

adding branch resolution keyword.

Verified checked into lxr.mozilla.org and bonsai.mozilla.org