From 85498d6823edcc25900973608a7235425f9cfe8e Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 3 Jun 2026 17:32:57 +0200 Subject: [PATCH 1/2] fix: skip expired ipns records in GetIPNS parallelRouter.GetIPNS returned the first record a router produced without checking its validity, so an expired record could be served. Treat an EOL-passed record as not found and keep waiting for a valid one from another router. --- CHANGELOG.md | 2 ++ server_routers.go | 33 ++++++++++++++++++++++++++- server_routers_test.go | 52 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c7bf962..bd990b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ 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. + ### Security ## [v0.13.0] - 2026-05-26 diff --git a/server_routers.go b/server_routers.go index ce7806f..39dd27d 100644 --- a/server_routers.go +++ b/server_routers.go @@ -244,12 +244,38 @@ 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. +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) @@ -282,6 +308,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 diff --git a/server_routers_test.go b/server_routers_test.go index 3523ab8..d51a5bb 100644 --- a/server_routers_test.go +++ b/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) { From 4482e4e4c2541793857b1f91be9d4736a8a1612f Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 3 Jun 2026 18:34:36 +0200 Subject: [PATCH 2/2] docs: explain why GetIPNS re-checks IPNS expiry every backend already validates the record it returns, but EOL is time-varying while signatures are not: a record can expire between a backend's check and the moment the aggregator returns it. parallelRouter is first-result-wins with no revalidation, so document why the re-check guards against expired records winning the race. --- server_routers.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/server_routers.go b/server_routers.go index 39dd27d..04f0b8a 100644 --- a/server_routers.go +++ b/server_routers.go @@ -248,6 +248,16 @@ func (mi *manyIter[T]) Close() error { // 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