Say how to allow a refused private target address (closes #398)

Adding or editing an http or slack target whose address is private or
reserved was refused with no hint that the refusal is deliberate or
that it can be lifted. The refusal now adds that such addresses are
refused by default and that the server's ALLOWED_EGRESS_CIDRS setting
allows named networks, naming the README section "Allowing egress to
your own network". Metadata refusals do not get it.

The default blocklist's public addresses move to a list of their own,
still checked after the allowlist, and are refused as cloud metadata
addresses. The private-and-reserved error is exported as
ErrBlockedPrivateOrReservedIP so the handler can tell them apart.

Model: opus-5-5

.dockerignore

@@ -1,18 +1,14 @@
-# .git is sent, without its config, so the build can derive the version it
-# stamps into the binary (script/version). The config can hold a remote URL
-# carrying a credential, and `git describe` does not need it.
-.git/config
-
-# No tracked file may be listed here: git in the build would see it as
-# deleted and mark the version -dirty.
-#
 # .ci-fingerprint is deliberately NOT excluded: it is the CI cache barrier
 # that keeps the check stages from replaying a cached pass. See the lint
 # stage of the Dockerfile.
+.git/
 bin/
 # Extracted from 3p/ by `make assets` inside the build; a host copy is not
 # needed. The tarball in 3p/ must stay in the context.
 static/js/alpine.min.js
+*.md
+LICENSE
+.editorconfig
 .env
 .env.*
 *.db

.gitea/workflows/check.yml

@@ -12,8 +12,9 @@ jobs:
       - name: Checkout
         uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 2024-10-23
         with:
-          # The superseded-status step needs history to walk ancestors (it
+          # The fingerprint step below needs history to find the last commit
-          # aborts on a shallow clone).
+          # that touched the Docker build context, and the superseded-status
+          # step needs it to walk ancestors (it aborts on a shallow clone).
           fetch-depth: 0
 
       - name: Mark superseded run statuses
@@ -27,11 +28,16 @@ jobs:
         run: script/ci-mark-superseded
 
       - name: Fingerprint the build context
-        # Writes the hash of the commit being checked into the context, which
+        # `.dockerignore` keeps docs out of the build context, so a docs-only
-        # invalidates the `COPY . .` layer of both check stages: a commit
+        # commit legitimately replays the whole image from cache and stays
-        # that was never linted, format-checked, tested and built cannot
+        # cheap. Every other commit writes a new fingerprint into the context,
-        # report success from cache.
+        # which invalidates the `COPY . .` layer of both check stages: a
-        run: git rev-parse HEAD > .ci-fingerprint
+        # commit that was never linted, formatted-checked, tested and built
+        # cannot report success from cache.
+        run: |
+          set -eu
+          fp="$(git log -1 --format=%H -- . ':!*.md' ':!LICENSE' ':!.editorconfig')"
+          printf '%s\n' "${fp:-$GITHUB_SHA}" > .ci-fingerprint
 
       - name: Build Docker image (runs make check)
         run: script/cibuild

Dockerfile

@@ -12,8 +12,8 @@ WORKDIR /src
 COPY go.mod go.sum ./
 RUN go mod download
 
-# Copy source code. In CI the context also carries .ci-fingerprint, which
+# Copy source code. In CI the context also carries .ci-fingerprint, whose
-# holds the hash of the commit being checked (see
+# value changes with every commit that touches the build context (see
 # .gitea/workflows/check.yml). That invalidates this layer, so the checks
 # below cannot report success by replaying a cached pass. Do not add it to
 # .dockerignore.
@@ -38,13 +38,8 @@ FROM golang:1.26.1-bookworm@sha256:4465644228bc2857a954b092167e12aa59c006a349228
 COPY --from=lint /src/go.sum /dev/null
 
 # jq is a runtime dependency of script/ci-mark-superseded, which the test
-# suite executes. git is what script/version derives the version with.
+# suite executes.
-RUN apt-get update && apt-get install -y --no-install-recommends make curl ca-certificates jq git && rm -rf /var/lib/apt/lists/*
+RUN apt-get update && apt-get install -y --no-install-recommends make curl ca-certificates jq && rm -rf /var/lib/apt/lists/*
-
-# A build context sent as a tar archive keeps its files' owners, and git
-# refuses to read a checkout owned by another user. Trust this one
-# whoever owns it.
-RUN git config --system --add safe.directory /build
 
 WORKDIR /build
 
@@ -60,22 +55,14 @@ COPY . .
 # from its tarball in 3p/.
 RUN make test
 
-# Version stamped into the binary: the VERSION build arg when one is
+# Version stamped into the binary. .dockerignore excludes .git/, so
-# given, otherwise what script/version derives from the .git the build
+# nothing in this stage can derive it: script/docker resolves it on the
-# context carries, so any `docker build .` of a clone stamps its commit.
+# host and passes it in. The default is what a bare `docker build .`
-# With neither, as from a source tarball, it is "unknown".
+# with no --build-arg gets, and it names no tag the tree may not be at.
 #
 # Declared here, below the test step, so a changed version does not
 # invalidate its cached layer.
-ARG VERSION
+ARG VERSION=unknown
-
-# A context that carries .git must not stamp "unknown": that means git is
-# missing here or could not read the checkout, and the image could not be
-# traced back to its commit.
-RUN if [ -d .git ] && [ "$(make version VERSION="$VERSION")" = unknown ]; then \
-        echo "version is unknown although the build context carries .git" >&2; \
-        exit 1; \
-    fi
 
 RUN make build VERSION="$VERSION"
 

Makefile

@@ -4,12 +4,12 @@
 .DEFAULT_GOAL := check
 
 # Version stamped into the binary. Derived from git by script/version;
-# override it (`make build VERSION=v1.2.3`) to stamp a given value, which is
+# override it (`make build VERSION=v1.2.3`) where git metadata is
-# how the Dockerfile passes its build arg in.
+# unavailable, which is how the Dockerfile passes its build arg in.
 VERSION ?= $(shell script/version)
 
-# An empty override (`make build VERSION=`, or the Dockerfile's `make build
+# An empty override (`make build VERSION=`, or a `--build-arg VERSION=`
-# VERSION="$VERSION"` when no VERSION build arg was given) means unset,
+# landing on the Dockerfile's `make build VERSION="$VERSION"`) means unset,
 # exactly as it does in script/version -- stamping "" would leave the binary
 # reporting no version and the footer back on its "dev" fallback. `override`
 # is required: a plain assignment loses to the command-line definition it

README.md

@@ -1133,26 +1133,13 @@ build itself.
 | Uncommitted changes | the above with a `-dirty` suffix |
 | No git metadata | `unknown` |
 
-The image derives it the same way, from the `.git` that the build
+`unknown` is what a source tarball or a `docker build .` with no
-context carries, so any `docker build .` of a clone, with no build
+`--build-arg VERSION=...` reports. `.dockerignore` excludes `.git/`, so
-arguments, stamps the commit it was built from; a shallow clone of one
+the build context carries no git metadata and the image cannot derive
-branch has no tags and stamps the short SHA. `.dockerignore` must
+the version itself: `script/docker` (and so `make docker`) resolves it
-therefore leave out neither `.git` nor any tracked file, which git in
+on the host and passes it in as the `VERSION` build arg. A build that
-the build would see as deleted, marking the version `-dirty`. It does
+reports `unknown` is a build nobody told what it was; it is not a
-leave out `.git/config`, which can hold a remote URL carrying a
+failure, but it cannot be traced back to a commit.
-credential and which `git describe` does not need. git in the build
-reads the checkout whoever owns its files, since a context sent as a tar
-archive keeps the sender's owners and git otherwise refuses a checkout
-owned by another user. A `VERSION` build arg (`--build-arg VERSION=...`)
-takes precedence; `script/docker` (and so `make docker`) passes the one
-`script/version` resolves on the host. The image build fails if its
-context carries `.git` and the version still comes out `unknown`, which
-means git is missing from the build or could not read the checkout.
-
-`unknown` is what a source tarball, or a `docker build` with no `.git`
-in its context and no `VERSION` build arg, reports. A build that reports
-`unknown` is a build nobody told what it was; it is not a failure, but
-it cannot be traced back to a commit.
 
 `make version` prints what the current checkout would stamp, and
 `make build VERSION=v1.2.3` overrides it. An empty override — from
@@ -3187,9 +3174,8 @@ version is fixed independently of the compiler's:
    rebuilds the binary with `CGO_ENABLED=1` and static linking so it
    runs on musl. Both builds go through `make build`, the relink adding
    its `-extldflags` via `GO_LDFLAGS`, so neither can drop the `-X` that
-   stamps the version. The version is the `VERSION` build arg if one is
+   stamps the version. The version arrives as the `VERSION` build arg,
-   given, otherwise derived from the `.git` in the context, and the
+   since the context has no `.git` (see
-   stage fails if a context with `.git` would stamp `unknown` (see
    [Version stamping](#version-stamping)).
 3. **Runtime stage** (`alpine:3.21`) — copies the static binary and
    `deploy/docker-entrypoint.sh`, creates the `/var/lib/webhooker`
@@ -3221,13 +3207,19 @@ A layer cache lets `docker build .` exit 0 in seconds with the lint and
 test stages replayed rather than executed, which would make a green
 check meaningless. The `check` workflow therefore writes
 `.ci-fingerprint` into the build context before building. Its value is
-the hash of the commit being checked, so every commit, docs-only ones
+the hash of the last commit that touched the build context, so:
-and a squash merge whose tree matches an already-built branch included,
-gets a new fingerprint, invalidates the `COPY . .` layer of both check
-stages, and really runs `make fmt-check`, `golangci-lint`, `make test`,
-and `make build`. A run that reports success ran them.
 
-The module download layer sits above `COPY . .` and stays cached.
+- Any commit that changes code (including a squash merge whose tree
+  matches an already-built branch) gets a new fingerprint, invalidates
+  the `COPY . .` layer of both check stages, and really runs
+  `make fmt-check`, `golangci-lint`, `make test`, and `make build`. A
+  run that reports success ran them.
+- A docs-only commit leaves the fingerprint unchanged — `.dockerignore`
+  excludes `*.md`, `LICENSE` and `.editorconfig` from the context
+  anyway — so the image replays from cache and costs seconds.
+
+The module download layer sits above `COPY . .` and stays cached either
+way.
 
 A separate workflow step, run before the fingerprint is written, covers
 a second way the gate lied: Gitea cancels an in-flight run when a newer

internal/delivery/export_test.go

@@ -40,11 +40,6 @@ const (
 	ExportPendingSweepMinAge = pendingSweepMinAge
 )
 
-// ExportIsBlockedIP exposes isBlockedIP for testing.
-func ExportIsBlockedIP(ip net.IP) bool {
-	return isBlockedIP(ip)
-}
-
 // NewTestGuard builds an SSRF Guard from an explicit egress
 // allowlist, without going through config. Passing no prefixes
 // yields the default guard, which blocks every private/reserved
@@ -70,6 +65,11 @@ func ExportBlockedNetworks() []*net.IPNet {
 	return blockedNetworks
 }
 
+// ExportBlockedPublicNetworks exposes blockedPublicNetworks.
+func ExportBlockedPublicNetworks() []*net.IPNet {
+	return blockedPublicNetworks
+}
+
 // ExportIsForwardableHeader exposes isForwardableHeader.
 func ExportIsForwardableHeader(name string) bool {
 	return isForwardableHeader(name)

internal/delivery/ssrf.go

@@ -25,8 +25,16 @@ var (
 	errNoIPs      = errors.New(
 		"hostname resolved to no IP addresses",
 	)
-	errBlockedIP = errors.New(
+	// ErrBlockedPrivateOrReservedIP reports an address in the
-		"blocked private, reserved or cloud metadata address",
+	// default blocklist's private and reserved ranges,
+	// blockedNetworks.
+	ErrBlockedPrivateOrReservedIP = errors.New(
+		"blocked private or reserved address",
+	)
+	// errBlockedPublicMetadata reports a public address on the
+	// default blocklist, one in blockedPublicNetworks.
+	errBlockedPublicMetadata = errors.New(
+		"blocked cloud metadata address",
 	)
 	errBlockedMetadata = errors.New(
 		"blocked link-local or cloud instance metadata " +
@@ -37,22 +45,32 @@ var (
 	)
 )
 
-// blockedNetworks is the default blocklist: the private and
+// blockedNetworks and blockedPublicNetworks together are the
-// reserved IP ranges, plus the public cloud metadata addresses,
+// default blocklist: the private and reserved IP ranges, plus
-// that are blocked to prevent SSRF attacks. An operator can
+// the public cloud metadata addresses, that are blocked to
-// permit specific blocks out of this set with
+// prevent SSRF attacks. An operator can permit specific blocks
-// ALLOWED_EGRESS_CIDRS; see Guard.
+// out of this set with ALLOWED_EGRESS_CIDRS; see Guard.
 //
-// A public address belongs on the default blocklist only if it
+// blockedNetworks holds the private and reserved IP ranges.
-// hands credentials, user data or bootstrap material to whatever
-// can reach it, without the caller presenting anything. A
-// provider's other public addresses are not refused, since
-// reaching them can be legitimate and no list of them could be
-// complete.
 //
 //nolint:gochecknoglobals // package-level network list is appropriate here
 var blockedNetworks []*net.IPNet
 
+// blockedPublicNetworks holds the default blocklist's public
+// addresses, kept apart from blockedNetworks so that they are
+// refused as cloud metadata addresses, never as private or
+// reserved ones.
+//
+// A public address belongs on the default blocklist only if it
+// hands credentials, user data or bootstrap material to whatever
+// can reach it, without the caller presenting anything; it goes
+// in this list. A provider's other public addresses are not
+// refused, since reaching them can be legitimate and no list of
+// them could be complete.
+//
+//nolint:gochecknoglobals // package-level network list is appropriate here
+var blockedPublicNetworks []*net.IPNet
+
 // alwaysBlockedNetworks are the ranges no configuration can
 // open: the link-local blocks and the cloud instance metadata
 // endpoints that live outside them. Reaching one is credential
@@ -88,8 +106,8 @@ var blockedNetworks []*net.IPNet
 // when it clears both halves. Nothing in this list can be
 // reopened, so putting a public address here leaves the operator
 // no escape hatch at all — the condition ALLOWED_EGRESS_CIDRS
-// exists to remove. Default-block it in blockedNetworks instead,
+// exists to remove. Default-block it in blockedPublicNetworks
-// which an allowlist can override.
+// instead, which an allowlist can override.
 //
 // This is a criterion, not an enumeration of every metadata
 // address in existence.
@@ -130,6 +148,9 @@ func init() {
 		"::1/128",
 		"fc00::/7",
 		"fe80::/10",
+	})
+
+	blockedPublicNetworks = mustParseCIDRs([]string{
 		// Azure WireServer, a public address that serves VM credentials.
 		"168.63.129.16/32",
 	})
@@ -225,13 +246,6 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool {
 	return false
 }
 
-// isBlockedIP checks whether an IP address falls within
-// the default blocklist, before any operator allowlist is
-// considered.
-func isBlockedIP(ip net.IP) bool {
-	return matchesAny(blockedNetworks, ip)
-}
-
 // Guard makes every SSRF decision in the process.
 //
 // It holds the operator's ALLOWED_EGRESS_CIDRS allowlist and
@@ -332,7 +346,8 @@ func (g *Guard) allows(ip net.IP) bool {
 //     consulted, so no configured CIDR reaches link-local or a
 //     cloud metadata endpoint at a non-public address.
 //  2. The allowlist is consulted next, so a listed private
-//     network becomes reachable.
+//     network, or a listed public address on the default
+//     blocklist, becomes reachable.
 //  3. Everything else keeps the default blocklist's answer.
 func (g *Guard) checkIP(ip net.IP) error {
 	if matchesAny(alwaysBlockedNetworks, ip) {
@@ -345,9 +360,15 @@ func (g *Guard) checkIP(ip net.IP) error {
 		return nil
 	}
 
-	if isBlockedIP(ip) {
+	if matchesAny(blockedNetworks, ip) {
 		return fmt.Errorf(
-			"target IP %s: %w", ip, errBlockedIP,
+			"target IP %s: %w", ip, ErrBlockedPrivateOrReservedIP,
+		)
+	}
+
+	if matchesAny(blockedPublicNetworks, ip) {
+		return fmt.Errorf(
+			"target IP %s: %w", ip, errBlockedPublicMetadata,
 		)
 	}
 

internal/delivery/ssrf_allowlist_test.go

@@ -23,6 +23,10 @@ const (
 	metadataIP  = "169.254.169.254"
 	metadataURL = "http://" + metadataIP + "/latest/meta-data/"
 
+	// linkLocalIPv4 is the IPv4 link-local block, which holds
+	// metadataIP.
+	linkLocalIPv4 = "169.254.0.0/16"
+
 	// loopbackHookURL is a target on this host: blocked by
 	// default, reachable only once an operator allowlists
 	// loopback.
@@ -237,7 +241,7 @@ func linkLocalRefusedCases() []metadataAlwaysRefusedCase {
 		},
 		{
 			name:   "whole link-local block",
-			allow:  "169.254.0.0/16",
+			allow:  linkLocalIPv4,
 			target: metadataURL,
 		},
 		{
@@ -412,6 +416,9 @@ func TestGuardAllowlist_AzureWireServerReopenable(t *testing.T) {
 		"WireServer must be refused by the default blocklist, "+
 			"which an allowlist can override",
 	)
+	require.NotErrorIs(t, err, delivery.ErrBlockedPrivateOrReservedIP,
+		"WireServer is public, not private or reserved",
+	)
 
 	assertDialRefused(t, defaultGuard, target)
 
@@ -496,7 +503,7 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) {
 	want := []string{
 		// IPv4 link-local: the 169.254.169.254 metadata
 		// service on AWS, Azure and others.
-		"169.254.0.0/16",
+		linkLocalIPv4,
 		// IPv6 link-local.
 		"fe80::/10",
 		// AWS IPv6 IMDS, inside the ULA space an operator may
@@ -526,6 +533,90 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) {
 	assert.Equal(t, want, got)
 }
 
+// TestDefaultBlocklist_PinnedSet pins each list of the default
+// blocklist on its own, the private and reserved ranges in
+// blockedNetworks and the public addresses in
+// blockedPublicNetworks, so moving an entry from one list to the
+// other fails it. For the first address of each entry it then
+// checks that the default guard refuses it, and that listing the
+// entry in ALLOWED_EGRESS_CIDRS opens it unless the unconditional
+// set holds that address.
+func TestDefaultBlocklist_PinnedSet(t *testing.T) {
+	t.Parallel()
+
+	// public marks an entry of blockedPublicNetworks; every other
+	// entry belongs in blockedNetworks.
+	tests := []struct {
+		cidr       string
+		public     bool
+		reopenable bool
+	}{
+		{cidr: "127.0.0.0/8", reopenable: true},
+		{cidr: "10.0.0.0/8", reopenable: true},
+		{cidr: "172.16.0.0/12", reopenable: true},
+		{cidr: "192.168.0.0/16", reopenable: true},
+		{cidr: linkLocalIPv4, reopenable: false},
+		{cidr: "0.0.0.0/8", reopenable: true},
+		{cidr: "100.64.0.0/10", reopenable: true},
+		{cidr: "192.0.0.0/24", reopenable: true},
+		{cidr: "192.0.2.0/24", reopenable: true},
+		{cidr: "198.18.0.0/15", reopenable: true},
+		{cidr: "198.51.100.0/24", reopenable: true},
+		{cidr: "203.0.113.0/24", reopenable: true},
+		{cidr: "224.0.0.0/4", reopenable: true},
+		{cidr: "240.0.0.0/4", reopenable: true},
+		{cidr: "::1/128", reopenable: true},
+		{cidr: "fc00::/7", reopenable: true},
+		{cidr: "fe80::/10", reopenable: false},
+		{cidr: "168.63.129.16/32", public: true, reopenable: true},
+	}
+
+	wantPrivate := make([]string, 0, len(tests))
+	wantPublic := make([]string, 0, len(tests))
+
+	for _, tt := range tests {
+		if tt.public {
+			wantPublic = append(wantPublic, tt.cidr)
+		} else {
+			wantPrivate = append(wantPrivate, tt.cidr)
+		}
+	}
+
+	gotPrivate := make([]string, 0, len(tests))
+	for _, n := range delivery.ExportBlockedNetworks() {
+		gotPrivate = append(gotPrivate, n.String())
+	}
+
+	gotPublic := make([]string, 0, len(tests))
+	for _, n := range delivery.ExportBlockedPublicNetworks() {
+		gotPublic = append(gotPublic, n.String())
+	}
+
+	assert.ElementsMatch(t, wantPrivate, gotPrivate, "blockedNetworks")
+	assert.ElementsMatch(t, wantPublic, gotPublic, "blockedPublicNetworks")
+
+	for _, tt := range tests {
+		t.Run(tt.cidr, func(t *testing.T) {
+			t.Parallel()
+
+			prefix := netip.MustParsePrefix(tt.cidr)
+			ip := net.IP(prefix.Addr().AsSlice())
+
+			require.Error(t,
+				delivery.NewTestGuard().ExportCheckIP(ip),
+				"the default guard must refuse %s", ip,
+			)
+
+			err := delivery.NewTestGuard(prefix).ExportCheckIP(ip)
+			if tt.reopenable {
+				assert.NoError(t, err, "listing %s must open it", tt.cidr)
+			} else {
+				assert.Error(t, err, "listing %s must not open it", tt.cidr)
+			}
+		})
+	}
+}
+
 // requireLoopback fails the test unless rawURL's host is a
 // loopback address, so the allowlist test cannot silently stop
 // exercising a blocked range.

internal/delivery/ssrf_test.go

@@ -10,7 +10,7 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-func TestIsBlockedIP_PrivateRanges(t *testing.T) {
+func TestGuardCheckIP_PrivateRanges(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
@@ -56,12 +56,14 @@ func TestIsBlockedIP_PrivateRanges(t *testing.T) {
 				"failed to parse IP %s", tt.ip,
 			)
 
+			refused := delivery.NewTestGuard().ExportCheckIP(ip) != nil
+
 			assert.Equal(t,
 				tt.blocked,
-				delivery.ExportIsBlockedIP(ip),
+				refused,
-				"isBlockedIP(%s) = %v, want %v",
+				"default guard refuses %s = %v, want %v",
 				tt.ip,
-				delivery.ExportIsBlockedIP(ip),
+				refused,
 				tt.blocked,
 			)
 		})

internal/handlers/source_management.go

@@ -1577,11 +1577,22 @@ func (h *Handlers) validateTargetURL(
 			"url", delivery.MaskURL(targetURL),
 			"error", err,
 		)
-		http.Error(
+
-			w,
+		msg := "Invalid target URL: " + err.Error()
-			"Invalid target URL: "+err.Error(),
+
-			http.StatusBadRequest,
+		// Only a private or reserved address's refusal says how
-		)
+		// to allow it. Metadata refusals never do: link-local and
+		// the other unconditional metadata addresses cannot be
+		// opened, and the default blocklist's public addresses,
+		// which listing does open, hand out credentials.
+		if errors.Is(err, delivery.ErrBlockedPrivateOrReservedIP) {
+			msg += ". Private and reserved addresses are refused " +
+				"by default; the server's ALLOWED_EGRESS_CIDRS " +
+				"setting allows named networks (see \"Allowing " +
+				"egress to your own network\" in the README)."
+		}
+
+		http.Error(w, msg, http.StatusBadRequest)
 
 		return err
 	}

internal/handlers/target_private_refusal_test.go

@@ -0,0 +1,116 @@
+package handlers_test
+
+import (
+	"net/http"
+	"net/url"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// privateRefusalHint is the sentence that tells an operator a private
+// destination is refused on purpose, and how to allow one.
+const privateRefusalHint = "Private and reserved addresses are " +
+	"refused by default; the server's ALLOWED_EGRESS_CIDRS setting " +
+	"allows named networks (see \"Allowing egress to your own " +
+	"network\" in the README)."
+
+// TestTargetRefusal_PrivateDestinationSaysHowToAllowIt covers both
+// target types that take a URL, on add and on edit.
+func TestTargetRefusal_PrivateDestinationSaysHowToAllowIt(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	targetTypes := []database.TargetType{
+		database.TargetTypeHTTP,
+		database.TargetTypeSlack,
+	}
+
+	for _, targetType := range targetTypes {
+		t.Run(string(targetType), func(t *testing.T) {
+			t.Parallel()
+
+			webhook := seedWebhookWithRetention(t, env.db, 30)
+			targetsPath := "/source/" + webhook.ID + "/targets"
+
+			form := url.Values{}
+			form.Set("name", "private")
+			form.Set("type", string(targetType))
+			form.Set("url", editBlockedURL)
+
+			added := serveTarget(
+				env, http.MethodPost, targetsPath, form,
+			)
+			assert.Equal(t, http.StatusBadRequest, added.Code)
+			assert.Contains(
+				t, added.Body.String(), privateRefusalHint,
+			)
+
+			form.Set("url", editOriginalURL)
+
+			created := serveTarget(
+				env, http.MethodPost, targetsPath, form,
+			)
+			require.Equal(
+				t, http.StatusSeeOther, created.Code,
+				created.Body.String(),
+			)
+
+			targets := targetsForWebhook(t, env.db, webhook.ID)
+			require.Len(t, targets, 1)
+
+			form.Set("url", editBlockedURL)
+
+			edited := submitTargetEdit(
+				env, webhook.ID, targets[0].ID, form,
+			)
+			assert.Equal(t, http.StatusBadRequest, edited.Code)
+			assert.Contains(
+				t, edited.Body.String(), privateRefusalHint,
+			)
+		})
+	}
+}
+
+// TestTargetRefusal_MetadataDestinationDoesNotSayHowToAllowIt: no
+// setting opens a link-local address, and Azure's WireServer hands out
+// VM credentials, so neither refusal points at the setting.
+func TestTargetRefusal_MetadataDestinationDoesNotSayHowToAllowIt(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	metadataURLs := map[string]string{
+		"link-local": "http://169.254.169.254/latest/meta-data/",
+		"wireserver": "http://168.63.129.16/?comp=versions",
+	}
+
+	for name, metadataURL := range metadataURLs {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			webhook := seedWebhookWithRetention(t, env.db, 30)
+
+			form := url.Values{}
+			form.Set("name", "metadata")
+			form.Set("type", string(database.TargetTypeHTTP))
+			form.Set("url", metadataURL)
+
+			w := serveTarget(
+				env, http.MethodPost,
+				"/source/"+webhook.ID+"/targets", form,
+			)
+			assert.Equal(t, http.StatusBadRequest, w.Code)
+			assert.NotContains(
+				t, w.Body.String(), privateRefusalHint,
+			)
+		})
+	}
+}

internal/versionscript/version_script_test.go

@@ -115,8 +115,8 @@ func TestVersion_EnclosingRepositoryIsNotUsed(t *testing.T) {
 	require.Equal(t, unknown, runScript(t, inner, nil))
 }
 
-// An explicit VERSION, such as the Dockerfile's build arg, wins over
+// The Docker build has no git metadata, so the version arrives as an
-// anything derivable.
+// environment override. It wins over anything derivable.
 func TestVersion_EnvironmentOverrideWins(t *testing.T) {
 	t.Parallel()
 
@@ -128,8 +128,8 @@ func TestVersion_EnvironmentOverrideWins(t *testing.T) {
 }
 
 // An empty VERSION is treated as unset rather than stamping an empty
-// string: a caller exporting VERSION= must not produce a binary
+// string: the Dockerfile's build arg has a non-empty default, but a
-// reporting "".
+// caller exporting VERSION= must not produce a binary reporting "".
 func TestVersion_EmptyOverrideFallsBackToGit(t *testing.T) {
 	t.Parallel()
 
@@ -168,8 +168,8 @@ func TestMakefile_BuildComposesVersionAndExtraFlags(t *testing.T) {
 }
 
 // A caller can define VERSION as the empty string -- `make build
-// VERSION=`, or the Dockerfile's `make build VERSION="$VERSION"` when no
+// VERSION=`, or a `--build-arg VERSION=` reaching the Dockerfile's `make
-// VERSION build arg was given. script/version's own guard does not cover
+// build VERSION="$VERSION"`. script/version's own guard does not cover
 // that: the value never passes through the script. Stamping "" would
 // leave the binary reporting no version and the footer on "dev", which
 // is the defect this package exists for.
@@ -231,7 +231,7 @@ func TestDockerfile_BuildsThroughTheMakeTarget(t *testing.T) {
 
 	require.NotContains(t, dockerfile, "go build",
 		"a raw go build bypasses the Makefile's -X flag")
-	require.Contains(t, dockerfile, "ARG VERSION")
+	require.Contains(t, dockerfile, "ARG VERSION=")
 	require.Contains(t, dockerfile,
 		`make build VERSION="$VERSION" GO_LDFLAGS='-extldflags "-static"'`)
 }

script/docker

@@ -2,9 +2,9 @@
 # script/docker: build the Docker image tagged with the project name.
 # The tag comes from script/projectname.
 #
-# The version script/version resolves here goes in as the VERSION build
+# .dockerignore excludes .git/, so the builder stage cannot derive the
-# arg, which takes precedence over what the build would derive from the
+# version itself. It is resolved here, where the checkout is, and passed
-# .git in its context.
+# in as a build arg; without it the image would stamp itself "unknown".
 set -eu
 
 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"

script/version

@@ -7,16 +7,18 @@
 #
 # Order of precedence:
 #
-#   1. $VERSION, if set and non-empty: an explicit value, such as the
+#   1. $VERSION, if set and non-empty. This is how the value reaches a
-#      Dockerfile's VERSION build arg.
+#      build that cannot derive it: .dockerignore excludes .git/, so the
+#      builder stage has no git metadata and the Dockerfile takes the
+#      value as a build arg instead.
 #   2. `git describe --tags --always --dirty` against this checkout. At
 #      a clean tagged commit that is exactly the tag; otherwise it
 #      carries the short SHA, the commit distance when a tag is
 #      reachable, and a -dirty suffix for uncommitted changes.
 #   3. "unknown", for a tree with no git metadata and no $VERSION -- a
-#      source tarball, or a `docker build` with no .git in its context
+#      source tarball, or `docker build .` with no --build-arg. That
-#      and no VERSION build arg. That case must not fail the build and
+#      case must not fail the build and must not name a tag the tree may
-#      must not name a tag the tree may not be at, so it names nothing.
+#      not be at, so it names nothing.
 #
 # The git step insists the enclosing repository is this checkout, not
 # merely some repository above it: an unpacked tarball sitting inside an