Bug 200850 – Put keygen element behind a runtime flag and disable it by default

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 376587 [details]
Patch

[Ryosuke Niwa]

Announced this on webkit-dev: https://lists.webkit.org/pipermail/webkit-dev/2019-August/030756.html

[John Wilander]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ryosuke Niwa]

Created attachment 376826 [details]
Fixed more tests

[Ryosuke Niwa]

Created attachment 376830 [details]
Updated for ToT

[John Wilander]

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.

[Jiewen Tan]

Comment on attachment 376830 [details]
Updated for ToT

LGTM. r=me. Please take John's comment before landing the patch.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Waiting for EWS...

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Ryosuke Niwa]

Created attachment 376846 [details]
DRT fix attempt for Windows

[Antti Koivisto]

Comment on attachment 376846 [details]
DRT fix attempt for Windows

Isn't this overly defensive? How about just deleting the whole thing?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Committed r248960: <https://trac.webkit.org/changeset/248960>

[Radar WebKit Bug Importer]

<rdar://problem/54569074>