fix(policy): enforce _in on filter and check paths#358
Open
taitelee wants to merge 2 commits into
Open
Conversation
github-actions
Bot
added
documentation
|
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 (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
|
| Layer / File(s) | Summary |
|---|---|
Policy engine: _in resolution and validation internal/policy/policy.go |
Adds _in resolution for filters and checks, including array-valued claims, fail-closed empty handling, and validation rejection of unsupported or ambiguous insert-check operator combinations. |
Ingest handler: _in check enforcement internal/api/ingest.go |
Updates insert processing so _in checks require the column to be present and match one allowed value, with membership comparison helpers added. |
Policy and ingest tests internal/policy/policy_test.go, internal/api/ingest_test.go |
Adds coverage for _in filter expansion, _in check evaluation, validation outcomes, and ingest allow/deny scenarios. |
Docs and changelog docs/src/content/docs/access-control.mdx, CHANGELOG.md |
Documents enforced _in semantics, insert-check omission behavior, and adds the security changelog entry. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and concisely summarizes the main change: enforcing _in on both filter and check paths. |
| Description check | ✅ Passed | The description matches the code changes and explains the _in filter and check enforcement accurately. |
| Linked Issues check | ✅ Passed | The PR implements the linked issue's core fix by enforcing _in on filter and check paths and keeping unsupported check ops rejected. |
| Out of Scope Changes check | ✅ Passed | The changes stay focused on policy enforcement, docs, tests, and changelog updates needed for the _in fix. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
policy_filter_drops_fix
✨ Simplify code
- Create PR with simplified code
- Commit simplified code in branch
policy_filter_drops_fix
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 @coderabbitai help to get the list of available commands.
|
📚 Docs preview is live → https://1c25c594-wavehouse-docs.wave-rf.workers.dev |
Contributor
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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: edd81fa6-a91a-4365-a72e-29ad77305e60
📒 Files selected for processing (6)
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/api/ingest.gointernal/api/ingest_test.gointernal/policy/policy.gointernal/policy/policy_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Integration tests
- GitHub Check: Docs build
- GitHub Check: Coverage
- GitHub Check: E2E tests
- GitHub Check: Lint
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
⚠️ CI failures not shown inline (2)
GitHub Actions: PR housekeeping / PR housekeeping: fix(policy): enforce _in on filter and check paths
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(policy): enforce _in on filter and check paths
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 (3)
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Core packages should be interface-first: behaviors such as cache, dedupe, publisher, and subscriber should be defined behind interfaces to keep implementations swappable.
Files:
internal/api/ingest_test.gointernal/policy/policy.gointernal/policy/policy_test.gointernal/api/ingest.go
docs/src/content/docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
Author Mermaid diagrams vertically by default, keep labels short, and avoid placing two large diagrams side by side.
Files:
docs/src/content/docs/access-control.mdx
internal/policy/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Hasura-style access control must fail closed:
IsAdminis the single admin check, empty roles authorize nobody,nilpolicy denies everyone, anddefault_roleis the only sanctioned roleless exception.
Files:
internal/policy/policy.gointernal/policy/policy_test.go
🧠 Learnings (4)
📚 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-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/ingest_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/ingest_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/ingest_test.gointernal/policy/policy_test.go
🔇 Additional comments (2)
docs/src/content/docs/access-control.mdx (1)
198-200: LGTM!Also applies to: 215-215, 253-254, 500-501
CHANGELOG.md (1)
30-30: LGTM!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_inwas accepted by the policyFilterschema but never enforced: on the row-filter/SELECT path it produced noWHEREpredicate (a fail-open — atenant_id: { _in: … }filter let the role see every row instead of its tenant subset, same family as #223), and on thecheck/INSERT path only_eqwas honored. This enforces it for real on both paths._intakes a single claim that resolves to a JSON array (the multi-tenant case — a token'stenant_idslist) and emitscol IN (?, …)with one bound param per element. A scalar claim is a one-element set; an empty/absent claim matches no rows (fail-closed) rather than widening._incheck requires the inserted column be present and one of the claim-derived set — no auto-inject (unlike_eq, there's no single value to stamp), so an omitted column is rejected.checknow honors_eq+_in;_neq/_gt/_ltstay a loud config-load rejection (no insert-time meaning)._instays a single templated string in Go + the SDK), matching the established "set = array" shape of the caller-queryinoperator. Supersedes the interim guard that merely rejected_inatpolicy.Validate.Related Issues
Closes #224