-
Notifications
You must be signed in to change notification settings - Fork 2
feat(agent): give fix agents live Grafana telemetry via MCP #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2568,9 +2568,18 @@ struct ConfigUpdateRequest { | |
| } | ||
|
|
||
| /// Redact values of keys that look like secrets in raw TOML content. | ||
| /// | ||
| /// `headers` is in the list because MCP servers carry auth in header bundles whose | ||
| /// key names say nothing about their contents - e.g. a Cloudflare Access client | ||
| /// secret inside `GRAFANA_EXTRA_HEADERS`, which none of the other patterns match. | ||
| /// | ||
| /// The value alternatives are ordered longest-form first, and the basic-string arm | ||
| /// understands backslash escapes. A naive `"[^"]*"` stops at the first escaped quote | ||
| /// inside the string, replacing only the opening fragment and leaving the rest of the | ||
| /// secret in the response - exactly the shape a JSON header bundle has. | ||
| fn redact_secrets(content: &str) -> String { | ||
| let re = regex_lite::Regex::new( | ||
| r#"(?im)^(\s*(?:[a-z_]*(?:token|secret|password|api_key|auth)[a-z_]*)\s*=\s*)("[^"]*"|'[^']*'|[^\n]*)"#, | ||
| r#"(?im)^(\s*(?:[a-z_]*(?:token|secret|password|api_key|auth|headers)[a-z_]*)\s*=\s*)("""[\s\S]*?"""|'''[\s\S]*?'''|"(?:[^"\\]|\\.)*"|'[^']*'|[^\n]*)"#, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new How this was verified: Knowledge Base Used: Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/claudear-engine/src/api/routes.rs
Line: 2582
Comment:
**MCP header secrets leak**
The new `headers` alternative only redacts assignments whose key is itself named `headers`; it does not track entries inside an MCP headers table. A valid configuration can use `[agent.providers.claude.mcp.grafana.headers]` followed by a quoted key such as `"CF-Access-Client-Secret" = "real-secret"`. That key does not match this expression, so `GET /api/config` returns the credential unchanged. The new test covers the separate `GRAFANA_EXTRA_HEADERS` JSON-string shape and misses the typed `McpServerConfig.headers` representation.
**How this was verified:** `McpServerConfig.headers` is a TOML map, while this expression only matches standalone secret-like assignment keys and cannot associate arbitrary entries with a surrounding headers table.
**Knowledge Base Used:**
- [Service API and control plane](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/service-api.md)
- [Configuration, secrets, and deployment](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/claudear/-/docs/configuration-secrets-and-deployment.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| ) | ||
| .expect("valid regex"); | ||
| re.replace_all(content, r#"${1}"[REDACTED]""#).into_owned() | ||
|
|
@@ -5319,6 +5328,49 @@ mod tests { | |
| assert!(!truncated); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_redact_secrets_header_bundle() { | ||
| // An MCP server's header bundle carries a secret under a key name that matches | ||
| // none of the other patterns, and wraps it in a JSON blob full of escaped | ||
| // quotes. Both parts have to work or GET /api/config leaks it to the dashboard. | ||
| let content = r#"GRAFANA_EXTRA_HEADERS = "{\"CF-Access-Client-Secret\": \"real-secret\"}" | ||
| "#; | ||
| let redacted = redact_secrets(content); | ||
| assert!(!redacted.contains("real-secret"), "redacted: {}", redacted); | ||
| assert!(redacted.contains("[REDACTED]")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_redact_secrets_multiline_string() { | ||
| // TOML multi-line strings must be consumed whole; matching the ordinary | ||
| // basic-string arm first would stop at the opening delimiter. | ||
| let content = | ||
| "auth_token = \"\"\"\nline-one-secret\nline-two-secret\n\"\"\"\nport = 3100\n"; | ||
| let redacted = redact_secrets(content); | ||
| assert!( | ||
| !redacted.contains("line-one-secret"), | ||
| "redacted: {}", | ||
| redacted | ||
| ); | ||
| assert!( | ||
| !redacted.contains("line-two-secret"), | ||
| "redacted: {}", | ||
| redacted | ||
| ); | ||
| // Non-secret keys after the multi-line value survive untouched. | ||
| assert!(redacted.contains("port = 3100")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_redact_secrets_leaves_instructions_intact() { | ||
| // `instructions` on an MCP server is prompt guidance, not a credential; it | ||
| // must survive redaction so the config editor round-trips it unchanged. | ||
| let content = "instructions = \"Call list_datasources once to find UIDs.\"\n"; | ||
| let redacted = redact_secrets(content); | ||
| assert!(redacted.contains("Call list_datasources once to find UIDs.")); | ||
| assert!(!redacted.contains("[REDACTED]")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_redact_secrets_basic() { | ||
| let content = "api_token = \"sk-12345\"\nwebhook_secret = \"whsec_abc\"\nnormal_key = \"not_a_secret\"\n"; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assertions copy the fixture’s exact Grafana command, argument, and instruction wording instead of testing an observable configuration outcome. That makes harmless wording or executable changes require test edits while failures in the actual prompt and run behavior can still pass. The same problem appears in the new runner tests that directly assert private helper results, server-name formatting, and template syntax. This violates the repository directive not to mirror source code or configuration in assertions and to test observable behavior instead, so the requirement must be satisfied before merging. Replace this cluster with boundary-level prompt and MCP invocation tests.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!