fix: skip expired ipns records in GetIPNS

CHANGELOG.md

@@ -21,6 +21,7 @@ The following emojis are used to highlight certain changes:
 
 ### Fixed
 
+- `GetIPNS` no longer returns an IPNS record whose EOL has already passed. An expired record is cryptographically invalid, so it is treated as not found, and when multiple routers answer the first non-expired record is returned. [#154](https://github.com/ipfs/someguy/pull/154)
 - `/routing/v1/peers/{peerid}` now serves addresses cache-first, the same way `/routing/v1/providers/{cid}` does. It answers from the cached address book and host peerstore before falling back to a DHT lookup, so a relay-dependent peer that is absent from peer routing but recently seen as a provider is no longer answered with an empty result. See [`docs/peer-address-caching.md`](https://github.com/ipfs/someguy/blob/main/docs/peer-address-caching.md).
 - A completed identify now prunes a peer's cached addresses down to its current advertised set (signed peer record or identify listen addresses) plus any live-connection address, instead of unioning forever. This stops stale certhashes, dead relay circuits, and rotated NAT ports from accumulating across provider lookups and gossip.
 - Multiaddrs in `/routing/v1` responses are returned in a stable sorted order. They previously came back in nondeterministic order, so repeated requests for the same peer or provider returned the same addresses shuffled differently.

server_routers.go

@@ -246,12 +246,48 @@ func (mi *manyIter[T]) Close() error {
 	return err
 }
 
+// isExpiredIPNSRecord reports whether an EOL-type IPNS record has passed its
+// validity. A record past its EOL is cryptographically invalid, so returning it
+// only hands the caller a record that fails validation. Records with a non-EOL
+// validity type or an unreadable validity are treated as not expired.
+//
+// Every backend already validates the record it returns, so this is a
+// re-check rather than the first line of defense. We still do it because a
+// signature is immutable and verified once, but EOL is time-varying: the
+// longer a record lingers (a cache hit, a stored copy, clock drift between
+// our infrastructure and the producer), the more likely it has expired
+// between the backend's check and the moment we hand it to the user.
+// parallelRouter is first-result-wins with no revalidation, so without this
+// the aggregator could let an expired record win the race. See
+// https://github.com/ipfs/boxo/pull/1166 for the upstream cache-control fix.
+func isExpiredIPNSRecord(rec *ipns.Record) bool {
+	if rec == nil {
+		return false
+	}
+	validityType, err := rec.ValidityType()
+	if err != nil || validityType != ipns.ValidityEOL {
+		return false
+	}
+	validity, err := rec.Validity()
+	if err != nil {
+		return false
+	}
+	return time.Now().After(validity)
+}
+
 func (r parallelRouter) GetIPNS(ctx context.Context, name ipns.Name) (*ipns.Record, error) {
 	switch len(r.routers) {
 	case 0:
 		return nil, routing.ErrNotFound
 	case 1:
-		return r.routers[0].GetIPNS(ctx, name)
+		rec, err := r.routers[0].GetIPNS(ctx, name)
+		if err != nil {
+			return nil, err
+		}
+		if isExpiredIPNSRecord(rec) {
+			return nil, routing.ErrNotFound
+		}
+		return rec, nil
 	}
 
 	ctx, cancel := context.WithCancel(ctx)
@@ -284,6 +320,11 @@ func (r parallelRouter) GetIPNS(ctx context.Context, name ipns.Name) (*ipns.Reco
 		case res := <-results:
 			switch res.err {
 			case nil:
+				// An expired record is invalid and must not be served; treat it
+				// as not found and keep waiting for another router to answer.
+				if isExpiredIPNSRecord(res.val) {
+					continue
+				}
 				return res.val, nil
 			case routing.ErrNotFound, routing.ErrNotSupported:
 				continue

server_routers_test.go

@@ -89,6 +89,17 @@ func makeIPNSRecord(t *testing.T, sk crypto.PrivKey, opts ...ipns.Option) (*ipns
 	return record, rawRecord
 }
 
+func makeExpiredIPNSRecord(t *testing.T, sk crypto.PrivKey) *ipns.Record {
+	cid, err := cid.Decode("bafkreifjjcie6lypi6ny7amxnfftagclbuxndqonfipmb64f2km2devei4")
+	require.NoError(t, err)
+
+	// EOL in the past so the record is already expired
+	record, err := ipns.NewRecord(sk, path.FromCid(cid), 1, time.Now().Add(-time.Hour), time.Second*20)
+	require.NoError(t, err)
+
+	return record
+}
+
 func TestGetIPNS(t *testing.T) {
 	t.Parallel()
 
@@ -158,6 +169,47 @@ func TestGetIPNS(t *testing.T) {
 		_, err := r.GetIPNS(ctx, name)
 		require.ErrorIs(t, err, routing.ErrNotFound)
 	})
+
+	t.Run("Expired Record Treated As Not Found", func(t *testing.T) {
+		ctx := context.Background()
+
+		expired := makeExpiredIPNSRecord(t, sk)
+
+		mr1 := &mockRouter{}
+		mr1.On("GetIPNS", mock.Anything, name).Return(expired, nil)
+
+		r := parallelRouter{
+			routers: []router{
+				composableRouter{ipns: mr1},
+			},
+		}
+
+		_, err := r.GetIPNS(ctx, name)
+		require.ErrorIs(t, err, routing.ErrNotFound)
+	})
+
+	t.Run("Skips Expired Record For Valid One", func(t *testing.T) {
+		ctx := context.Background()
+
+		expired := makeExpiredIPNSRecord(t, sk)
+
+		mr1 := &mockRouter{}
+		mr1.On("GetIPNS", mock.Anything, name).Return(expired, nil)
+
+		mr2 := &mockRouter{}
+		mr2.On("GetIPNS", mock.Anything, name).Return(rec, nil)
+
+		r := parallelRouter{
+			routers: []router{
+				composableRouter{ipns: mr1},
+				composableRouter{ipns: mr2},
+			},
+		}
+
+		getRec, err := r.GetIPNS(ctx, name)
+		require.NoError(t, err)
+		require.EqualValues(t, rec, getRec)
+	})
 }
 
 func TestPutIPNS(t *testing.T) {