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	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	79	\| Webhook payload integrity \| in place \| HMAC-SHA256 header \|
80	80	\| SMTP credentials protected in transit \| in place \| STARTTLS required for non-local relays, implicit TLS optional (=internal/mail/mail.go=) \|
81	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	13	\| #259 \| Recovery \| No restore has been exercised; the drill is written (Admin wiki) and not yet run \| high \|
14	14	\| #260 \| CI network \| Builds share the runner's source address; no egress policy \| medium \|
15	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	17	* Not filed
19	18

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

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

CHANGELOG.org +7

		@@ -19,6 +19,13 @@ anything beyond "replace the binary and restart" is needed.
19	19	global gitconfig are ignored. A source that redirects (a renamed
20	20	repository) fails; import from the URL it redirects to. Needs git
21	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	29	- =repo mirror add= refuses a host written numerically (=127.1=,
23	30	=2130706433=, =0x7f.1=) when the mirror is added, rather than at its
24	31	first sync (#298).

internal/control/ghimport.go +49 −11

		@@ -14,11 +14,11 @@ import (
14	14	"strings"
15	15	"time"
16	16
	17	"gitbay.org/gitbay/internal/gitpin"
17	18	"gitbay.org/gitbay/internal/gitutil"
18	19	"gitbay.org/gitbay/internal/policy"
19	20	"gitbay.org/gitbay/internal/protocol"
20	21	"gitbay.org/gitbay/internal/store"
21		"gitbay.org/gitbay/internal/webhook"
22	22	)
23	23
24	24	func init() {
		@@ -134,6 +134,20 @@ func (g *ghClient) get(path string, out any) error {
134	134	return json.NewDecoder(resp.Body).Decode(out)
135	135	}
136	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. Each import builds its own
	140	// client, so connections are not kept for reuse.
	141	func pinnedClient(api gitpin.Remote) *http.Client {
	142	return &http.Client{
	143	Timeout: 30 * time.Second,
	144	Transport: &http.Transport{Proxy: nil, DialContext: api.DialContext, TLSHandshakeTimeout: 10 * time.Second, DisableKeepAlives: true},
	145	CheckRedirect: func(req http.Request, _ []http.Request) error {
	146	return fmt.Errorf("refusing redirect to %s://%s; a renamed repository is imported under its new name", req.URL.Scheme, req.URL.Host)
	147	},
	148	}
	149	}
	150
137	151	func ghDate(iso string) string {
138	152	if t, err := time.Parse(time.RFC3339, iso); err == nil {
139	153	return t.UTC().Format("2006-01-02")
		@@ -157,12 +171,25 @@ func runImportIssues(c *Ctx, args []string) int {
157	171	if path == "" \|\| from == "" {
158	172	return c.usage()
159	173	}
160		if apiBase == "" {
	174	given := apiBase != ""
	175	if !given {
161	176	apiBase = "https://api.github.com"
162		} else if err := webhook.ValidateURL(apiBase, c.Cfg.Webhooks.AllowLocal); err != nil {
163		// A writer-supplied API base is the same SSRF surface as a
164		// webhook target; same rules apply.
165		return c.fail(protocol.ExitUsage, "--api-base: %v", err)
	177	} else if strings.Contains(apiBase, "@") \|\| strings.ContainsAny(apiBase, "?#") {
	178	// Same rule as repo import: the URL is echoed and becomes the
	179	// site prefix, so credentials and queries stay out of it.
	180	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")
	181	}
	182	// A writer-supplied API base is the same SSRF surface as a webhook
	183	// target. Resolve and check it once here; the client connects only
	184	// to those addresses (#301).
	185	rctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	186	api, err := gitpin.Resolve(rctx, importLookup, apiBase, c.Cfg.Webhooks.AllowLocal)
	187	cancel()
	188	if err != nil {
	189	if given {
	190	return c.fail(protocol.ExitUsage, "--api-base: %v", err)
	191	}
	192	return c.fail(protocol.ExitFailure, "%v", err)
166	193	}
167	194	site := siteFromAPI(apiBase)
168	195	host := strings.TrimPrefix(strings.TrimPrefix(site, "https://"), "http://")
		@@ -190,7 +217,7 @@ func runImportIssues(c *Ctx, args []string) int {
190	217	}
191	218	token = strings.TrimSpace(line)
192	219	}
193		g := &ghClient{base: apiBase, token: token, http: &http.Client{Timeout: 30 * time.Second}}
	220	g := &ghClient{base: apiBase, token: token, http: pinnedClient(api)}
194	221	g.detect()
195	222	dir := RepoDir(c.Cfg.Server.Root, repo.OwnerName, repo.Name)
196	223
		@@ -361,8 +388,21 @@ esac
361	388	// Forgejo both publish pull heads under that name. Reports whether it
362	389	// worked; a failure is not fatal, since importing issues from a
363	390	// repository whose git data is not here yet is a reasonable thing to do.
	391	// git connects only to the addresses the remote's host resolved to and
	392	// passed the check here, as repo import does (#298, #301).
364	393	func fetchPullHeads(c *Ctx, dir, remote, token string) bool {
365		env := []string{"GIT_TERMINAL_PROMPT=0", "HOME=" + c.Cfg.Server.Root}
	394	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
	395	defer cancel()
	396	if err := gitpin.CheckGit(ctx); err != nil {
	397	fmt.Fprintf(c.Stderr, "pull heads not fetched: %v\n", err)
	398	return false
	399	}
	400	pinned, err := gitpin.Resolve(ctx, importLookup, remote, c.Cfg.Webhooks.AllowLocal)
	401	if err != nil {
	402	fmt.Fprintf(c.Stderr, "pull heads not fetched: %v\n", err)
	403	return false
	404	}
	405	env := gitpin.Env(c.Cfg.Server.Root)
366	406	if token != "" {
367	407	askpass := filepath.Join(c.Cfg.Server.Root, "gh-import-askpass.sh")
368	408	if err := os.WriteFile(askpass, []byte(ghAskpass), 0o700); err != nil {
		@@ -370,9 +410,7 @@ func fetchPullHeads(c *Ctx, dir, remote, token string) bool {
370	410	}
371	411	env = append(env, "GIT_ASKPASS="+askpass, "GITBAY_GH_TOKEN="+token)
372	412	}
373		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
374		defer cancel()
375		if err := gitutil.FetchPullHeads(ctx, dir, remote, io.Discard, env); err != nil {
	413	if err := gitutil.FetchPullHeads(ctx, dir, remote, io.Discard, pinned.Args(), env); err != nil {
376	414	return false
377	415	}
378	416	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	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	44	var importLookup gitpin.Lookup = gitpin.LookupIP
44	45
45	46	func runRepoImport(c *Ctx, args []string) int {

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

internal/gitutil/gitutil.go +6 −5

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