feat: support Authentication and Authorization for the proxy#41
feat: support Authentication and Authorization for the proxy#41jiashengguo wants to merge 8 commits into
Conversation
- Introduced `--studioAuthKey` option for authentication key from ZenStack Studio. - Added `--signatureToleranceSecs` option to configure maximum age of signed requests. - Implemented `createSignatureMiddleware` to verify ed25519 signatures on incoming requests. - Normalized public key format for signature verification. - Enhanced server options to include signature tolerance and public API key. - Updated request handling to enforce signature verification for protected routes. - Added comprehensive tests for signature verification and middleware functionality.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 33483236 | Triggered | Generic High Entropy Secret | 649f9b4 | src/signature-verifier.test.ts | View secret |
| 33485392 | Triggered | Generic Private Key | febf960 | test/signature.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Warning Review limit reached
More reviews will be available in 10 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Ed25519 signed-request verification to the ZenStack proxy. A new ChangesSigned Request Verification Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds request-signature verification and basic Authorization-claim handling to secure the proxy’s API endpoints.
Changes:
- Introduces Ed25519 request signature utilities + Express middleware for
x-zenstack-signature. - Adds server-side gating for signed requests (public key + timestamp tolerance) and resolves enhanced Prisma client behavior based on
Authorizationbearer claims. - Adds tests/docs and updates dependencies/scripts to support the new signature middleware.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/signature.test.ts | End-to-end tests for key normalization, signature verification, and middleware behavior. |
| src/signature.ts | Implements public key normalization, Ed25519 signature verification, and Express signature middleware. |
| src/signature-verifier.ts | Adds a standalone signature verification helper (non-Express) for payload+timestamp validation. |
| src/signature-verifier.test.ts | Adds unit tests for signature-verifier (currently located under src/). |
| src/server.ts | Wires signature middleware into /api/model and /api/schema; adds Authorization-claim based client resolution. |
| src/index.ts | Adds CLI options for auth key and signature tolerance; passes options into startServer. |
| README.md | Documents the signed request header format and payload construction. |
| package.json | Bumps version; adds test script and supertest dev deps. |
| pnpm-lock.yaml | Updates lockfile for new dev dependencies (includes a vitest entry). |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/server.ts (1)
417-435: ⚡ Quick winConsider adding Helmet middleware for security headers.
The static analysis tool flags that this Express app doesn't use Helmet. Helmet sets various HTTP headers that help protect against common web vulnerabilities (XSS, clickjacking, etc.). While not strictly required for an API proxy, it's a low-effort security improvement.
+import helmet from 'helmet' // ... const app = express() +app.use(helmet()) app.use(cors())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 417 - 435, The Express app initialization is missing Helmet middleware, which provides important HTTP security headers to protect against common web vulnerabilities. Import the helmet package and add it as middleware early in the chain by calling app.use(helmet()) right after the express app is created (after the line const app = express()). Place this before or alongside the other middleware like cors() to ensure security headers are applied to all responses.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 53-57: The pnpm-lock.yaml file is out of sync with the updated
dependencies in package.json (specifically the versions of supertest,
typescript, rimraf, copyfiles, and `@types/supertest`). Regenerate the lockfile by
running pnpm install to update pnpm-lock.yaml with the new dependency
specifications, then commit the updated lockfile to the repository to resolve
the CI installation failures.
- Line 22: The "test" script in package.json is too narrow and only runs
test/signature.test.ts, which excludes the src/signature-verifier.test.ts test
suite. Update the "test" script to run both test files instead of just one, so
that all signature-related tests execute in CI. This ensures the complete test
coverage of the newly added signature verification logic.
In `@README.md`:
- Around line 57-61: The signed message format documentation in README.md is
incomplete and does not account for the Authorization token. Update the
documentation to clarify that when a Bearer token is present in the
Authorization header, it must be included in the signed message format.
Specifically, modify the explanation of the signed message format (currently
documented as `payload + timestamp`) to indicate that the format is actually
`payload + timestamp + bearerToken` (or similar notation) when an Authorization
header with a Bearer token is present, and explain what happens when no
Authorization header is provided. This ensures clients implementing the
signature verification will generate valid signatures for both authenticated and
unauthenticated requests.
In `@src/index.ts`:
- Around line 28-31: The `--studioAuthKey` option is defined in the CLI parser
but is never forwarded to the `startServer` function, making it a dead option
that has no effect. Locate where the option is parsed from the command line
arguments and either: (1) extract the studioAuthKey value and pass it to the
`startServer` function call alongside other parameters like `publicApiKey`, or
(2) if the feature is not yet implemented, remove the entire `--studioAuthKey`
option definition to avoid confusing users. Choose based on whether this
authentication feature is intended to be supported.
In `@src/server.ts`:
- Line 383: The variable publicAPIKey is being destructured from options on line
383 and then re-declared with const on line 407, causing a duplicate variable
declaration error. Remove publicAPIKey from the destructuring statement on line
383 (where zenstackPath, port, zmodelConfig, and zmodelSchemaDir are being
extracted) so that publicAPIKey is only declared once when it is later declared
on line 407.
- Around line 302-308: In the conditional block where claim.type === 'user', add
validation to ensure that claim.data exists and is a non-null object before
passing it to the enhanceFunc call. Add a check after the type comparison to
verify claim.data is not undefined or null, and only proceed with the
enhanceFunc invocation if the validation passes. This prevents creating an
enhanced client with an undefined user identity context.
In `@src/signature.ts`:
- Around line 70-77: The signature header parsing in the code that finds
timestampPart and sigPart fails for valid headers with spaces after commas
(e.g., "t=..., v1=...") because the parts are not trimmed after splitting. When
the header contains a space, the part check with startsWith will fail. Add
trim() calls to each part when finding the timestampPart (checking for 't=') and
sigPart (checking for 'v1=') to remove any leading or trailing whitespace before
performing the startsWith checks.
- Around line 97-99: The `authHeader` variable (from
`req.headers['authorization']`) can be a string, string array, or undefined per
Node.js/Express type definitions, but the code only checks for truthiness before
calling `.startsWith()`, which throws a TypeError if the header is an array. Add
a type guard to verify that the header is a string (using `typeof authHeader ===
'string'`) before calling `.startsWith('Bearer ')`. Apply the same fix to the
`x-signature` header extraction that follows, which has the identical
vulnerability.
---
Nitpick comments:
In `@src/server.ts`:
- Around line 417-435: The Express app initialization is missing Helmet
middleware, which provides important HTTP security headers to protect against
common web vulnerabilities. Import the helmet package and add it as middleware
early in the chain by calling app.use(helmet()) right after the express app is
created (after the line const app = express()). Place this before or alongside
the other middleware like cors() to ensure security headers are applied to all
responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26fd14e4-c4e4-4fec-aea5-22170d85c158
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
README.mdpackage.jsonsrc/index.tssrc/server.tssrc/signature-verifier.test.tssrc/signature-verifier.tssrc/signature.tstest/signature.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… related examples
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/index.ts (1)
32-42:⚠️ Potential issue | 🟠 MajorAlign the CLI flag with docs and use strict integer validation.
The CLI registers
--signatureToleranceSecsbut the README documents--signature-tolerance-secs. Users following the docs will get an unrecognized option error. Additionally,parseIntaccepts partial strings like"10s"(parses to10) and"1.5"(parses to1). Use kebab-case to match CLI conventions and validate withNumber.isInteger()for strict integer checking.Proposed fix
new Option( - '--signatureToleranceSecs <seconds>', + '--signature-tolerance-secs <seconds>', 'Maximum age (in seconds) of a signed request before it is rejected as a replay. Defaults to 60.' ) .default(60) .argParser((v) => { - const parsed = parseInt(v, 10) - if (isNaN(parsed) || parsed < 0) { - throw new CliError(`--signatureToleranceSecs must be a positive integer, got: ${v}`) + const parsed = Number(v) + if (!Number.isInteger(parsed) || parsed < 0) { + throw new CliError(`--signature-tolerance-secs must be a non-negative integer, got: ${v}`) } return parsed })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 32 - 42, The CLI flag `--signatureToleranceSecs` uses camelCase but the README documents it in kebab-case as `--signature-tolerance-secs`, causing a mismatch. Additionally, the argParser using parseInt is too lenient and accepts partial strings like "10s" and decimals like "1.5". Change the flag name in the Option constructor from `--signatureToleranceSecs` to `--signature-tolerance-secs` to match CLI conventions and the documentation. In the argParser function, replace the parseInt validation logic to use Number.isInteger() for strict integer validation instead of relying on parseInt which silently truncates invalid inputs.src/server.ts (1)
421-434:⚠️ Potential issue | 🟠 MajorAdd
verifycallback toexpress.urlencodedmiddleware to capturerawBody.The
express.urlencodedparser mounted at line 429 does not capturerawBody, but runs before signature verification. When non-GET/DELETE requests withapplication/x-www-form-urlencodedcontent reach the signature middleware, it will verify the signature against an empty string instead of the actual body (line 93 in src/signature.ts:req.rawBody ?? ''), causing signature validation to fail or be bypassed.Proposed fix
+ const captureRawBody = (req: express.Request, _res: express.Response, buf: Buffer) => { + ;(req as express.Request & { rawBody?: string }).rawBody = buf.toString('utf8') + } + app.use( express.json({ limit: '5mb', - verify: (req, _res, buf) => { - // Capture the raw body string for use in signature verification. - ;(req as express.Request & { rawBody?: string }).rawBody = buf.toString('utf8') - }, + verify: captureRawBody, }) ) - app.use(express.urlencoded({ extended: true, limit: '5mb' })) + app.use(express.urlencoded({ extended: true, limit: '5mb', verify: captureRawBody }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 421 - 434, The express.urlencoded middleware at line 429 is missing a verify callback to capture the raw body string. Currently, only express.json has this callback that sets req.rawBody. When form-urlencoded requests are processed, the signature middleware (as referenced in src/signature.ts line 93) will use an empty string for verification instead of the actual body content, causing signature validation to fail. Add a verify callback to the express.urlencoded middleware configuration with the same pattern used in express.json - capture the buffer and store it as a string in req.rawBody using the same casting approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server.ts`:
- Line 20: Make the `signatureToleranceSecs` property optional by adding a
question mark to its declaration (change from `signatureToleranceSecs: number`
to `signatureToleranceSecs?: number`). This aligns with the runtime behavior
where `startServer` already has a fallback to 60 seconds when the value is not
provided, and ensures programmatic callers are not forced to provide this value
at compile-time.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 32-42: The CLI flag `--signatureToleranceSecs` uses camelCase but
the README documents it in kebab-case as `--signature-tolerance-secs`, causing a
mismatch. Additionally, the argParser using parseInt is too lenient and accepts
partial strings like "10s" and decimals like "1.5". Change the flag name in the
Option constructor from `--signatureToleranceSecs` to
`--signature-tolerance-secs` to match CLI conventions and the documentation. In
the argParser function, replace the parseInt validation logic to use
Number.isInteger() for strict integer validation instead of relying on parseInt
which silently truncates invalid inputs.
In `@src/server.ts`:
- Around line 421-434: The express.urlencoded middleware at line 429 is missing
a verify callback to capture the raw body string. Currently, only express.json
has this callback that sets req.rawBody. When form-urlencoded requests are
processed, the signature middleware (as referenced in src/signature.ts line 93)
will use an empty string for verification instead of the actual body content,
causing signature validation to fail. Add a verify callback to the
express.urlencoded middleware configuration with the same pattern used in
express.json - capture the buffer and store it as a string in req.rawBody using
the same casting approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ba050e2-4ed3-4788-b80a-a551bd7ff683
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.github/workflows/build-test.ymlREADME.mdpackage.jsonsrc/index.tssrc/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
| zmodelSchemaDir: string | ||
| logLevel?: string[] | ||
| studioAuthKey?: string | ||
| signatureToleranceSecs: number |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect startServer/ServerOptions usage and tolerance configuration.
# Expect: ServerOptions callers should not need to pass signatureToleranceSecs unless they override the default.
rg -n -C3 '\bstartServer\s*\(|\bServerOptions\b|signatureToleranceSecs' --type tsRepository: zenstackhq/proxy
Length of output: 2826
🏁 Script executed:
# Search for all startServer function calls across the entire codebase
rg -n 'startServer\s*\(' --type tsRepository: zenstackhq/proxy
Length of output: 180
🏁 Script executed:
# Check if there are test files or examples that call startServer
fd -e 'ts' -e 'tsx' -e 'js' | xargs rg -l 'startServer' | head -20Repository: zenstackhq/proxy
Length of output: 87
Keep signatureToleranceSecs optional to match the runtime default.
startServer already falls back to 60 at line 432 with options.signatureToleranceSecs ?? 60, so making this exported option required creates an unnecessary compile-time break for programmatic callers. The only current caller (CLI) always provides the value anyway due to its default, so making the field optional does not affect existing usage and improves the API contract.
Proposed fix
- signatureToleranceSecs: number
+ signatureToleranceSecs?: number📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| signatureToleranceSecs: number | |
| signatureToleranceSecs?: number |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server.ts` at line 20, Make the `signatureToleranceSecs` property
optional by adding a question mark to its declaration (change from
`signatureToleranceSecs: number` to `signatureToleranceSecs?: number`). This
aligns with the runtime behavior where `startServer` already has a fallback to
60 seconds when the value is not provided, and ensures programmatic callers are
not forced to provide this value at compile-time.
Summary by CodeRabbit
New Features
--studioAuthKeyCLI option--signature-tolerance-secsconfiguration option (default 60 seconds)X-ZenStack-Signatureheader for authenticationChores