feat: add API key management and authentication to TypeScript and Rus… #44
Conversation
…t SDKs This commit implements API key functionality for both SDKs: TypeScript SDK: - Add API key CRUD operations (create, list, delete) with encrypted API wrapper - Enable API key authentication for OpenAI endpoints (/v1/models and /v1/chat/completions) - Add CustomFetchOptions interface to support API key in createCustomFetch - Add openAiAuthenticatedApiCall function for OpenAI-specific endpoints - Export new API key types and functions from index.ts - Add comprehensive test suite for API key management and authentication Rust SDK: - Add API key types (ApiKey, ApiKeyCreateResponse, ApiKeyListResponse) - Implement API key CRUD operations in Client - Add API key storage and authentication support in Session - Enable dual authentication (JWT and API key) for OpenAI endpoints - Add comprehensive test suite covering all API key functionality Both implementations: - Maintain full backward compatibility with existing JWT authentication - API keys are UUID format with dashes - Keys are only returned once during creation (not in list operations) - Only OpenAI endpoints accept API key authentication - All other endpoints continue to use JWT authentication only Testing: - TypeScript: 19/19 tests passing - Rust: 49/49 tests passing - No regressions in existing functionality 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
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. WalkthroughAdds runtime and constructor API-key support across Rust and TypeScript SDKs, introduces an OpenAI-specific encrypted request path with API-key precedence, adds API-key CRUD endpoints and types, updates auth/header logic for OpenAI vs non-OpenAI paths, and includes new unit/integration tests for API-key flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SDK as SDK Consumer
participant Client as OpenSecretClient
participant Session as SessionManager
participant Server as OpenSecret Server
SDK->>Client: new_with_api_key(base_url, api_key)
Client->>Session: new_with_api_key(api_key)
note right of Session #e6f7ff: api_key stored in Arc<RwLock>
SDK->>Client: request(path)
Client->>Session: get_api_key()
alt path matches /v1/* AND api_key present
Client->>Server: HTTP request with Authorization: Bearer <api_key> (encrypted_openai_call)
else otherwise
Client->>Session: get_token()
Client->>Server: HTTP request with Authorization: Bearer <jwt_token> (encrypted API call)
end
sequenceDiagram
autonumber
participant WebApp as Web App
participant Fetch as createCustomFetch
participant Enc as internalEncryptedApiCall
participant Auth as Token Refresh
participant Srv as Server
WebApp->>Fetch: call(url, init, options?.apiKey)
alt options.apiKey provided
Fetch->>Enc: send with Authorization: Bearer <apiKey>
Enc->>Srv: request
Srv-->>Enc: 2xx / 401
alt 401 with apiKey
Enc-->>WebApp: error (no refresh attempted)
else success
Enc-->>WebApp: response
end
else No apiKey
Fetch->>Enc: send with Authorization: Bearer <token>
Enc->>Srv: request
Srv-->>Enc: 2xx / 401
alt 401
Fetch->>Auth: refresh token
Auth-->>Fetch: new token or error
alt refresh ok
Fetch->>Enc: retry with new token
Enc-->>WebApp: response
else refresh failed
Fetch-->>WebApp: error
end
else success
Enc-->>WebApp: response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying opensecret-sdk with
|
| Latest commit: |
d3b8b56
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://59f9b326.opensecret-sdk.pages.dev |
| Branch Preview URL: | https://feat-api-key-management.opensecret-sdk.pages.dev |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements comprehensive API key management functionality for both TypeScript and Rust SDKs, introducing an alternative authentication method specifically for OpenAI endpoints while maintaining full backward compatibility with existing JWT authentication.
Key Changes:
TypeScript SDK:
- Added API key CRUD operations (
createApiKey,listApiKeys,deleteApiKey) inapi.tswith proper encrypted API wrapper integration - Modified
createCustomFetchinai.tsto support API key authentication via newCustomFetchOptionsinterface - Added
openAiAuthenticatedApiCallfunction inencryptedApi.tsfor dual authentication support - Updated
fetchModelsto accept optional API key parameter for OpenAI endpoint access - Exported all new API key types and functions from
index.ts
Rust SDK:
- Added API key types (
ApiKey,ApiKeyCreateResponse,ApiKeyListResponse) intypes.rs - Implemented API key storage and management in
SessionManagerwith thread-safe Arc pattern - Added API key CRUD operations and authentication logic in
Clientwith preference hierarchy (API key over JWT) - Updated all HTTP request methods (regular, encrypted, streaming) to support dual authentication
Architecture Design:
The implementation follows a dual authentication model where API keys are exclusively used for OpenAI endpoints (/v1/models and /v1/chat/completions) while JWT tokens continue to handle all other protected endpoints. Both SDKs implement a preference hierarchy where API keys take precedence over JWT tokens when both are available. The design ensures API keys follow UUID format with dashes and are only exposed once during creation for security.
Integration with Existing Codebase:
The changes integrate seamlessly with the existing authentication infrastructure, leveraging established patterns like the encrypted API wrapper system and session management. The implementation maintains the SDK's core security model while adding granular access control through API keys.
Confidence score: 4/5
- This PR appears safe to merge with careful attention to the authentication logic changes
- Score reflects the complexity of dual authentication implementation across critical authentication paths
- Pay close attention to the client authentication logic and session management files
9 files reviewed, 2 comments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/lib/encryptedApi.ts (1)
39-44: Standardize missing-credentials error message across codebaseTo keep test expectations and logs consistent with
ai.ts, update both the runtime error inencryptedApi.tsand the corresponding assertion in the signing integration test:• In src/lib/encryptedApi.ts, change the thrown error from
throw new Error("No access token available");to
throw new Error("No access token or API key available");• In src/lib/test/integration/signing.test.ts, update the expectation on line 237 from
await expect(signMessage(message, "schnorr")).rejects.toThrow("No access token available");to
await expect(signMessage(message, "schnorr")).rejects.toThrow("No access token or API key available");Diffs:
--- a/src/lib/encryptedApi.ts +++ b/src/lib/encryptedApi.ts @@ -39,7 +39,7 @@ export async function encryptedApiFetch<Res>(...) // Always get the latest token from localStorage const accessToken = window.localStorage.getItem("access_token"); if (!accessToken) { - throw new Error("No access token available"); + throw new Error("No access token or API key available"); } // ...--- a/src/lib/test/integration/signing.test.ts +++ b/src/lib/test/integration/signing.test.ts @@ -234,7 +234,7 @@ describe("signMessage", () => { it("fails when no token is stored", async () => { const message = new TextEncoder().encode("Hello, World!"); - await expect(signMessage(message, "schnorr")).rejects.toThrow("No access token available"); + await expect(signMessage(message, "schnorr")).rejects.toThrow("No access token or API key available"); }); });
🧹 Nitpick comments (16)
src/lib/encryptedApi.ts (4)
79-110: Add: OpenAI-specific API key path looks correct; minor polish suggestedThe API-key path bypasses refresh and delegates to internalEncryptedApiCall, which is the right call. Two small follow-ups:
- Consider surfacing a clearer error on 401 when apiKey is provided (e.g., “Invalid or unauthorized API key”) to aid DX.
- Align the “no credentials” message with ai.ts for consistency (see separate comment below).
Would you like me to add a small 401 branch here that throws a more explicit error when using API keys?
137-139: Avoid falsy checks when serializing request dataUsing a truthy check will drop valid values like 0, false, or empty string. Prefer explicit nullish checks.
Apply this diff:
- const jsonData = data ? JSON.stringify(data) : undefined; + const jsonData = data !== undefined && data !== null ? JSON.stringify(data) : undefined;
190-196: Retry-on-encryption-error logic may miss different server messagesYou match “Encryption error” exactly. Servers often vary the text. Consider broadening to a code or status, or checking for 400 with a structured error code once available. If not available, a case-insensitive substring like “encrypt” is a pragmatic stopgap.
112-181: Header completeness: consider setting Accept headerTo be explicit and help proxies, set Accept: application/json for JSON paths.
const headers: Record<string, string> = { "Content-Type": "application/json", "x-session-id": sessionId }; + // Prefer JSON responses + headers["Accept"] = "application/json";rust/src/types.rs (1)
295-300: Consider strong-typing ApiKeyCreateResponse.key as UuidPR notes say keys are UUIDs with dashes. Strong-typing reduces parsing/validation errors and documents the contract.
#[derive(Debug, Clone, Serialize, Deserialize)] pub struct ApiKeyCreateResponse { pub id: i64, - pub key: String, // UUID format, only returned on creation + pub key: Uuid, // UUID format, only returned on creation pub name: String, pub created_at: DateTime<Utc>, }If the server ever prefixes keys (e.g., “sk-”), keep String. Can you confirm the exact format?
rust/src/session.rs (1)
29-53: API key getters/setters: error mapping and locking look correct; add unit tests locallyThe RwLock usage mirrors tokens/session. Since this module includes tests for session/tokens, consider mirroring with a small test for api_key to catch regressions (reads, writes, clears).
I can add a minimal test in this module or rely on rust/tests/api_keys.rs—your call.
src/lib/ai.ts (4)
38-42: Body encryption assumes string; guard or normalize non-string bodiesIf init.body is not a string (Blob/FormData/ReadableStream), this will throw. Either restrict to string/object or perform safe normalization.
Option A (restrict and fail fast with a clearer message):
- if (init?.body) { - const encryptedBody = encryptMessage(sessionKey, init.body as string); + if (init?.body !== undefined && init?.body !== null) { + if (typeof init.body !== "string") { + throw new Error("createCustomFetch only supports string request bodies for encryption"); + } + const encryptedBody = encryptMessage(sessionKey, init.body); requestOptions.body = JSON.stringify({ encrypted: encryptedBody }); headers.set("Content-Type", "application/json"); }Option B (normalize plain objects too):
- Accept
string | object, and if object, JSON.stringify before encrypting.
46-53: Refresh-on-401 only for JWT: correct tradeoffGood: avoids pointless refresh with API keys. Minor nit: reuse of headers is fine; reassigning requestOptions.headers is redundant since it already references headers.
- requestOptions.headers = headers;
66-126: Reduce sensitive console logging in productionCurrent logs include decrypted payloads. Gate behind a debug flag to avoid leaking PII in production.
Example:
- console.groupCollapsed("Decrypting chunk"); + if (process.env.NODE_ENV !== "production") console.groupCollapsed("Decrypting chunk"); ... - console.log("Decrypted data:", decrypted); + if (process.env.NODE_ENV !== "production") console.log("Decrypted data:", decrypted); ... - console.groupEnd(); + if (process.env.NODE_ENV !== "production") console.groupEnd();[security]
29-36: Parity with internalEncryptedApiCall: consider one-time re-attestation on 400internalEncryptedApiCall retries once on 400/encryption error with a renewed attestation. You may want similar resilience here to reduce transient failures for non-streaming calls made with createCustomFetch.
If desired, I can draft a scoped retry (single re-attestation) that won’t affect streaming paths.
Also applies to: 44-55
src/lib/index.ts (1)
11-15: Re-exports: confirm ApiKeyListResponse shape aligns with server and RustTS exports
ApiKeyListResponse(array type per api.ts). Rust currently uses{ keys: [] }. Please verify the server payload and align both SDKs to the same shape to avoid downstream confusion.src/lib/test/integration/apiKeys.test.ts (2)
17-19: Consider adding validation for environment variables.While you throw an error when environment variables are missing, consider adding more specific validation for the URL format to prevent runtime issues.
if (!TEST_EMAIL || !TEST_PASSWORD || !TEST_CLIENT_ID || !API_URL) { throw new Error("Test credentials must be set in .env.local"); } + +// Validate API_URL format +try { + new URL(API_URL); +} catch { + throw new Error("VITE_OPEN_SECRET_API_URL must be a valid URL"); +}
125-146: Consider parameterizing the model name for flexibility.The model name is hardcoded. Consider reading it from an environment variable or configuration to make tests more flexible across different environments.
- const model = "ibnzterrell/Meta-Llama-3.3-70B-Instruct-AWQ-INT4"; + const model = process.env.VITE_TEST_MODEL || "ibnzterrell/Meta-Llama-3.3-70B-Instruct-AWQ-INT4";rust/src/client.rs (1)
129-143: Consider extracting common authorization header logic.The authorization header logic is duplicated across multiple methods. Consider extracting it into a helper method to reduce code duplication and ensure consistency.
+ fn get_authorization_header(&self) -> Result<Option<HeaderValue>> { + // Prefer API key over JWT token if both are present + if let Some(api_key) = self.session_manager.get_api_key()? { + return Ok(Some(HeaderValue::from_str(&format!("Bearer {}", api_key)) + .map_err(|e| Error::Authentication(format!("Invalid API key format: {}", e)))?)); + } else if let Some(token) = self.session_manager.get_access_token()? { + return Ok(Some(HeaderValue::from_str(&format!("Bearer {}", token)) + .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?)); + } + Ok(None) + } async fn perform_key_exchange(&self, nonce: &str) -> Result<()> { // ... existing code ... - // Add authorization header if we have a token or API key - // Prefer API key over JWT token if both are present - if let Some(api_key) = self.session_manager.get_api_key()? { - headers.insert( - AUTHORIZATION, - HeaderValue::from_str(&format!("Bearer {}", api_key)) - .map_err(|e| Error::Authentication(format!("Invalid API key format: {}", e)))?, - ); - } else if let Some(token) = self.session_manager.get_access_token()? { - headers.insert( - AUTHORIZATION, - HeaderValue::from_str(&format!("Bearer {}", token)) - .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, - ); - } + if let Some(auth_header) = self.get_authorization_header()? { + headers.insert(AUTHORIZATION, auth_header); + }Apply the same refactoring to the
encrypted_api_callmethod (lines 330-344) and thecreate_chat_completion_streammethod (lines 903-917).rust/tests/api_keys.rs (1)
67-113: Redundant environment loading in test.The environment loading code is duplicated here when you could use the
setup_test_client()helper function that already handles this.#[tokio::test] async fn test_api_key_authentication() -> Result<()> { - // Load .env.local from OpenSecret-SDK directory - let env_path = std::path::Path::new("../.env.local"); - if env_path.exists() { - dotenv::from_path(env_path).ok(); - } else { - dotenv::dotenv().ok(); - } - - let api_url = env::var("VITE_OPEN_SECRET_API_URL") - .unwrap_or_else(|_| "http://localhost:3000".to_string()); - let email = env::var("VITE_TEST_EMAIL").expect("VITE_TEST_EMAIL must be set"); - let password = env::var("VITE_TEST_PASSWORD").expect("VITE_TEST_PASSWORD must be set"); - let client_id = env::var("VITE_TEST_CLIENT_ID") - .expect("VITE_TEST_CLIENT_ID must be set") - .parse::<Uuid>() - .expect("VITE_TEST_CLIENT_ID must be a valid UUID"); - - // First create an API key using regular auth - let client = OpenSecretClient::new(&api_url)?; - client.perform_attestation_handshake().await?; - client.login(email, password, client_id).await?; + // Setup client with authentication + let client = setup_test_client().await?; + + // Get API URL for creating new client with API key + let api_url = env::var("VITE_OPEN_SECRET_API_URL") + .unwrap_or_else(|_| "http://localhost:3000".to_string());Apply the same refactoring to
test_streaming_chat_with_api_key(lines 119-143) andtest_invalid_api_key_fails(lines 221-232).src/lib/api.ts (1)
1247-1255: Consider improving the response structure handling.The API returns
{ keys: ApiKeyListResponse }but the function signature returnsApiKeyListResponse. While the current implementation correctly unwraps the response, consider documenting this or using a more explicit type for clarity.+type ApiKeyListApiResponse = { + keys: ApiKeyListResponse; +}; + export async function listApiKeys(): Promise<ApiKeyListResponse> { - const response = await authenticatedApiCall<void, { keys: ApiKeyListResponse }>( + const response = await authenticatedApiCall<void, ApiKeyListApiResponse>( `${apiUrl}/protected/api-keys`, "GET", undefined, "Failed to list API keys" ); return response.keys; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
rust/src/client.rs(5 hunks)rust/src/session.rs(2 hunks)rust/src/types.rs(1 hunks)rust/tests/api_keys.rs(1 hunks)src/lib/ai.ts(2 hunks)src/lib/api.ts(4 hunks)src/lib/encryptedApi.ts(1 hunks)src/lib/index.ts(1 hunks)src/lib/test/integration/apiKeys.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
rust/src/types.rs (2)
src/lib/api.ts (3)
ApiKey(1165-1169)ApiKeyListResponse(1175-1175)ApiKeyCreateResponse(1171-1173)src/lib/index.ts (3)
ApiKey(12-12)ApiKeyListResponse(14-14)ApiKeyCreateResponse(13-13)
src/lib/test/integration/apiKeys.test.ts (2)
src/lib/api.ts (5)
fetchLogin(46-56)createApiKey(1219-1226)listApiKeys(1247-1255)deleteApiKey(1277-1284)fetchModels(1122-1146)src/lib/ai.ts (1)
createCustomFetch(9-134)
rust/src/client.rs (2)
rust/src/session.rs (4)
new_with_api_key(21-27)new(13-19)set_api_key(29-36)clear_api_key(46-53)src/lib/api.ts (3)
ApiKeyCreateResponse(1171-1173)ApiKey(1165-1169)ApiKeyListResponse(1175-1175)
rust/src/session.rs (1)
rust/src/client.rs (15)
new(28-39)None(540-540)None(553-553)None(560-560)None(568-568)None(580-580)None(585-585)None(607-607)None(630-630)None(676-676)None(804-804)None(847-847)new_with_api_key(41-52)set_api_key(54-56)clear_api_key(58-60)
rust/tests/api_keys.rs (2)
rust/src/client.rs (13)
new(28-39)new_with_api_key(41-52)None(540-540)None(553-553)None(560-560)None(568-568)None(580-580)None(585-585)None(607-607)None(630-630)None(676-676)None(804-804)None(847-847)rust/src/session.rs (2)
new(13-19)new_with_api_key(21-27)
src/lib/ai.ts (1)
src/lib/encryption.ts (1)
encryptMessage(5-18)
src/lib/api.ts (2)
src/lib/index.ts (7)
Model(24-24)ApiKey(12-12)ApiKeyCreateResponse(13-13)ApiKeyListResponse(14-14)createApiKey(18-18)listApiKeys(18-18)deleteApiKey(18-18)src/lib/encryptedApi.ts (2)
openAiAuthenticatedApiCall(80-110)authenticatedApiCall(17-77)
🪛 GitHub Actions: Library Tests
src/lib/encryptedApi.ts
[error] 42-42: No access token available
src/lib/api.ts
[error] 865-865: fetchPublicKey failed: No access token available.
[error] 810-810: Error during signMessage call due to missing access token.
🪛 GitHub Actions: Rust CI
rust/tests/api_keys.rs
[error] 1-1: Test 'test_streaming_chat_with_api_key' failed: Api { status: 404, message: "" }
[error] 1-1: Test 'test_multiple_api_keys' failed: Api { status: 404, message: "" }
[error] 1-1: Test 'test_api_key_authentication' failed: Api { status: 404, message: "" }
[error] 1-1: Test 'test_create_list_delete_api_key' failed: Api { status: 404, message: "" }
🔇 Additional comments (12)
rust/src/types.rs (2)
276-283: LGTM: API key entity shape is reasonable and minimalid/name/timestamp is sufficient for listings. No secret material in list responses—good.
284-287: Confirm and alignApiKeyListResponseshape across SDKsRust currently deserializes the payload as an object with a
keysfield, whereas the TypeScript SDK expects a bare array. Please verify the actual JSON response in the OpenAPI spec (e.g.openapi.yaml) for theApiKeyListResponse(typically theGET /api/keysendpoint) and update both clients to match:• File & Lines
- rust/src/types.rs:284–287
• If the server returns a bare array (
ApiKey[]):
– Rust should simplify to a type alias:
diff -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ApiKeyListResponse { - pub keys: Vec<ApiKey>, -} +pub type ApiKeyListResponse = Vec<ApiKey>;• If the server returns an object with a
keysarray ({ keys: ApiKey[] }):
– Update the TS SDK to:
ts export type ApiKeyListResponse = { keys: ApiKey[] };rust/src/session.rs (2)
21-27: Constructor with API key: good additionnew_with_api_key cleanly seeds the key into the lock. Matches Client::new_with_api_key usage.
147-152: clear_all now clears API key as wellGood catch—prevents stale auth state when switching auth modes.
src/lib/ai.ts (2)
5-9: Public surface: CustomFetchOptions and API key precedence look goodAPI key taking precedence over JWT is consistent with the PR intent and other call paths.
Also applies to: 9-17
18-23: Great: unified missing-credentials error messageMatches intended behavior and should reduce confusion across call sites.
src/lib/index.ts (1)
17-22: API key management + AI customization re-exports look goodSurface area is cohesive and discoverable.
src/lib/test/integration/apiKeys.test.ts (1)
36-71: LGTM! Comprehensive test for API key lifecycle.The test thoroughly covers creation, validation of UUID format, listing, verification that the key is not exposed in listings, and deletion with proper cleanup.
rust/src/client.rs (2)
41-52: LGTM! Clean implementation of API key constructor.The constructor properly initializes the session manager with the API key while maintaining consistency with the original constructor's logic for mock attestation.
545-562: LGTM! Clean implementation of API key management endpoints.The API key management methods are well-structured and follow the existing patterns in the codebase. The endpoints correctly use encrypted API calls and handle the response wrapping appropriately.
rust/tests/api_keys.rs (1)
1-247: Missing environment variable fileOur investigation shows that neither
.env.localnor.envexists at the project root or relative to the Rust crate (i.e. atrust/../.env*), so the tests are falling back tohttp://localhost:3000and hitting undefined routes, resulting in 404 errors.• No
.env.localor.envfound at project root
• No.env.localor.envfound atrust/../.env*
• Environment variables in Rust tests (VITE_OPEN_SECRET_API_URL,VITE_TEST_EMAIL,VITE_TEST_PASSWORD,VITE_TEST_CLIENT_ID) match those in the TypeScript tests
• API endpoints (/protected/api-keys) are correctly defined in both client implementationsNext steps:
- Add a
.env.local(or.env) at the project root containing the four required variables.- Verify that it’s loaded properly (or adjust the
Path::new("../.env.local")if your test runner’s working directory differs).- Ensure the target server is running with the API key endpoints deployed at the specified
VITE_OPEN_SECRET_API_URL.src/lib/api.ts (1)
1122-1146: LGTM! Clean implementation of API key support for models endpoint.The function properly supports both JWT and API key authentication via the
openAiAuthenticatedApiCallwrapper, with appropriate validation of the response structure.
- TypeScript: Return server response structure directly from listApiKeys()
- Changed return type from ApiKeyListResponse to { keys: ApiKeyListResponse }
- Updated all test usage to access response.keys
- Maintains transparency about actual API response structure
- Rust: Extract common environment setup into helper functions
- Added load_env_vars() to handle .env.local loading
- Added get_test_config() to centralize test configuration
- Eliminated code duplication across 4 test functions
- Improved maintainability of test setup code
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit c24d899), not the entire PR.
The latest changes focus on code quality improvements and consistency fixes. In the Rust test files, duplicated environment variable loading code has been refactored into two reusable helper functions: load_env_vars() and get_test_config(). This eliminates approximately 50 lines of duplicate code across multiple test functions and centralizes environment configuration logic.
In the TypeScript SDK, the listApiKeys function has been updated to return a structured response object { keys: ApiKeyListResponse } instead of directly returning the array. This change aligns the client with the actual server API response format and follows standard REST API patterns where collections are wrapped in objects for future extensibility.
Corresponding test files have been updated to handle the new response structure, with test assertions now accessing response.keys instead of treating the response as a direct array. These changes maintain the same functionality while improving code organization and API consistency.
Confidence score: 4/5
- This PR contains solid refactoring improvements with minimal risk of breaking functionality
- Score reflects good code quality improvements but involves structural changes that need verification
- Pay close attention to the API response structure changes in TypeScript files
3 files reviewed, no comments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/lib/api.ts (2)
1165-1176: API key types look good; confirm no duplicate declarations remain.Types match the documented behavior (key only present at creation). Past reviews flagged duplicate declarations elsewhere in this file; please ensure only one block exists now.
Run to verify duplicates were removed:
#!/bin/bash set -euo pipefail echo "Searching for duplicate API key type declarations..." rg -nP 'export\s+type\s+ApiKey\b|export\s+type\s+ApiKeyCreateResponse\b|export\s+type\s+ApiKeyListResponse\b' -C1 src/lib/api.ts echo echo "Counts (should each be 1):" for t in ApiKey ApiKeyCreateResponse ApiKeyListResponse; do c=$(rg -nP "export\\s+type\\s+${t}\\b" src/lib/api.ts | wc -l | tr -d ' ') echo "${t}: ${c}" if [ "$c" -ne 1 ]; then echo "Unexpected duplicate(s) detected for ${t}" >&2 exit 1 fi done echo "OK: no duplicates found."
1247-1254: listApiKeys: wrapper shape is fine; ensure server contract is stable.The function returns { keys: ApiKeyListResponse }. This matches tests, but a previous review questioned whether the server returns a bare array. If the backend ever flattens the response, consider a tolerant client that normalizes both shapes.
Optional normalization:
-export async function listApiKeys(): Promise<{ keys: ApiKeyListResponse }> { - return authenticatedApiCall<void, { keys: ApiKeyListResponse }>( +export async function listApiKeys(): Promise<{ keys: ApiKeyListResponse }> { + const result = await authenticatedApiCall<void, { keys: ApiKeyListResponse } | ApiKeyListResponse>( `${apiUrl}/protected/api-keys`, "GET", undefined, "Failed to list API keys" - ); + ); + return Array.isArray((result as any)) ? { keys: result as ApiKeyListResponse } : (result as { keys: ApiKeyListResponse }); }
🧹 Nitpick comments (7)
src/lib/test/integration/apiKeys.test.ts (5)
17-19: Avoid throwing at module load time; move env checks into beforeAll and mark tests skipped with a clear reason.Throwing here aborts discovery of the entire test suite. Prefer a guard inside each describe’s beforeAll so this file reports as “skipped” rather than crashing the runner.
Example:
-if (!TEST_EMAIL || !TEST_PASSWORD || !TEST_CLIENT_ID || !API_URL) { - throw new Error("Test credentials must be set in .env.local"); -} +const MISSING_ENV = + !TEST_EMAIL || !TEST_PASSWORD || !TEST_CLIENT_ID || !API_URL;Then at the top of each describe:
describe("API Key Management", () => { - beforeAll(async () => { + beforeAll(async () => { + if (MISSING_ENV) { + test.skip("Missing required env in .env.local"); + return; + } await setupTestUser(); });
52-53: Remove noisy logs (may leak metadata in CI logs).Console logging the full list response adds noise and could surface user metadata in CI. Drop it or gate behind an env flag.
- console.log("Listed response:", response); + // Debug: console.log("Listed response:", response);
130-135: Make the streaming test deterministic (reduce flakiness).Without temperature/top_p constraints, some models won’t respond with exactly echo. Pin temperature and limit tokens.
- const stream = openai.beta.chat.completions.stream({ + const stream = openai.beta.chat.completions.stream({ model, messages, - stream: true + stream: true, + temperature: 0, + top_p: 1, + max_tokens: 5 });
161-179: Tighten assertions for invalid API key path.Catching “any error” can mask unrelated failures (e.g., attestation issues). Assert on 401 status or an error message containing 401/Unauthorized.
- try { - const response = await customFetch(`${API_URL}/v1/models`, { + try { + const response = await customFetch(`${API_URL}/v1/models`, { method: "GET", headers: { "Content-Type": "application/json" } }); - - // Should get 401 Unauthorized - expect(response.status).toBe(401); + // Should get 401 Unauthorized + expect(response.status).toBe(401); } catch (error) { - // If it throws before the response, that's also acceptable - expect(error).toBeDefined(); + // If it throws, make sure it looks like an auth failure + const msg = String(error?.message ?? error); + expect(msg).toMatch(/401|unauthorized/i); }
114-123: Minor: Accept-Encoding override may be unnecessary.Setting "Accept-Encoding": "identity" can reduce network efficiency and is generally not required for either JSON or SSE in modern runtimes. If not strictly needed for your encryption/SSE path, consider removing it.
src/lib/api.ts (2)
1114-1146: fetchModels: good API-key/JWT dual auth and response validation.
- Optional apiKey parameter preserves backward compatibility.
- Validation of object === "list" and data is Array is solid.
- Error propagation maintains call-site context.
Two small suggestions:
- Consider narrowing thrown errors to include URL and auth mode for easier telemetry.
- Optionally accept AbortSignal via an overload for cancellable UI flows.
1276-1283: deleteApiKey: clear contract and docs; consider returning 204 vs void.Current signature is fine. If the backend returns a JSON envelope, returning it (or validating 204) could improve diagnostics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
rust/tests/api_keys.rs(1 hunks)src/lib/api.ts(4 hunks)src/lib/test/integration/apiKeys.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/tests/api_keys.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib/api.ts (2)
src/lib/index.ts (6)
ApiKey(12-12)ApiKeyCreateResponse(13-13)ApiKeyListResponse(14-14)createApiKey(18-18)listApiKeys(18-18)deleteApiKey(18-18)src/lib/encryptedApi.ts (2)
openAiAuthenticatedApiCall(80-110)authenticatedApiCall(17-77)
src/lib/test/integration/apiKeys.test.ts (2)
src/lib/api.ts (5)
fetchLogin(46-56)createApiKey(1219-1226)listApiKeys(1247-1254)deleteApiKey(1276-1283)fetchModels(1122-1146)src/lib/ai.ts (1)
createCustomFetch(9-134)
⏰ 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: Cloudflare Pages
🔇 Additional comments (2)
src/lib/api.ts (2)
2-2: Importing openAiAuthenticatedApiCall is correct and scoped.This aligns fetchModels with the OpenAI path and enables API-key-based auth without affecting JWT flows.
1199-1226: createApiKey: interface and docs match server contract (“key” only once).Good clarity in the JSDoc and return type. No functional concerns.
Based on backend changes, the SDK now uses API key names as identifiers instead of internal database IDs. This provides better security and user experience. Changes across both TypeScript and Rust SDKs: API Key Types: - Removed 'id' field from ApiKey and ApiKeyCreateResponse types - API keys are now identified by their unique name (per user) - UUID key is only exposed once during creation Delete Operation: - Changed delete endpoint from /protected/api-keys/:id to /protected/api-keys/:name - Both deleteApiKey (TS) and delete_api_key (Rust) now accept name parameter - Names are URL-encoded to handle special characters List Operation: - Added client-side sorting by created_at (newest first) - Maintains consistent ordering across both SDKs All tests updated and passing: - TypeScript: 6/6 tests passing - Rust: 5/5 tests passing
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit df4ca48), not the entire PR.
The most recent changes shift the API key management system from ID-based to name-based identification across both TypeScript and Rust SDKs. This affects how API keys are referenced, looked up, and deleted throughout the codebase.
Key changes in this update:
-
API Key Type Modifications: Removed the
idfield fromApiKeyandApiKeyCreateResponsestructs in Rust (types.rs), simplifying the data model to use only names as identifiers. -
Function Parameter Changes: Updated
deleteApiKey()functions in both SDKs to accept name parameters instead of ID parameters, with proper URL encoding for special characters. -
Test Suite Updates: Modified all test files to use name-based key lookups and deletions instead of ID-based operations. Tests now use
createdKey.nameinstead ofcreatedKey.idfor key identification. -
Client-side Sorting: Added sorting by
created_atin descending order (newest first) in thelistApiKeys()functions for better user experience. -
Authentication Priority: Enhanced the authentication logic to prefer API keys over JWT tokens when both are available.
These changes maintain full backward compatibility while making the API key management system more user-friendly by allowing users to manage keys by meaningful names rather than numeric IDs. The implementation spans both the TypeScript SDK (api.ts, test files) and Rust SDK (client.rs, types.rs, test files), ensuring consistency across both platforms.
Confidence score: 4/5
- This PR appears safe to merge with minimal risk as it maintains backward compatibility and follows established patterns
- Score reflects well-structured changes with comprehensive test coverage, though the fundamental shift from ID-based to name-based identification is significant
- Pay attention to the URL encoding implementation in delete operations and ensure API consumers are aware of the breaking change in function signatures
5 files reviewed, 1 comment
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 (1)
rust/src/client.rs (1)
330-344: Restrict API-key auth to OpenAI endpoints; require JWT elsewhere
encrypted_api_callis used for both OpenAI (/v1/...) and internal/protected/...endpoints. Current logic prefers API key for everything, which violates the stated contract and will break non-OpenAI calls when only an API key is set.Condition on the endpoint prefix:
-// Add authorization header if we have a token or API key -// Prefer API key over JWT token if both are present -if let Some(api_key) = self.session_manager.get_api_key()? { - headers.insert( - AUTHORIZATION, - HeaderValue::from_str(&format!("Bearer {}", api_key)) - .map_err(|e| Error::Authentication(format!("Invalid API key format: {}", e)))?, - ); -} else if let Some(token) = self.session_manager.get_access_token()? { - headers.insert( - AUTHORIZATION, - HeaderValue::from_str(&format!("Bearer {}", token)) - .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, - ); -} +// Authorization: API key only for OpenAI endpoints (/v1/*), JWT for others +if endpoint.starts_with("/v1/") { + if let Some(api_key) = self.session_manager.get_api_key()? { + headers.insert( + AUTHORIZATION, + HeaderValue::from_str(&format!("Bearer {}", api_key)) + .map_err(|e| Error::Authentication(format!("Invalid API key format: {}", e)))?, + ); + } else if let Some(token) = self.session_manager.get_access_token()? { + headers.insert( + AUTHORIZATION, + HeaderValue::from_str(&format!("Bearer {}", token)) + .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, + ); + } // neither present: let server handle unauth or rely on earlier flow constraints +} else { + if let Some(token) = self.session_manager.get_access_token()? { + headers.insert( + AUTHORIZATION, + HeaderValue::from_str(&format!("Bearer {}", token)) + .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, + ); + } else { + return Err(Error::Authentication( + "JWT access token required for this endpoint".to_string(), + )); + } +}
♻️ Duplicate comments (2)
src/lib/api.ts (2)
1165-1175: API key types look correct; duplicate declarations are gone.The earlier duplicate type block flagged in prior reviews is no longer present. Current single-source definitions for
ApiKey,ApiKeyCreateResponse, andApiKeyListResponseare concise and match the backend contract (UUID returned only on create).
1247-1261: listApiKeys: Wrapper shape now matches earlier review feedback.Returning
{ keys: ApiKeyListResponse }(not just the array) addresses the prior concern about response wrapping. Thanks for fixing that.
🧹 Nitpick comments (7)
src/lib/api.ts (3)
1115-1146: fetchModels: Nice addition of API-key path and runtime response validation.
- Optional API-key parameter + fallback to JWT via
openAiAuthenticatedApiCallis clean.- Response shape checks for
object === "list"and arraydataare good defensive coding.Optional for a future PR:
- Consider an options object for extensibility (
{ apiKey?, signal? }) to avoid future param churn and enable aborts.
1198-1225: createApiKey: Add a minimal client-side name guard.Backend will validate, but a quick client-side check improves DX and error messaging.
Apply this small guard:
export async function createApiKey(name: string): Promise<ApiKeyCreateResponse> { + if (!name || name.trim().length === 0) { + throw new Error("API key name must be a non-empty string"); + } return authenticatedApiCall<{ name: string }, ApiKeyCreateResponse>( `${apiUrl}/protected/api-keys`, "POST", { name }, "Failed to create API key" ); }
1247-1261: listApiKeys: Avoid in-place mutation and add a light runtime guard.
- Sorting
response.keysmutates the array returned from the call. Prefer immutability for safer composition.- Quick guard on
response.keysshape clarifies failure modes.Apply:
export async function listApiKeys(): Promise<{ keys: ApiKeyListResponse }> { const response = await authenticatedApiCall<void, { keys: ApiKeyListResponse }>( `${apiUrl}/protected/api-keys`, "GET", undefined, "Failed to list API keys" ); - - // Sort by created_at descending (newest first) - response.keys.sort((a, b) => - new Date(b.created_at).getTime() - new Date(a.created_at).getTime() - ); - - return response; + + // Validate and sort by created_at descending (newest first) without mutating input + if (!response || !Array.isArray(response.keys)) { + throw new Error("Invalid API key list response"); + } + const sorted = [...response.keys].sort( + (a, b) => new Date(b.created_at).getTime() - new Date(a.created_at).getTime() + ); + return { keys: sorted }; }rust/src/types.rs (1)
293-298: Document “key” semantics; optionally strong-type asUuidAdd a short doc comment that the
keyis a secret, UUID-with-dashes, and only returned on creation. Optionally, change it toUuidfor stronger validation (serde already supportsUuidas a hyphenated string). If you keepString, validate on set (see client.rs suggestions).Minimal doc improvement:
#[derive(Debug, Clone, Serialize, Deserialize)] pub struct ApiKeyCreateResponse { - pub key: String, // UUID format with dashes, only returned on creation + /// Secret API key. UUID format with dashes. Only returned on creation. + pub key: String, pub name: String, pub created_at: DateTime<Utc>, }rust/src/client.rs (3)
41-52: Validate API key format in constructorGuard against accidental non-UUID values at construction time to fail fast and meet “UUID with dashes” requirement.
pub fn new_with_api_key(base_url: impl Into<String>, api_key: String) -> Result<Self> { let base_url = base_url.into(); let use_mock = base_url.contains("localhost") || base_url.contains("127.0.0.1"); + // Validate expected UUID-with-dashes format upfront + Uuid::parse_str(&api_key).map_err(|_| { + Error::Authentication("Invalid API key format: expected UUID with dashes".to_string()) + })?; + Ok(Self { client: Client::new(), base_url: base_url.trim_end_matches('/').to_string(), session_manager: SessionManager::new_with_api_key(api_key), use_mock_attestation: use_mock, server_public_key: RefCell::new(None), }) }
54-56: Also validate onset_api_keyMirror constructor checks when setting at runtime.
pub fn set_api_key(&self, api_key: String) -> Result<()> { - self.session_manager.set_api_key(api_key) + Uuid::parse_str(&api_key).map_err(|_| { + Error::Authentication("Invalid API key format: expected UUID with dashes".to_string()) + })?; + self.session_manager.set_api_key(api_key) }
910-924: SSE path auth is fine; consider centralizing header buildingFor streaming OpenAI endpoint, preferring API key then falling back to JWT matches the contract. To reduce duplication/inconsistency across three call sites, extract an auth-header helper that takes
endpointand applies the same rules as inencrypted_api_call.Example helper (outside the shown ranges):
fn apply_auth_header( headers: &mut HeaderMap, endpoint: &str, session_manager: &SessionManager, ) -> Result<()> { if endpoint.starts_with("/v1/") { if let Some(api_key) = session_manager.get_api_key()? { headers.insert( AUTHORIZATION, HeaderValue::from_str(&format!("Bearer {}", api_key)) .map_err(|e| Error::Authentication(format!("Invalid API key format: {}", e)))?, ); } else if let Some(token) = session_manager.get_access_token()? { headers.insert( AUTHORIZATION, HeaderValue::from_str(&format!("Bearer {}", token)) .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, ); } } else if let Some(token) = session_manager.get_access_token()? { headers.insert( AUTHORIZATION, HeaderValue::from_str(&format!("Bearer {}", token)) .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, ); } else { return Err(Error::Authentication( "JWT access token required for this endpoint".to_string(), )); } Ok(()) }Then reuse in
perform_key_exchange(with endpoint"/key_exchange"),encrypted_api_call, and the SSE method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
rust/src/client.rs(5 hunks)rust/src/types.rs(1 hunks)rust/tests/api_keys.rs(1 hunks)src/lib/api.ts(4 hunks)src/lib/test/integration/apiKeys.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/tests/api_keys.rs
- src/lib/test/integration/apiKeys.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/lib/api.ts (2)
src/lib/index.ts (6)
ApiKey(12-12)ApiKeyCreateResponse(13-13)ApiKeyListResponse(14-14)createApiKey(18-18)listApiKeys(18-18)deleteApiKey(18-18)src/lib/encryptedApi.ts (2)
openAiAuthenticatedApiCall(80-110)authenticatedApiCall(17-77)
rust/src/types.rs (2)
src/lib/api.ts (3)
ApiKey(1165-1168)ApiKeyListResponse(1174-1174)ApiKeyCreateResponse(1170-1172)src/lib/index.ts (3)
ApiKey(12-12)ApiKeyListResponse(14-14)ApiKeyCreateResponse(13-13)
rust/src/client.rs (2)
rust/src/session.rs (4)
new_with_api_key(21-27)new(13-19)set_api_key(29-36)clear_api_key(46-53)src/lib/api.ts (3)
ApiKeyCreateResponse(1170-1172)ApiKey(1165-1168)ApiKeyListResponse(1174-1174)
🔇 Additional comments (7)
src/lib/api.ts (2)
2-2: Importing OpenAI-aware auth helper is appropriate here.Using
openAiAuthenticatedApiCallonly for OpenAI-compatible endpoints keeps JWT vs API-key concerns localized to those paths. Good separation of concerns.
1284-1293: deleteApiKey: URL-encoding the identifier is the right call.Simple and correct. No further action needed.
rust/src/types.rs (1)
276-299: No API shape mismatch – both Rust and TypeScript wrap the list under akeyspropertyBoth sides expect and handle an object of the form
{ keys: […] }:
- In Rust (
rust/src/types.rs):pub struct ApiKeyListResponse { pub keys: Vec<ApiKey>, }- In TypeScript (
src/lib/api.ts):export type ApiKeyListResponse = ApiKey[]; export async function listApiKeys(): Promise<{ keys: ApiKeyListResponse }> { … }The TS client calls
authenticatedApiCall<void, { keys: ApiKeyListResponse }>, so it likewise deserializes an object with akeysarray. No migration to a bare-array type is needed on either side.Likely an incorrect or invalid review comment.
rust/src/client.rs (4)
58-60: LGTM — clear API keyStraightforward passthrough; no concerns.
544-550: LGTM — API key creation wrapperType usage matches the Rust types. See list endpoint comment for the response-shape alignment follow-up.
563-569: LGTM — safe name encoding on deleteURL-encoding the key name avoids path issues; behavior is correct.
551-561: No type alias exists forVec<ApiKey>; current implementation is correctThe review suggestion to switch from deserializing into
ApiKeyListResponseto directly using aVec<ApiKey>alias is invalid because:
- There is no
type ApiKeyListResponse = Vec<ApiKey>alias in the codebase. Instead,ApiKeyListResponseis a struct defined inrust/src/types.rsthat wraps akeys: Vec<ApiKey>field.- Deserializing directly into
ApiKeyListResponseviaencrypted_api_call<_, ApiKeyListResponse>matches the server’s JSON (an object with akeysarray) and allows sorting bycreated_at.- Adopting the proposed diff would not compile, as
encrypted_api_callwould be asked to deserialize an object into aVec<ApiKey>.Please disregard the suggested diff; the existing code correctly handles the response format and requires no changes here.
Likely an incorrect or invalid review comment.
CRITICAL SECURITY FIX: API keys were being sent to ALL encrypted endpoints, including sensitive operations like key exchange. This was a major security vulnerability identified by CodeRabbit in PR review. Changes: - Removed API key authentication from key exchange (should only use JWT) - Created separate encrypted_openai_call() method for OpenAI endpoints - Updated encrypted_api_call() to only use JWT tokens (never API keys) - OpenAI endpoints (/v1/models, /v1/chat/completions) now use encrypted_openai_call() - All other endpoints continue using encrypted_api_call() with JWT only Authentication hierarchy: 1. Key exchange: No auth or JWT only 2. Regular encrypted endpoints: JWT only 3. OpenAI endpoints: API key (priority) or JWT (fallback) All tests passing - verified both API key and JWT authentication work correctly in their appropriate contexts.
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit c4a1e17), not the entire PR. The most recent changes focus on implementing API key authentication support for OpenAI endpoints in the Rust SDK's client module.
The key changes include:
-
New
encrypted_openai_callmethod - A specialized API call method that supports dual authentication (API key preferred over JWT) specifically for OpenAI endpoints (/v1/*). This method properly handles authorization headers by prioritizing API keys when available, falling back to JWT tokens. -
Updated OpenAI endpoint methods - The
get_modelsandcreate_chat_completionmethods now use the new OpenAI-specific call method instead of the generalencrypted_api_call, enabling API key authentication for these operations. -
Enhanced streaming chat completion - Modified the streaming functionality to support API key authentication with proper authorization header handling, maintaining the same priority logic (API key over JWT).
-
Maintained endpoint separation - The implementation correctly restricts API key usage to OpenAI endpoints only, while ensuring other platform endpoints continue using JWT authentication exclusively through the existing
encrypted_api_callmethod.
The changes maintain full backward compatibility with existing JWT authentication while adding the new API key capability. The dual authentication logic aligns with the PR's stated goal of preferring API keys over JWT tokens when both are available for OpenAI endpoints.
Confidence score: 4/5
- This change appears well-implemented with proper authentication logic separation
- Score reflects solid implementation of dual authentication with appropriate endpoint restrictions
- Pay close attention to the key exchange method to ensure it doesn't inadvertently use API keys for platform endpoints
1 file reviewed, no comments
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rust/src/client.rs (1)
129-136: Previous auth leak addressed: key exchange now uses JWT onlyThe header logic correctly avoids sending API keys on the platform key exchange endpoint. Thanks for fixing this.
🧹 Nitpick comments (6)
rust/src/client.rs (6)
41-52: Validate API key format (UUID with dashes) at constructionContract says API keys are UUIDs with dashes. Fail fast if the provided key is malformed and trim incidental whitespace. This avoids persisting unusable keys and produces clearer errors.
Apply within this range:
pub fn new_with_api_key(base_url: impl Into<String>, api_key: String) -> Result<Self> { let base_url = base_url.into(); let use_mock = base_url.contains("localhost") || base_url.contains("127.0.0.1"); + // Validate UUID-based API key up front + let api_key = api_key.trim().to_string(); + Uuid::parse_str(&api_key).map_err(|e| { + Error::Authentication(format!("API key must be a UUID with dashes: {}", e)) + })?; + Ok(Self { client: Client::new(), base_url: base_url.trim_end_matches('/').to_string(), - session_manager: SessionManager::new_with_api_key(api_key), + session_manager: SessionManager::new_with_api_key(api_key), use_mock_attestation: use_mock, server_public_key: RefCell::new(None), }) }
54-61: Guard against empty/invalid API keys in settersAdd basic validation to prevent setting an empty or malformed API key at runtime; normalize with trim. Keeps Session state clean and avoids header construction errors later.
- pub fn set_api_key(&self, api_key: String) -> Result<()> { - self.session_manager.set_api_key(api_key) - } + pub fn set_api_key(&self, api_key: String) -> Result<()> { + let api_key = api_key.trim(); + if api_key.is_empty() { + return Err(Error::Authentication("API key cannot be empty".to_string())); + } + Uuid::parse_str(api_key).map_err(|e| { + Error::Authentication(format!("API key must be a UUID with dashes: {}", e)) + })?; + self.session_manager.set_api_key(api_key.to_string()) + } pub fn clear_api_key(&self) -> Result<()> { self.session_manager.clear_api_key() }
386-394: Add a defensive guard to ensure this helper is only used for /v1/ endpoints*Since this method is the API-key-capable path, add a prefix check to prevent accidental use with non-OpenAI endpoints in the future.
- let url = format!("{}{}", self.base_url, endpoint); + // Guard misuse: this helper must only handle OpenAI endpoints + if !endpoint.starts_with("/v1/") { + return Err(Error::Authentication( + "encrypted_openai_call used for non-/v1/* endpoint".to_string(), + )); + } + let url = format!("{}{}", self.base_url, endpoint);
415-428: Deduplicate OpenAI auth-header constructionThe API-key/JWT selection logic is duplicated between encrypted_openai_call and create_chat_completion_stream. Factor it into a small helper to keep behavior consistent.
Apply in these ranges:
- // For OpenAI endpoints: Prefer API key over JWT token if both are present - if let Some(api_key) = self.session_manager.get_api_key()? { - headers.insert( - AUTHORIZATION, - HeaderValue::from_str(&format!("Bearer {}", api_key)) - .map_err(|e| Error::Authentication(format!("Invalid API key format: {}", e)))?, - ); - } else if let Some(token) = self.session_manager.get_access_token()? { - headers.insert( - AUTHORIZATION, - HeaderValue::from_str(&format!("Bearer {}", token)) - .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, - ); - } + self.add_openai_auth_header(&mut headers)?- // Add authorization header if we have a token or API key - // Prefer API key over JWT token if both are present - if let Some(api_key) = self.session_manager.get_api_key()? { - headers.insert( - AUTHORIZATION, - HeaderValue::from_str(&format!("Bearer {}", api_key)) - .map_err(|e| Error::Authentication(format!("Invalid API key format: {}", e)))?, - ); - } else if let Some(token) = self.session_manager.get_access_token()? { - headers.insert( - AUTHORIZATION, - HeaderValue::from_str(&format!("Bearer {}", token)) - .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, - ); - } + self.add_openai_auth_header(&mut headers)?Add this helper (place within impl OpenSecretClient):
fn add_openai_auth_header(&self, headers: &mut HeaderMap) -> Result<()> { if let Some(api_key) = self.session_manager.get_api_key()? { headers.insert( AUTHORIZATION, HeaderValue::from_str(&format!("Bearer {}", api_key)) .map_err(|e| Error::Authentication(format!("Invalid API key format: {}", e)))?, ); } else if let Some(token) = self.session_manager.get_access_token()? { headers.insert( AUTHORIZATION, HeaderValue::from_str(&format!("Bearer {}", token)) .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, ); } Ok(()) }Also applies to: 993-1007
465-468: Unify error classification for base64 decode failuresHere base64 decoding maps to Encryption errors, while in the streaming path decode failures become Api errors. Consider picking one category (e.g., Encryption) for consistency.
993-1007: Auth selection for streaming endpoint is correctFor SSE, preferring API key over JWT aligns with the contract. After extracting the helper (above), this stays consistent with non-streaming.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
rust/src/client.rs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rust/src/client.rs (3)
rust/src/session.rs (4)
new_with_api_key(21-27)new(13-19)set_api_key(29-36)clear_api_key(46-53)rust/src/crypto.rs (2)
encrypt_data(65-82)decrypt_data(84-98)src/lib/api.ts (3)
ApiKeyCreateResponse(1170-1172)ApiKey(1165-1168)ApiKeyListResponse(1174-1174)
🔇 Additional comments (5)
rust/src/client.rs (5)
323-331: Correct: Non-OpenAI endpoints use JWT onlyThe explicit JWT-only Authorization for encrypted_api_call aligns with the PR contract that API keys are only valid for OpenAI (/v1/*) endpoints.
627-653: API key management endpoints look good
- create_api_key returns the one-time key material as expected.
- list_api_keys sorts by created_at descending; DateTime ordering is correct.
- delete_api_key percent-encodes names; safe for special characters.
937-939: Correct routing through OpenAI-specific pathget_models uses the OpenAI-enforced call path. Matches the “API key for /v1/* only” rule.
948-950: Non-streaming chat completion correctly uses OpenAI pathAPI-key-preferred authorization will apply here via the OpenAI helper. Looks good.
378-474: No issues found withencrypted_openai_callusage or unhandled/v1/endpointsAll of the Rust code paths that hit “/v1/” either go through
encrypted_openai_callor the dedicated streaming implementation:
encrypted_openai_callis only ever invoked with/v1/modelsand/v1/chat/completions(rust/src/client.rs:937,948) and never with a non-/v1/path.- The streaming path for
/v1/chat/completionsdirectly constructs that URL (rust/src/client.rs:974) as intended.- No accidental non-
/v1/calls toencrypted_openai_callwere detected.Since all
/v1/endpoints in the Rust client are correctly routed, no further changes are needed here.
API key management functions (createApiKey, listApiKeys, deleteApiKey) are now
available through the useOpenSecret() hook, making them accessible like all
other API functions in the SDK.
Changes:
- Added API key function signatures to OpenSecretContextType
- Added API key functions to default context value
- Added API key functions to provider's context value
Users can now access API key management through:
const os = useOpenSecret();
await os.createApiKey('My API Key');
const keys = await os.listApiKeys();
await os.deleteApiKey('My API Key');
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit f94d8f9), not the entire PR.
The most recent changes add API key management functionality to the React context in src/lib/main.tsx. Specifically, the changes extend the OpenSecretContextType interface and context provider implementation to include three new API key management functions:
createApiKey(name: string): Creates new API keys with comprehensive JSDoc warning about key visibilitylistApiKeys(): Retrieves API key metadata without exposing actual key valuesdeleteApiKey(name: string): Permanently deletes API keys by name
These functions are integrated into the OpenSecret React context using the same pattern as existing authentication and storage methods. The implementation maintains consistency with the current error handling approach and follows the established context architecture. The functions are added to both the default context value and the actual provider context value, making them available throughout React applications via the useOpenSecret hook.
This change is part of the broader API key authentication feature that enables dual authentication modes (JWT and API key) specifically for OpenAI endpoints while maintaining backward compatibility with existing JWT-only authentication for other platform endpoints.
Confidence score: 5/5
- This change is safe to merge with minimal risk as it only adds new functionality without modifying existing behavior
- Score reflects simple, additive changes that follow established patterns and maintain backward compatibility
- No files require special attention as the changes are straightforward context extensions
1 file reviewed, no comments
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/lib/main.tsx (4)
598-607: JSDoc is solid; small clarity nit about UUID format and single-returned keyDocs correctly warn that the key is returned only once. Given the PR guarantees keys are UUIDs with dashes, consider explicitly stating that here to set consumer expectations.
620-630: Deletion by “name”: confirm uniqueness contracts and cross-SDK parityDocs assert names are unique per user and deletion happens by name. Confirm the server enforces this uniqueness (and that Rust SDK also deletes by name). If the backend primarily identifies keys by UUID, consider supporting both id and name for deletion to avoid accidental collisions or future migration friction.
Example API ergonomics in TS (outside this file) if you decide to broaden support:
// in api.ts export async function deleteApiKey(arg: { id: string } | { name: string }) { if ("id" in arg) return authenticatedApiCall(`/v1/api-keys/${arg.id}`, { method: "DELETE" }); return authenticatedApiCall(`/v1/api-keys/by-name/${encodeURIComponent(arg.name)}`, { method: "DELETE" }); }
692-696: Guard default context to fail fast outside ProviderIn the default context value, these functions currently call the real API functions. If a consumer accidentally uses the context outside a Provider, this will perform live network calls with incomplete configuration. Prefer throwing explicit errors (pattern already used by
getAttestationDocument) to surface misuse early.Apply:
- uploadDocumentWithPolling: api.uploadDocumentWithPolling, - createApiKey: api.createApiKey, - listApiKeys: api.listApiKeys, - deleteApiKey: api.deleteApiKey + uploadDocumentWithPolling: api.uploadDocumentWithPolling, + createApiKey: ((..._args: any[]) => { + throw new Error("createApiKey called outside of OpenSecretProvider"); + }) as unknown as typeof api.createApiKey, + listApiKeys: ((..._args: any[]) => { + throw new Error("listApiKeys called outside of OpenSecretProvider"); + }) as unknown as typeof api.listApiKeys, + deleteApiKey: ((..._args: any[]) => { + throw new Error("deleteApiKey called outside of OpenSecretProvider"); + }) as unknown as typeof api.deleteApiKey
1053-1056: Consider enabling API-key–only usage paths (don’t gate aiCustomFetch on auth.user)The PR adds API key auth for OpenAI endpoints. Today
aiCustomFetchis only instantiated whenauth.useris present, which blocks API-key–only consumers from using the OpenAI path via context. Prefer creatingaiCustomFetchunconditionally onceapiUrlis configured, lettingcreateCustomFetchselect JWT or API-key auth internally.Suggested change (in the existing effect around Lines 762–769):
-// Create aiCustomFetch when user is authenticated -useEffect(() => { - if (auth.user) { - setAiCustomFetch(() => createCustomFetch()); - } else { - setAiCustomFetch(undefined); - } -}, [auth.user]); +// Create aiCustomFetch when API is configured (supports JWT or API key internally) +useEffect(() => { + setAiCustomFetch(() => createCustomFetch()); +}, [apiUrl, auth.user]);If
createCustomFetchneeds an API key passed in, consider threading it from a caller-supplied prop or a setter in context rather than binding it to auth state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/main.tsx(3 hunks)
🔇 Additional comments (1)
src/lib/main.tsx (1)
609-618: I've added a script to verify the shape of the API key response types, ensuring that the list response type excludes thekeyfield and that the creation response type includes it. Let me know the results to finalize the review comment.
Previously, aiCustomFetch was only created when auth.user existed, blocking
API-key-only usage patterns. Now it's created whenever apiUrl is configured,
allowing more flexible authentication strategies.
Changes:
- Create aiCustomFetch based on apiUrl presence, not auth.user
- Supports future API-key-only usage patterns
- Users can still use createCustomFetch directly with API keys
Note: Context-based aiCustomFetch still defaults to JWT auth. For API-key
usage, import createCustomFetch directly with { apiKey: 'key' } option.
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit e060171), not the entire PR.
The recent changes modify the OpenSecretProvider component to support API key authentication alongside existing JWT authentication. The key architectural change is in the useEffect hook that creates the aiCustomFetch function - it now depends on apiUrl instead of auth.user. This allows the custom fetch function to be available even when no user is authenticated via JWT, enabling API key-based workflows.
The change integrates with the broader API key management feature by ensuring that OpenAI endpoints can be accessed using API keys without requiring traditional user sign-in. The createCustomFetch function has been enhanced internally to support both JWT tokens from localStorage and API keys passed as options. Three new API key management functions (createApiKey, listApiKeys, deleteApiKey) have been exposed through both the authenticated and unauthenticated contexts, maintaining backward compatibility while expanding authentication options.
Confidence score: 4/5
- This change is relatively safe to merge with low risk of breaking existing functionality
- Score reflects a well-architected change that maintains backward compatibility while adding new functionality
- The main.tsx file requires attention to ensure the dual authentication logic works correctly
1 file reviewed, no comments
Added full API key support to the React context, allowing aiCustomFetch to use
API keys for OpenAI endpoints instead of requiring JWT authentication.
Changes:
- Added apiKey state and setApiKey method to OpenSecretContextType
- Pass apiKey to createCustomFetch when creating aiCustomFetch
- aiCustomFetch now supports both JWT and API key authentication
Usage:
const os = useOpenSecret();
os.setApiKey('your-api-key-uuid');
// Now os.aiCustomFetch will use the API key for OpenAI endpoints
This enables API-key-only usage patterns where users can interact with
OpenAI endpoints without needing to authenticate with email/password.
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit 4c7d656), not the entire PR. The most recent changes focus on adding comprehensive API key management functionality to the OpenSecret React context provider in src/lib/main.tsx.
The changes introduce API key state management at the React context level, adding an apiKey state variable and setApiKey method to the OpenSecretContext. The implementation modifies the aiCustomFetch creation to support API key authentication as an alternative to JWT tokens specifically for OpenAI endpoints (/v1/*). Three new API key management functions are added to the context: createApiKey, listApiKeys, and deleteApiKey, all wrapped with proper error handling and user feedback mechanisms.
The implementation maintains full backward compatibility with existing JWT authentication while providing a new authentication path for AI services. The API key functionality is properly scoped - only OpenAI endpoints accept API key authentication, while all other endpoints continue using JWT tokens. The changes follow React best practices with proper state management, effect dependencies, and comprehensive JSDoc documentation.
The integration fits well with the existing codebase architecture by leveraging the established encrypted API call patterns and extending the context provider pattern already used throughout the SDK. The API key state is managed alongside other authentication state, and the custom fetch function intelligently chooses between API key and JWT authentication based on availability and endpoint compatibility.
Confidence score: 4/5
- This change is safe to merge with minimal risk as it maintains backward compatibility and follows established patterns
- Score reflects solid implementation following React patterns, but slightly lowered due to the complexity of dual authentication logic
- Pay close attention to the
useEffectdependency array and API key precedence logic in the custom fetch creation
1 file reviewed, no comments
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/main.tsx (3)
37-42: Setter contract is fine; consider light runtime validation (UUID) and trimmingSmall guardrails on
setApiKeyhelp catch copy/paste errors and leading/trailing spaces without burdening callers.If you’d like, I can wire a validated wrapper (see diff attached under Lines 753-755). Want me to send a follow-up PR for that?
651-653: Default context values OK; optional dev-ergonomics improvementA no-op
setApiKeyis fine. Optionally, log a dev warning when invoked outside the provider to catch misconfiguration earlier.- setApiKey: () => {}, + setApiKey: () => { + if (process?.env?.NODE_ENV !== "production") { + console.warn("OpenSecretContext.setApiKey called outside of OpenSecretProvider"); + } + },
777-786: Optional: Guard aiCustomFetch to only send API keys on/v1endpointsSince
createCustomFetchwill attach your API key to every request, it’s easy to accidentally leak it (or trigger confusing 401s) when calling non-OpenAI endpoints. Consider wrapping the fetch returned bycreateCustomFetchwith a lightweight URL check so that ifapiKeyis set but the request path isn’t under/v1/, you emit a warning.• File:
src/lib/main.tsx
UseEffect around line 779 where you callsetAiCustomFetch- setAiCustomFetch(() => createCustomFetch(apiKey ? { apiKey } : undefined)); + setAiCustomFetch(() => { + const inner = createCustomFetch(apiKey ? { apiKey } : undefined); + return async (requestUrl: RequestInfo, init?: RequestInit) => { + if (apiKey && typeof requestUrl === "string") { + const url = new URL(requestUrl, apiUrl); + if (!url.pathname.startsWith("/v1/")) { + console.warn( + "aiCustomFetch: API key is set but request is not targeting /v1/*; " + + "this may fail or leak your key. API keys are only accepted on OpenAI-compatible paths." + ); + } + } + return inner(requestUrl, init); + }; + });This change is purely additive and optional, but it helps prevent accidental misuse of your key when integrating with other back-end routes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/main.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/main.tsx (1)
src/lib/ai.ts (1)
createCustomFetch(9-134)
🔇 Additional comments (7)
src/lib/main.tsx (7)
31-36: Good addition: explicit API-key field in contextAdding
apiKey?: stringwith OpenAI-only scope documented is clear and backwards compatible. No issues.
610-620: createApiKey surface looks correct; ensure “key shown once” is enforced in UI flowsAPI returns key once—SDK state keeps it ephemeral, which is good. Ensure any UI that calls this immediately prompts copy/save and never persists the key.
Would you like a small helper that returns
{ key, onCopyRequired: true }to nudge consumers toward correct UX?
621-631: listApiKeys shape is reasonable; confirm sort order is guaranteed server-sideDocs say “sorted by created_at desc.” Please verify the backend enforces this so clients don’t need to sort.
I can add a client-side stable sort as a defensive fallback if needed.
632-642: Delete by name: confirm uniqueness constraint at the API layerSince deletion uses
name, double-check the API enforces unique names per user and returns a clear 404/409 when applicable.If uniqueness isn’t guaranteed, recommend switching this surface to an immutable ID.
706-710: Default exports for API key CRUD in context are consistentExposing
createApiKey,listApiKeys, anddeleteApiKeyon the default value mirrors the provider. Looks good.
1017-1019: Provider value wires the new fields correctly
apiKeyand the validatedsetApiKeyare exposed. After applying the setter refactor (Lines 753-755), this will reference the wrapper, not the rawuseStatesetter.
1071-1075: API key CRUD exposed on provider value—LGTMSurface area matches the context type and default.
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit 5561494), not the entire PR.
This latest change specifically addresses the Rust SDK's thread safety concerns by migrating the OpenSecretClient from RefCell to Arc<RwLock> for managing the server_public_key field. The change introduces thread-safe concurrent access patterns that will be essential for streaming operations and potential multi-threaded usage scenarios.
The migration maintains the existing API surface while internally restructuring how the client handles shared mutable state. The server_public_key field, which stores cryptographic key material used for secure communication with the OpenSecret platform, now uses Rust's read-write lock mechanism wrapped in an atomic reference counter. This allows multiple readers or a single writer, which is appropriate for a field that's set once during initialization and read frequently during API operations.
The implementation properly handles lock acquisition errors by converting them into appropriate Error::Session variants with descriptive messages. The dual authentication logic for OpenAI endpoints remains unchanged, continuing to prefer API keys over JWT tokens when both are available. The streaming chat completion functionality also maintains its existing behavior while now being thread-safe.
Confidence score: 4/5
- This change is generally safe to merge with proper understanding of the concurrency implications
- Score reflects the complexity of thread safety changes requiring careful review of lock usage patterns
- Pay close attention to the lock acquisition and error handling patterns in the client implementation
1 file reviewed, no comments
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rust/src/client.rs (1)
131-138: Correct implementation: Key exchange uses JWT only, not API keysThe implementation correctly ensures that key exchange endpoints only use JWT tokens and never API keys, which aligns with the PR objectives stating "Only OpenAI endpoints accept API key authentication."
🧹 Nitpick comments (1)
rust/src/client.rs (1)
383-478: Consider extracting common encryption logicThe
encrypted_openai_callmethod duplicates significant encryption/decryption logic fromencrypted_api_call. While the authentication header logic differs, the rest of the implementation is nearly identical.Consider extracting the common encryption/decryption logic into a shared helper method to reduce code duplication:
+ /// Common helper for encrypted API calls with configurable auth strategy + async fn encrypted_call_internal<T: Serialize, U: DeserializeOwned>( + &self, + endpoint: &str, + method: &str, + data: Option<T>, + use_api_key: bool, + ) -> Result<U> { + // Ensure we have a session + let session = self.session_manager.get_session()?.ok_or_else(|| { + Error::Session( + "No active session. Call perform_attestation_handshake first".to_string(), + ) + })?; + + let url = format!("{}{}", self.base_url, endpoint); + + // Encrypt the request data if provided + let encrypted_body = if let Some(data) = data { + let json = serde_json::to_string(&data)?; + let encrypted = crypto::encrypt_data(&session.session_key, json.as_bytes())?; + Some(EncryptedRequest { + encrypted: BASE64.encode(&encrypted), + }) + } else { + None + }; + + // Build headers + let mut headers = HeaderMap::new(); + headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); + headers.insert( + "x-session-id", + HeaderValue::from_str(&session.session_id.to_string()) + .map_err(|e| Error::Session(format!("Invalid session ID: {}", e)))?, + ); + + // Add authorization header based on strategy + if use_api_key { + // For OpenAI endpoints: Prefer API key over JWT token + if let Some(api_key) = self.session_manager.get_api_key()? { + headers.insert( + AUTHORIZATION, + HeaderValue::from_str(&format!("Bearer {}", api_key)) + .map_err(|e| Error::Authentication(format!("Invalid API key format: {}", e)))?, + ); + } else if let Some(token) = self.session_manager.get_access_token()? { + headers.insert( + AUTHORIZATION, + HeaderValue::from_str(&format!("Bearer {}", token)) + .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, + ); + } + } else { + // For non-OpenAI endpoints: Only use JWT + if let Some(token) = self.session_manager.get_access_token()? { + headers.insert( + AUTHORIZATION, + HeaderValue::from_str(&format!("Bearer {}", token)) + .map_err(|e| Error::Authentication(format!("Invalid token format: {}", e)))?, + ); + } + } + + // Make the request + let request_builder = match method { + "GET" => self.client.get(&url), + "POST" => self.client.post(&url), + "PUT" => self.client.put(&url), + "DELETE" => self.client.delete(&url), + _ => { + return Err(Error::Api { + status: 0, + message: format!("Unsupported HTTP method: {}", method), + }) + } + }; + + let request = request_builder.headers(headers); + let response = if let Some(body) = encrypted_body { + request.json(&body).send().await? + } else { + request.send().await? + }; + + let status = response.status(); + let body = response.bytes().await?; + + if !status.is_success() { + let error_msg = String::from_utf8_lossy(&body).to_string(); + return Err(Error::Api { + status: status.as_u16(), + message: error_msg, + }); + } + + // Decrypt the response + let encrypted_response: EncryptedResponse<U> = serde_json::from_slice(&body)?; + let encrypted_data = BASE64 + .decode(encrypted_response.encrypted.as_bytes()) + .map_err(|e| Error::Encryption(format!("Failed to decode response: {}", e)))?; + + let decrypted = crypto::decrypt_data(&session.session_key, &encrypted_data)?; + let result: U = serde_json::from_slice(&decrypted)?; + + Ok(result) + } + async fn encrypted_api_call<T: Serialize, U: DeserializeOwned>( &self, endpoint: &str, method: &str, data: Option<T>, ) -> Result<U> { - // Original implementation... + self.encrypted_call_internal(endpoint, method, data, false).await } async fn encrypted_openai_call<T: Serialize, U: DeserializeOwned>( &self, endpoint: &str, method: &str, data: Option<T>, ) -> Result<U> { - // Original implementation... + self.encrypted_call_internal(endpoint, method, data, true).await }This refactoring would eliminate approximately 70 lines of duplicated code while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
rust/src/client.rs(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rust/src/client.rs (2)
rust/src/session.rs (4)
new(13-19)new_with_api_key(21-27)set_api_key(29-36)clear_api_key(46-53)rust/src/crypto.rs (2)
encrypt_data(65-82)decrypt_data(84-98)
⏰ 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: Cloudflare Pages
🔇 Additional comments (8)
rust/src/client.rs (8)
16-16: Consider usingMutexinstead ofRwLockfor write-heavy operationsThe
server_public_keyfield has been changed fromRefCelltoArc<RwLock<Option<Vec<u8>>>>. Given that this field is primarily written once during attestation handshake and read multiple times,RwLockis appropriate. However, for better clarity in the codebase, consider documenting this read-heavy access pattern.Also applies to: 24-24, 37-37, 50-50
41-52: LGTM! Constructor properly initializes API keyThe new constructor correctly initializes the session manager with the provided API key and maintains consistency with the existing constructor pattern.
54-60: LGTM! API key management methods are well-implementedThe
set_api_keyandclear_api_keymethods properly delegate to the session manager with appropriate error handling.
328-335: Correct: Non-OpenAI endpoints use JWT authentication onlyThe standard
encrypted_api_callmethod correctly uses only JWT tokens for authentication, ensuring API keys are not sent to non-OpenAI endpoints.
383-478: Well-structured OpenAI-specific authentication pathThe new
encrypted_openai_callmethod properly implements the dual authentication strategy with API key precedence for OpenAI endpoints. The implementation correctly:
- Prioritizes API keys over JWT tokens when both are present
- Falls back to JWT when no API key is available
- Maintains the same encryption/decryption flow as the standard path
632-657: Comprehensive API key CRUD operationsThe API key management methods are well-implemented:
create_api_key: Properly creates new API keyslist_api_keys: Correctly sorts keys by creation date (newest first)delete_api_key: Properly URL-encodes the name parameterThe sorting in
list_api_keysprovides a good user experience by showing the most recent keys first.
942-944: Correct routing to OpenAI-specific call pathThe
get_modelsandcreate_chat_completionmethods correctly useencrypted_openai_callinstead of the standardencrypted_api_call, ensuring proper API key authentication for these OpenAI endpoints.Also applies to: 953-954
998-1012: Streaming endpoint correctly implements API key precedenceThe streaming chat completion method properly implements the same authentication precedence (API key first, then JWT) as the non-streaming OpenAI endpoints, maintaining consistency across all OpenAI-specific paths.
Addressed security concerns raised by CodeRabbit in PR review: - Added UUID validation for API keys (v1-v5 format with dashes) - Clear API key on sign-out to prevent post-logout usage - Clear API key on all sign-in methods (email, guest, OAuth) for user-scoped keys - Added validation wrapper to prevent invalid API keys from being set Security improvements: - Prevents lingering API keys in shared browser contexts - Ensures API keys are properly formatted UUIDs - Makes API keys user-scoped by clearing on auth state changes
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review (commit 5ee7596), not the entire PR.
The latest changes implement comprehensive API key authentication and clearing mechanisms in the TypeScript SDK's main context file (src/lib/main.tsx). The implementation adds UUID validation for API keys with a strict regex pattern that automatically trims input and clears invalid keys with warning messages. The API key is integrated into the aiCustomFetch function to enable OpenAI endpoint authentication when available. Most importantly, the system now automatically clears stored API keys on all authentication flows (sign-in, sign-up, OAuth callbacks) to ensure proper user session isolation and prevent API key leakage between users. The context exposes new CRUD operations for API key management (createApiKey, listApiKeys, deleteApiKey) while maintaining full backward compatibility with existing JWT authentication.
Confidence score: 3/5
- This PR requires careful review due to security-sensitive authentication changes and potential UUID validation edge cases
- Score reflects complex authentication logic changes that could affect user session security if not properly tested
- Pay close attention to the UUID validation implementation and authentication flow clearing logic
1 file reviewed, no comments
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/lib/main.tsx (3)
31-42: API key surface looks good; tiny doc nit to clarify scope and persistenceConsider clarifying that apiKey is in-memory only and applied only to OpenAI (/v1/*) calls via aiCustomFetch; other SDK calls still use JWT.
Apply:
/** * Optional API key for OpenAI endpoints. * When set, this will be used instead of JWT for /v1/* endpoints. + * Note: Held in memory only (not persisted) and only applied to OpenAI + * requests made via aiCustomFetch. All non-/v1 SDK requests continue to use JWT. */
756-772: Validator is solid; avoid noisy logs in productionKeep the warning dev-only to reduce production console noise. Optional.
- if (!uuidWithDashes.test(trimmed)) { - console.warn("setApiKey: provided key does not look like a UUID; clearing apiKey"); + if (!uuidWithDashes.test(trimmed)) { + if (process.env.NODE_ENV !== "production") { + console.warn("setApiKey: provided key does not look like a UUID; clearing apiKey"); + } setApiKeyState(undefined); return; }
794-802: Optional: derive aiCustomFetch with useMemo instead of state+effectRemoves a state/effect pair and avoids an extra render. Behavior remains identical.
- const [aiCustomFetch, setAiCustomFetch] = useState<OpenSecretContextType["aiCustomFetch"]>(); + // Derived once per (apiUrl, apiKey) change + const aiCustomFetch = React.useMemo(() => { + if (!apiUrl) return undefined; + return createCustomFetch(apiKey ? { apiKey } : undefined); + }, [apiUrl, apiKey]); - // Create aiCustomFetch when API is configured (supports JWT or API key internally) - useEffect(() => { - if (apiUrl) { - // Pass API key if available, otherwise falls back to JWT - setAiCustomFetch(() => createCustomFetch(apiKey ? { apiKey } : undefined)); - } else { - setAiCustomFetch(undefined); - } - }, [apiUrl, apiKey]); + // (effect removed)Also update imports and provider value:
- import React, { createContext, useState, useEffect } from "react"; + import React, { createContext, useState, useEffect, useMemo } from "react";- aiCustomFetch: aiCustomFetch || (async () => new Response()), + aiCustomFetch: aiCustomFetch || (async () => new Response()),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/main.tsx(17 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/main.tsx (1)
src/lib/ai.ts (1)
createCustomFetch(9-134)
⏰ 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: test
🔇 Additional comments (8)
src/lib/main.tsx (8)
651-653: Good defaults for new context fieldsDefaulting apiKey to undefined and providing a no-op setter in the context stub is correct.
706-709: Default context wiring for new API key methods looks correctExposing createApiKey, listApiKeys, and deleteApiKey from the default value matches the type surface.
753-753: Decoupled state setter resolves earlier security concernRenaming to setApiKeyState and wrapping via a validator prevents accidental direct mutation and aligns with the prior review feedback.
840-842: Thorough: apiKey is cleared on all sign-in/out and OAuth pathsPrevents post-authentication misuse and keeps keys user-scoped. Nice.
Also applies to: 860-862, 875-877, 893-895, 926-928, 952-954, 979-981, 1006-1008, 1024-1026
840-842: Confirm handling on convertGuestToUserAccountIf keys are strictly “user-scoped,” consider also clearing apiKey on convertGuestToUserAccount to avoid carrying a guest-era key across the upgrade, unless that continuity is intentional.
1052-1054: Provider value now includes apiKey and setApiKey — looks correctMatches OpenSecretContextType and enables consumers to read/update the key.
1106-1109: Provider value exposes API key CRUD — good wiringSurfacing create/list/delete aligns with the new API module and PR objectives.
610-642: API key CRUD wiring OK – verify server-side contract
- CustomFetchOptions declares an optional
apiKey?: string(src/lib/ai.ts lines 5–7), andcreateCustomFetchprioritizes API-key auth and skips token-refresh on 401 when an API key is used (src/lib/ai.ts lines 9–16).- The SDK exports
createApiKey,listApiKeys, anddeleteApiKey, and these are correctly wired intomain.tsxastypeof api.createApiKey, etc. (src/lib/api.ts lines 1218–1221; 1247–1250; 1284–1286).ApiKeyCreateResponseincludes{ name, created_at, key }, whileApiKeyListResponseisApiKey[](nokeyfield), solistApiKeysnever returns key material (src/lib/api.ts lines 1165–1174).Points needing manual confirmation:
- That the server enforces
nameas unique per user and uses it as the canonical delete selector.- That the
listApiKeysendpoint returns keys sorted descending bycreated_at, as documented.
…t SDKs
This commit implements API key functionality for both SDKs:
TypeScript SDK:
Rust SDK:
Both implementations:
Testing:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests