Bug 70617 – Optional "keytype" attribute for the <keygen> tag is handled incorrectly in appendFormData().

• Component: DOM
• Status: RESOLVED FIXED

[Gaurav Shah]

Change http://trac.webkit.org/changeset/97658 introduced a logical bug where-by all <keygen keytype=".."> are incorrectly handled as unsupported in appendFormData(). This effectively renders the the tag on a website as a no-op.

[Gaurav Shah]

Created attachment 111981 [details]
Patch

[Gaurav Shah]

I tested this on a Chromium on Chromium OS build, and the <keygen> tag is now handled correctly. 

The site I used was: http://foaf.me/simple_KEYGEN_CreateClientCertificate.php

[Darin Adler]

Comment on attachment 111981 [details]
Patch

Normally we require regression tests with all bug fixes. Is there a reason you can’t write a regression test for this?

[Andreas Kling]

Ouch, I'm terribly sorry about this mistake guys!

[Jason Kersey]

I merged this by hand to the chromium 912 branch.

[Gaurav Shah]

Re:#3 I am totally new to Webkit, so not sure where the test goes and how it needs to be written. I didn't see any unit tests for this piece of code either. In any case, I will get some help and try to get a regression test included (if possible) and upload the new patch. Thanks for the quick review!

[Ryan Sleevi]

(In reply to comment #3)
> (From update of attachment 111981 [details])
> Normally we require regression tests with all bug fixes. Is there a reason you can’t write a regression test for this?

As someone who has worked with the <keygen> implementation on the Chromium side, and studied the various ports on the WebKit side, I think one of the big challenges to this is that it requires an actual form submission in order to repro.

<keygen>, however, is not idempotent with respect to system state, and the act of submitting a <form> element with a <keygen> tag can result in creating a new private key within whatever keystore the port uses (which, in most implementations, is the OS-provided keystore/cryptographic APIs)

Unlike other APIs which may modify system state (such as those that may create/modify files), this is not presently something WebKit supports mocking/stubbing. The current <keygen> tests just focus on the immutable aspects - parsing and rendering - and ignore submission.

Since the <keygen> element isn't processed on the C++ side until form submission, this is not something where you could poke with JavaScript as a LayoutTest to grab the resultant value in an onsubmit() handler.

[Adam Barth]

Sounds like we need to add the ability to mock this interaction with the system.  Presumably WebKit interacts with the system via the Chromium embedding API.  Can't DRT, as the embedder, inject the mock at that time?  It's ok if the test only runs on the Chromium port.

[Gaurav Shah]

Created attachment 112100 [details]
Patch

[Gaurav Shah]

Based on some pointers by abarth(thanks!), I added a regression test which uses the http server. Basically, it tests if the post request contains the <keygen> name= parameter. The test passes with the fix (and fails without). 

However, to make that work I need to remove the following check from appendForm():

if (value.isNull())
  return false;

As rsleevi mentioned in comment#7, signedPublicKeyAndChallenge() is a platform method whose implementation varies depending on the specific port in question. WebKit test runs (I tested this by building WebKit for chromium) seem to stub it out returning an empty String, causing the above check to fail. The POST is empty  (since the subsequent appendData() never gets called) and the test doesn't work.

Is this is a bad idea? An empty signedPublicKeyandChallengeString error should be handled by the server. I would argue that receiving an empty post request is worse (which happens now when keygen processing fails on the client) than receiving an empty "name= " POST (with this change).

Thoughts?

[Darin Adler]

(In reply to comment #10)
> However, to make that work I need to remove the following check from appendForm():
> 
> if (value.isNull())
>   return false;
> 
> WebKit test runs (I tested this by building WebKit for chromium) seem to stub it out returning an empty String, causing the above check to fail.

If test runs returned an empty string, then that check would not fail. There is a distinct empty string and null string. If you want to change the stub to return an empty string instead of a null string, that’s OK with me. Making that change should not be difficult.

> I would argue that receiving an empty post request is worse (which happens now when keygen processing fails on the client) than receiving an empty "name= " POST (with this change).

If we’re going to actually consider changing the WebCore code then lets not talk about which of these two worse, lets talk about what desired behavior would be. Are there any real-world examples where keygen fails. If so, what would be the best possible behavior for that case and why? But lets discuss this in another bug, because there is no need for you to change WebCore just to make the test work.

[Darin Adler]

(In reply to comment #11)
> (In reply to comment #10)
> > However, to make that work I need to remove the following check from appendForm():
> > 
> > if (value.isNull())
> >   return false;
> > 
> > WebKit test runs (I tested this by building WebKit for chromium) seem to stub it out returning an empty String, causing the above check to fail.
> 
> If test runs returned an empty string, then that check would not fail. There is a distinct empty string and null string. If you want to change the stub to return an empty string instead of a null string, that’s OK with me. Making that change should not be difficult.

It occurs to me that for testing, it would be even better to change this to return some placing string that is helpful to interpret the test output rather than an empty string. That might be easy to do.

[Darin Adler]

(In reply to comment #12)
> to return some placing string

some placeholder string

[Gaurav Shah]

(In reply to comment #11)
> (In reply to comment #10)
> > However, to make that work I need to remove the following check from appendForm():
> > 
> > if (value.isNull())
> >   return false;
> > 
> > WebKit test runs (I tested this by building WebKit for chromium) seem to stub it out returning an empty String, causing the above check to fail.
> 
> If test runs returned an empty string, then that check would not fail. There is a distinct empty string and null string. If you want to change the stub to return an empty string instead of a null string, that’s OK with me. Making that change should not be difficult.
> 

I can make that change. Chromium's stub implementation comes from the glue code, so I need to first land a chromium change first. For qt and efl, there are already stub implementations which I can modify to return an empty string. I am not sure about the Mac or Windows case though. From my reading of it, they don't use stub implementations during tests. 

Looks like the simplest route is to just make this test run on the chromium port.

> > I would argue that receiving an empty post request is worse (which happens now when keygen processing fails on the client) than receiving an empty "name= " POST (with this change).
> 
> If we’re going to actually consider changing the WebCore code then lets not talk about which of these two worse, lets talk about what desired behavior would be. Are there any real-world examples where keygen fails. If so, what would be the best possible behavior for that case and why? But lets discuss this in another bug, because there is no need for you to change WebCore just to make the test work.


Fair enough. Agreed that the discussion is better done in a separate bug. (To answer your question: keygen depends on the platform specific crypto library implementation and there are many reasons why it would fail - misconfigured library or crypto hardware, etc.)

[Gaurav Shah]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > However, to make that work I need to remove the following check from appendForm():
> > > 
> > > if (value.isNull())
> > >   return false;
> > > 
> > > WebKit test runs (I tested this by building WebKit for chromium) seem to stub it out returning an empty String, causing the above check to fail.
> > 
> > If test runs returned an empty string, then that check would not fail. There is a distinct empty string and null string. If you want to change the stub to return an empty string instead of a null string, that’s OK with me. Making that change should not be difficult.
> 
> It occurs to me that for testing, it would be even better to change this to return some placing string that is helpful to interpret the test output rather than an empty string. That might be easy to do.

This would be fine if there was a concept of mock/stub implementations which gets used during test runs. The Chromium implementation, for example, re-use the stub implementation while building tests for the Chromium Webkit port.

Side Note: To do this for all ports is quite complicated. Right now there are no universal stub/mock implementations of these platform-specific methods that get used across all ports during tests. (See my earlier comment about Chromium's stub implementation coming from its glue layer whose sources are not part of WebKit)

[Gaurav Shah]

Created attachment 112622 [details]
Updated patch after fixing Chromium Stub Implementation to return an empty string.

[Gaurav Shah]

In parallel, I have a Chromium change to return an empty instead of a NULL string:
http://codereview.chromium.org/8401019/

What do I need to do to mark this test as only succeeding on the chromium port?

[WebKit Review Bot]

Comment on attachment 112622 [details]
Updated patch after fixing Chromium Stub Implementation to return an empty string.

Attachment 112622 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10228539

New failing tests:
http/tests/misc/submit-post-keygen.html

[Gaurav Shah]

ping? The Chromium patch has landed, so cr-linux test failure should be going away. If there a way I can request a re-try of the patch? (didn't find it in the documentation here: http://www.webkit.org/coding/contributing.html)

[Gaurav Shah]

Created attachment 113332 [details]
Re-uploaded patch for another EWS run now that the chromium changes are in

[WebKit Review Bot]

Comment on attachment 113332 [details]
Re-uploaded patch for another EWS run now that the chromium changes are in

Clearing flags on attachment: 113332

Committed r99297: <http://trac.webkit.org/changeset/99297>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Fehér Zsolt]

After r99297 failed inspector/cookie-parser.html
Can someone to look up or can i skipped this test?

[Jer Noble]

This test has been failing occasionally but consistently on Mac Lion test bots.

See: 
http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r123255%20(1382)/results.html
http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r123255%20(1382)/http/tests/misc/submit-post-keygen-pretty-diff.html