fix(stream): apply policy row-filter per subscriber on SSE#381
fix(stream): apply policy row-filter per subscriber on SSE#381taitelee wants to merge 9 commits into
Conversation
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (9)
🧰 Additional context used📓 Path-based instructions (2)docs/src/content/docs/**/*.{md,mdx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-06-10T15:01:09.027ZApplied to files:
📚 Learning: 2026-07-07T12:38:12.052ZApplied to files:
📚 Learning: 2026-06-26T12:23:22.696ZApplied to files:
🔇 Additional comments (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR makes SSE live-stream delivery apply row-level policy filters per subscriber, using claims-aware policy resolution, schema-aware numeric comparisons, and replay-path filtering. It also updates wiring, tests, and documentation to match the new streaming behavior. ChangesStreaming row-level security enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StreamHandler
participant Subscriber
participant Hub
participant Policy
StreamHandler->>Subscriber: SetClaims(claims)
StreamHandler->>Hub: register subscriber
Hub->>Policy: Evaluate(role, claims)
Policy-->>Hub: resolved rowFilter
Hub->>Hub: projectColumns(role, event)
Hub->>Hub: RowVisible(row, numericCols)
Hub-->>Subscriber: deliver frame if visible
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
|
📚 Docs preview is live → https://c89f5022-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall coverage in the branch remains at 90%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/content/docs/access-control.mdx (1)
373-373: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the Live stream row in the enforcement table to include row-level filtering.
The "Where each rule is enforced" table at line 373 still reads "denied columns are masked from each event" with no mention of row-level
filter, but this PR adds exactly that enforcement. The Structured read row (line 371) explicitly lists "row filter", so the table is now inconsistent with both the caution block below (lines 377–378) and the actual code (hub.goBroadcastcallsRowVisibleper subscriber). Per the docs code↔docs sync guideline, the table should reflect the changed behavior.📝 Proposed fix to update the table row
-| Live stream | `GET /v1/stream` | table+role `select` required (a table the role can't read is skipped), then denied columns are masked from each event | +| Live stream | `GET /v1/stream` | table+role `select` required (a table the role can't read is skipped), then denied columns are masked from each event, and row-level `filter` predicates are evaluated per subscriber against their JWT claims (see caution below) |Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2efc09f7-0227-4f93-844b-1b18b486200f
📒 Files selected for processing (17)
CHANGELOG.mdcmd/wavehouse/main.godocs/src/content/docs/access-control.mdxdocs/src/content/docs/architecture.mdinternal/api/errors_test.gointernal/api/router_test.gointernal/api/stream.gointernal/api/stream_test.gointernal/discovery/validation.gointernal/discovery/validation_test.gointernal/policy/policy.gointernal/policy/rowfilter.gointernal/policy/rowfilter_test.gointernal/stream/hub.gointernal/stream/hub_test.gointernal/stream/subscriber.gotests/integration/setup_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: E2E tests
- GitHub Check: Docs build
- GitHub Check: Coverage
- GitHub Check: Lint
- GitHub Check: Analyze (go)
⚠️ CI failures not shown inline (2)
GitHub Actions: PR housekeeping / 0_PR housekeeping.txt: fix(stream): apply policy row-filter per subscriber on SSE
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
GitHub Actions: PR housekeeping / PR housekeeping: fix(stream): apply policy row-filter per subscriber on SSE
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
🧰 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/api/errors_test.gotests/integration/setup_test.gointernal/api/stream_test.gointernal/api/router_test.gointernal/discovery/validation_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_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.mdxdocs/src/content/docs/architecture.md
🧠 Learnings (5)
📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:00.784Z
Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.
Applied to files:
internal/api/errors_test.gointernal/api/stream_test.gointernal/api/router_test.go
📚 Learning: 2026-05-23T01:23:59.268Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:23:59.268Z
Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.
Applied to files:
internal/api/errors_test.gointernal/api/stream_test.gointernal/api/router_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/api/errors_test.gotests/integration/setup_test.gointernal/api/stream_test.gointernal/api/router_test.gointernal/discovery/validation_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_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/api/errors_test.gocmd/wavehouse/main.gointernal/discovery/validation.gotests/integration/setup_test.gointernal/api/stream.gointernal/api/stream_test.gointernal/api/router_test.gointernal/discovery/validation_test.gointernal/policy/policy.gointernal/policy/rowfilter.gointernal/stream/subscriber.gointernal/policy/rowfilter_test.gointernal/stream/hub.gointernal/stream/hub_test.go
📚 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:
docs/src/content/docs/architecture.mdCHANGELOG.md
🔇 Additional comments (20)
CHANGELOG.md (1)
34-34: LGTM!docs/src/content/docs/access-control.mdx (1)
377-378: LGTM!docs/src/content/docs/architecture.md (1)
88-88: LGTM!cmd/wavehouse/main.go (1)
295-295: LGTM!tests/integration/setup_test.go (1)
315-315: LGTM!internal/api/errors_test.go (1)
197-197: LGTM!internal/api/router_test.go (1)
286-286: LGTM!Also applies to: 342-342, 414-414, 475-475, 526-526, 551-551, 648-648
internal/api/stream_test.go (1)
25-25: LGTM!Also applies to: 53-53, 78-78, 117-117
internal/stream/hub_test.go (2)
41-51: LGTM!
68-68: LGTM!Also applies to: 100-100, 136-136, 288-288, 331-331, 352-352, 373-373, 390-390, 411-411, 440-440
internal/policy/policy.go (1)
64-82: LGTM!Also applies to: 197-208, 228-313
internal/policy/rowfilter.go (2)
13-15: LGTM!
17-103: 🔒 Security & PrivacyLexicographic fallback is intentional
NewHubis already wired with a schema registry incmd/wavehouse/main.go, andnumericColsdeliberately falls back to string comparison when schema information is unavailable. That makes>/<best-effort by design, so this isn’t a security bug; the doc comment is the part that should be softened.> Likely an incorrect or invalid review comment.internal/policy/rowfilter_test.go (1)
1-108: LGTM!internal/discovery/validation.go (1)
51-69: LGTM!Also applies to: 152-158
internal/discovery/validation_test.go (1)
227-255: LGTM!internal/api/stream.go (1)
45-51: LGTM!Also applies to: 85-85, 103-103
internal/stream/hub.go (2)
25-31: LGTM!Also applies to: 43-45
185-198: LGTM!Also applies to: 218-233, 245-277
internal/stream/subscriber.go (1)
26-31: LGTM!Also applies to: 51-55
github-project-automation
Bot
moved this from In progress
to In review
in WaveHouse Task Board
github-actions
Bot
added
the
dependencies
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: c2faf18f-1e6f-44b3-92f4-aa3152c98c0c
📒 Files selected for processing (6)
go.modinternal/policy/policy.gointernal/policy/rowfilter.gointernal/policy/rowfilter_test.gointernal/stream/hub.gointernal/stream/hub_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Integration tests
- GitHub Check: E2E tests
- GitHub Check: Docs build
- GitHub Check: Unit tests
- GitHub Check: Coverage
- GitHub Check: Lint
- GitHub Check: Analyze (go)
⚠️ CI failures not shown inline (2)
GitHub Actions: PR housekeeping / PR housekeeping: fix(stream): apply policy row-filter per subscriber on SSE
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
GitHub Actions: PR housekeeping / 0_PR housekeeping.txt: fix(stream): apply policy row-filter per subscriber on SSE
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
🧰 Additional context used
📓 Path-based instructions (1)
**/*_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/rowfilter_test.gointernal/stream/hub_test.go
🧠 Learnings (2)
📚 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.gointernal/policy/rowfilter.gointernal/policy/policy.gointernal/policy/rowfilter_test.gointernal/stream/hub_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/rowfilter_test.gointernal/stream/hub_test.go
🔇 Additional comments (8)
internal/policy/policy.go (2)
111-175: LGTM!Also applies to: 177-233
235-260: LGTM!Also applies to: 262-461
internal/policy/rowfilter.go (1)
78-139: LGTM!internal/policy/rowfilter_test.go (1)
111-167: LGTM!Also applies to: 169-232
internal/stream/hub.go (2)
142-171: LGTM!Also applies to: 221-236
172-173: 🩺 Stability & AvailabilityNo race here —
Subscriber.claimsis set once viaSetClaimsbeforeHub.Addand documented as read-only for the subscriber’s lifetime, soBroadcast’s read does not have a concurrent writer.> Likely an incorrect or invalid review comment.internal/stream/hub_test.go (1)
22-29: LGTM!Also applies to: 188-214, 440-464, 519-526, 530-545
go.mod (1)
3-3: LGTM!
…ties at full precision
1 participant
Summary
The Server-Sent Events stream applied a role's column allowlist but never its row-level
filter, so a subscriber could receive rows the structured-query path would have hidden for that same role — a row-level-security bypass on the streaming surface. This applies the same row-filter on the stream:ResolvedPermissions.RowVisible) on the stream — so the two can't drift._gt/_lt) are schema-informed — numeric columns compare numerically via the schema registry now wired into the hub — while equality/set (_eq/_neq/_in) stay exact. Ambiguous values fail closed; the best-effort ordering boundary is documented in access-control docs.Related Issues
Closes #319