[Graphite MQ] Draft PR GROUP:spec_887ff5 (PRs 3828, 3809, 3452, 3830, 3831)

[graphite-app]

This draft PR was created by the Graphite merge queue.
Trunk will be fast forwarded to the HEAD of this PR when CI passes, and the original PRs will be closed.

The following PRs are included in this draft PR:

• #3828
• #3809
• #3452
• #3830
• #3831

[railway-app]

This PR was not deployed automatically as @graphite-app[bot] does not have access to the Railway project.

In order to get automatic PR deploys, please add @graphite-app[bot] to your workspace on Railway.

[claude]

PR Review - Graphite Merge Queue

This is a merge queue PR combining 5 individual PRs. Below is my analysis:

Summary of Changes

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

• ✅ Adds namespace requirement to actor delete and KV get routes
• ✅ Implements proper namespace validation before operations

PR #3809: fix(dashboard): use proper timestamp for actor status

• ✅ Uses connectableTs instead of startedAt for actor status determination
• ✅ Minor but important correctness fix

PR #3452: refactor: performance improvements for inspector

• ⚠️ Large refactor (5308 additions, 11792 deletions)
• Removes old actor component files
• Adds comprehensive testing documentation

PR #3830: chore: update sdks

• ✅ Generated SDK updates to match API changes

PR #3831: feat(dashboard): provide ns when querying kv

• ✅ Adds namespace parameter to KV queries in frontend
• ✅ Adds timing-safe comparison utility

---

Code Quality & Best Practices

✅ Strengths

1. Security Improvements

   • engine/packages/api-peer/src/actors/kv_get.rs:60-70 - Proper namespace validation prevents unauthorized access
   • rivetkit-typescript/packages/rivetkit/src/utils/crypto.ts:5-24 - Implements timing-safe comparison to prevent timing attacks
   • Both delete and KV get operations verify actor belongs to the specified namespace

2. Error Handling

   • Uses custom error system correctly with .build() pattern
   • Returns appropriate 404 errors when namespace mismatch occurs
   • Base64 decoding includes proper error context

3. Code Style Adherence

   • Structured logging used correctly: tracing::warn!(actor_id=?path.actor_id, "...")
   • Lowercase log messages as per CLAUDE.md
   • Proper use of workspace dependencies

⚠️ Concerns & Suggestions

1. Potential Breaking Change

   • The namespace requirement on actor-specific routes (delete, KV get) is a breaking API change
   • SDK updates are included, but existing API consumers may break
   • Recommendation: Ensure this is communicated in release notes and consider API versioning

2. Timing-Safe Comparison Implementation

   • rivetkit-typescript/packages/rivetkit/src/utils/crypto.ts:14-15
   • The implementation pads to max length, but still performs XOR on length difference
   • While better than naive comparison, this still leaks length information through the initial XOR
   • Recommendation: Consider using a constant-time comparison library like tweetnacl or ensure padding happens before the length XOR

3. Base64 Key Handling

   • engine/packages/api-peer/src/actors/kv_get.rs:73-75
   • Base64 decode happens after namespace validation, which is correct
   • However, error context is generic: "failed to decode base64 key"
   • Minor suggestion: Consider adding the key to error context for debugging (if it's not sensitive)

4. Missing Tests

   • No test coverage visible for the namespace validation logic
   • Recommendation: Add tests for:
     • Actor from namespace A accessing with namespace B query param
     • Invalid namespace in query param
     • Base64 decode failures

5. Frontend Changes (PR refactor: performance improvements for inspector #3452)

   • Large deletion of 11,792 lines is concerning without seeing the replacement code
   • Cannot fully verify if functionality is preserved
   • Testing documentation added is good, but unclear if tests were updated

Performance Considerations

1. Double Database Query Pattern

   • engine/packages/api-peer/src/actors/kv_get.rs:47-70
   • Queries actor first, then validates namespace, then queries KV
   • Consider: Could namespace validation be combined with actor fetch to reduce round trips?

2. URL Encoding in Remote Datacenter Requests

   • engine/packages/api-public/src/actors/kv_get.rs:85
   • URL encoding happens at runtime for every request
   • This is correct but note that base64 keys might be large

Security Concerns

1. Namespace Authorization ✅

   • Properly implemented to prevent cross-namespace access
   • Returns generic 404 instead of 403 to avoid information leakage

2. Timing Attack Mitigation ⚠️

   • Implementation has minor issues as noted above
   • Length information may still leak

3. Base64 Decoding ✅

   • Properly validates before use
   • Returns error rather than panicking

Test Coverage

⚠️ No test files included in this PR

• Namespace validation logic should have integration tests
• KV operations with cross-namespace attempts should be tested
• Timing-safe comparison should have unit tests

Documentation

✅ Good: PR #3452 adds extensive testing documentation
⚠️ Missing: No API documentation updates for the breaking change

---

Recommendations

Critical

1. Add tests for namespace validation in actor delete and KV get operations
2. Review timing-safe comparison implementation or use a vetted library
3. Document breaking change in API for namespace requirement

Important

4. Consider combining database queries for performance
5. Verify PR refactor: performance improvements for inspector #3452 refactor maintains all functionality (unable to fully review due to size)

Nice to Have

6. Add more context to base64 decode errors
7. Consider adding metrics for cross-namespace access attempts

---

Overall Assessment

Recommendation: ⚠️ Approve with Cautions

The security improvements are valuable and the code generally follows best practices. However:

• The breaking API change needs clear communication
• Test coverage is insufficient for security-critical validation logic
• The timing-safe comparison implementation needs review
• PR refactor: performance improvements for inspector #3452 is too large to fully review (consider breaking into smaller PRs in the future)

The namespace validation logic itself is well-implemented and prevents a real security issue, so the core functionality is sound.

---

Generated by Claude Code - Review based on CLAUDE.md conventions