@mwithheld opened this Pull Request on July 25th 2021 Contributor

Issue #17738 Don't log an error when not supported browser is used

Description:

Issue #17738 Don't log an error when not supported browser is used

To reproduce the error (before the changes in this ticket):

  • Monitor the server (e.g. Apache) error log, e.g. tail -f error_log
  • Do a GET on matomo with an unsupported browser version e.g. Firefox < version 32, e.g.
    wget -U "Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0" --no-check-certificate https://127.0.0.1/matomo/

To test the fix:

  • Repeat the steps to reproduce above - you should not see the message in the error log.
  • In config.ini.php, set loglevel=DEBUG (the default is WARN):
    [log]
    log_level = DEBUG
  • Repeat the steps to reproduce above - you should now see the message in the error log.

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@mwithheld commented on July 25th 2021 Contributor

It might be better to instead change core/SupportedBrowser.php to not throw an exception but instead to set a 403 HTTP header, output the message, and exit within that file.

@sgiehl commented on August 4th 2021 Member

@mwithheld to change the response code for this exception we could adjust the NotSupportedBrowserException to extend HttpCodeException

@mwithheld commented on August 4th 2021 Contributor

@sgiehl While true and a good idea, I'm proposing that this is not an Exception and so should not be handled like one -- it's normal program flow that should just [set a 403 HTTP header, output the message, and exit] (there's no code for this in core/SupportedBrowser.php). It's more a design decision if you want to use the exception handling anyway - and that'd be fine IMO -- I just don't feel it's my choice to make.

@sgiehl commented on August 5th 2021 Member

Guess using an exception makes it easy to automatically have the error screen rendered. Otherwise this would need to be triggered manually. But I agree that it's useless to log that exception. Personally I would keep the exception and maybe extend our exception with an additional flag that makes it possible to make the exception as "do not log". But I'm also fine with any other solution. Maybe @tsteur or @diosmosis have a stronger opinion on that?

@tsteur commented on August 5th 2021 Member

While it's only one or two exceptions and it be good to do it like in the PR for simplicity 👍 . And when it's needed later down the road then we can still go for the more complex solution of having a flag or so.

@diosmosis commented on August 5th 2021 Member

Random exits in the code makes tests unpredictable (eg, I just spent 2-3 days debugging a travis only test failure that was happening because our redirect logic just exits, and one test was triggering it but only on travis) and makes it much harder to debug user issues where the exit is triggered especially when we don't have access to the users install and/or they are non-technical while throwing an exception means we can get a stack trace. It would make sense to me to use HttpCodeException and in ExceptionHandler ignore those or log them at a debug level.

@github-actions[bot] commented on August 17th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@sgiehl commented on August 17th 2021 Member

@mwithheld might it be possible that you pushed the latest changes to the wrong branch? Seems they are not included in this PR

@mwithheld commented on August 24th 2021 Contributor

Those commits are in patch-4 but not coming through in the PR for some reason. I've instead created PR #17915

@mwithheld commented on September 8th 2021 Contributor

Please see #17915 awaiting review.

@sgiehl commented on September 8th 2021 Member

Seems both PRs meanwhile have the same changes. Will close this on again in favor of #17915

This Pull Request was closed on September 8th 2021
Powered by GitHub Issue Mirror