Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-ui] 'input-security' considered harmful #6788

Open
MatsPalmgren opened this issue Nov 2, 2021 · 7 comments
Open

[css-ui] 'input-security' considered harmful #6788

MatsPalmgren opened this issue Nov 2, 2021 · 7 comments
Labels
css-ui-4 Current Work

Comments

@MatsPalmgren
Copy link

MatsPalmgren commented Nov 2, 2021

The css-ui spec says:

Users may wish to temporarily disable this obscuring in order to confirm that they’ve typed their sensitive information correctly. The input-security property may be used by authors to enable or disable this obscuring.

https://drafts.csswg.org/css-ui-4/#input-security

I agree with the use case -- users need a way to temporarily disable the obscuring -- but I think the spec authors arrive at the wrong solution.
I'm guessing their intent is that the page author should add a separate button to the page which sets the input-security property on the password control to do the unmasking.
I think this is a bad solution to the problem. This feature is much better implemented in the UA by supporting it directly in the password field. E.g.


(I believe Edge and some other UAs already do this.)

That way the user only need to learn one way of doing the unmasking, the same way for all sites, and it will always be available for all password controls. A UA is much more likely to implement a good solution in terms of usability/discoverability/accessibility, keyboard shortcuts consistent with the platform etc.. Having the unmasking button directly inside the password field itself also makes it obvious what it's for.

I don't really see a use case for input-security on other form controls either. It could be used on <input type=text> to obscure the text there, but if there's a need for that then that's a strong indication the author should have used an <input type=password> in the first place.

We've already seen that relying on page authors to implement custom form control behavior leads to problems. It will undoubtedly lead to numerous incompatible implementations, some of which completely fail to work in some UAs. Typically such implementations will also require JavaScript, so they will not work for NoScript users. They usually also fail to have proper support for keyboard navigation, accessibility, high-contrast themes etc...

input-security could also be abused, e.g. * { input-security: none; }

I request that input-security is removed from the spec.

@Loirooriol
Copy link
Contributor

This was added in #2495 (comment)

@frivoal
Copy link
Collaborator

frivoal commented Dec 22, 2021

It could be used on to obscure the text there, but if there's a need for that then that's a strong indication the author should have used an in the first place.

No it cannot, the property is only defined to apply to <input type=password> and similar sensitive text input as defined by the host language, that obscure the content by default.

The rest of your point stands.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed input-security considered harmful.

The full IRC log of that discussion <TabAtkins> Topic: input-security considered harmful
<TabAtkins> github: https://github.com//issues/6788
<TabAtkins> florian: I disagreed with just one of his statements, not all
<TabAtkins> florian: In UI4 we introduced 'input-security: auto | none'
<TabAtkins> florian: 'auto' does nothing by default, but on password fields (and other host-defined "sensitive" things) it obscures the text via *** or whatever
<TabAtkins> florian: 'none' turns that off
<TabAtkins> florian: Mats says this is a useful feature, but shouldn't be under the author's control, needing them to use JS on things.
<TabAtkins> florian: UAs are also much more likely to get a11y right on things like this.
<TabAtkins> dholbert: Also Edge already does this by default with a little button on password fields
<TabAtkins> astearns: Anyone implemented the CSS value?
<TabAtkins> TabAtkins: WebKit did, Chrome inherited it pre-fork
<fantasai> TabAtkins: Not ok to drop without a replacement
<fantasai> TabAtkins: maybe mark as deprecated and that we will remove, once HTML spec is updated to require
<fantasai> TabAtkins: No mandatory UI, but required functionality
<TabAtkins> smfr: I don't have much of an opinion.
<TabAtkins> smfr: If we need it internally we can keep it internally, don't have a strong opinion
<fantasai> florian: Can we not have the dependency on HTML?
<fantasai> TabAtkins: The functionality is useful
<fantasai> TabAtkins: optimal place is not in CSS, but if we don't have the functionality otherwise should leave it in
<fantasai> florian: I'm saying we drop it from CSS now, and encourage browsers to do the right thing
<fantasai> florian: rather than not removing it now
<fantasai> TabAtkins: That falls into the failure mode that's likely, which is that we remove it and nothing happens to HTML
<fantasai> TabAtkins: and I think this is useful enough for users that we shouldn't encourage nothing
<oriol> Firefox also has a UA button to show text (like Edge) behind pref: layout.forms.input-type-show-password-button.enabled
<fantasai> florian: Chrome has it?
<fantasai> TabAtkins: yeah
<fantasai> florian: Edge has it?
<fantasai> dholbert: Firefox also has it on Nightly
<fantasai> florian: So seems like the scenario of not having it is unlikely
<fantasai> Rossen: talking about the property?
<fantasai> florian: The behavior of being able to reveal the passwrod
<jensimmons> Issue at HTML: https://github.com/whatwg/html/issues/7293
<fantasai> TabAtkins: We don't have it in Chrome
<fantasai> ??: We disabled in Chrome because of compat issues
<fantasai> astearns: Issue in HTML spec?
<fantasai> scribe+
<fantasai> astearns: That issue mentions something I'm concerned about, which is the HTML spec might need a CSS definition in order to say "here's what happens"
<fantasai> astearns: with this attribute or UI
<fantasai> florian: OK, maybe let's go back to what Tab suggests
<fantasai> florian: Resolve, we would like to remove this, would like it to be in HTML
<fantasai> astearns: So adding an issue to draft, we'd like to remove pls don't implement?
<fantasai> dholbert: My concern is that if we leave it, HTML spec might point at CSS for how to do it, and then JS can set in buggy ways
<fantasai> astearns: Note would say we'd like to remove, pls don't implement property, and HTML should define in a way that doesn't depend on CSS
<fantasai> florian: Tess is arguing for what we are saying to not do
<fantasai> astearns: Which is we should make it an issue and get Tess's input
<fantasai> Tim_Nguyen: Issue on our side was that inputs tend to have buttons for e.g. password autocomplete, and would need UI that wouldn't interfere
<fantasai> astearns: Sounds like we're not going to resolve any spec edits today, but let's add an issue to the draft saying we'd like to remove this
<fantasai> astearns: Proposed resolution is to put our recommendation into an issue in the draft, and come back to it later when we can get more discussion on it
<fantasai> astearns: Objections?

@astearns astearns removed the Agenda+ label Jan 19, 2022
@astearns
Copy link
Member

The minutes missed this part:

RESOLVED: Put an issue in the draft saying we'd like to remove

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Jan 26, 2022
https://bugs.webkit.org/show_bug.cgi?id=235557
rdar://87984277

Reviewed by Dean Jackson.

Source/WebCore:

CSSWG is planning to remove input-security from CSS UI 4
(w3c/csswg-drafts#6788).

Keep the property around as an experimental feature, so that
it can be used in the UA stylesheet, and easily turned on if the
resolution is not finalized.

* css/CSSComputedStyleDeclaration.cpp:
(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* css/CSSProperties.json:
* css/parser/CSSParserContext.cpp:
(WebCore::CSSParserContext::CSSParserContext):

Enable input-security when parsing the UA stylesheet, as it is used to
obscure text in password inputs.

(WebCore::operator==):
(WebCore::add):
(WebCore::CSSParserContext::isPropertyRuntimeDisabled const):
* css/parser/CSSParserContext.h:
* css/parser/CSSParserFastPaths.cpp:
(WebCore::CSSParserFastPaths::isValidKeywordPropertyAndValue):

Source/WebKitLegacy/win:

Add support for tests enabling the CSSInputSecurityEnabled preference.

* WebPreferences.cpp:
(WebPreferences::cssInputSecurityEnabled):
* WebPreferences.h:
* WebView.cpp:
(WebView::notifyPreferencesChanged):

Source/WTF:

* Scripts/Preferences/WebPreferencesExperimental.yaml:

Make input-security a disabled-by-default experimental feature.

Tools:

* DumpRenderTree/TestOptions.cpp:
(WTR::TestOptions::defaults):

Add default for WebKitLegacy on Windows.

LayoutTests:

* fast/css/computed-text-security-for-input-security.html:



Canonical link: https://commits.webkit.org/246412@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288590 268f45cc-cd09-0410-ab3c-d52691b4dbfc
annulen pushed a commit to qtwebkit/qtwebkit that referenced this issue Jan 26, 2022
https://bugs.webkit.org/show_bug.cgi?id=235557
rdar://87984277

Reviewed by Dean Jackson.

Source/WebCore:

CSSWG is planning to remove input-security from CSS UI 4
(w3c/csswg-drafts#6788).

Keep the property around as an experimental feature, so that
it can be used in the UA stylesheet, and easily turned on if the
resolution is not finalized.

* css/CSSComputedStyleDeclaration.cpp:
(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* css/CSSProperties.json:
* css/parser/CSSParserContext.cpp:
(WebCore::CSSParserContext::CSSParserContext):

Enable input-security when parsing the UA stylesheet, as it is used to
obscure text in password inputs.

(WebCore::operator==):
(WebCore::add):
(WebCore::CSSParserContext::isPropertyRuntimeDisabled const):
* css/parser/CSSParserContext.h:
* css/parser/CSSParserFastPaths.cpp:
(WebCore::CSSParserFastPaths::isValidKeywordPropertyAndValue):

Source/WebKitLegacy/win:

Add support for tests enabling the CSSInputSecurityEnabled preference.

* WebPreferences.cpp:
(WebPreferences::cssInputSecurityEnabled):
* WebPreferences.h:
* WebView.cpp:
(WebView::notifyPreferencesChanged):

Source/WTF:

* Scripts/Preferences/WebPreferencesExperimental.yaml:

Make input-security a disabled-by-default experimental feature.

Tools:

* DumpRenderTree/TestOptions.cpp:
(WTR::TestOptions::defaults):

Add default for WebKitLegacy on Windows.

LayoutTests:

* fast/css/computed-text-security-for-input-security.html:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@288590 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@hober
Copy link
Member

hober commented Feb 8, 2022

See the recent discussion in whatwg/html#7293. I think it would be best to retain input-security and to mandate that "IF the UA exposes its own UI for this, that UI MUST be implemented by setting the input-security property on the element." That way we could at least ensure browser-provided UI and page-provided UI didn't fight each other.

JonWBedard pushed a commit to WebKit/WebKit that referenced this issue Feb 10, 2022
    Disable input-security CSS property
    https://bugs.webkit.org/show_bug.cgi?id=235557
    rdar://87984277

    Reviewed by Dean Jackson.

    Source/WebCore:

    CSSWG is planning to remove input-security from CSS UI 4
    (w3c/csswg-drafts#6788).

    Keep the property around as an experimental feature, so that
    it can be used in the UA stylesheet, and easily turned on if the
    resolution is not finalized.

    * css/CSSComputedStyleDeclaration.cpp:
    (WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
    * css/CSSProperties.json:
    * css/parser/CSSParserContext.cpp:
    (WebCore::CSSParserContext::CSSParserContext):

    Enable input-security when parsing the UA stylesheet, as it is used to
    obscure text in password inputs.

    (WebCore::operator==):
    (WebCore::add):
    (WebCore::CSSParserContext::isPropertyRuntimeDisabled const):
    * css/parser/CSSParserContext.h:
    * css/parser/CSSParserFastPaths.cpp:
    (WebCore::CSSParserFastPaths::isValidKeywordPropertyAndValue):

    Source/WebKitLegacy/win:

    Add support for tests enabling the CSSInputSecurityEnabled preference.

    * WebPreferences.cpp:
    (WebPreferences::cssInputSecurityEnabled):
    * WebPreferences.h:
    * WebView.cpp:
    (WebView::notifyPreferencesChanged):

    Source/WTF:

    * Scripts/Preferences/WebPreferencesExperimental.yaml:

    Make input-security a disabled-by-default experimental feature.

    Tools:

    * DumpRenderTree/TestOptions.cpp:
    (WTR::TestOptions::defaults):

    Add default for WebKitLegacy on Windows.

    LayoutTests:

    * fast/css/computed-text-security-for-input-security.html:

    Canonical link: https://commits.webkit.org/246412@main
    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288590 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Canonical link: https://commits.webkit.org/245886.128@safari-613-branch
git-svn-id: https://svn.webkit.org/repository/webkit/branches/safari-613-branch@289308 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@lukewarlow
Copy link
Member

See the recent discussion in whatwg/html#7293. I think it would be best to retain input-security and to mandate that "IF the UA exposes its own UI for this, that UI MUST be implemented by setting the input-security property on the element." That way we could at least ensure browser-provided UI and page-provided UI didn't fight each other.

How do you mean "don't fight each other" here. Because this would still lead to page UI getting out of sync with the browser UI.

User clicks browser UI to show password. Author toggle would still think the input is set to "hidden". and Is now out of sync with reality.

This as a mechanism also relies on password inputs to use input security and not the current technique of flipping between type="password" and type="text".

@lukewarlow
Copy link
Member

whatwg/html#7293 (comment) linking my comment on the html issue here because it's probably more relevant here. TLDR should ::-ms-reveal be standardised as an effective (and back compatible) form of feature detection for password reveal buttons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-ui-4 Current Work
Projects
None yet
Development

No branches or pull requests

7 participants