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

Hide password form fields by default #3991

Merged
merged 10 commits into from
Nov 13, 2019

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Apr 18, 2019

The goal here is twofold:

  • To prevent password auto-fill
  • To reduce the times browsers will offer to save (unrelated) passwords that are part of form submissions.

Example: Proxy Configuration

When the field value is empty to begin with, show a text field to prevent auto-fill.

proxy-empty

When the user enters a value, the field type is switched to password to mask it.

proxy-empty-filled

When a value is already filled in, show just a placeholder:

proxy-prefilled

Clicking the button will revert to how it looks in the second screen shot (different from f:multilineSecret which has some extra UI around the text area that appears).

Example: Parameterized Build

All concealed by default

Screen Shot 2019-04-19 at 01 22 31

Changing one password

Screen Shot 2019-04-19 at 01 22 37

Proposed changelog entries

  • Redesign password fields to prevent password auto-fill (undesirable except for the login form) and reduce the instances of browsers offering to update stored passwords. This change can be reverted any time by setting the system property hudson.Functions.hidingPasswordFields to false.

Submitter checklist

  • [n/a] JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

@daniel-beck daniel-beck requested review from jvz and Wadeck April 18, 2019 23:30
@daniel-beck daniel-beck added work-in-progress The PR is under active development, not ready to the final review web-ui The PR includes WebUI changes which may need special expertise labels Apr 18, 2019
@daniel-beck daniel-beck marked this pull request as ready for review April 18, 2019 23:33
@daniel-beck
Copy link
Member Author

I want a CI build so I cannot keep this in draft 😭

@daniel-beck
Copy link
Member Author

the problem that browsers will autofill forms with username and password fields if they're empty

Even adding a password field long after the page is loaded (e.g. getting rid of the choose/when in the view) will autofill saved credentials in Firefox. 😭

@daniel-beck
Copy link
Member Author

Right now I have something that appears to not crash & burn while preventing autofill or offers to save or update credentials in most cases.

Assuming of course, that Javascript is enabled.

How do we ensure this doesn't introduce worse problems than the one this is attempting to fix?

ATTRIBUTES="${attrs}" EXCEPT="field clazz value" />
<div class="hidden-password-placeholder">
<div class="hidden-password-legend">
<svg width="20px" height="25px" viewBox="0 0 25 32" version="1.1" xmlns="http://www.w3.org/2000/svg">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come to think of it, I think we don't need to inline this svg anymore. It was inlined in the standalone library because there was no good way to reference a static asset from a jar. This could be moved to a more appropriate location in jenkins to be a reusable static asset.

@zbynek
Copy link
Contributor

zbynek commented Apr 19, 2019

@daniel-beck to disable autocomplete in Chrome you can just add autocomplete="new-password" to the input element. Hopefully this will get implemnted in FF in the future, given that it's mentioned in MDN

EDIT: it is also working in Firefox Beta, see https://bugzilla.mozilla.org/show_bug.cgi?id=1119063

@daniel-beck
Copy link
Member Author

@zbynek I saw that on MDN, tried it, and failed. Thanks for linking the Firefox bug. Once that works and is reasonably widely supported (is Firefox ESR still around?) we can always change how we prevent autofill, but would rather not hold this change to wait for that (assuming it works otherwise).

@josephbrueggen
Copy link

@daniel-beck in the final screenshot (Changing one password) does selecting 'Build' beneath the form save the new password?

@daniel-beck
Copy link
Member Author

@josephbrueggen No. It's just valid for the current build you're starting, behavior in general in unchanged from before.

@josephbrueggen
Copy link

@daniel-beck understood, thanks. This looks good to me.

Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with Chrome 74.0.3729.131, FireFox 66.0.3 (and 66.0.5), IE 11.706.17134.0, Edge 42.17134.1.0 under Windows 10.
Feature tested:

  • writing directly,
  • copy-pasting
  • drag&dropping text

Every feature works as expected with all browsers.

My only concern was about the design, as in the multiline secret, the button is encapsulated in the border, with larger padding. But as Joe approved it, that's fine.

👍 for the proposal, but require to have tests to be approved :)

daniel-beck and others added 4 commits October 5, 2019 20:37
…at first

Something like onfocus didn't work, you'd tab through form elements
and unless you filled in the user name, changing the form field to
password would cause it to autocomplete.

It looks like, at least in Mac/Firefox, going from plain text to password
in the 'oninput' event handler works. The plain text is revealed neither
with typing nor pasting.
@daniel-beck daniel-beck force-pushed the hidden-password-fields branch from a90d18a to 9c694b4 Compare October 5, 2019 18:37
@daniel-beck daniel-beck removed the work-in-progress The PR is under active development, not ready to the final review label Oct 5, 2019
@daniel-beck
Copy link
Member Author

Anyone brave enough to write a noscript compatible variant of this the empty password field? I.e. only show it as a text field with the event handler if JavaScript is enabled? Perhaps @Wadeck has an idea how to do that?

@Wadeck
Copy link
Contributor

Wadeck commented Oct 9, 2019

No-JavaScript proposal in #4280.

@daniel-beck
Copy link
Member Author

Given how broken the Jenkins UI is without JavaScript, we don't need to specifically take additional care of that.

@daniel-beck daniel-beck added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Oct 10, 2019
@daniel-beck
Copy link
Member Author

I do not want this to be merged in the same weekly as #4239.

@daniel-beck daniel-beck removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Oct 12, 2019
@daniel-beck
Copy link
Member Author

Manually tested that form field validation continues to work, and it will show on the "concealed" field when the page is loaded.

@oleg-nenashev oleg-nenashev added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Oct 31, 2019
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine from the code standpoint, did not test manually.
I plan to merge it tomorrow if no negative feedback

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 31, 2019
@oleg-nenashev
Copy link
Member

Ugh, I missed it during the reviews, because it was on the third page. Will go ahead and merge it

@oleg-nenashev
Copy link
Member

@daniel-beck Just in case, did you have a chance to run ATH against it? The change is unlikely covered by smoke tests

@oleg-nenashev oleg-nenashev self-requested a review November 12, 2019 14:44
@oleg-nenashev oleg-nenashev added the upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed label Nov 12, 2019
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is likely to cause regressions in ATH. CC @olivergondza . I still think it worth merging even if we get a number of extra regressions there. I plan to merge it on Friday if no negative feedback

Copy link
Member

@olivergondza olivergondza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, if this is to improve things, we can handle som ATH breakage. We are looking for contributors, btw :P

@oleg-nenashev oleg-nenashev merged commit 90e693b into jenkinsci:master Nov 13, 2019
@oleg-nenashev
Copy link
Member

Maybe it should be a major RFE

@oleg-nenashev oleg-nenashev added major-rfe For changelog: Major enhancement. Will be highlighted on the top and removed rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Nov 18, 2019
@msmitty12
Copy link

How long would this change be expected to take before making it into an LTS release? We've had a lot of issues with a user's browser auto-filling forms in the Jenkins System Configuration without their knowledge and it seems like this change would address that.

@MarkEWaite
Copy link
Contributor

It was first included in Jenkins 2.205. Jenkins 2.204 was selected as the basis for the next long term support release. Jenkins 2.204.1 will be released in January and will be the long term support base for 3 months.

An LTS release including this capability will most likely be available in April 2020.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Nov 26, 2019 via email

@msmitty12
Copy link

msmitty12 commented Nov 26, 2019 via email

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Nov 26, 2019

@msmitty12 that matches with my reading of the pull request description. Because it has released with Jenkins 2.205, you could test drive it yourself and verify the behavior with:

$ docker run --rm -i -t -p 8080:8080 jenkins/jenkins:2.205

That will provide you a locally running copy of Jenkins 2.205 that can be accessed on port 8080.

I'm not aware of any other workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-rfe For changelog: Major enhancement. Will be highlighted on the top ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants