-
Notifications
You must be signed in to change notification settings - Fork 146
fix(engine): require namespace for actor id-specific routes #3828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(engine): require namespace for actor id-specific routes #3828
Conversation
|
🚅 Deployed to the rivet-pr-3828 environment in rivet-frontend
|
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR Review: fix(engine): require namespace for actor id-specific routesSummaryThis PR changes the namespace query parameter from optional to required for actor ID-specific routes (DELETE /actors/{actor_id} and GET /actors/{actor_id}/kv/keys/{key}). This is a breaking API change that improves security by ensuring namespace ownership verification is always performed. Strengths
Issues Found1. Breaking Change - Test Failures ExpectedThe tests in api_actors_delete.rs will fail because they still use Option for the namespace parameter:
Action Required: Update all test cases to provide a required namespace string instead of Option. The delete_existing_actor_without_namespace test should either be removed or repurposed to test a different scenario since namespace is now mandatory. 2. Missing OpenAPI Documentation for kv_getThe utoipa path macro in api-peer/src/actors/kv_get.rs:32-43 does not include the new namespace query parameter in its documentation. Action Required: Add the namespace parameter to the utoipa path documentation, similar to how it is done in the public API at line 42 in api-public/src/actors/kv_get.rs by adding KvGetQuery to the params. 3. Performance Consideration - Duplicate Actor FetchIn api-peer/src/actors/kv_get.rs, the code now performs two separate operations:
While functionally correct, this adds an extra database round trip. Consider if the namespace verification could be done more efficiently, perhaps by checking namespace_id directly in the KV lookup query (though this may not be possible depending on the KV storage implementation). This is a minor performance concern and may be acceptable for security, but worth documenting or optimizing if it becomes a bottleneck. 4. Frontend Code CleanupThe frontend change (frontend/src/app/runner-config-table.tsx) removes unused imports. While this is good cleanup, it is unrelated to the PR stated purpose of requiring namespace for actor routes. This should ideally be in a separate cleanup commit or PR. Security AnalysisPositive Security Impact:
No Security Concerns Found: The implementation properly validates ownership before performing operations. Recommendations
Test Coverage AssessmentThe existing test suite in api_actors_delete.rs has excellent coverage including:
However, these tests need to be updated to compile with the new required namespace parameter. Missing test coverage for kv_get endpoint: No similar comprehensive test suite found for the KV get endpoint with namespace validation. Consider adding tests similar to those in api_actors_delete.rs. Code Style NotesThe code follows the repository conventions from CLAUDE.md:
Summary Score: 7/10Good: Security improvement, consistent implementation, proper error handling The core security improvement is solid, but the PR needs test updates and documentation fixes before merging. |
More templates
@rivetkit/cloudflare-workers
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/virtual-websocket
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
14a854f to
9814eb6
Compare
9814eb6 to
96eb8ac
Compare
96eb8ac to
d16e118
Compare
d16e118 to
96eb8ac
Compare
96eb8ac to
d16e118
Compare
Merge activity
|

No description provided.