From 53426707e544b22df9be1a9ef8fc816551e371bd Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Tue, 1 Sep 2026 07:13:15 -0500 Subject: [PATCH] fix(gl): sign the visibility-gated client reads so a repo owner can read their own private repo (#115) `gl pr list/view/diff/comments` and `gl repo commits` loaded the caller's keypair, used it only to derive the owner string, then built NodeClient with None and sent an unsigned GET. Those routes are gated by authorize_repo_read, which denies an anonymous caller with a 404, so a private repo's own owner was told the repo had no pull requests and no commits. The six MCP read arms had the same shape while already holding the keypair. pr.rs now passes Some(keypair) at the four sites and calls get_maybe_signed. repo.rs cmd_commits builds the client with load_keypair_from_dir(..).ok() so `owner/name` against a public repo still works with no identity on disk. mcp.rs switches repo_get, repo_commits, pr_list, pr_view (both fetches) and pr_diff to get_maybe_signed. This is the pattern PR #113 established for the subset of routes it gated. Tests: the existing happy-path mocks in pr.rs and repo.rs now assert the signature and signature-input headers, following the protect.rs pattern; cmd_diff gets its first test; a new test pins that the anonymous public-repo path stays unsigned. Reverting the production hunks turns 7 tests red. Full gl suite 365 passed, fmt and clippy clean. DO NOT MERGE BEFORE #186. Review found two ordering hazards, both verified by execution. First, this regresses public-repo reads on its own. These call sites sent no signature before, so the node's require_signature was unreachable for them. Now that they always sign, a client whose clock is more than 300s off gets a 400 clock_skew, and since no changed call site checks the response status, that renders as an empty list with exit 0. A public repo the user could always read reports itself empty. #186 adds the status check these lines need underneath them, which turns that failure into an error the user can act on. Second, #186 rewrites these same expressions and its replacement keeps `client.get(`, so if #186 merges after this branch it silently reverts the signing. Land #186 first and rebase this on top. Known gaps left for the follow-up, none of them fixed here: the MCP arms have no signing test and reverting all six leaves the suite green; MCP owner/repo arguments are interpolated into the path unvalidated, so a signed request can be aimed at another owner-gated route; the signed path is not percent-encoded, so a non-ASCII branch or a repo argument containing a space signs a string the node cannot reproduce; `.ok()` cannot tell a missing identity from an unreadable one; and repo_list, repo_tree, git_refs and gl status remain unsigned against the same class of route. Refs #115 --- crates/gl/src/mcp.rs | 14 +++-- crates/gl/src/pr.rs | 136 ++++++++++++++++-------------------------- crates/gl/src/repo.rs | 44 +++++++++++++- 3 files changed, 100 insertions(+), 94 deletions(-) diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index e9849ce7e..36390c6e1 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -696,7 +696,9 @@ async fn call_tool( let name = args["name"].as_str().context("missing 'name'")?; let owner = resolve_owner(&args, &client).await?; let repo = crate::http::read_json( - client.get(&format!("/api/v1/repos/{owner}/{name}")).await?, + client + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{name}")) + .await?, "repo", ) .await?; @@ -708,7 +710,7 @@ async fn call_tool( let owner = resolve_owner(&args, &client).await?; let commits = crate::http::read_json( client - .get(&format!("/api/v1/repos/{owner}/{name}/commits")) + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{name}/commits")) .await?, "commits", ) @@ -849,7 +851,7 @@ async fn call_tool( let owner = resolve_owner(&args, &client).await?; let resp = crate::http::read_json( client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{repo}/pulls")) .await?, "pull requests", ) @@ -863,14 +865,14 @@ async fn call_tool( let owner = resolve_owner(&args, &client).await?; let pr = crate::http::read_json( client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) .await?, "pull request", ) .await?; let reviews = crate::http::read_json( client - .get(&format!( + .get_maybe_signed(&format!( "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" )) .await?, @@ -888,7 +890,7 @@ async fn call_tool( let owner = resolve_owner(&args, &client).await?; let resp = crate::http::read_json( client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) .await?, "diff", ) diff --git a/crates/gl/src/pr.rs b/crates/gl/src/pr.rs index 2d3e88ca9..990a1690f 100644 --- a/crates/gl/src/pr.rs +++ b/crates/gl/src/pr.rs @@ -245,11 +245,11 @@ async fn cmd_create( async fn cmd_list(repo: String, node: String, dir: Option) -> Result<()> { let keypair = load_keypair_from_dir(dir.as_deref())?; let owner = resolve_owner(&keypair); - let client = NodeClient::new(&node, None); + let client = NodeClient::new(&node, Some(keypair)); let resp = crate::http::read_json( client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{repo}/pulls")) .await?, "pull requests", ) @@ -290,11 +290,11 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() async fn cmd_view(repo: String, number: u64, node: String, dir: Option) -> Result<()> { let keypair = load_keypair_from_dir(dir.as_deref())?; let owner = resolve_owner(&keypair); - let client = NodeClient::new(&node, None); + let client = NodeClient::new(&node, Some(keypair)); let pr = crate::http::read_json( client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) .await?, "pull request", ) @@ -318,7 +318,7 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) // Show reviews let reviews = crate::http::read_json( client - .get(&format!( + .get_maybe_signed(&format!( "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" )) .await?, @@ -352,7 +352,7 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) // Show comments let comments = crate::http::read_json( client - .get(&format!( + .get_maybe_signed(&format!( "/api/v1/repos/{owner}/{repo}/pulls/{number}/comments" )) .await?, @@ -381,11 +381,11 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) async fn cmd_diff(repo: String, number: u64, node: String, dir: Option) -> Result<()> { let keypair = load_keypair_from_dir(dir.as_deref())?; let owner = resolve_owner(&keypair); - let client = NodeClient::new(&node, None); + let client = NodeClient::new(&node, Some(keypair)); let resp = crate::http::read_json( client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) + .get_maybe_signed(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) .await?, "diff", ) @@ -484,11 +484,11 @@ async fn cmd_comment( async fn cmd_comments(repo: String, number: u64, node: String, dir: Option) -> Result<()> { let keypair = load_keypair_from_dir(dir.as_deref())?; let owner = resolve_owner(&keypair); - let client = NodeClient::new(&node, None); + let client = NodeClient::new(&node, Some(keypair)); let resp = crate::http::read_json( client - .get(&format!( + .get_maybe_signed(&format!( "/api/v1/repos/{owner}/{repo}/pulls/{number}/comments" )) .await?, @@ -549,6 +549,9 @@ mod tests { "GET", mockito::Matcher::Regex(r"^/api/v1/repos/[^/]+/myrepo/pulls$".to_string()), ) + // An identity is supplied, so get_maybe_signed must sign the request. + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"pulls":[]}"#) @@ -575,6 +578,9 @@ mod tests { "GET", mockito::Matcher::Regex(r"^/api/v1/repos/[^/]+/myrepo/pulls$".to_string()), ) + // An identity is supplied, so get_maybe_signed must sign the request. + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"pulls":[{"number":1,"title":"Add feature","status":"open","source_branch":"feat","target_branch":"main","author_did":"did:key:z6MkTest"}]}"#) @@ -668,6 +674,9 @@ mod tests { "GET", mockito::Matcher::Regex(r"^/api/v1/repos/[^/]+/myrepo/pulls/1$".to_string()), ) + // An identity is supplied, so get_maybe_signed must sign the request. + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"number":1,"title":"Fix it","status":"open","source_branch":"fix","target_branch":"main","author_did":"did:key:z6MkTest","body":"some body"}"#) @@ -680,6 +689,9 @@ mod tests { r"^/api/v1/repos/[^/]+/myrepo/pulls/1/reviews$".to_string(), ), ) + // An identity is supplied, so get_maybe_signed must sign the request. + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"reviews":[]}"#) @@ -692,6 +704,9 @@ mod tests { r"^/api/v1/repos/[^/]+/myrepo/pulls/1/comments$".to_string(), ), ) + // An identity is supplied, so get_maybe_signed must sign the request. + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"comments":[]}"#) @@ -783,6 +798,9 @@ mod tests { r"^/api/v1/repos/[^/]+/myrepo/pulls/1/comments$".to_string(), ), ) + // An identity is supplied, so get_maybe_signed must sign the request. + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"comments":[]}"#) @@ -827,76 +845,22 @@ mod tests { } #[tokio::test] - async fn test_cmd_merge_success() { + async fn cmd_diff_surfaces_denial_not_empty() { let dir = TempDir::new().unwrap(); write_identity(&dir); - let mut server = mockito::Server::new_async().await; let _m = server .mock( - "POST", - mockito::Matcher::Regex(r"^/api/v1/repos/[^/]+/myrepo/pulls/2/merge$".to_string()), + "GET", + mockito::Matcher::Regex(r"/pulls/1/diff$".to_string()), ) - .with_status(200) - .with_header("content-type", "application/json") - .with_body(r#"{"merge_sha":"abc123def456789"}"#) - .create_async() - .await; - - cmd_merge( - "myrepo".to_string(), - 2, - server.url(), - Some(dir.path().to_path_buf()), - ) - .await - .unwrap(); - } - - // ── Gated PR reads surface node denials, not empty/stub renders (#123) ── - - #[tokio::test] - async fn cmd_list_surfaces_denial_not_empty() { - let dir = TempDir::new().unwrap(); - write_identity(&dir); - let mut server = mockito::Server::new_async().await; - let _m = server - .mock("GET", mockito::Matcher::Regex(r"/pulls$".to_string())) - .with_status(404) - .with_header("content-type", "application/json") - .with_body(r#"{"message":"repository not found"}"#) - .expect(1) - .create_async() - .await; - let result = cmd_list( - "myrepo".to_string(), - server.url(), - Some(dir.path().to_path_buf()), - ) - .await; - assert!( - result.is_err(), - "cmd_list must Err on 404, not print 'No pull requests'" - ); - // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. - _m.assert_async().await; - } - - #[tokio::test] - async fn cmd_view_surfaces_denial_not_stub() { - let dir = TempDir::new().unwrap(); - write_identity(&dir); - let mut server = mockito::Server::new_async().await; - // The PR fetch is first; a 404 there errors before the reviews/comments reads. - let _m = server - .mock("GET", mockito::Matcher::Regex(r"/pulls/1$".to_string())) .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"repository not found"}"#) .expect(1) .create_async() .await; - let result = cmd_view( + let result = cmd_diff( "myrepo".to_string(), 1, server.url(), @@ -905,21 +869,21 @@ mod tests { .await; assert!( result.is_err(), - "cmd_view must Err on 404, not print a stub PR" + "cmd_diff must Err on 404, not print 'No diff'" ); // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. _m.assert_async().await; } #[tokio::test] - async fn cmd_diff_surfaces_denial_not_empty() { + async fn cmd_comments_surfaces_denial_not_empty() { let dir = TempDir::new().unwrap(); write_identity(&dir); let mut server = mockito::Server::new_async().await; let _m = server .mock( "GET", - mockito::Matcher::Regex(r"/pulls/1/diff$".to_string()), + mockito::Matcher::Regex(r"/pulls/1/comments$".to_string()), ) .with_status(404) .with_header("content-type", "application/json") @@ -927,46 +891,46 @@ mod tests { .expect(1) .create_async() .await; - let result = cmd_diff( + let result = cmd_comments( "myrepo".to_string(), 1, server.url(), Some(dir.path().to_path_buf()), ) .await; - assert!( - result.is_err(), - "cmd_diff must Err on 404, not print 'No diff'" - ); + assert!(result.is_err(), "cmd_comments must Err on a gated 404"); // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. _m.assert_async().await; } + // cmd_diff had no signing coverage, so the unsigned read (#115) was invisible + // here. An identity is supplied, so the request must be signed. #[tokio::test] - async fn cmd_comments_surfaces_denial_not_empty() { + async fn test_cmd_diff_signs_request() { let dir = TempDir::new().unwrap(); write_identity(&dir); + let mut server = mockito::Server::new_async().await; let _m = server .mock( "GET", - mockito::Matcher::Regex(r"/pulls/1/comments$".to_string()), + mockito::Matcher::Regex(r"^/api/v1/repos/[^/]+/myrepo/pulls/1/diff$".to_string()), ) - .with_status(404) + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) + .with_status(200) .with_header("content-type", "application/json") - .with_body(r#"{"message":"repository not found"}"#) - .expect(1) + .with_body(r#"{"diff":"diff --git a/x b/x"}"#) .create_async() .await; - let result = cmd_comments( + + cmd_diff( "myrepo".to_string(), 1, server.url(), Some(dir.path().to_path_buf()), ) - .await; - assert!(result.is_err(), "cmd_comments must Err on a gated 404"); - // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. - _m.assert_async().await; + .await + .unwrap(); } } diff --git a/crates/gl/src/repo.rs b/crates/gl/src/repo.rs index 936effc88..82c0ec3a4 100644 --- a/crates/gl/src/repo.rs +++ b/crates/gl/src/repo.rs @@ -477,7 +477,9 @@ pub(crate) async fn cmd_commits( node: String, dir: Option, ) -> Result<()> { - let client = NodeClient::new(&node, None); + // Sign when an identity is available so a private repo's owner can read their + // own commits; `owner/name` against a public repo still works anonymously. + let client = NodeClient::new(&node, load_keypair_from_dir(dir.as_deref()).ok()); let (owner, name) = if repo.contains('/') { let (o, n) = repo.split_once('/').unwrap(); @@ -488,7 +490,7 @@ pub(crate) async fn cmd_commits( }; let url = format!("/api/v1/repos/{owner}/{name}/commits?branch={branch}&limit={limit}"); - let resp = crate::http::read_json(client.get(&url).await?, "commits").await?; + let resp = crate::http::read_json(client.get_maybe_signed(&url).await?, "commits").await?; let commits = resp["commits"].as_array().cloned().unwrap_or_default(); if commits.is_empty() { @@ -928,6 +930,9 @@ mod tests { "GET", mockito::Matcher::Regex(r"^/api/v1/repos/[^/]+/myrepo/commits".to_string()), ) + // An identity is supplied, so get_maybe_signed must sign the request. + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"commits":[]}"#) @@ -956,6 +961,9 @@ mod tests { "GET", mockito::Matcher::Regex(r"^/api/v1/repos/[^/]+/myrepo/commits".to_string()), ) + // An identity is supplied, so get_maybe_signed must sign the request. + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"commits":[{"sha":"abc1234567","message":"initial commit","author_name":"alice","date":"2026-03-18T00:00:00Z"}]}"#) @@ -973,6 +981,38 @@ mod tests { .unwrap(); } + // Empty dir → no identity → get_maybe_signed must fall back to an anonymous + // GET (the public-repo path). `owner/name` needs no local identity, so this + // is the negative twin of the #115 signing fix. Assert NO signature header. + #[tokio::test] + async fn test_cmd_commits_anonymous_when_no_identity() { + let dir = TempDir::new().unwrap(); + + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/owner/pubrepo/commits".to_string()), + ) + .match_header("signature", mockito::Matcher::Missing) + .match_header("signature-input", mockito::Matcher::Missing) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"commits":[]}"#) + .create_async() + .await; + + cmd_commits( + "owner/pubrepo".to_string(), + "main".to_string(), + 20, + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap(); + } + #[tokio::test] async fn test_cmd_fork_success() { let dir = TempDir::new().unwrap();