import-issues: connect only to checked addresses !515

merged merged by cmc on 2026-09-29 02:59 UTC · krz/gitbay:import-issues-pin into main

9 files changed, +287 −26

Layout: unified · split

.gitbay/wiki/Architecture/09-Controls.org +1 −1

	@@ -75,7 +75,7 @@ chapter names of OWASP ASVS 4.0 where one fits.
75		75
76	\| Control \| Status \| Evidence \|	76	\| Control \| Status \| Evidence \|
77	\|---------------------------------------------+----------+------------------------------------------------------------------\|	77	\|---------------------------------------------+----------+------------------------------------------------------------------\|
78	\| SSRF protection on user-supplied URLs \| partial \| webhooks at save and connect; mirrors at save and sync, =repo import= before its fetch, git pinned to the checked address (=internal/gitpin=); =repo import-issues --api-base= checked once and fetched unpinned (#301) \|	78	\| SSRF protection on user-supplied URLs \| in place \| webhooks at save and connect; mirrors at save and sync, =repo import= and =repo import-issues= before they fetch, git and the import API client pinned to the checked address (=internal/gitpin=) \|
79	\| Webhook payload integrity \| in place \| HMAC-SHA256 header \|	79	\| Webhook payload integrity \| in place \| HMAC-SHA256 header \|
80	\| SMTP credentials protected in transit \| in place \| STARTTLS required for non-local relays, implicit TLS optional (=internal/mail/mail.go=) \|	80	\| SMTP credentials protected in transit \| in place \| STARTTLS required for non-local relays, implicit TLS optional (=internal/mail/mail.go=) \|
81	\| Upload size limits \| in place \| per-owner storage quota at push (=internal/sshd/sshd.go=); API body 1 MiB \|	81	\| Upload size limits \| in place \| per-owner storage quota at push (=internal/sshd/sshd.go=); API body 1 MiB \|

.gitbay/wiki/Architecture/10-Known-Gaps.org −1

	@@ -13,7 +13,6 @@ what the 2026-09-27 review found; remove a row when its issue closes.
13	\| #259 \| Recovery \| No restore has been exercised; the drill is written (Admin wiki) and not yet run \| high \|	13	\| #259 \| Recovery \| No restore has been exercised; the drill is written (Admin wiki) and not yet run \| high \|
14	\| #260 \| CI network \| Builds share the runner's source address; no egress policy \| medium \|	14	\| #260 \| CI network \| Builds share the runner's source address; no egress policy \| medium \|
15	\| #261 \| Various \| Migration foreign-key check after commit; three web writes bypass dispatch; documentation drift \| medium \|	15	\| #261 \| Various \| Migration foreign-key check after commit; three web writes bypass dispatch; documentation drift \| medium \|
16	\| #301 \| SSRF \| =repo import-issues --api-base= checks the address once; fetches unpinned \| medium \|
17		16
18	* Not filed	17	* Not filed
19		18

.gitbay/wiki/Threat-Model.org +9 −6

 scheme must be http/https and, unless =webhooks.allow_local= is set,
 the resolved address must not be loopback, private, shared
 (100.64.0.0/10), link-local, or multicast. The webhook dialer re-checks
-at connect time, and mirror sync and =repo import= resolve and check
+at connect time, and mirror sync, =repo import= and =repo
-immediately before running git and pin it to the checked addresses
+import-issues= resolve and check immediately before running git and pin
-(=internal/gitpin=), so a DNS answer that changes after validation
+it to the checked addresses (=internal/gitpin=), so a DNS answer that
-still cannot reach private space. Redirects are never followed.
+changes after validation still cannot reach private space;
+=import-issues= holds its API client to the API host's checked
+addresses the same way, with no proxy taken from the environment.
+Redirects are never followed.
 =repo import= refuses =git://=, which cannot be pinned. Mirror sync
 and =repo import= refuse a host written as a bare number or in
 hex/octal (=0x7f.1=, =2130706433=, =127.1=) rather than dotted
 decimal, since that form resolves differently across parsers; =repo
-mirror add= refuses it when the mirror is saved. GitHub-history import (=repo
+mirror add= refuses it when the mirror is saved, and =repo
-import-issues --api-base=) is not yet pinned (#301).
+import-issues= refuses it in =--api-base=.
 * Rendering pushed markup

CHANGELOG.org +7

	@@ -19,6 +19,13 @@ anything beyond "replace the binary and restart" is needed.
19	global gitconfig are ignored. A source that redirects (a renamed	19	global gitconfig are ignored. A source that redirects (a renamed
20	repository) fails; import from the URL it redirects to. Needs git	20	repository) fails; import from the URL it redirects to. Needs git
21	2.37 or later on the server (#298).	21	2.37 or later on the server (#298).
		22	- =repo import-issues= resolves and checks the API host and the git
		23	host once and connects only to the checked addresses: the API client
		24	dials them with no proxy from the environment and follows no
		25	redirect, and the pull-head fetch runs git pinned, with redirects
		26	off and without the system and global gitconfig. =--api-base= refuses
		27	credentials, a query or a fragment. An API that redirects (a renamed
		28	repository) fails; import under the new name (#301).
22	- =repo mirror add= refuses a host written numerically (=127.1=,	29	- =repo mirror add= refuses a host written numerically (=127.1=,
23	=2130706433=, =0x7f.1=) when the mirror is added, rather than at its	30	=2130706433=, =0x7f.1=) when the mirror is added, rather than at its
24	first sync (#298).	31	first sync (#298).

internal/control/ghimport.go +49 −11

 	"strings"
 	"time"
+	"gitbay.org/gitbay/internal/gitpin"
 	"gitbay.org/gitbay/internal/gitutil"
 	"gitbay.org/gitbay/internal/policy"
 	"gitbay.org/gitbay/internal/protocol"
 	"gitbay.org/gitbay/internal/store"
-	"gitbay.org/gitbay/internal/webhook"
 )
 func init() {
 	return json.NewDecoder(resp.Body).Decode(out)
 }
+// pinnedClient reaches api's host only at its checked addresses, never
+// through a proxy from the environment, and follows no redirect, as
+// webhook delivery and repo import do. Each import builds its own
+// client, so connections are not kept for reuse.
+func pinnedClient(api gitpin.Remote) *http.Client {
+	return &http.Client{
+		Timeout:   30 * time.Second,
+		Transport: &http.Transport{Proxy: nil, DialContext: api.DialContext, TLSHandshakeTimeout: 10 * time.Second, DisableKeepAlives: true},
+		CheckRedirect: func(req *http.Request, _ []*http.Request) error {
+			return fmt.Errorf("refusing redirect to %s://%s; a renamed repository is imported under its new name", req.URL.Scheme, req.URL.Host)
+		},
+	}
+}
 func ghDate(iso string) string {
 	if t, err := time.Parse(time.RFC3339, iso); err == nil {
 		return t.UTC().Format("2006-01-02")
 	if path == "" || from == "" {
 		return c.usage()
 	}
-	if apiBase == "" {
+	given := apiBase != ""
+	if !given {
 		apiBase = "https://api.github.com"
-	} else if err := webhook.ValidateURL(apiBase, c.Cfg.Webhooks.AllowLocal); err != nil {
+	} else if strings.Contains(apiBase, "@") || strings.ContainsAny(apiBase, "?#") {
-		// A writer-supplied API base is the same SSRF surface as a
+		// Same rule as repo import: the URL is echoed and becomes the
-		// webhook target; same rules apply.
+		// site prefix, so credentials and queries stay out of it.
-		return c.fail(protocol.ExitUsage, "--api-base: %v", err)
+		return c.fail(protocol.ExitUsage, "--api-base: use the plain API URL, without credentials, a query or a fragment; a token goes on stdin with --token-stdin")
+	}
+	// A writer-supplied API base is the same SSRF surface as a webhook
+	// target. Resolve and check it once here; the client connects only
+	// to those addresses (#301).
+	rctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	api, err := gitpin.Resolve(rctx, importLookup, apiBase, c.Cfg.Webhooks.AllowLocal)
+	cancel()
+	if err != nil {
+		if given {
+			return c.fail(protocol.ExitUsage, "--api-base: %v", err)
+		}
+		return c.fail(protocol.ExitFailure, "%v", err)
 	}
 	site := siteFromAPI(apiBase)
 	host := strings.TrimPrefix(strings.TrimPrefix(site, "https://"), "http://")
 		}
 		token = strings.TrimSpace(line)
 	}
-	g := &ghClient{base: apiBase, token: token, http: &http.Client{Timeout: 30 * time.Second}}
+	g := &ghClient{base: apiBase, token: token, http: pinnedClient(api)}
 	g.detect()
 	dir := RepoDir(c.Cfg.Server.Root, repo.OwnerName, repo.Name)
 // Forgejo both publish pull heads under that name. Reports whether it
 // worked; a failure is not fatal, since importing issues from a
 // repository whose git data is not here yet is a reasonable thing to do.
+// git connects only to the addresses the remote's host resolved to and
+// passed the check here, as repo import does (#298, #301).
 func fetchPullHeads(c *Ctx, dir, remote, token string) bool {
-	env := []string{"GIT_TERMINAL_PROMPT=0", "HOME=" + c.Cfg.Server.Root}
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
+	defer cancel()
+	if err := gitpin.CheckGit(ctx); err != nil {
+		fmt.Fprintf(c.Stderr, "pull heads not fetched: %v\n", err)
+		return false
+	}
+	pinned, err := gitpin.Resolve(ctx, importLookup, remote, c.Cfg.Webhooks.AllowLocal)
+	if err != nil {
+		fmt.Fprintf(c.Stderr, "pull heads not fetched: %v\n", err)
+		return false
+	}
+	env := gitpin.Env(c.Cfg.Server.Root)
 	if token != "" {
 		askpass := filepath.Join(c.Cfg.Server.Root, "gh-import-askpass.sh")
 		if err := os.WriteFile(askpass, []byte(ghAskpass), 0o700); err != nil {
 		}
 		env = append(env, "GIT_ASKPASS="+askpass, "GITBAY_GH_TOKEN="+token)
 	}
-	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
+	if err := gitutil.FetchPullHeads(ctx, dir, remote, io.Discard, pinned.Args(), env); err != nil {
-	defer cancel()
-	if err := gitutil.FetchPullHeads(ctx, dir, remote, io.Discard, env); err != nil {
 		return false
 	}
 	return true

internal/control/ghimport_test.go added +191

@@ -0,0 +1,191 @@
	1	package control
	2
	3	import (
	4	"context"
	5	"net"
	6	"net/http"
	7	"net/http/cgi"
	8	"net/http/httptest"
	9	"net/url"
	10	"os"
	11	"path/filepath"
	12	"slices"
	13	"strings"
	14	"testing"
	15
	16	"gitbay.org/gitbay/internal/gitutil"
	17	"gitbay.org/gitbay/internal/protocol"
	18	)
	19
	20	// pullUpstream serves a bare repository whose refs/pull/1/head is one
	21	// commit over smart HTTP, and returns its port and that commit.
	22	func pullUpstream(t *testing.T) (string, string) {
	23	t.Helper()
	24	git := gitRunner(t)
	25	parent := t.TempDir()
	26	bare := filepath.Join(parent, "o", "r.git")
	27	work := filepath.Join(parent, "work")
	28	git(parent, "init", "-q", "--bare", "--initial-branch=main", bare)
	29	git(parent, "init", "-q", "--initial-branch=main", work)
	30	git(work, "commit", "-q", "--allow-empty", "-m", "pr")
	31	git(work, "push", "-q", bare, "HEAD:refs/pull/1/head")
	32	sha := strings.TrimSpace(git(work, "rev-parse", "HEAD"))
	33	execPath := strings.TrimSpace(git(parent, "--exec-path"))
	34	srv := httptest.NewServer(&cgi.Handler{
	35	Path: filepath.Join(execPath, "git-http-backend"),
	36	Env: []string{"GIT_PROJECT_ROOT=" + parent, "GIT_HTTP_EXPORT_ALL=1"},
	37	})
	38	t.Cleanup(srv.Close)
	39	u, _ := url.Parse(srv.URL)
	40	return u.Port(), sha
	41	}
	42
	43	// stubLookup answers every host with ip and records what was asked.
	44	func stubLookup(t testing.T, ip string) []string {
	45	t.Helper()
	46	var asked []string
	47	prev := importLookup
	48	importLookup = func(ctx context.Context, host string) ([]net.IP, error) {
	49	asked = append(asked, host)
	50	return []net.IP{net.ParseIP(ip)}, nil
	51	}
	52	t.Cleanup(func() { importLookup = prev })
	53	return &asked
	54	}
	55
	56	// pulls.test does not resolve, and the daemon's environment points git
	57	// at a proxy and rewrites the URL to a dead port: the fetch works only
	58	// because git was pinned to the checked address and ran with its own
	59	// environment (#301).
	60	func TestFetchPullHeadsIsPinned(t *testing.T) {
	61	port, sha := pullUpstream(t)
	62	t.Setenv("http_proxy", "http://127.0.0.1:1")
	63	t.Setenv("HTTP_PROXY", "http://127.0.0.1:1")
	64	global := filepath.Join(t.TempDir(), "gitconfig")
	65	rewrite := "[url \"http://127.0.0.1:1/\"]\n\tinsteadOf = http://pulls.test:" + port + "/\n"
	66	if err := os.WriteFile(global, []byte(rewrite), 0o644); err != nil {
	67	t.Fatal(err)
	68	}
	69	t.Setenv("GIT_CONFIG_GLOBAL", global)
	70	c, errOut, _, root := importCtx(t, true)
	71	asked := stubLookup(t, "127.0.0.1")
	72	dir := filepath.Join(root, "dest.git")
	73	if err := gitutil.InitBare(dir, "main", ""); err != nil {
	74	t.Fatal(err)
	75	}
	76	if !fetchPullHeads(c, dir, "http://pulls.test:"+port+"/o/r.git", "") {
	77	t.Fatalf("fetch failed: %s", errOut.String())
	78	}
	79	got := strings.TrimSpace(gitRunner(t)(dir, "rev-parse", "refs/gh-pull/1"))
	80	if got != sha {
	81	t.Fatalf("refs/gh-pull/1 = %s, want %s", got, sha)
	82	}
	83	if !slices.Equal(*asked, []string{"pulls.test"}) {
	84	t.Fatalf("looked up %v", *asked)
	85	}
	86	}
	87
	88	// A git host that resolves to loopback is refused on a default
	89	// instance; the fetch reports it and fetches nothing.
	90	func TestFetchPullHeadsRefusesALocalAddress(t *testing.T) {
	91	port, _ := pullUpstream(t)
	92	c, errOut, _, root := importCtx(t, false)
	93	stubLookup(t, "127.0.0.1")
	94	dir := filepath.Join(root, "dest.git")
	95	if err := gitutil.InitBare(dir, "main", ""); err != nil {
	96	t.Fatal(err)
	97	}
	98	if fetchPullHeads(c, dir, "http://pulls.test:"+port+"/o/r.git", "sekrit") {
	99	t.Fatal("fetched from a local address")
	100	}
	101	if !strings.Contains(errOut.String(), "private or local address") \|\| strings.Contains(errOut.String(), "sekrit") {
	102	t.Fatalf("stderr %q", errOut.String())
	103	}
	104	if refExists(dir, "refs/gh-pull/1") {
	105	t.Fatal("refs/gh-pull/1 was fetched")
	106	}
	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	}
	179
	180	// --api-base takes a plain http or https URL: no credentials, query,
	181	// fragment or other scheme, and nothing of a refused one is echoed.
	182	func TestImportIssuesRefusesAnAPIBaseShape(t *testing.T) {
	183	for _, base := range []string{"https://abc@api.test", "https://api.test/?abc", "https://api.test/#abc", "ftp://api.test/abc"} {
	184	c, errOut, _, _ := importCtx(t, true)
	185	asked := stubLookup(t, "127.0.0.1")
	186	code := Dispatch(c, []string{"repo", "import-issues", "alice/app", "--from", "o/r", "--api-base", base})
	187	if code != protocol.ExitUsage \|\| len(*asked) != 0 \|\| strings.Contains(errOut.String(), "abc") {
	188	t.Fatalf("%s: exit %d, looked up %v: %s", base, code, *asked, errOut.String())
	189	}
	190	}
	191	}

internal/control/import.go +2 −1

	@@ -39,7 +39,8 @@ case "$1" in
39	esac	39	esac
40	`	40	`
41		41
42	// importLookup resolves an import's host; tests replace it.	42	// importLookup resolves the hosts repo import and repo import-issues
		43	// connect to; tests replace it.
43	var importLookup gitpin.Lookup = gitpin.LookupIP	44	var importLookup gitpin.Lookup = gitpin.LookupIP
44		45
45	func runRepoImport(c *Ctx, args []string) int {	46	func runRepoImport(c *Ctx, args []string) int {

internal/gitpin/gitpin.go +22 −1

	@@ -1,6 +1,7 @@
1	// Package gitpin runs git against a user-supplied http or https remote	1	// Package gitpin runs git against a user-supplied http or https remote
2	// only at addresses resolved and checked immediately before: mirror	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	package gitpin	5	package gitpin
5		6
6	import (	7	import (
	@@ -11,6 +12,7 @@ import (
11	"os/exec"	12	"os/exec"
12	"strconv"	13	"strconv"
13	"strings"	14	"strings"
		15	"time"
14		16
15	"gitbay.org/gitbay/internal/toolpath"	17	"gitbay.org/gitbay/internal/toolpath"
16	"gitbay.org/gitbay/internal/webhook"	18	"gitbay.org/gitbay/internal/webhook"
	@@ -63,6 +65,25 @@ func Resolve(ctx context.Context, lookup Lookup, raw string, allowLocal bool) (R
63	return Remote{URL: u, IPs: ips}, nil	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	// CheckHost refuses a host written as a number in a form other than	87	// CheckHost refuses a host written as a number in a form other than
67	// an IP literal: 127.1, 2130706433 and 0x7f.1 are loopback to curl's	88	// an IP literal: 127.1, 2130706433 and 0x7f.1 are loopback to curl's
68	// parser but not to Go's, so they are refused rather than left to a	89	// parser but not to Go's, so they are refused rather than left to a

internal/gitutil/gitutil.go +6 −5

 //
 // One fetch for every pull request rather than one each: the ref count is
 // the repository's history, and asking a hundred times is a hundred
-// handshakes. extraEnv carries credentials via GIT_ASKPASS; the URL must
+// handshakes. pin and env are as for FetchMirror; env carries
-// never contain them.
+// credentials via GIT_ASKPASS, and the URL must never contain them.
-func FetchPullHeads(ctx context.Context, dir, url string, errW io.Writer, extraEnv []string) error {
+func FetchPullHeads(ctx context.Context, dir, url string, errW io.Writer, pin, env []string) error {
-	cmd := exec.CommandContext(ctx, toolpath.Look("git"), "-C", dir, "fetch", "--no-write-fetch-head", "--no-tags",
+	args := append(append([]string{}, pin...), "-C", dir, "fetch", "--no-write-fetch-head", "--no-tags",
 		url, "+refs/pull/*/head:refs/gh-pull/*")
-	cmd.Env = append(os.Environ(), extraEnv...)
+	cmd := exec.CommandContext(ctx, toolpath.Look("git"), args...)
+	cmd.Env = env
 	cmd.Stderr = errW
 	if err := cmd.Run(); err != nil {
 		return fmt.Errorf("fetch pull heads from %s: %w", url, err)