fix(usage): bound the usage-log permission hardening cache - #4976
Conversation
…eanup Co-authored-by: chilung <b0423031@gmail.com>
Keep consecutive appends off redundant filesystem hardening while reapplying owner-only modes after a one-second cache window. Co-authored-by: chilung <b0423031@gmail.com>
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 77 / 80이 PR(#4976)은 기여자 지금 고치는 방법은 모듈 캐시 이 로그는 테스트는 세 갈래입니다. (1) 라인 src/usage/log.ts usageLogPermissionCheckIsCurrent - 1초 창 안에는 외부에서 풀어 둔 모드가 그대로 남을 수 있다. 본문이 명시한 한계이지만, 장시간 idle 뒤 첫 append 전에도 풀어져 있으면 그 한 번까지는 남는다(다음 경계에서야 고쳐짐). 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Merging with macOS legs outstanding, and recording why rather than leaving it implicit. At this exact head the full Linux suite (test 1/4 through 4/4), This change is platform-neutral, so waiting on a queue that is both saturated and known-unreliable would delay the work without adding information. The evidence that governs the release is not per-PR macOS legs; it is the full-platform Stating the boundary plainly: this is merged on Linux, gates and cross-platform smoke evidence at its exact head, with macOS coverage deferred to the candidate run rather than claimed here. |
Carries #4795 by @chilung-cgu, rebased onto current
devwith the review finding fixed. Original commit authorship is preserved.Summary
appendUsageEntrycalledmkdirSyncand twochmodSyncon every single append. The original PR removed that syscall storm by remembering the path it had already hardened.The defect this carry fixes. That cache was keyed on the path alone and never expired, so after the first append the process would never narrow permissions again. An externally widened mode —
0755on a directory that must be0700, or a world-readableusage.jsonl— stayed widened for the life of the process. The perf win was real and the correctness loss was real, and they are separable: the cache now expires after one second. Consecutive appends still skip the syscalls; the first append at or after the boundary reapplies directory mode0700before the write and file mode0600after it. A path change, a clock rollback, or anENOENTon append also invalidates it.Security boundary. The usage log is an append-only ledger of request metadata and bounded account attribution, kept owner-only by POSIX mode on both the directory and the file. That mode is the whole boundary.
What this does and does not guarantee. It guarantees bounded, write-triggered repair: an external widening is corrected on the next append at or after the one-second boundary, and
chmodremains best-effort on platforms that ignore it. It does not monitor the filesystem, does not stop another local process from widening the mode again immediately after a check, and does not narrow the window betweenappendFileSynccreating the file and the followingchmod. It adds no symlink or path-replacement defense beyond what was already there; those boundaries are unchanged.Verification
Local verification was not run: this lane forbids running any local suite, typecheck, build, or install. Hosted CI on this PR head is the executable verification.
Static checks performed in place of local execution:
chmodcalls permanently, not merely for a burst — the only invalidations were a path change and anENOENTretry.recordOwnedConfigPathwas read to confirm it is idempotent and separately cached, so calling it once per second rather than once per append changes no ownership bookkeeping. Its ownmkdirSyncis guarded by an existence check, which is what makes the syscall-count assertion in the new test exact rather than approximate.Date.nowso it measures the cache rather than the runner's scheduling: five appends that straddled the one-second boundary would legitimately harden twice, and a saturated runner can take that long.Checklist
Co-authored-by: chilung b0423031@gmail.com