fix(github): canonicalize accepted API base URLs - #430
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (36)
📝 WalkthroughWalkthroughCI 검증 단계를 분리했습니다. OIDC와 GitHub API 입력 검증을 강화했습니다. 요청 및 응답 스트림의 fail-closed 처리를 보완했습니다. App ID, installation ID, workflow SHA의 canonical 검사를 추가했습니다. Changes자격 증명 교환 검증
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Entrypoint
participant OIDCVerifier
participant GitHubAPI
Client->>Entrypoint: /exchange 요청
Entrypoint->>Entrypoint: 본문 및 Bearer envelope 검증
Entrypoint->>OIDCVerifier: OIDC 토큰 검증
OIDCVerifier->>GitHubAPI: 설치 및 토큰 요청
GitHubAPI-->>OIDCVerifier: JSON 응답
OIDCVerifier->>OIDCVerifier: 저장소와 토큰 만료 검증
OIDCVerifier-->>Client: 교환 결과 또는 제한된 오류
✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const expectedRepositoryOwnerId = "295022177"; | ||
| const expectedRepositoryIds = new Map<string, string>([ | ||
| ["ContextualWisdomLab/noema", "1285107801"], | ||
| ["ContextualWisdomLab/.github", "1274066402"], | ||
| ]); |
There was a problem hiding this comment.
🔍 Hardcoded owner/repository IDs become mandatory 403 gates
expectedRepositoryOwnerId and expectedRepositoryIds (index.ts) hardcode GitHub numeric IDs with no configuration. Real OIDC tokens always carry repository_owner_id, so the owner-id check (index.ts) applies to every request; a wrong constant fails all exchanges with 403. Tests reuse the same literals and cannot catch an incorrect value. Verify the IDs match the live org and repositories.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ALLOWED_WORKFLOW_REPOSITORY = "ContextualWisdomLab/.github" | ||
| ALLOWED_WORKFLOW_REF_PREFIX = "ContextualWisdomLab/.github/.github/workflows/noema-review.yml@refs/heads/main" | ||
| ALLOWED_WORKFLOW_SHA = "fce028b4c3bf8e2e5e4819c1c5622e90cfa6ab39" | ||
| ALLOWED_WORKFLOW_SHA = "5a8b83773bd5190d972eec3d7c76ac9504665f21" |
There was a problem hiding this comment.
🔍 Workflow source SHA must match live central commit
ALLOWED_WORKFLOW_SHA is rolled forward to 5a8b83773bd5190d972eec3d7c76ac9504665f21, and the test constant matches. OIDC verification requires job_workflow_sha to equal this value exactly (index.ts), so a stale or wrong SHA rejects all legitimate central-workflow tokens. Confirm the configured SHA is the current source commit for the audited workflow.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export function isBoundedOidcBearer(value: string | null): boolean { | ||
| if (value === null) return true; | ||
| if (!/^Bearer(?:\s|$)/i.test(value)) return true; | ||
| if (value.length > MAX_AUTHORIZATION_HEADER_LENGTH) return false; | ||
|
|
||
| const match = value.match(/^Bearer\s+(\S+)$/i); | ||
| if (!match) return true; | ||
| if (value.length > MAX_AUTHORIZATION_HEADER_LENGTH) return false; | ||
| if (!match) return false; | ||
|
|
||
| const segments = match[1].split("."); | ||
| if (segments.length !== 3) return false; |
There was a problem hiding this comment.
📝 Info: Bearer envelope now returns 400 instead of 401 for whitespace-only tokens
isBoundedOidcBearer now treats any value starting with Bearer (then whitespace or end) as a JWT envelope that must match ^Bearer\s+(\S+)$. Whitespace-only and embedded-whitespace Bearer values that previously reached the 401 missing-token path now fail closed with 400 ERR_TOKEN_MALFORMED. The prefilter test was updated to match; any client depending on the old 401 sees a changed status and error code.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Scope
Repair GitHub App exchange/readiness and credential-egress integrity without widening GitHub authority, weakening OIDC/replay controls, or relaxing release/security gates.
Current repair lineage
This canonical branch now includes the bounded Bearer-envelope repair plus follow-on exact-head regressions for request-body cleanup and installation-token expiry authority. During the current run, Application CI exposed two uncovered defensive branches rather than a product-policy failure:
POST /exchangerequests intentionally remain unchanged atboundExchangeJsonBody()because there is no runtime stream to consume; the regression now covers that exact contract instead of manufacturing a 413 from an unobservable body;+00:00offset are explicitly rejected, preserving canonical UTCZauthority.Central
.githubprotected main also advanced tob04a40807a71e807700bb67f2a0ea4d776f58b22. The auditednoema-review.ymlblob remains unchanged (59b25e343444d0b97fc1c7ba33cb15543dd70102), but GitHub OIDCjob_workflow_shabinds the source commit, so stale Noema configuration was a runtime trust defect. The executable trust regression was moved first, thenwrangler.tomlALLOWED_WORKFLOW_SHAwas minimally rolled forward to the same central commit.Existing canonical GitHub API origin, App/installation identifier, immutable owner/repository/workflow identity, replay, token-expiry, cleanup-liveness and no-redirect egress controls remain in force.
Current exact identity and evidence
046c1043b93ba2183b80edad441efc238f643fc7fecd911fc71fd7fe0517e74a1d6e25a71e9c655532560097543: in progress at the latest exact-head refetch32560097523: success32560097517: queuedMerge boundary
Keep Draft until application CI, reviewer-ci, and eligible central Security Scan are terminal-success on this unchanged exact head, review/thread state is freshly clean, protected
mainremains the live base, and central scanner authority is freshly revalidated.Summary by CodeRabbit