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

[JENKINS-21052] Warn user that the copy button requires HTTPS #7665

Merged
merged 1 commit into from
Feb 26, 2023

Conversation

MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Feb 20, 2023

JENKINS-21052 Warn user that copy button requires HTTPS

Users that run Jenkins over an unencrypted (HTTP) connection will see the copy button, but it does not perform a copy. Browsers will only allow access to the clipboard if the page is in a secure context.

With this change, the text that is displayed when the user clicks the copy button from an insecure context will alert the user that copy requires a secure connection.

https://stackoverflow.com/questions/400212/how-do-i-copy-to-the-clipboard-in-javascript describes the alternatives in more detail.

JENKINS-21052 reported that the copy button does not work. I've confirmed that it works when using an HTTPS connection.

Testing done

  • Confirmed that the copy button now displays the "requires a secure context" when attempted with HTTP

Insecure context

HTTP-connection-without-copy

  • Confirmed that the copy button displays the expected text when used in in a secure context (HTTPS)

Secure context

HTTPS-connection-with-copy

No automated test because configuring an automated test in a secure context seems very complicated.

Proposed changelog entries

  • Warn user that copy button requires HTTPS.

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

Use a space suppressing diff to clearly see the added conditional for contexts that are not secure.

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

Users that run Jenkins over an unencrypted (HTTP) connection will see
the copy button, but it does not perform a copy.  Browsers will only
allow access to the clipboard if the page is in a secure context.

With this change, the text that is displayed when the user clicks the
copy button from an insecure context will alert the user that copy
requires a secure connection.

https://stackoverflow.com/questions/400212/how-do-i-copy-to-the-clipboard-in-javascript
describes the alternatives in more detail.

https://issues.jenkins.io/browse/JENKINS-21052 reported that the copy
button does not work.  I've confirmed that it works when using an HTTPS
connection.
@MarkEWaite MarkEWaite added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Feb 20, 2023
@MarkEWaite MarkEWaite changed the title Warn user that the copy button requires HTTPS [JENKINS-21052] Warn user that the copy button requires HTTPS Feb 20, 2023
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

likely doesn't affect much in the wild but may as well 👍, likely affects Jesse because he uses a non localhost url to avoid cookie clashes.

@NotMyFault
Copy link
Member

/label ready-for-merge


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 22, 2023
@MarkEWaite MarkEWaite merged commit f0be0ca into jenkinsci:master Feb 26, 2023
@MarkEWaite MarkEWaite deleted the warn-if-copy-over-http branch February 26, 2023 23:48
@jtnord
Copy link
Member

jtnord commented Mar 30, 2023

Caused JENKINS-70895 - some plugin automated tests fail because HTMLUnit does not recognize isSecureContext.

MarkEWaite added a commit to MarkEWaite/jenkins that referenced this pull request Mar 30, 2023
https://issues.jenkins.io/browse/JENKINS-70895 reports that
`isSecureContext` is not recognized by HTMLUnit in its JavaScript
handling.  That breaks tests for some plugins that use HTMLUnit to test
configuration round trips.

jenkinsci#7665 introduced this change
to notify users running over HTTP connections (insecure) that the copy
button does not work over insecure connections.

https://issues.jenkins.io/browse/JENKINS-21052 was the original motivation
for that informational message to the user.

James Nord tested this change in the automated test that was failing
with Jenkins 2.397 and confirmed that the test passes with this change.

I tested interactively over HTTP and confirmed that the copy button on
the inbound agent page still reports that copy is not supported over an
insecure session.

I tested interactively over HTTPS and confirmed that the copy button on
the inbound agent page still correctly copies the expected content.

I spent several unsuccessful hours trying to create an automated test
to show the failure.  I think we should include this fix without an
automated test because the automated tests in the plugin that James
maintains will detect the issue.
MarkEWaite added a commit that referenced this pull request Apr 1, 2023
https://issues.jenkins.io/browse/JENKINS-70895 reports that
`isSecureContext` is not recognized by HTMLUnit in its JavaScript
handling.  That breaks tests for some plugins that use HTMLUnit to test
configuration round trips.

#7665 introduced this change
to notify users running over HTTP connections (insecure) that the copy
button does not work over insecure connections.

https://issues.jenkins.io/browse/JENKINS-21052 was the original motivation
for that informational message to the user.

James Nord tested this change in the automated test that was failing
with Jenkins 2.397 and confirmed that the test passes with this change.

I tested interactively over HTTP and confirmed that the copy button on
the inbound agent page still reports that copy is not supported over an
insecure session.

I tested interactively over HTTPS and confirmed that the copy button on
the inbound agent page still correctly copies the expected content.

I spent several unsuccessful hours trying to create an automated test
to show the failure.  I think we should include this fix without an
automated test because the automated tests in the plugin that James
maintains will detect the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants