fix(credentials): surface error responses from credential server endpoint#694
Merged
fix(credentials): surface error responses from credential server endpoint#694
Conversation
Check HTTP status code before attempting JSON decode. When the credentials server returns an error, include the actual error message instead of an opaque JSON parse failure. Fixes #645
Write structured JSON errors instead of plain text so the response is always valid JSON regardless of success or failure.
Only run setupGitSSHSignature when both the signing key is configured AND the git SSH signature forwarding context option is enabled. Fixes #645
Add getSignatureURL() helper with a package-level override var so tests can inject httptest server URLs without changing function signatures.
Verify handleGitSSHSignatureRequest returns JSON error responses (not plain text) when the tunnel gRPC call fails, and returns valid JSON on success.
Test the full HTTP request/response cycle using httptest servers. Covers plain-text 500 (customer bug scenario), JSON 500, valid success, and invalid JSON responses.
Wire up the full signing chain with a test credentials server and mock tunnel client. Verify that signing failures surface clear error messages (not JSON parse errors) and that successful signing writes the .sig file correctly. Remove credentials package import from gitsshsigning to avoid an import cycle in tests - inline the port resolution logic instead. Fixes #645
📝 WalkthroughWalkthroughThis PR adds comprehensive testing and error handling improvements for Git SSH signature forwarding. Changes include gating the signature setup in cmd/up.go by a context option, implementing an HTTP signature server with JSON error responses, adding test utilities and integration tests, and enhancing client-side error handling with status code validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Git as Git Process
participant Handler as HTTP Handler<br/>(handleGitSSHSignatureRequest)
participant TunnelClient as Tunnel Client
participant Server as Signature Server
Git->>Handler: POST /git-ssh-signature<br/>(buffer file path)
Handler->>TunnelClient: GitSSHSignature(ctx, request)
alt Tunnel Client Error
TunnelClient-->>Handler: error (e.g., Permission denied)
Handler->>Handler: Marshal error to JSON
Handler-->>Git: HTTP 500<br/>{"error": "Permission denied"}
else Tunnel Client Success
TunnelClient->>Server: Forward signature request
Server-->>TunnelClient: {"signature": "..."}
TunnelClient-->>Handler: tunnel.Message (JSON response)
Handler->>Handler: Parse signature from JSON
Handler-->>Git: HTTP 200<br/>{"signature": "..."}
end
Git->>Git: Write signature to file
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Add two e2e tests to the SSH test suite: 1. Verify helper is not installed when --git-ssh-signing-key is omitted 2. Verify signing failure produces human-readable errors (not JSON parse artifacts) Also extract repeated "windows" string to a constant to satisfy goconst linter.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/gitsshsigning/client.go (1)
90-96: Package-level mutable state may cause test flakiness with parallel execution.The
signatureServerURLvariable can cause race conditions if tests run in parallel. Consider usingsync.Mutexorsync.RWMutexto protect access, or restructure to pass the URL as a parameter.🔧 Thread-safe alternative
+import "sync" + -// signatureServerURL overrides the server URL for testing. Empty means use credentials.GetPort(). -var signatureServerURL string +var ( + signatureServerURL string + signatureServerURLMu sync.RWMutex +) // SetSignatureServerURL sets the server URL override for testing. func SetSignatureServerURL(url string) { + signatureServerURLMu.Lock() + defer signatureServerURLMu.Unlock() signatureServerURL = url } func getSignatureURL() (string, error) { + signatureServerURLMu.RLock() + override := signatureServerURL + signatureServerURLMu.RUnlock() + if override != "" { + return override, nil - if signatureServerURL != "" { - return signatureServerURL, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gitsshsigning/client.go` around lines 90 - 96, The package-level mutable variable signatureServerURL and its setter SetSignatureServerURL are not safe for parallel tests; protect reads/writes with a sync.RWMutex (declare e.g. signatureURLMu) and lock in SetSignatureServerURL and any accessor, or remove the global and instead pass the URL as an explicit parameter to functions that need it (refactor callers of SetSignatureServerURL to accept the URL argument). Ensure every place that reads signatureServerURL uses the guarded accessor or the new parameter to avoid races in parallel test execution.pkg/gitsshsigning/client_test.go (1)
32-49: Consider marking tests as non-parallel due to shared state.These tests mutate the package-level
signatureServerURLvariable. If Go's test runner executes them in parallel with-parallel, race conditions could occur. Consider either addingt.Parallel()guards or documenting that these tests cannot run in parallel.Note: Since
go testdoesn't parallelize tests by default within a package unlesst.Parallel()is explicitly called, this is low risk but worth noting for future maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gitsshsigning/client_test.go` around lines 32 - 49, Tests mutate the package-level signatureServerURL which can cause races if tests run concurrently; protect that shared state by introducing a package-level sync.Mutex (e.g., testMutex) and lock it at the start of each test that changes signatureServerURL (including TestRequestContentSignature_ServerError_PlainText) and release it in t.Cleanup so the original value restoration still runs; alternatively, explicitly avoid parallel execution by not calling t.Parallel() and documenting the tests are serialized — but the recommended change is to add the mutex lock/unlock around modifications to signatureServerURL to ensure safe concurrent test runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/gitsshsigning/client_test.go`:
- Around line 32-49: Tests mutate the package-level signatureServerURL which can
cause races if tests run concurrently; protect that shared state by introducing
a package-level sync.Mutex (e.g., testMutex) and lock it at the start of each
test that changes signatureServerURL (including
TestRequestContentSignature_ServerError_PlainText) and release it in t.Cleanup
so the original value restoration still runs; alternatively, explicitly avoid
parallel execution by not calling t.Parallel() and documenting the tests are
serialized — but the recommended change is to add the mutex lock/unlock around
modifications to signatureServerURL to ensure safe concurrent test runs.
In `@pkg/gitsshsigning/client.go`:
- Around line 90-96: The package-level mutable variable signatureServerURL and
its setter SetSignatureServerURL are not safe for parallel tests; protect
reads/writes with a sync.RWMutex (declare e.g. signatureURLMu) and lock in
SetSignatureServerURL and any accessor, or remove the global and instead pass
the URL as an explicit parameter to functions that need it (refactor callers of
SetSignatureServerURL to accept the URL argument). Ensure every place that reads
signatureServerURL uses the guarded accessor or the new parameter to avoid races
in parallel test execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0ff7178-c293-4210-a007-50b1b003ad79
📒 Files selected for processing (8)
cmd/up.goe2e/tests/ssh/ssh.gopkg/credentials/integration_test.gopkg/credentials/mock_tunnel_client_test.gopkg/credentials/server.gopkg/credentials/server_test.gopkg/gitsshsigning/client.gopkg/gitsshsigning/client_test.go
skevetter
changed the title
This was referenced Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
invalid character 'g')/git-ssh-signatureendpointFixes #645
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes