Closed Bug 92721 Opened 24 years ago Closed 15 years ago

[quirks]FORM does not work if ACTION parameter is mis-quoted

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
Future

People

(Reporter: yancey, Unassigned)

References

(
URL
)

Details

(Keywords: compat, testcase)

Attachments

(2 files)

Testcase showing # of attrs when 1 is malformed
235 bytes, text/html
Details
Proposed Patch
2.38 KB, patch
Details | Diff | Splinter Review
Reproducible: Always I'll set this as PC:Win2K, but I know it exhibits the same behavior under Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:0.9.2+) Gecko/20010728. Probably should be All:All, but I haven't tried it on other systems yet. Technically, this is bad HTML. However, it works under both IE and Communicator. Here is the offending tag... <form method=post action=http://cgi.cascss.unt.edu:8000/cgi-bin/untdirectory"> This is a large email directory, so any name like smith should return results.
After further investigation, I realized that Mozilla's view source is not showing the full text of the original HTML. Here is the line in the bad tag in the real source... <FORM method=post "action=http://cgi.cascss.unt.edu:8000/cgi-bin/untdirectory"> As you can see, Mozilla does not show the quote in front of 'action'. When you submit the form, mozilla attempts to load something and will wait forever for a page that will never load. I do not know what URL it sends.
I was finally able to get a look at the web server logs. Mozilla is sending the closing quotation mark to the webserver as part of the script name. The error logs report that they cannot find the untdirectory" script.
over to parser
Assignee: rods → harishd
Status: UNCONFIRMED → NEW
Component: Form Submission → Parser
Ever confirmed: true
QA Contact: vladimire → bsharma
Discussed this in IRC with Yancey, and we've found the problem (because the quote is at the beginning of the name-value pair, the string after the equals sign is sent to ConsumeAttributeValueText rather than ConsumeQuotedText). Apparently this works for both IE and NN; do we want to add it to Quirks mode?
Seems to be a duplicate of BUG 88243
This is not a dupe of bug 88243.
Status: NEW → ASSIGNED
The issue here seems to be that CAttributeToken::Consume does not expect a random quoted string to be inside the start tag. As an example, if you had this... <body "title='demo page'"> From my short review of the code, it looks like the CAttributeToken::Consume function looks for text to put into mTextKey, but stops at the first quote instead of discovering the actual key name. It then just skips ahead until it finds the equal sign, completely dropping the intended key name, possibly leaving the name of the key as a null string! It should probably call ConsumeQuotedString in this case when it finds the first quote, but calls ConsmeAttributeValueText instead. It might decide to ignore the quoted string, but should it at least realize that it is indeed a quoted string?I intended to dump out the token for debugging, but ran out of time while attempting to figure out how to do that. If you can show me how to dump the token to the console, I will be able to verify my suspicions.. and possibly provide a patch.
I tried the following example and it works fine: <html> <body "bgcolor='red'" text="yellow"> <p> should be yellow on a red background </p> </body> </html>
Removing the single quotes from that testcase results in yellow text on a *green* background!
That sounds like a different bug ( somewhere in layout..I guess ). Replace "red" with #ff0000; and you should see the correct background.
I do see the correct background in this testcase: <html> <body "bgcolor=#FF0000" text="yellow"> <p> should be yellow on a red background </p> </body> </html> but I think that's because of the way we parse the hex color attributes. "bgcolor=#FF0000"" will also create a red background.
I'm attaching an interesting little testcase. It seems that what the parser is doing when it encounters a malformed attribute like this is that it interprets the leading quote as an attribute. It then proceeds to put the rest of the attribute into a *second* attribute, which has a trailing quote at the end. The testcase contains a table with an onclick attribute setting the Javascript and a malformed width attribute, "width=50%". The onclick pops up an alert containing the number of attributes for the table (using this.attributes.length). It currently returns 3, rather than 2.
Suggested stategy: 1) Remove the double-quote from theTerminalsChars. (In fact, I'm not entirely sure what the rationale is for some of the stuff defined in there. ReadWhitespace and SkipWhitespace both consume space, \b, and \t, so those are unnecessary. We need to keep \r and \n for View Source, but those will not affect our parsing in any of the normal modes.) 2) Create a new Boolean variable, call it advancedQuote (for the purposes of this comment, I don't know what the naming conventions are here...) Initialize to false. 3) [The code described in this step should be enclosed in a quirks-mode-only conditional.] At line 1641, call nsScanner::Peek to look at the next character. (The whitespace has been consumed, so this is the first non-whitespace character in the attribute.) If the character is a double quote or a single quote, advance the scanner by one, so the quote gets dropped, and change advancedQuote to true. 4) Add "|| advancedQuote" to the conditional on line 1671, so that such attributes will have their value sent to ConsumeQuotedText rather than ConsumeAttributeValue for stripping. Thoughts?
I figured out how to turn on the debugging code and here's the result I get with the following tag... <FORM method=post "action=http://cgi.cascss.unt.edu:8000/cgi-bin/untdirectory"> [attr] method=post: 0 [attr] =: 0 [attr] action=http://cgi.cascss.unt.edu:8000/cgi-bin/untdirectory": 0 Should it create an attribute token with a null mTextKey and mTextValue? Should there ever be a quote in mTextValue?
>Should it create an attribute token with a null mTextKey and mTextValue? >Should there ever be a quote in mTextValue? We're dealing with a malformed testcase here...and hence the unexpected behavior. Chistopher: Do you think it's worth taking a risk to fix such malformedness? Tell me how many sites are affected by this problem and if there are a bunch then we should consider fixing it. IMO, just fix the content not the user agent.
harishd: beats me, I have no idea how common this malformation is. I'm not terribly enthusiastic about seeing Quirks Mode expanded either, but part of the fix (removing the double-quote from theTerminalsChars) would also help us in standards mode (in standards mode, there shouldn't be an action attribute at all). It's your component, you make the call. As for risk, the main effect I can see of removing the double-quote is that it would break malformed key-value pairs of form key"value", but I don't know that anyone ever generates them. Having only examined the parser very recently, I'll have to defer to you on assessing the overall risk. (Also, ignore my above comments on whitespace in theTerminalsChars; I plead temporary stupidity.)
Harish: I do not wish to waste more of your time. I believe that I can create a patch that will be of minimal risk and solve the issue, so I will continue to work on this as I have time and will post my patch when I am satisfied that it works correctly. Please leave this bug open until I can get my patch ready (probably next week). If everyone tests the patch and likes it, then we can decide whether it should be submitted to the tree. Please spend your time on more important bugs now.
Attached patch Proposed Patch — — Splinter Review
Christopher: Please try the patch to see if it solves your issue(s).
A patch from BUG 91437 has modified the way that nsHTMLTokens.cpp handles whitespace. I will be reviewing this again to see how the updates affect misquoted attributes.
I think the better approach to achieve this behavior is to include kQuote and kApostrophe in the nsReadEndCondition of an unquoted attribute value. My patch to bug 57724 does that. Then the quote will be ignored like the quote in front of the key.
Ahh, but then do those quotes appear in view-source? I feel that view-source must be a 100% accurate representation of the original source sent from the web server or else it is useless. Otherwise, how do we find our own mistakes in our HTML or scripts? I have found the w3c's docs to say that the value may be quoted and must be if it contains spaces. I have not found any statement saying that quoting the key or the entire key=value construct is illegal. Even if it is not valid XML, quoting the entire construct appears to be allowable HTML according to the w3c specs! If there is a document I missed, please post its location here. By the way, I greatly appreciate all your work on optimizing the tokenizer.
> Ahh, but then do those quotes appear in view-source? Not now (like the first quote), but with my changes in bug 57724 they will. > I have found the w3c's docs to say that the value may be quoted and must be if > it contains spaces. I have not found any statement saying that quoting the key > or the entire key=value construct is illegal. HTML inherits its syntax from SGML. AFAIK the SGML spec isn't available online, but a summary of its syntax is at http://www.tiac.net/users/bingham/sgmlsyn/ (won't be very useful without further knowledge of SGML, though).
QA Contact: bsharma → moied
This bug has been marked "future" because the original netscape engineer working on this is over-burdened. If you feel this is an error, that you or another known resource will be working on this bug,or if it blocks your work in some way -- please attach your concern to the bug for reconsideration -----
Target Milestone: --- → Future
Keywords: patch, testcase
Summary: FORM does not work if ACTION parameter is mis-quoted → [quirks]FORM does not work if ACTION parameter is mis-quoted
Keywords: compat
Assignee: harishd → nobody
Status: ASSIGNED → NEW
QA Contact: moied → parser
Tokenization is now HTML5-compliant and, on this point, matches Opera and almost matches IE8. Marking WONTFIX. If you disagree, please file a bug against the HTML5 spec in the W3C Bugzilla.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: