Skip to content

fix(update): fence Bun runtime stop ownership - #618

Open
luvs01 wants to merge 3 commits into
devfrom
codex/investigate-runtime-stop-ownership-vulnerability
Open

luvs01 wants to merge 3 commits into
devfrom
codex/investigate-runtime-stop-ownership-vulnerability

Conversation

@luvs01

@luvs01 luvs01 commented Sep 22, 2026

Copy link
Copy Markdown
Owner

Motivation

  • The updater previously used an ownership decision captured before the destructive ocx stop, which allowed a race where ownership could change (desktop takeover) after observation but before the stop, letting the updater stop a runtime it no longer owned.

Description

  • Acquire the shared ownership mutation lease via acquireOwnershipMutationLease(serviceStatePaths()) before making the final stop authorization and re-read ownership under that lease to prevent TOCTOU races.
  • Compare the locked ownership subject to the initial observation and refuse the update while the lease is held if the subject changed, printing a clear refusal message and exiting early.
  • Delegate the lease token to the ocx stop child by creating a stop environment with ownershipMutationLeaseChildEnvironment(process.env, token) and pass it as env when spawning the stop child, and keep the lease through post-stop liveness checks and package replacement.
  • Tests updated to assert the new ordering and delegation behavior; primary edits touch src/update/index.ts and tests/update/update-desktop-owner.test.ts.

Testing

  • Ran bun run typecheck and it completed successfully.
  • Ran bun test tests/update/update-desktop-owner.test.ts and the focused suite passed (21 pass, 0 fail).
  • Ran bun test tests/ci-workflows/file-size-ratchet.test.ts and it passed (all relevant checks green).
  • Ran bun run structure:check and it passed.
  • Attempted bun test tests/update/update-stop-first.test.ts but the local Bun (v1.2.14) lacks the node:zlib zstdDecompressSync export required by the checkout; that focused test could not be executed in this environment.
  • Started bun run test (full suite) but the run was either interrupted or exercised unrelated long-running integration tests (tests/lab/*) and was not used as a validation gate for this focused change.

Codex Task

Link to Devin session: https://app.devin.ai/sessions/7af3920bf099491589d8e1176625d373
Open in Devin Desktop: https://app.devin.ai/desktop/session/7af3920bf099491589d8e1176625d373?variant=devin


Devin Review

Link to Devin session: https://app.devin.ai/sessions/7af3920bf099491589d8e1176625d373
Open in Devin Desktop: https://app.devin.ai/desktop/session/7af3920bf099491589d8e1176625d373?variant=devin


Devin Review

@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Repository: luvs01/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 61a9fb5e-3ed8-43d3-acd3-93bd37e24847


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-22T12:44:11.949750Z 2e8c489 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 22, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e8c489bcc

ℹ️ 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".

Comment thread src/update/index.ts
Comment on lines +479 to +483
if (lockedOwnership.subjectToken !== initialOwnership.subjectToken || !lockedPlan.mayReplacePackage) {
replacementLease.release();
console.error(lockedPlan.notice
?? "⚠️ Update stopped because runtime ownership changed before stop authorization; rerun from the beginning.");
process.exit(1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore the tray when locked ownership aborts

On Windows, if ownership changes after the initial observation but before this locked recheck, the updater has already stopped a running tray via handoffWindowsTrayForUpdate, yet this new early-exit path releases the lease and terminates without restarting it. This leaves the user's tray unexpectedly down even though no runtime stop or package replacement occurred; restore it when trayWasRunning is true, as the later replacement-refusal path does, or acquire and validate the lease before the tray handoff.

Useful? React with 👍 / 👎.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Devin Review

Comment thread src/update/index.ts
Comment on lines +479 to +483
if (lockedOwnership.subjectToken !== initialOwnership.subjectToken || !lockedPlan.mayReplacePackage) {
replacementLease.release();
console.error(lockedPlan.notice
?? "⚠️ Update stopped because runtime ownership changed before stop authorization; rerun from the beginning.");
process.exit(1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 A fenced pre-stop refusal leaves the Windows tray stopped

The updater stops a running Windows tray before acquiring and validating the ownership lease. If ownership changes, this branch exits without restoring the tray, leaving its UI unavailable until manually restarted.

Learn more

The Windows tray handoff occurs before the ownership lease is acquired. A running tray is stopped and recorded in trayWasRunning. The newly added locked-ownership refusal releases the lease and exits directly, unlike the later replacement refusal, which calls startWindowsTray() first. Lease acquisition itself can also throw after the tray has stopped, producing the same stranded-tray outcome.

Example: The tray is running when ocx update begins. The desktop app changes runtime ownership before line 474. The locked subject differs from the initial subject, so the updater exits here; the package and proxy remain untouched, but the tray remains stopped.

Recommended fix: Acquire and validate the ownership lease before tray handoff, or wrap every post-handoff pre-replacement failure—including lease acquisition and this refusal—in shared best-effort tray restoration. Add a behavioral regression test that simulates an ownership change after handoff and verifies the tray restart callback runs.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +179 to +191
test("the Bun updater fences and delegates its final stop authorization", () => {
const leaseAt = bunPath.indexOf("const replacementLease = acquireOwnershipMutationLease");
const lockedReadAt = bunPath.indexOf("const lockedOwnership = await resolvedRuntimeOwnership()", leaseAt);
const stopAt = bunPath.indexOf('selfLaunchArgv(["stop"])', lockedReadAt);
const releaseAt = bunPath.indexOf("replacementLease.release()", stopAt);
expect(leaseAt).toBeGreaterThan(-1);
expect(lockedReadAt).toBeGreaterThan(leaseAt);
expect(stopAt).toBeGreaterThan(lockedReadAt);
expect(releaseAt).toBeGreaterThan(stopAt);
expect(bunPath.slice(lockedReadAt, stopAt)).toContain("lockedOwnership.subjectToken !== initialOwnership.subjectToken");
expect(bunPath.slice(lockedReadAt, stopAt)).toContain("ownershipMutationLeaseChildEnvironment");
expect(bunPath.slice(stopAt, releaseAt)).toContain("env: stopEnvironment");
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 The race fix lacks behavioral regression coverage

The new test only searches source text for ordering and environment delegation. It cannot prove an ownership transition blocks stop or that the child joins the lease.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
devin-ai-integration Bot force-pushed the codex/investigate-runtime-stop-ownership-vulnerability branch from 15237d4 to 5b442bb Compare September 22, 2026 13:30

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark bug Something isn't working codex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant