fix(policy): add protocol/enforcement/tls to GitHub endpoints#1225
fix(policy): add protocol/enforcement/tls to GitHub endpoints#1225latenighthackathon wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGitHub endpoints in Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent
participant Proxy as OpenShell L7 Proxy
participant GitAPI as api.github.com:443
participant GitHost as github.com:443
Agent->>Proxy: HTTPS request (method + path)
Proxy-->>Proxy: Terminate TLS & inspect (protocol: rest, enforcement: enforce)
alt request to api.github.com and allowed by rules
Proxy->>GitAPI: Forward request
GitAPI-->>Proxy: Response
Proxy->>Agent: Return response
else request to github.com and allowed by rules
Proxy->>GitHost: Forward request
GitHost-->>Proxy: Response
Proxy->>Agent: Return response
else denied by L7 rules
Proxy->>Agent: Reject (403)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
Comment |
|
@latenighthackathon Thanks for working on this. I agree with the core direction here: #1111 is real, and moving the GitHub endpoints off access: full toward L7-enforced REST policies is the right goal. I’m not comfortable merging this branch as written yet. The main issue is that this changes the default GitHub policy behavior without validating that the narrowed method set still supports the expected gh and git workflows.
If you want to get this re-reviewable, I’d suggest one of two paths:
If you respin it with that extra validation or tighter scoping, I’d be happy to take another look. |
wscurran
added
security
✨ Thanks for submitting this pull request, which proposes a way to improve the security of NemoClaw by adding protocol and enforcement rules for GitHub endpoints.Possibly related open issues:
|
wscurran
added
Provider: GitHub
Integration: GitHub
|
@kjw3 Thanks for the detailed feedback — you're right that the original GET/POST shape was too narrow for I've updated the PR to split the two hosts with different method scoping:
The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw-blueprint/policies/openclaw-sandbox.yaml`:
- Around line 110-125: The current api.github.com policy rules permit PATCH and
PUT on "/**" (see the rules block under host: api.github.com), which is overly
broad; replace those wildcard write rules with path-scoped entries that list
only the necessary mutable endpoints (for example restrict PATCH to specific
issue/PR endpoints like /repos/*/*/issues/* and /repos/*/*/pulls/* and restrict
PUT to allowed merge endpoints such as /repos/*/*/pulls/*/merge and any needed
review submission paths), keeping GET/POST as-is and ensuring no generic
PATCH/PUT "/**" remains so the policy enforces least privilege.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69cad4cd-bf51-4c1e-92b3-ad0fb120c2d3
📒 Files selected for processing (1)
nemoclaw-blueprint/policies/openclaw-sandbox.yaml
|
@kjw3 @coderabbitai Addressed the over-broad write scope feedback. Changes:
DELETE remains excluded. PATCH/PUT no longer wildcard — scoped to the minimum set of mutable endpoints an AI agent needs for standard GitHub workflows. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll look at the current policy file and verify the GitHub API path patterns before responding. 🏁 Script executed: Length of output: 7516 🌐 Web query: 💡 Result: Below are common “agent” workflows on GitHub and the REST API method + path you typically call (plus the matching Using
|
| Operation | Required rule | Currently covered? |
|---|---|---|
| Create feature branch | POST /repos/*/*/git/refs |
❌ No |
| Move/update a branch ref | PATCH /repos/*/*/git/refs/* |
❌ No |
| Create/update file via API | PUT /repos/*/*/contents/** |
❌ No |
gh pr create will silently fail if it can't first create a branch via POST /repos/{owner}/{repo}/git/refs. Similarly, any agent using the contents API to commit file changes (rather than raw git push) needs the PUT /contents/** path. The double-star (**) matters here because file paths are multi-segment (e.g., /repos/owner/repo/contents/src/lib/foo.ts).
Suggested additions for api.github.com:
- allow: { method: POST, path: "/repos/*/*/git/refs" }
- allow: { method: PATCH, path: "/repos/*/*/git/refs/*" }
- allow: { method: PUT, path: "/repos/*/*/contents/**" }2. ⚠️ Single-segment * doesn't cover sub-paths
PATCH /repos/*/*/pulls/* matches /repos/owner/repo/pulls/123 correctly, but not sub-paths like /repos/owner/repo/pulls/123/requested_reviewers. Requesting reviewers is a POST to that sub-path — which falls under your existing POST /repos/*/*/pulls/*/reviews... actually no, reviewer requests go to POST /repos/*/*/pulls/*/requested_reviewers, which is not listed. If reviewer assignment is needed, add:
- allow: { method: POST, path: "/repos/*/*/pulls/*/requested_reviewers" }3. ℹ️ npm_registry still uses access: full
Issue #1111 specifically called out converting npm_registry to protocol: rest with GET-only. It's still using access: full in the current file — worth a follow-up issue or a small companion change here.
4. ✅ What looks good
git.832008.xyzgit transport scoping is correct —/**/git-upload-packand/**/git-receive-packhandle repo-namespaced paths cleanly.DELETEexclusion by omission is the right approach.- GET
/**onapi.github.comprovides unrestricted reads without needing to enumerate every GET path. - The
tls: terminate+enforcement: enforcecombination aligns with all other REST endpoints in this file.
The branch/ref and contents gaps are the ones most likely to break a standard gh pr create → push → merge loop in practice. Everything else is a refinement.
|
@coderabbitai Good catches — pushed a follow-up addressing all three gaps: Added:
Also organized rules by workflow category (PR, issues, git ops) for readability. Re: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the current state of the policy file before responding. 🏁 Script executed: Length of output: 7935
🚨
|
|
@coderabbitai Good catch — pushed fix. Re: npm_registry follow-up — yes, please go ahead and file the issue. Thanks! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 53 🏁 Script executed: Length of output: 46
Filed the npm_registry follow-up: #1355 — tracked against this PR and With that, all the items raised in this review are addressed:
This should be in good shape for |
latenighthackathon
force-pushed
the
fix/github-policy-protocol
branch
from
86e7f38 to
3bb9d17
Compare
latenighthackathon
force-pushed
the
fix/github-policy-protocol
branch
from
a8779de to
38f44e6
Compare
|
@kjw3 Thanks for the patience on this one — I've taken another pass addressing both of your asks. 1. Scoping (per #1111): Split
2. Workflow validation: Updated the PR body with a full mapping table — each common Also rebased onto current main while I was in there — clean linear history now. Happy to take another round if you see anything missed. |
…1358) ## Summary - Replace L4 pass-through (`access: full`) on `npm_registry` endpoint with L7-enforced REST policy restricted to `GET /**` - Agents only fetch packages from the npm registry and never publish, so write methods are not needed - Enables per-request rule evaluation, per-request logging, and SecretResolver credential injection ## Motivation - Issue #1111 called out converting `npm_registry` alongside the GitHub endpoints - Discussed and agreed upon during review of PR #1225 but deferred to keep that PR scoped - CodeRabbit filed #1355 to track this follow-up ## Test plan - [ ] Verify `openclaw plugins install` succeeds with GET-only policy - [ ] Verify `npm install` inside sandbox completes normally - [ ] Confirm write attempts (e.g. `npm publish`) are blocked by the policy Closes #1355 Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Security** * Registry access is now read-only (HTTP GET only) and TLS termination is enforced for registry connections, tightening network access and transport security for package registry interactions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
latenighthackathon
force-pushed
the
fix/github-policy-protocol
branch
from
3106014 to
334be03
Compare
…VIDIA#1358) ## Summary - Replace L4 pass-through (`access: full`) on `npm_registry` endpoint with L7-enforced REST policy restricted to `GET /**` - Agents only fetch packages from the npm registry and never publish, so write methods are not needed - Enables per-request rule evaluation, per-request logging, and SecretResolver credential injection ## Motivation - Issue NVIDIA#1111 called out converting `npm_registry` alongside the GitHub endpoints - Discussed and agreed upon during review of PR NVIDIA#1225 but deferred to keep that PR scoped - CodeRabbit filed NVIDIA#1355 to track this follow-up ## Test plan - [ ] Verify `openclaw plugins install` succeeds with GET-only policy - [ ] Verify `npm install` inside sandbox completes normally - [ ] Confirm write attempts (e.g. `npm publish`) are blocked by the policy Closes NVIDIA#1355 Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Security** * Registry access is now read-only (HTTP GET only) and TLS termination is enforced for registry connections, tightening network access and transport security for package registry interactions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
wscurran
added
status: rebase
Replace access: full with scoped L7 rules in the github preset (moved from base policy to presets/github.yaml by NVIDIA#1583). github.com gets GET + POST scoped to git-upload-pack / git-receive-pack. api.github.com gets GET + PR/issue/refs/contents writes; DELETE excluded to block destructive ops. Closes NVIDIA#1111 Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
latenighthackathon
force-pushed
the
fix/github-policy-protocol
branch
from
05f1abc to
cc197a7
Compare
|
@wscurran Thanks for the kind words on the hardening approach. Rebased onto latest Cheers! |
|
Great work on this — the move from One thing I've been thinking about: what happens when an agent inside the sandbox needs to create a release, fork a repo, or manage labels? Those are pretty common in day-to-day workflows, and they'd silently fail under the current allowlist since only GET covers them. Could you take a look at adding scoped rules for these? The same pattern you're already using works perfectly: # Releases
- allow: { method: POST, path: "/repos/*/*/releases" }
- allow: { method: PATCH, path: "/repos/*/*/releases/*" }
# Forks
- allow: { method: POST, path: "/repos/*/*/forks" }
# Labels
- allow: { method: POST, path: "/repos/*/*/labels" }
- allow: { method: PATCH, path: "/repos/*/*/labels/*" }
# Milestones
- allow: { method: POST, path: "/repos/*/*/milestones" }
- allow: { method: PATCH, path: "/repos/*/*/milestones/*" }
# PR update-branch
- allow: { method: PUT, path: "/repos/*/*/pulls/*/update-branch" }The nice thing is this doesn't change the security posture you've built — still L7, still no DELETE, still path-scoped. It just makes the allowlist complete enough that agents won't hit unexpected denials during normal work. What do you think — does that feel right, or did you intentionally leave some of these out? |
wscurran
removed
the
status: rebase
5 participants
Summary
access: fullongit.832008.xyzandapi.github.comwithprotocol: rest,enforcement: enforce,tls: terminate, and explicitly scoped method/path rulesRelated Issue
Closes #1111
Changes
The GitHub policy used
access: fullwithoutprotocol: rest, which means the proxy treats connections as L4-only — method/path rules cannot be evaluated, per-request logging is unavailable, and credential injection via SecretResolver won't work.This fix adds the L7 inspection fields that every other REST endpoint in the policy already has, and replaces the
access: fullshorthand with explicit rules scoped to PR/issue/git workflows.github.com — git transport only
GET /**— ref discovery (git clone,git ls-remote)POST /**/git-upload-pack— clone/fetch handshakePOST /**/git-receive-pack— push handshakeapi.github.com — REST API
GET /**— unrestricted readsPOST /repos/*/*/actions/workflows/*/dispatches— manual workflow dispatch (gh workflow run)Testing — workflow validation
Each common
ghandgitworkflow was mapped against the rule set to confirm coverage. All standard agent operations are covered; destructive operations are explicitly blocked.git transport (github.com)
git clone(ref discovery)GET /{o}/{r}.git/info/refsGET /**git clone(handshake)POST /{o}/{r}.git/git-upload-packPOST /**/git-upload-packgit fetch/git pullGET /info/refs+POST /git-upload-packgit pushGET /info/refs+POST /git-receive-packgit ls-remoteGET /info/refsGET /**gh CLI / REST API (api.github.com)
gh auth status,gh api userGET /userGET /**gh repo viewGET /repos/{o}/{r}GET /**gh issue list/viewGET /repos/{o}/{r}/issues[/{n}]GET /**gh issue createPOST /repos/{o}/{r}/issuesPOST /repos/*/*/issuesgh issue edit/gh issue closePATCH /repos/{o}/{r}/issues/{n}PATCH /repos/*/*/issues/*gh issue commentPOST /repos/{o}/{r}/issues/{n}/commentsPOST /repos/*/*/issues/*/commentsgh pr list/viewGET /repos/{o}/{r}/pulls[/{n}]GET /**gh pr create(branch)POST /repos/{o}/{r}/git/refsPOST /repos/*/*/git/refsgh pr create(PR)POST /repos/{o}/{r}/pullsPOST /repos/*/*/pullsgh pr edit/gh pr closePATCH /repos/{o}/{r}/pulls/{n}PATCH /repos/*/*/pulls/*gh pr commentPOST /repos/{o}/{r}/issues/{n}/comments(shared with issues)POST /repos/*/*/issues/*/commentsgh pr review --approve/--request-changesPOST /repos/{o}/{r}/pulls/{n}/reviewsPOST /repos/*/*/pulls/*/reviewsgh pr review --request <user>POST /repos/{o}/{r}/pulls/{n}/requested_reviewersPOST /repos/*/*/pulls/*/requested_reviewersgh pr mergePUT /repos/{o}/{r}/pulls/{n}/mergePUT /repos/*/*/pulls/*/mergegh release list/viewGET /repos/{o}/{r}/releasesGET /**gh run list/viewGET /repos/{o}/{r}/actions/runsGET /**gh workflow runPOST /repos/{o}/{r}/actions/workflows/{id}/dispatchesPOST /repos/*/*/actions/workflows/*/dispatchesPUT /repos/{o}/{r}/contents/{path}PUT /repos/*/*/contents/**PATCH /repos/{o}/{r}/git/refs/heads/{branch}PATCH /repos/*/*/git/refs/**Intentionally blocked (destructive / out-of-scope)
gh repo deleteDELETE /repos/{o}/{r}gh release deleteDELETE /repos/{o}/{r}/releases/{id}DELETE /repos/{o}/{r}/git/refs/heads/{b}gh repo createPOST /user/reposPUT /repos/{o}/{r}/branches/{b}/protectionChecklist
Summary by CodeRabbit