Skip to content

fix(#7): allow POST git-upload-pack in default GitHub provider #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ghost wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(#7): allow POST git-upload-pack in default GitHub provider #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion crates/openshell-providers/src/profiles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1672,12 +1672,29 @@ mod tests {
}),
"github profile should include read-only GraphQL endpoint"
);
// api.github.com endpoints use access: read-only.
// github.com uses explicit rules (no access preset) to allow
// POST git-upload-pack for clone/fetch while blocking push.
assert!(
proto
.endpoints
.iter()
.filter(|endpoint| endpoint.host == "api.github.com")
.all(|endpoint| endpoint.access == "read-only"),
"github profile endpoints should all be read-only"
"api.github.com endpoints should be read-only"
);
let git_endpoint = proto
.endpoints
.iter()
.find(|endpoint| endpoint.host == "github.com")
.expect("github.com endpoint");
assert!(
git_endpoint.access.is_empty(),
"github.com should use explicit rules, not an access preset"
);
assert!(

@ghost ghost Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] test-inadequate

The new test assertions check .access.is_empty() and !.rules.is_empty(), but do not validate the actual content of the rules array (specific methods, paths).

Suggested fix: Add assertions that validate at least one critical rule (e.g., the POST git-upload-pack rule) to ensure the rules contain expected values.

!git_endpoint.rules.is_empty(),
"github.com should have explicit L7 rules"
);
assert_eq!(proto.binaries.len(), 4);
}
Expand Down
20 changes: 19 additions & 1 deletion providers/github.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,27 @@ endpoints:
access: read-only
enforcement: enforce
# github.com is the git transport (clone / fetch by default).
# Explicit rules instead of `access: read-only` so that POST to
# git-upload-pack (clone/fetch) is allowed while git-receive-pack

@ghost ghost Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] design-smell

The github.com endpoint uses explicit L7 rules while api.github.com endpoints use the read-only access preset, creating inconsistent policy expression patterns within a single provider profile. The inconsistency is inherent and documented by the inline comment.

# (push) stays blocked.
- host: github.com
port: 443
protocol: rest
access: read-only
enforcement: enforce
rules:

@ghost ghost Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] yaml_structure

The rules array is the first use of explicit L7 rules in any provider YAML file in the providers/ directory. This introduces a new pattern without similar examples for consistency.

- allow:
method: GET
path: "**"

@ghost ghost Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[info] permission-expansion

The github.com:443 endpoint is upgraded from access: read-only to explicit rules that additionally allow POST on **/git-upload-pack and **/info/refs. The expansion is justified by issue #7 and follows least-privilege for the stated use case.

- allow:
method: HEAD
path: "**"
- allow:
method: OPTIONS
path: "**"
- allow:

@ghost ghost Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] edge-case

The rule { method: POST, path: **/info/refs } allows POST requests to /info/refs. In Git smart HTTP protocol, /info/refs is only accessed via GET. POST is used only for /git-upload-pack and /git-receive-pack. Since the GET ** rule already covers GET /info/refs, this POST rule is unnecessary.

Suggested fix: Change the method from POST to GET, or remove this rule entirely since GET ** already covers it.

method: POST

@ghost ghost Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[info] authorization

POST to **/info/refs is not part of the standard Git smart HTTP protocol (which uses GET for ref discovery). The practical security impact is negligible given the default-deny policy.

path: "**/git-upload-pack"
- allow:
method: POST
path: "**/info/refs"
binaries: [/usr/bin/gh, /usr/local/bin/gh, /usr/bin/git, /usr/local/bin/git]
Loading