Bug 200850 - Put keygen element behind a runtime flag and disable it by default
Summary: Put keygen element behind a runtime flag and disable it by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 167018
  Show dependency treegraph
 
Reported: 2019-08-16 20:06 PDT by Ryosuke Niwa
Modified: 2019-08-21 13:27 PDT (History)
9 users (show)

See Also:


Attachments
Patch (55.30 KB, patch)
2019-08-16 20:41 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.23 MB, application/zip)
2019-08-16 21:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.08 MB, application/zip)
2019-08-16 22:35 PDT, Build Bot
no flags Details
Fixed more tests (62.54 KB, patch)
2019-08-20 16:41 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for ToT (62.54 KB, patch)
2019-08-20 17:13 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.89 MB, application/zip)
2019-08-20 19:56 PDT, Build Bot
no flags Details
DRT fix attempt for Windows (69.29 KB, patch)
2019-08-20 22:46 PDT, Ryosuke Niwa
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-08-16 20:06:55 PDT
HTML keygen element had been removed from Chrome & Firefox in 2017, and it has been removed from the specifications.
Given the number of security bugs we've had with keygen elements, we should probably remove it altogether.

In order to gather any developer feedback regarding this element, disable keygen element by default using runtime flag.
Comment 1 Ryosuke Niwa 2019-08-16 20:41:11 PDT
Created attachment 376587 [details]
Patch
Comment 2 Ryosuke Niwa 2019-08-16 20:45:35 PDT
Announced this on webkit-dev: https://lists.webkit.org/pipermail/webkit-dev/2019-August/030756.html
Comment 3 John Wilander 2019-08-16 21:12:55 PDT
Comment on attachment 376587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376587&action=review

Looks good to me.

> Source/WebCore/ChangeLog:9
> +        This patch disables the keygen element by default by adding a runtime enabled flag which is disabled by default.

Let’s point out that it’s an internal runtime flag for anyone who looks for the switch.

> Source/WebCore/page/RuntimeEnabledFeatures.h:-428
> -    bool m_dialogElementEnabled { false };

Why move this line?

> Source/WebKit/Shared/WebPreferences.yaml:1216
> +  humanReadableDescription: "Enables the deprecated and disabled-by-default HTML keygen element."

Is it deprecated or obsoleted? I’d say the latter.
Comment 4 Build Bot 2019-08-16 21:55:23 PDT
Comment on attachment 376587 [details]
Patch

Attachment 376587 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12929484

New failing tests:
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-newelements-xhtml.xhtml
imported/w3c/web-platform-tests/html/semantics/forms/the-label-element/labelable-elements.html
svg/dom/css-animate-input-foucs-crash.html
imported/w3c/web-platform-tests/html/semantics/interfaces.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-newelements.html
Comment 5 Build Bot 2019-08-16 21:55:26 PDT
Created attachment 376590 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 6 Build Bot 2019-08-16 22:35:24 PDT
Comment on attachment 376587 [details]
Patch

Attachment 376587 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12929509

New failing tests:
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-newelements-xhtml.xhtml
imported/w3c/web-platform-tests/html/semantics/forms/the-label-element/labelable-elements.html
svg/dom/css-animate-input-foucs-crash.html
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.getElementsByName/document.getElementsByName-newelements.html
Comment 7 Build Bot 2019-08-16 22:35:26 PDT
Created attachment 376593 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 8 Ryosuke Niwa 2019-08-20 16:41:19 PDT
Created attachment 376826 [details]
Fixed more tests
Comment 9 Ryosuke Niwa 2019-08-20 17:13:49 PDT
Created attachment 376830 [details]
Updated for ToT
Comment 10 John Wilander 2019-08-20 17:20:27 PDT
Comment on attachment 376830 [details]
Updated for ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=376830&action=review

> Source/WebCore/page/RuntimeEnabledFeatures.h:432
> +    bool m_dialogElementEnabled { false };

Again, why move the dialog element flag? It'll just look weird in blame.
Comment 11 Jiewen Tan 2019-08-20 18:17:40 PDT
Comment on attachment 376830 [details]
Updated for ToT

LGTM. r=me. Please take John's comment before landing the patch.
Comment 12 Ryosuke Niwa 2019-08-20 18:37:42 PDT
Comment on attachment 376830 [details]
Updated for ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=376830&action=review

>> Source/WebCore/page/RuntimeEnabledFeatures.h:432
>> +    bool m_dialogElementEnabled { false };
> 
> Again, why move the dialog element flag? It'll just look weird in blame.

Fixed.
Comment 13 Ryosuke Niwa 2019-08-20 18:43:45 PDT
Waiting for EWS...
Comment 14 Build Bot 2019-08-20 19:56:27 PDT
Comment on attachment 376830 [details]
Updated for ToT

Attachment 376830 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12949804

Number of test failures exceeded the failure limit.
Comment 15 Build Bot 2019-08-20 19:56:30 PDT
Created attachment 376839 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 16 Ryosuke Niwa 2019-08-20 22:46:42 PDT
Created attachment 376846 [details]
DRT fix attempt for Windows
Comment 17 Antti Koivisto 2019-08-21 07:08:39 PDT
Comment on attachment 376846 [details]
DRT fix attempt for Windows

Isn't this overly defensive? How about just deleting the whole thing?
Comment 18 Ryosuke Niwa 2019-08-21 13:04:45 PDT
(In reply to Antti Koivisto from comment #17)
> Comment on attachment 376846 [details]
> DRT fix attempt for Windows
> 
> Isn't this overly defensive? How about just deleting the whole thing?

Well, we don't know what kind of enterprise solutions are using this obsolete tech, and enterprise people don't test it until last minute.
Comment 19 Ryosuke Niwa 2019-08-21 13:25:12 PDT
Committed r248960: <https://trac.webkit.org/changeset/248960>
Comment 20 Radar WebKit Bug Importer 2019-08-21 13:27:25 PDT
<rdar://problem/54569074>