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

The error message of a ForbiddenException is not returned in body but body is empty #1206

Closed
5 tasks done
de-jcup opened this issue Apr 4, 2022 · 4 comments · Fixed by #1212
Closed
5 tasks done
Assignees
Labels
bug Something isn't working
Milestone

Comments

@de-jcup
Copy link
Member

de-jcup commented Apr 4, 2022

Situation

When a project is deactivated (via project access level) , a ForbiddenException is thrown and the caller gets a 403 Forbidden as expected, but the body is empty instead of the given exception error message.

Wanted

The message of the exception must be available inside the body / the body may not be empty

Analyze

There is something really wrong here. The exception handling is no longer working as expected (and designed for SecHub).

What happens in log files

After reading some logs we have the situation that the forbidden exception was logged correctly with

HTTP status code: 403,
JSON:

"status" : 403,

"error" : "Forbidden",

"message:" : "Project xyz does currently not allow write access.",

"details" : []

After a dedicated timestamp we got
HTTP status code: 403,
JSON:

so body is empty.

Which version does it correctly, which wrong?

After further log evaluation it became clear that

  • v0.28.0 did log correctly (so JSON filled)
  • newer releases do no longer log correctly (body empty)

Which changes could be relevant?

  • with v0.29.0-server we switched from spring boot 2.5.8 to 2.6.2
  • there were no relevant issues for logging inside v0.29.0-server or v0.29.1-server

So it seems to be the spring boot update.

Solution

  • analyze and find the underlying problem/reason
  • extend test api to be check not only for http status numbers but also for expected JSON and/or string content
  • adopt 2 existing integration tests with new test api parts and check it's failing currently
  • adopt all other http status tests with new api and check JSON content
  • fix the problem to have wanted behaviour again
@de-jcup de-jcup added the bug Something isn't working label Apr 4, 2022
@de-jcup de-jcup added this to the Server 0.31.0 milestone Apr 4, 2022
@de-jcup de-jcup self-assigned this Apr 4, 2022
de-jcup added a commit that referenced this issue Apr 5, 2022
- TestAPi contains now a simple check for http status failures
  containing a text
- TestAPI contains now also a possibility to check
  for http status failures + expected JSON output
- Json check does ignore ordering and formatting but
  relies only on content
- use new TestAPI methods in some integration tests now
- problem shows up now (build will break here)
de-jcup added a commit that referenced this issue Apr 6, 2022
- Improved and simplified TestAPI
- Improved status code exception validator
@de-jcup
Copy link
Member Author

de-jcup commented Apr 6, 2022

With latest commit every http status check by TestAPI does automatically check per default

  • status code
  • json content contains "status", "error", "message", "timestamp"
  • field "error" is the http status code from spring boot HTTPStatus
  • field "status" is always the status code from exception (means http status code of returned result)

There are some special additional JSON specific checks available as well.

@de-jcup
Copy link
Member Author

de-jcup commented Apr 6, 2022

Next step

Identify exactly which spring boot version is the reason for this behaviour.

Now I checked develop branch inside another IDE instance and only changed spring boot versions, do a fresh build and run PDS and SecHub server and run from the other IDE the integration tests.

  • 2.5.8 - no failures
  • 2.5.9 - no failures
  • 2.6.0 - failed with the new failures from our tests

@de-jcup
Copy link
Member Author

de-jcup commented Apr 6, 2022

At the official Spring Boot release notes at https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.6-Release-Notes I did not find any information about the new behavior.

After doing some resarch, I found some github issues explaining that the /error page security protection has changed with 2.6.x and that the error page can only be accessed when authenticated.

Reading spring-projects/spring-boot#29655 it became also clear, that the information about authentication in a stateless http session is not available for spring security at this point of inspection.

And this is the reason why we have currently the correct HTTP status but no JSON output on (any) errors - even when correct authenticated.

@de-jcup
Copy link
Member Author

de-jcup commented Apr 7, 2022

Inside spring-projects/spring-security#10918 they mentioned
RequestAttributeSecurityContextRepository to hold/keep the authentication data inside STATLESS requests.
The mentioned changes should make authenticated() to work again, so only authenticated users could see the error page.

The issue was closed for spring security 5.7.0-M3.
But currently with Spring Boot 2.6.6 an older spring security library is sused where the authentication data get lost.

After checking the implementation and thinking about the impact and the processes, we decided, that it is good way to always permit here the /error page even when somebody has not been logged in.

Reason by an example:
When we temporary forbid a API call which is available even anonymous (e.g. a user signup) we want to inform the anonymous user as well that the signup was not possible, because the signup is disabled temporarily and the user shall try it again in n minutes...

When we would not use permitAll() we could not provide this information.
So even when spring-projects/spring-boot#29655 is available, we will still keep the permitAll() approach for error pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant