Conversation
📝 WalkthroughWalkthroughAdds optional AWS S3 session token support end-to-end: UI input, secure-storage set/get/delete, types, persistence, propagation to backend services, and inclusion of sessionToken when constructing S3 credentials and DuckLake secret payloads. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as ConnectionForm / Explorer UI
participant Storage as SecureStorage
participant Backend as Backend Service (DuckLake / cloudExplorer)
participant AWS as AWS S3 Client
User->>UI: Enter accessKeyId, secretAccessKey, optional sessionToken
UI->>Storage: setCloudAwsSessionToken(token, connId)
Storage-->>UI: ack
UI->>Backend: submit connection (providerConfig without token)
Backend->>Storage: getCloudAwsSessionToken(connId)
Storage-->>Backend: sessionToken?
Backend->>Backend: build credentials {accessKeyId, secretAccessKey, sessionToken?}
Backend->>AWS: initialize S3Client(credentials)
AWS-->>Backend: ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 (3)
src/main/services/duckLake/adapters/base.adapter.ts (1)
349-363:⚠️ Potential issue | 🟠 MajorSQL injection risk:
sessionTokenis interpolated directly into the query string.This follows the same pattern as
accessKeyId,secretAccessKey, etc., but any of these values containing a single quote (') would break the SQL. ThesessionTokenvalue originates from secure storage, which reduces risk, but consider escaping single quotes (as done forescapedTokenin the GCS path at line 423) for defense-in-depth.🛡️ Suggested defensive escaping
if (sessionToken) { + const escapedSessionToken = sessionToken.replace(/'/g, "''"); secretQuery += `, - SESSION_TOKEN '${sessionToken}'`; + SESSION_TOKEN '${escapedSessionToken}'`; }Note: The same escaping concern applies to the existing
accessKeyId,secretAccessKey,region, andendpointvalues, but that's a pre-existing issue outside the scope of this PR.src/main/services/cloudExplorer.service.ts (1)
29-53:⚠️ Potential issue | 🔴 Critical
sessionTokenis not passed during S3 connection validation inDuckLakeService.In
src/main/services/duckLake.service.tsat lines 731–735, thevalidateStorageConnectionmethod callsCloudExplorerService.testConnection('aws', ...)without includingsessionToken:success = await CloudExplorerService.testConnection('aws', { region: fullConfig.s3.region, accessKeyId: fullConfig.s3.accessKeyId, secretAccessKey: fullConfig.s3.secretAccessKey, });This will fail for users who authenticate with temporary STS credentials (where the session token is mandatory). The
sessionTokenshould be included:🐛 Proposed fix in duckLake.service.ts (lines 731-735)
success = await CloudExplorerService.testConnection('aws', { region: fullConfig.s3.region, accessKeyId: fullConfig.s3.accessKeyId, secretAccessKey: fullConfig.s3.secretAccessKey, + sessionToken: fullConfig.s3.sessionToken, });src/main/services/duckLake/instanceStore.service.ts (1)
434-447:⚠️ Potential issue | 🔴 CriticalSecurity:
sessionTokenis not stripped from the persisted storage config, leaking it to plaintext JSON on disk.
secretAccessKeyis explicitly deleted at Line 438 before writing to the JSON file, butsessionTokenis not. This means the session token will be written in cleartext toinstances.json, defeating the purpose of storing it in secure storage (Lines 254-260).🔒 Proposed fix: strip sessionToken alongside secretAccessKey
if (storagePersisted?.s3) { delete (storagePersisted.s3 as any).secretAccessKey; + delete (storagePersisted.s3 as any).sessionToken; }
🤖 Fix all issues with AI agents
In `@src/main/helpers/cloudAuth.helper.ts`:
- Around line 110-122: The session token is directly interpolated into the SQL
in the sessionTokenClause, creating an SQL injection risk; update the code that
builds sessionTokenClause (used in the CREATE OR REPLACE SECRET s3_secret block)
to escape single quotes in awsConfig.sessionToken (e.g., replace each ' with
''), or use an existing SQL-escaping helper if available, so the value is safely
quoted before concatenation into the returned SQL string.
In `@src/renderer/components/dataLake/DataLakeConnectionSelector.tsx`:
- Around line 275-279: providerConfig currently includes the temporary
sessionToken and is passed to createConnection.mutateAsync, which causes the
token to be persisted in plaintext; before calling createConnection.mutateAsync
(or before any persistence), remove or set sessionToken to undefined on
providerConfig (or clone providerConfig and delete the sessionToken property) so
only the non-sensitive fields are sent/stored; locate the providerConfig
assignment in DataLakeConnectionSelector.tsx and ensure the call site for
createConnection.mutateAsync uses the sanitized config without sessionToken.
🧹 Nitpick comments (2)
src/renderer/components/cloudExplorer/ExplorerBucketContent.tsx (1)
101-112: Consider parallelizing the two independent credential fetches.
getCloudAwsSecretandgetCloudAwsSessionTokenare independent calls that could run concurrently viaPromise.all, similar to howretrieveCredentialsdoes it ininstanceStore.service.ts(Lines 324-327).♻️ Suggested parallel fetch
if (connection.provider === 'aws') { - const secret = await getCloudAwsSecret(connection.id); - const sessionToken = await getCloudAwsSessionToken(connection.id); + const [secret, sessionToken] = await Promise.all([ + getCloudAwsSecret(connection.id), + getCloudAwsSessionToken(connection.id), + ]); ( config as { secretAccessKey?: string;src/renderer/components/cloudExplorer/ConnectionForm.tsx (1)
144-151: Consider parallelizing the two independent credential fetches.Same pattern as flagged in
ExplorerBucketContent.tsx— the secret and session token fetches are independent.♻️ Suggested parallel fetch
} else if (provider === 'aws') { - const stored = await getCloudAwsSecret(id); - const storedSessionToken = await getCloudAwsSessionToken(id); - setFormData((prev) => ({ - ...prev, - secretAccessKey: stored || '', - sessionToken: storedSessionToken || '', - })); + const [stored, storedSessionToken] = await Promise.all([ + getCloudAwsSecret(id), + getCloudAwsSessionToken(id), + ]); + setFormData((prev) => ({ + ...prev, + secretAccessKey: stored || '', + sessionToken: storedSessionToken || '', + }));
- Escape single quotes in AWS session tokens to prevent SQL injection - Remove sessionToken from providerConfig before persistence Session tokens are still stored securely via setCloudAwsSessionToken but are no longer persisted in connection configs or vulnerable to SQL injection in secret queries.
# Conflicts: # src/main/helpers/cloudAuth.helper.ts # src/renderer/components/cloudExplorer/ConnectionForm.tsx # src/renderer/components/cloudExplorer/ExplorerBucketContent.tsx # src/renderer/components/cloudExplorer/ExplorerBuckets.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/renderer/components/cloudExplorer/ConnectionForm.tsx`:
- Around line 383-394: The form currently writes setCloudAwsSessionToken only
when formData.sessionToken is non-empty and never removes a previously stored
token; update the useSecureStorage destructuring in ConnectionForm to include
deleteCloudAwsSessionToken, then change the AWS branch where
setCloudAwsSessionToken is called (referencing formData.sessionToken,
setCloudAwsSessionToken, deleteCloudAwsSessionToken, connId) so that if
formData.sessionToken.trim() is non-empty you call
setCloudAwsSessionToken(connId) else call deleteCloudAwsSessionToken(connId);
also remove sessionToken from config as already done (rawConfig/config handling
stays the same).
🧹 Nitpick comments (1)
src/renderer/components/cloudExplorer/ConnectionForm.tsx (1)
545-554: Session Token field lacks a show/hide toggle, unlike Secret Access Key.The Secret Access Key field has a visibility toggle (
showPasswordstate + eye icon), but the Session Token field is hardcoded totype="password"with no way to reveal the value. Minor UX inconsistency — consider reusing the same toggle pattern for parity, especially since session tokens are long and users may want to verify what they pasted.
3 participants
Summary by CodeRabbit