fix(api): reject unsupported exchange methods before side effects - #101
fix(api): reject unsupported exchange methods before side effects#101seonghobae wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthrough
Changes/exchange 메서드 계약
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/exchange-method-contract.test.ts (1)
23-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win새 405 응답 헤더도 회귀 테스트로 고정하세요.
Line 23-35의
expectMethodNotAllowed는 상태 코드,Allow, JSON 본문만 확인합니다. 구현은content-type,cache-control: no-store,pragma: no-cache,x-content-type-options: nosniff,x-trace-id를 추가했습니다. 이 검증이 없으면 보안, 캐시 제어, 추적 헤더가 제거되어도 테스트가 통과합니다.응답 본문을 변수에 저장한 뒤 필수 헤더와
body.trace_id및x-trace-id의 일치를 확인하세요.As per coding guidelines:
test/**/*.ts: “Add or update regression tests for security and API behavior changes; use Vitest and preserve assertions covering token non-disclosure and protocol contracts.”제안하는 테스트 변경
async function expectMethodNotAllowed(response: Response): Promise<void> { expect(response.status).toBe(405); expect(response.headers.get("allow")).toBe("POST"); - expect(await response.json()).toMatchObject({ + expect(response.headers.get("content-type")).toBe("application/json; charset=utf-8"); + expect(response.headers.get("cache-control")).toContain("no-store"); + expect(response.headers.get("pragma")).toBe("no-cache"); + expect(response.headers.get("x-content-type-options")).toBe("nosniff"); + const traceId = response.headers.get("x-trace-id"); + expect(traceId).toEqual(expect.any(String)); + const body = await response.json(); + expect(body).toMatchObject({ ok: false, error_code: "ERR_VALIDATION_INPUT", message: "Method not allowed", details: { allowed_methods: "POST", hint: expect.any(String), }, }); + expect(body.trace_id).toBe(traceId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/exchange-method-contract.test.ts` around lines 23 - 35, Update expectMethodNotAllowed to store the parsed response body, then assert the required content-type, cache-control, pragma, x-content-type-options, and x-trace-id headers. Preserve the existing 405, Allow, and JSON contract assertions, and additionally verify that body.trace_id matches the x-trace-id header.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/exchange-method-contract.test.ts`:
- Around line 23-35: Update expectMethodNotAllowed to store the parsed response
body, then assert the required content-type, cache-control, pragma,
x-content-type-options, and x-trace-id headers. Preserve the existing 405,
Allow, and JSON contract assertions, and additionally verify that body.trace_id
matches the x-trace-id header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96ff38d6-db09-4315-a9b7-a39aea9fdb7b
📒 Files selected for processing (2)
src/entrypoint.tstest/exchange-method-contract.test.ts
|
Superseded by #214 on the current protected-main lineage. Fresh proof immediately before this decision:
#101's complete two-file delta is preserved byte-for-byte on #214:
#101 has no current inline review threads to preserve. #214 also has zero current inline review threads and its exact current head has fresh terminal-success Closing #101 unmerged removes the historical duplicate without losing behavior, test coverage, or review state. #214 remains the sole current-main implementation owner and is otherwise left untouched in this invocation because its branch moved during the fresh writer-state sweep. |
|
Superseded by protected-main integration through PR #221. Fresh pre-closure proof:
No predecessor checks/reviews transfer. The merged protected line preserves the complete runtime fix and a stronger executable contract, so this stale PR is closed unmerged as superseded. |
1 participant
Purpose
Make
/exchangemethod validation fail closed before credential-egress configuration or distributed rate-limit state is consulted, while preserving HTTP method semantics. Unsupported methods return the public405contract without leaking configuration/limiter availability or consuming limiter capacity;HEADreturns the same status/representation metadata without response content.Exact source identity
mainatc85d710804139c0697d7ef8fa47d02b1389e6d84;fix/exchange-method-short-circuit-on-main;4b4af1506aad1e0d67e408afa198eeafbb7ba006;0572ace139819fb41f9507ca72ca430691a5a045;50af2026b30693524d5307b4e9449c888d4dc8b8;657652cd573f9dd00c148e7bd6ed4b351e2c48a3;4df1b1983a00fda499b5cae5f402b9af3bbd2c18;6e49407fddef1ad493a66341537560d1dfe19a6e.RCA -> RED -> GREEN
The original repair moved method validation to the outer
/exchangeentrypoint before OIDC envelope/body checks, GitHub egress configuration, outbound-policy activation, and the distributed limiter. A throwing limiter sentinel and intentionally invalid GitHub API base prove unsupported methods short-circuit those side effects.CodeRabbit then correctly noted that the new 405 security/protocol headers were not pinned. The regression was hardened to assert content type,
no-store,Pragma,nosniff, trace presence, and body/header trace-ID consistency.A later method-matrix audit added
HEAD, but the shared JSON helper still constructed a response body forHEAD. The first failing boundary wasexchangeMethodResponse: the route correctly rejectedHEADwith 405 before side effects, but the returnedResponsestill contained JSON content.Exact RED
4df1b1983a00fda499b5cae5f402b9af3bbd2c18adds a dedicatedHEAD /exchangeregression that preserves the same 405/Allow/security/trace metadata but requires an empty response body. Applicationcirun31423347385, job93569113127, checked out that exact head and failed exactly the new assertion; 652 other tests passed.Current GREEN
6e49407fddef1ad493a66341537560d1dfe19a6ekeeps the same structured 405 payload for non-HEAD methods and passesnullresponse content only forHEAD. This is deliberately narrow; POST behavior and every credential-bearing path are unchanged.Current exact-head evidence
For unchanged head
6e49407fddef1ad493a66341537560d1dfe19a6eagainst protected basec85d710804139c0697d7ef8fa47d02b1389e6d84:cirun31423489615: terminal success, exact checkout verified;npm audit --audit-level=high: 0 vulnerabilities;reviewer-cirun31423489534: terminal success;Security Scanrun31423489537: terminal success under current protected-base scanner semantics;COMMENTEDevidence anchored to predecessor0572ace139819fb41f9507ca72ca430691a5a045, not qualifying approval;main.The ordinary release verification still reports non-strict KPI verification as
SKIPbecause no real retained production KPI log is present, and the acquisition manifest continues to identify missing final-gate production/release/deployment/governance/revenue/transfer evidence. Those states are not promoted to operational or acquisition success.Behavioral boundary
GET,HEAD,PUT,PATCH,DELETE, andOPTIONS /exchangefail before they can expose GitHub API trust/egress state, consume distributed rate-limit capacity, invoke a Durable Object limiter, or enter credential-bearing GitHub/OIDC work.HEADcarries no response content; the other unsupported methods retain the structured JSON error body. POST behavior, body/JWT bounds, egress policy, rate limiting, replay protection, credential minting, dependencies, workflow permissions, licensing, release, and deployment authority are unchanged.Merge / operational boundary
Technical exact-head success is not merge authority. Live enforceable governance under #27 and an eligible qualifying independent non-author formal-review path under #29 remain unverified/fail-closed. No self-approval, protection weakening, synthetic status, repair workflow, release, deployment, outbound-license decision, or acquisition-readiness claim is made.