Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@railway-app
Copy link

railway-app bot commented Jan 11, 2026

🚅 Deployed to the rivet-pr-3828 environment in rivet-frontend

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Jan 12, 2026 at 9:50 pm
frontend-cloud 😴 Sleeping (View Logs) Web Jan 11, 2026 at 10:54 pm
frontend-inspector 😴 Sleeping (View Logs) Web Jan 11, 2026 at 10:54 pm

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3828 January 11, 2026 22:37 Destroyed
Copy link
Member Author

NathanFlurry commented Jan 11, 2026


How to use the Graphite Merge Queue

Add 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.

@claude
Copy link

claude bot commented Jan 11, 2026

PR Review: fix(engine): require namespace for actor id-specific routes

Summary

This 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

  1. Security Improvement: Making namespace required prevents potential cross-namespace access issues where actors from different namespaces could be accessed without proper authorization checks.

  2. Consistent Implementation: The namespace verification logic is consistently applied across both affected endpoints:

    • Fetches actor to verify existence
    • Resolves namespace by name
    • Validates actor belongs to the specified namespace
    • Returns Actor::NotFound if namespace does not match (good security practice - does not leak namespace mismatch info)
  3. Proper Error Handling: Returns Actor::NotFound instead of a more specific error when namespace mismatch occurs, which is good security practice to avoid information leakage.

  4. Cross-datacenter Support: The changes properly handle remote datacenter routing by passing the namespace query parameter through the request_remote_datacenter_raw function.


Issues Found

1. Breaking Change - Test Failures Expected

The tests in api_actors_delete.rs will fail because they still use Option for the namespace parameter:

  • Line 39: namespace: Some(namespace.clone())
  • Line 85: namespace: None (Will fail at compile time)
  • Line 126: namespace: Some(namespace.clone())
  • Line 174: namespace: Some(namespace.clone())
  • Line 201: namespace: None (Will fail at compile time)

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_get

The 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 Fetch

In api-peer/src/actors/kv_get.rs, the code now performs two separate operations:

  1. Fetches the actor to verify namespace ownership (lines 47-58, 61-70)
  2. Fetches KV data (lines 79-80)

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 Cleanup

The 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 Analysis

Positive Security Impact:

  • Prevents potential unauthorized cross-namespace actor access
  • Enforces namespace ownership verification on all actor ID-specific operations
  • Uses safe error messages that do not leak information about namespace mismatches

No Security Concerns Found: The implementation properly validates ownership before performing operations.


Recommendations

  1. High Priority: Fix test compilation errors by updating DeleteQuery usage to match the new required namespace type.

  2. High Priority: Update OpenAPI documentation in api-peer/src/actors/kv_get.rs to include the namespace query parameter.

  3. Medium Priority: Consider documenting this as a breaking API change in release notes/changelog, as existing API clients will need to update their calls.

  4. Low Priority: Consider if the extra actor fetch in kv_get could be optimized, or document why this trade-off is acceptable for security.

  5. Low Priority: Move unrelated frontend cleanup to a separate commit.


Test Coverage Assessment

The existing test suite in api_actors_delete.rs has excellent coverage including:

  • Basic delete operations
  • Cross-datacenter routing
  • Wrong namespace validation
  • Non-existent namespace handling
  • Race condition handling
  • Idempotency (double delete)

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 Notes

The code follows the repository conventions from CLAUDE.md:

  • Uses structured logging with tracing
  • Proper error handling with custom error types
  • Lowercase log messages
  • Returns anyhow::Result from fallible functions
  • Hard tabs for formatting (visible in diff)

Summary Score: 7/10

Good: Security improvement, consistent implementation, proper error handling
Needs Fixing: Test compilation errors, missing OpenAPI docs
Consider: Performance implications, breaking change documentation

The core security improvement is solid, but the PR needs test updates and documentation fixes before merging.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 11, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3828

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3828

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3828

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3828

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3828

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3828

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3828

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3828

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3828

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3828

commit: 96eb8ac

@jog1t jog1t force-pushed the 01-11-fix_engine_require_namespace_for_actor_id-specific_routes branch from 9814eb6 to 96eb8ac Compare January 12, 2026 21:41
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3828 January 12, 2026 21:41 Destroyed
@NathanFlurry NathanFlurry force-pushed the 01-11-fix_engine_require_namespace_for_actor_id-specific_routes branch from 96eb8ac to d16e118 Compare January 12, 2026 21:49
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3828 January 12, 2026 21:49 Destroyed
@jog1t jog1t force-pushed the 01-11-fix_engine_require_namespace_for_actor_id-specific_routes branch from d16e118 to 96eb8ac Compare January 12, 2026 21:52
@NathanFlurry NathanFlurry force-pushed the 01-11-fix_engine_require_namespace_for_actor_id-specific_routes branch from 96eb8ac to d16e118 Compare January 12, 2026 21:54
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 12, 2026

Merge activity

  • Jan 12, 9:55 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 12, 9:56 PM UTC: CI is running for this pull request on a draft pull request (#3840) due to your merge queue CI optimization settings.
  • Jan 12, 9:56 PM UTC: Merged by the Graphite merge queue via draft PR: #3840.

@graphite-app graphite-app bot closed this Jan 12, 2026
@graphite-app graphite-app bot deleted the 01-11-fix_engine_require_namespace_for_actor_id-specific_routes branch January 12, 2026 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants