feat(api): log rejected related/distinct field access as security events#41306
feat(api): log rejected related/distinct field access as security events#41306rusackas wants to merge 2 commits into
Conversation
When a user-supplied column_name fails the related/distinct allowlist check, the API incremented a statsd counter and returned 404 but emitted no structured log event, so rejected field-access attempts were absent from the security audit trail (no user identity, endpoint, or attempted value). Emit a sanitized security log event (user id, endpoint, attempted column) on these denials, alongside the existing statsd counter. The column name is sanitized to a single bounded token to prevent log injection. The allowlist control itself is unchanged. Extends the related-field failure integration test to assert the event is logged with the rejected column name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
api
Code Review Agent Run #6fff82Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged security issue is correct. The current sanitization logic only removes control characters and newlines, allowing attackers to inject spaces and punctuation like Here is the corrected implementation for # Sanitize the user-supplied column name to a safe token set
import re
sanitized_column = re.sub(r'[^a-zA-Z0-9_.-]', '', str(column_name))[:200]
logger.warning(
"Rejected disallowed field access: user_id=%s endpoint=%s.%s column=%s",
get_user_id(),
self.__class__.__name__,
func_name,
sanitized_column,
)There are no other review comments in this pull request. Would you like me to assist with any other part of the implementation? superset/views/base_api.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41306 +/- ##
==========================================
- Coverage 64.34% 64.34% -0.01%
==========================================
Files 2653 2653
Lines 144952 144957 +5
Branches 33433 33433
==========================================
Hits 93273 93273
- Misses 49995 49998 +3
- Partials 1684 1686 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
SUMMARY
When a user-supplied
column_namefails therelated/distinctallowlist check inBaseSupersetModelRestApi, the API incremented a statsd counter and returned a 404 but emitted no structured log event. As a result, rejected field-access attempts against these secondary checks were absent from the security audit trail — no caller identity, endpoint, or attempted value.This adds a sanitized security log event (user id, endpoint, attempted column) at both denial points, alongside the existing statsd counter. The attempted column name is sanitized to a single bounded token (printable, no newlines, length-capped) so it cannot inject log lines. The allowlist control itself is unchanged.
This addresses two related audit-trail gaps in
views/base_api.py: the secondary-authorization denial (therelated/distinct404) and the input-validation/allowlist-bypass attempt that produces it.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — audit logging.
TESTING INSTRUCTIONS
test_get_related_fail(run for each resource viaApiOwnersTestCaseMixin) is extended to assert alogger.warningsecurity event is emitted with the rejected column name when a disallowed related field is requested.Run e.g.:
pytest tests/integration_tests/charts/api_tests.py -k related_failADDITIONAL INFORMATION