fix(guard): fix url decoding actor id + token

[NathanFlurry]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 5, 2025 4:06am

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 5, 2025 4:06am
rivet-inspector	Ignored	Preview		Nov 5, 2025 4:06am
rivet-site	Ignored	Preview		Nov 5, 2025 4:06am

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore(pegboard): allow configuring reschedule_backoff_max_exponent #3370
• fix(next-js): auto shut down runners when source code updates #3369
• fix(rivetkit): fix duplicate runner key in serverless #3366
• chore(runner): improve runner connection logs #3365
• chore(rivetkit): add runner sse ping interval #3362
• chore(rivetkit): log close code when client ws closes #3361
• chore(rivetkit): log runner id with actor logs #3360
• chore(scripts): add endian converter script #3359
• fix(guard): fix url decoding actor id + token #3357 👈 (View in Graphite)
• chore(rivetkit): remove unneeded headers & protocols in requests #3356
• fix(rivetkit): reduce pegboard actor retry for spawned engines #3355
• chore(pegboard): allow configuring base_retry_timeout, actor_start_threshold, actor_stop_threshold, and retry_reset_duration #3354
• feat(pb): add reschedule_ts to actors #3363
• fix: fix all dashboards #3358
• chore: change log levels #3333
• chore: misc bug fixes, add logs for outbound req #3332
• fix: use correct vbare functions for runner tunnel #3300 : 1 other dependent PR (#3301 )
• feat(grafana): fix api dashboards #3193
• fix: docker compose build #3273
• chore: fix remaining warnings for all targets #3305
• fix(gas): fix from_workflow when creating standalone ctx from op ctx #3342
• ci: udpate ci filter to succeed if no relevant changes #3343
• main

---

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]

Pull Request Review

This PR adds URL decoding for actor IDs and tokens in the path-based routing system for both Rust (guard) and TypeScript (RivetKit). The implementation is solid and includes comprehensive test coverage.

✅ Strengths

1. Consistent implementation across languages: Both Rust and TypeScript implementations handle URL decoding identically, which is critical for maintaining routing parity.

2. Comprehensive test coverage: Excellent test additions covering:

   • Encoded characters in actor_id and token
   • Invalid encoding rejection
   • Edge cases like spaces and special characters
   • Both languages have matching test suites

3. Proper error handling:

   • Rust: Uses urlencoding::decode(aid).ok()?.to_string() which gracefully returns None on invalid encoding
   • TypeScript: Uses try-catch around decodeURIComponent to return null on invalid encoding

4. Security consideration: The change properly handles URL-encoded characters that could otherwise be used to bypass routing or authentication checks.

5. Dependency management: Correctly added urlencoding to workspace dependencies following CLAUDE.md conventions.

🔍 Code Quality Observations

Rust implementation (engine/packages/guard/src/routing/mod.rs:211-220):

• Clean and idiomatic use of the ? operator for error propagation
• Maintains existing function structure well
• Decoding happens in the right place - after splitting by @ but before returning

TypeScript implementation (rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts:399-428):

• Proper use of try-catch for error handling
• Good variable naming (rawActorId, rawToken vs decoded actorId, token)
• Clear separation of concerns

💡 Minor Suggestions

1. Comment clarity: Consider adding a brief comment explaining why URL decoding is needed here. For future maintainers, it would be helpful to understand that clients may URL-encode these path segments, and we need to decode them to get the actual actor ID/token values.

// URL-decode both actor_id and token since clients may encode special characters
let decoded_aid = urlencoding::decode(aid).ok()?.to_string();

2. Test case consideration: The current tests cover invalid hex encoding (%ZZ, %GG), but you might also want to test:

   • Plus signs (+) which can be interpreted as spaces in some contexts
   • Percent-encoded percent signs (%25) to ensure they're decoded correctly

   Not critical for this PR, but worth considering for future robustness.

3. Remaining path encoding: I notice the remaining_path is not decoded (line 177 in Rust shows it preserves encoding: /api%20endpoint/test%2Fpath). This is actually the correct behavior since the remaining path should be forwarded as-is to the actor, but it might be worth adding a comment to clarify this is intentional.

🎯 Potential Concerns

None identified. This is a straightforward bug fix that addresses a real issue with URL-encoded actor IDs and tokens.

📋 Test Coverage

Both Rust and TypeScript test suites now include:

• ✅ URL-encoded hyphens in actor_id
• ✅ URL-encoded @ symbol in token
• ✅ URL-encoded characters in both actor_id and token
• ✅ URL-encoded spaces
• ✅ Invalid encoding rejection

The test coverage is thorough and validates the behavior comprehensively.

🏁 Recommendation

Approved ✅

This is a well-implemented fix with excellent test coverage. The code follows repository conventions (as outlined in CLAUDE.md), handles errors properly, and maintains consistency between the Rust and TypeScript implementations.

The minor suggestions above are optional improvements that don't block merging.

[claude]

Pull Request Review

Summary

This PR fixes URL decoding issues in actor ID and token parsing for both Rust (guard package) and TypeScript (rivetkit) implementations. The changes ensure that URL-encoded characters in actor IDs and tokens are properly decoded before use.

Positive Aspects ✓

1. Excellent Test Coverage: Both Rust and TypeScript implementations include comprehensive test cases covering:

   • Encoded actor IDs
   • Encoded tokens
   • Encoded spaces
   • Invalid encoding scenarios
   • Edge cases with both actor ID and token encoded

2. Consistent Implementation: The fix is consistently applied across both Rust and TypeScript codebases, maintaining parity between the two implementations.

3. Proper Error Handling:

   • Rust: Uses .ok()? to propagate errors from invalid URL encoding
   • TypeScript: Uses try-catch to handle decodeURIComponent exceptions

4. Follows Project Conventions:

   • Proper use of workspace dependencies in Cargo.toml
   • Added dependency correctly with urlencoding.workspace = true

Potential Issues & Concerns

1. Critical: Double Decoding Risk 🔴

The most significant concern is the potential for double URL decoding. This is a common security and correctness issue:

• Problem: If the HTTP framework/server (e.g., Hyper, Axum, or whatever proxy is in front) already decodes the path before passing it to parse_actor_path(), this code will decode it a second time.
• Impact:
  • An actor ID of actor-123 would need to be sent as actor%252D123 (double-encoded) to work correctly
  • Security risk: Could lead to path traversal or authentication bypass if an attacker can craft specially encoded URLs
  • Example: actor%2540admin → actor%40admin (after first decode) → actor@admin (after second decode)

Recommendation:

• Verify whether the HTTP framework is already decoding the path
• If it is, remove this decoding logic
• If it isn't, document this clearly in comments
• Add integration tests that verify the actual HTTP layer behavior

2. Security: No Validation After Decoding ⚠️

After decoding, there's no validation of the decoded content:

let decoded_aid = urlencoding::decode(aid).ok()?.to_string();
// No validation that decoded_aid meets expected format

Issues:

• Decoded actor IDs could contain unexpected characters (newlines, null bytes, etc.)
• No length limits enforced post-decoding
• Could bypass validation if any exists elsewhere in the codebase

Recommendation:

• Add validation after decoding to ensure actor IDs match expected format
• Consider enforcing the "must be valid DNS subdomain" rule mentioned in CLAUDE.md
• Add test cases for malicious inputs (e.g., %00, %0A, etc.)

3. Query String and Fragment Not Preserved in Decoding ⚠️

Looking at the implementation, the query string and fragment handling happens before the decoding:

// Line 156-174: Extract query string and base path
// Line 201-220: Decode actor_id and token

The decoding only applies to the actor ID segment, not the entire path. This is likely correct, but:

• The test test_parse_actor_path_encoded_characters expects the remaining path to stay encoded: /api%20endpoint/test%2Fpath
• This is inconsistent - why decode actor ID but not the remaining path?

Recommendation:

• Document the design decision: Why is the remaining path not decoded?
• Ensure this matches the behavior of the downstream handlers

4. Inconsistency: Remaining Path Encoding 🔶

The test case shows:

fn test_parse_actor_path_encoded_characters() {
    let path = "/gateway/actor-123/api%20endpoint/test%2Fpath";
    assert_eq!(result.remaining_path, "/api%20endpoint/test%2Fpath");
}

This keeps the remaining path encoded, which is correct only if the downstream handler will decode it. But this is inconsistent with decoding the actor ID.

Recommendation: Clarify in documentation or comments whether downstream handlers expect encoded or decoded paths.

5. Performance: Unnecessary String Allocation 💡

Minor optimization opportunity:

let decoded_aid = urlencoding::decode(aid).ok()?.to_string();

The urlencoding::decode() returns a Cow<str>, and immediately converting to String allocates even when no decoding was needed.

Recommendation:

let decoded_aid = urlencoding::decode(aid).ok()?.into_owned();

Or accept Cow<str> in ActorPathInfo if this is a hot path.

Test Coverage

Excellent additions:

• ✓ Encoded actor ID
• ✓ Encoded token
• ✓ Encoded spaces
• ✓ Invalid encoding
• ✓ Both actor ID and token encoded

Missing test cases:

• ❌ Malicious payloads (null bytes, newlines, path traversal attempts)
• ❌ Very long encoded strings (DoS potential)
• ❌ Mixed encoded and unencoded special characters
• ❌ Integration test showing actual HTTP request flow

Additional Considerations

1. Documentation: Consider adding a comment explaining:

   • Why decoding is needed at this layer
   • What characters are expected to be encoded
   • Whether the HTTP layer has already decoded once

2. Logging: Consider adding a trace log when decoding occurs for debugging:

   tracing::trace!(original = aid, decoded = %decoded_aid, "decoded actor id");

3. TypeScript Parity: Good that both implementations match, but ensure they're tested against the same test cases.

Verdict

Overall Assessment: The PR addresses a real issue and includes good test coverage, but has a critical concern about double-decoding that needs verification before merge.

Recommendation: ⚠️ Request Changes

Action Items Before Merge:

1. 🔴 Critical: Verify whether the HTTP layer already decodes URLs
2. ⚠️ Add validation for decoded actor IDs
3. 📝 Document the decoding strategy and why only actor ID/token are decoded
4. 🧪 Add security-focused test cases
5. 💡 Consider the performance optimization

Let me know if you need help investigating the HTTP layer behavior or implementing any of these suggestions!

[claude]

Pull Request Review

This PR fixes URL decoding for actor IDs and tokens in the gateway routing logic. The changes look solid overall with good test coverage. Here are my findings:

✅ Positive Points

1. Good Test Coverage: Both Rust and TypeScript implementations include comprehensive tests for URL-encoded characters, including:

   • Encoded characters in actor_id
   • Encoded characters in token
   • Encoded characters in both
   • Encoded spaces
   • Invalid encoding rejection

2. Consistent Implementation: The fix is applied consistently across both Rust (guard package) and TypeScript (rivetkit) implementations

3. Proper Error Handling:

   • Rust: Uses .ok()? to propagate errors gracefully
   • TypeScript: Uses try-catch to handle decodeURIComponent errors

4. Follows Workspace Conventions: Correctly added urlencoding as a workspace dependency

🔍 Code Quality Observations

Rust Implementation (engine/packages/guard/src/routing/mod.rs:208-220)

• Clean and idiomatic use of the urlencoding crate
• Proper error propagation using ? operator
• Good symmetry in handling both token and non-token cases

TypeScript Implementation (rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts:399-428)

• Appropriate use of decodeURIComponent (standard library)
• Proper error handling with try-catch blocks
• Clear variable naming (rawActorId, rawToken)

🤔 Potential Concerns & Questions

1. Double Decoding Risk: The remaining path is NOT being decoded - is this intentional?

   • In line 227-238 of the Rust code, remaining_path is constructed from the original base_path without decoding
   • This appears intentional since the remaining path will be passed to the downstream service which may need to decode it themselves
   • Question: Should this behavior be documented? It's a bit subtle that actor_id/token get decoded but the path doesn't.

2. Test Coverage Gap - Remaining Path:

   • Tests verify encoded characters in remaining path are preserved (/api%20endpoint/test%2Fpath)
   • But there's no explicit test documenting that this is intentional behavior
   • Consider adding a test comment explaining why remaining path stays encoded

3. Security - No Length Limits:

   • URL decoding could potentially expand the string size
   • While unlikely to be a practical issue, decoded strings have no length validation
   • Recommendation: Consider if there should be max length checks on decoded actor_id/token

4. Edge Case - Multiple @ Symbols:

   • Code uses find('@') / indexOf("@") which finds the first occurrence
   • If an actor_id contains @, it would be incorrectly split
   • Example: actor%40id@token would decode to actor@id but be split at first @
   • Question: Is this scenario possible? Should actor_ids be validated to not contain @ even when encoded?

🔒 Security Considerations

1. URL Decoding is Generally Safe: Using established libraries (urlencoding crate, decodeURIComponent) mitigates most risks
2. Invalid Encoding Rejection: Good that invalid encodings return None/null rather than panicking
3. No Injection Risks Introduced: The decoded values are used for routing, not executed or interpreted as code

⚡ Performance Considerations

• Minimal performance impact - URL decoding is fast
• Decoding happens only once per request during routing
• No concerns here

📝 Minor Suggestions

1. Consider Adding a Comment explaining why remaining_path is not decoded:

   // Note: remaining_path is not URL-decoded here as it will be passed to the
   // downstream service which may need to decode it in its own context
   let remaining_path = if remaining_base.is_empty() || !remaining_base.starts_with('/') {

2. Test Name Consistency: The test names are clear and follow good conventions

🎯 Verdict

Approved with minor suggestions ✅

The code correctly fixes the URL decoding issue for actor IDs and tokens. The implementation is clean, well-tested, and follows the repository's conventions. The only questions are around edge cases and documentation of intentional behavior regarding the remaining path not being decoded.

Action Items (Optional)

• Consider documenting why remaining_path stays encoded
• Consider if multiple @ symbols in actor_id is a valid edge case to handle
• Consider adding max length validation on decoded strings (low priority)

[claude]

Code Review: URL Decoding for Actor ID and Token

Thank you for this PR! This is a well-implemented fix that addresses URL encoding issues in actor path parsing. Here's my comprehensive review:

---

✅ Strengths

1. Correct Implementation

• Both Rust and TypeScript implementations properly decode URL-encoded characters in actor_id and token
• The placement of decoding after validation but before usage is correct
• Consistent implementation across both languages

2. Excellent Error Handling

Rust (mod.rs:212-220):

let decoded_aid = urlencoding::decode(aid).ok()?.to_string();

• Uses .ok()? pattern to gracefully handle invalid encoding
• Returns None on decode failure, preventing partial/invalid data propagation
• Consistent with existing error handling patterns in the codebase

TypeScript (gateway.ts:410-417):

try {
    actorId = decodeURIComponent(rawActorId);
    token = decodeURIComponent(rawToken);
} catch (e) {
    return null;  // Invalid URL encoding
}

• Proper try-catch for decode errors
• Returns null to indicate parsing failure

3. Comprehensive Test Coverage

Both implementations have excellent test coverage:

• ✅ Encoded hyphens in actor_id (%2D)
• ✅ Encoded @ symbols in token (%40)
• ✅ Encoded spaces (%20)
• ✅ Invalid encoding rejection (%ZZ, %GG)
• ✅ Combined encoding scenarios

4. Proper Dependency Management

• Added urlencoding to workspace dependencies in Cargo.toml
• Used workspace dependency with .workspace = true pattern (follows CLAUDE.md guidelines)

5. Security Considerations

• Validation happens before decoding (empty checks, path structure)
• Decoding happens after initial validation but before usage
• Invalid encoding is rejected rather than partially processed
• Actor ID undergoes additional validation via Id::parse() in pegboard_gateway.rs

---

🔍 Observations & Considerations

1. Remaining Path Not Decoded (Intentional?)

The remaining_path is not URL-decoded in either implementation:

Rust (mod.rs:115):

assert_eq\!(result.remaining_path, "/api%20endpoint/test%2Fpath");

Analysis: This appears intentional and correct:

• The remaining path is passed through to proxied services
• Services should handle their own URL decoding for their endpoints
• Decoding too early could cause double-decoding issues
• Preserves original encoding for proper HTTP forwarding

✅ This is correct behavior

2. Token Validation Deferred

From pegboard_gateway.rs:113:

// NOTE: Token validation implemented in EE

The decoded token string is not validated for format/content at the guard level. This is consistent with the existing architecture where token validation happens in the Enterprise Edition.

3. Error Observability

Currently, invalid URL encoding silently returns None/null, making it indistinguishable from other parsing failures. Consider whether adding specific logging would help debugging:

let decoded_aid = match urlencoding::decode(aid) {
    Ok(s) => s.to_string(),
    Err(e) => {
        tracing::debug\!(actor_segment = aid, "failed to decode actor_id");
        return None;
    }
};

However, this is not critical since:

• The guard already logs routing decisions at debug level
• Invalid requests will hit the NoRoute error path
• Existing code doesn't log validation failures either

---

🎯 Best Practices Compliance

✅ Follows CLAUDE.md Guidelines:

• Uses workspace dependencies correctly
• Follows existing error handling patterns (.ok()?)
• Includes comprehensive tests
• Maintains consistent code style (hard tabs, structured logging ready)
• No unnecessary formatting changes

✅ Security Best Practices:

• Input validation before processing
• Graceful error handling
• No partial data exposure on decode failures
• Downstream validation still applies (Id::parse)

---

📊 Performance Considerations

Impact: Minimal

• URL decoding is O(n) where n is string length
• Only applied to actor_id and token (typically short strings)
• Only runs when path matches /gateway/... pattern
• No additional allocations beyond necessary string creation

---

🔒 Security Assessment

✅ No Security Concerns Identified

Attack Surface Analysis:

1. Injection Attacks: Not applicable - decoded strings undergo strict validation

   • Actor IDs: Base36 validation via Id::parse() (30 chars, [0-9a-z] only)
   • Tokens: Validated in EE (noted in comments)

2. DoS via Malformed Encoding: Mitigated

   • Invalid encoding returns early (None/null)
   • No resource-intensive retry logic
   • No allocation on invalid input

3. Path Traversal: Not applicable

   • Decoding only affects actor_id and token segments
   • Remaining path structure preserved
   • Validation checks prevent empty/invalid segments

4. Double Decoding: Prevented

   • Decoding happens once per segment
   • Remaining path explicitly not decoded

---

📝 Suggestions (Optional)

1. Consider Adding Tracing for Failed Decodes (Optional)

For better observability during debugging:

let decoded_aid = urlencoding::decode(aid).map_err(|e| {
    tracing::debug\!(?aid, "invalid url encoding in actor_id");
    e
}).ok()?.to_string();

Priority: Low - Nice to have but not critical

2. Document Encoding Behavior (Optional)

Consider adding a comment in the function documentation about encoding expectations:

/// Parse actor routing information from path
/// Matches patterns:
/// - /gateway/{actor_id}/{...path}
/// - /gateway/{actor_id}@{token}/{...path}
///
/// Note: actor_id and token are URL-decoded. remaining_path is NOT decoded
/// to allow proper forwarding to downstream services.

Priority: Low - The behavior is clear from tests

---

✅ Final Verdict: LGTM

This PR is well-implemented, thoroughly tested, and ready to merge. The changes:

• ✅ Fix the stated bug (URL-encoded actor IDs and tokens now work)
• ✅ Maintain backward compatibility (non-encoded paths still work)
• ✅ Follow security best practices
• ✅ Include comprehensive test coverage
• ✅ Have minimal performance impact
• ✅ Follow project conventions

No blocking issues identified. The optional suggestions above are minor enhancements that could be addressed in future PRs if desired.

---

Test Results Expected:

• ✅ All new tests pass (both Rust and TypeScript)
• ✅ Existing tests remain passing (backward compatible)
• ✅ URL-encoded special characters now handled correctly

Great work on fixing this issue! 🎉

[claude]

PR Review: fix(guard): fix url decoding actor id + token

Summary

This PR addresses URL decoding issues for actor IDs and tokens in path-based routing, ensuring that URL-encoded characters are properly handled in both the Rust guard package and TypeScript rivetkit implementation.

Code Quality ✅

Rust Implementation (engine/packages/guard/)

Strengths:

• Clean implementation using the urlencoding crate for proper URL decoding
• Correctly added as a workspace dependency following the project's dependency management patterns
• Proper error handling using Option return type with ? operator for decode failures
• Decoding applied to both actor_id and token in all code paths

Areas of Concern:

1. Inconsistent decoding approach (engine/packages/guard/src/routing/mod.rs:212-219):

   • The code decodes the actor_id and token segments AFTER splitting on @
   • This could be problematic if the @ symbol itself is URL-encoded in the actor_id
   • Example issue: An actor_id like "user%40example" (user@example encoded) would be incorrectly split at the encoded %40, not recognizing it as part of the ID

   Recommendation: Consider whether the splitting should happen after decoding the entire segment, or if @ in actor IDs should be prohibited/escaped differently.

2. Silent failure on decode errors:

   • When urlencoding::decode() fails, it returns None via the ? operator
   • While this is correct, there's no logging to help debug URL encoding issues

   Suggestion: Add a debug/trace log when decode fails:

   let decoded_aid = urlencoding::decode(aid).inspect_err(|e| {
       tracing::debug!(?e, aid, "failed to decode actor_id");
   }).ok()?.to_string();

TypeScript Implementation (rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts)

Strengths:

• Proper use of decodeURIComponent for URL decoding
• Good error handling with try-catch blocks that return null on invalid encoding
• Consistent with Rust implementation approach
• Validates empty strings before attempting decode

Note: TypeScript has the same potential issue with splitting on @ before decoding.

Test Coverage ✅

Excellent test coverage! Both Rust and TypeScript test suites include:

• ✅ URL-encoded characters in actor_id
• ✅ URL-encoded characters in token
• ✅ URL-encoded characters in both
• ✅ URL-encoded spaces
• ✅ Invalid URL encoding rejection

Missing test case:

• ❌ Test for encoded @ symbol within actor_id (e.g., actor%40id → should this decode to actor@id or should it split incorrectly?)

Suggestion: Add test to clarify expected behavior:

#[test]
fn test_parse_actor_path_encoded_at_in_actor_id() {
    // What should happen with %40 (encoded @) in actor_id?
    let path = "/gateway/user%40example.com/endpoint";
    let result = parse_actor_path(path);
    // Should this:
    // A) Decode to actor_id="[email protected]", token=None
    // B) Split incorrectly treating %40 as literal @ 
    // Current implementation does (B), is this intended?
}

Performance Considerations ⚠️

Concern: Double allocation in Rust code (engine/packages/guard/src/routing/mod.rs:212-213):

let decoded_aid = urlencoding::decode(aid).ok()?.to_string();
let decoded_tok = urlencoding::decode(tok).ok()?.to_string();

urlencoding::decode() returns Cow<str> which may already be owned. Calling .to_string() forces an allocation even when not needed.

Optimization:

let decoded_aid = urlencoding::decode(aid).ok()?.into_owned();
let decoded_tok = urlencoding::decode(tok).ok()?.into_owned();

This is more efficient as it reuses the owned String when available. Minor improvement but good practice in hot paths.

Security Considerations ✅

Good security practices:

• ✅ Proper validation before decoding (empty checks)
• ✅ Handles malformed URL encoding by rejecting the request
• ✅ No injection vulnerabilities introduced
• ✅ Maintains existing path traversal protections

No security concerns identified.

Potential Bugs 🐛

Critical Issue: Encoded @ Symbol Handling

As mentioned above, the current implementation splits on @ BEFORE decoding, which means:

• actor%40id@token would split into actor%40id and token
• After decoding: actor@id and token ✅ (works)
• BUT actor@id (without token) encoded as actor%40id would NOT split
• Result: actor@id treated as actor_id with no token ✅ (also works)

However:

• actor@id@token encoded as actor%40id@token would split at the second @
• Result: actor%40id and token, then decode to actor@id and token ✅

Actually, after analysis, the current approach seems correct for the case where:

1. Actor IDs can contain @ (must be encoded)
2. The @ separator between actor_id and token is never encoded

But this should be documented! Add a comment explaining this behavior.

Minor Issue: Query String Preservation

The remaining_path correctly preserves query strings, but the URL-encoded segments are decoded. This is correct behavior, but verify downstream services expect decoded values.

Recommendations

1. Add documentation comment to parse_actor_path explaining the @ encoding rules
2. Add debug logging for decode failures (helpful for troubleshooting)
3. Use into_owned() instead of to_string() for Cow types
4. Add test case for encoded @ in actor_id to document expected behavior
5. Consider logging when path parsing returns None to aid debugging

Verdict

Approval with minor suggestions ✅

The PR correctly fixes the URL decoding issue and maintains consistency across Rust and TypeScript implementations. The test coverage is excellent. The suggested improvements are minor optimizations and documentation enhancements that don't block merging.

Great work on maintaining parity between the two implementations and comprehensive testing!

[claude]

Code Review - PR #3357: fix(guard): fix url decoding actor id + token

Summary

This PR adds URL decoding for actor IDs and tokens in the path-based routing system. The implementation is consistent across both Rust (guard) and TypeScript (rivetkit) codebases.

---

✅ Strengths

1. Comprehensive Test Coverage: Excellent test additions covering multiple edge cases:

   • URL-encoded actor IDs
   • URL-encoded tokens
   • Both encoded simultaneously
   • Spaces in actor IDs
   • Invalid encoding rejection (TypeScript)

2. Consistent Implementation: The fix is applied consistently in both:

   • engine/packages/guard/src/routing/mod.rs (Rust)
   • rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts (TypeScript)

3. Proper Error Handling:

   • Rust uses .ok()? to propagate decoding errors
   • TypeScript uses try-catch to gracefully handle invalid encodings

4. Follows Dependency Conventions: Correctly added urlencoding to workspace dependencies first, then referenced with .workspace = true

---

🔍 Potential Issues & Concerns

1. Security: Double Decoding Vulnerability ⚠️ CRITICAL

The implementation decodes the actor_id/token segments, but there's a potential double-decoding issue:

Current flow:

1. HTTP server receives request with path /gateway/actor%2D123/endpoint
2. HTTP framework may already URL-decode this to /gateway/actor-123/endpoint
3. Your code decodes again: actor-123 → actor-123 (no change, but could be problematic)

Attack scenario:
If a malicious actor sends /gateway/actor%252D123/endpoint:

• First decode (by HTTP server): /gateway/actor%2D123/endpoint
• Second decode (your code): /gateway/actor-123/endpoint

This could bypass validation or cause mismatches with stored actor IDs.

Recommendation: Verify whether the HTTP framework (hyper) already performs URL decoding on the path. If it does, this change may introduce a vulnerability.

// Recommended: Check if path is already decoded
// Line 155 in routing/mod.rs - add comment documenting behavior

2. Remaining Path Not Decoded 🤔

The remaining_path is NOT decoded (as shown in test test_parse_actor_path_encoded_characters:115). This asymmetry could be confusing:

• Actor ID: decoded ✅
• Token: decoded ✅
• Remaining path: NOT decoded ❌

Question: Is this intentional? Should the remaining path segments also be decoded before routing to the actor?

3. Missing Invalid Encoding Tests in Rust

The TypeScript implementation has tests for invalid URL encoding:

test('should reject invalid URL encoding in actor_id', () => {
  const path = '/gateway/actor%ZZ123/endpoint';
  expect(parseActorPath(path)).toBeNull();
});

But the Rust tests don't have equivalent test cases. While the implementation handles it correctly with .ok()?, explicit tests would improve coverage.

Recommendation: Add Rust tests for:

#[test]
fn test_parse_actor_path_invalid_encoding() {
    // %ZZ is invalid hex
    assert\!(parse_actor_path("/gateway/actor%ZZ123/endpoint").is_none());
}

4. Performance Consideration 📊

Every request now performs URL decoding (which includes UTF-8 validation and allocation). For high-traffic scenarios:

• Consider if actor IDs actually need encoding (if they're always alphanumeric+hyphens, this may be unnecessary overhead)
• If encoding is rare, consider lazy decoding only when special characters are detected

5. Documentation Gap

The function comment at routing/mod.rs:151-154 should document:

• That actor_id and token are URL-decoded
• That remaining_path is NOT decoded (if intentional)
• Valid character expectations for actor IDs

---

🔒 Security Review

Concerns:

1. Double Decoding (see above) - Could enable bypass attacks
2. Path Traversal: URL decoding could introduce path traversal if actor IDs contain ../ after decoding
   • Example: /gateway/..%2F..%2Fetc%2Fpasswd/endpoint → /gateway/../../etc/passwd/endpoint
   • Mitigation: Validate decoded actor_id doesn't contain / or other path separators

Recommendation: Add validation after decoding:

// After line 218
if decoded_aid.contains('/') || decoded_aid.contains('\\') {
    return None;
}

---

📝 Additional Recommendations

1. Add integration tests that verify end-to-end routing with encoded paths
2. Document the expected format of actor IDs in the codebase (DNS subdomain format per CLAUDE.md suggests kebab-case)
3. Verify HTTP framework behavior regarding automatic URL decoding
4. Consider rejecting non-alphanumeric actor IDs after decoding to prevent confusion

---

🎯 Action Items

Before Merge:

• Verify hyper doesn't already decode paths (check for double-decoding vulnerability)
• Add path traversal validation after decoding
• Add invalid encoding tests for Rust
• Document decoding behavior in function comments
• Clarify whether remaining_path should be decoded

Nice to Have:

• Add integration test with actual HTTP requests
• Benchmark performance impact of URL decoding on every request
• Consider validating actor_id format after decoding

---

Overall Assessment

Quality: ⭐⭐⭐⭐ (4/5)

The PR implements a necessary fix with good test coverage and consistent cross-language implementation. However, the potential for double-decoding and lack of post-decode validation raise security concerns that should be addressed before merging.

Recommendation: Request changes to address security concerns, particularly around double-decoding and path traversal prevention.

[claude]

PR Review: URL Decoding Fix for Actor ID and Token

Summary

This PR adds URL decoding support for actor IDs and tokens in the gateway path parsing logic. The implementation is solid and addresses a real issue where URL-encoded characters in actor IDs and tokens would not be properly decoded before use.

---

✅ Strengths

1. Comprehensive Test Coverage: Excellent test coverage with multiple test cases covering:

   • Encoded hyphens (%2D)
   • Encoded @ symbols (%40)
   • Encoded spaces (%20)
   • Invalid encoding rejection
   • Both Rust and TypeScript implementations tested consistently

2. Consistent Implementation: Both Rust and TypeScript implementations follow the same approach, which is critical for maintaining consistency across the stack.

3. Error Handling: Proper error handling for invalid URL encoding:

   • Rust: Uses .ok()? to return None on decode failure
   • TypeScript: Uses try-catch to return null on decode failure

4. Dependency Management: Correctly follows the workspace pattern by adding urlencoding to workspace dependencies.

---

🔍 Code Quality Observations

Rust Implementation (engine/packages/guard/src/routing/mod.rs)

Good:

• Clean, idiomatic Rust code
• Proper use of the urlencoding crate
• Error handling via Option return type

Minor consideration:

• The urlencoding::decode() returns a Cow<str>, and calling .to_string() always allocates. For most use cases this is fine, but if you're concerned about performance, you could use into_owned() which only allocates when necessary.

TypeScript Implementation (rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts)

Good:

• Proper use of decodeURIComponent
• Error handling via try-catch
• Consistent with JavaScript/TypeScript best practices

---

🔒 Security Considerations

Positive:

1. No Injection Risk: URL decoding before use is the correct approach and doesn't introduce injection vulnerabilities since the decoded values are used as identifiers, not in SQL or command execution contexts.

2. Invalid Encoding Rejection: The code properly rejects invalid URL encoding rather than silently passing through malformed data.

3. No Double-Decoding: The implementation only decodes once, avoiding double-decoding vulnerabilities.

Note:

• The remaining path is intentionally not decoded, which is correct since it's passed to upstream services that should handle their own decoding. This prevents double-decoding issues.

---

🐛 Potential Issues

Minor: Consider Edge Cases

The code handles most edge cases well, but consider documenting the behavior for:

1. Plus signs (+): In URL encoding, + can represent spaces in query strings but not in path segments. Your implementation correctly uses decodeURIComponent/urlencoding::decode which treats + as literal +, not as space. This is correct for path segments.

2. Null bytes: If an actor ID or token contains %00 (null byte), the Rust code will decode it to a string with a null byte. Depending on downstream usage, this might cause issues. Consider adding validation if null bytes should be rejected.

---

📊 Test Coverage

Excellent coverage including:

• ✅ Basic encoded characters
• ✅ Special characters like @ in tokens
• ✅ Spaces in actor IDs
• ✅ Invalid encoding rejection
• ✅ Combined actor ID and token encoding

Suggestion: Consider adding a test for:

• Plus signs in actor IDs (e.g., /gateway/actor%2Btest/endpoint should decode to actor+test)
• Empty string encoding (e.g., %00 if it should be rejected)

---

🎯 Recommendations

1. Consider documenting the decoding behavior in code comments, specifically:

   • Why actor_id/token are decoded
   • Why remaining_path is NOT decoded

2. Minor optimization (Rust only): Consider using into_owned() instead of .to_string():

   let decoded_aid = urlencoding::decode(aid).ok()?.into_owned();

3. Future consideration: If actor IDs and tokens have format requirements (e.g., no spaces, no special characters), consider adding validation after decoding.

---

✅ Verdict

LGTM! This is a well-implemented fix with excellent test coverage. The code is clean, secure, and follows best practices for both Rust and TypeScript implementations.

The changes appropriately address URL encoding issues while maintaining security and avoiding common pitfalls like double-decoding.

[claude]

PR Review: fix(guard): fix url decoding actor id + token

Summary

This PR adds URL decoding for actor IDs and tokens in path-based routing, ensuring that URL-encoded characters in these path segments are properly decoded before being used. The fix is implemented consistently in both the Rust guard package and the TypeScript RivetKit gateway.

---

Positive Observations

1. Comprehensive Implementation

• The fix is implemented in both Rust (engine/packages/guard/src/routing/mod.rs) and TypeScript (rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts), ensuring consistent behavior across the stack.
• Proper error handling for invalid URL encoding in both implementations.

2. Excellent Test Coverage

• Added 6 new test cases covering various URL encoding scenarios:
  • Encoded characters in actor_id (actor%2D123 → actor-123)
  • Encoded characters in token (tok%40en → tok@en)
  • Encoded characters in both actor_id and token
  • Encoded spaces in actor_id
  • Invalid URL encoding rejection (in TypeScript tests)
• Tests cover both success and failure cases.

3. Proper Dependency Management

• Correctly added urlencoding to workspace dependencies in Cargo.toml.
• Used workspace dependency pattern (urlencoding.workspace = true).

---

Issues and Suggestions

1. Security Concern: Missing Validation After Decoding (HIGH PRIORITY)

The code decodes actor IDs and tokens but doesn't validate the decoded values. This could allow problematic characters that were URL-encoded to bypass validation:

In engine/packages/guard/src/routing/mod.rs:211-219:

// URL-decode both actor_id and token
let decoded_aid = urlencoding::decode(aid).ok()?.to_string();
let decoded_tok = urlencoding::decode(tok).ok()?.to_string();

(decoded_aid, Some(decoded_tok))

Recommendation: Add validation after decoding to ensure actor IDs and tokens contain only allowed characters. Consider:

• What characters are valid in an actor ID? (typically alphanumeric, hyphens, underscores)
• Should newlines, null bytes, or control characters be rejected?
• Are there length limits that should be enforced?

Example validation:

// After decoding
let decoded_aid = urlencoding::decode(aid).ok()?.to_string();
if decoded_aid.is_empty() || decoded_aid.chars().any(|c| c.is_control()) {
    return None;
}

2. Inconsistent Encoding Handling for Remaining Path

The code now URL-decodes the actor_id and token, but NOT the remaining path. This is inconsistent and potentially confusing:

In engine/packages/guard/src/routing/mod.rs:232-238:

let remaining_path = if remaining_base.is_empty() || !remaining_base.starts_with('/') {
    format!("/{}{}", remaining_base, query_string)
} else {
    format!("{}{}", remaining_base, query_string)
};

Question for consideration: Should the remaining path segments also be URL-decoded? This depends on your routing architecture:

• If the downstream service expects URL-encoded paths, keep as-is (but document this).
• If the downstream service expects decoded paths, you should decode here.

Recommendation: Add a comment explaining why remaining_path is NOT decoded, or decode it if appropriate.

3. Test Coverage Gap: @ Symbol in Actor ID

The tests don't verify what happens when an actor ID itself contains a @ symbol that's URL-encoded. For example:

• Path: /gateway/actor%40id/endpoint
• Expected: actor_id = "actor@id", token = None

However, since the code splits on the FIRST @ character (line 202), this would incorrectly parse as:

• actor_id = "actor", token = "id" (after decoding)

Recommendation: Add a test case for this scenario and document the expected behavior. If @ symbols in actor IDs are valid, you may need to adjust the parsing logic.

4. Minor: Decoding Before Splitting vs After

Current implementation splits on @ first, then decodes each part. This means:

• /gateway/actor%40id@token → splits to ["actor%40id", "token"] → decodes to ["actor@id", "token"]

This is likely correct (treating the literal @ as the delimiter), but consider:

• /gateway/actor@tok%40en → actor_id="actor", token="tok@en" ✓

If this is the intended behavior, it's fine. Just ensure it's documented.

5. Code Organization: Repeated Decoding Logic

In the Rust code, the decoding logic is duplicated in two branches (lines 212-213 and line 218). Consider extracting to a helper:

fn decode_path_segment(segment: &str) -> Option<String> {
    urlencoding::decode(segment).ok().map(|s| s.to_string())
}

Then use:

let decoded_aid = decode_path_segment(aid)?;
let decoded_tok = decode_path_segment(tok)?;

This improves maintainability and ensures consistent handling.

---

Performance Considerations

The urlencoding::decode() function returns a Cow<str>, which avoids allocating a new string if no decoding is needed. However, the code immediately calls .to_string(), which always allocates. Consider:

let decoded_aid = urlencoding::decode(aid).ok()?;
// Only convert to String when needed
(decoded_aid.to_string(), Some(decoded_tok.to_string()))

This is a micro-optimization but could be beneficial if this code path is hit frequently.

---

Testing Recommendations

Consider adding these additional test cases:

1. Double-encoded values:

   • /gateway/actor%252Did/endpoint → Should decode to actor%2Did (single encoding) or actor-id (double decoding)?

2. Plus signs vs %20 for spaces:

   • /gateway/actor+id/endpoint → actor+id or actor id?
   • Note: In URL path segments, + is NOT decoded to space (only in query strings)

3. Special characters that might be problematic:

   • /gateway/actor%00id/endpoint (null byte)
   • /gateway/actor%0Aid/endpoint (newline)
   • /gateway/actor%2F%2Fid/endpoint (double slash)

4. @ symbol in actor_id (as mentioned above):

   • /gateway/actor%40id/endpoint

---

Documentation

Consider adding comments to explain:

1. Why URL decoding is performed on actor_id/token but not on remaining_path
2. The order of operations: split first, then decode (and why)
3. Valid characters in actor IDs and tokens after decoding

---

Conclusion

Overall Assessment: This is a solid fix that addresses a real bug. The implementation is consistent across Rust and TypeScript, with good test coverage.

Blockers:

• ⚠️ Security validation after decoding should be addressed before merging

Recommendations:

• Add validation for decoded actor_id and token values
• Add test case for @ symbol in actor_id
• Consider extracting decoding logic to helper function
• Document encoding/decoding behavior

Nice to Have:

• Additional edge case tests (double encoding, null bytes, etc.)
• Performance optimization (avoid unnecessary .to_string() calls)
• Documentation comments

Let me know if you'd like me to elaborate on any of these points or provide code examples for the recommended changes!

[claude]

PR Review: fix(guard): fix url decoding actor id + token

Summary

This PR adds URL decoding support for actor IDs and tokens in path-based routing for both the Rust guard package and TypeScript RivetKit. The implementation is generally solid with good test coverage.

---

Strengths

1. Comprehensive Test Coverage: Both Rust and TypeScript implementations include extensive test cases covering basic URL encoding scenarios, edge cases (spaces, special characters), invalid encoding detection, and combined encoding in actor_id and token

2. Consistent Implementation: The fix is applied symmetrically in both Rust (engine/packages/guard/src/routing/mod.rs) and TypeScript (rivetkit-typescript/packages/rivetkit/src/manager/gateway.ts)

3. Proper Error Handling: Both implementations gracefully handle invalid URL encoding by returning None/null

4. Follows Project Conventions: Uses workspace dependencies, proper structured logging in TypeScript, and good test organization

---

Potential Issues and Recommendations

1. Security: Path Traversal Risk (Priority: Medium)

After URL decoding, there is no validation that decoded values don't contain path traversal sequences. An attacker could send /gateway/actor%2F..%2Fadmin@token/endpoint which decodes to actor/../admin

Recommendation: Add validation after decoding to reject actor_ids containing path separators, parent directory references (..), or null bytes.

2. Inconsistent Decoding (Priority: High)

The implementation only decodes actor_id and token segments, but NOT the remaining_path. The test at line 111-116 shows encoded characters in remaining path are preserved. Is this intentional?

Recommendation: Add a comment explaining why remaining_path is intentionally not decoded, or verify that downstream handlers properly decode it.

3. Token Validation Missing (Priority: Low)

Similar to actor_id, the decoded token should also be validated. The comment mentions Token validation implemented in EE - ensure the EE implementation validates decoded tokens.

4. Test Gap: Invalid Characters After Decoding (Priority: Low)

The test suite validates invalid URL encoding but doesn't test for dangerous characters that appear after successful decoding. Recommend adding tests for path traversal attempts and null bytes.

---

Testing Recommendations

1. Integration tests that exercise the full routing pipeline with encoded actor IDs
2. Fuzz testing to catch edge cases
3. Cross-language consistency tests

---

Performance Considerations

URL decoding is computationally inexpensive with efficient string operations and no blocking operations. Performance impact is negligible.

---

Verdict

Approve with minor recommendations

The core functionality is solid and well-tested. Main concerns:

1. Potential security issues with path traversal (medium priority - should be addressed)
2. Clarification needed on whether remaining_path should be decoded

The fix correctly addresses the original issue of URL-encoded actor IDs and tokens not being decoded properly.

---

Suggested Action Items

• Add validation to reject path traversal sequences after decoding
• Add test cases for malicious input patterns
• Add comment explaining why remaining_path is not decoded
• Verify downstream handlers properly handle the encoding in remaining_path

[graphite-app]

Merge activity

• Nov 5, 5:07 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 5, 5:08 AM UTC: CI is running for this pull request on a draft pull request (#3371) due to your merge queue CI optimization settings.
• Nov 5, 5:11 AM UTC: Merged by the Graphite merge queue via draft PR: #3371.