Skip to content

Conversation

@tvdijen
Copy link
Member

@tvdijen tvdijen commented Sep 2, 2024

Closes #2233

@tvdijen tvdijen requested review from monkeyiq and thijskh September 2, 2024 16:53
@codecov
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.92%. Comparing base (7470c4c) to head (441b72e).
Report is 1 commits behind head on simplesamlphp-2.3.

Additional details and impacted files
@@                   Coverage Diff                   @@
##             simplesamlphp-2.3    #2234      +/-   ##
=======================================================
+ Coverage                44.90%   44.92%   +0.01%     
- Complexity                3895     3898       +3     
=======================================================
  Files                      162      163       +1     
  Lines                    12982    12998      +16     
=======================================================
+ Hits                      5830     5839       +9     
- Misses                    7152     7159       +7     

@tvdijen
Copy link
Member Author

tvdijen commented Sep 2, 2024

afbeelding

@tvdijen
Copy link
Member Author

tvdijen commented Sep 2, 2024

Note to self: should fix the debug info box overflow

Copy link
Member

@thijskh thijskh left a comment

Choose a reason for hiding this comment

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

It makes it a lot friendlier, but I think the core issue remains and that is that OPTIONS and HEAD requests will still be logging exceptions and I doubt we want that. You get a lot of OPTIONS passing by from browsers etc that should just be silently responded to with a 405 but not generate a lot of logging. I think.

@tvdijen
Copy link
Member Author

tvdijen commented Sep 2, 2024

I'm not sure we can fix that entirely...
We can stop logging this specific error, but that kind of defeats the purpose of the debug.backtraces = true setting...

One possible solution could be to add a parameter to Error::show() to pass the desired log-level. This would allow us to upscale or downscale exceptions.. In this particular case we could downscale to debug-level to hide the tracebacks (but log-level debug would still show them)
This will still generate 1 line of logging telling us that an error report was generated.

@tvdijen tvdijen force-pushed the bugfix/method-not-allowed branch from 20cbae4 to ccf9cb8 Compare September 2, 2024 20:50
@tvdijen tvdijen requested a review from thijskh September 2, 2024 21:02
@thijskh
Copy link
Member

thijskh commented Sep 3, 2024

I would only do this in debug mode indeed. Because it's very common for browsers etc to send OPTIONS requests which is spec compliant to do and the server just needs to respond with 405. Generating higher loglevel entries for this common and normal behaviour obscures the actual exceptions.

@tvdijen
Copy link
Member Author

tvdijen commented Sep 3, 2024

So your OK with the changes I made last night?

@thijskh
Copy link
Member

thijskh commented Sep 3, 2024

The changes seem to be about the email form? Or am I misunderstanding it. The browsers sending legitimate OPTIONS requests will not be submitting the email form anyway...

@tvdijen
Copy link
Member Author

tvdijen commented Sep 4, 2024

The email form would still be generated and produce output to the logs.
I've added a boolean to be able to not show the form at all for this type of error (and therefore not produce the log output).

The OPTIONS request would still lead to:
"Error report generated with track-id xxxx"

The end-result now is that these requests do not log anything at all, unless debug-level is set.
A convenient side-effect of the change is that we can disable the email-form for specific kinds of errors.

@tvdijen tvdijen force-pushed the bugfix/method-not-allowed branch from 823302f to 441b72e Compare September 4, 2024 11:35
Copy link
Contributor

@monkeyiq monkeyiq left a comment

Choose a reason for hiding this comment

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

I see that in Error::show the old direct call to logError() is now moved to log($logLevel) which will dispatch the logBacktrace() call based on the level that is passed and the system settings.

Just a thinking out loud took a closer look comment :)

@tvdijen
Copy link
Member Author

tvdijen commented Sep 5, 2024

I see that in Error::show the old direct call to logError() is now moved to log($logLevel) which will dispatch the logBacktrace() call based on the level that is passed and the system settings.

Yes, that's the trick to downgrade exceptions. Any exception we want to downgrade to debug-level can be dealt with this way in ExceptionHandler

@tvdijen tvdijen merged commit d2f2be1 into simplesamlphp-2.3 Sep 5, 2024
@tvdijen tvdijen deleted the bugfix/method-not-allowed branch September 5, 2024 16:22
tvdijen added a commit that referenced this pull request Sep 5, 2024
* Throw user-friendly exception with the proper HTTP statuscode for 'method not found'

* Suppress traceback for MethodNotAllowed error, unless debug-logging is set

* Be able to suppress error reporting for specific types of errors
tvdijen added a commit that referenced this pull request Sep 5, 2024
* Throw user-friendly exception with the proper HTTP statuscode for 'method not found'

* Suppress traceback for MethodNotAllowed error, unless debug-logging is set

* Be able to suppress error reporting for specific types of errors
tvdijen added a commit that referenced this pull request Sep 23, 2024
* Throw user-friendly exception with the proper HTTP statuscode for 'method not found'

* Suppress traceback for MethodNotAllowed error, unless debug-logging is set

* Be able to suppress error reporting for specific types of errors
tvdijen added a commit that referenced this pull request Oct 7, 2024
* Throw user-friendly exception with the proper HTTP statuscode for 'method not found'

* Suppress traceback for MethodNotAllowed error, unless debug-logging is set

* Be able to suppress error reporting for specific types of errors
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants