Name each database target's archive for its webhook and target (closes #376)

Each database target now has its own archive file,
archive-WEBHOOKNAME-TARGETNAME-TARGETID.db, instead of one
archive-WEBHOOKID.db per webhook. delivery.ArchiveFileName builds the
name: each name is lowercased, keeps ASCII letters and digits, turns
every other run of characters into one dash, and is cut to 40
characters.

A change of webhook or target name renames its archive files under the
archive writer's lock, before the new name is saved, and back again if
the save fails. A rename never replaces a file, and one that fails part
way moves back what it moved. Deleting a target evicts only that
target's writer. Archive files are never deleted, and nothing looks for
files under the old name.

Model: opus-5-5

README.md

@@ -158,19 +158,20 @@ WireServer, which serves an Azure VM its credentials. Because it is a
 public address, listing it in `ALLOWED_EGRESS_CIDRS` reopens it.
 
 That is all the default blocklist covers: the IPv4 private and reserved
-ranges; of IPv6, only loopback (`::1`), unique local addresses
+ranges; of IPv6, only loopback (`::1`), the unspecified address (`::`),
-(`fc00::/7`) and link-local addresses (`fe80::/10`); and certain public
+unique local addresses (`fc00::/7`), link-local addresses (`fe80::/10`),
-addresses. A public address belongs on the default blocklist only if it
+multicast (`ff00::/8`) and documentation space (`2001:db8::/32`); and
-hands credentials, user data or bootstrap material to whatever can reach
+certain public addresses. A public address belongs on the default
-it, without the caller presenting anything. A provider's other public
+blocklist only if it hands credentials, user data or bootstrap material
-addresses are not refused. IBM Cloud, for example, serves its package
+to whatever can reach it, without the caller presenting anything. A
-mirrors, time servers and object storage on `161.26.0.0/16`, and the
+provider's other public addresses are not refused. IBM Cloud, for
-private endpoints of its own cloud services on `166.8.0.0/14`. Neither
+example, serves its package mirrors, time servers and object storage on
-range hands out credentials that way: the token service among those
+`161.26.0.0/16`, and the private endpoints of its own cloud services on
-endpoints issues a token only in exchange for something the caller
+`166.8.0.0/14`. Neither range hands out credentials that way: the token
-presents, such as an API key. Reaching these services can be a
+service among those endpoints issues a token only in exchange for
-legitimate delivery, and every cloud has some, so a partial list would
+something the caller presents, such as an API key. Reaching these
-promise coverage it does not give.
+services can be a legitimate delivery, and every cloud has some, so a
+partial list would promise coverage it does not give.
 
 That default is also inconvenient for the thing webhooker is mostly
 for: taking a public webhook and forwarding it to something on your own
@@ -210,16 +211,16 @@ Two things this setting cannot do:
   the list is always an allowlist; an empty list (the default) means
   every private and reserved range stays refused. Note that
   `0.0.0.0/0` gets you most of the way there anyway, per above.
-- **It cannot open link-local, or a cloud metadata endpoint at a
+- **It cannot open link-local, the unspecified addresses, or a cloud
-  non-public address that discloses credentials or user data.** An
+  metadata endpoint at a non-public address that discloses credentials
-  address is on the list below when it is not a public address and both
+  or user data.** A metadata address is on the list below when it is not
-  of these hold: the provider fixes it, so it cannot collide with
+  a public address and both of these hold: the provider fixes it, so it
-  anything you run; and reaching it hands out credentials, user data or
+  cannot collide with anything you run; and reaching it hands out
-  bootstrap material. Those stay blocked no matter what you list,
+  credentials, user data or bootstrap material. Those stay blocked no
-  including when you list them outright or list a supernet such as
+  matter what you list, including when you list them outright or list a
-  `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best
+  supernet such as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`.
-  effort rather than a guarantee — it is a hand-maintained list and the
+  Treat this as best effort rather than a guarantee — it is a
-  caveat below the table applies:
+  hand-maintained list and the caveat below the table applies:
 
   | Blocked unconditionally | What it is |
   | ----------------------- | ---------- |
@@ -233,14 +234,25 @@ Two things this setting cannot do:
   | `fd00:a9fe:a9fe::1/128` | Linode/Akamai metadata over IPv6 |
   | `100.100.100.200/32` | Alibaba Cloud metadata, inside CGNAT |
   | `192.0.0.192/32` | Oracle Cloud Classic metadata |
+  | `0.0.0.0/32` | IPv4 unspecified address, which reaches this host's loopback on Linux |
+  | `::/128` | IPv6 unspecified address, which reaches this host's loopback on Linux |
   | `::a9fe:a9fe/128` | `169.254.169.254` as an IPv4-compatible IPv6 address |
   | `64:ff9b::a9fe:a9fe/128` | `169.254.169.254` behind the NAT64 well-known prefix |
 
   The IPv4-mapped form `::ffff:169.254.169.254` is covered by the
-  `169.254.0.0/16` entry. Reaching any of these is credential or
+  `169.254.0.0/16` entry. Reaching any of these but the two unspecified
-  user-data theft rather than delivery to an internal service. Every
+  addresses is credential or user-data theft rather than delivery to an
-  entry outside the two link-local blocks is a single address, so
+  internal service. Every entry outside the two link-local blocks is a
-  blocking it costs you nothing else on the network around it.
+  single address, so blocking it costs you nothing else on the network
+  around it.
+
+  The unspecified addresses `0.0.0.0` and `::` hand out nothing
+  themselves, but no host can have either, and on Linux a connection to
+  one reaches this host's own loopback. They are listed so that an
+  allowlist reaches loopback only through an entry that covers a loopback
+  address, such as `127.0.0.0/8`, `::1` or `0.0.0.0/0`, never through one
+  that covers only `0.0.0.0` or `::`; `0.0.0.0/8`, for example, does not
+  open loopback.
 
   The six ULA entries, all inside `fd00::/8`, are why this matters in
   practice: `fd00::/8` is an ordinary block to allowlist for your own
@@ -3123,7 +3135,8 @@ check, see [The login endpoint](#the-login-endpoint).
   route through a single decision function, so they cannot disagree
   about a destination. An operator can permit specific blocks with
   [`ALLOWED_EGRESS_CIDRS`](#allowing-egress-to-your-own-network); the
-  guard cannot be switched off, and link-local plus a
+  guard cannot be switched off, and link-local, the unspecified
+  addresses `0.0.0.0` and `::`, and a
   [pinned set](#allowing-egress-to-your-own-network) of known cloud
   metadata endpoints — several of which are ULAs outside link-local —
   stay blocked whatever is listed, though listing `0.0.0.0/0` or

internal/config/config.go

@@ -196,12 +196,13 @@ type Config struct {
 	// otherwise refuse. The guard itself is always on: there is no
 	// setting that disables SSRF protection, and delivery's
 	// alwaysBlockedNetworks stays blocked no matter what is listed
-	// here. That set is link-local plus the cloud metadata
+	// here. That set is link-local, the unspecified addresses
-	// endpoints outside it that disclose credentials or user data
+	// 0.0.0.0 and ::, and the cloud metadata endpoints outside
-	// at a provider-fixed, non-public address; it is not
+	// link-local that disclose credentials or user data at a
-	// exhaustive of every cloud's metadata address. See
+	// provider-fixed, non-public address; it is not exhaustive of
-	// alwaysBlockedNetworks for the authoritative list and the
+	// every cloud's metadata address. See
-	// criterion it is built from.
+	// alwaysBlockedNetworks for the authoritative list and why
+	// each entry is on it.
 	AllowedEgressCIDRs []netip.Prefix
 
 	params *ConfigParams

internal/config/config_test.go

@@ -124,6 +124,11 @@ func testEnvironmentConfigSuccess(
 
 	app := fxtest.New(
 		t,
+		// fx's own log is discarded, not sent to t.Logf: a hook still
+		// running after a start or stop timeout would write there after
+		// the test has returned. The same holds for every fxtest.New
+		// below.
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,
@@ -272,6 +277,7 @@ func testRetentionSweepIntervalSuccess(
 
 	app := fxtest.New(
 		t,
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,
@@ -364,6 +370,7 @@ func testSessionIdleTimeoutSuccess(
 
 	app := fxtest.New(
 		t,
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,
@@ -404,6 +411,7 @@ func TestDefaultDataDir(t *testing.T) {
 
 			app := fxtest.New(
 				t,
+				fx.NopLogger,
 				fx.Provide(
 					globals.New,
 					logger.New,
@@ -534,6 +542,7 @@ func testReceiverRateLimitSuccess(
 
 	app := fxtest.New(
 		t,
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,
@@ -650,6 +659,7 @@ func testTrustedProxiesSuccess(
 
 	app := fxtest.New(
 		t,
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,
@@ -763,6 +773,7 @@ func testAllowedEgressCIDRsSuccess(
 
 	app := fxtest.New(
 		t,
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,
@@ -1006,6 +1017,7 @@ func assertMetricsAuthAccepted(t *testing.T, expectAuth bool) {
 
 	app := fxtest.New(
 		t,
+		fx.NopLogger,
 		fx.Provide(globals.New, logger.New, config.New),
 		fx.Populate(&cfg),
 	)

internal/delivery/ssrf.go

@@ -37,8 +37,8 @@ var (
 		"blocked cloud metadata address",
 	)
 	errBlockedMetadata = errors.New(
-		"blocked link-local or cloud instance metadata " +
+		"blocked link-local, cloud instance metadata or " +
-			"address: ALLOWED_EGRESS_CIDRS cannot open it",
+			"unspecified address: ALLOWED_EGRESS_CIDRS cannot open it",
 	)
 	errInvalidScheme = errors.New(
 		"only http and https are allowed",
@@ -72,14 +72,17 @@ var blockedNetworks []*net.IPNet
 var blockedPublicNetworks []*net.IPNet
 
 // alwaysBlockedNetworks are the ranges no configuration can
-// open: the link-local blocks and the cloud instance metadata
+// open, so a supplied CIDR that covers one still leaves it
-// endpoints that live outside them. Reaching one is credential
+// blocked. An entry is here for one of two reasons: it is a
-// or user-data theft rather than delivery to an internal
+// metadata endpoint (the link-local blocks and the cloud
-// service, so a supplied CIDR that covers such an address still
+// instance metadata endpoints that live outside them), or it is
-// leaves it blocked.
+// an unspecified address. Reaching a metadata endpoint is
+// credential or user-data theft rather than delivery to an
+// internal service.
 //
-// Inclusion criterion — an address belongs here only if BOTH
+// Inclusion criterion for metadata endpoints — one belongs here
-// hold, and every entry below satisfies both:
+// only if BOTH hold, and every metadata entry below satisfies
+// both:
 //
 //  1. It is a fixed address assigned by the provider, or a
 //     range reserved by IANA — never one the operator chose.
@@ -90,8 +93,8 @@ var blockedPublicNetworks []*net.IPNet
 //     not cheaply rotated.
 //
 // Both halves are load-bearing, so use them to refuse a
-// candidate and say why. An endpoint disclosing only the
+// metadata candidate and say why. An endpoint disclosing only
-// operator's own inventory (instance id, region, disks, NICs)
+// the operator's own inventory (instance id, region, disks, NICs)
 // fails (2): letting a delivery target reach the operator's own
 // infrastructure is the feature ALLOWED_EGRESS_CIDRS exists to
 // provide. But (2) is not "IAM credentials only" either —
@@ -112,6 +115,15 @@ var blockedPublicNetworks []*net.IPNet
 // This is a criterion, not an enumeration of every metadata
 // address in existence.
 //
+// The unspecified addresses 0.0.0.0 and :: are here for a
+// separate reason: they disclose nothing, but no host can have
+// either, and on Linux a connection to one reaches this host's
+// own loopback. Listing them means an allowlist reaches loopback
+// only through an entry that covers a loopback address
+// (127.0.0.0/8, ::1/128, 0.0.0.0/0), never through one that
+// covers only 0.0.0.0 or :: (0.0.0.0/8, for example). Nothing
+// else lives at either address, so refusing them costs nothing.
+//
 // Every entry is either already in blockedNetworks — this list is
 // what makes it unconditional — or an alternate encoding of
 // 169.254.169.254 that Contains does not match against
@@ -131,23 +143,46 @@ var alwaysBlockedNetworks []*net.IPNet
 //nolint:gochecknoinits // init is the idiomatic way to parse CIDRs once at startup
 func init() {
 	blockedNetworks = mustParseCIDRs([]string{
+		// IPv4 loopback.
 		"127.0.0.0/8",
+		// RFC 1918 private network.
 		"10.0.0.0/8",
+		// RFC 1918 private network.
 		"172.16.0.0/12",
+		// RFC 1918 private network.
 		"192.168.0.0/16",
+		// IPv4 link-local.
 		"169.254.0.0/16",
+		// "This network", holding the IPv4 unspecified address 0.0.0.0.
 		"0.0.0.0/8",
+		// Carrier-grade NAT shared address space.
 		"100.64.0.0/10",
+		// IETF protocol assignments.
 		"192.0.0.0/24",
+		// IPv4 documentation (TEST-NET-1).
 		"192.0.2.0/24",
+		// Benchmarking.
 		"198.18.0.0/15",
+		// IPv4 documentation (TEST-NET-2).
 		"198.51.100.0/24",
+		// IPv4 documentation (TEST-NET-3).
 		"203.0.113.0/24",
+		// IPv4 multicast.
 		"224.0.0.0/4",
+		// Reserved, including the broadcast address.
 		"240.0.0.0/4",
+		// IPv6 loopback.
 		"::1/128",
+		// IPv6 unspecified address.
+		"::/128",
+		// IPv6 unique local addresses.
 		"fc00::/7",
+		// IPv6 link-local.
 		"fe80::/10",
+		// IPv6 multicast.
+		"ff00::/8",
+		// IPv6 documentation.
+		"2001:db8::/32",
 	})
 
 	blockedPublicNetworks = mustParseCIDRs([]string{
@@ -207,6 +242,14 @@ func init() {
 		// allowlist from opening it.
 		"192.0.0.192/32",
 
+		// The unspecified addresses, each of which reaches this
+		// host's loopback on Linux.
+		//
+		// IPv4 unspecified address, inside the blocked 0.0.0.0/8.
+		"0.0.0.0/32",
+		// IPv6 unspecified address.
+		"::/128",
+
 		// 169.254.169.254 as an IPv4-compatible IPv6 address.
 		"::a9fe:a9fe/128",
 		// 169.254.169.254 behind the NAT64 well-known prefix.
@@ -343,8 +386,9 @@ func (g *Guard) allows(ip net.IP) bool {
 // The order is the policy:
 //
 //  1. alwaysBlockedNetworks is refused before the allowlist is
-//     consulted, so no configured CIDR reaches link-local or a
+//     consulted, so no configured CIDR reaches link-local, a
-//     cloud metadata endpoint at a non-public address.
+//     cloud metadata endpoint at a non-public address, or an
+//     unspecified address.
 //  2. The allowlist is consulted next, so a listed private
 //     network, or a listed public address on the default
 //     blocklist, becomes reachable.

internal/delivery/ssrf_allowlist_test.go

@@ -168,12 +168,13 @@ func TestGuardAllowlist_UnlistedPrivateStillRefused(t *testing.T) {
 
 // TestGuardAllowlist_MetadataAlwaysRefused is the load-bearing
 // case: cloud instance metadata endpoints are credential theft
-// rather than delivery to an internal service, so no allowlist
+// rather than delivery to an internal service, and the
-// reaches one. Every guard below names a CIDR that covers its
+// unspecified addresses 0.0.0.0 and :: reach this host's loopback
-// target — including 0.0.0.0/0, ::/0, and the ordinary ULA and
+// on Linux, so no allowlist reaches any of them. Every guard
-// CGNAT blocks an operator would really list — and the address
+// below names a CIDR that covers its target — including
-// must stay refused anyway, on both the validation and the
+// 0.0.0.0/0, ::/0, and the ordinary ULA and CGNAT blocks an
-// delivery path.
+// operator would really list — and the address must stay
+// refused anyway, on both the validation and the delivery path.
 func TestGuardAllowlist_MetadataAlwaysRefused(t *testing.T) {
 	t.Parallel()
 
@@ -219,15 +220,17 @@ type metadataAlwaysRefusedCase struct {
 }
 
 // metadataAlwaysRefusedCases enumerates every unconditionally
-// blocked address together with an allowlist entry that would
+// blocked address (link-local, the cloud metadata endpoints and
-// otherwise reach it. Split by family of address only to stay
+// the unspecified addresses) together with an allowlist entry
-// under the function-length limit.
+// that would otherwise reach it. Split by family of address only
+// to stay under the function-length limit.
 func metadataAlwaysRefusedCases() []metadataAlwaysRefusedCase {
 	cases := linkLocalRefusedCases()
 	cases = append(cases, ulaMetadataRefusedCases()...)
 	cases = append(cases, ipv4MetadataRefusedCases()...)
+	cases = append(cases, encodedMetadataRefusedCases()...)
 
-	return append(cases, encodedMetadataRefusedCases()...)
+	return append(cases, unspecifiedRefusedCases()...)
 }
 
 // linkLocalRefusedCases covers the link-local blocks, including
@@ -367,6 +370,23 @@ func encodedMetadataRefusedCases() []metadataAlwaysRefusedCase {
 	}
 }
 
+// unspecifiedRefusedCases covers the unspecified addresses, each
+// of which reaches this host's loopback on Linux.
+func unspecifiedRefusedCases() []metadataAlwaysRefusedCase {
+	return []metadataAlwaysRefusedCase{
+		{
+			name:   "IPv4 unspecified address under 0.0.0.0/0",
+			allow:  allowAllIPv4,
+			target: "http://0.0.0.0:8080/hook",
+		},
+		{
+			name:   "IPv6 unspecified address under ::/0",
+			allow:  allowAllIPv6,
+			target: "http://[::]:8080/hook",
+		},
+	}
+}
+
 // TestGuardAllowlist_PublicUnaffected asserts the allowlist does
 // not narrow anything: public addresses were reachable before it
 // existed and stay reachable, whether or not a list is set.
@@ -524,6 +544,10 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) {
 		// Oracle Cloud Classic metadata, inside the blocked
 		// 192.0.0.0/24.
 		"192.0.0.192/32",
+		// The IPv4 and IPv6 unspecified addresses, each of
+		// which reaches this host's loopback on Linux.
+		"0.0.0.0/32",
+		"::/128",
 		// 169.254.169.254 as an IPv4-compatible IPv6 address.
 		"::a9fe:a9fe/128",
 		// 169.254.169.254 behind the NAT64 well-known prefix.
@@ -556,7 +580,8 @@ func TestDefaultBlocklist_PinnedSet(t *testing.T) {
 		{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},
+		// Its first address, 0.0.0.0, is in the unconditional set.
+		{cidr: "0.0.0.0/8", reopenable: false},
 		{cidr: "100.64.0.0/10", reopenable: true},
 		{cidr: "192.0.0.0/24", reopenable: true},
 		{cidr: "192.0.2.0/24", reopenable: true},
@@ -566,8 +591,11 @@ func TestDefaultBlocklist_PinnedSet(t *testing.T) {
 		{cidr: "224.0.0.0/4", reopenable: true},
 		{cidr: "240.0.0.0/4", reopenable: true},
 		{cidr: "::1/128", reopenable: true},
+		{cidr: "::/128", reopenable: false},
 		{cidr: "fc00::/7", reopenable: true},
 		{cidr: "fe80::/10", reopenable: false},
+		{cidr: "ff00::/8", reopenable: true},
+		{cidr: "2001:db8::/32", reopenable: true},
 		{cidr: "168.63.129.16/32", public: true, reopenable: true},
 	}
 

internal/delivery/ssrf_test.go

@@ -101,6 +101,42 @@ func TestValidateTargetURL_Blocked(t *testing.T) {
 	}
 }
 
+// TestDefaultGuard_RefusesUnspecifiedMulticastAndDocumentation
+// covers the unspecified addresses and the IPv6 multicast and
+// documentation ranges: with no allowlist set, each is refused
+// both when a target is created and when a delivery dials it.
+func TestDefaultGuard_RefusesUnspecifiedMulticastAndDocumentation(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	guard := delivery.NewTestGuard()
+
+	targets := []string{
+		// The unspecified addresses. On Linux a connection to
+		// either reaches this host's loopback.
+		"http://0.0.0.0:8080/hook",
+		"http://[::]:8080/hook",
+		// IPv6 multicast, all nodes.
+		"http://[ff02::1]/hook",
+		// IPv6 documentation.
+		"http://[2001:db8::1]/hook",
+	}
+
+	for _, target := range targets {
+		t.Run(target, func(t *testing.T) {
+			t.Parallel()
+
+			require.Error(t,
+				guard.ValidateTargetURL(context.Background(), target),
+				"%s must be refused at target creation", target,
+			)
+
+			assertDialRefused(t, guard, target)
+		})
+	}
+}
+
 func TestValidateTargetURL_Allowed(t *testing.T) {
 	t.Parallel()
 

internal/gormlog/firstboot_test.go

@@ -137,6 +137,10 @@ func bootAtDebug(t *testing.T, dataDir string) string {
 
 	app := fxtest.New(
 		t,
+		// fx's own log is discarded, not sent to t.Logf: a hook still
+		// running after a start or stop timeout would write there after
+		// the test has returned.
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,

internal/handlers/handlers_test.go

@@ -163,6 +163,10 @@ func newTestApp(
 
 	return fxtest.New(
 		t,
+		// fx's own log is discarded, not sent to t.Logf: a hook still
+		// running after a start or stop timeout would write there after
+		// the test has returned.
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,

internal/handlers/source_management.go

@@ -574,17 +574,18 @@ func (h *Handlers) applyWebhookEdit(
 	webhook.RetentionDays = retentionDays
 
 	// A new name renames the archive files before it is saved (see
-	// delivery.Engine.Rename). If either step fails, they go back to
+	// delivery.Engine.Rename). If either step fails, the same targets'
-	// the name that is still stored.
+	// archives go back to the name that is still stored, without
-	err := h.renameWebhookArchives(webhook.ID, oldName, webhook.Name)
+	// reading the main database again.
+	targets, err := h.renameWebhookArchives(
+		webhook.ID, oldName, webhook.Name,
+	)
 	if err == nil {
 		err = h.db.DB().Save(webhook).Error
 	}
 
 	if err != nil {
-		restoreErr := h.renameWebhookArchives(
+		restoreErr := h.renameArchives(targets, oldName)
-			webhook.ID, webhook.Name, oldName,
-		)
 		if restoreErr != nil {
 			h.log.Error(
 				"failed to rename archives back",
@@ -779,14 +780,13 @@ func (h *Handlers) evictTargetArchiveWriter(targetID string) {
 // renameWebhookArchives renames the archive file of every database
 // target of a webhook from the webhook name oldName to newName,
 // keeping each target's own name. It does nothing when the name is
-// unchanged. It tries every target even after one fails, so that
+// unchanged. It returns the targets it read, so that a failed edit can
-// moving the archives back after a failed edit leaves none under the
+// move those same archives back with renameArchives.
-// new name, and returns every failure joined.
 func (h *Handlers) renameWebhookArchives(
 	webhookID, oldName, newName string,
-) error {
+) ([]database.Target, error) {
 	if h.archives == nil || oldName == newName {
-		return nil
+		return nil, nil
 	}
 
 	var targets []database.Target
@@ -798,14 +798,25 @@ func (h *Handlers) renameWebhookArchives(
 		).
 		Find(&targets).Error
 	if err != nil {
-		return err
+		return nil, err
 	}
 
+	return targets, h.renameArchives(targets, newName)
+}
+
+// renameArchives renames the archive file of each of the given
+// database targets to the webhook name webhookName, keeping each
+// target's own name. It tries every target even after one fails, so
+// that moving the archives back after a failed edit leaves none under
+// the new name, and returns every failure joined.
+func (h *Handlers) renameArchives(
+	targets []database.Target, webhookName string,
+) error {
 	var errs []error
 
 	for i := range targets {
-		err = h.archives.Rename(
+		err := h.archives.Rename(
-			targets[i].ID, newName, targets[i].Name,
+			targets[i].ID, webhookName, targets[i].Name,
 		)
 		if err != nil {
 			errs = append(errs, err)
@@ -1606,10 +1617,11 @@ func (h *Handlers) validateTargetURL(
 		msg := "Invalid target URL: " + err.Error()
 
 		// Only a private or reserved address's refusal says how
-		// to allow it. Metadata refusals never do: link-local and
+		// to allow it. Other refusals never do: link-local, the
-		// the other unconditional metadata addresses cannot be
+		// unspecified addresses and the unconditional metadata
-		// opened, and the default blocklist's public addresses,
+		// addresses cannot be opened, and the default
-		// which listing does open, hand out credentials.
+		// 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 " +

internal/handlers/source_management_test.go

@@ -612,10 +612,12 @@ func TestHandleSourceEditSubmit_FailedSaveRenamesBack(t *testing.T) {
 }
 
 // TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack proves
-// that when a webhook has two database targets and only the second
+// that when a webhook has three database targets and only the middle
-// one's archive cannot be renamed, the first is renamed back and the
+// one's archive cannot be renamed, the stored name stays and both
-// stored name stays. Every target is tried in each direction, so this
+// others are renamed back, the last one included: the move back does
-// holds whichever order the two come in.
+// not stop at the target it cannot rename. The handler reaches the
+// targets in the order they were created, which the exact sequence
+// below pins, so the refused target always comes before the last.
 func TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack(
 	t *testing.T,
 ) {
@@ -624,9 +626,10 @@ func TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack(
 	env := setupSourceTest(t)
 	wh := seedWebhookWithRetention(t, env.db, 7)
 	first := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
-	second := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
+	middle := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
+	last := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
 
-	env.archives.FailRenames(second.ID, errNameTaken)
+	env.archives.FailRenames(middle.ID, errNameTaken)
 
 	oldName := wh.Name
 	wh.Name = renamedWebhookName
@@ -641,13 +644,15 @@ func TestHandleSourceEditSubmit_FailedRenameRenamesTheOthersBack(
 	)
 	assert.Equal(t, oldName, stored.Name)
 
-	assert.ElementsMatch(
+	assert.Equal(
 		t,
 		[]archiveRename{
 			{first.ID, renamedWebhookName, first.Name},
-			{second.ID, renamedWebhookName, second.Name},
+			{middle.ID, renamedWebhookName, middle.Name},
+			{last.ID, renamedWebhookName, last.Name},
 			{first.ID, oldName, first.Name},
-			{second.ID, oldName, second.Name},
+			{middle.ID, oldName, middle.Name},
+			{last.ID, oldName, last.Name},
 		},
 		env.archives.Renames(),
 	)

internal/resetpw/resetpw_test.go

@@ -158,6 +158,10 @@ func newServerApp(
 
 	app := fxtest.New(
 		t,
+		// fx's own log is discarded, not sent to t.Logf: a hook still
+		// running after a start or stop timeout would write there after
+		// the test has returned.
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,

internal/server/routes_test.go

@@ -110,6 +110,10 @@ func newTestEnvWithConfig(
 
 	app := fxtest.New(
 		t,
+		// fx's own log is discarded, not sent to t.Logf: a hook still
+		// running after a start or stop timeout would write there after
+		// the test has returned.
+		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,