Closed Bug 314980 Opened 17 years ago Closed 17 years ago
.get Attribute() method error (CAttribute Token::Sanitize Key())
17 years ago
Assignee: nobody → mrbkap
Component: General → HTML: Parser
Product: Firefox → Core
QA Contact: general → parser
Version: unspecified → Trunk
Yeah, might as well sync up with IE. It might be worth it to figure out if IE doesn't allow various characters in attribute keys (note: this function was added for bug 7548).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Priority: -- → P4
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
IMHO we should do as little "fixup" as possible of the markup. Dropping ending '/' seems like a good idea to ease xhtml migration. Other then that I think we should not strip any more characters then what IE does.
I did some testing; IE treats / as a terminator for attributes (which means that in cases like <foo f/g>, it treats foo as having 2 attributes, f and g). Do we want to emulate that?
Ugh, our current behavour is less then ideal. For <div f/b> we create an attribute named 'f/b'. IMHO we should never create attributes or elements with names containing illigal characters. Not sure what qualifies as legal characters in HTML/SGML, but in XML it's pretty clearly defined http://www.w3.org/TR/2004/REC-xml11-20040204/#NT-Name Ideally we should drop other charaters, so parsing <div f/b> as <div f b> or something similar seems good to me. Very imporantly, we should never put an ascii-null inside a element or attribute name. I suspect that that could cause security issues if we compare strings and atoms.
This needs to be pounded on, hard. I think it should do the right thing, though.
Running these testcases should be self-explanatory. Are there any cases that I'm missing? Obsoleting attachment 201898 [details] [diff] [review] in preparation for a better patch.
Attachment #201898 - Attachment is obsolete: true
This patch seems to provide the wanted compatibility.
Attachment #202115 - Flags: review?(bzbarsky)
Comment on attachment 202115 [details] [diff] [review] Fixed patch >Index: parser/htmlparser/src/nsHTMLTokens.cpp >+ NS_ASSERTION(aChar == kApostrophe|| aChar == kQuote || aChar == kForwardSlash, Space before first ||, please. >@@ -2029,39 +1994,41 @@ nsresult CAttributeToken::Consume(PRUnic >+ result = aScanner.SkipOver(aChar); //strip quote. quote or slash r=bzbarsky
Attachment #202115 - Flags: review?(bzbarsky) → review+
Attachment #202115 - Flags: superreview?(jst)
Comment on attachment 202115 [details] [diff] [review] Fixed patch sr=jst
Attachment #202115 - Flags: superreview?(jst) → superreview+
Fix checked into trunk. I'll file a new bug on our potentially allowing NULs in attribute keys in a second.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
This patch is simply the result of CVS joins. It applies cleanly to the 1.8 and 1.8.0 branches.
Wanted for compatibility with other browser's parsing of attributes; being non-standard here is complicating web apps trying to filter content.
Comment on attachment 250215 [details] [diff] [review] Patch for the 1.8 branches Approved for both branches, a=jay for drivers.
Using provided testcase confirmed existence of bug in 220.127.116.11. Verified for 18.104.22.168pre and 22.214.171.124pre. All PASS for testcase. Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:126.96.36.199pre) Gecko/2007011103 BonEcho/188.8.131.52pre Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:184.108.40.206pre) Gecko/20070111 Firefox/220.127.116.11pre
You need to log in before you can comment on or make changes to this bug.