fix(desktop): fail closed when adopted host-service has no version#3679
Conversation
The version gate only killed the host when fetchHostVersion returned a string less than MIN_HOST_SERVICE_VERSION. If the host lacked the host.info route entirely (older releases), the helper returned null and we silently adopted it, producing 404s on project.findByPath and other new routes. Flip the guard to fail-closed: any host that can't prove it meets the minimum version is killed and the manifest removed so the next spawn brings up a compatible service.
📝 WalkthroughWalkthroughHost-service adoption now validates reported versions with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Greptile SummaryThis PR fixes a fail-open vulnerability in the host-service adoption path: previously, when
Confidence Score: 5/5Safe to merge — a minimal, correct fail-closed guard fix with no interface or behavioral regressions for valid services. The change is a single boolean condition flip that correctly addresses the described bug. The null/falsy case was the only missing branch; the existing kill/remove/respawn path was already sound. The only comment is a P2 log-message clarity suggestion that doesn't affect correctness. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/host-service-coordinator.ts | Guard condition in tryAdopt flipped from fail-open (version && version < MIN) to fail-closed (`!version |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[tryAdopt called] --> B[readAndValidateManifest]
B -->|null| C[return null]
B -->|manifest| D[fetchHostVersion\nGET /trpc/host.info]
D -->|network/HTTP error| E[version = null]
D -->|pre-host.info binary\n404 / bad response| E
D -->|valid response| F[version = string]
E --> G{"!version ||\nversion < MIN_HOST_SERVICE_VERSION\n← CHANGED: was 'version &&'"}
F --> G
G -->|true – fail closed| H[SIGTERM pid\nremoveManifest\nreturn null]
G -->|false – version OK| I[instances.set / adopt\nreturn Connection]
style G fill:#f9c,stroke:#c66
style H fill:#fcc,stroke:#c44
style I fill:#cfc,stroke:#4c4
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-coordinator.ts
Line: 298-301
Comment:
**Misleading log message for unknown version**
When `version` is `null`, the log line reads `Adopted service version unknown < 0.2.0, killing`, implying the version was compared and found to be too old. The actual reason is that the version couldn't be determined at all. Consider splitting the message to make root-cause clearer:
```suggestion
if (!version || version < MIN_HOST_SERVICE_VERSION) {
const reason = !version
? `version unknown (host predates host.info)`
: `version ${version} < ${MIN_HOST_SERVICE_VERSION}`;
console.log(
`[host-service:${organizationId}] Adopted service ${reason}, killing`,
);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): fail closed when adopted h..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| console.log( | ||
| `[host-service:${organizationId}] Adopted service version ${version} < ${MIN_HOST_SERVICE_VERSION}, killing`, | ||
| `[host-service:${organizationId}] Adopted service version ${ | ||
| version ?? "unknown" | ||
| } < ${MIN_HOST_SERVICE_VERSION}, killing`, |
There was a problem hiding this comment.
When version is null, the log line reads Adopted service version unknown < 0.2.0, killing, implying the version was compared and found to be too old. The actual reason is that the version couldn't be determined at all. Consider splitting the message to make root-cause clearer:
| console.log( | |
| `[host-service:${organizationId}] Adopted service version ${version} < ${MIN_HOST_SERVICE_VERSION}, killing`, | |
| `[host-service:${organizationId}] Adopted service version ${ | |
| version ?? "unknown" | |
| } < ${MIN_HOST_SERVICE_VERSION}, killing`, | |
| if (!version || version < MIN_HOST_SERVICE_VERSION) { | |
| const reason = !version | |
| ? `version unknown (host predates host.info)` | |
| : `version ${version} < ${MIN_HOST_SERVICE_VERSION}`; | |
| console.log( | |
| `[host-service:${organizationId}] Adopted service ${reason}, killing`, | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-coordinator.ts
Line: 298-301
Comment:
**Misleading log message for unknown version**
When `version` is `null`, the log line reads `Adopted service version unknown < 0.2.0, killing`, implying the version was compared and found to be too old. The actual reason is that the version couldn't be determined at all. Consider splitting the message to make root-cause clearer:
```suggestion
if (!version || version < MIN_HOST_SERVICE_VERSION) {
const reason = !version
? `version unknown (host predates host.info)`
: `version ${version} < ${MIN_HOST_SERVICE_VERSION}`;
console.log(
`[host-service:${organizationId}] Adopted service ${reason}, killing`,
);
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Around line 297-301: The current check in host-service-coordinator.ts uses
string comparison (version < MIN_HOST_SERVICE_VERSION) which breaks semantic
version ordering; update the logic in the block that references version and
MIN_HOST_SERVICE_VERSION to use a proper semver comparison (e.g., import and use
the semver.compare/gt/lt or semver.satisfies utilities) so that null/undefined
still triggers the fail-closed path but otherwise compare numerically (e.g.,
semver.lt(version, MIN_HOST_SERVICE_VERSION)) before logging and killing; ensure
you handle pre-release/build metadata correctly by using the semver library
rather than string comparisons.
🪄 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: 7f4283a4-f8ad-4e64-8235-1219bcc33b01
📒 Files selected for processing (1)
apps/desktop/src/main/lib/host-service-coordinator.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/host-service-coordinator.ts">
<violation number="1" location="apps/desktop/src/main/lib/host-service-coordinator.ts:297">
P2: Use proper semver comparison instead of the `<` operator. String comparison is lexicographic, so `"0.10.0" < "0.2.0"` evaluates to `true` in JavaScript, meaning any host-service version with a double-digit component will be incorrectly killed. A simple numeric split-and-compare helper would fix this.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
String comparison made "0.10.0" < "0.2.0" evaluate to true, which would have killed any future double-digit minor version. Split the version on dots and compare segments as integers. Also split the adopt-killing log into two branches so the null case reads "version unknown" instead of "version unknown < 0.2.0".
Replace the hand-rolled split-and-compare with `semver.satisfies`, which already handles malformed input by returning false. The `semver` package is already a dependency.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
fetchHostVersionreturned a string belowMIN_HOST_SERVICE_VERSION. If the host predatedhost.infoentirely, the helper returnednulland we silently kept the stale service, producing 404s on newer routes (project.findByPath, etc).Test plan
host.infohost-service binary → confirm it gets killed and respawned (log:Adopted service version unknown < 0.2.0, killing).project.findByPathworks on a fresh launch.Summary by cubic
Fail closed when adopting a host-service that can’t prove it meets the minimum version, preventing 404s on routes like
project.findByPath.Bug Fixes
semvercheck), kill the process and remove its manifest so the next spawn is compatible.Refactors
semverfor version checks; malformed versions are treated as unsupported.Written for commit 8ec6d07. Summary will update on new commits.
Summary by CodeRabbit