fix(browser): add missing Linux Chromium fallback paths to findChro...#48563
fix(browser): add missing Linux Chromium fallback paths to findChro...#48563lupuletic wants to merge 4 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR improves Linux browser detection by adding four commonly-missed Chromium-based executable paths to Key changes:
Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/browser/chrome.ts
Line: 267-270
Comment:
**Error message could include the config file path**
The new hint is a great improvement, but it leaves the user guessing where "your OpenClaw config" actually lives. Since `CONFIG_DIR` is already imported in this file, you can make the message immediately actionable by embedding the actual file path:
```suggestion
throw new Error(
"No supported browser found (Chrome/Brave/Edge/Chromium on macOS, Linux, or Windows)." +
` Set \`browser.executablePath\` in ${CONFIG_DIR}/openclaw.json to the path of a Chromium-based browser.`,
);
```
This way users on any OS see exactly which file to edit without having to hunt for it.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: dc657c1 |
847e158 to
a8e9d4a
Compare
0a713e3 to
1eeaca1
Compare
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
Vulnerabilities1. 🟡 Linux browser auto-discovery executes first existing path without validating file type/permissions (TOCTOU + symlink risk)
Description
This is risky because:
Vulnerable code: function findFirstExecutable(candidates: Array<BrowserExecutable>): BrowserExecutable | null {
for (const candidate of candidates) {
if (exists(candidate.path)) {
return candidate;
}
}
return null;
}RecommendationHarden executable discovery before returning a path:
Example: import fs from "node:fs";
import path from "node:path";
function isSafeExecutable(p: string): boolean {
try {
const st = fs.lstatSync(p);
if (!st.isFile()) return false;
if (st.isSymbolicLink()) return false; // or resolve+validate realpath
fs.accessSync(p, fs.constants.X_OK);
return true;
} catch {
return false;
}
}
function findFirstExecutable(candidates: Array<BrowserExecutable>): BrowserExecutable | null {
for (const candidate of candidates) {
if (isSafeExecutable(candidate.path)) return candidate;
}
return null;
}Additionally, when running as root/in containers, consider requiring an explicit Analyzed PR: #48563 at commit Last updated on: 2026-03-29T14:16:31Z Latest run failed. Keeping previous successful results. Trace ID: Last updated on: 2026-03-30T09:47:26Z |
1eeaca1 to
ce63a6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce63a6ec6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -303,7 +303,8 @@ export async function launchOpenClawChrome( | |||
| const exe = resolveBrowserExecutable(resolved); | |||
| if (!exe) { | |||
| throw new Error( | |||
| "No supported browser found (Chrome/Brave/Edge/Chromium on macOS, Linux, or Windows).", | |||
| "No supported browser found (Chrome/Brave/Edge/Chromium on macOS, Linux, or Windows)." + | |||
| ` Set \`browser.executablePath\` in ${CONFIG_DIR}/openclaw.json to the path of a Chromium-based browser.`, | |||
There was a problem hiding this comment.
Use active config path in browser-not-found hint
The new error text hardcodes ${CONFIG_DIR}/openclaw.json, but OpenClaw can load config from other locations (for example via OPENCLAW_CONFIG_PATH/profile-specific path resolution in resolveConfigPath in src/config/paths.ts). In those environments this points users to the wrong file, so they can set browser.executablePath in a file that is never read and still fail to launch a browser.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
CONFIG_DIR already respects the OPENCLAW_STATE_DIR override and is the canonical config path variable used elsewhere (e.g. src/tts/tts.ts). The "active config" concern is addressed by this variable — it resolves correctly in all environments.
fa4804d to
1546790
Compare
1546790 to
cbe89bb
Compare
|
Codex deep review: this still looks valid and current-path. What it solves: Linux managed-browser startup currently only checks I do not think the security-bot finding should block this PR. The existing auto-detection code already returns the first existing absolute system path; this PR does not introduce PATH lookup or a new trust boundary. If we want to harden executable validation, that should be a separate browser discovery hardening change across all platforms and configured Remaining before merge: rebase/rerun. Current PR CI is stale/red across broad unrelated lanes, while the browser extension lane passed. I did not find a code blocker in this diff. |
|
Codex deep review update: the fix is still useful, but the branch is too stale to merge directly. Current
Those are worth adding with regression coverage. I still do not think the security-bot finding blocks this narrow path addition: OpenClaw already auto-detects absolute system browser paths, and executable validation hardening should be a separate cross-platform change covering all auto-discovery/configured executable paths. But this branch is ~8.3k commits behind |
Co-authored-by: Catalin Lupuleti <105351510+lupuletic@users.noreply.github.com>
|
Landed via maintainer rewrite on current Kept the useful payload from this PR:
Skipped the stale Source PR head: cbe89bb Thanks @lupuletic! |
Co-authored-by: Catalin Lupuleti <105351510+lupuletic@users.noreply.github.com>
Co-authored-by: Catalin Lupuleti <105351510+lupuletic@users.noreply.github.com>
Co-authored-by: Catalin Lupuleti <105351510+lupuletic@users.noreply.github.com>
Linux browser detection misses common Chromium install paths (e.g. /usr/lib/chromium/chromium) and the error message doesn't guide users to set browser.executablePath in config as a workaround
Closes #19185
Changes:
Testing:
AI-assisted (Claude + Codex committee consensus, fully tested).
AI-Assisted PR Checklist