feat: implement signedConfig and signature support for HMAC auth #129
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAuthentication changed from direct API key/access token to an HMAC-signed bundle: components now accept Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as App (Browser)
participant Signer as Signing Server (/api/sign or dev plugin)
participant Terminal as AblyCliTerminal (Client component)
participant Backend as Terminal Backend / Control Server
Browser->>Signer: POST { apiKey, bypassRateLimit? }
Signer-->>Browser: 200 { signedConfig, signature }
Browser->>Browser: store signedConfig & signature (local/session)
Browser->>Terminal: render with signedConfig + signature props
Terminal->>Backend: createAuthPayload { config: signedConfig, signature }
Backend-->>Terminal: auth response / connection established
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/react-web-cli/src/AblyCliTerminal.tsx (1)
2644-2724: Guard session-resume hashing against invalidsignedConfig.If
signedConfigparsing fails, hashing(undefined, undefined)risks producing a constant hash, which can lead to surprising session restore/clear behavior. Consider treating parse failure as “credentials invalid”: don’t attempt restore (and maybe surface a user-visible error rather than continuing).packages/react-web-cli/src/terminal-shared.ts (1)
161-177: Remove credential duplication from AuthPayload—extract only from signed config on server side.The
createAuthPayloadfunction (lines 206–215) extractsapiKeyandaccessTokenfrom the signedconfigand populates them as unsigned fields in the payload. This duplicates sensitive credentials outside the signed blob, creating unnecessary attack surface.Recommended fix: Stop extracting and copying these fields. Instead, send only
configandsignatureto the server, and let the server derive credentials after verifying the signature. If server convenience requires the unsigned copies, add a comment explicitly stating that the server validates or cross-checks them against the signed config to prevent tampering.
🤖 Fix all issues with AI agents
In @packages/react-web-cli/src/AblyCliTerminal.test.tsx:
- Around line 1933-1936: The test contains Prettier formatting violations around
the blocks using createTestSignedConfig and renderTerminal; run Prettier or
reformat the specified snippets so they match project style (e.g., correct
trailing commas, spacing, and indentation) — specifically fix the occurrences
where createTestSignedConfig("test-key-123", "test-token-456") and
renderTerminal({ signedConfig: customConfig }) are used (also update the same
patterns at the other two locations referenced) and then commit the formatted
file or run the Prettier CLI (npx prettier --write) to ensure CI passes.
In @packages/react-web-cli/src/AblyCliTerminal.tsx:
- Around line 95-107: The AblyCliTerminal API changed: update docs and code to
reflect that AblyCliTerminalProperties now requires signedConfig and signature
(replace prior ablyApiKey/ablyAccessToken references in README.md and CHANGELOG
to document this breaking change), remove the dead ciAuthToken field from the
AblyCliTerminalProperties interface and from any destructuring/usages inside the
AblyCliTerminal component, and verify any other occurrences (e.g., other
references to the old props in related code around AblyCliTerminal usage) are
updated to use signedConfig and signature so documentation and code match.
🧹 Nitpick comments (4)
packages/react-web-cli/src/AblyCliTerminal.resize.test.tsx (1)
57-63: Make the signed config deterministic to reduce test flake potential.
Date.now()under fake timers can be surprising; since the timestamp isn’t asserted here, using a constant (or passing it in) keeps the fixture stable.Proposed change
-const createTestSignedConfig = (apiKey: string = "test-key") => { +const createTestSignedConfig = ( + apiKey: string = "test-key", + timestamp: number = 1_700_000_000_000, +) => { return JSON.stringify({ apiKey, - timestamp: Date.now(), + timestamp, }); };packages/react-web-cli/src/AblyCliTerminal.test.tsx (2)
1085-1139: Auth payload assertions are good, but avoid assuming the firstsend()is the auth frame.If the component ever sends another frame before auth (or a second socket appears),
mock.calls[0]becomes brittle—filter for the first JSON payload that hasconfig/signature.Example hardening
-const sentPayload = JSON.parse(mockSend.mock.calls[0][0]); +const sentPayload = (() => { + for (const [arg0] of mockSend.mock.calls) { + if (typeof arg0 !== "string") continue; + try { + const parsed = JSON.parse(arg0); + if (parsed?.config) return parsed; + } catch { + // ignore non-JSON frames + } + } + throw new Error("No auth payload with config found in mockSend calls"); +})();Also: PR objectives mention “signedConfig with and without signature”, but these tests cover “with and without accessToken”. Please confirm whether “missing signature” is a real supported case (it can’t happen via props right now).
776-776:test.skipwarnings: consider switching totest.todo(or re-enabling).CI is flagging these via
vitest/no-disabled-tests. If they’re intentionally parked,test.todo(...)is usually the least noisy option.Also applies to: 830-830, 911-911, 1207-1207, 1609-1609, 1647-1647
packages/react-web-cli/src/terminal-shared.ts (1)
197-224: Consider failing fast whensignedConfig/signatureare missing.Logging an error but returning a “mostly valid” payload makes it easy to ship a broken connection attempt with unclear UX. If missing auth is unrecoverable, consider throwing (caller can catch and render an overlay) or returning an explicit error object.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/react-web-cli/src/AblyCliTerminal.resize.test.tsxpackages/react-web-cli/src/AblyCliTerminal.test.tsxpackages/react-web-cli/src/AblyCliTerminal.tsxpackages/react-web-cli/src/terminal-shared.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
**/*.{ts,tsx}: Use TypeScript and follow best practice naming conventions
ESLint is used to ensure all code adheres best practices; ensure you check that all code passes the linter before concluding your work.
Files:
packages/react-web-cli/src/AblyCliTerminal.resize.test.tsxpackages/react-web-cli/src/AblyCliTerminal.tsxpackages/react-web-cli/src/AblyCliTerminal.test.tsxpackages/react-web-cli/src/terminal-shared.ts
🧬 Code graph analysis (1)
packages/react-web-cli/src/AblyCliTerminal.resize.test.tsx (1)
packages/react-web-cli/src/AblyCliTerminal.tsx (1)
AblyCliTerminal(3826-3829)
🪛 GitHub Actions: E2E Tests
packages/react-web-cli/src/AblyCliTerminal.test.tsx
[error] 1933-1933: Prettier formatting error detected by lint step: Replace "test-key-123", "test-token-456" with a multi-line array formatting as required by prettier.
[warning] 776-776: vitest/no-disabled-tests: Disabled test - if you want to skip a test temporarily, use .todo() instead
[warning] 830-830: vitest/no-disabled-tests: Disabled test - if you want to skip a test temporarily, use .todo() instead
[warning] 911-911: vitest/no-disabled-tests: Disabled test - if you want to skip a test temporarily, use .todo() instead
[warning] 1207-1207: vitest/no-disabled-tests: Disabled test - if you want to skip a test temporarily, use .todo() instead
[warning] 1609-1609: vitest/no-disabled-tests: Disabled test - if you want to skip a test temporarily, use .todo() instead
[warning] 1647-1647: vitest/no-disabled-tests: Disabled test - if you want to skip a test temporarily, use .todo() instead
🪛 GitHub Actions: Web CLI E2E Tests (Parallel)
packages/react-web-cli/src/AblyCliTerminal.test.tsx
[error] 1933-1933: Prettier formatting error: Replace the single-line array with multiline formatting as suggested by prettier/prettier.
[warning] 776-1647: ESLint warning: vitest/no-disabled-tests - Disabled test detected (use .todo() to skip a test temporarily).
🪛 GitHub Check: e2e-cli
packages/react-web-cli/src/AblyCliTerminal.test.tsx
[failure] 1933-1933:
Replace "test-key-123",·"test-token-456" with ⏎······"test-key-123",⏎······"test-token-456",⏎····
[failure] 2130-2130:
Replace "secure-key-123",·"secure-token-456" with ⏎······"secure-key-123",⏎······"secure-token-456",⏎····
[failure] 2273-2273:
Replace "different-key",·"different-token" with ⏎······"different-key",⏎······"different-token",⏎····
🪛 GitHub Check: setup
packages/react-web-cli/src/AblyCliTerminal.test.tsx
[failure] 1933-1933:
Replace "test-key-123",·"test-token-456" with ⏎······"test-key-123",⏎······"test-token-456",⏎····
[failure] 2130-2130:
Replace "secure-key-123",·"secure-token-456" with ⏎······"secure-key-123",⏎······"secure-token-456",⏎····
[failure] 2273-2273:
Replace "different-key",·"different-token" with ⏎······"different-key",⏎······"different-token",⏎····
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
packages/react-web-cli/src/AblyCliTerminal.resize.test.tsx (1)
73-77: Prop migration tosignedConfig/signaturelooks consistent with the new auth flow.packages/react-web-cli/src/AblyCliTerminal.test.tsx (2)
146-164: Test fixtures for signed config/signature are a good consolidation.
394-402: Test renders updated to the new required props (signedConfig/signature).Also applies to: 1471-1480, 1805-1813, 2117-2125, 2354-2362
packages/react-web-cli/src/terminal-shared.ts (1)
230-240: CI token length logging refactor is fine.packages/react-web-cli/src/AblyCliTerminal.tsx (2)
1489-1540: Auth payload wiring for primary socket looks consistent.
2939-2945: Secondary terminal correctly reuses the same signed auth inputs.Also applies to: 3255-3260
kennethkalmer
force-pushed
the
inf-6536/update-react-web-cli
branch
from
46d9640 to
f883dfc
Compare
kennethkalmer
force-pushed
the
inf-6536/update-react-web-cli
branch
from
0d90eb4 to
08e3f9a
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 4
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
kennethkalmer
force-pushed
the
inf-6536/update-react-web-cli
branch
from
08e3f9a to
dcad6ff
Compare
kennethkalmer
force-pushed
the
inf-6536/update-react-web-cli
branch
from
dcad6ff to
dfd9874
Compare
…ation - Add support for signedConfig and signature in AblyCliTerminal for HMAC authentication. - Update createAuthPayload to prioritize signedConfig over direct credentials. - Enhance tests to verify correct payload structure when using signedConfig with and without signature.
Add server-side signing endpoint that works across all environments: - Development: Vite plugin (configureServer) - Preview/Tests: Vite plugin (configurePreviewServer) - Production: Vercel serverless function All three share signing logic from server/sign-handler.ts. Supports SIGNING_SECRET, TERMINAL_SERVER_SIGNING_SECRET, and CI_BYPASS_SECRET environment variables for compatibility. Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
kennethkalmer
force-pushed
the
inf-6536/update-react-web-cli
branch
from
dfd9874 to
7257ebb
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/web-cli/helpers/test-helpers.ts (1)
125-133: Storage key check uses outdated format after auth migration.The
willAutoConnectcheck at lines 128-133 looks forably.web-cli.apiKeyin storage. However, the codebase has migrated to a new format where credentials are stored asably.web-cli.signedConfig.${domain}andably.web-cli.signature.${domain}(as seen inexamples/web-cli/src/App.tsx). The current check would fail to detect auto-connect scenarios using the new signed config storage format.Update this check to also look for the domain-scoped signed config keys.
🧹 Nitpick comments (16)
test/unit/commands/did-you-mean.test.ts (1)
37-45: Timeout handling is a good addition, but consider applying consistently.The safety timeout pattern correctly prevents tests from hanging indefinitely. However, there's a minor race condition: if the timeout fires, the
exithandler still runs and may throw assertion errors after the promise is already rejected (e.g.,expect(foundPrompt).toBe(true)fails whenfoundPromptis stillfalse).Consider wrapping the assertions in a guard:
child.on("exit", () => { clearTimeout(killTimeout); + if (foundPrompt) { expect(foundPrompt).toBe(true); expect(output).toContain( "account current is not an ably command", ); + } resolve(); });Also, other interactive tests in this file (lines 123-181, 183-275, 296-347, 349-397) spawn child processes similarly but lack this timeout protection—they could still hang. Consider applying the same pattern for consistency.
Also applies to: 68-75
test/unit/commands/queues/delete.test.ts (2)
376-401: Good documentation, but consider fixing the underlying test helper limitation.The comment clearly explains why the test is skipped, which is helpful. However, the confirmation prompt is an important safety feature for a destructive operation (queue deletion) and should ideally be tested.
Consider one of these approaches:
- Investigate if
runCommandcan be enhanced to properly pipe stdin- Use a different testing strategy (e.g., directly unit-testing the prompt logic)
- Mock the confirmation prompt in tests and test it separately
Would you like me to investigate alternative testing approaches for stdin-dependent interactions?
403-431: Same limitation affects both confirmation flow paths.The added comment is consistent with the previous one. However, together these two skipped tests mean the entire confirmation prompt flow (both "yes" and "no" paths) remains untested.
Since both tests are blocked by the same
runCommandstdin limitation, fixing the test helper would enable testing the complete confirmation UX in one go.examples/web-cli/vite.config.ts (1)
10-14: Consider using exact path matching.Using
startsWith("/api/sign")could unintentionally match other paths like/api/signoutor/api/sign-something. Consider using an exact match for more precise routing.♻️ Proposed fix
const apiSignMiddleware = (req: any, res: any, next: any) => { // Only handle /api/sign requests - if (!req.url?.startsWith("/api/sign")) { + if (req.url !== "/api/sign") { return next(); }packages/react-web-cli/src/terminal-shared.ts (1)
197-223: Consider failing early when authentication requirements are not met.The function logs an error when
signedConfigorsignatureis missing but continues execution, returning a payload that will fail authentication. This could lead to confusing behavior where the connection attempt proceeds but fails later.Consider throwing an error or returning a sentinel value to make the failure explicit:
♻️ Suggested improvement
// Signed config authentication is required if (!signedConfig || !signature) { console.error( "[createAuthPayload] Missing signedConfig or signature - authentication will fail", ); + // Consider: throw new Error("signedConfig and signature are required for authentication"); } else {Also, the
JSON.parseon line 209 could throw for malformed JSON that isn't caught properly. While there's a try-catch, the parsed object is used without type validation. Consider adding runtime checks:const parsedConfig = JSON.parse(signedConfig) as Record<string, unknown>; if (typeof parsedConfig.apiKey === 'string') { payload.apiKey = parsedConfig.apiKey; }test/e2e/web-cli/shared-setup.ts (1)
108-115: Redundant environment variable assignments.Since
...process.envalready spreads all environment variables includingTERMINAL_SERVER_SIGNING_SECRET,SIGNING_SECRET, andCI_BYPASS_SECRET, the explicit assignments are redundant. They would only be needed if you wanted to override or transform the values.♻️ Suggested simplification
{ stdio: "pipe", cwd: EXAMPLE_DIR, - env: { - ...process.env, - // Ensure signing secret is passed to preview server - TERMINAL_SERVER_SIGNING_SECRET: - process.env.TERMINAL_SERVER_SIGNING_SECRET, - SIGNING_SECRET: process.env.SIGNING_SECRET, - CI_BYPASS_SECRET: process.env.CI_BYPASS_SECRET, - }, + env: process.env, },If the intent is documentation, a comment would suffice without the redundant assignments.
test/e2e/web-cli/authentication.test.ts (2)
343-347: LGTM - tests appropriately skipped with clear rationale.The inline comments explain why the tests are skipped (access token field removed until server confirmation). This is good practice for maintaining test coverage intent.
Would you like me to create a tracking issue for re-enabling these tests once access token support in signed configs is confirmed?
469-472: Consider moving the skip comment above the test declaration for consistency.The skip comment is placed between the
test.skipdeclaration and the destructuredpageparameter. While syntactically valid, placing it above the test (like in lines 343-347) would be more consistent.♻️ Suggested improvement
+ // SKIPPED: Test uses invalid access token which is not supported until confirmed + // Re-enable once access token support in signed configs is confirmed test.skip("should show SERVER DISCONNECT overlay for invalid credentials", async ({ - // SKIPPED: Test uses invalid access token which is not supported until confirmed - // Re-enable once access token support in signed configs is confirmed page, }) => {test/e2e/web-cli/domain-scoped-auth.test.ts (1)
265-271: Skipped test contains stale references to old authentication pattern.The skipped test still references
apiKeyin localStorage checks (lines 339, 346) rather thansignedConfig. When this test is re-enabled, it should be updated to use the new authentication pattern.📝 Suggested update when re-enabling
const accessibleCredentials = await page.evaluate(() => { const wsUrl = new URL( new URLSearchParams(window.location.search).get("serverUrl") || (window as any).__ABLY_CLI_WEBSOCKET_URL__ || "wss://web-cli.ably.com", ); const domain = wsUrl.host; return { - apiKey: localStorage.getItem(`ably.web-cli.apiKey.${domain}`), + signedConfig: localStorage.getItem(`ably.web-cli.signedConfig.${domain}`), rememberCredentials: localStorage.getItem( `ably.web-cli.rememberCredentials.${domain}`, ), }; }); - expect(accessibleCredentials.apiKey).toBeNull(); + expect(accessibleCredentials.signedConfig).toBeNull();Also applies to: 331-348
test/e2e/web-cli/auth-helper.ts (1)
56-74: Remove the legacy apiKey URL check or update it to check for signedConfig instead.The check for
apiKey=orapikey=in the URL (line 57) is dead code. All tests have migrated to signed config authentication and construct URLs withsignedConfig=andsignature=parameters. No test passes apiKey directly in the URL. Either remove this obsolete fallback or update it to check forsignedConfig=to align with the current authentication flow.examples/web-cli/src/components/AuthScreen.tsx (1)
22-45: Early returns bypassfinallyblock butisLoadingshould still be reset.The validation early returns on lines 29-31 and 35-37 exit the function before
onAuthenticateis called. Whilefinallydoes execute on early returns, the UX issue is that the button briefly shows "Signing credentials..." before the validation error appears. Consider moving validation before settingisLoading:♻️ Proposed fix to validate before showing loading state
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); setError(''); - setIsLoading(true); - try { - if (!apiKey.trim()) { - setError('API Key is required to connect to Ably'); - return; - } + if (!apiKey.trim()) { + setError('API Key is required to connect to Ably'); + return; + } - // Basic validation for API key format - if (!apiKey.includes(':')) { - setError('API Key should be in the format: app_name.key_name:key_secret'); - return; - } + // Basic validation for API key format + if (!apiKey.includes(':')) { + setError('API Key should be in the format: app_name.key_name:key_secret'); + return; + } + setIsLoading(true); + + try { await onAuthenticate(apiKey.trim(), rememberCredentials); } catch (error) { console.error('[AuthScreen] Authentication failed:', error); setError(error instanceof Error ? error.message : 'Authentication failed'); } finally { setIsLoading(false); } };packages/react-web-cli/src/AblyCliTerminal.tsx (1)
2638-2658: Consider extracting credential parsing to a shared helper.The signedConfig parsing logic for extracting
apiKeyandaccessTokenis duplicated here and inAuthSettings.tsx. A shared utility would improve maintainability.// Could add to terminal-shared.ts or a new utils file: export function parseSignedConfigCredentials(signedConfig: string): { apiKey?: string; accessToken?: string } { try { const parsed = JSON.parse(signedConfig); return { apiKey: parsed.apiKey, accessToken: parsed.accessToken }; } catch { return {}; } }examples/web-cli/src/components/AuthSettings.tsx (1)
2-2: Unused import:Lockis imported but not used.The
Lockicon was likely used for the removed Access Token input field.♻️ Remove unused import
-import { X, Key, Lock, AlertCircle, CheckCircle, Save } from 'lucide-react'; +import { X, Key, AlertCircle, CheckCircle, Save } from 'lucide-react';test/e2e/web-cli/helpers/signing-helper.ts (1)
19-36: Consider failing tests instead of using fallback secret in CI.The fallback
"dev-test-secret-example-only"prevents test failures when the secret is missing, but this could mask configuration issues in CI. Consider checking if running in CI and throwing an error instead.♻️ Proposed enhancement for CI environments
export function getTestSecret(): string { const secret = process.env.TERMINAL_SERVER_SIGNING_SECRET || process.env.SIGNING_SECRET || process.env.CI_BYPASS_SECRET; if (!secret) { + // Fail hard in CI to catch configuration issues early + if (process.env.CI) { + throw new Error( + "[Signing Helper] No signing secret found in CI environment. " + + "Set TERMINAL_SERVER_SIGNING_SECRET, SIGNING_SECRET, or CI_BYPASS_SECRET." + ); + } console.warn( "[Signing Helper] No signing secret found in environment. Using fallback dev secret.", ); console.warn( "[Signing Helper] Set TERMINAL_SERVER_SIGNING_SECRET in .env for production parity.", ); return "dev-test-secret-example-only"; } return secret; }examples/web-cli/src/App.tsx (2)
165-169: Error response parsing may fail for non-JSON responses.If the server returns a non-JSON error response (e.g., 500 with HTML error page),
response.json()will throw, masking the original HTTP error with an unhelpful parsing error.🔧 Suggested fix
if (!response.ok) { - const error = await response.json(); - console.error('[App] Failed to sign credentials:', error); - throw new Error(error.error || 'Failed to sign credentials'); + let errorMessage = 'Failed to sign credentials'; + try { + const error = await response.json(); + errorMessage = error.error || errorMessage; + } catch { + // Response was not JSON, use status text + errorMessage = `Failed to sign credentials: ${response.statusText}`; + } + console.error('[App] Failed to sign credentials:', errorMessage); + throw new Error(errorMessage); }
30-33: Remove unusedgetCIAuthTokenfunction.The
getCIAuthTokenfunction defined at lines 31-33 is never called within this file or imported elsewhere in the codebase. Remove it.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
.env.example.tool-versionsexamples/web-cli/api/sign.tsexamples/web-cli/package.jsonexamples/web-cli/server/sign-handler.tsexamples/web-cli/src/App.tsxexamples/web-cli/src/components/AuthScreen.tsxexamples/web-cli/src/components/AuthSettings.tsxexamples/web-cli/vite.config.tspackages/react-web-cli/README.mdpackages/react-web-cli/src/AblyCliTerminal.resize.test.tsxpackages/react-web-cli/src/AblyCliTerminal.test.tsxpackages/react-web-cli/src/AblyCliTerminal.tsxpackages/react-web-cli/src/terminal-shared.tstest/e2e/web-cli/auth-helper.tstest/e2e/web-cli/authentication.test.tstest/e2e/web-cli/domain-scoped-auth.test.tstest/e2e/web-cli/helpers/signing-helper.tstest/e2e/web-cli/helpers/test-helpers.tstest/e2e/web-cli/prompt-integrity.test.tstest/e2e/web-cli/reconnection.test.tstest/e2e/web-cli/shared-setup.tstest/unit/commands/did-you-mean.test.tstest/unit/commands/queues/delete.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/AI-Assistance.mdc)
**/*.test.ts: When testing, mock the Ably SDK properly by stubbing the constructor (e.g., sinon.stub(Ably, 'Rest').returns(mockClient)).
Always ensure resources are properly closed/cleaned up in tests (e.g., call await client.close() and sinon.restore() in afterEach).
Files:
test/unit/commands/queues/delete.test.tstest/e2e/web-cli/domain-scoped-auth.test.tstest/unit/commands/did-you-mean.test.tstest/e2e/web-cli/prompt-integrity.test.tstest/e2e/web-cli/reconnection.test.tstest/e2e/web-cli/authentication.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
**/*.{ts,tsx}: Use TypeScript and follow best practice naming conventions
ESLint is used to ensure all code adheres best practices; ensure you check that all code passes the linter before concluding your work.
Files:
test/unit/commands/queues/delete.test.tstest/e2e/web-cli/domain-scoped-auth.test.tsexamples/web-cli/api/sign.tspackages/react-web-cli/src/AblyCliTerminal.resize.test.tsxpackages/react-web-cli/src/AblyCliTerminal.test.tsxtest/e2e/web-cli/auth-helper.tsexamples/web-cli/vite.config.tsexamples/web-cli/server/sign-handler.tstest/unit/commands/did-you-mean.test.tspackages/react-web-cli/src/terminal-shared.tsexamples/web-cli/src/components/AuthSettings.tsxtest/e2e/web-cli/helpers/signing-helper.tstest/e2e/web-cli/helpers/test-helpers.tspackages/react-web-cli/src/AblyCliTerminal.tsxtest/e2e/web-cli/prompt-integrity.test.tstest/e2e/web-cli/reconnection.test.tsexamples/web-cli/src/components/AuthScreen.tsxtest/e2e/web-cli/shared-setup.tsexamples/web-cli/src/App.tsxtest/e2e/web-cli/authentication.test.ts
{test/**/*.{ts,js},**/*.{test,spec}.{ts,js}}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
When new features are added or changes made, tests must be updated or added, and it is your responsibility to ensure the tests pass before deeming your work complete.
Files:
test/unit/commands/queues/delete.test.tstest/e2e/web-cli/domain-scoped-auth.test.tstest/e2e/web-cli/auth-helper.tstest/unit/commands/did-you-mean.test.tstest/e2e/web-cli/helpers/signing-helper.tstest/e2e/web-cli/helpers/test-helpers.tstest/e2e/web-cli/prompt-integrity.test.tstest/e2e/web-cli/reconnection.test.tstest/e2e/web-cli/shared-setup.tstest/e2e/web-cli/authentication.test.ts
🧬 Code graph analysis (11)
examples/web-cli/api/sign.ts (1)
examples/web-cli/server/sign-handler.ts (2)
getSigningSecret(55-62)signCredentials(24-49)
packages/react-web-cli/src/AblyCliTerminal.resize.test.tsx (1)
packages/react-web-cli/src/AblyCliTerminal.tsx (1)
AblyCliTerminal(3822-3825)
test/e2e/web-cli/auth-helper.ts (2)
test/e2e/web-cli/helpers/signing-helper.ts (1)
createSignedConfig(56-91)test/e2e/web-cli/test-rate-limiter.ts (1)
incrementConnectionCount(168-193)
examples/web-cli/vite.config.ts (1)
examples/web-cli/server/sign-handler.ts (2)
getSigningSecret(55-62)signCredentials(24-49)
examples/web-cli/src/components/AuthSettings.tsx (1)
src/services/control-api.ts (1)
Key(36-46)
test/e2e/web-cli/helpers/test-helpers.ts (1)
test/e2e/web-cli/helpers/signing-helper.ts (1)
createSignedConfig(56-91)
packages/react-web-cli/src/AblyCliTerminal.tsx (2)
packages/react-web-cli/src/terminal-shared.ts (1)
createAuthPayload(183-248)packages/react-web-cli/src/utils/crypto.ts (1)
hashCredentials(6-37)
test/e2e/web-cli/prompt-integrity.test.ts (2)
test/e2e/web-cli/helpers/signing-helper.ts (1)
createSignedConfig(56-91)test/e2e/web-cli/helpers/test-helpers.ts (1)
getTestUrl(58-73)
test/e2e/web-cli/reconnection.test.ts (2)
test/e2e/web-cli/helpers/test-helpers.ts (1)
getTestUrl(58-73)test/e2e/web-cli/helpers/signing-helper.ts (1)
createSignedConfig(56-91)
examples/web-cli/src/App.tsx (2)
packages/react-web-cli/src/AblyCliTerminal.tsx (1)
AblyCliTerminal(3822-3825)packages/react-web-cli/src/index.ts (1)
AblyCliTerminal(1-1)
test/e2e/web-cli/authentication.test.ts (3)
test/e2e/web-cli/helpers/base-test.ts (1)
test(10-176)test/e2e/web-cli/helpers/rate-limit-test-wrapper.ts (1)
test(8-34)test/e2e/web-cli/helpers/rate-limited-test.ts (1)
test(13-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: session-tests
- GitHub Check: ui-tests
- GitHub Check: e2e-cli
- GitHub Check: Cursor Bugbot
🔇 Additional comments (40)
.tool-versions (1)
2-2: LGTM! Addition aligns with PR tooling updates.The addition of pnpm 10.28.0 (latest stable release) supports the new signing workflow and dev server tooling mentioned in the PR objectives. No known security vulnerabilities affect this version.
examples/web-cli/package.json (1)
26-26: LGTM!The
@vercel/nodedevDependency is correctly added to support the Vercel serverless function types used in the new signing endpoint..env.example (1)
13-22: LGTM!Clear documentation explaining the purpose of the signing secret, its relationship to the live server, and retention of the legacy fallback for compatibility.
examples/web-cli/server/sign-handler.ts (2)
24-49: LGTM!The signing implementation is well-structured:
- HMAC-SHA256 is appropriate for this authentication use case
- Config structure with timestamp enables replay protection on the server side
- Hex encoding for the signature is a good choice for transport
55-61: LGTM!The fallback chain for environment variables provides good compatibility while transitioning to the new naming convention.
examples/web-cli/api/sign.ts (1)
21-48: LGTM!The Vercel function correctly validates the request method, checks for the signing secret, validates required input, and delegates to the shared signing logic.
Minor observation: The function is marked
asyncbut doesn't useawait. This is harmless but could be simplified to a synchronous function if preferred.examples/web-cli/vite.config.ts (2)
31-60: Body parsing implementation looks good.The streaming body parser correctly handles the request data and includes appropriate error handling for malformed JSON.
63-76: LGTM!The plugin is well-structured, correctly registers the middleware for both dev and preview servers, and integrates cleanly into the Vite config.
packages/react-web-cli/src/terminal-shared.ts (1)
231-240: LGTM!Good refactor to use a local
ciTokenvariable for cleaner code and consistent logging.packages/react-web-cli/src/AblyCliTerminal.resize.test.tsx (2)
57-63: LGTM!The test helper correctly creates a minimal signed config structure for testing purposes. Using a fixed
"test-signature"is appropriate for unit tests where actual HMAC validation isn't performed.
71-78: LGTM!The test correctly migrates to the new
signedConfigandsignatureprops, maintaining the same test behavior.test/e2e/web-cli/helpers/test-helpers.ts (1)
88-109: Consider whetheraccessTokenin params should also be signed.The current logic only checks for
apiKeyand signs it. Ifparamscontains anaccessToken, it will be added as a raw URL parameter instead of being included in the signed config. Based on thecreateSignedConfighelper insigning-helper.ts(lines 55-90), it supports bothapiKeyandaccessTokenin the credentials.If access tokens should also be signed (once confirmed supported), consider:
if (params.apiKey) { const { signedConfig, signature } = createSignedConfig({ apiKey: params.apiKey, + accessToken: params.accessToken, timestamp: Date.now(), bypassRateLimit: true, }); url.searchParams.set("signedConfig", signedConfig); url.searchParams.set("signature", signature); // Add other params except apiKey (already handled) Object.entries(params).forEach(([key, value]) => { - if (key !== "apiKey") { + if (key !== "apiKey" && key !== "accessToken") { url.searchParams.set(key, value); } });Otherwise, if access token support is intentionally excluded (per the skipped tests in authentication.test.ts), this is acceptable as-is.
test/e2e/web-cli/reconnection.test.ts (2)
21-21: LGTM! Consistent migration to signed config authentication.The test file correctly migrates from direct API key usage to the signed config pattern. The
createSignedConfighelper is used consistently across all tests with appropriate parameters (apiKey,timestamp,bypassRateLimit: true).Also applies to: 145-154
153-156: URL construction with signed credentials is correct.The signed config and signature are properly URI-encoded before being added to the query string, which correctly handles the JSON content in
signedConfig.test/e2e/web-cli/prompt-integrity.test.ts (1)
20-20: LGTM! Prompt integrity tests correctly updated for signed config authentication.The test file follows the same pattern as other E2E tests, properly generating signed credentials before navigation.
Also applies to: 40-51
test/e2e/web-cli/domain-scoped-auth.test.ts (1)
71-86: LGTM! Domain-scoped storage correctly updated to check for signedConfig and signature keys.The assertions properly verify that credentials are now stored with
.signedConfig.and.signature.key patterns, matching the new authentication model.test/e2e/web-cli/auth-helper.ts (1)
76-117: LGTM! Signed config authentication flow correctly implemented.The helper properly signs credentials with
bypassRateLimit: truefor tests and sets bothsignedConfigandsignaturequery parameters. TheclearCredentials=trueparameter ensures consistent test state.packages/react-web-cli/src/AblyCliTerminal.test.tsx (6)
146-163: LGTM! Well-designed test helper for creating signed configs.The
createTestSignedConfighelper correctly mirrors the production signing helper structure. The default constants provide convenient reusable values for tests.
1091-1117: LGTM! Auth payload test correctly validates the new authentication structure.The test properly verifies that:
sentPayload.configcontains the signed config JSONsentPayload.signaturecontains the HMAC signature- Credentials (
apiKey,accessToken) are extracted from the signed config for server convenience
1119-1145: LGTM! Anonymous mode test validates API-key-only authentication.This test correctly verifies that the terminal works when only an API key is provided in the signed config, without an access token. The assertion that
accessTokenisundefinedin the payload confirms proper handling of this case.
2022-2058: LGTM! Anonymous mode credential handling correctly tested.The test at line 2057 correctly expects
"hash-test-key:"format, validating that the hash is computed with the API key present and an empty access token component.
776-779: Skipped tests have clear documentation.All skipped tests include explanatory comments describing why they are skipped (timing issues with fake timers, CI environment instability, React internal dependencies). This is good practice for maintaining test suites.
Also applies to: 832-835, 915-918, 1213-1216, 1616-1619, 1654-1658
390-403: LGTM! renderTerminal helper correctly updated for signed config authentication.The helper function provides sensible defaults (
DEFAULT_SIGNED_CONFIG,DEFAULT_SIGNATURE) while allowing tests to override props as needed.packages/react-web-cli/README.md (3)
50-79: LGTM! Comprehensive usage example with proper async pattern.The example demonstrates the signing workflow correctly, showing how to call the backend signing endpoint and pass credentials to the component. The loading state handling is appropriate.
116-161: Excellent security documentation.The security requirements are well-documented with clear warnings about never exposing the signing secret client-side. The reference implementation pointers and complete signing endpoint example are helpful for implementers.
163-201: Clear migration guide with before/after comparison.The breaking changes section effectively communicates the API migration path with concrete code examples showing removed and required props. The "Why this change?" section provides good rationale.
examples/web-cli/src/components/AuthScreen.tsx (2)
48-59: Good migration-friendly cleanup of both old and new credential formats.The function correctly clears both the new signed config keys and the legacy keys, ensuring a clean state regardless of which format was previously stored.
117-124: LGTM! Proper disabled state and loading feedback.The submit button correctly disables during loading and shows appropriate feedback text.
packages/react-web-cli/src/AblyCliTerminal.tsx (3)
96-107: LGTM! Well-documented prop interface for the new authentication model.The JSDoc comments clearly explain the purpose of
signedConfigandsignature, and both are correctly marked as required for authentication.
1485-1491: Auth payload correctly uses signedConfig and signature.The
createAuthPayloadcall properly passes the signed credentials for server authentication.
2935-2941: Secondary terminal authentication correctly mirrors primary terminal.The secondary WebSocket connection uses the same
signedConfigandsignatureprops, ensuring consistent authentication across both terminals.examples/web-cli/src/components/AuthSettings.tsx (3)
31-40: LGTM! Safe JSON parsing with fallback.The
extractApiKeyhelper properly handles invalid JSON and returns an empty string as a safe default.
59-80: Async error handling correctly surfaces failures to the user.The try/catch pattern properly converts errors to user-visible messages in the component state.
117-149: LGTM! Simplified credential form after removing accessToken.The form structure is clean and the help text provides useful guidance for users.
test/e2e/web-cli/helpers/signing-helper.ts (4)
6-13: LGTM! Well-structured credential configuration interface.The interface covers all necessary fields for terminal server authentication with appropriate optional markers.
44-48: LGTM! Correct HMAC-SHA256 implementation.The signature generation follows the standard pattern for HMAC authentication.
56-91: LGTM! Config building correctly includes conditional fields.The implementation properly builds the config object with mandatory fields (
apiKey,timestamp) and conditionally includes optional fields only when provided.
107-124: Useful debugging utility for troubleshooting signing configuration.The status logging clearly shows which environment variables are set and which secret is being used, which will be helpful for debugging test failures.
examples/web-cli/src/App.tsx (2)
243-259: LGTM!The
TerminalInstancecallback correctly passes the newsignedConfigandsignatureprops toAblyCliTerminal, with proper dependency tracking inuseCallback. The triple condition on line 244 provides defensive redundancy.
342-348: No action needed. TheAuthSettingscomponent correctly accepts thecurrentSignedConfigprop, which is properly defined in theAuthSettingsPropsinterface as an optional string (line 8) and is correctly destructured and used throughout the component.
|
@emptyhammond @AndyTWF I've taken this further by updating the example app to work with the new signed configuration. I've dropped the "JWT Token" from the login field as I'm not sure if it is actually possible to turn it into something that can be signed and passed on... But if needed, with the right guidance I can bring it back (otherwise I'll remove the disabled tests). Our production deployment for the web CLI also needs to be configured with the signing key for the terminal server. |
| apiKey?: string, | ||
| accessToken?: string, | ||
| sessionId?: string | null, | ||
| additionalEnvVars?: Record<string, string>, |
There was a problem hiding this comment.
AFAICT we don't use additionalEnvVars any where now, so can we get rid of it here?
- App.tsx: Call /api/sign endpoint to get signed credentials - Store signedConfig/signature in localStorage/sessionStorage (domain-scoped) - Pass signed credentials to AblyCliTerminal component - Remove accessToken fields from forms (not confirmed to work with signing yet) - Handle migration from old credential storage format (clear old keys) - Form authentication now works via server-side signing Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
Add Node.js signing helper for E2E tests supporting two strategies: - Direct injection: Sign in test code, inject via query params (faster) - Form submission: Browser calls /api/sign endpoint (validates endpoint) Updates: - signing-helper.ts: Node.js crypto-based signing matching server logic - auth-helper.ts: Sign credentials before adding to URLs - test-helpers.ts: buildTestUrl() auto-signs apiKey parameters - shared-setup.ts: Pass signing secrets (including CI_BYPASS_SECRET) to preview server This enables form-based auth tests to work in CI by making /api/sign available in the preview server. Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
Update all E2E tests to use signed credentials: - authentication.test.ts: Update auth screen text expectations, skip accessToken tests - domain-scoped-auth.test.ts: Check for signedConfig/signature storage keys instead of apiKey - prompt-integrity.test.ts: Sign credentials before manual URL building - reconnection.test.ts: Sign credentials before all page.goto calls All tests now authenticate using signedConfig + signature instead of raw API keys. AccessToken tests skipped until confirmed to work with signed configs. Results: 29 passing, 6 skipped, 4 failing (down from 22 failing) Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
Document TERMINAL_SERVER_SIGNING_SECRET environment variable used for: 1. Signing credentials via /api/sign endpoint (Vite + Vercel) 2. Bypassing rate limits in E2E tests (via signed config) Replace CI_BYPASS_SECRET documentation (still supported as fallback for backwards compatibility). Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
… clearing
Fix two issues identified by code review:
1. Inconsistent environment variable priority order:
- sign-handler.ts and signing-helper.ts now both prioritize
TERMINAL_SERVER_SIGNING_SECRET first (matching .env.example docs)
- Prevents signature mismatches when multiple env vars are set
2. AuthScreen credential detection and clearing:
- hasSavedCredentials now checks for domain-scoped signedConfig keys
- handleClearSavedCredentials removes ALL domain-scoped credentials
- "Clear saved credentials" button now appears and works correctly
Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
Update willAutoConnect check in reloadPageWithRateLimit to detect the new signed config format: - Check URL for signedConfig= and signature= (not apiKey=) - Check storage for domain-scoped ably.web-cli.signedConfig.* keys This ensures rate limiting properly tracks connections when using signed credentials, preventing undercounting of connection attempts. Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
kennethkalmer
force-pushed
the
inf-6536/update-react-web-cli
branch
from
0cbff2c to
0ddc1b6
Compare
kennethkalmer
force-pushed
the
inf-6536/update-react-web-cli
branch
from
85db61b to
a65c771
Compare
The additionalEnvVars parameter in createAuthPayload is no longer needed since endpoint and controlAPIHost configuration are now included in the signed config itself. Both call sites pass undefined for this parameter. Co-Authored-By: Claude Sonnet 4.5 (1M context) <[email protected]>
kennethkalmer
force-pushed
the
inf-6536/update-react-web-cli
branch
from
a65c771 to
d454d36
Compare
Overview
This PR updates the Web CLI to use HMAC-signed authentication, implementing the client-side changes to work with the terminal server signed config support.
Breaking Changes
AblyCliTerminal Component API
Removed props:
ablyApiKeyablyAccessTokenablyEndpointablyControlHostciAuthTokenNew required props:
signedConfig(string) - JSON-encoded config containing credentials and metadatasignature(string) - HMAC-SHA256 signature of the signedConfigMigration:
Implementation
1. Signing Infrastructure (
/api/signendpoint)Added server-side signing endpoint that works across all environments:
Development:
/api/sign(viaconfigureServerhook)TERMINAL_SERVER_SIGNING_SECRETfrom local.envPreview/E2E Tests:
configurePreviewServerhook)Production:
examples/web-cli/api/sign.tsSIGNING_SECRETfrom Vercel environment variablesAll three share:
examples/web-cli/server/sign-handler.tsfor consistent signing logic2. Example App Updates
React Components:
App.tsx: Calls/api/signendpoint to get signed credentials before passing toAblyCliTerminalAuthScreen.tsx: Removed accessToken field (not confirmed to work with signing yet)AuthSettings.tsx: Shows read-only view of current credentials extracted from signed configStorage:
ably.web-cli.signedConfig.${domain},ably.web-cli.signature.${domain}apiKey/accessTokenkeys)3. E2E Test Updates
Test Infrastructure:
test/e2e/web-cli/helpers/signing-helper.ts- Node.js crypto signing for direct credential injectionauth-helper.ts- Signs credentials before adding to URLstest-helpers.ts-buildTestUrl()auto-signsapiKeyparametersshared-setup.ts- Passes signing secrets to preview serverTest Strategies:
/api/signendpoint (validates endpoint works)Updated Tests:
authentication.test.ts- Updated expectations, skipped accessToken tests until confirmeddomain-scoped-auth.test.ts- Check forsignedConfig/signaturestorage keysprompt-integrity.test.ts- Sign credentials before manual URL buildingreconnection.test.ts- Sign credentials before allpage.goto()calls4. Documentation
README.md:
.env.example:
TERMINAL_SERVER_SIGNING_SECRETdocumentationCI_BYPASS_SECRETstill supported as fallback5. Bug Fixes
Consistency:
TERMINAL_SERVER_SIGNING_SECRET→SIGNING_SECRET→CI_BYPASS_SECRETStorage:
Security Improvements
Files Changed
New Files:
examples/web-cli/api/sign.ts- Vercel serverless functionexamples/web-cli/server/sign-handler.ts- Shared signing logictest/e2e/web-cli/helpers/signing-helper.ts- Test signing utilityModified:
packages/react-web-cli/src/AblyCliTerminal.tsxpackages/react-web-cli/src/terminal-shared.tsexamples/web-cli/src/App.tsx, auth components,vite.config.tsREADME.md,.env.exampleDependencies
@vercel/node(dev dependency) for TypeScript typespnpmto.tool-versionsfor consistencyRelated
Note
Introduces HMAC-signed authentication and removes API-key/token props, aligning the Web CLI with the terminal server’s signed config flow.
AblyCliTerminalnow requiressignedConfigandsignature; removesablyApiKey,ablyAccessToken,ablyEndpoint,ablyControlHost,ciAuthTokenserver/sign-handler.ts, Vite dev/preview/api/signmiddleware, and Vercel functionexamples/web-cli/api/sign.tsApp.tsx,AuthScreen.tsx,AuthSettings.tsx) to fetch and persist signed credentials using domain-scoped storage keys (ably.web-cli.signedConfig.*,ably.web-cli.signature.*); migrates/cleans old keysterminal-shared.ts) and component logic (AblyCliTerminal.tsx) to send signed config, parse credentials for session resumption, and support split-screenhelpers/signing-helper.ts), update auth flows, and skip legacy access token cases.env.exampledocumentsTERMINAL_SERVER_SIGNING_SECRET@vercel/nodedev dep, updates.tool-versionswithpnpmWritten by Cursor Bugbot for commit d454d36. This will update automatically on new commits. Configure here.