Closed Bug 390565 Opened 12 years ago Closed 12 years ago

HTML parser should not move <input type="hidden">

Categories

(Core :: DOM: Core & HTML, defect, P1)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: bugzilla, Assigned: bzbarsky)

References

()

Details

(Keywords: regression, testcase)

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072522 Minefield/3.0a7pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072522 Minefield/3.0a7pre

Since this build I cannot access the internet banking service from my bank everything was ok before.

When I enter my account info it tries to call:
https://bankline.itau.com.br/GRIPNET/bklcgi.exe

and it display a message that the service is not available, but if I use a previous build I can access it.

Reproducible: Always

Steps to Reproduce:
1.go to www.itau.com.br
2.Fill account info ( I can provide this info if this bug is not public )
3.hit ok
Actual Results:  
it display a message that the service is not available

Expected Results:  
it should display a box with my name on it
Summary: Cannot access my bank service since this build → Cannot access my bank service since build 2007072522
Just to confirm the builds

this one works ->20070725_2047_firefox-3.0a7pre.en-US.win32.zip
since this one don't ->20070725_2206_firefox-3.0a7pre.en-US.win32.zip 
If I understand the page correctly, you login info is not necessary.
I think the bug can also be seen when you fill in random numbers as account info, right?
Attached file testcase (obsolete) —
The more specific url is https://bankline.itau.com.br/lgnet/BanklineItauPF.htm
The attached testcase shows what the problem is in current trunk builds.

During the onsubmit handler the value of the input is changed, but in current trunk builds this isn't posted anymore.
Regression from bug 353415.

Login info is not needed, so this bug can become public now.
Blocks: 353415
Group: security
Status: UNCONFIRMED → NEW
Component: General → DOM: HTML
Ever confirmed: true
Keywords: regression, testcase
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
So... if I load the testcase, then click the button the URL that's loaded is <https://bugzilla.mozilla.org/attachment.cgi?a=this+should+be+posted>.

That's what I get with older builds too.  Martijn, is that not what you're seeing?
Note that the page _does_ have the sort of "hidden inputs in a bogus place inside the table" thing going on that bug 353415 would affect.  If they're also depending on posting order (which they might be if they use a hand-rolled exe to handle the post) we might have a problem. :(  But the testcase doesn't have anything like that in it...
Flags: blocking1.9?
Blake, how hard would it be to leave hidden inputs and only hidden inputs (not other kinds of inputs) as kids of <tr> or whatever the page sticks them inside?  If we did that, things would Just Work....
Comment on attachment 274869 [details]
testcase

(In reply to comment #6)
> So... if I load the testcase, then click the button the URL that's loaded is
> <https://bugzilla.mozilla.org/attachment.cgi?a=this+should+be+posted>.
> 
> That's what I get with older builds too.  Martijn, is that not what you're
> seeing?

Sorry, I'm seeing that too now (I thought I didn't, before). So this testcase is just wrong.
Attachment #274869 - Attachment is obsolete: true
Since what you thought this bug was about isn't do you need my account info to login to try to diagnose this ?

How can I help to get this fixed ?
(In reply to comment #10)
> Since what you thought this bug was about isn't do you need my account info to
> login to try to diagnose this ?

No, we don't need your account info anymore, thanks for the offer. It's known what the source of issue is.

> How can I help to get this fixed ?

You can't help anymore atm, unless you can code yourself, then you could make a patch.
You can help again with testing afterwards, if a fix has been made.
Boris, can you work on this regression? If not, please reassign...
Flags: blocking1.9? → blocking1.9+
Assignee: nobody → bzbarsky
I'm sort of waiting on Blake to see how easy this is to fix on the parser end.  If it's not, we have to redesign how form.elements works.... :(  And yeah, I guess I can do that.
Damon, I think we should block for M10 on this one way or another.  Either to get this fixed, or to back out the various patches we've made to form control storage that have gotten us here (including support for the :default pseudo-class, probably).  I wouldn't feel comfortable with the sort of changes we need here after M10.  In fact I'm not sure how comfortable I am even then, but both going forward and going back are risky.

I'm sort of assuming Blake's not going to be able to find time to work on this at this point, so I'll try to find time to sort out the HTML parser magic myself, I guess.  :(
Target Milestone: --- → mozilla1.9 M10
So the problem here is that we're moving <input type=hidden>s placed at bad places in a <table> to before the table? Thus rearranging their order when submitted since we are now better at keeping form.elements in the actual order of the DOM?

If so, this is actually worrying me a bit since this could actually happen to <input type=text> too. I wonder if we can add code to form.elements that keeps parser-moved form controls out of order? That seems like the best and safest solution.
> this could actually happen to <input type=text> 

It could, but unlike with hidden inputs there's visual indication that it's been moved outside the table in that case.  So while hidden inputs interspersed with table parts are common, text inputs in that situation are not....

As I see it, we have 4 main options here:

1)  Leave things as-is, wontfix this bug, evangelize sites that have this
    issue.  This last could be hard, since any site hitting this is not using
    any off-the-shelf CGI library, so would need to actually update their
    server-side code.
2)  Go back to storing form controls out of order in form.elements.  That
    means backing out bug 353415, bug 347165, bug 352980, and the "default
    submit button" part of bug 302186, since this last is pretty much
    impossible to implement efficiently if any mutation requires us to work
    the full elements array (which it would if it were out of order).  There
    might be other patches that would need to come out too; I'd have to
    look pretty carefully at the CVS log to make sure.  It would also make us
    violate the proposed HTML5 spec, but so will solutions 3 and 4, so that's
    not a big deal.  See the thread at <http://lists.w3.org/Archives/Public/public-html/2007Sep/0053.html>.
3)  Keep form.elements in document order, but submit in some other order.  This
    is what Ian proposes in <http://lists.w3.org/Archives/Public/public-html/2007Sep/0128.html>.
4)  Make the parser change I propose in this bug.

In my opinion, 2 is pretty risky due to the number of things we have to make sure we back out correctly, and removes a feature we want.  3 is even riskier because it's hard to get right.  See the responses to Ian in the thread cited above.  4 has the issue comment 15 points out, but I think that this is not likely to bite sites.

Safari has been doing 4 for a while, and they're doing OK with it, apparently.  They feel that this is what the HTML5 spec should say, in fact.  See <http://lists.w3.org/Archives/Public/public-html/2007Sep/0318.html>.
Priority: -- → P1
Assignee: bzbarsky → mrbkap
I'm guessing Blake is not likely to get to this before M10 - is there someone else who knows the parser code?  I'm nervous to touch this post b2...
Blake, any pointers here would really help as I don't know the innards of the parser code at all :(
Attached patch Patch, kinda (obsolete) — Splinter Review
This is ugly.  Real ugly.  So ugly the rest of CNavDTD doesn't invite it to parties.  But it's the best I can think of as far as non-invasive changes go.  I tried doing a separate tag type for hidden inputs, and it's just too much pain I think.  Especially getting the nsHTMLTag -> string mappings to be sane.

The tests do cover the cases we care about, and this seems to work... Blake, what do you think?
Assignee: mrbkap → bzbarsky
Status: NEW → ASSIGNED
Attachment #290656 - Flags: superreview?(mrbkap)
Attachment #290656 - Flags: review?(mrbkap)
I guess I should perhaps take the code I'm adding to CNavDTD::HandleToken and reuse it in DoesRequireBody?  And use the GetAttributeCount() thing instead of just walking till I get to a non-attribute token?

I'd still like to get an overall sanity-check on the patch, but I suspect I'll have to make those changes, right?
Summary: Cannot access my bank service since build 2007072522 → [FIX]Cannot access my bank service since build 2007072522
Attachment #290656 - Attachment is obsolete: true
Attachment #290725 - Flags: superreview?(mrbkap)
Attachment #290725 - Flags: review?(mrbkap)
Attachment #290656 - Flags: superreview?(mrbkap)
Attachment #290656 - Flags: review?(mrbkap)
Comment on attachment 290725 [details] [diff] [review]
With that cleanup, and including tests for bug 66985

>Index: parser/htmlparser/src/CNavDTD.cpp
>+// when all the attributes for the input are still in the tokenizer.
>+static PRBool
>+IsHiddenInput(CToken* aToken, nsITokenizer* aTokenizer)
>...
>+  PRInt32 ac = aToken->GetAttributeCount();
>+  NS_ASSERTION(ac <= aTokenizer->GetCount(),
>+               "Not enough tokens in the tokenizer");

I'd rather make this assertion (and the one below) if/returns. CNavDTD does weird things with the tokenizer and things occasionally don't actually match up, though they might be all fixed by this point.

>+  for (PRInt32 i = 0; i < ac; ++i) {
>+    NS_ASSERTION(eHTMLTokenTypes(aTokenizer->GetTokenAt(i)->GetTokenType()) ==
>+                   eToken_attribute, "Unexpected token type");

...especially because if this assertion fails, it's an exploitable crash.

> static PRBool
> DoesRequireBody(CToken* aToken, nsITokenizer* aTokenizer)
> {
>   PRBool result = PR_FALSE;
> 
>   if (aToken) {

Perhaps this isn't the bug to make this change in, but this function is only called from HandleToken, which bails on a null aToken. So you could get rid of this if statement and the |result| variable (or maybe just inline the function into its one us).

>+      PRBool isHiddenInputInsideTableElement = PR_FALSE;
>+      if (aChildTag == eHTMLTag_input &&
>+          FindTagInSet(theParentTag, sTableElements,
>+                       NS_ARRAY_LENGTH(sTableElements))) {

FWIW, if this |FindTagInSet| turns out to be expensive, we could add some data to the element table to help make this decision.

Thanks for taking this on and sorry for not getting to it. I was worried that creating a new element table entry might be a bunch of extra work :-/.
Attachment #290725 - Flags: superreview?(mrbkap)
Attachment #290725 - Flags: superreview+
Attachment #290725 - Flags: review?(mrbkap)
Attachment #290725 - Flags: review+
> I'd rather make this assertion (and the one below) if/returns.

I left both assertions, but made us only go up to the PR_MIN of ac and GetTokenCount() when walking attributes, and made us bail out if we get a non-attr token.

> or maybe just inline the function

I did that, since at this point it's just a single boolean conditional, basically.

Checked in, with the tests.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I'm the reporter of the bug and can confirm it as verified in the original url of the problem.

Thank you!
Status: RESOLVED → VERIFIED
WFM too.
Thanks.
Depends on: 406903
Summary: [FIX]Cannot access my bank service since build 2007072522 → HTML parser should not move <input type="hidden">
Component: DOM: HTML → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.