Skip to content

fix(config): skip optional hardening for non-directory homes - #7688

Merged
lidge-jun merged 31 commits into
lidge-jun:devfrom
luvs01:fix/config-nondirectory-hardening
Oct 27, 2026
Merged

lidge-jun merged 31 commits into
lidge-jun:devfrom
luvs01:fix/config-nondirectory-hardening

Conversation

@luvs01

@luvs01 luvs01 commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

When OPENCODEX_HOME points to a regular file, an account-store read can start optional directory hardening before a later profile check rejects the path. Check that the target is a directory before starting chmod or an ACL task. Keep directory symlinks/junctions supported, preserve the real-home guard, and leave required write-path validation unchanged.

Drain the two account suites' configuration-hardening work before removing their fixtures, using the original fixture directory even when a case changes OPENCODEX_HOME. Assertions, timeouts and the shared teardown helper's ordering remain unchanged.

Head d06cf9b3d5b55fc8dd024dc2854e5d0b778d2bd4 normally merges the current #7610 parent cf72010f480d607bdf390717c28f62be240b0981 into previous child a554968b. All five child-specific blobs are byte-identical to that child; the parent refresh does not change this guard or its callers. Retarget to dev after the parent lands.

Verification

On this merged head, official Windows Bun 1.4.4, using the existing CI timeout contract:

  • codex-account-slot-identity, codex-auth-headless-accounts and windows-config-read-acl-memo: 33 passed / 1 skipped / 0 failed. The inaccessible-default-home case explicitly excludes Windows; it is not counted as passing.
  • TypeScript, structure/index, five-file privacy/size and committed-diff checks passed.
  • Every native validation process exited 0 with both output streams complete and no remaining direct child. Isolated fixtures were cleaned. Existing dependencies were reused without installation.

The unchanged child at a554968b previously passed full cross-platform run 40109666635, attempt 2: 28 successful jobs / 6 conditional skips. That is historical evidence for a554968b, not a new-head full run. Earlier cancellations/timeouts and the older macOS hardlink failure are retained separately; this guard is not claimed to explain or repair them. #7610 owns the related catalog path-identity change.

Exact-head automatic CI 40227402907 passed. The regular-file guard does not solve concurrent path-replacement races or establish broad native permission guarantees. Merge order remains parent-first. No live runtime, account, certificate or GUI changed.

Checklist

  • Current parent and unchanged child contract verified.
  • Focused tests and source gates passed on the merged head.
  • Platform exclusions and historical CI scope recorded.
  • Final-head automatic CI.
  • Maintainer review.

Summary by CodeRabbit

  • Bug Fixes
    • Optional configuration-directory security hardening now skips paths that are missing, cannot be checked, or point to a regular file, avoiding permission changes to non-directories.
    • When a configuration path points to a directory through a symbolic link or junction, hardening applies to the directory it targets.
  • Tests
    • Added coverage for these behaviors on Windows and macOS.

luvs01 added 18 commits October 23, 2026 14:46
…tics

A relative symlink target containing .. was normalized lexically, so a symlinked intermediate directory made the comparator follow the wrong parent. Walk each intermediate component within the existing read budget: resolve symlinked or missing directories with realpath, treat non-directories and failures as unknown, and keep the leaf for the next lstat hop.
Measured OS behavior splits by platform: Windows normalizes .. lexically across junctions/reparse points and even past missing components, while POSIX resolves each directory and refuses a missing crossing. Keep lexical resolve on Windows; on POSIX walk each intermediate component inside the read budget, record intermediate symlinks in the revalidation snapshot, and treat missing or non-directory crossings as unknown. Regressions assert the independent readFileSync/realpathSync result before the comparator verdict, and the POSIX-only literal-backslash case is an explicit platform skip.
A symlinked intermediate that resolves to a regular file must fail the traversal with ENOTDIR on POSIX; dirname() after realpath would have accepted the wrong catalog. Stat the resolved intermediate inside the read budget, require a directory, and record the resolved identity in the link snapshot so revalidation also catches a target swap.
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 51ab52c and faeead3.

Files selected for processing: 2
  • src/config/paths.ts
  • structure/config.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Walkthrough

Config-directory hardening now checks that a path resolves to a directory before applying permission changes. Tests cover regular files and directory links. Integration-test teardown waits for hardening work before removing fixture roots.

Changes: Config Directory Hardening

Layer / File(s) Summary
Directory target validation
src/config/paths.ts, structure/config.md, tests/windows/windows-config-read-acl-memo.test.ts
Hardening skips paths that cannot be statted or do not resolve to directories. Documentation and tests cover regular files and directory symlinks or junctions.
Fixture cleanup synchronization
tests/codex-integration/codex-account-slot-identity.test.ts, tests/codex-integration/codex-auth-headless-accounts.test.ts
Asynchronous teardown drains and removes fixture roots, restores environment values, and flushes config-hardening work for the fixture’s ocx directory.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

Merge Risk: Minimal · up to faeea

The reviewed changes have no established merge-blocking risk.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title "fix(config): skip optional hardening for non-directory homes" matches the main change. The change makes hardenConfigDir call statSync and return early when the resolved path is missing, err…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "fix(config): skip optional hardening for non-directory homes" matches the main change. The change makes hardenConfigDir call statSync and return early when the resolved path is missing, errors on stat, or is not a directory, so optional chmod and ACL work is skipped. The PR objectives describe the same goal: OPENCODEX_HOME pointing to a regular file. The title is concise, names the type and scope, and does not list files. It is 60 characters, within a normal commit-title length.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 24, 2026
@luvs01
luvs01 changed the base branch from dev to codex/catalog-heal-hardlink-path-20261023 October 24, 2026 16:01
@luvs01
luvs01 marked this pull request as ready for review October 24, 2026 20:02
@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer decision for v2.93.0 release prep: DEFERRED_POST_RELEASE, together with its parent #7610.

The same independent review (gpt-6.1-sol, high) covered this child at d06cf9b3d5b55fc8dd024dc2854e5d0b778d2bd4 and found no findings in #7688's own changes. The directory check runs before chmod/ACL work, directory junctions are still followed, the real-home guard ordering is preserved, and the teardown drain does not weaken assertions. The parent #7610 is deferred, however, because of two blocking regressions (see #7610), and this head contains the parent's commits, so it cannot land on dev by itself as it stands.

After the release, this can be retargeted once #7610 lands. Alternatively, the five child-specific files can be rebased directly onto dev as an independent PR, since they do not depend on the catalog identity change. The exact-head lane=all dispatch for this head was cancelled together with this decision.

@lidge-jun
lidge-jun deleted the branch lidge-jun:codex/catalog-heal-hardlink-path-20261023 October 26, 2026 19:44
@lidge-jun lidge-jun closed this Oct 26, 2026
@lidge-jun lidge-jun reopened this Oct 26, 2026
@lidge-jun
lidge-jun changed the base branch from codex/catalog-heal-hardlink-path-20261023 to dev October 26, 2026 19:44
Retargeted to dev. The four conflicted files belong to lidge-jun#7610 and take dev's squashed version (9a61bd9); the five lidge-jun#7688 files are unchanged from the reviewed head d06cf9b apart from dev's own edits to structure/config.md.
@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer integration decision: MERGE (squash) into dev

Head: faeead3. It merges current dev (4111e93, which includes #7777) into 51ab52c. dev has not moved since, so the merge result equals the tested head.

Change. Optional config-directory hardening (src/config/paths.ts) now applies chmod/ACL work only when the config home is a directory, following directory symlinks and junctions as before. A non-directory or unstatable home is skipped after the real-home guard and before any mutation. The account test teardowns now drain their nested config flights.

Review. Independent sol review r2 (rel293-lb-review-7688-r2) on 51ab52c against dev with #7610 merged: PASS, 0 blocking. It confirmed directory links still harden, the real-home guard still runs before the stat gate, and all account-suite assertions are unchanged. The merge into dev touched only structure/config.md: dev's shorter paragraph is kept, plus this PR's non-directory sentence and test-coverage note. No code conflicts.

Combined-tree checks (pinned Bun 1.4.4): account-slot-identity, auth-headless-accounts, windows-config-read-acl-memo, the #7777 intercept ACL barrier, file-size ratchet and both layout guards: 100 pass, 1 skip (POSIX-only), 0 fail. Typecheck, structure SSOT, privacy scan and git diff --check: pass.

CI on the exact head faeead3:

NOT RUN: full suite locally, native macOS/Linux permission checks outside CI.

@lidge-jun
lidge-jun merged commit 19b7f19 into lidge-jun:dev Oct 27, 2026
67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants