fix(policy): fail closed on unresolvable row-filter claims - #457
fix(policy): fail closed on unresolvable row-filter claims#457taitelee wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughJWT row-filter templates now fail closed for unresolved claims across comparison operators and ChangesJWT filter resolution
Query alias validation
Stream payload filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant PolicyResolver
participant QueryBuilder
participant StreamProjection
Request->>PolicyResolver: evaluate JWT claim templates
PolicyResolver-->>QueryBuilder: resolved predicate or 1 = 0
QueryBuilder-->>Request: filtered query or validation error
Request->>StreamProjection: project stream payload
StreamProjection-->>Request: filtered event or dropped payload
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
github-actions
Bot
added
documentation
|
📚 Docs preview is live → https://d7f10e86-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall coverage in commit 74eb8c9 in the Show a code coverage summary of the most impacted files.
Updated |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 252af621-04dc-4466-8a78-4f1e72149e72
📒 Files selected for processing (4)
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy.gointernal/policy/policy_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Write tests in table-driven form witht.Run(tt.name, ...)for multiple cases.
Use shared mocks frominternal/testutil/instead of ad-hoc mocks in tests.
Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (testutil.MakeJWT,testutil.MakeExpiredJWT,NewTestSchemaRegistry,policy.NewMemoryStore,pipes.NewMemoryStore,AssertJSONResponse,AssertJSONContains) where applicable.
Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Files:
internal/policy/policy_test.go
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Documentation prose under the Starlight docs site must stay accurate against code, include runnable examples where relevant, and reflect code↔docs sync for changed behavior.
Files:
docs/src/content/docs/access-control.mdx
🧠 Learnings (20)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
📚 Learning: 2026-05-20T20:30:22.556Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/policy/**/*.{go} : Policy code must preserve fail-closed access control: `IsAdmin` is the single admin check, empty roles match nothing, `Validate` rejects empty role keys, and policy deletion denies everyone except the operator-key break-glass path.
Applied to files:
CHANGELOG.mdinternal/policy/policy.gointernal/policy/policy_test.godocs/src/content/docs/access-control.mdx
📚 Learning: 2026-07-08T12:46:29.364Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Applied to files:
CHANGELOG.mdinternal/policy/policy.gointernal/policy/policy_test.godocs/src/content/docs/access-control.mdx
📚 Learning: 2026-06-30T14:22:44.209Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 358
File: internal/policy/policy.go:289-305
Timestamp: 2026-06-30T14:22:44.209Z
Learning: In the Go policy/ingest path, `internal/policy/policy.go:resolveInValues` returns `[]any`, so `return nil` produces a typed nil slice. When that value is stored in `ResolvedPermissions.CheckClauses` and later type-asserted in `internal/api/ingest.go`, it still matches `[]any` and is handled as an `_in` membership check, preserving fail-closed behavior for absent claims. This is covered by `internal/api/ingest_test.go:TestIngest_Policy_CheckIn_AbsentClaim_FailsClosed`.
Applied to files:
CHANGELOG.mdinternal/policy/policy.gointernal/policy/policy_test.godocs/src/content/docs/access-control.mdx
📚 Learning: 2026-08-11T15:22:47.380Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: docs/src/content/docs/sdk/index.mdx:330-334
Timestamp: 2026-08-11T15:22:47.380Z
Learning: In WaveHouse Go server authentication, `internal/auth/auth.go` `bearerToken` returns from the `Authorization` header path before modifying `r.URL`. It removes the `token` query parameter only when authentication uses the query parameter without an `Authorization` header. Documentation must state that this protects WaveHouse's own logs only; reverse proxies, CDNs, load balancers, and other upstream intermediaries require query-string redaction.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/query/**/*.{go} : Structured-query code must enforce schema validation, permission checks, timestamp bucketing, and fail-closed column authorization inside `query.Build`.
Applied to files:
CHANGELOG.mdinternal/policy/policy.gointernal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/pipes/**/*.{go} : Named query pipes must remain fail-closed: per-pipe `allowed_roles` is the only execute-path gate, with admin-only behavior when no allowlist is present.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/auth/**/*.{go} : JWT auth middleware must always run, verify with either HMAC or JWKS (not both), pin accepted `alg` to the active verifier, and keep authN/authZ decoupled except for the sanctioned operator key.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-10T19:54:03.032Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: CHANGELOG.md:0-0
Timestamp: 2026-06-10T19:54:03.032Z
Learning: In the Wave-RF/WaveHouse repository, CHANGELOG.md entries under `[Unreleased]` use descriptive Keep-a-Changelog leads (e.g. "The structured-query column allowlist is now a hard cap…"), NOT the Conventional Commit PR title verbatim. Do not flag CHANGELOG entry leads for not matching the PR title — that is not a rule in this repo. There is no `.coderabbit.yaml`, and neither `AGENTS.md` nor `CONTRIBUTING.md` requires CHANGELOG leads to match PR titles.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/policy/policy.gointernal/policy/policy_test.go
📚 Learning: 2026-08-11T21:55:53.726Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/stream_test.go:159-176
Timestamp: 2026-08-11T21:55:53.726Z
Learning: For Go files in this repository, do not report direct type assertions solely because the `forcetypeassert` rule is commented out in `.golangci.yml`. Only flag a type assertion when there is an independent correctness, safety, or maintainability issue.
Applied to files:
internal/policy/policy.gointernal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (`testutil.MakeJWT`, `testutil.MakeExpiredJWT`, `NewTestSchemaRegistry`, `policy.NewMemoryStore`, `pipes.NewMemoryStore`, `AssertJSONResponse`, `AssertJSONContains`) where applicable.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-20T20:35:48.141Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:147-153
Timestamp: 2026-05-20T20:35:48.141Z
Learning: In WaveHouse internal/api pipes tests, when testing the non-forbidden (allowed) path via `safeHandle`, the response body is empty because `safeHandle` recovers the nil-Conn panic before any body is written. Use plain `assert.NotEqual(t, http.StatusForbidden, w.Code)` / `assert.NotEqual(t, http.StatusNotFound, w.Code)` rather than JSON-body helpers, which would fail on `json.Unmarshal` of an empty body.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-13T20:41:09.256Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/api/health_test.go:100-163
Timestamp: 2026-05-13T20:41:09.256Z
Learning: In the WaveHouse repository (`internal/testutil/testutil.go`), `testutil.AssertJSONResponse(t, rec, expectedStatus, expected any)` does full-body equality (`assert.Equal`) and `testutil.AssertJSONContains(t, rec, expectedStatus, expectedKeys map[string]any)` does per-key equality (`assert.Equal` per key). Neither helper supports substring/Contains checks. Passing a string to `AssertJSONContains` would not compile.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-20T01:02:03.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:03.228Z
Learning: In the WaveHouse project (`internal/api/**/*_test.go`), the convention for testing `RequireRole` middleware is to inject `ContextKeyRole` directly into the request context rather than using `testutil.MakeJWT`. JWT token parsing is covered separately in `middleware_test.go` (17 dedicated tests). Do not suggest switching role-gate tests to JWT-driven tests — the separation of concerns is intentional to keep failure surfaces isolated.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Write tests in table-driven form with `t.Run(tt.name, ...)` for multiple cases.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-13T20:40:56.906Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/discovery/discovery_test.go:404-513
Timestamp: 2026-05-13T20:40:56.906Z
Learning: In `internal/discovery/discovery_test.go`, the five `TestRetryRefresh_*` tests (SucceedsOnFirstAttempt, RetriesUntilSuccess, ReturnsOnContextCancel, BackoffIsBounded, NilOnAttemptIsSafe) are intentionally written as individual named tests rather than a table-driven suite. Their setup pipelines and assertion shapes are fundamentally heterogeneous: ReturnsOnContextCancel requires goroutine + channel + select-with-timeout orchestration, BackoffIsBounded uses wall-clock elapsed bounds, and NilOnAttemptIsSafe is a nil-callback panic-safety check. Forcing them into a table would produce mostly-null rows with nested `if` branches, which is worse readability. The table-driven pattern is correctly applied to `TestClampBackoff` in the same file (pure function, uniform I/O shape). Do not suggest converting these RetryRefresh tests to a table-driven suite.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/policy/policy_test.go
🪛 LanguageTool
docs/src/content/docs/access-control.mdx
[style] ~241-~241: Consider using a more formal/concise alternative here.
Context: ...njected as '', and any supplied value other than '' is rejected with `403 check failed...
(OTHER_THAN)
🔇 Additional comments (2)
internal/policy/policy.go (1)
205-210: LGTM!Also applies to: 231-248, 268-288, 314-320
docs/src/content/docs/access-control.mdx (1)
222-235: LGTM!Also applies to: 237-240
github-project-automation
Bot
moved this from In progress
to In review
in WaveHouse Task Board
…ejection; parallelize TestResolveTemplate subtests
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e8c0ba5-f1a3-4644-b604-5b359c7cce68
📒 Files selected for processing (3)
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Integration tests
- GitHub Check: Unit tests
- GitHub Check: Coverage
- GitHub Check: Docs build
- GitHub Check: E2E tests
🧰 Additional context used
📓 Path-based instructions (2)
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Documentation prose under the Starlight docs site must stay accurate against code, include runnable examples where relevant, and reflect code↔docs sync for changed behavior.
Files:
docs/src/content/docs/access-control.mdx
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Write tests in table-driven form witht.Run(tt.name, ...)for multiple cases.
Use shared mocks frominternal/testutil/instead of ad-hoc mocks in tests.
Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (testutil.MakeJWT,testutil.MakeExpiredJWT,NewTestSchemaRegistry,policy.NewMemoryStore,pipes.NewMemoryStore,AssertJSONResponse,AssertJSONContains) where applicable.
Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Files:
internal/policy/policy_test.go
🧠 Learnings (22)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/policy/**/*.{go} : Policy code must preserve fail-closed access control: `IsAdmin` is the single admin check, empty roles match nothing, `Validate` rejects empty role keys, and policy deletion denies everyone except the operator-key break-glass path.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy_test.go
📚 Learning: 2026-05-20T20:30:22.556Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdx
📚 Learning: 2026-06-30T14:22:44.209Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 358
File: internal/policy/policy.go:289-305
Timestamp: 2026-06-30T14:22:44.209Z
Learning: In the Go policy/ingest path, `internal/policy/policy.go:resolveInValues` returns `[]any`, so `return nil` produces a typed nil slice. When that value is stored in `ResolvedPermissions.CheckClauses` and later type-asserted in `internal/api/ingest.go`, it still matches `[]any` and is handled as an `_in` membership check, preserving fail-closed behavior for absent claims. This is covered by `internal/api/ingest_test.go:TestIngest_Policy_CheckIn_AbsentClaim_FailsClosed`.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy_test.go
📚 Learning: 2026-07-08T12:46:29.364Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/query/**/*.{go} : Structured-query code must enforce schema validation, permission checks, timestamp bucketing, and fail-closed column authorization inside `query.Build`.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-11T15:22:47.380Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: docs/src/content/docs/sdk/index.mdx:330-334
Timestamp: 2026-08-11T15:22:47.380Z
Learning: In WaveHouse Go server authentication, `internal/auth/auth.go` `bearerToken` returns from the `Authorization` header path before modifying `r.URL`. It removes the `token` query parameter only when authentication uses the query parameter without an `Authorization` header. Documentation must state that this protects WaveHouse's own logs only; reverse proxies, CDNs, load balancers, and other upstream intermediaries require query-string redaction.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/pipes/**/*.{go} : Named query pipes must remain fail-closed: per-pipe `allowed_roles` is the only execute-path gate, with admin-only behavior when no allowlist is present.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/auth/**/*.{go} : JWT auth middleware must always run, verify with either HMAC or JWKS (not both), pin accepted `alg` to the active verifier, and keep authN/authZ decoupled except for the sanctioned operator key.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (`testutil.MakeJWT`, `testutil.MakeExpiredJWT`, `NewTestSchemaRegistry`, `policy.NewMemoryStore`, `pipes.NewMemoryStore`, `AssertJSONResponse`, `AssertJSONContains`) where applicable.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Write tests in table-driven form with `t.Run(tt.name, ...)` for multiple cases.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-08-11T21:55:41.475Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/cmd/wavehouse-codegen/main_test.go:55-59
Timestamp: 2026-08-11T21:55:41.475Z
Learning: In the Go SDK tests, table-driven test loops do not require named `t.Run` subtests when the assertion error already identifies the failing input and expected and actual values. Do not raise a style-only finding to add `t.Run` in that case.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-13T20:40:56.906Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/discovery/discovery_test.go:404-513
Timestamp: 2026-05-13T20:40:56.906Z
Learning: In `internal/discovery/discovery_test.go`, the five `TestRetryRefresh_*` tests (SucceedsOnFirstAttempt, RetriesUntilSuccess, ReturnsOnContextCancel, BackoffIsBounded, NilOnAttemptIsSafe) are intentionally written as individual named tests rather than a table-driven suite. Their setup pipelines and assertion shapes are fundamentally heterogeneous: ReturnsOnContextCancel requires goroutine + channel + select-with-timeout orchestration, BackoffIsBounded uses wall-clock elapsed bounds, and NilOnAttemptIsSafe is a nil-callback panic-safety check. Forcing them into a table would produce mostly-null rows with nested `if` branches, which is worse readability. The table-driven pattern is correctly applied to `TestClampBackoff` in the same file (pure function, uniform I/O shape). Do not suggest converting these RetryRefresh tests to a table-driven suite.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Use shared mocks from `internal/testutil/` instead of ad-hoc mocks in tests.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-08-11T21:55:46.227Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/client_test.go:40-44
Timestamp: 2026-08-11T21:55:46.227Z
Learning: In `clients/go/client_test.go`, do not validate typed pointer fields by storing them in `map[string]any` and checking `ns == nil`. A nil typed pointer stored in an interface value is non-nil. Compare each concrete pointer field directly, such as `c.Sys == nil`, so constructor tests detect missing namespace assignments.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-20T01:02:03.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:03.228Z
Learning: In the WaveHouse project (`internal/api/**/*_test.go`), the convention for testing `RequireRole` middleware is to inject `ContextKeyRole` directly into the request context rather than using `testutil.MakeJWT`. JWT token parsing is covered separately in `middleware_test.go` (17 dedicated tests). Do not suggest switching role-gate tests to JWT-driven tests — the separation of concerns is intentional to keep failure surfaces isolated.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-20T20:35:48.141Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:147-153
Timestamp: 2026-05-20T20:35:48.141Z
Learning: In WaveHouse internal/api pipes tests, when testing the non-forbidden (allowed) path via `safeHandle`, the response body is empty because `safeHandle` recovers the nil-Conn panic before any body is written. Use plain `assert.NotEqual(t, http.StatusForbidden, w.Code)` / `assert.NotEqual(t, http.StatusNotFound, w.Code)` rather than JSON-body helpers, which would fail on `json.Unmarshal` of an empty body.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-08-11T21:55:53.726Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/stream_test.go:159-176
Timestamp: 2026-08-11T21:55:53.726Z
Learning: For Go files in this repository, do not report direct type assertions solely because the `forcetypeassert` rule is commented out in `.golangci.yml`. Only flag a type assertion when there is an independent correctness, safety, or maintainability issue.
Applied to files:
internal/policy/policy_test.go
🔇 Additional comments (4)
internal/policy/policy_test.go (2)
416-418: 🎯 Functional CorrectnessVerify the Go target before keeping these subtests parallel.
The subtest closure captures
ttand callst.Parallel(). If the module targets Go before 1.22, all subtests can observe the final loop value. Check the declared Go version. If it is below Go 1.22, shadowttinside the loop. If it is Go 1.22 or later, no code change is required.The supplied files do not include the repository's declared Go target.
Portable fix for targets below Go 1.22
for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) {
399-415: LGTM!Also applies to: 419-422, 654-681, 683-723, 725-743, 745-762
docs/src/content/docs/access-control.mdx (1)
222-222: LGTM!Also applies to: 235-237, 241-244
CHANGELOG.md (1)
40-40: LGTM!
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Ok, findings inline, and I tried to split a good chunk of things that I/Claude found here out to separate issues so that this one doesn't just keep growing in scope, but it definitely did a bit – this PR now closes #323 too (was already mostly done we just never noticed, just want to add a test for regression guard there).
Then the new issues I/claude opened from here:
- #460 — a typo'd operator key (
eq:for_eq:) produces no predicate at all andValidate()returns{"valid":true}. Also coversfilter:underinsert:being accepted and ignored. - #461 —
Store.load/Store.Watchcache without validating; a dependency of. #460, since every rule #460 adds won't apply to KV-loaded policies until it's fixed. - #463 — insert-
checksemantics for an unresolvable claim, which #371 doesn't cover. - #322 — commented, not filed: its "latent, identifiers are gated elsewhere" premise is wrong, and the impact is deletion of the predicate rather than misplacement. Asked for it off Backlog, since every fail-closed predicate we add widens its reachable surface.
- #323, #371 — status and cross-link comments.
Then the other big thing I want to sort out is the overlap between this PR and the other one you have in-flight, #381. They both touch a lot of the same files, and I've already noticed now a few things they both are trying to solve. Specifically: #381 resolves the filter once into predicates shared by both read paths, evaluated as SQL on query and in memory via RowVisible on the stream. If that in-memory evaluator doesn't implement this PR's fail-closed rule, a claim-less token gets no rows on /v1/query and every row on /v1/stream...
I would say let's try to land this PR first, since it's smaller and we are still going back and forth working on #381 too, then we NEED to make SURE that #381 rebases and adds an explicit assertion that an unresolvable claim yields no rows on BOTH paths.
| // or `col > ''` matches essentially every row, erasing the | ||
| // restriction (#385). Matching the _in branch below, a filter scoped | ||
| // to a claim the token doesn't carry matches no rows. | ||
| clauses = append(clauses, "1 = 0") |
There was a problem hiding this comment.
[MUST — blocker] This 1 = 0 is deletable by the caller, which turns the _eq case into a full-table read.
The fix itself is right. The problem is downstream: InjectPermissionFilters splices the predicate into already-rendered SQL by first-substring match (internal/query/builder.go:148-149), and an aggregation alias is validated only for ? (builder.go:237-241). So a caller-chosen alias containing " WHERE " swallows the splice:
POST /v1/query?table=clicks
{"columns":["user_id"],"group_by":["user_id"],
"aggregations":[{"fn":"any","column":"email","alias":"e WHERE z"}]}
SELECT `user_id`, any(`email`) AS `e WHERE (1 = 0) AND z` FROM `clicks` GROUP BY `user_id` LIMIT 10000No WHERE clause at all. I ran this against clickhouse local on a 3-tenant table: it returns every tenant's rows, while the correctly-formed query returns none.
Why this line specifically. 1 = 0 is the only WhereClause form the engine emits that contains no backtick — and therefore the only one that survives inside a backtick-quoted alias as valid SQL. Control cases:
claim present + evil alias -> AS `e WHERE (`tenant_id` = ?) AND z` -> SYNTAX_ERROR (500), no bypass
claim absent + evil alias -> valid SQL, filter gone -> full table
So the bypass is reachable precisely when a filter fails closed.
Scope, stated fairly. The splice flaw is pre-existing (#322) and was already reachable via _in since #358. This PR doesn't create the mechanism — it changes which policies land in the exploitable state:
| Operator | Missing claim, before | Missing claim, after |
|---|---|---|
_eq |
tenant_id = '' → empty-tenant rows only (bounded) |
0 rows normally, whole table with the alias trick |
_neq/_gt/_lt |
leaked ~everything with no trick at all | needs the alias trick |
Three of four operators get strictly better. _eq — the shape in every example in our own docs — gets worse in the worst case. That asymmetry is why I'd rather not ship this without a guard.
Suggested minimum here: reject an alias or ORDER BY alias-reference matching (?i)\s(where|group by|order by|limit)\s, right next to the existing chsql.BindUnsafe call in builder.go. ~5 lines, no product decision, no behavior change for any legitimate query. The structural fix (emit the predicate from Build rather than splicing text) stays #322's scope — I've corrected that issue's "latent, identifiers are gated elsewhere" premise, which is false, and asked for it to come off Backlog.
/v1/query is not admin-gated (internal/api/router.go:146-148), so any role with a select entry on the table can reach this. Column allow/deny still applies — it's a pure row bypass.
There was a problem hiding this comment.
For my next push, I'm adding the guard next to the BindUnsafe checks in builder.go. validateAndAuthorizeColumns rejects an alias or ORDER BY reference matching (?i)\s(where|group\s+by|order\s+by|limit)\s. Just building off regex in your comment, with \s+ so a GROUP BY can't sneak through. It fires in Build, before the splice, so the request errors instead of running unfiltered.
Agree the 1 = 0 being the one predicate with no backtick is the crux, which is why it's guarded on the alias and not the predicate. Got the ORDER BY reference too.
Doesn't touch the permissive alias behavior: TestBuild_AggregationAliasQuotedAndContained and TestIntegration_AliasInjectionContained still pass, since no clause keyword appears in any of them. Added TestBuild_RejectsSpliceKeywordAlias and ...OrderRef.
There was a problem hiding this comment.
The guard closes the vector I described but not its sibling, and the bypass is still live. Leaving this open.
InjectPermissionFilters matches " WHERE " with strings.Contains — byte-exact, and your guard fully covers that one. But when the query has no WHERE, the insert point comes from findInsertPoint, which uppercases via strings.ToUpper first. Go's (?i) uses simple case folding and does not fold ı (U+0131) to i; ToUpper does map it to I. So lımıt evades the regex and still reads as " LIMIT " to the splice:
alias "e limit z" -> rejected
alias "e lımıt z" -> accepted, and Build+Inject emits:
SELECT groupArray(`email`) AS `e WHERE 1 = 0 lımıt z` FROM `clicks` LIMIT 10000
Against a 3-tenant table on clickhouse local that returns ['a@acme','b@globex','c@initech']. The correct query returns []. groupArray is allowlisted, so it is arbitrary column exfiltration rather than a row peek.
A fuzz over 400k aliases found 227 evading variants, 209 of which executed with a 100% leak rate. Only two runes in all of Unicode uppercase to ASCII (ı→I, ſ→S) and only LIMIT contains an I, so it is a single-rune hole — but it is enough.
The guard is also over-inclusive in the other direction: it 400s "Total order by region", lowercase " where " (which the byte-exact splice never matched anyway), and keywords padded with tabs. Being wrong in both directions is the tell that it approximates the splice rather than matching it.
I would rather delete the mechanism than patch the regex again. Build already receives perms — the predicate can be appended to whereParts at builder.go:95-102, and InjectPermissionFilters, findInsertPoint and spliceKeywordRe all go away. Same refactor also fixes the max_rows cap silently no-opping (ApplyMaxRows has the same ToUpper-offset bug). I have updated #322 with the full scope and corrected its impact — it was filed as "misplaces the predicate, latent"; it deletes the predicate and is not latent.
Details on both offset bugs in a separate comment on builder.go.
| ok = false | ||
| return "" | ||
| } | ||
| return fmt.Sprint(val) |
There was a problem hiding this comment.
[Discussion — no action proposed] What counts as "resolved" is doing more work than it looks like.
Raising this as an open question rather than a change request, because I don't think there's an obviously-correct answer and I'd rather we decide it together than have me guess.
The mechanism. A JWT claim is JSON, so its value can be any JSON type. This function asks exactly one question — did I find anything at this path? — and if the answer is "yes", fmt.Sprint turns whatever it found into a string that gets bound as a database value. It never asks the second question: is this a sensible value to compare a column against?
Verified against the live code:
{{ jwt.app_metadata }} -> "map[tenant_id:acme]" ok=true
{{ jwt.tids }} -> "[a b]" ok=true
{{ jwt.ok }} -> "true" ok=true
{{ jwt.big }} -> "1.2345678901234567e+19" ok=true
Why it's reachable by accident. Given a token like {"app_metadata": {"tenant_id": "acme"}}, writing {{ jwt.app_metadata }} when you meant {{ jwt.app_metadata.tenant_id }} — dropping the last path segment — resolves successfully, to the object. With _neq that becomes tenant_id != 'map[tenant_id:acme]', which matches every row. The engine treated it as a success, so none of this PR's new fail-closed machinery engages.
A second, different problem hiding in the same place. JSON numbers decode to float64, which is exact only to 2^53. A 19-digit numeric tenant id becomes 1.2345678901234567e+19. That isn't a fail-open — it's a wrong value, and two tenants whose ids differ in the trailing digits can round to the same string, so _eq can match a different tenant's rows.
The open questions, as I see them:
- Object → almost certainly never a legitimate column value.
- Array → but arrays are legitimate here; that's the
_inmulti-tenant feature. So "reject non-scalars" isn't a clean rule. - Boolean →
{{ jwt.is_admin }}→"true"seems reasonable and someone may already rely on it. - Large number → the value is legitimate; only the representation is broken, which likely wants
json.Numberdecoding rather than a resolution failure.
That last bullet is what makes me think this isn't one decision — it looks like two fixes wearing one hat, with a type policy underneath them.
Prior art worth reacting to: #381 hit the column-side version of this ("fmt.Sprint loses the type a correct comparison needs") and landed on schema-informed comparison, ambiguous values fail closed. #371's background section makes the same observation from the operator side ("9" > "100" is true lexically, false numerically). This is the third face — the claim side, the input end — and it's the one nobody's looked at yet. Whether the #381 precedent should extend here is exactly what I'd like your read on.
Entirely fine by me if the answer is "out of scope for this PR, file it" — I just didn't want to file it for you and pre-commit to an answer.
There was a problem hiding this comment.
I think for now we can just create another issue. #381 extends only partway. An object should fail closed since it's never a real column value, but "reject non-scalars" isn't the rule, because arrays are the _in feature and a boolean is plausibly fine, so it lands as a per operator type check. The float64 case is separate: the value is legitimate and only fmt.Sprint mangles it, so it wants json.Number at decode, and it's arguably worse since it silently matches the wrong tenant instead of failing open.
There was a problem hiding this comment.
Note to self: Since we claim Hasura-like policy, let's look at how Hasura handles this
There was a problem hiding this comment.
Looked at Hasura. It backs your read on all four points.
Object should fail closed — no Hasura path binds an object to a scalar comparison; v2 casts the session variable through Postgres (::integer) and a bad cast errors the request, v3 JSON mode type-checks and errors. Arrays stay legitimate — both versions support array session variables for _in, v2 as a Postgres array literal ("{1,2,3}"), v3 as real JSON arrays. Boolean is fine to keep — v3 handles it as a first-class type. And the float64 case really is separate: Hasura hit this exact bug (legacy v3 parsed ints as i32, floats as f32) and fixed it by widening plus real JSON typing, not by rejecting the claim.
Worth knowing almost none of that is documented — it is from reading the engine.
Two things we are not taking. Hasura errors outright on a missing session variable and explicitly declined to add optional-session-variable support (their answer is "make your auth layer emit a default"). We are keeping 1 = 0, which is equivalent for security. And Hasura keys coercion on the column's declared type, which we cannot do — the policy layer has no schema access.
On scope, I think both halves belong in this PR rather than a follow-up.
Map/slice → ok=false. Two touch points, both in functions this PR already rewrote. resolveTemplate is the obvious one; the easy miss is resolveInValues' default: branch, because a bare-claim _in against an object goes through there and never touches resolveTemplate. The legitimate array path is unaffected — I checked:
_in "{{ jwt.tids }}" -> ["a","b"] handled before resolveTemplate
_in "t-{{ jwt.tids }}" -> ["t-[a b]"] via resolveTemplate
_in "{{ jwt.meta }}" -> ["map[t:acme]"] via resolveInValues default
Keep the rule narrow: map or slice → ok=false, strings/numbers/booleans bind as today.
jwt.WithJSONNumber() on the parser at internal/auth/auth.go:206. I flagged this on #381 and deferred it as outside that PR's footprint. It belongs here — this is the PR that owns claim resolution, and deferring it twice is how it never lands. I checked the blast radius: no float64 assertions on claims anywhere in auth/policy/api/stream; extractClaim asserts string only, so a numeric role claim returns "" before and after; golang-jwt's parseNumericDate handles json.Number explicitly so exp/nbf/iat validation is unaffected; and we already use UseNumber() in record_reader.go with json.Number type-switches in discovery. One line, and it fixes the query path, the stream and the check auto-inject together — which also means #381 rebases onto a fixed foundation instead of carrying a workaround.
Worth a test that takes a large-integer claim through a real jwt.Parse rather than a hand-built claims map — per my note on #381, a string-valued test claim passes while production does not.
No follow-up issue then; leaving this open until the code lands.
github-project-automation
Bot
moved this from Ready
to In review
in WaveHouse Task Board
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7620f40d-3d68-4e07-b562-76d335d5739c
📒 Files selected for processing (7)
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy.gointernal/policy/policy_test.gointernal/query/builder.gointernal/query/builder_test.gointernal/stream/hub_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Docs build
- GitHub Check: E2E tests
- GitHub Check: Coverage
- GitHub Check: Integration tests
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Write tests in table-driven form witht.Run(tt.name, ...)for multiple cases.
Use shared mocks frominternal/testutil/instead of ad-hoc mocks in tests.
Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (testutil.MakeJWT,testutil.MakeExpiredJWT,NewTestSchemaRegistry,policy.NewMemoryStore,pipes.NewMemoryStore,AssertJSONResponse,AssertJSONContains) where applicable.
Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Files:
internal/stream/hub_test.gointernal/query/builder_test.gointernal/policy/policy_test.go
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Documentation prose under the Starlight docs site must stay accurate against code, include runnable examples where relevant, and reflect code↔docs sync for changed behavior.
Files:
docs/src/content/docs/access-control.mdx
🧠 Learnings (28)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/stream/hub_test.gointernal/query/builder_test.gointernal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/stream/**/*.{go} : Streaming/SSE code must preserve the hub’s per-role projection model, subscriber queues, bucket fan-out, heartbeating, and metrics semantics.
Applied to files:
internal/stream/hub_test.go
📚 Learning: 2026-05-23T01:24:02.141Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:24:02.141Z
Learning: In WaveHouse tests under internal/api/**/*_test.go, use `testutil.AssertJSONErrorResponse(t, w)` (from `internal/testutil`) for HTTP error-path assertions — NOT a package-local `assertJSONErrorResponse` helper. The package-local helper was removed in PR `#174` and its functionality was promoted to `internal/testutil.AssertJSONErrorResponse`. This helper asserts `Content-Type: application/json`, `X-Content-Type-Options: nosniff` headers, and the presence of an `"error"` field in the JSON body.
Applied to files:
internal/stream/hub_test.go
📚 Learning: 2026-05-20T20:35:48.141Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:147-153
Timestamp: 2026-05-20T20:35:48.141Z
Learning: In WaveHouse internal/api pipes tests, when testing the non-forbidden (allowed) path via `safeHandle`, the response body is empty because `safeHandle` recovers the nil-Conn panic before any body is written. Use plain `assert.NotEqual(t, http.StatusForbidden, w.Code)` / `assert.NotEqual(t, http.StatusNotFound, w.Code)` rather than JSON-body helpers, which would fail on `json.Unmarshal` of an empty body.
Applied to files:
internal/stream/hub_test.gointernal/policy/policy.gointernal/policy/policy_test.godocs/src/content/docs/access-control.mdx
📚 Learning: 2026-05-13T20:41:09.256Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/api/health_test.go:100-163
Timestamp: 2026-05-13T20:41:09.256Z
Learning: In `internal/api/health_test.go` (WaveHouse), every handler test explicitly asserts `Content-Type: application/json` and `X-Content-Type-Options: nosniff` headers, including on 503 responses. This is deliberate regression coverage: the comment in `TestHealth_Readiness_PingFails` explains that without the 503-path header test, a future refactor moving header setup into the success branch would silently drop headers on error responses. New boot-degraded tests should follow the same pattern.
Applied to files:
internal/stream/hub_test.go
📚 Learning: 2026-05-20T01:02:03.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:03.228Z
Learning: In the WaveHouse project (`internal/api/**/*_test.go`), the convention for testing `RequireRole` middleware is to inject `ContextKeyRole` directly into the request context rather than using `testutil.MakeJWT`. JWT token parsing is covered separately in `middleware_test.go` (17 dedicated tests). Do not suggest switching role-gate tests to JWT-driven tests — the separation of concerns is intentional to keep failure surfaces isolated.
Applied to files:
internal/stream/hub_test.go
📚 Learning: 2026-05-13T20:40:56.906Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/discovery/discovery_test.go:404-513
Timestamp: 2026-05-13T20:40:56.906Z
Learning: In `internal/discovery/discovery_test.go`, the five `TestRetryRefresh_*` tests (SucceedsOnFirstAttempt, RetriesUntilSuccess, ReturnsOnContextCancel, BackoffIsBounded, NilOnAttemptIsSafe) are intentionally written as individual named tests rather than a table-driven suite. Their setup pipelines and assertion shapes are fundamentally heterogeneous: ReturnsOnContextCancel requires goroutine + channel + select-with-timeout orchestration, BackoffIsBounded uses wall-clock elapsed bounds, and NilOnAttemptIsSafe is a nil-callback panic-safety check. Forcing them into a table would produce mostly-null rows with nested `if` branches, which is worse readability. The table-driven pattern is correctly applied to `TestClampBackoff` in the same file (pure function, uniform I/O shape). Do not suggest converting these RetryRefresh tests to a table-driven suite.
Applied to files:
internal/stream/hub_test.gointernal/policy/policy.gointernal/policy/policy_test.go
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/stream/hub_test.gointernal/query/builder_test.gointernal/policy/policy.gointernal/policy/policy_test.gointernal/query/builder.go
📚 Learning: 2026-08-11T21:55:53.726Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/stream_test.go:159-176
Timestamp: 2026-08-11T21:55:53.726Z
Learning: For Go files in this repository, do not report direct type assertions solely because the `forcetypeassert` rule is commented out in `.golangci.yml`. Only flag a type assertion when there is an independent correctness, safety, or maintainability issue.
Applied to files:
internal/stream/hub_test.gointernal/query/builder_test.gointernal/policy/policy.gointernal/policy/policy_test.gointernal/query/builder.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/query/**/*.{go} : Structured-query code must enforce schema validation, permission checks, timestamp bucketing, and fail-closed column authorization inside `query.Build`.
Applied to files:
internal/query/builder_test.gointernal/policy/policy.gointernal/query/builder.goCHANGELOG.md
📚 Learning: 2026-06-30T14:22:44.209Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 358
File: internal/policy/policy.go:289-305
Timestamp: 2026-06-30T14:22:44.209Z
Learning: In the Go policy/ingest path, `internal/policy/policy.go:resolveInValues` returns `[]any`, so `return nil` produces a typed nil slice. When that value is stored in `ResolvedPermissions.CheckClauses` and later type-asserted in `internal/api/ingest.go`, it still matches `[]any` and is handled as an `_in` membership check, preserving fail-closed behavior for absent claims. This is covered by `internal/api/ingest_test.go:TestIngest_Policy_CheckIn_AbsentClaim_FailsClosed`.
Applied to files:
internal/policy/policy.gointernal/policy/policy_test.godocs/src/content/docs/access-control.mdxCHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/policy/**/*.{go} : Policy code must preserve fail-closed access control: `IsAdmin` is the single admin check, empty roles match nothing, `Validate` rejects empty role keys, and policy deletion denies everyone except the operator-key break-glass path.
Applied to files:
internal/policy/policy.godocs/src/content/docs/access-control.mdxCHANGELOG.md
📚 Learning: 2026-07-08T12:46:29.364Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Applied to files:
internal/policy/policy.gointernal/policy/policy_test.godocs/src/content/docs/access-control.mdxCHANGELOG.md
📚 Learning: 2026-05-20T20:30:22.556Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Applied to files:
internal/policy/policy.godocs/src/content/docs/access-control.mdxCHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/pipes/**/*.{go} : Named query pipes must remain fail-closed: per-pipe `allowed_roles` is the only execute-path gate, with admin-only behavior when no allowlist is present.
Applied to files:
internal/policy/policy.godocs/src/content/docs/access-control.mdxCHANGELOG.md
📚 Learning: 2026-08-11T21:56:03.206Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/query_builder.go:278-291
Timestamp: 2026-08-11T21:56:03.206Z
Learning: In `clients/go/query_builder.go`, `fetchNextTyped` intentionally treats a failed JSON decode of a non-object typed `Row` as normal end-of-pagination. This behavior matches the existing “cursor column was not in the projection” path and TypeScript SDK parity. The broader behavior change is tracked in GitHub issue `#452`.
Applied to files:
internal/policy/policy.goCHANGELOG.md
📚 Learning: 2026-05-25T11:24:16.432Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 182
File: internal/discovery/validation.go:107-123
Timestamp: 2026-05-25T11:24:16.432Z
Learning: In `internal/discovery/validation.go` (WaveHouse project, Go), the `isTypeCompatible` function is intentionally permissive: it accepts any string for Bool and numeric ClickHouse types (and similarly broad coercions for other types) because the design philosophy is to avoid false-negative rejections at the pre-validation layer. ClickHouse's own type coercion is more forgiving and will handle the final validation. Stricter lexical/value checks (e.g., `strconv.ParseFloat` for numerics, allowlisting "true"/"false" for bools) should NOT be suggested, as accepting incorrect types is preferred over rejecting values ClickHouse would accept.
Applied to files:
internal/policy/policy.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Applied to files:
internal/policy/policy.gointernal/policy/policy_test.go
📚 Learning: 2026-06-26T12:23:26.034Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:26.034Z
Learning: In this Go repository, the `**/*_test.go` table-driven test guideline is intended for genuinely multi-scenario tests. Single sequential behavioral-flow tests, such as `internal/stream/subscriber_test.go`'s `TestSubscriber_SendDeliversThenDropsWhenFull`, do not need to be rewritten into `[]struct{...}` + `t.Run(...)` when that would be artificial and less clear.
Applied to files:
internal/policy/policy.go
📚 Learning: 2026-08-11T21:55:41.475Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/cmd/wavehouse-codegen/main_test.go:55-59
Timestamp: 2026-08-11T21:55:41.475Z
Learning: In the Go SDK tests, table-driven test loops do not require named `t.Run` subtests when the assertion error already identifies the failing input and expected and actual values. Do not raise a style-only finding to add `t.Run` in that case.
Applied to files:
internal/policy/policy.gointernal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Write tests in table-driven form with `t.Run(tt.name, ...)` for multiple cases.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (`testutil.MakeJWT`, `testutil.MakeExpiredJWT`, `NewTestSchemaRegistry`, `policy.NewMemoryStore`, `pipes.NewMemoryStore`, `AssertJSONResponse`, `AssertJSONContains`) where applicable.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Use shared mocks from `internal/testutil/` instead of ad-hoc mocks in tests.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-08-11T21:55:46.227Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/client_test.go:40-44
Timestamp: 2026-08-11T21:55:46.227Z
Learning: In `clients/go/client_test.go`, do not validate typed pointer fields by storing them in `map[string]any` and checking `ns == nil`. A nil typed pointer stored in an interface value is non-nil. Compare each concrete pointer field directly, such as `c.Sys == nil`, so constructor tests detect missing namespace assignments.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/auth/**/*.{go} : JWT auth middleware must always run, verify with either HMAC or JWKS (not both), pin accepted `alg` to the active verifier, and keep authN/authZ decoupled except for the sanctioned operator key.
Applied to files:
docs/src/content/docs/access-control.mdxCHANGELOG.md
📚 Learning: 2026-08-11T15:22:47.380Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: docs/src/content/docs/sdk/index.mdx:330-334
Timestamp: 2026-08-11T15:22:47.380Z
Learning: In WaveHouse Go server authentication, `internal/auth/auth.go` `bearerToken` returns from the `Authorization` header path before modifying `r.URL`. It removes the `token` query parameter only when authentication uses the query parameter without an `Authorization` header. Documentation must state that this protects WaveHouse's own logs only; reverse proxies, CDNs, load balancers, and other upstream intermediaries require query-string redaction.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
CHANGELOG.md
🔇 Additional comments (6)
CHANGELOG.md (1)
40-40: Use issue#463for unresolved_eqcheck behavior.Issue
#371tracks supportedcheckoperators. Issue#463tracks the unresolved-claim required-value behavior. The documentation already uses#463.internal/policy/policy.go (1)
205-207: LGTM!Also applies to: 269-287, 496-531, 553-562, 582-587
internal/policy/policy_test.go (1)
725-739: LGTM!Also applies to: 795-855
docs/src/content/docs/access-control.mdx (1)
222-222: LGTM!Also applies to: 233-244, 284-285, 531-531
internal/query/builder.go (1)
193-206: LGTM!Also applies to: 252-261, 278-285
internal/query/builder_test.go (1)
817-829: LGTM!
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Ok so some more findings after this review:
- Claude found some crazy unicode type issues that your guard stuff still fails on defending against, and it feels like we're always going to be playing catch-up with this approach – but I realized we already have an issue open for this separately: #322 – so rather than keep patching and adding to all of this, let's rip out all this guard stuff and regex but you added and whatnot, and instead do a proper fix for #322 separately
- Then that #322 split off also will apply not just to the where clause builder stuff, but also to the limit bit too, so that issue was updated to track that
- Then Hasura it looks like is basically what you suggested so let's roll with that
- The only other hole was the JWT number bit that we also have found here I think and definitely was something I mentioned in #381, so it keeps coming up and may be worth a test for (so anywhere we call jwt stuff we use numbers safely as they are floats and json and whatnot) or abstracting into a helper or something, or just fix as they come up to get these all merged asap either way
| // filter and returning the whole table. Refuse such identifiers until the splice | ||
| // is replaced by structural predicate emission (#322; the fail-closed predicate | ||
| // this protects is #385/#457). | ||
| var spliceKeywordRe = regexp.MustCompile(`(?i)\s(where|group\s+by|order\s+by|limit)\s`) |
There was a problem hiding this comment.
Second bug in the same family as the alias splice, and this one survives any fix to it — different function.
findInsertPoint:534 and ApplyMaxRows:164 both uppercase the SQL, find a keyword, then index into the original string with that offset. strings.ToUpper is not length-preserving in UTF-8 — 31 runes change byte length — so the offset can be wrong.
findInsertPoint: a column named ɐɐɐ (2→3 bytes) with a GROUP BY produces FROM \tbl` GR WHERE 1 = 0OUP BY …` — malformed SQL, so a caller-triggerable 500 rather than a leak.
ApplyMaxRows is the one I'd care about: a column named ıı makes strconv.Atoi receive "T 10000", the parse fails, and the role's max_rows cap is silently not applied. A policy control failing open with no error anywhere. max_result_rows is the backstop and it doesn't help against a groupArray, which returns one row.
Worth noting ApplyMaxRows is the same mistake as InjectPermissionFilters — structured_query.go:117-147 passes perms into Build, then two post-processors pull fields off that same perms and text-edit the SQL Build just wrote:
result, err := query.Build(table, &sq, schema, perms, h.BucketSecs, h.defaultMaxRows)
query.InjectPermissionFilters(result, perms.WhereClause, perms.WhereParams)
if perms.MaxRows > 0 { query.ApplyMaxRows(result, perms.MaxRows) }Folding the cap into Build's existing LIMIT computation at :130-138 is behaviour-preserving — today you get min(q.Limit, defaultMaxRows) then ApplyMaxRows lowers it to perms.MaxRows if smaller, which is just min of the three.
Both are in #322's scope now; I've updated it.
| - `{{ jwt.app_metadata.tenant_id }}` → a nested claim. | ||
|
|
||
| Values are always bound as SQL **parameters**, never concatenated into the query, so templating is injection-safe. If a claim path can't be resolved, the template renders as an **empty string** (rather than leaking `<nil>` into the predicate) — which, for a tenant filter, means the caller matches no tenant and sees nothing. Make sure your identity provider actually issues the claims your policy templates reference. | ||
| Claim paths may contain only letters, digits, `_`, and `.` (the segment separator). A `{{ jwt.… }}` value outside that grammar — a hyphen (`{{ jwt.tenant-id }}`), or a namespaced claim like `{{ jwt.https://app.example.com/tenant_id }}` — is **not** recognized as a template, and left unchecked the resolver would bind the literal `{{ … }}` text as a value. That is *not* fail-closed: on a read filter `_neq`/`_lt` would then match essentially every row (a leak), and on a write `check` the literal text would be stamped into every inserted row (silent corruption). So such a policy is **rejected at config load** — flatten hyphenated or namespaced claims into a supported path at your identity provider. (Auth0 and Okta custom claims are namespaced URLs by default, so this is the shape you are most likely to reach for first.) |
There was a problem hiding this comment.
"Rejected at config load" is true when a policy is written, but not when one is loaded, and the difference matters for exactly the deployments most likely to have the bad policy.
Validate runs from Store.Put only — so the bootstrap file at first boot (fatal, server won't start) and PUT/validate on /v1/admin/policy (400). But NewStore tries Store.load first and takes that branch whenever KV is populated, which is every boot after the first, and load caches without validating. Watch is the same.
So an operator who PUT {{ jwt.https://app.example.com/tenant_id }} under an older build upgrades to this one and keeps binding the literal text, silently, with no boot failure and no warning. Which is the leak this paragraph now tells them is closed.
Suggest qualifying it and giving them the action — something like: "…is rejected when the policy is written: the bootstrap file at first boot (WaveHouse refuses to start) and PUT/validate on /v1/admin/policy (400). A policy already stored in NATS KV is not re-validated when a node loads it (#461), so re-PUT your policy once after upgrading to surface a template written before this rule existed."
Not asking for the code fix here — that's #461, and Eric scoped it out deliberately.
|
|
||
| Values are always bound as SQL **parameters**, never concatenated into the query, so templating is injection-safe. If a claim path in a `filter` template can't be resolved (a validly-signed token that simply doesn't carry the claim), that filter **fails closed**: the predicate becomes constant-false, so on the structured-query path (`POST /v1/query`) the role sees **no rows**. This holds for every operator — `_eq`, `_neq`, `_gt`, `_lt`, and `_in` alike. The alternative, binding the empty string the template would render to, would leave a live predicate against `''`: `_eq` would match every empty-valued row, and `_neq`/`_gt` on a string column would match essentially *all* rows, erasing the restriction. A literal value with no template in it — including an explicit `""` — binds exactly as written. | ||
|
|
||
| Row filters apply on the structured-query path. The SSE stream does not yet scope rows by token claims (tracked in [#381](https://github.com/Wave-RF/WaveHouse/issues/381)), and named pipes authorize by `allowed_roles` membership rather than by row filters — scope a pipe's exposure with its query text and column rules. |
There was a problem hiding this comment.
Pipes have no column rules — internal/api/pipes.go only calls policy.RoleAllowed at :126. No Evaluate, no IsColumnAllowed on the result set. pipes.mdx:122 says as much explicitly ("The pipe's SQL is not re-checked against the table policy's column or row rules").
Pointing at a control that doesn't exist is worse than saying nothing, especially on this page — a reader will assume column masking covers pipes.
Suggest: "…scope a pipe's exposure in its SQL text — neither the table policy's row filter nor its column allow/deny list is applied on the pipe path (see Named Pipes)."
Related, while you're in there: pipes.mdx:122 also suggests scoping "via a claim-templated predicate". Pipes never see JWT claims — BindParams substitutes caller-supplied {{param}} values from the query string and body. Following that advice builds a boundary the caller controls. Pre-existing, but this new sentence routes readers straight to it.
|
|
||
| ### Security | ||
|
|
||
| - **Row-filter claim templates now fail closed on every operator when the token doesn't carry the claim** (`internal/policy/policy.go`, `internal/policy/policy_test.go`, `internal/query/builder.go`, `internal/query/builder_test.go`, `docs/src/content/docs/access-control.mdx`): closes [#385](https://github.com/Wave-RF/WaveHouse/issues/385), the remaining fail-open half of the class [#224](https://github.com/Wave-RF/WaveHouse/issues/224) opened. An unresolvable `{{ jwt.* }}` template in a `filter` rendered as the empty string and still bound a real predicate for `_eq`/`_neq`/`_gt`/`_lt` — so a validly-signed token missing the claim (mixed IdP audiences, service tokens) got `WHERE tenant_id = ''` (leaking every empty-valued row), and `_neq`/`_gt` on a string column (`col != ''` / `col > ''`) matched essentially all rows, erasing the restriction entirely. Only `_in` failed closed. Now any filter template containing an unresolvable claim path emits the same constant-false predicate `_in` already used (`1 = 0`): on the structured-query path (`POST /v1/query`) the role sees no rows, matching what `access-control.mdx` promised all along (the SSE stream is not yet claims-scoped — [#381](https://github.com/Wave-RF/WaveHouse/issues/381) — and named pipes authorize by role, not row filter). A template-free literal value — including an explicit `""` — still binds exactly as written, and an `_in` template with surrounding text joins the fail-closed path too (previously it bound the partial literal). Breaking only for deployments that relied on the fail-open: a role whose token lacks a templated claim now reads nothing instead of *more* than intended. Insert-`check` `_eq` semantics are unchanged — the template still renders (unresolvable placeholder → empty string, surrounding literal text kept) and that rendered value is auto-injected (check-path standardization is [#371](https://github.com/Wave-RF/WaveHouse/issues/371)) — but a `check: _in` template with surrounding text and an unresolvable claim now resolves to the empty set (every insert to that column rejected) instead of requiring membership in the partial literal, since the `_in` resolver is shared with the filter path. This PR also closes two adjacent fail-open paths in the same class surfaced in review: a claim template whose path is outside the `{{ jwt.<letters/digits/_/.> }}` grammar (a hyphen, or a namespaced OIDC URL) is now **rejected at config load** rather than bound as literal `{{…}}` text — a read leak for `_neq`/`_lt` and silent write corruption for `check`; and the query builder refuses an aggregation alias or `ORDER BY` reference containing a SQL clause keyword, so a crafted alias can no longer swallow the injected `WHERE` predicate and drop the row filter (the textual-splice root cause is tracked in [#322](https://github.com/Wave-RF/WaveHouse/issues/322)). |
There was a problem hiding this comment.
Two things here.
The #371 pointer is the one the repoint missed — policy.go and access-control.mdx both moved to #463, this didn't. #371 is about which operators check honours; the unresolvable-claim question is #463.
Second, the entry says such a policy "is now rejected at config load" but doesn't say what that does to a running deployment, and the effects are asymmetric enough to be worth a sentence: a bootstrap file carrying a namespaced or hyphenated claim now makes the server refuse to start, an admin PUT now 400s, and a policy already in KV is silently never re-validated (#461) so it keeps its old behaviour. The existing "Breaking only for deployments that relied on the fail-open" line covers the #385 semantic change, not this.
| // EventMessage — so there is no table to evaluate policy against — must be | ||
| // dropped, never passed through unfiltered. Only the no-policy legacy passthrough | ||
| // (filter=false) may forward it, and invalid JSON is dropped either way. | ||
| func TestProject_FailsClosedOnUndecodedPayload(t *testing.T) { |
There was a problem hiding this comment.
First, a correction on my side — I told Eric the empty-table_name half of #323 was untested and that this PR needed to add the guard. That was wrong, and I don't want you acting on it. It was already pinned: json.Unmarshal into EventMessage succeeds for any JSON object (no required fields), so {"custom":"data"} decodes fine with TableName == "" — meaning "not an EventMessage" and "empty table_name" are the same condition, and TestHub_PassthroughAndFailClosed was already covering it. I verified by deleting && evt.TableName != "" from both sites and watching the suite fail. Closes #323 is correct as-is.
The nit that remains: this test passes decoded as a literal false, so it exercises project's branch but never the derivation at hub.go:125/:160. And the thing that does catch the mutation is the passthrough content assertion in the filter=false case — so the coverage is incidental, on the non-security side. If the legacy passthrough is ever changed or that case dropped, the conjunct loses its guard silently.
Optional: drive {"table_name":"","data":{…}} through hub.ReplayFrame with a policy wired, asserting it's dropped. TestHub_ReplayFrame at :288 already has the harness. Pins it on the side that matters.
2 participants
Summary
A row filter whose
{{ jwt.* }}template references a claim the token doesn't carry (absent ornull) previously rendered to''and bound a real predicate —tenant_id != ''matches essentially every row, erasing the restriction.resolveTemplatenow reports resolution failure andresolveFiltersemits the constant-false1 = 0for every operator, matching the fail-closed behavior_inalready had from #224.nullfails closed.checkkeeps its resolve-to-''semantics (security(policy): define insert-check behavior when a claim template can't be resolved #463), now pinned by a test.access-control.mdx.Also pins the untested half of #323. That issue's behavior fix landed incidentally in #353 (a perf refactor), which is why it was never closed; both of its conditions — non-
EventMessageand emptytable_name— collapse into thedecodedflag atinternal/stream/hub.go:160, but only the first half has a regression test. This adds the second.Test plan
""and present-but-empty claims bind normally, check-path_eqresolution pinnedEvaluate: unresolvable claim yields constant-falseWhereClausetable_namefail-closed path inTestHub_PassthroughAndFailClosed(security(streaming): applyStreamPolicy passes non-EventMessage / empty table_name payloads through unfiltered (fail-open) #323)Related Issues
Closes #385
Closes #323