gabotorresruiz commented on code in PR #44666:
URL: https://github.com/apache/superset/pull/44666#discussion_r4187343280
##########
superset/views/error_handling.py:
##########
@@ -248,7 +248,15 @@ def refresh_csrf_token(ex: CSRFError) -> FlaskResponse:
@app.errorhandler(HTTPException)
def show_http_exception(ex: HTTPException) -> FlaskResponse:
- logger.warning("HTTPException", exc_info=True)
+ status = ex.code or 500
+ if status == 404:
+ # Unmatched URLs are a client condition, and scanner traffic makes
+ # them frequent; a traceback here only adds noise.
+ logger.debug("HTTPException: 404 %r", request.path)
Review Comment:
This block worries me a bit, because the 404 branch is a little wider than
the case the PR describes.
The rationale is about unmatched URLs, but `show_http_exception` also sees
every 404 raised from inside a matched view, and some of those are not the
client asking for a URL that does not exist. The one I care about is
`superset/views/core.py:604-608`:
```python
try:
dashboard.raise_for_access()
except SupersetSecurityException:
if not get_current_user():
return redirect_to_login()
abort(404)
```
That is an authorization denial deliberately masked as a 404. I verified on
this branch that it now produces a single DEBUG record, and since `LOG_LEVEL`
defaults to `INFO` (`superset/config.py:1737`) nothing lands in the application
log at all. On master the same request logged a WARNING whose traceback named
`core.py` and that `abort(404)` line, which is how you could tell a masked
denial apart from a scanner hit.
I also checked that nothing else inside Superset picks it up:
`raise_for_access` does not log the denial itself (its only `logger.info` is
the `EXTRA_RAISE_FOR_ACCESS_BYPASS` granted path), and
`event_logger.log_this_with_extra_payload` writes its row only after the
wrapped view returns, so an aborted request leaves no event log row either. I
confirmed that with a spy event logger: a normal request recorded its action, a
request whose view called `abort(404)` recorded nothing. The WSGI access log
still shows a `404` for the path, so it is not invisible everywhere, just
inside Superset's own logs.
`request.url_rule` separates the two cases cleanly, it is `None` for an
unmatched URL and the matched `Rule` for an in-view `abort(404)` (I verified
both). So the noisy case can stay at DEBUG without taking the rest with it:
```python
if status == 404 and request.url_rule is None:
logger.debug("HTTPException: 404 %r", request.path)
elif status < 500:
logger.warning("HTTPException: %r", str(ex))
else:
logger.warning("HTTPException", exc_info=True)
```
If you go that way, a `test_in_view_404_logs_a_warning` next to the existing
`test_view_404_logs_one_debug_line_with_path_and_no_traceback` would lock it
in. Right now both 404 tests assert DEBUG, which is exactly why nothing in the
suite notices the difference between the two entry points.
Or am I misreading the intent here, and routing in-view 404s to DEBUG along
with the scanner traffic is deliberate?
##########
superset/views/error_handling.py:
##########
@@ -248,7 +248,15 @@ def refresh_csrf_token(ex: CSRFError) -> FlaskResponse:
@app.errorhandler(HTTPException)
def show_http_exception(ex: HTTPException) -> FlaskResponse:
- logger.warning("HTTPException", exc_info=True)
+ status = ex.code or 500
+ if status == 404:
+ # Unmatched URLs are a client condition, and scanner traffic makes
+ # them frequent; a traceback here only adds noise.
+ logger.debug("HTTPException: 404 %r", request.path)
+ elif status < 500:
+ logger.warning("HTTPException: %r", str(ex))
Review Comment:
Small symmetry nit, not a blocker: the 404 branch gained `request.path` but
this one did not, so a 400/401/403/405 record now carries neither the route nor
a traceback. When the raiser passes no description the message is a werkzeug
constant, so for example the two different `abort(403)` in
`superset/embedded/view.py` (referrer allowlist at line 71, `Sec-Fetch-Dest` at
line 85) now emit byte identical lines. Adding `request.path` here too would
keep it correlatable for roughly free.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
