fix(review): restack private target auth after nanoid - #92
Open
seonghobae wants to merge 5 commits into
Open
Conversation
📝 WalkthroughWalkthrough중앙 reviewer 워크플로를 초기 입력 검증과 live-head 검증으로 분리했습니다. 대상 저장소 범위의 읽기 전용 Noema GitHub App 토큰으로 PR 상태와 저장소 정보를 확인합니다. 관련 문서와 테스트도 추가했습니다. Changes대상 PR 인증 흐름
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CentralReview
participant NoemaGitHubApp
participant GitHubAPI
participant TrustedCheckout
CentralReview->>NoemaGitHubApp: 저장소 범위 읽기 전용 토큰 발급
NoemaGitHubApp-->>CentralReview: App 토큰 반환
CentralReview->>GitHubAPI: PR 상태와 head/base 정보 조회
GitHubAPI-->>CentralReview: live PR 정보 반환
CentralReview->>CentralReview: dispatch 입력과 exact-head 검증
CentralReview->>TrustedCheckout: 검증된 head 체크아웃
Possibly related PRs
🚥 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 |
This was referenced Aug 10, 2026
seonghobae
marked this pull request as ready for review
August 10, 2026 09:21
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/central-review.yml (1)
43-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
EXPECTED_HEAD_SHA의 대소문자 정책을 일관되게 적용하십시오.Line 45는 대문자 SHA를 허용합니다. GitHub API의
.head.sha는 소문자 형식이므로, 같은 객체 ID를 대문자로 전달하면 Line 80에서 stale 또는 untrusted 입력으로 잘못 거부됩니다. 모든 소비 지점에서 SHA를 정규화하거나, 검증 정규식을^[0-9a-f]{40}$로 제한하십시오. 대문자 입력의 거부 또는 정규화를 확인하는 회귀 테스트도 추가하십시오.As per coding guidelines, “Prefer adding regression tests over relaxing security, audit, KPI, or readiness checks.”
🤖 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 @.github/workflows/central-review.yml around lines 43 - 50, Update EXPECTED_HEAD_SHA validation in the workflow to accept only the lowercase 40-character hexadecimal format used by GitHub’s head.sha, rather than normalizing or relaxing downstream checks. Ensure all subsequent SHA consumers, including the stale/untrusted validation path, use this same policy. Add a regression test confirming uppercase SHA input is rejected.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.
Inline comments:
In `@docs/doctoring/private-target-review-auth.md`:
- Around line 17-20: Update target validation in the central review workflow so
head SHA casing cannot cause false stale-head failures: either restrict the
validation pattern to lowercase hexadecimal or normalize the validated SHA once
and consistently reuse that lowercase value for live-head comparison, step
outputs, and checkout.
In `@reviewer/tests/test_central_review_workflow.py`:
- Around line 24-47: Strengthen the central workflow security test around the
mint_step and bind_step assertions: parse permission-* entries and require the
exact approved read-only allowlist, rejecting any additional keys or write
values. Explicitly assert bind_step compares live head SHA, head repository, and
base repository fields, and checks out using steps.target.outputs.head_sha,
while retaining the existing app-token and API-call checks.
---
Outside diff comments:
In @.github/workflows/central-review.yml:
- Around line 43-50: Update EXPECTED_HEAD_SHA validation in the workflow to
accept only the lowercase 40-character hexadecimal format used by GitHub’s
head.sha, rather than normalizing or relaxing downstream checks. Ensure all
subsequent SHA consumers, including the stale/untrusted validation path, use
this same policy. Add a regression test confirming uppercase SHA input is
rejected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9baea58a-456c-4c5b-9319-f8e6896a9a82
📒 Files selected for processing (4)
.github/workflows/central-review.ymlCHANGELOG.mddocs/doctoring/private-target-review-auth.mdreviewer/tests/test_central_review_workflow.py
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Clean protected-main successor to stale Draft #85 after #76 integrated. This branch starts from protected
mainc85d710804139c0697d7ef8fa47d02b1389e6d84and carries only the security-relevant private-target reviewer bootstrap, deterministic regression coverage, APA 7 doctoring, and the bounded current-protected-CHANGELOG entry.Exact source boundary
Current exact head:
399e47fe1550ac4b58a02f9dc10ec610e5ab05e3.The PR remains a four-path bounded change:
.github/workflows/central-review.yml— first live target PR read occurs only after the existing target-repository-scoped read-only Noema App token is minted, and the dispatched exact head must be canonical lowercase 40-hex;reviewer/tests/test_central_review_workflow.py— requires syntax validation → exact read-only permission allowlist → authenticated live state/head/head-repository/base-repository binding → exact target checkout fromsteps.target.outputs.head_shaand rejects${{ github.token }}for the live target read;docs/doctoring/private-target-review-auth.md— Proposed security/interoperability decision, canonical SHA identity, failure/recovery behavior and GitHub primary-source references;CHANGELOG.md— bounded## Unreleasedentry from the protected post-fix(security): update transitive nanoid for CVE-2026-67213 #76 changelog.No predecessor #85 check/review/model evidence transfers.
Root cause and test-first repair
The workflow runs in
ContextualWisdomLab/noemabut can review another repository. The original first target PR lookup used workflow-repository${{ github.token }}before minting the target-scoped App token. Public target metadata can mask that authority mismatch; private target repositories cannot rely on it.Fresh CodeRabbit review then exposed two current defects in the clean successor:
.head.shacomparison is exact lowercase, creating a false stale-head path for equivalent casing;RED commit
1d3256f5ae7d6c40344618f8ea388b3db028cc5dstrengthened the reviewer contract and failed exactly on the uppercase-accepting workflow: reviewer-ci ran 121 tests, reported 1 failed / 120 passed, with reviewer production coverage still 100%. GREEN commitsdac96293e74df4322ef7aac11e6254419a4262c1and399e47fe1550ac4b58a02f9dc10ec610e5ab05e3restricted the workflow to^[0-9a-f]{40}$and aligned the Proposed doctoring contract.Fresh exact-head evidence
For unchanged exact head
399e47fe1550ac4b58a02f9dc10ec610e5ab05e3on protected basec85d710804139c0697d7ef8fa47d02b1389e6d84:cirun31376257360: terminal success;reviewer-cirun31376256712: terminal success;Security Scanrun31376256657: terminal success;The CodeRabbit review is
COMMENTEDevidence, not a qualifying formal independent approval. Current technical success also does not prove the live ruleset or private-target operational exercise.Ready / merge / operational boundary
The implementation is technically Ready. Merge remains prohibited until actual live Noema/CWL governance and any qualifying independent non-author formal approval requirement are satisfied on the unchanged head. Issue #27 owns enforceable ruleset/protection evidence and #29 owns an eligible Reviewer/Maintainer App route.
After protected integration, a real dispatch against a private ContextualWisdomLab repository on which the Noema App is installed must prove exact-head evidence collection without permission broadening; that protected-main exercise is operational acceptance, not source merge authority.
No repair/self-modifying workflow, protection weakening, synthetic approval, force push, version bump or release is introduced.
Related: #27, #29, #85