feat(logging): add query parameter logging for /api/v2 requests - BED-8563 - #2895
Conversation
📝 WalkthroughWalkthrough
ChangesQuery Parameter Logging Exclusions
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/api/src/api/middleware/logging_test.go`:
- Line 28: The import statement in logging_test.go at line 28 references an
invalid module path `github.com/specterops/bloodhound/cmd/api/src/ctx` that does
not exist in the repository, causing compilation failures. Replace this import
statement with the correct request-context package path that is already used by
the middleware code in this repository. Additionally, update all references to
this context package in lines 105-109 to use the corrected import path so the
tests can compile successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: febec8aa-0b3e-483d-be6e-c7123e5de545
📒 Files selected for processing (2)
cmd/api/src/api/middleware/logging.gocmd/api/src/api/middleware/logging_test.go
cami-specter
added
api
superlinkx
left a comment
superlinkx
left a comment
There was a problem hiding this comment.
This shows a lot of attention to detail and will make logging far more valuable
| return pathSegments[0] == "api" && | ||
| pathSegments[1] == "v2" && | ||
| pathSegments[2] == "sso" && | ||
| pathSegments[4] == "callback" | ||
| } |
There was a problem hiding this comment.
Wanted to note that there are old but still maintained due to still in use saml endpoints that likely also should be ignored and those are "versionless"
Further if sso is a concern, there is the sso/{id}/metadata endpoint as well. Might be worth considering a way to flag these in the route layer to be skipped 🤔 vs maintaining them here
There was a problem hiding this comment.
Function has been modified to add a few additional paths, thanks for bringing up one of them! See https://specterops.atlassian.net/browse/BED-8722 for full list of considered endpoints.
v1 is already excluded where the helper is called, but added a preventive guard for a deprecated path anyway.
As for the architecture, that's an interesting suggestion that I looked into. It is more involved however, although we could consider that in the future.
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Blocked to verify intent
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
approving to remove block. discussed offline. will likely benefit from a longer audit into logging queries. thank you for your patience and waiting 🙇
c41382f to
6e40fbf
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/api/src/api/middleware/logging_test.go (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse descriptive fixture and context names.
Replace
testURL1andtestURL2with names that identify their routes. ReplacebhCtxwithrequestContext.As per coding guidelines, “Prefer descriptive variable names, such as
databaseInterface, instead of abbreviated names such asdiordbi.”Also applies to: 147-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/api/src/api/middleware/logging_test.go` around lines 34 - 35, Rename the test fixtures testURL1 and testURL2 to descriptive names identifying their bloodhound-users and search routes, respectively, and rename bhCtx to requestContext throughout the affected logging tests while preserving behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/api/src/api/middleware/logging_test.go`:
- Around line 34-35: Rename the test fixtures testURL1 and testURL2 to
descriptive names identifying their bloodhound-users and search routes,
respectively, and rename bhCtx to requestContext throughout the affected logging
tests while preserving behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a2c94618-8f27-4a5e-9fa5-36d1690fd825
📒 Files selected for processing (2)
cmd/api/src/api/middleware/logging.gocmd/api/src/api/middleware/logging_test.go
|
Full audit on logging queries is now complete: Adds two additional exclusions: the login/support already pointed out by TJ + a guard against a deprecated path (v2 acs as v1 is already excluded). See details in the audit. |
…uests - adds query parameters as structured slog attributes in LoggingMiddleware - only logs for /api/v2 endpoints with non-empty query strings - adds tests fixes: BED-8563
6e40fbf to
724e83e
Compare
3 participants
Description
Add query parameter logging for /api/v2 requests for easier Elastic/Kibana lookups. This logs the raw string and makes it easier to filter and search in Elastic without parsing the full request_uri field.
request_uri, it is probably better not to add them to the newquery_parametersfield.Note:
After some deliberation, decided not to add nested logging due to potential Elastic index mapping problems. (If we were to add a large list of parameters (in the hundreds) to our Elastic daily indexes mapping, that could cause issues – indexes are already > 1000).
Motivation and Context
Resolves BED-8563.
There's a general benefit to having query_parameters as a dedicated field since it makes it easier to filter and aggregate on query param usage across most endpoints.
Specifically, we were looking at non-default limit and skip parameters queries for the api/v2/search endpoint
How Has This Been Tested?
Screenshots (optional):
log output showing query_parameters field (/api/v2 request with a single parameter)
pretty print of the above log output (disabling text logging)
log output showing query_parameters field (/api/v2 request with multiple parameters)
Types of changes
Checklist:
Summary by CodeRabbit
Improvements
Tests