Commit `1d81484849`

1d8148484928929f136dfe1e712e4c65e3ca87a0

parent: fa31441639

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-29 02:50 UTC

control: import-issues API client dials only the checked address

The API base is resolved and checked once with gitpin; the client dials
those addresses, takes no proxy from the environment, and follows no
redirect.

Ref #301

Layout: unified · split

internal/control/ghimport.go +28 −7

		@@ -19,7 +19,6 @@ import (
19	19	"gitbay.org/gitbay/internal/policy"
20	20	"gitbay.org/gitbay/internal/protocol"
21	21	"gitbay.org/gitbay/internal/store"
22		"gitbay.org/gitbay/internal/webhook"
23	22	)
24	23
25	24	func init() {
		@@ -135,6 +134,19 @@ func (g *ghClient) get(path string, out any) error {
135	134	return json.NewDecoder(resp.Body).Decode(out)
136	135	}
137	136
	137	// pinnedClient reaches api's host only at its checked addresses, never
	138	// through a proxy from the environment, and follows no redirect, as
	139	// webhook delivery and repo import do.
	140	func pinnedClient(api gitpin.Remote) *http.Client {
	141	return &http.Client{
	142	Timeout: 30 * time.Second,
	143	Transport: &http.Transport{Proxy: nil, DialContext: api.DialContext, TLSHandshakeTimeout: 10 * time.Second},
	144	CheckRedirect: func(req http.Request, _ []http.Request) error {
	145	return fmt.Errorf("refusing redirect to %s://%s; a renamed repository is imported under its new name", req.URL.Scheme, req.URL.Host)
	146	},
	147	}
	148	}
	149
138	150	func ghDate(iso string) string {
139	151	if t, err := time.Parse(time.RFC3339, iso); err == nil {
140	152	return t.UTC().Format("2006-01-02")
		@@ -158,12 +170,21 @@ func runImportIssues(c *Ctx, args []string) int {
158	170	if path == "" \|\| from == "" {
159	171	return c.usage()
160	172	}
161		if apiBase == "" {
	173	given := apiBase != ""
	174	if !given {
162	175	apiBase = "https://api.github.com"
163		} else if err := webhook.ValidateURL(apiBase, c.Cfg.Webhooks.AllowLocal); err != nil {
164		// A writer-supplied API base is the same SSRF surface as a
165		// webhook target; same rules apply.
166		return c.fail(protocol.ExitUsage, "--api-base: %v", err)
	176	}
	177	// A writer-supplied API base is the same SSRF surface as a webhook
	178	// target. Resolve and check it once here; the client connects only
	179	// to those addresses (#301).
	180	rctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	181	api, err := gitpin.Resolve(rctx, importLookup, apiBase, c.Cfg.Webhooks.AllowLocal)
	182	cancel()
	183	if err != nil {
	184	if given {
	185	return c.fail(protocol.ExitUsage, "--api-base: %v", err)
	186	}
	187	return c.fail(protocol.ExitFailure, "%v", err)
167	188	}
168	189	site := siteFromAPI(apiBase)
169	190	host := strings.TrimPrefix(strings.TrimPrefix(site, "https://"), "http://")
		@@ -191,7 +212,7 @@ func runImportIssues(c *Ctx, args []string) int {
191	212	}
192	213	token = strings.TrimSpace(line)
193	214	}
194		g := &ghClient{base: apiBase, token: token, http: &http.Client{Timeout: 30 * time.Second}}
	215	g := &ghClient{base: apiBase, token: token, http: pinnedClient(api)}
195	216	g.detect()
196	217	dir := RepoDir(c.Cfg.Server.Root, repo.OwnerName, repo.Name)
197	218

internal/control/ghimport_test.go +73

		@@ -3,6 +3,7 @@ package control
3	3	import (
4	4	"context"
5	5	"net"
	6	"net/http"
6	7	"net/http/cgi"
7	8	"net/http/httptest"
8	9	"net/url"
		@@ -13,6 +14,7 @@ import (
13	14	"testing"
14	15
15	16	"gitbay.org/gitbay/internal/gitutil"
	17	"gitbay.org/gitbay/internal/protocol"
16	18	)
17	19
18	20	// pullUpstream serves a bare repository whose refs/pull/1/head is one
		@@ -103,3 +105,74 @@ func TestFetchPullHeadsRefusesALocalAddress(t *testing.T) {
103	105	t.Fatal("refs/gh-pull/1 was fetched")
104	106	}
105	107	}
	108
	109	// apiServer serves an empty issue list for o/r, plus whatever extra
	110	// registers, and returns the server's port.
	111	func apiServer(t testing.T, extra func(mux http.ServeMux)) string {
	112	t.Helper()
	113	mux := http.NewServeMux()
	114	mux.HandleFunc("/repos/o/r/issues", func(w http.ResponseWriter, r *http.Request) {
	115	w.Write([]byte("[]"))
	116	})
	117	if extra != nil {
	118	extra(mux)
	119	}
	120	srv := httptest.NewServer(mux)
	121	t.Cleanup(srv.Close)
	122	u, _ := url.Parse(srv.URL)
	123	return u.Port()
	124	}
	125
	126	// api.test does not resolve; the import reaches the API only because the
	127	// client dialed the address import-issues looked up and checked (#301).
	128	func TestImportIssuesConnectsToTheCheckedAddress(t *testing.T) {
	129	port := apiServer(t, nil)
	130	c, errOut, _, _ := importCtx(t, true)
	131	asked := stubLookup(t, "127.0.0.1")
	132	code := Dispatch(c, []string{"repo", "import-issues", "alice/app", "--from", "o/r", "--api-base", "http://api.test:" + port})
	133	if code != protocol.ExitOK {
	134	t.Fatalf("exit %d: %s", code, errOut.String())
	135	}
	136	if len(asked) == 0 \|\| (asked)[0] != "api.test" {
	137	t.Fatalf("looked up %v", *asked)
	138	}
	139	}
	140
	141	// A redirect is refused and its target never asked, although the dialer
	142	// would land it on the checked address.
	143	func TestImportIssuesRefusesARedirect(t *testing.T) {
	144	var port string
	145	reached := false
	146	port = apiServer(t, func(mux *http.ServeMux) {
	147	mux.HandleFunc("/repos/o/x/issues", func(w http.ResponseWriter, r *http.Request) {
	148	http.Redirect(w, r, "http://other.test:"+port+"/repos/o/x/moved", http.StatusMovedPermanently)
	149	})
	150	mux.HandleFunc("/repos/o/x/moved", func(w http.ResponseWriter, r *http.Request) {
	151	reached = true
	152	w.Write([]byte("[]"))
	153	})
	154	})
	155	c, errOut, _, _ := importCtx(t, true)
	156	stubLookup(t, "127.0.0.1")
	157	code := Dispatch(c, []string{"repo", "import-issues", "alice/app", "--from", "o/x", "--api-base", "http://api.test:" + port})
	158	if code != protocol.ExitFailure \|\| !strings.Contains(errOut.String(), "refusing redirect to http://other.test") {
	159	t.Fatalf("exit %d: %s", code, errOut.String())
	160	}
	161	if reached {
	162	t.Fatal("followed a redirect to another host")
	163	}
	164	}
	165
	166	// An API base that resolves to private space is refused on a default
	167	// instance before anything connects.
	168	func TestImportIssuesRefusesAPrivateAPIBase(t *testing.T) {
	169	c, errOut, _, _ := importCtx(t, false)
	170	asked := stubLookup(t, "10.0.0.1")
	171	code := Dispatch(c, []string{"repo", "import-issues", "alice/app", "--from", "o/r", "--api-base", "https://api.test"})
	172	if code != protocol.ExitUsage \|\| !strings.Contains(errOut.String(), "private or local address") {
	173	t.Fatalf("exit %d: %s", code, errOut.String())
	174	}
	175	if !slices.Equal(*asked, []string{"api.test"}) {
	176	t.Fatalf("looked up %v", *asked)
	177	}
	178	}

internal/gitpin/gitpin.go +22 −1

		@@ -1,6 +1,7 @@
1	1	// Package gitpin runs git against a user-supplied http or https remote
2	2	// only at addresses resolved and checked immediately before: mirror
3		// sync (#279) and repo import (#298).
	3	// sync (#279), repo import (#298) and repo import-issues (#301), whose
	4	// API client dials the same way.
4	5	package gitpin
5	6
6	7	import (
		@@ -11,6 +12,7 @@ import (
11	12	"os/exec"
12	13	"strconv"
13	14	"strings"
	15	"time"
14	16
15	17	"gitbay.org/gitbay/internal/toolpath"
16	18	"gitbay.org/gitbay/internal/webhook"
		@@ -63,6 +65,25 @@ func Resolve(ctx context.Context, lookup Lookup, raw string, allowLocal bool) (R
63	65	return Remote{URL: u, IPs: ips}, nil
64	66	}
65	67
	68	// DialContext connects to r's checked addresses, trying each in turn,
	69	// whatever host addr names; only its port is used. An HTTP client
	70	// built on it must not follow a redirect to another host.
	71	func (r Remote) DialContext(ctx context.Context, network, addr string) (net.Conn, error) {
	72	_, port, err := net.SplitHostPort(addr)
	73	if err != nil {
	74	return nil, err
	75	}
	76	d := net.Dialer{Timeout: 10 * time.Second}
	77	for _, ip := range r.IPs {
	78	var conn net.Conn
	79	conn, err = d.DialContext(ctx, network, net.JoinHostPort(ip.String(), port))
	80	if err == nil {
	81	return conn, nil
	82	}
	83	}
	84	return nil, err
	85	}
	86
66	87	// CheckHost refuses a host written as a number in a form other than
67	88	// an IP literal: 127.1, 2130706433 and 0x7f.1 are loopback to curl's
68	89	// parser but not to Go's, so they are refused rather than left to a

Commit 1d81484849

Commit `1d81484849`