fix(loki): scope namespace-lockdown check to the LogQL stream selector - #532
Conversation
validateNamespaceLockdown regex-matched the *entire* raw query string, so a
query like `{namespace="evil"} |= `namespace="prod"`` satisfied the lockdown
via the backtick line-filter text while the real stream selector targeted a
different namespace — a silent bypass of the multi-tenant namespace
isolation the lockdown exists to enforce. Now the check is scoped to the
leading `{...}` stream selector only, mirroring the argv-tokenization fix
already applied to kubectl/aws safety (#526).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FyBW9iLrDdjDxndKvAr8TD
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughNamespace lockdown now uses selector-aware LogQL parsing. It skips quoted and raw-string contents while extracting selectors, parses label matchers, validates exact positive namespace matches, and adds coverage for spoofed, malformed, negated, quoted, and valid selectors. ChangesNamespace Lockdown Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LokiQuery
participant NamespaceLockdown
participant SelectorParser
LokiQuery->>NamespaceLockdown: validate query and locked namespace
NamespaceLockdown->>SelectorParser: extract and parse stream selector
SelectorParser-->>NamespaceLockdown: return structured matchers
NamespaceLockdown-->>LokiQuery: allow or reject query
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves LogQL namespace lockdown validation by extracting and validating only the stream selector portion of the query, preventing bypasses via line filters. However, a critical security vulnerability was identified in the extractStreamSelector parser, which fails to track backtick-quoted raw string literals. This omission allows attackers to bypass the namespace lockdown by embedding double quotes and closing braces inside backticks. A code suggestion was provided to track both double quotes and backticks correctly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22c058bad6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…elector text Review from gemini-code-assist and codex both found real gaps in the prior fix: extractStreamSelector only tracked double quotes, so a backtick raw string (valid LogQL syntax) confused brace-depth counting — letting an attacker's decoy `namespace="prod"` text inside another matcher's backtick value spoof the lockdown regex (real bypass), while a legitimate backtick value containing a brace could get a valid query wrongly rejected (false-negative). Root cause was deeper than quote-tracking: even with correct selector boundaries, running a regex over the raw selector text is still spoofable by decoy text inside an unrelated matcher's unescaped backtick value. Replaced that regex check with a real parse of the selector into label/operator/value matchers (handling both quote styles), so only the actual "namespace" label's parsed value is compared — decoy text elsewhere can no longer masquerade as the real matcher, and malformed selectors fail closed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyBW9iLrDdjDxndKvAr8TD
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 076cc76921
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🧹 Nitpick comments (1)
src/lib/__tests__/loki.test.ts (1)
97-144: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a regression test for multi-selector queries.
The new cases thoroughly cover single-selector spoofing, quoting, negation, and malformed selectors. They don't cover a query with more than one stream selector (see the bypass flagged in
loki.ts). Recommend adding a case such assum(rate({namespace="prod"}[5m])) / sum(rate({namespace="evil"}[5m]))assertingfalsefor'prod', so the fix is locked in.🤖 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 `@src/lib/__tests__/loki.test.ts` around lines 97 - 144, The tests are missing coverage for queries containing multiple stream selectors. In the Loki validation test suite around validateNamespaceLockdown, add a regression case using a query such as sum(rate({namespace="prod"}[5m])) / sum(rate({namespace="evil"}[5m])) and assert it returns false for the locked namespace "prod".
🤖 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 `@src/lib/loki.ts`:
- Around line 147-154: Update validateNamespaceLockdown to extract and validate
every stream selector in the query rather than only the first one. Require each
selector to parse successfully and contain exactly a namespace matcher using '='
or '=~' with value lockedNamespace; return false for any missing, invalid, or
mismatched selector, and only return true when all selectors pass.
---
Nitpick comments:
In `@src/lib/__tests__/loki.test.ts`:
- Around line 97-144: The tests are missing coverage for queries containing
multiple stream selectors. In the Loki validation test suite around
validateNamespaceLockdown, add a regression case using a query such as
sum(rate({namespace="prod"}[5m])) / sum(rate({namespace="evil"}[5m])) and assert
it returns false for the locked namespace "prod".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a54b065e-0b2e-44a9-a61d-f1f7b3d82252
📒 Files selected for processing (2)
src/lib/__tests__/loki.test.tssrc/lib/loki.ts
…ockdown check
Two more review findings (gemini-code-assist/codex/coderabbit) on the same
namespace-lockdown check, both real:
- A decoy selector hidden in a leading `#`-comment (which Loki itself ignores
per the LogQL spec) was picked up by indexOf('{') and validated in place
of the real, executed selector.
- Only the *first* stream selector was checked. LogQL metric queries can
combine multiple selectors via binary operators (e.g.
`sum(rate({a}[5m])) / sum(rate({b}[5m]))`), so a second selector could
target any namespace while the whole query is forwarded to Loki unchanged.
Fixed by stripping quote-aware `#` comments before extraction, and by
extracting *every* top-level `{...}` selector and requiring all of them to
carry the locked namespace matcher, not just the first.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FyBW9iLrDdjDxndKvAr8TD
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e1efe241d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
codex flagged that the matcher-value decoder only understood escapes as "drop the backslash, keep the next char" — correct for \" and \\, but wrong for anything else. prod would naively decode to the literal text "u0070rod" instead of the "prod" Loki actually parses it as (a real LogQL unicode escape), so a namespace value written with an escape sequence could compare against the wrong string in either direction. We don't implement full Go/LogQL string-literal decoding (octal, \xHH, \uHHHH, \UHHHHHHHH, \n/\t/...); K8s namespace names are simple DNS-1123 labels that never need escaping in practice, so instead of guessing wrong, any escape other than \" or \\ now fails the whole selector parse closed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyBW9iLrDdjDxndKvAr8TD
2 participants
Summary
The Linear
Heimdallproject's Backlog/Todo/In Progress were all empty and the open-PR queue was empty too, so per the autonomous-shipping routine's fallback I audited the codebase for a genuine bug rather than another cosmetic refactor (the repo already merged a long streak of those — #521-#531).Problem
validateNamespaceLockdown(src/lib/loki.ts) is supposed to reject any LogQL query that doesn't target the operator-configuredlockedNamespace— a multi-tenant isolation control. It worked by running a regex fornamespace="<locked>"(ornamespace=~"<locked>") against the entire raw query string, not just the stream selector.LogQL supports backtick-delimited raw string literals in line filters (no quote-escaping needed), so a query can put the literal text
namespace="prod"in an unrelated line filter while the real stream selector targets a completely different namespace:With
lockedNamespace: "prod"configured, this query passedvalidateNamespaceLockdown(confirmed via a standalone repro before the fix) and was forwarded to Loki verbatim — Loki executes the real selector{namespace="evil"}, silently bypassing the lockdown. This is the same class of bug as PR #526 (kubectl/aws quoted-value bypass of the read-only policy), just in the Loki namespace-isolation control instead.I checked the other tools with namespace-lockdown-style logic (
kubecost.ts,helm.tsviaresolveNamespaceLockdownintool-config.ts) — those compare a structuredrequestedNamespacefield, not a free-text regex over a query string, so they aren't affected. Datadog/Jaeger/Prometheus don't implement a namespace lockdown at all. Loki is the only tool with this exposure.Changes
src/lib/loki.ts: addedextractStreamSelector, which locates the leading{...}brace group of a LogQL query (quote-aware, so braces inside quoted label values like{namespace=~"prod-[0-9]{3}"}don't break the boundary).validateNamespaceLockdownnow runs its regex against only that selector substring, and fails safe (false) if no selector is found at all.src/lib/__tests__/loki.test.ts: added regression tests — confirmed 2 of the 4 new cases fail on pre-fix code ({namespace="evil"} |= \namespace="prod"`and a barenamespace="prod"` with no selector) and all pass with the fix, plus coverage for the brace-inside-quotes selector-boundary case.Validation
validateNamespaceLockdownreturnedtruefor the malicious query above).git stash/manual revert) and pass with the fix.npm run typecheck— clean.npm test— full suite: 108 files / 3049 tests passing.Not auto-verified: no live Loki/Grafana Tempo server was used — this is a pure query-string-parsing fix validated at the unit level, consistent with this repo's testing policy (mock HTTP clients, never spawn real external services in tests).
Generated by Claude Code
Summary by CodeRabbit