Skip to content

fix(security): preserve external JSON decisions across sync cancel faults - #676

Merged
seonghobae merged 3 commits into
mainfrom
fix/external-json-sync-cancel-authority-20260912
Sep 12, 2026
Merged

fix(security): preserve external JSON decisions across sync cancel faults#676
seonghobae merged 3 commits into
mainfrom
fix/external-json-sync-cancel-authority-20260912

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Purpose

Repair #674 after the #673/#675 single-writer lane released. The target is Noema's existing base credential-exchange external JSON reader only: synchronous reader.cancel() cleanup faults must not replace an already-decided read-deadline or byte-limit failure. OIDC/GitHub identity, provider routing, outbound policy, credentials, quarantine/security authority, release/deployment authority, and foreign domain truth remain outside this repair.

Verified RED

Test-only exact 69c99113b523381661ed442c4d8667b0af1acfc9 added hostile synchronous-cancel coverage for both cancellation sites without changing production source. Hosted application CI run 34694095240 checked out that exact head, passed toolchain/install/typecheck, then failed release tests with 2 failed / 4,737 passed:

  • deadline path: synchronous cleanup exception escaped from src/index.ts:443, replacing the established read-timeout decision;
  • oversize path: the canonical SyntaxError("JSON response exceeded byte limit") was never constructed because synchronous cancellation threw first.

Minimal causal repair

Source exact 42c6c62fefa3316c7e1b977e2a79e93d16e3bab9 wrapped the existing best-effort cancellation in a small local try/catch, still sinks asynchronous rejection, never awaits cancellation completion, and reuses the helper at the deadline and byte-limit sites. Its application CI then failed 1 / 4,739 only because the protected executable authority test still required two duplicated raw reader.cancel().catch(...) call sites. That was a stale test/contract finding, not a reason to weaken the runtime repair.

Final exact f8de7fc27d475c76477271fa50dd7032afd82a6c converges that protected test to the guarded helper shape while retaining the #668 authority assertions. The existing 65,536-byte ceiling, 10-second absolute deadline, fixed retained buffer, fatal UTF-8/duplicate-key JSON admission, reader-lock cleanup, and fail-closed caller behavior remain unchanged. Diff review shows only the guarded cancellation helper, the protected authority-test convergence, and the two hostile regressions.

Exact-head GREEN

All required hosted gates are terminal SUCCESS on f8de7fc27d475c76477271fa50dd7032afd82a6c:

  • application CI 34694527820
  • reviewer-ci 34694527834
  • Security Scan 34694527843
  • patch-validator-image 34694527873

No force push, destructive rebase, self-approval, or gate weakening was used. This PR repairs only the source defect in #674; canonical CHANGELOG/product-gap documentation and immutable release/deployment/production evidence remain separate successor authority.

Summary by CodeRabbit

  • 버그 수정

    • 외부 JSON 응답을 읽는 중 취소 처리에서 오류가 발생하더라도 타임아웃 및 응답 크기 제한 결과가 정상적으로 유지됩니다.
    • 응답 읽기가 제한 시간을 초과하면 기존의 502 오류 응답이 반환됩니다.
    • 응답이 허용된 바이트 한도를 초과하면 크기 제한 오류가 일관되게 반환됩니다.
  • 테스트

    • 동기적 취소 오류와 응답 읽기 중단 상황에 대한 검증을 추가했습니다.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 3bce6103-d058-4aa8-b726-1712a8cba6a5

📥 Commits

Reviewing files that changed from the base of the PR and between abc41f1 and f8de7fc.

📒 Files selected for processing (3)
  • src/index.ts
  • test/external-json-cancellation-authority.test.ts
  • test/external-json-sync-cancel-authority.test.ts

📝 Walkthrough

Walkthrough

외부 JSON 스트림의 취소 처리를 공통 헬퍼로 통합했습니다. 동기 취소 예외를 무시하도록 변경했습니다. 타임아웃과 바이트 제한 경로의 결과를 검증하는 테스트를 추가했습니다.

Changes

외부 JSON 스트림 취소 처리

Layer / File(s) Summary
취소 헬퍼와 호출 경로
src/index.ts
reader.cancel()의 동기 예외와 프로미스 거부를 처리하는 cancelReaderBestEffort를 추가했습니다. 타임아웃과 바이트 제한 초과 경로에서 헬퍼를 호출합니다.
동기 취소 예외 검증
test/external-json-cancellation-authority.test.ts, test/external-json-sync-cancel-authority.test.ts
공통 헬퍼 사용을 검증합니다. 동기적인 취소 예외가 타임아웃 및 바이트 제한 결과를 변경하지 않는지 검증합니다.

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

Change: Bug fix

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/external-json-sync-cancel-authority-20260912

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.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exact-head review for f8de7fc27d475c76477271fa50dd7032afd82a6c: verified causal RED on test-only 69c99113b523381661ed442c4d8667b0af1acfc9; the intermediate application-CI failure on 42c6c62... was a stale protected static authority assertion and is converged without weakening runtime behavior. Final diff is limited to the guarded best-effort cancellation helper, two hostile synchronous-cancel regressions, and the protected authority-test shape. Application CI 34694527820, reviewer-ci 34694527834, Security Scan 34694527843, and patch-validator-image 34694527873 are all terminal SUCCESS on this exact head. No unresolved inline review threads. This supports normal source merge only, not immutable release/deployment/production evidence.

@seonghobae
seonghobae marked this pull request as ready for review September 12, 2026 12:49
@seonghobae
seonghobae merged commit 6844c78 into main Sep 12, 2026
17 of 18 checks passed
@seonghobae
seonghobae deleted the fix/external-json-sync-cancel-authority-20260912 branch September 12, 2026 12:50
seonghobae added a commit that referenced this pull request Sep 12, 2026
seonghobae added a commit that referenced this pull request Sep 12, 2026
seonghobae added a commit that referenced this pull request Sep 12, 2026
…nc-cancel-authority-20260912

docs(noema): converge #676 synchronous-cancel authority
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant