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

warn user when deprecated Secret#toString() is being used #3668

Merged
merged 2 commits into from Oct 11, 2018

Conversation

6 participants
@ndeloof
Member

ndeloof commented Oct 3, 2018

Secret.toString is deprecated since 1.356 but probably still used in various places.
This PR adds a warning to let user know about this misuse, the same way remoting's AnonymousClassWarnings warn about bad class design. I expect this to help us prepare the next step, which is to remove Secret.toString override and ensure secret never will leak by misuse of Object.toString (logs, dump, etc)

Proposed changelog entries

  • Warn user when deprecated Secret#toString() is being used

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change).
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@daniel-beck

warn user when Secret.toString is being used
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@jvz

jvz approved these changes Oct 3, 2018

LGTM in theory at least.

@daniel-beck daniel-beck self-requested a review Oct 5, 2018

@ndeloof

This comment has been minimized.

Member

ndeloof commented Oct 8, 2018

@daniel-beck are you ok with this PR so I can merge it ?

@@ -92,6 +95,8 @@
@Override
@Deprecated
public String toString() {
final String from = new Throwable().getStackTrace()[1].toString();
LOGGER.warning("Use of toString() on hudson.util.Secret from "+from+". Prefer getPlainText() or getEncryptedValue() depending your needs.");

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 8, 2018

Member

This message should be proposing creating an issue to the plugin developer IMHO.

This comment has been minimized.

@ndeloof

ndeloof Oct 8, 2018

Member

or even better, offer a link to some explanatory page on https://jenkins.io/redirect/ just like AnonymousClassWarnings does. How to create one ?

This comment has been minimized.

This comment has been minimized.

@bitwiseman

bitwiseman Oct 10, 2018

Contributor

@ndeloof @oleg-nenashev
This redirect should go to somewhere in the jenkins.io/ site and move the information from the wiki to jenkins-infra/jenkins.io#1823.
Also can we make the URL more specific? redirect/hudson.util.Secrrect.toString or at least redirect/secrect.tostring.

This comment has been minimized.

@ndeloof

ndeloof Oct 10, 2018

Member

have a look at the wiki page, this one is not specific to toString

@@ -67,6 +67,6 @@ THE SOFTWARE.
value="${h.getPasswordValue(attrs.value ?: instance[attrs.field])}"
type="password"
checkMethod="post"
ATTRIBUTES="${attrs}" EXCEPT="field clazz" />
ATTRIBUTES="${attrs}" EXCEPT="field clazz value" />

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 8, 2018

Member

IIUC, it is not related to the pull request description. What is the objective here? Should it be tracked separately?

This comment has been minimized.

@ndeloof

ndeloof Oct 8, 2018

Member

it is. Without this change MorphTagLibrary will invoke ${value}.toString to set value attribute, which is immediately overriden based on value expression
(jelly tags ... 🤕 )

@oleg-nenashev oleg-nenashev self-requested a review Oct 8, 2018

@Wadeck

Wadeck approved these changes Oct 10, 2018

@oleg-nenashev

Typo in the link: SecreCt

The rest LGTM, happy to merge once redirect is merged

Add link to https://jenkins.io/redirect/hudson.util.Secret/
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

oups. Fixed

@ndeloof ndeloof merged commit 408e135 into jenkinsci:master Oct 11, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Oct 11, 2018

Has there been any effort to statically find such calls to understand how common this is, or are we running the risk of drowning admins in warnings they can do little about?

@ndeloof

This comment has been minimized.

Member

ndeloof commented Oct 11, 2018

I don't know any way to search for calls to an overridden method, especially when this call is itself an implicit call by String concatenation. If you have some suggestion, you're welcome to tell me.
Also, Secret.toString is deprecated for years, this warning approach is the more conservative I can imagine.

@daniel-beck

This comment has been minimized.

@ndeloof ndeloof deleted the ndeloof:secret-tostring branch Oct 11, 2018

@ndeloof

This comment has been minimized.

Member

ndeloof commented Oct 11, 2018

would be interesting to know how you catch those.
so, either those plugins haven't been maintained since 1.356, or maintainer don't look at this red mark within their IDE to tell them "hey, this method is deprecated, look at me please". In both case, this deserve a warning.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Oct 11, 2018

would be interesting to know how you catch those.

As I wrote,

I usually patch deprecated-usage-in-plugins to find something else. In this case,

void methodCalled(String className, String name, String desc) {
    final String method = DeprecatedApi.getMethodKey(className, name, desc);
    if (className.endsWith("Secret") && name.equals("toString")) {
        methods.add(method);
    }
}

https://github.com/jenkins-infra/deprecated-usage-in-plugins

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment