separate cli auth keys by host - #2808
Conversation
📝 WalkthroughWalkthroughAuthentication is centralized in a new exported module that stores API-specific tokens, validates and migrates legacy credentials, and supports removal. CLI commands, token context, HTTP URL resolution, login flows, and create-instant-app now use the shared implementation. ChangesCLI authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant getBaseUrl
participant authModule
participant API
participant configFile
CLI->>getBaseUrl: resolve API base URL
getBaseUrl-->>CLI: validated API URL
CLI->>authModule: read or save API token
authModule->>configFile: read or write credentials
authModule->>API: validate token at /dash/me
API-->>authModule: validation result
authModule-->>CLI: token or persistence result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View Vercel preview at instant-www-js-drewh-cli-auth-map-jsv.vercel.app. |
02f7676 to
6d5129a
Compare
![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
🧹 Nitpick comments (3)
client/packages/cli/src/auth.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate production URL literal.
productionApiUrlduplicates the'https://api.instantdb.com'fallback literal defined inapiUrl.ts(Line 39). Consider exporting one and importing it here to avoid future drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/packages/cli/src/auth.ts` at line 6, Remove the duplicate production URL literal from auth.ts by exporting and reusing the existing production URL constant from apiUrl.ts. Update the productionApiUrl reference to import that shared symbol, preserving the current URL value and fallback behavior.client/packages/cli/src/util/apiUrl.ts (2)
22-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
INSTANT_CLI_API_URIisn't validated as an HTTP(S) URL.The
instant.config.tsapiURIpath is validated with theHttpUrlschema, but theINSTANT_CLI_API_URIenv var is returned as-is at Line 23. A malformed value here bypasses validation entirely and only fails later, deep in the HTTP client, with a less helpful error.♻️ Suggested fix
if (Option.isSome(setEnv)) { - return setEnv.value; + yield* Schema.decodeUnknown(HttpUrl)(setEnv.value).pipe( + Effect.mapError(() => + BadArgsError.make({ + message: + 'Invalid INSTANT_CLI_API_URI. Expected a valid HTTP(S) URL.', + }), + ), + ); + return setEnv.value; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/packages/cli/src/util/apiUrl.ts` around lines 22 - 24, Validate the value returned by the setEnv branch in the API URL resolution flow using the same HttpUrl schema applied to instant.config.ts apiURI, rather than returning setEnv.value directly. Preserve the existing behavior for valid HTTP(S) URLs and surface schema validation errors before the HTTP client is invoked.
26-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConfig-file read failures aren't mapped to a domain error.
Effect.tryPromise(readInstantConfigFile)has noEffect.mapError, so a brokeninstant.config.ts(e.g. a throw during load) surfaces as a rawUnknownExceptioninstead of the friendlyBadArgsErrorused just below for invalidapiURIvalues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/packages/cli/src/util/apiUrl.ts` at line 26, Update the instantConfig loading flow around readInstantConfigFile to map Effect.tryPromise failures to the existing BadArgsError domain error, preserving the friendly error behavior already used for invalid apiURI values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/packages/cli/src/auth.ts`:
- Around line 19-31: The legacy token returned by parseAuthConfig must be
trimmed before use. Update the non-JSON fallback to return a token derived from
trimmed contents, and ensure readConfigAuthToken reuses that normalized value
during validation fallback so surrounding whitespace cannot corrupt the Bearer
token.
- Around line 62-69: Update writeAuthConfigFile to create the auth directory and
token file with restrictive permissions: use a private directory mode and a file
mode that prevents group/other access when calling mkdir and writeFile. Preserve
the existing paths, serialization, and UTF-8 encoding.
---
Nitpick comments:
In `@client/packages/cli/src/auth.ts`:
- Line 6: Remove the duplicate production URL literal from auth.ts by exporting
and reusing the existing production URL constant from apiUrl.ts. Update the
productionApiUrl reference to import that shared symbol, preserving the current
URL value and fallback behavior.
In `@client/packages/cli/src/util/apiUrl.ts`:
- Around line 22-24: Validate the value returned by the setEnv branch in the API
URL resolution flow using the same HttpUrl schema applied to instant.config.ts
apiURI, rather than returning setEnv.value directly. Preserve the existing
behavior for valid HTTP(S) URLs and surface schema validation errors before the
HTTP client is invoked.
- Line 26: Update the instantConfig loading flow around readInstantConfigFile to
map Effect.tryPromise failures to the existing BadArgsError domain error,
preserving the friendly error behavior already used for invalid apiURI values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 05d32d46-21c8-49bb-a2f2-189767023443
📒 Files selected for processing (9)
client/packages/cli/package.jsonclient/packages/cli/src/auth.tsclient/packages/cli/src/commands/logout.tsclient/packages/cli/src/context/authToken.tsclient/packages/cli/src/lib/http.tsclient/packages/cli/src/lib/login.tsclient/packages/cli/src/util/apiUrl.tsclient/packages/cli/src/util/getAuthPaths.tsclient/packages/create-instant-app/src/login.ts
| async function writeAuthConfigFile(paths: AuthPaths, tokens: AuthTokens) { | ||
| await mkdir(paths.appConfigDirPath, { recursive: true }); | ||
| await writeFile( | ||
| paths.authConfigFilePath, | ||
| serializeAuthTokens(tokens), | ||
| 'utf8', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Auth token file/dir written without restrictive permissions.
writeAuthConfigFile persists bearer tokens via mkdir/writeFile with default modes (typically 644/755 after umask), making the credentials file world-readable on shared/multi-user systems.
🔒 Suggested fix
async function writeAuthConfigFile(paths: AuthPaths, tokens: AuthTokens) {
- await mkdir(paths.appConfigDirPath, { recursive: true });
+ await mkdir(paths.appConfigDirPath, { recursive: true, mode: 0o700 });
await writeFile(
paths.authConfigFilePath,
serializeAuthTokens(tokens),
- 'utf8',
+ { encoding: 'utf8', mode: 0o600 },
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function writeAuthConfigFile(paths: AuthPaths, tokens: AuthTokens) { | |
| await mkdir(paths.appConfigDirPath, { recursive: true }); | |
| await writeFile( | |
| paths.authConfigFilePath, | |
| serializeAuthTokens(tokens), | |
| 'utf8', | |
| ); | |
| } | |
| async function writeAuthConfigFile(paths: AuthPaths, tokens: AuthTokens) { | |
| await mkdir(paths.appConfigDirPath, { recursive: true, mode: 0o700 }); | |
| await writeFile( | |
| paths.authConfigFilePath, | |
| serializeAuthTokens(tokens), | |
| { encoding: 'utf8', mode: 0o600 }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/packages/cli/src/auth.ts` around lines 62 - 69, Update
writeAuthConfigFile to create the auth directory and token file with restrictive
permissions: use a private directory mode and a file mode that prevents
group/other access when calling mkdir and writeFile. Preserve the existing
paths, serialization, and UTF-8 encoding.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/packages/cli/src/auth.ts (1)
42-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim mapped API keys before storing them.
Unlike legacy tokens, JSON-mapped token values are persisted verbatim. A token copied with trailing whitespace or a newline can therefore be returned as an invalid Bearer token. Normalize the value here, and add a regression test for mapped credentials containing surrounding whitespace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/packages/cli/src/auth.ts` around lines 42 - 45, Update the mapped-token loop in the parsed credentials flow to trim each token value before assigning it to tokens using normalizeApiUrl(apiUrl). Preserve the existing API-key normalization, and add a regression test covering mapped credentials whose token contains surrounding whitespace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@client/packages/cli/src/auth.ts`:
- Around line 42-45: Update the mapped-token loop in the parsed credentials flow
to trim each token value before assigning it to tokens using
normalizeApiUrl(apiUrl). Preserve the existing API-key normalization, and add a
regression test covering mapped credentials whose token contains surrounding
whitespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ed8b5ec-4d14-4dc2-86e6-812651fda676
📒 Files selected for processing (1)
client/packages/cli/src/auth.ts
|
Talked about this with the LLM and it suggested instead of having one file for all tokens we would be better of having a separate file per backend. The benefit here is it would be fully backward compatible (if someone upgrades instant-cli and now the file type changes to JSON, if they go to a project using an older instant-cli that will break since our auth file changed). Put up a PR for that here #2813 |
2 participants
Transitions the api key storage for the instant-cli to use JSON with a map between api hosts and the key itself.
People who are upgrading will have their previous key tested against their current apiURI, then production, on first initial read.
Also updated create-instant-app to work with the new format. Uses a new export in package.json from cli for auth specific code that cia can borrow
Logging out now only removes the corresponding value from the map, rather than deleting the entire file.