feat(rbac-proxy): introducing rbac proxy worker - #353
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 41 minutes and 33 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds the Changesrbac-proxy worker
Sequence Diagram(s)sequenceDiagram
participant DownstreamWorker
participant Proxy as rbac-proxy::proxy
participant Rbac as rbac-proxy::rbac
participant IIIClient
participant Engine as iii engine
participant Interceptor
DownstreamWorker->>Proxy: websocket upgrade request
Proxy->>Rbac: resolve_session(headers, query, ip_address)
Rbac->>IIIClient: trigger auth_function_id
IIIClient->>Engine: auth trigger
Engine-->>IIIClient: AuthResult
IIIClient-->>Rbac: AuthResult
Proxy->>Engine: open upstream websocket
Proxy->>Interceptor: handle_downstream(text frame)
Interceptor->>Engine: forward rewritten frame
Engine-->>Interceptor: invocationresult
Interceptor-->>DownstreamWorker: filtered response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
skill-check — worker0 verified, 28 skipped (no docs/).
Four for four. Nicely done. |
The `description` in iii.worker.yaml contained an unquoted `engine:: ` which YAML parsed as a mapping value, breaking validate_worker.py and the interface-smoke opt-out check. Quote the string. Also apply `cargo fmt --all` to satisfy the rust formatting check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The harness package-lock.json is gitignored and generated at first `npm install`, so `setup-node@v4` with `cache: 'npm'` against the missing lockfile is a hard failure. Mirror the storage-e2e workflow and omit the cache. run-tests.sh uses `npm install`, not `npm ci`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/rbac-proxy-e2e.yml:
- Around line 43-47: The installer step in the workflow pipes a mutable remote
script from the iii install endpoint directly into sh, which should be removed.
Update the “Install iii engine (next)” step to use a pinned, reproducible
installer source or a downloaded, verified artifact instead of executing the
live main branch script. Keep the change localized to the workflow job that
contains the iii installation step and preserve the PATH update after the
install completes.
- Line 23: The checkout step is persisting the GitHub token before executing
PR-controlled code, which can expose credentials to the test script. Update the
actions/checkout usage in the workflow to disable credential persistence, and
ensure the later ./run-tests.sh execution in the same job cannot read the token
from Git config. Keep the fix scoped to the checkout step and the job that runs
the PR branch code.
In `@rbac-proxy/iii.worker.yaml`:
- Line 7: The manifest description in the iii worker YAML is an unquoted scalar
that includes the text engine:: result, which can be parsed as YAML syntax and
break validation. Update the description field in the worker manifest so the
full value is quoted, keeping the existing wording intact, and verify the same
change is applied anywhere the iii worker description is defined to ensure the
manifest stays discoverable and valid.
In `@rbac-proxy/src/configuration.rs`:
- Line 71: This change is a formatting-only fix needed to satisfy cargo fmt
--all -- --check. Reformat the fetch_config function signature and the
tracing::warn! match arm in configuration.rs to match rustfmt output, and ensure
the related lines around the referenced block are updated consistently so CI can
pass without manual formatting diffs.
In `@rbac-proxy/src/engine_overrides.rs`:
- Line 68: The CI failure is due to formatting-only issues in
engine_overrides.rs, so run rustfmt over the affected expressions/macros to
normalize them. Update the code around the message formatting in the relevant
functions and blocks referenced by the diff, then reformat the rest of the
impacted spots consistently so cargo fmt --all passes cleanly.
In `@rbac-proxy/src/interceptor.rs`:
- Around line 218-236: The failure is formatting-only in the interceptor logic
around bare_id, call_hook, and apply_function_hook_mapping, so run rustfmt/cargo
fmt --all to normalize the affected expressions and macros in this area and the
other referenced spots, without changing behavior.
- Around line 456-460: The invocation result currently preserves the
engine-prefixed target, which can leak a self-invoke prefix like tenant::foo
back to callers expecting bare function names. Update the invocation result
construction in interceptor.rs, including the tokio::spawn block and the related
result handling around the other referenced section, so the function_id/value is
normalized by stripping any session/engine prefix before emitting the
invocationresult frame. Use the existing target/inv_id result-building logic as
the place to apply the prefix removal consistently for both regular and
middleware paths.
- Around line 270-321: The on_register_trigger flow in interceptor.rs is
forwarding trigger configs with an api_path that still starts with a leading
slash, which can create double-slash routes downstream. Update
on_register_trigger and/or apply_trigger_hook_mapping to validate the config
after hook remapping and reject registrations whose config.api_path begins with
a slash, using deny_trigger_registration with a clear message before forwarding.
Make sure the check applies to the RegisterTriggerInput path regardless of
whether the value came from the original payload or from call_hook mapping.
In `@rbac-proxy/src/lib.rs`:
- Around line 33-40: The fallback in redact_url currently returns the original
input when url::Url::parse fails, which can leak credentials in logs and status
output. Update redact_url in rbac-proxy::lib::redact_url so the Err(_) branch
never returns the raw URL; instead use a best-effort redaction approach or a
generic redacted placeholder, while keeping the successful parse path unchanged.
In `@rbac-proxy/src/manifest.rs`:
- Around line 37-50: The test block in manifest.rs is not formatted according to
rustfmt, causing the format check to fail. Run cargo fmt on the file and keep
the existing json_roundtrip_has_required_fields test logic unchanged, with
special attention to the wrapped assert on supported_targets so it matches
rustfmt’s preferred layout.
In `@rbac-proxy/src/rbac.rs`:
- Around line 291-310: The FunctionFilter serialization blocks are failing
rustfmt, so update the affected Serialize for FunctionFilter implementation to
match standard Rust formatting. Run cargo fmt on the file and ensure the
FunctionFilter::Match and FunctionFilter::Metadata arms, along with the
surrounding impl Serialize and serialize_map usage, are formatted consistently
with rustfmt so CI passes.
- Around line 553-567: The AuthRejection handling in rbac.rs is leaking internal
backend and deserialization details through the message field, so replace the
raw e.to_string() responses in the match over result and
serde_json::from_value::<AuthResult>(v) with a stable generic client-facing
rejection message. Keep the detailed error text server-side by logging it near
the existing auth/session flow in the relevant rbac:: function, while preserving
the AUTH_ERROR code and using a consistent non-sensitive message for all failure
branches.
In `@rbac-proxy/src/server.rs`:
- Around line 118-154: The server shutdown path in `spawn_server` and `shutdown`
only signals the watch channel and never waits for the `axum::serve` task to
finish, so in-flight requests may be dropped. Change `spawn_server` to retain
the spawned `JoinHandle` alongside the shutdown sender in the proxy
state/handle, then update `shutdown()` to send the signal and await the server
task’s completion before returning. Use the existing `spawn_server`, `shutdown`,
and `current`/proxy handle plumbing to locate the async task ownership and make
graceful draining actually complete.
In `@rbac-proxy/tests/e2e/run-tests.sh`:
- Around line 88-92: The E2E harness currently reuses the existing data
directory, so persisted engine state can leak between runs and make fixed-id
tests in run-tests.sh non-hermetic. Update the test setup in the run-tests.sh
flow around the ROOT_DIR preparation and cd step to start from a clean state by
removing or recreating the persisted data used by the engine before launching
the suite, while keeping the reports directory behavior intact.
In `@rbac-proxy/tests/golden/schemas/rbac-proxy.status.json`:
- Around line 32-34: The status schema for rbac_enabled does not match the
worker’s actual RBAC enforcement, since the proxy can still block calls when
expose_functions is set even if auth_function_id is unset. Update the schema
contract in rbac-proxy.status.json so the rbac_enabled field name/description
reflects the full enforcement behavior, or rename the field to match the current
meaning used by the proxy status output.
In `@rbac-proxy/tests/integration.rs`:
- Around line 209-211: The `assert_eq!` in the integration test needs to be
reformatted to satisfy rustfmt and unblock CI. Adjust the call in the echoed
payload assertion so it matches standard formatting conventions, keeping the
same check and message while letting `cargo fmt --check` pass.
- Around line 109-115: The early-exit branch in the harness setup still returns
a Harness even when wait_for_listen fails, so update the integration test flow
to self-skip by returning None from the branch in the setup path around
wait_for_listen and Harness construction; keep the skipping message, but do not
spawn the placeholder proxy or call register_support() when the engine never
binds.
In `@rbac-proxy/tests/manifest.rs`:
- Around line 40-44: The manifest test block in manifest.rs needs formatting to
satisfy cargo fmt --all -- --check; run cargo fmt on this file and let it reflow
the default_config assertions around the cfg loop and port check without
changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abc028c0-f43a-496a-bd63-d78f7c46f57c
⛔ Files ignored due to path filters (1)
rbac-proxy/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.github/workflows/create-tag.yml.github/workflows/rbac-proxy-e2e.yml.github/workflows/release.ymlREADME.mdiii-permissions.yamlrbac-proxy/Cargo.tomlrbac-proxy/README.mdrbac-proxy/build.rsrbac-proxy/iii.worker.yamlrbac-proxy/src/channels.rsrbac-proxy/src/config.rsrbac-proxy/src/configuration.rsrbac-proxy/src/engine_overrides.rsrbac-proxy/src/functions/mod.rsrbac-proxy/src/functions/status.rsrbac-proxy/src/interceptor.rsrbac-proxy/src/lib.rsrbac-proxy/src/main.rsrbac-proxy/src/manifest.rsrbac-proxy/src/proxy.rsrbac-proxy/src/rbac.rsrbac-proxy/src/server.rsrbac-proxy/tests/e2e/.gitignorerbac-proxy/tests/e2e/README.mdrbac-proxy/tests/e2e/config.yamlrbac-proxy/tests/e2e/reports/.gitkeeprbac-proxy/tests/e2e/run-tests.shrbac-proxy/tests/e2e/workers/harness/iii.worker.yamlrbac-proxy/tests/e2e/workers/harness/package.jsonrbac-proxy/tests/e2e/workers/harness/src/cases.tsrbac-proxy/tests/e2e/workers/harness/src/runner.tsrbac-proxy/tests/e2e/workers/harness/src/support.tsrbac-proxy/tests/e2e/workers/harness/src/worker.tsrbac-proxy/tests/e2e/workers/harness/tsconfig.jsonrbac-proxy/tests/golden/schemas/rbac-proxy.status.jsonrbac-proxy/tests/integration.rsrbac-proxy/tests/manifest.rsrbac-proxy/tests/schemas.rsrbac-proxy/tests/support/mod.rstech-specs/2026-06-rbac-proxy-worker/presentation/src/content/deck.tstech-specs/2026-06-rbac-proxy-worker/presentation/src/sections/Hero.tsx
Comment on lines
+32
to
+34
| "rbac_enabled": { | ||
| "description": "Whether an `auth_function_id` is configured.", | ||
| "type": "boolean" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
rbac_enabled doesn't match the worker's actual enforcement.
This defines the field as "Whether an auth_function_id is configured", but the proxy still denies calls when expose_functions is populated and no auth function is set. Publishing this schema will tell downstream tooling "rbac off" for a proxy that is still filtering traffic. Rename the field or broaden its semantics before this becomes the public contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rbac-proxy/tests/golden/schemas/rbac-proxy.status.json` around lines 32 - 34,
The status schema for rbac_enabled does not match the worker’s actual RBAC
enforcement, since the proxy can still block calls when expose_functions is set
even if auth_function_id is unset. Update the schema contract in
rbac-proxy.status.json so the rbac_enabled field name/description reflects the
full enforcement behavior, or rename the field to match the current meaning used
by the proxy status output.
Comment on lines
+109
to
+115
| if !wait_for_listen(49134, Duration::from_secs(8)).await { | ||
| eprintln!("skipping: engine did not come up on :49134"); | ||
| return Some(Harness { | ||
| proxy: Command::new("true").spawn().ok()?, | ||
| iii, | ||
| _support: register_support(), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return None when the engine never binds.
Line 110 says this branch is skipping, but Lines 111-115 still return Some(Harness) and even call register_support(). That makes the test continue against a dead engine instead of self-skipping.
Proposed fix
if !wait_for_listen(49134, Duration::from_secs(8)).await {
eprintln!("skipping: engine did not come up on :49134");
- return Some(Harness {
- proxy: Command::new("true").spawn().ok()?,
- iii,
- _support: register_support(),
- });
+ let mut iii = iii;
+ let _ = iii.kill();
+ let _ = iii.wait();
+ return None;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if !wait_for_listen(49134, Duration::from_secs(8)).await { | |
| eprintln!("skipping: engine did not come up on :49134"); | |
| return Some(Harness { | |
| proxy: Command::new("true").spawn().ok()?, | |
| iii, | |
| _support: register_support(), | |
| }); | |
| if !wait_for_listen(49134, Duration::from_secs(8)).await { | |
| eprintln!("skipping: engine did not come up on :49134"); | |
| let mut iii = iii; | |
| let _ = iii.kill(); | |
| let _ = iii.wait(); | |
| return None; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rbac-proxy/tests/integration.rs` around lines 109 - 115, The early-exit
branch in the harness setup still returns a Harness even when wait_for_listen
fails, so update the integration test flow to self-skip by returning None from
the branch in the setup path around wait_for_listen and Harness construction;
keep the skipping message, but do not spawn the placeholder proxy or call
register_support() when the engine never binds.
This was referenced Jul 3, 2026
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
1 participant
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 by CodeRabbit
New Features
rbac-proxyworker, including a public RBAC-protected proxy endpoint, channel bridging, and live configuration updates.rbac-proxybuilds can be published and released.Documentation
Tests