Repository navigation
fix(startup): honor documented env loading - #369
Conversation
Align the app bootstrap paths with the documented CLI env lookup. The CLI wrapper already loads DATA_DIR/.env, ~/.omniroute/.env, or ./.env, but run-next, run-standalone, and Electron were bypassing that behavior. On machines with encrypted credentials, that could generate a fresh STORAGE_ENCRYPTION_KEY in server.env and make existing tokens unreadable. This change: - uses the same preferred .env lookup in bootstrapEnv and Electron - keeps Electron secrets rooted in DATA_DIR and passes DATA_DIR to the child - refuses to mint a new encryption key over an existing encrypted database - adds a focused regression test for env precedence and key safety
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical inconsistency in how environment variables, particularly secrets, were loaded across different application startup paths (e.g., Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)
Files Reviewed (3 files)
SummaryThis PR introduces important improvements to the DATA_DIR resolution and encryption key handling:
The main concerns are around consistency between the Electron and Node.js implementations, and some complex logic in env resolution that could benefit from additional testing. Recommendation: Address the inconsistencies between the two implementations before merging. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a solid improvement that aligns environment variable loading across different startup paths, fixing a critical bug where an incorrect encryption key could be generated, leading to data decryption failures. The changes are well-structured, and the addition of regression tests is excellent. My main feedback is to improve error handling in one of the new helper functions to make it more robust against edge cases.
Propagate database inspection failures instead of treating them as missing encrypted credentials. This keeps startup from generating a fresh encryption key when an existing database cannot be inspected and adds a regression test for that path.
Treat empty or whitespace-only dataDirOverride values as unset so bootstrapEnv keeps using the normal DATA_DIR and .env lookup path. Adds a focused regression test for the whitespace override case.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical issue with inconsistent environment variable loading across different startup paths, which could lead to problems with data decryption. The changes align the behavior with the documented lookup order and add an important safety check to prevent overwriting encryption keys for existing encrypted databases. The addition of focused regression tests is also a great improvement. My review includes a suggestion to address code duplication between the Electron main process and the bootstrap script to improve long-term maintainability, and also points out a minor bug in one of the new helper functions.
| function resolveDataDir(overridePath, env = process.env) { | ||
| if (overridePath && overridePath.trim()) return path.resolve(overridePath); | ||
|
|
||
| const configured = env.DATA_DIR?.trim(); | ||
| if (configured) return path.resolve(configured); | ||
|
|
||
| if (process.platform === "win32") { | ||
| const appData = env.APPDATA || path.join(require("os").homedir(), "AppData", "Roaming"); | ||
| return path.join(appData, "omniroute"); | ||
| } | ||
|
|
||
| const xdg = env.XDG_CONFIG_HOME?.trim(); | ||
| if (xdg) return path.join(path.resolve(xdg), "omniroute"); | ||
|
|
||
| return path.join(require("os").homedir(), ".omniroute"); | ||
| } | ||
|
|
||
| function getPreferredEnvFilePath(env = process.env) { | ||
| const candidates = []; | ||
|
|
||
| if (env.DATA_DIR?.trim()) { | ||
| candidates.push(path.join(path.resolve(env.DATA_DIR.trim()), ".env")); | ||
| } | ||
|
|
||
| candidates.push(path.join(resolveDataDir(), ".env")); | ||
| candidates.push(path.join(process.cwd(), ".env")); | ||
|
|
||
| return candidates.find((filePath) => fs.existsSync(filePath)) || null; | ||
| } | ||
|
|
||
| function hasEncryptedCredentials(dbPath) { | ||
| if (!fs.existsSync(dbPath)) return false; | ||
|
|
||
| try { | ||
| const Database = require("better-sqlite3"); | ||
| const db = new Database(dbPath, { readonly: true, fileMustExist: true }); | ||
| try { | ||
| const row = db | ||
| .prepare( | ||
| `SELECT 1 | ||
| FROM provider_connections | ||
| WHERE access_token LIKE 'enc:v1:%' | ||
| OR refresh_token LIKE 'enc:v1:%' | ||
| OR api_key LIKE 'enc:v1:%' | ||
| OR id_token LIKE 'enc:v1:%' | ||
| LIMIT 1` | ||
| ) | ||
| .get(); | ||
| return !!row; | ||
| } finally { | ||
| db.close(); | ||
| } | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error(`Unable to inspect existing database at ${dbPath}: ${message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
These helper functions (resolveDataDir, getPreferredEnvFilePath, hasEncryptedCredentials) are duplicated from scripts/bootstrap-env.mjs. While I understand this is due to the CJS/ESM module differences between Electron's main process and the scripts, this duplication creates a maintenance burden. If the logic changes in one place, it must be manually updated in the other.
To avoid this, consider extracting these shared functions into a separate CommonJS file (e.g., scripts/env-helpers.cjs). This new file could be require'd here and also imported into the ESM bootstrap-env.mjs module. This would centralize the logic and improve maintainability.
There was a problem hiding this comment.
Keeping this PR scoped to the startup/key-safety fix. I agree the duplication is a maintenance cost, but extracting a shared helper across the Electron CJS path and the bootstrap ESM path would broaden the runtime/module-loading surface for this PR. I would prefer to handle that as a separate follow-up refactor once the startup regression is merged.
Keep getPreferredEnvFilePath consistent with its env parameter by passing that env through resolveDataDir in both bootstrap and Electron. This avoids silently falling back to process.env when a custom env map is supplied.
|
Damn, those code reviews are legit |
|
I've found a potential issue, please do not merge yet |
|
Those issues are not related (I was facing 3 separate issues) Opening PRs soon for them (Already opened) |
bug fixes (PRs #373, #371, #372, #369 by @kfiramar): - fix(db): provider_connections.group column migration for existing DBs - fix(i18n): replace missing deleteConnection key with delete in tooltip - fix(auth): clear stale error metadata on genuine provider recovery - fix(startup): unify env loading across npm/electron startup paths code quality improvements (per kilo-code-bot review): - docs: document result.success vs response.ok patterns in auth.ts - refactor: normalize overridePath?.trim() in electron/main.js - docs: explain preferredEnv merge order intent
…v-key-loading Thanks @kfiramar! 🎉 Critical security fix — different startup paths were generating different `STORAGE_ENCRYPTION_KEY` values over the same SQLite database, causing `Unsupported state or unable to authenticate data` for all stored tokens. Improvements added on top: - Normalized `overridePath?.trim()` in `electron/main.js` to match `bootstrap-env.mjs` (addresses kilo-code-bot warning cheaper-inference#1) - Added explanatory comment documenting the `preferredEnv` merge order intent in Electron startup (addresses kilo-code-bot warning cheaper-inference#3) 4 commits + 113-line test file. The fail-closed behaviour (refusing to mint a new key when encrypted rows exist) is an excellent safeguard. Merged!
bug fixes (PRs cheaper-inference#373, cheaper-inference#371, cheaper-inference#372, cheaper-inference#369 by @kfiramar): - fix(db): provider_connections.group column migration for existing DBs - fix(i18n): replace missing deleteConnection key with delete in tooltip - fix(auth): clear stale error metadata on genuine provider recovery - fix(startup): unify env loading across npm/electron startup paths code quality improvements (per kilo-code-bot review): - docs: document result.success vs response.ok patterns in auth.ts - refactor: normalize overridePath?.trim() in electron/main.js - docs: explain preferredEnv merge order intent
…v-key-loading Thanks @kfiramar! 🎉 Critical security fix — different startup paths were generating different `STORAGE_ENCRYPTION_KEY` values over the same SQLite database, causing `Unsupported state or unable to authenticate data` for all stored tokens. Improvements added on top: - Normalized `overridePath?.trim()` in `electron/main.js` to match `bootstrap-env.mjs` (addresses kilo-code-bot warning cheaper-inference#1) - Added explanatory comment documenting the `preferredEnv` merge order intent in Electron startup (addresses kilo-code-bot warning cheaper-inference#3) 4 commits + 113-line test file. The fail-closed behaviour (refusing to mint a new key when encrypted rows exist) is an excellent safeguard. Merged!
bug fixes (PRs cheaper-inference#373, cheaper-inference#371, cheaper-inference#372, cheaper-inference#369 by @kfiramar): - fix(db): provider_connections.group column migration for existing DBs - fix(i18n): replace missing deleteConnection key with delete in tooltip - fix(auth): clear stale error metadata on genuine provider recovery - fix(startup): unify env loading across npm/electron startup paths code quality improvements (per kilo-code-bot review): - docs: document result.success vs response.ok patterns in auth.ts - refactor: normalize overridePath?.trim() in electron/main.js - docs: explain preferredEnv merge order intent
…v-key-loading Thanks @kfiramar! 🎉 Critical security fix — different startup paths were generating different `STORAGE_ENCRYPTION_KEY` values over the same SQLite database, causing `Unsupported state or unable to authenticate data` for all stored tokens. Improvements added on top: - Normalized `overridePath?.trim()` in `electron/main.js` to match `bootstrap-env.mjs` (addresses kilo-code-bot warning cheaper-inference#1) - Added explanatory comment documenting the `preferredEnv` merge order intent in Electron startup (addresses kilo-code-bot warning cheaper-inference#3) 4 commits + 113-line test file. The fail-closed behaviour (refusing to mint a new key when encrypted rows exist) is an excellent safeguard. Merged!
bug fixes (PRs cheaper-inference#373, cheaper-inference#371, cheaper-inference#372, cheaper-inference#369 by @kfiramar): - fix(db): provider_connections.group column migration for existing DBs - fix(i18n): replace missing deleteConnection key with delete in tooltip - fix(auth): clear stale error metadata on genuine provider recovery - fix(startup): unify env loading across npm/electron startup paths code quality improvements (per kilo-code-bot review): - docs: document result.success vs response.ok patterns in auth.ts - refactor: normalize overridePath?.trim() in electron/main.js - docs: explain preferredEnv merge order intent
Summary
run-next,run-standalone, and Electron with the documented.envlookup orderDATA_DIRand pass that sameDATA_DIRto the child serverSTORAGE_ENCRYPTION_KEYwhen encrypted credentials already existProblem
The documented behavior says OmniRoute loads config from
~/.omniroute/.envor./.env.In practice, the published CLI wrapper already does that, but the app bootstrap paths used by:
npm run start/scripts/run-next.mjsscripts/run-standalone.mjswere bypassing the CLI wrapper behavior and bootstrapping from
server.envplus./.envinstead.On a machine with an existing encrypted
storage.sqlite, that can generate a newSTORAGE_ENCRYPTION_KEYinserver.envand make previously stored tokens fail to decrypt.What I Observed
On a local instance using the default
DATA_DIR:~/.omniroute/.envalready contained aSTORAGE_ENCRYPTION_KEY~/.omniroute/storage.sqlitealready contained encrypted provider credentials (enc:v1:...)~/.omniroute/server.envUnsupported state or unable to authenticate datadecryption failuresI verified the mismatch directly against a real encrypted row:
~/.omniroute/.envsuccessfully decrypted the stored tokenserver.envfailed on the same tokenThat is the implementation gap this PR fixes.
Root Cause
The startup paths were inconsistent:
bin/omniroute.mjsalready prefersDATA_DIR/.env, then~/.omniroute/.env, then./.envbootstrapEnv()did not mirror that lookupDATA_DIRto the child serverThis meant different launch modes could resolve different secret sources while still pointing at the same SQLite database.
Fix
scripts/bootstrap-env.mjsuse the same preferred.envlookup asbin/omniroute.mjsDATA_DIRinstead of a divergent pathDATA_DIRthrough to the Electron child server processVerification
tests/unit/bootstrap-env.test.mjsbootstrapEnv()now prefers~/.omniroute/.envoverserver.envbootstrapEnv()refuses to generate a new key when encrypted credential rows already existnode --test tests/unit/bootstrap-env.test.mjsNotes
This PR is intentionally narrow and does not address unrelated runtime issues such as standalone startup warnings or separate schema/migration problems.