Skip to content

Commit 77fe530

Browse files
N0zoM1z0N0zoM1z0
committed
fix(security): reject credential-bearing git remotes
1 parent bad6e90 commit 77fe530

10 files changed

Lines changed: 344 additions & 110 deletions

File tree

‎cmd/gitcontribute/main.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,7 @@ func main() {
3030
// Generate a trace ID for this invocation so all log lines from the
3131
// same command run can be correlated.
3232
traceID := uuid.NewString()
33-
logger.InfoContext(ctx, "starting",
34-
"version", version,
35-
"trace_id", traceID,
36-
"args", os.Args[1:],
37-
)
33+
logInvocationStart(ctx, logger, traceID, os.Args[1:])
3834
ctx = gitlog.WithTrace(ctx, traceID)
3935

4036
paths := config.NewPaths(nil)
@@ -62,6 +58,14 @@ func main() {
6258

6359
const ExitGeneral = 1
6460

61+
func logInvocationStart(ctx context.Context, logger *slog.Logger, traceID string, args []string) {
62+
logger.InfoContext(ctx, "starting",
63+
"version", version,
64+
"trace_id", traceID,
65+
"arg_count", len(args),
66+
)
67+
}
68+
6569
func reportCommandError(ctx context.Context, logger *slog.Logger, stderr io.Writer, traceID string, err error) int {
6670
var cliErr *cli.CLIError
6771
if errors.As(err, &cliErr) {

‎cmd/gitcontribute/main_test.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,3 +53,20 @@ func TestReportCommandErrorLogsUnexpectedError(t *testing.T) {
5353
t.Fatalf("unexpected error log = %q", logOutput)
5454
}
5555
}
56+
57+
func TestLogInvocationStartDoesNotLogArguments(t *testing.T) {
58+
var logs bytes.Buffer
59+
logger := slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelInfo}))
60+
fixturePassword := strings.Join([]string{"fixture", "password"}, "-")
61+
remote := "https://fixture-user:" + fixturePassword + "@github.com/owner/repo.git"
62+
63+
logInvocationStart(context.Background(), logger, "trace-test", []string{"workspace", "create", "--remote", remote})
64+
65+
got := logs.String()
66+
if strings.Contains(got, fixturePassword) || strings.Contains(got, remote) || strings.Contains(got, "--remote") {
67+
t.Fatalf("invocation log exposed argument values: %q", got)
68+
}
69+
if !strings.Contains(got, "arg_count=4") {
70+
t.Fatalf("invocation log omitted safe argument count: %q", got)
71+
}
72+
}

‎internal/acquire/acquire.go‎

Lines changed: 3 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,8 @@ import (
1010
"encoding/json"
1111
"errors"
1212
"fmt"
13-
"net/url"
1413
"os"
1514
"os/exec"
16-
"path"
1715
"path/filepath"
1816
"strings"
1917
"sync"
@@ -22,6 +20,7 @@ import (
2220
"github.com/gofrs/flock"
2321
"github.com/google/uuid"
2422
"github.com/morluto/gitcontribute/internal/domain"
23+
"github.com/morluto/gitcontribute/internal/gitremote"
2524
)
2625

2726
var (
@@ -461,48 +460,10 @@ func (m *Manager) writeMetadata(acq *Acquisition) error {
461460
}
462461

463462
func validateRemote(remote string) error {
464-
remote = strings.TrimSpace(remote)
465-
if remote == "" || strings.HasPrefix(remote, "-") || strings.ContainsAny(remote, "\x00\r\n") {
463+
if err := gitremote.Validate(remote); err != nil {
466464
return ErrInvalidRemote
467465
}
468-
if strings.Contains(remote, "::") {
469-
return ErrInvalidRemote
470-
}
471-
if filepath.IsAbs(remote) || path.IsAbs(remote) || strings.HasPrefix(remote, "file://") {
472-
return nil
473-
}
474-
if strings.HasPrefix(remote, "https://") {
475-
u, err := url.Parse(remote)
476-
if err != nil || u.User != nil || u.Host == "" {
477-
return ErrInvalidRemote
478-
}
479-
return nil
480-
}
481-
if strings.HasPrefix(remote, "ssh://") {
482-
u, err := url.Parse(remote)
483-
if err != nil || u.Host == "" {
484-
return ErrInvalidRemote
485-
}
486-
if u.User != nil {
487-
if _, ok := u.User.Password(); ok {
488-
return ErrInvalidRemote
489-
}
490-
}
491-
if u.Path == "" || u.Path == "/" {
492-
return ErrInvalidRemote
493-
}
494-
return nil
495-
}
496-
if at := strings.IndexByte(remote, '@'); at > 0 {
497-
if strings.Contains(remote[:at], ":") {
498-
return ErrInvalidRemote
499-
}
500-
hostPath := remote[at+1:]
501-
if colon := strings.IndexByte(hostPath, ':'); colon > 0 && colon < len(hostPath)-1 {
502-
return nil
503-
}
504-
}
505-
return ErrInvalidRemote
466+
return nil
506467
}
507468

508469
func cacheNameFor(owner, repo, remote string) string {

‎internal/acquire/acquire_test.go‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,45 @@ func TestValidateRemoteRejectsCredentialsAndPreservesSSH(t *testing.T) {
6666
}
6767
}
6868

69+
type countingRunner struct {
70+
calls int
71+
}
72+
73+
func (r *countingRunner) Run(context.Context, string, ...string) (string, error) {
74+
r.calls++
75+
return "", errors.New("unexpected Git invocation")
76+
}
77+
78+
func TestAcquireRejectsCredentialRemoteBeforeSideEffects(t *testing.T) {
79+
fixtureUser := strings.Join([]string{"fixture", "user"}, "-")
80+
fixturePassword := strings.Join([]string{"fixture", "password"}, "-")
81+
remote := "https://" + fixtureUser + ":" + fixturePassword + "@github.com/owner/repo.git"
82+
root := t.TempDir()
83+
runner := &countingRunner{}
84+
mgr, err := NewManager(root, runner)
85+
if err != nil {
86+
t.Fatal(err)
87+
}
88+
89+
_, err = mgr.Acquire(context.Background(), "owner", "repo", remote)
90+
if !errors.Is(err, ErrInvalidRemote) {
91+
t.Fatalf("Acquire credential remote error = %v, want ErrInvalidRemote", err)
92+
}
93+
if strings.Contains(err.Error(), fixturePassword) {
94+
t.Fatalf("Acquire error exposed credential: %v", err)
95+
}
96+
if runner.calls != 0 {
97+
t.Fatalf("Git runner calls = %d, want 0", runner.calls)
98+
}
99+
entries, err := os.ReadDir(root)
100+
if err != nil {
101+
t.Fatal(err)
102+
}
103+
if len(entries) != 0 {
104+
t.Fatalf("acquisition files written before remote validation: %v", entries)
105+
}
106+
}
107+
69108
func TestWriteMetadataAtomicallyWithPrivatePermissions(t *testing.T) {
70109
if runtime.GOOS == "windows" {
71110
t.Skip("Windows does not expose Unix permission bits")

‎internal/app/app_test.go‎

Lines changed: 0 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -821,53 +821,6 @@ func TestDefineValidationParsesQuotedArguments(t *testing.T) {
821821
}
822822
}
823823

824-
func TestWorkspaceCreateAndShow(t *testing.T) {
825-
if _, err := exec.LookPath("git"); err != nil {
826-
t.Skip("git not available")
827-
}
828-
ctx := context.Background()
829-
remote, baseSHA, candidateSHA := setupAppGitRemote(t)
830-
831-
paths := config.NewPaths(&config.Env{Home: t.TempDir()})
832-
svc, err := New(paths, "test", nil)
833-
if err != nil {
834-
t.Fatalf("new service: %v", err)
835-
}
836-
defer func() { _ = svc.Close() }()
837-
if _, err := svc.Init(ctx); err != nil {
838-
t.Fatalf("init: %v", err)
839-
}
840-
841-
inv, err := svc.StartInvestigation(ctx, cli.RepoRef{Owner: "owner", Repo: "repo"}, candidateSHA, "")
842-
if err != nil {
843-
t.Fatalf("start investigation: %v", err)
844-
}
845-
846-
ws, err := svc.CreateWorkspace(ctx, inv.ID, cli.WorkspaceCreateOptions{
847-
Remote: remote,
848-
BaseRef: "master",
849-
CandidateRef: "feature",
850-
Name: "ws-test",
851-
})
852-
if err != nil {
853-
t.Fatalf("create workspace: %v", err)
854-
}
855-
if ws.ID != "ws-test" || ws.InvestigationID != inv.ID || ws.BaseSHA != baseSHA || ws.CandidateSHA != candidateSHA {
856-
t.Fatalf("unexpected workspace: %+v", ws)
857-
}
858-
if _, err := os.Stat(ws.Path); err != nil {
859-
t.Fatalf("workspace path missing: %v", err)
860-
}
861-
862-
shown, err := svc.ShowWorkspace(ctx, ws.ID)
863-
if err != nil {
864-
t.Fatalf("show workspace: %v", err)
865-
}
866-
if shown.ID != ws.ID || shown.BaseSHA != baseSHA {
867-
t.Fatalf("workspace roundtrip failed: %+v", shown)
868-
}
869-
}
870-
871824
func TestMirrorNamesAreUnambiguous(t *testing.T) {
872825
a := mirrorNameFor("a", "b-c", "https://github.com/a/b-c.git")
873826
b := mirrorNameFor("a-b", "c", "https://github.com/a-b/c.git")

‎internal/app/workspace_security_test.go‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package app
2+
3+
import (
4+
"context"
5+
"errors"
6+
"os"
7+
"os/exec"
8+
"path/filepath"
9+
"strings"
10+
"testing"
11+
12+
"github.com/morluto/gitcontribute/internal/cli"
13+
"github.com/morluto/gitcontribute/internal/config"
14+
"github.com/morluto/gitcontribute/internal/workspace"
15+
)
16+
17+
func TestWorkspaceCreateAndShow(t *testing.T) {
18+
if _, err := exec.LookPath("git"); err != nil {
19+
t.Skip("git not available")
20+
}
21+
ctx := context.Background()
22+
remote, baseSHA, candidateSHA := setupAppGitRemote(t)
23+
24+
paths := config.NewPaths(&config.Env{Home: t.TempDir()})
25+
svc, err := New(paths, "test", nil)
26+
if err != nil {
27+
t.Fatalf("new service: %v", err)
28+
}
29+
defer func() { _ = svc.Close() }()
30+
if _, err := svc.Init(ctx); err != nil {
31+
t.Fatalf("init: %v", err)
32+
}
33+
34+
inv, err := svc.StartInvestigation(ctx, cli.RepoRef{Owner: "owner", Repo: "repo"}, candidateSHA, "")
35+
if err != nil {
36+
t.Fatalf("start investigation: %v", err)
37+
}
38+
39+
t.Run("rejects credential remote before persistence", func(t *testing.T) {
40+
fixtureUser := strings.Join([]string{"fixture", "user"}, "-")
41+
fixturePassword := strings.Join([]string{"fixture", "password"}, "-")
42+
credentialRemote := "https://" + fixtureUser + ":" + fixturePassword + "@github.com/owner/repo.git"
43+
_, err := svc.CreateWorkspace(ctx, inv.ID, cli.WorkspaceCreateOptions{
44+
Remote: credentialRemote,
45+
Name: "credential-test",
46+
})
47+
if !errors.Is(err, workspace.ErrInvalidRemote) {
48+
t.Fatalf("CreateWorkspace credential remote error = %v, want ErrInvalidRemote", err)
49+
}
50+
if strings.Contains(err.Error(), fixturePassword) {
51+
t.Fatalf("CreateWorkspace error exposed credential: %v", err)
52+
}
53+
54+
dataDir, err := paths.DataDir()
55+
if err != nil {
56+
t.Fatal(err)
57+
}
58+
if _, err := os.Stat(filepath.Join(dataDir, "workspaces", "mirrors")); !errors.Is(err, os.ErrNotExist) {
59+
t.Fatalf("mirror directory was written before remote validation: %v", err)
60+
}
61+
c, err := svc.openCorpus(ctx)
62+
if err != nil {
63+
t.Fatal(err)
64+
}
65+
if _, err := c.GetWorkspace(ctx, "credential-test"); !errors.Is(err, workspace.ErrNotFound) {
66+
t.Fatalf("credential remote workspace was persisted: %v", err)
67+
}
68+
})
69+
70+
ws, err := svc.CreateWorkspace(ctx, inv.ID, cli.WorkspaceCreateOptions{
71+
Remote: remote,
72+
BaseRef: "master",
73+
CandidateRef: "feature",
74+
Name: "ws-test",
75+
})
76+
if err != nil {
77+
t.Fatalf("create workspace: %v", err)
78+
}
79+
if ws.ID != "ws-test" || ws.InvestigationID != inv.ID || ws.BaseSHA != baseSHA || ws.CandidateSHA != candidateSHA {
80+
t.Fatalf("unexpected workspace: %+v", ws)
81+
}
82+
if _, err := os.Stat(ws.Path); err != nil {
83+
t.Fatalf("workspace path missing: %v", err)
84+
}
85+
86+
shown, err := svc.ShowWorkspace(ctx, ws.ID)
87+
if err != nil {
88+
t.Fatalf("show workspace: %v", err)
89+
}
90+
if shown.ID != ws.ID || shown.BaseSHA != baseSHA {
91+
t.Fatalf("workspace roundtrip failed: %+v", shown)
92+
}
93+
}

0 commit comments

Comments
 (0)