feat(core): support routing via query params #1418
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via 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: Support routing via query paramsCritical Security Concern 🔴This PR introduces a serious security vulnerability by allowing sensitive tokens to be passed via query parameters, which directly contradicts an existing security comment in the codebase. In // IMPORTANT: Params must be in headers or in an E2EE part of the request (i.e. NOT the URL or query string)
// in order to ensure that tokens can be securely passed in params.The ProblemThe PR adds fallback support for these parameters via query strings:
Why this is dangerous:
Recommended FixOption 1: Remove token parameters from query param fallback (Recommended) // Fallback to query parameters if not provided via protocols
// SECURITY: Do NOT allow tokens/params via query string
target = target || queryParams.get("x_rivet_target");
actorId = actorId || queryParams.get("x_rivet_actor");
encodingRaw = encodingRaw || queryParams.get("x_rivet_encoding");
// Explicitly omit connParamsRaw, connIdRaw, connTokenRaw from query fallbackOption 2: Add explicit security validation
Code Quality Issues1. Potential URL Parsing BugLocation: The dummy base URL approach may fail with relative URLs in certain edge cases: const urlObj = new URL(url, "http://dummy");Issue: If Recommendation: Use Hono's built-in query parsing instead: const queryParams = new Map<string, string>();
for (const [key, value] of Object.entries(c.req.query())) {
queryParams.set(key, value);
}2. Silent Error HandlingLocation: Line 223-224 } catch {
// If URL parsing fails, return empty params
}Issue: Silently swallowing errors makes debugging difficult. If URL parsing fails, it likely indicates a real problem. Recommendation: Add logging: } catch (error) {
logger().warn({ msg: "failed to parse query params", url, error });
}3. Inconsistent Parameter NamingLocation: Lines 115-120 The query parameter names use underscores (
Recommendation: Document this naming convention or consider using consistent naming (e.g., converting underscores to hyphens when reading from query params). Missing Test CoverageNo tests were added for this new functionality. The following scenarios should be tested:
Performance ConsiderationsURL parsing on every request: The new Recommendation: Only parse query params when fallback is actually needed: // Check headers first
const target = c.req.header(HEADER_RIVET_TARGET);
const actorId = c.req.header(HEADER_RIVET_ACTOR);
// Only parse query params if headers are missing
if (!target || !actorId) {
const queryParams = parseQueryParams(c.req.url);
target = target || queryParams.get("x_rivet_target");
actorId = actorId || queryParams.get("x_rivet_actor");
}This avoids unnecessary URL parsing in the common case where headers are provided. Documentation Gaps
Summary
Recommendation: Do not merge until the security concern is addressed. At minimum, remove token-related parameters from query param fallback. Review generated based on RivetKit coding standards from CLAUDE.md |
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
commit: |
1 participant
No description provided.