fix(security): isolate hourly readiness gh environment - #109
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGitHub CLI 하위 프로세스에 ChangesGitHub CLI 하위 프로세스 환경
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant runGh
participant createGhSubprocessEnvironment
participant ghCLI
runGh->>createGhSubprocessEnvironment: 환경 구성 요청
createGhSubprocessEnvironment-->>runGh: 제한된 환경 반환
runGh->>ghCLI: 제한된 환경으로 gh 실행
Possibly related issues
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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
test/hourly-commercial-readiness-script.test.ts (1)
246-273: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win고정된
GH_HOST와NO_COLOR의 덮어쓰기도 검증하십시오.현재 입력에는
GH_HOST와NO_COLOR의 공격자 값이 없습니다. 따라서 Line 262-Line 267의 기대값은 기본 상수가 존재하는지만 확인합니다. 입력에GH_HOST: "evil.example"과NO_COLOR: "0"을 추가하고, 결과가 계속github.com과"1"인지 확인하십시오.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.회귀 입력 보강 예시
NOEMA_MAINTENANCE_ENABLED: "true", + GH_HOST: "evil.example", + NO_COLOR: "0",🤖 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/hourly-commercial-readiness-script.test.ts` around lines 246 - 273, 보안 회귀 테스트의 입력에 공격자 제공 값인 GH_HOST: "evil.example"과 NO_COLOR: "0"을 추가하고, createGhSubprocessEnvironment의 결과가 여전히 GH_HOST를 "github.com", NO_COLOR를 "1"로 고정하는지 검증하십시오. 기존의 GH_TOKEN 전달 및 민감한 환경 변수 제외 검증은 그대로 유지하십시오.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 `@scripts/hourly-commercial-readiness.mjs`:
- Around line 32-44: Update createGhSubprocessEnvironment so GH_TOKEN is
supplied through an explicit argument populated by the KV or credential
registry, rather than read from sourceEnvironment or process.env. Keep
process.env usage limited to non-secret configuration such as PATH, and include
the provided token in the child environment only when valid.
In `@test/hourly-commercial-readiness-script.test.ts`:
- Around line 300-307: Update the test describing bounded report and post-action
queue outputs to explicitly assert that GH_TOKEN and its token value are not
present in the script’s report, follow-up queue, or error output paths. Preserve
the existing protocol and bounded-length assertions for open_pull_request_count,
remaining_open_pull_request_count, report_path, remainingOpenPullRequestCount,
and MAX_ERROR_CHARS.
---
Nitpick comments:
In `@test/hourly-commercial-readiness-script.test.ts`:
- Around line 246-273: 보안 회귀 테스트의 입력에 공격자 제공 값인 GH_HOST: "evil.example"과
NO_COLOR: "0"을 추가하고, createGhSubprocessEnvironment의 결과가 여전히 GH_HOST를
"github.com", NO_COLOR를 "1"로 고정하는지 검증하십시오. 기존의 GH_TOKEN 전달 및 민감한 환경 변수 제외 검증은
그대로 유지하십시오.
🪄 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: 38261d6d-09b7-4cfe-a447-8113ec7921b8
📒 Files selected for processing (2)
scripts/hourly-commercial-readiness.mjstest/hourly-commercial-readiness-script.test.ts
| export function createGhSubprocessEnvironment(sourceEnvironment = process.env) { | ||
| const childEnvironment = { | ||
| GH_HOST: "github.com", | ||
| NO_COLOR: "1", | ||
| }; | ||
| if (typeof sourceEnvironment.PATH === "string" && sourceEnvironment.PATH.length > 0) { | ||
| childEnvironment.PATH = sourceEnvironment.PATH; | ||
| } | ||
| if (typeof sourceEnvironment.GH_TOKEN === "string" && sourceEnvironment.GH_TOKEN.length > 0) { | ||
| childEnvironment.GH_TOKEN = sourceEnvironment.GH_TOKEN; | ||
| } | ||
| return childEnvironment; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
GH_TOKEN을 process.env에서 읽지 마십시오.
Line 32의 기본 인자와 Line 40의 접근은 process.env.GH_TOKEN을 직접 읽습니다. GH_TOKEN은 실제 자격 증명입니다. KV 또는 credential registry에서 토큰을 읽고, 명시적 입력으로 createGhSubprocessEnvironment에 전달하십시오. process.env를 기본 시크릿 소스로 사용하지 마십시오.
As per coding guidelines, scripts/*.mjs: Scripts may read process.env only for non-secret build-time configuration such as file paths and thresholds; real secrets must come from KV or a credential registry.
🤖 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 `@scripts/hourly-commercial-readiness.mjs` around lines 32 - 44, Update
createGhSubprocessEnvironment so GH_TOKEN is supplied through an explicit
argument populated by the KV or credential registry, rather than read from
sourceEnvironment or process.env. Keep process.env usage limited to non-secret
configuration such as PATH, and include the provided token in the child
environment only when valid.
Source: Coding guidelines
|
Superseded by fresh protected-main successor #225. Fresh pre-closure proof:
The unresolved credential-source policy finding is not declared fixed or discarded by this supersession; issue #111 remains its explicit governance/architecture owner and blocks any claim that the current source satisfies the KV-only policy. #225 owns only the current-line child-environment and diagnostic-redaction repair. No checks, reviews, scanner/model output, or merge authority transfer from this stale-base PR. |
1 participant
Purpose
Test-first repair for #108. The hourly commercial-readiness adapter let
spawnSync("gh", ...)inherit the complete parent environment. This branch makes GitHub CLI subprocess authority explicit instead of forwarding ambient workflow/model/provider/runner credentials and configuration.Exact source identity
mainatc85d710804139c0697d7ef8fa47d02b1389e6d84;fix/hourly-commercial-readiness-gh-env;e76d8be9023d5dfb344890cf7250e7f17ee65c04;05d6a0ae0f2c9d59d2557679c25db0ddbcdfa822.RCA -> RED -> GREEN
runGh()usedspawnSync("gh", ...)without anenvoption, so Node passed the complete ambient environment togh. That unnecessarily exposed unrelated workflow/model/provider credentials and runner configuration to a subprocess whose only intended authority is GitHub CLI access.The RED regression supplies a hostile synthetic parent containing
GITHUB_TOKEN, NVIDIA NIM, Maintainer/Reviewer App private material, Cloudflare credentials, proxy state,HOME,NODE_OPTIONS, reviewer configuration and unrelatedNOEMA_*state. It requires the child environment to contain only optional non-emptyPATH, explicitGH_TOKEN, pinnedGH_HOST=github.com, and deterministicNO_COLOR=1; an empty parent retains only the two pinned non-secret defaults. The source contract separately requiresrunGh()to pass that environment explicitly and forbidsenv: process.env.Application CI run
31436718995, job93612364591, checked out exact RED heade76d8be9023d5dfb344890cf7250e7f17ee65c04and failed exactly the new contract:createGhSubprocessEnvironmentwas absent andrunGh()did not pass the explicit environment. The rest of the suite passed.GREEN
05d6a0ae0f2c9d59d2557679c25db0ddbcdfa822exports the narrow child-environment builder and passes its result tospawnSync, preservingshell: false, bounded output, pagination, exact-head/live-base validation and all existing readiness/governance decisions.Current exact-head proof
For unchanged head
05d6a0ae0f2c9d59d2557679c25db0ddbcdfa822:cirun31436885124: terminal success; verify job93612862199checked out this exact SHA, reported 62/62 test files and 647/647 tests passing, and exposed 100% statements / 100% branches / 100% functions / 100% lines for the owned production coverage set;reviewer-cirun31436885115: terminal success;Security Scanrun31436885127: terminal success under live central workflow revision6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba/security-scan.ymlblobc3b8fa5dbbb21a684209683a3d4b6d29c8e0d536;main,master, ordevelop; OSV and dependency review are diff-scoped hard gates, fixable MEDIUM/HIGH/CRITICAL Trivy findings are repo-wide hard failures, and Scorecard is soft posture evidence;COMMENTED, not qualifying independent approval.Boundary
This change narrows only the hourly readiness adapter's
ghchild-process environment. It does not change workflow permissions, reviewer/maintainer authority, merge/release/deployment authority, branch/ruleset policy, central.githubworkflows, KPI evidence, licensing posture, or acquisition claims.Technical exact-head success is not merge authority. Any live qualifying independent-review requirement remains fail-closed; no self-approval, synthetic status, gate weakening, release or deployment is performed.
Related: #27, #29, #80, #108
Summary by CodeRabbit
개선 사항
보안 및 안정성