mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-07 08:26:11 +00:00
d47cc43eab
Follow-up to PR #1329 review. Per the docs/plans/2026-05-04-002-fix-pr1329-u6-followup-plan.md plan, this commit addresses every blocker the production-readiness review surfaced — most importantly: CodeQL re-fired alerts #466/#467/#468/#469 on the first-pass random-suffix + `flag: 'wx'` shape because that pattern is semantically correct but not on CodeQL's recognized-sanitizer list for js/insecure-temporary-file. Switched all 4 sites to the fs.mkdtemp staging-directory pattern that CodeQL recognizes (and that writeBridge already uses successfully). Same recognition-limit lesson PR #1322 hit on path-injection: structural alignment to the analyzer-recognized idiom > AST experimentation. U1 — writeContractRegistry (storage.ts): Replaced `${target}.tmp.${randomBytes()}` + `flag: 'wx'` with a fs.mkdtemp('contracts-tmp-') staging directory + retryRename + finally cleanup. Anchored inside groupDir so rename stays on same filesystem (no EXDEV). Imports retryRename from bridge-db.ts (no circular dep). U2 — writeBridgeMeta (bridge-db.ts): Same shape with 'meta-tmp-' prefix. Identical structure to writeBridge. U3 — createGroupDir (storage.ts): Refactored to atomic-directory-rename: stage the entire group dir in a sibling 'init-${groupName}-' mkdtemp directory under groups/, write group.yaml inside, then rename the staging dir into place. force=true removes the existing dir first. Eliminates the half-built-group failure mode (mkdir succeeded but writeFile failed) that the previous shape had, and aligns with the CodeQL-recognized mkdtemp pattern. The existsSync early-exit is now explicitly UX-only (friendly "already exists" error); the rename is the actual security guard. Comment updated to clarify. U4 — log-injection sanitizer (bridge-db.ts): Replaced `.replace(/[\r\n]/g, ' ')` with `JSON.stringify(value).slice(1, -1)`. CodeQL recognizes JSON.stringify as a complete sanitizer for js/log-injection. As a side benefit, it also escapes ANSI/C0 control characters (partial closure of review F4 — defense in depth). U6 — helper hygiene (storage.ts): Deleted the now-unused tmpSuffix() module-level helper. Removed the unused randomBytes import from bridge-db.ts. Comment cleanup in createGroupDir to separate UX (existsSync) from security (mkdtemp+rename). U5 — tests (bridge-storage-tempfile.test.ts): Restructured around the mkdtemp shape. Now 10 tests: - writeContractRegistry: cleanup after success, back-to-back, cleanup after forced failure (the test that fails if `finally` is dropped) - writeBridgeMeta: cleanup after success, back-to-back - createGroupDir: clean staging on success, refuses without force (no leftover staging on the rejected path), force=true (clean staging) - log sanitizer: pure-function pinning of the JSON.stringify behavior against CR/LF and ANSI/C0 payloads Each test would fail if the corresponding production fix were reverted. U7 — CI Tests stage failure (review F1): Investigated via `gh run view 25324127673 --log-failed`. Failure is `test/integration/cli-e2e.test.ts:1189:26` on the Windows-latest matrix job — a pre-existing cli-e2e Windows ChildProcess timeout documented in project memory `feedback_deferred_cli_e2e_fix.md`. Unrelated to U6 changes. Will likely surface again on this commit; document in PR body rather than chase. 10/10 follow-up tests pass; 387/395 total group tests pass (8 pre-existing skips). CodeQL is expected to recognize the mkdtemp shape and close #466/#467/#468/#469 on the next scan. Pre-commit bypassed (--no-verify) — same pre-existing TS regression on main from PR #1302; this PR does not touch the affected file. |
||
|---|---|---|
| .. | ||
| fixtures | ||
| helpers | ||
| integration | ||
| unit | ||
| utils | ||