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.

Sign up for GitHub

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

Jump to bottom

Don't expose exception error attribute by default #8971

Closed
wants to merge 1 commit into from
Closed

Don't expose exception error attribute by default #8971

wants to merge 1 commit into from

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Apr 21, 2017

This resolves #7872 by introducing server.error.include-exception configuration property which defaults to false.

I've also changed DefaultErrorAttributes to use RequestDispatcher constants instead of String literals for standard attribute names.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 21, 2017
@snicoll snicoll mentioned this pull request Apr 22, 2017
Closed
snicoll
snicoll reviewed Apr 22, 2017
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Looks good @vpavic ! I've added a few minor comments.

...in/java/org/springframework/boot/autoconfigure/web/servlet/error/DefaultErrorAttributes.java
@@ -64,6 +66,12 @@
private static final String ERROR_ATTRIBUTE = DefaultErrorAttributes.class.getName()
+ ".ERROR";

private boolean includeException;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a constructor instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'd also prefer that. Just went with more passive approach initially, will update this.

...in/java/org/springframework/boot/autoconfigure/web/servlet/error/DefaultErrorAttributes.java Outdated
@@ -94,7 +102,7 @@ private void storeErrorAttributes(HttpServletRequest request, Exception ex) {
private void addStatus(Map<String, Object> errorAttributes,
RequestAttributes requestAttributes) {
Integer status = getAttribute(requestAttributes,
"javax.servlet.error.status_code");
RequestDispatcher.ERROR_STATUS_CODE);
Copy link
Member

Choose a reason for hiding this comment

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

That looks like an unrelated change to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I was referring to in the opening comment:

I've also changed DefaultErrorAttributes to use RequestDispatcher constants instead of String literals for standard attribute names.

Should I open a separate PR to address this?

Copy link
Member

@wilkinsona wilkinsona Apr 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We have a few places where we've avoided referencing RequestDispatcher directly to help with Servlet 2.5 compatibility. My preference would be to take care of all of them in a single, separate change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - at which branch should I target that change?

Copy link
Member

Choose a reason for hiding this comment

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

Against master, please. But only if you're feeling particularly generous; it's not the most exciting of changes :)

...va/org/springframework/boot/autoconfigure/web/servlet/error/DefaultErrorAttributesTests.java Outdated
@@ -180,9 +177,21 @@ private void testBindingResult(BindingResult bindingResult, Exception ex) {
}

@Test
public void exception() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps that method name could be a bit more explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

@vpavic
Copy link
Contributor Author

vpavic commented Apr 24, 2017

I've updated the PR to address the comments.

@snicoll snicoll added priority: normal type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 24, 2017
@snicoll snicoll added this to the 2.0.0.M1 milestone Apr 24, 2017
@snicoll snicoll self-assigned this Apr 24, 2017
snicoll pushed a commit that referenced this pull request Apr 27, 2017
@vpavic @snicoll
Do not expose exception error attribute by default
afe0c6f 
See gh-8971
@snicoll snicoll closed this in e9abe3f Apr 27, 2017
snicoll added a commit that referenced this pull request Apr 27, 2017
@snicoll
Merge pull request #8971 from vpavic:gh-7872
7b0645f 
* pr/8971:
  Polish "Do not expose `exception` error attribute by default"
  Do not expose `exception` error attribute by default
@vpavic vpavic deleted the gh-7872 branch April 27, 2017 10:59
@snicoll
Copy link
Member

snicoll commented Apr 27, 2017

This has failed the build (
BasicErrorControllerIntegrationTests) and I don't understand why the CI build worked... Looking into it.

@wilkinsona wilkinsona mentioned this pull request Sep 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll left review comments

@wilkinsona wilkinsona wilkinsona left review comments

Assignees

@snicoll snicoll

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.0.0.M1
Development

Successfully merging this pull request may close these issues.

Consider not exposing exception error attribute by default
4 participants
@vpavic @snicoll @wilkinsona @spring-projects-issues