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

Keep exceptions http response generic #3384

Merged
merged 1 commit into from Nov 5, 2021

Conversation

juliushaertl
Copy link
Member

There is no need to return the exception details through the API as it is logged properly.

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

👍

@stefan-niedermann
Copy link
Member

Actually we have use cases where some information of the stacktraces are useful information.

Example: Uploading attachments which are too big: stefan-niedermann/nextcloud-deck#830

The maximum filesize is part of the stacktrace.

@stefan-niedermann
Copy link
Member

What is the benefit of less information at all? This is only useful for proprietary and closed source server applications which are doing security by obscurity.

Less transparency when trying to figure out what went wrong is not very helpful. Many current issues with the API would have been even harder to find and analyze without stacktraces in the responses, i therefore hardly encourage you to keep the valuable information in case something went wrong.

Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

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

Both ways are okay for me, either we default to false here to we merge it as-is. (though that may make debugging harder as others said)

Context: This error message can leak sensitive details and should not be returned by default.

@juliushaertl
Copy link
Member Author

Then let's keep it in debug mode 👍

@juliushaertl juliushaertl force-pushed the techdept/noid/cleanup-middleware branch from 5103359 to 2f610a2 Compare October 25, 2021 11:46
@stefan-niedermann
Copy link
Member

Can you name an example for "sensitive information"? I strongly doubt that one logs passwords or stuff, which would be even worse and should be fixed in the root, not just cloaking them in an API. Maybe some kind of compromise could be to only return the Internal server error message for not-authenticated requests or so?

@LukasReschke
Copy link
Member

LukasReschke commented Oct 25, 2021

Can you name an example for "sensitive information"? I strongly doubt that one logs passwords or stuff, which would be even worse and should be fixed in the root, not just cloaking them in an API.

For example file paths, folder structures, etc. - Returning raw error messages isn't the right thing to do and most software won't do that for a reason. (and rather return generic error codes that you can then correlate with the error logs)

Alternatively you could use HintException or such and only display these. But that would require changing the exception throwing sites to throw these exceptions instead and make sure to not include any stack trace here. (some of these exceptions that bubble up here are from core)

@stefan-niedermann
Copy link
Member

I agree that proper hints may be a alternative solution, but i doubt the order of the implementation. This should be done first and then remove any trace that might lead to a solution.

Taking my example from above this should of course not be a 500 at all, but stripping each information we will be hopelessly lost when an error occurs. Debug mode is not a proper replacement as many users are not selfhosting and can't just ask their provider to switch on the debug mode.

If one would prioritize clear hints higher than removing all information i wouldn't even have an issue, but in fact by far most of the users will complain at the Android client if an issue occurs.

I have a feeling that we are some kind of first level support / first line of defense and i feel a bit lost if we only are able to comment "sorry, but no idea - ask your million users hoster to enable debug mode for you 🤷".

@stefan-niedermann
Copy link
Member

Not that i want to nag, but we just have yet another perfect example why this is problematic for us. A HTTP 500 would have told us nothing. This way we can at least locate the issue in a certain way...

@juliushaertl juliushaertl force-pushed the techdept/noid/cleanup-middleware branch from 2f610a2 to b032002 Compare November 5, 2021 16:28
@juliushaertl
Copy link
Member Author

juliushaertl commented Nov 5, 2021

I agree that proper hints may be a alternative solution, but i doubt the order of the implementation. This should be done first and then remove any trace that might lead to a solution.

As Lukas mentioned the trace may contain path and folder structures but depending on the server configuration and PHP version may also contain arguments of method calls.

If one would prioritize clear hints higher than removing all information i wouldn't even have an issue, but in fact by far most of the users will complain at the Android client if an issue occurs.

Your point if of course understandable. Nextcloud offers a hint exception in order to provide user facing texts but that is so far not used in deck unfortunately. I'll open a follow up ticket on how we can improve exception handling in general as we indeed should rather catch them in the controller and return predictable API responses then. However that is unfortunately a larger restructuring task, so this is why this PR was the first take on the topic. I'll have a look if we can get some static analysis that we have running with psalm to get a list of uncatched exceptions that the controllers might throw so we can properly align that in a follow up.

I have a feeling that we are some kind of first level support / first line of defense and i feel a bit lost if we only are able to comment "sorry, but no idea - ask your million users hoster to enable debug mode for you shrug".

For now I've extended the message that will be returned in non-debug mode to always contain a hint to further request the server administrator for help in addition with the request ID which can then be used to get the actual log entry. This is the same behavior that we have in Nextcloud on pages that throw an exception.

…urther tracing

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl force-pushed the techdept/noid/cleanup-middleware branch from b032002 to 83fc432 Compare November 5, 2021 16:41
@stefan-niedermann
Copy link
Member

Thank you for your considerations, we'll see how it will work out in the field.

@juliushaertl juliushaertl merged commit 0ce431e into master Nov 5, 2021
23 checks passed
@juliushaertl juliushaertl deleted the techdept/noid/cleanup-middleware branch November 5, 2021 17:01
@juliushaertl
Copy link
Member Author

/backport to stable22

@juliushaertl
Copy link
Member Author

/backport to stable1.4

@juliushaertl
Copy link
Member Author

/backport to stable1.2

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

Successfully merging this pull request may close these issues.

None yet

4 participants