Conversation
WalkthroughAdds configurable ignore list for query parameters to access logs; wires the option through config, schema, fixtures, router core, and request-logger internals; and adds tests exercising ignored params for GET and POST GraphQL requests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
router/core/router.go (1)
164-169: Document the new field for clarityConsider adding a short comment describing matching semantics (e.g., exact/case-sensitive) and behavior when parsing fails, to align with requestlogger behavior.
type ( // Router is the main application instance. Router struct { @@ AccessLogsConfig struct { - Attributes []config.CustomAttribute - Logger *zap.Logger - SubgraphEnabled bool - SubgraphAttributes []config.CustomAttribute - IgnoreQueryParamsList []string + Attributes []config.CustomAttribute + Logger *zap.Logger + SubgraphEnabled bool + SubgraphAttributes []config.CustomAttribute + // IgnoreQueryParamsList lists query parameter names that must be excluded + // from the access log's "query" field. Matching is exact (case-sensitive). + // If query parsing fails, the logger clears the query to avoid leaking data. + IgnoreQueryParamsList []string }router/pkg/config/config.go (1)
894-896: Config surface looks good; consider deduping values post-loadThe field and tags look correct. Optionally, dedupe values after config load to avoid redundant work downstream.
You could add a post-processing step in LoadConfig (near other post-processing) like:
// After unmarshalling and existing post-processing: cfg.Config.AccessLogs.Router.IgnoreQueryParamsList = unique.SliceElements(cfg.Config.AccessLogs.Router.IgnoreQueryParamsList)router/pkg/config/config.schema.json (1)
695-704: Tighten schema and clarify semantics
- Add uniqueItems to avoid duplicates.
- Clarify that matching is exact/case-sensitive to set expectations.
- "ignore_query_params_list": { - "type": "array", - "description": "List of query params to be ignored from being logged in the query field.", - "items": { - "type": "string" - } - } + "ignore_query_params_list": { + "type": "array", + "description": "List of query parameter names to exclude from the logged 'query' field. Matching is exact (case-sensitive).", + "items": { + "type": "string" + }, + "uniqueItems": true + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
router-tests/structured_logging_test.go(2 hunks)router-tests/testenv/testenv.go(2 hunks)router/core/graph_server.go(1 hunks)router/core/router.go(1 hunks)router/core/supervisor_instance.go(1 hunks)router/internal/requestlogger/requestlogger.go(5 hunks)router/internal/requestlogger/subgraphlogger.go(2 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/fixtures/full.yaml(1 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)
👮 Files not reviewed due to content moderation or server errors (7)
- router-tests/testenv/testenv.go
- router/core/supervisor_instance.go
- router/internal/requestlogger/subgraphlogger.go
- router/pkg/config/testdata/config_defaults.json
- router/core/graph_server.go
- router-tests/structured_logging_test.go
- router/internal/requestlogger/requestlogger.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
router/core/router.go (1)
router/pkg/config/config.go (1)
CustomAttribute(43-47)
router/core/graph_server.go (1)
router/internal/requestlogger/requestlogger.go (1)
WithIgnoreQueryParamsList(113-117)
router-tests/structured_logging_test.go (2)
router-tests/testenv/testenv.go (4)
Run(104-121)Config(281-337)Environment(1722-1758)GraphQLRequest(1890-1898)router/core/operation_processor.go (1)
GraphQLRequest(181-186)
router/internal/requestlogger/subgraphlogger.go (1)
router/internal/expr/expr.go (1)
Request(65-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
router/pkg/config/fixtures/full.yaml (1)
87-88: LGTM: example config shows intended usageThe sample ignore_query_params_list under access_logs.router is correct and clear.
router/pkg/config/testdata/config_full.json (1)
418-421: LGTM: test fixture updated correctlyIgnoreQueryParamsList is added under AccessLogs.Router with a representative value.
…on-to-redact-variables-query-parameter-in-access-logs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/pkg/config/testdata/config_full.json (1)
418-422: Use realistic example values to aid future readers/tests.For a “full” config fixture, consider replacing "anothervalue" with "operationName" so examples match common GraphQL query params and align with tests/docs.
Apply this diff:
"IgnoreQueryParamsList": [ "variables", - "anothervalue" + "operationName" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
router/core/graph_server.go(1 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/fixtures/full.yaml(1 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- router/pkg/config/config.schema.json
- router/core/graph_server.go
- router/pkg/config/config.go
- router/pkg/config/fixtures/full.yaml
- router/pkg/config/testdata/config_defaults.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
router/pkg/config/testdata/config_full.json (1)
418-422: Field placement is correct here; AI summary is off.The new IgnoreQueryParamsList is added under AccessLogs.Router (not Headers.All). Placement and casing look consistent with surrounding keys in this JSON fixture.
Motivation
Right now we print the entire raw query params, however in certain cases, especially when sending queries over GET, sensitive values can be logged. As we currently log the raw
queryin the access logs by default. This adds an option to ignore certain query parameters. In the case there are skip list entries and we cannot parse the query params, we will reset the query to "" for logging.Summary by CodeRabbit
New Features
Tests
Checklist