Clarify review app security guidance#350
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR updates two documentation files to enhance guidance around review-app deployments in public repositories. It establishes security constraints for fork PRs, revises secret token requirements, adds a comprehensive review-app security section, and clarifies safe secret storage using Control Plane references and scoped credentials. ChangesReview App Security for Public Repositories
Sequence Diagram(s)(Diagrams are embedded in the hidden review stack artifact above.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
added
documentation
Greptile SummaryThis PR adds security guidance for review apps in public repositories, explaining why fork PR heads are skipped during deploy, clarifying that trusted
Confidence Score: 4/5Documentation-only changes with accurate security guidance; safe to merge. All changes are documentation. The security claims are accurate and internally consistent, with one bullet point in the new security section using an "unless" clause that reads differently from the clearer prose elsewhere in the file — a reader could interpret it as an automatic workflow check rather than a manual workaround. The one bullet at line 209 of Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[PR opened or push] --> B{Is PR head from\nbase repository?}
B -- Yes\n(same-repo branch) --> C[Review app deploy eligible]
B -- No\n(fork head) --> D[Skip deploy\nPost help comment only]
C --> E{Trigger: push or\n+review-app-deploy comment?}
E -- Push --> F{Is review app\nalready active?}
F -- Yes --> G[Auto-redeploy]
F -- No --> H[Skip until first comment]
E -- Comment --> I{Is commenter\nOWNER / MEMBER /\nCOLLABORATOR?}
I -- No --> J[Ignore comment]
I -- Yes --> K[Create or update review app]
K --> L{Are secrets disposable?\nCPLN_TOKEN_STAGING\nnot prod-scoped?}
L -- Yes --> M[Safe review-app deploy]
L -- No --> N[⚠️ Credential exposure risk\nRotate and scope secrets]
D --> O[Maintainer workaround:\nmove change to base-repo branch\nthen open new PR]
O --> C
Reviews (1): Last reviewed commit: "docs: clarify review app security" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09f553d9a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
justin808
force-pushed
the
jg-codex/review-app-security-docs
branch
from
09f553d to
d144951
Compare
Address-review summaryScan scope: full PR history; no previous address-review summary cutoff was present. Mattered
Optional
Skipped
Next default scan starts after this comment. Say |
Review: Clarify review app security guidanceThis PR is a documentation-only addition that explains review-app security constraints for public/open-source repos — fork-PR limitations, scoped token guidance, 1. The new paragraph says the key is available to "allowed review-app Docker builds", but both generated workflows pass it:
A staging branch push from anyone with write access also receives the key during the Docker build. The caution should say "review-app and staging Docker builds" to avoid underselling the scope. 2. Same-repo auto-deploy trust boundary not documented (line 421) The security section says "same-repository pull requests can create or update review apps" without stating who qualifies. The generated workflow checks only that the PR head repo matches the base repo — there is no 3. "open-source repositories" / "public repositories" inconsistency (line 391) The new org-layout bullet uses "open-source repositories" — the only occurrence of this term in the entire file. Every other new addition (lines 13, 173, 413, 777) uses "public repositories". A maintainer of a public-but-proprietary repo could read "open-source" and conclude the scoped-token recommendation does not apply to them, even though the security concern is identical. Aligning to "public repositories" throughout is straightforward. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Address-review summaryScan scope: since previous summary at 2026-06-30T09:39:31Z. Mattered
Optional
Skipped
Next default scan starts after this comment. Say |
Review: Clarify review app security guidanceThis PR adds welcome security documentation for review apps in public repositories, particularly the fork-PR skipping behavior and the importance of disposable credentials. The two files are well-structured and the guidance is directionally sound. However, the review surfaced five confirmed factual issues — two of which are security-relevant documentation gaps — that should be fixed before merge. Confirmed Issues1.
Both vectors run untrusted PR code before human review, making them the highest-severity omission in an otherwise thorough security section. 2. "automatically on each push" is false for first-time deploys Line 421–422 states "same-repository pull requests from any contributor with push access can create or update review apps automatically on each push." This is only true for subsequent pushes to an already-existing review app. The workflow's "Skip auto deploy until a review app is created" step ( 3. "push access" is the wrong permission threshold The same bullet uses "push access" as the gate, but the workflow checks 4. Line 429 says the workflow "targets the staging/review Control Plane org and token from 5. "skipped" framing understates GitHub's platform-level enforcement Line 423 says "fork pull requests are skipped for review-app deploys," framing this as a workflow design choice. In practice the workflow uses the Minor / Structural
|
Address-review summaryScan scope: current-head unresolved review threads on PR #350 fetched on 2026-06-30T10:08Z; no Mattered
Optional
Skipped
Next default scan starts after this comment. Say |
|
Review: Clarify review app security guidance This is a docs-only PR adding security guidance for review apps on public repos. The framing is accurate and the new section fills a real gap. Three findings worth addressing before merge. CONFIRMED -- hooks.pre_deletion omitted from the attack-surface list (ci-automation.md lines 415-418) The docs warn about release_script and hooks.post_creation as PR-controllable attack surfaces, but omit hooks.pre_deletion. This hook also executes in the PR's container image: the nightly cleanup workflow runs cpflow cleanup-stale-apps which calls cpflow delete which invokes run_pre_deletion_hook via cpflow run --image latest (lib/command/base.rb). The hook command string comes from the base branch's controlplane.yml, but the execution environment is the PR's built Docker image. A PR that embeds malicious code in its Dockerfile or entrypoints will have that code run with staging CP credentials when nightly cleanup teardown fires -- even after the PR closes. Suggestion: extend the list at line 416-417 to include hooks.pre_deletion alongside hooks.post_creation. PLAUSIBLE -- Security section omits the auto-delete-on-close trigger (ci-automation.md line 426) Line 426 states that +review-app-delete is accepted only from trusted commenters. This is narrowly true for the comment command, but cpflow-delete-review-app.yml also fires on pull_request_target: closed with no commenter gate. Line 505 already correctly documents this, but the new security section omits it. A reader of the security section alone could incorrectly conclude that deletion is exclusively commenter-gated. Suggestion: add a note that review apps are also auto-deleted when the PR closes (no trusted-commenter check for that path). PLAUSIBLE -- Tightly scoped token guidance is not actionable (ci-automation.md lines 173, 391) The phrase tightly scoped token appears twice in new text without explaining that on Control Plane, scope is determined by the service account and RBAC policies -- not a property of the token itself. There is no link to Control Plane service account or policy docs, and no cpflow command to help. A user following this has no clear path to actually scoping CPLN_TOKEN_STAGING. Suggestion: add a note such as: create a Control Plane service account with a policy limited to review-app CI operations and generate a token for that account, with a link to Control Plane RBAC docs. |
Address-review summaryScan scope: current-head unresolved review threads on PR #350 fetched on 2026-06-30T10:26Z; no Mattered
Optional
Skipped
Next default scan starts after this comment. Say |
Review: Clarify review app security guidanceThis PR adds a detailed "Review app security for repositories with external contributors" section to Findings (most severe first)1. Semantic ambiguity: "public repositories" vs. "external contributors" used interchangeably 2. "Separate, disposable environment" path is mentioned but never defined (ci-automation.md line 18) 3. 4. Generated Workflow Behavior bullet omits manual dispatch as a first-review-app-creation path (ci-automation.md line 537) 5. Nightly cleanup bullet missing 6. Fork-guard re-explanation in 7. CHANGELOG.md missing entry 8. Manual dispatch fork-guard rationale omitted (ci-automation.md line 538–540) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f105673dc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review: Clarify review app security guidanceThis PR adds thorough security documentation for review-app deployments in public repositories — the coverage of fork-PR guards, token scoping, template trust, and credential hygiene is genuinely useful and technically grounded. Two factual inaccuracies and three gaps in the guidance survived verification and are worth addressing before merge. Confirmed bugs in the documentation1. Incorrect claim about 2. Plausible gaps3. 4. Injection field list anchors on two examples (lines 478–479) 5. Dispatch vs. comment permission level undifferentiated (lines 431–432) |
Review: Clarify review app security guidanceThis PR adds ~140 lines of documentation to Findings (most-severe first)1. Self-contradictory claim about Guard 1 and secret access ( 2. 3. 4. Template policy escalation vector not addressed in secrets doc ( 5. Heading case inconsistent with rest of file ( 6. Missing paragraph break in secrets doc ( 7. Missing CHANGELOG entry 8. Duplicate trusted-commenter bullet ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5696a62e2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Summary
+review-app-deploycomments still require a branch in the base repositorycpln://secretruntime exposureVerification
git diff --checkscript/check_cpln_linksscript/check_command_docsNote
Low Risk
Documentation-only; no runtime, workflow, or secret configuration changes.
Overview
Documentation-only updates to
docs/ci-automation.mdanddocs/secrets-and-env-values.mdthat spell out review-app security for public and open-source repos—no workflow or generator changes.ci-automation.mdnow states upfront that review deploys only run for base-repo branches (fork PRs get help but not deploys because builds use repo secrets), and that maintainers must land fork work on a trusted branch for a review app. It expandsCPLN_TOKEN_STAGINGguidance (prefer a review/staging token that cannot touch production),DOCKER_BUILD_SSH_KEYcautions (minimal read-only deploy keys), and a new Review App Security For Public Repositories section covering trusted commenters, fork skips, disposable credentials, scoped tokens, separate data stores, rotation, and cleanup. Workflow descriptions and the demo rollout checklist add matching bullets (including verifying staging tokens cannot reach production).secrets-and-env-values.mdadds that review apps execute PR code, socpln://secret/...does not protect values once mounted, and recommends dummy/disposable/revocable review credentials plus separate secret dictionaries from production or long-lived staging when orgs are shared.Reviewed by Cursor Bugbot for commit 09f553d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit