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-60410] Add stack trace suppression into core as a standard behavior. #4389

Merged
merged 76 commits into from
Mar 4, 2020

Conversation

jeffret-b
Copy link
Contributor

@jeffret-b jeffret-b commented Dec 6, 2019

See JENKINS-60410.

I propose to move the capability to suppress stack traces for non-administrator users into core and improve the behavior. Security has become a greater concern for many people in the past years, including for Jenkins administrators. Suppressing stack traces is industry standard behavior and we should align with that.

See jenkinsci/stapler#176 for related, dependent changes.
See also jenkinsci/jenkins-test-harness#201 for dependent changes to get the tests to pass.

Proposed changelog entries

  • Suppress error stack traces for non-administrator users as core capability. (issue 60410)

Submitter checklist

  • 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

@jeffret-b jeffret-b changed the title Add stack trace suppression into core as a standard. [JENKINS-60410] Add stack trace suppression into core as a standard behavior. Dec 9, 2019
@jeffret-b jeffret-b marked this pull request as ready for review December 9, 2019 19:09
Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

Code changes look reasonable. Nit: since these are new files, you can apply some automated code formatting and import optimization.

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.

Thanks for the pull request! I am -1 w.r.t such change if there is no opt-out. On my pet project instances I just have no user with ADMINISTER permission by default, because the instances are managed as-code. Also, it may hammer advanced users on instances and JIRA reporting to the Jenkins project.

IMHO it needs more reviews and feedback

@oleg-nenashev oleg-nenashev added needs-more-reviews Complex change, which would benefit from more eyes rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Dec 10, 2019
@oleg-nenashev oleg-nenashev requested a review from a team December 10, 2019 07:31
@jeffret-b
Copy link
Contributor Author

@oleg-nenashev , would you think a system property or a setting on the Configure Global Security page would be suitable here?

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.

Reproduction steps:

  • docker run --rm -it -p 8081:8080 -e ID=4389 jenkins/core-pr-tester
  • Install Matrix-auth
  • Configure a user with just read access
  • Log as that user
  • Browse to /configureSecurity
  • You got an error page

In this case the Exception is thrown by Jelly during the parsing of the page as there is an AccessDeniedException thrown (due to a missing permission) but then wrapped inside a ServletException. In the plugin it's only a check on the AccessDeniedException, but in your code you test if the exception contains the ACE, leading to still displaying some traces.

Second type of problem: http://[jenkins-url]/404, I got the full stack trace in a different format.

IOW we need to have tests covering the cases as it seems we do not cover them (or I miss something)

@oleg-nenashev
Copy link
Member

would you think a system property or a setting on the Configure Global Security page would be suitable here?

Until there is a standard engine to manage feature flags, a UI control is a preference for me

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.

Requesting changes to remove it from my review dashboard for now

@jeffret-b
Copy link
Contributor Author

I like the idea of the UI control. That makes sense. I'll discuss this more with Wadeck and others and try to get back to this next week.

Check isCommitted().
Guard against circular causes when checking for AccessDeniedException.
@jeffret-b
Copy link
Contributor Author

jeffret-b commented Jan 9, 2020

In this case the Exception is thrown by Jelly during the parsing of the page as there is an AccessDeniedException thrown (due to a missing permission) but then wrapped inside a ServletException. In the plugin it's only a check on the AccessDeniedException, but in your code you test if the exception contains the ACE, leading to still displaying some traces.

Good catch! I've got that working.

Second type of problem: http://[jenkins-url]/404, I got the full stack trace in a different format.

There isn't any requirement to have all errors presented in the same format. The requirement is to not show the stack trace. I don't see any problems with the way that page looks.

IOW we need to have tests covering the cases as it seems we do not cover them (or I miss something)

Yes, I need to see if it's possible to put together some tests.

@daniel-beck
Copy link
Member

daniel-beck commented Jan 10, 2020

Until there is a standard engine to manage feature flags, a UI control is a preference for me

I strongly disagree with this requirement, as it is a workaround for a very unusual setup but will add UI complexity for all users.

We also should not accept that experiencing unhandled errors currently resulting in stack traces to be part of the normal user experience; the more painful we make it to experience them, the more motivated we are to actually properly handle and prevent them.

To that end, I would even prefer to not show stack traces even to administrators, but only add a link to the Jenkins log on the error page to make it easy enough to figure out what went wrong exactly.

That said, a reasonable compromise could be to show the innermost exception message on the error screen. In many "normal" cases, that should give users a hint what went wrong without all the accompanying garbage information.

@jeffret-b
Copy link
Contributor Author

I agree with Daniel. It would be much better to just make stack suppression a standard behavior in Jenkins. While showing the stack can make it easier for some Jenkins developers, it really isn't good user experience. These days especially it kind of suggests a level of product immaturity.

Showing the trace to administrators is a compromise, but still not good UX for real admins. It's more a developer convenience for developers who typically run as admin.

The lesser compromise Daniel suggests of showing the innermost exception message could make sense if we restrict that additional content to admins. A lot of content can get mixed into that message that we don't want to regularly share. I'm not really sure there's enough benefit in these messages for the compromise. It's probably better to just promote the behavior of looking up things in the log.

@daniel-beck
Copy link
Member

Showing the trace to administrators is a compromise, but still not good UX for real admins. It's more a developer convenience for developers who typically run as admin.

This is a good point I thought about after posting my previous comment as well: It's not really for administrators either. It's for developers; specifically of Jenkins. Sure, we're all developers of Jenkins, as well as administrators, and happy if we can save a click or two before typing out a bug report for 10 minutes or sinking half an hour into debugging the error cause; but OTOH I don't really want to see this internal mess when I'm just a user. Give me the ability to file a useful bug report (if I can even be bothered to), but otherwise please don't expose this internal mess.

(I remember a vaguely similar argument around the masking of forms while they're loading from years ago, with the argument being that, as developers, it's useful to see at what UI element the Jelly view stopped rendering. Yet we make everyone look at the progressively built up form UI. Ugh.)

A system property could be supported, and set automatically when running hpi:run and jetty-dev:run to enable error stack traces for error messages, and that could be used in (I'm guessing fairly rare) configurations like the one Oleg mentioned too. But the default should be to hide it, and only provide admins a reference to the log so they can get there easily enough, with no new UI option clutter.

Co-Authored-By: Daniel Beck <1831569+daniel-beck@users.noreply.github.com>
@daniel-beck
Copy link
Member

Is it deliberate that the 'apply' error dialog doesn't show the stack trace even when the system property is set to show stack traces?

Not requesting this change, just asking.

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Still not a fan of the messaging on the screen, but that can be massaged later. This accomplishes the goal without any brokenness I was able to discover 👍

@jeffret-b
Copy link
Contributor Author

No intentional, but that popup is pretty weird. It's limited in what it understands. It doesn't show the rage image or the "Oops!" message. I couldn't get it to show the stack trace, at least not with the effort I've put into it so far. I didn't figure it was worth the effort to try and show the trace.

If you click the "Save" button rather than the "Apply" you get the regular oops page with the image and possibly the stack trace.

@jeffret-b
Copy link
Contributor Author

This needs JTH update and dependency update and then it should be ready to go.

@daniel-beck
Copy link
Member

that popup is pretty weird. It's limited in what it understands. It doesn't show the rage image or the "Oops!" message. I couldn't get it to show the stack trace, at least not with the effort I've put into it so far

All of that is done by

var error = doc.getElementById('error-description');
and
$(containerId).appendChild(error);

@jeffret-b
Copy link
Contributor Author

We could do more to make that look nicer and add the stack trace to it if we want. I doubt it's worth it.

@daniel-beck
Copy link
Member

more to make that look nicer

Besides the sizing challenge (larger with stack trace), all that's needed AFAICT is a new container div on the error page, and referencing that from apply.js. Not needed, but not a big challenge either. Like my other feedback, can be deferred.

@daniel-beck
Copy link
Member

@timja I think this is on-hold because of the incrementals dependency.

@jeffret-b
Copy link
Contributor Author

This one is on-hold because of the incrementals dependency to jenkins-test-harness. If we can get that one released, I can update this one and then this is ready.

test/pom.xml Outdated Show resolved Hide resolved
@jeffret-b
Copy link
Contributor Author

Thanks for the help with the JTH release, @timja! Once this builds with the released version, we should be able to remove the on-hold label.

@timja timja removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Feb 26, 2020
@jeffret-b
Copy link
Contributor Author

Looks this is all ready to go. Unless there are any objections we should go ahead and merge this.

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 squash-merge-me Unclean or useless commit history, should be merged only with squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants