From 52a2cbc90d3595eb5cf3a6171a8a22041757dada Mon Sep 17 00:00:00 2001 From: Joe Corall Date: Fri, 1 May 2026 16:00:48 +0000 Subject: [PATCH] Do not cache auth probe denials --- docs/authorization.md | 17 +++++----- internal/storage/local_url.go | 50 +++++++++--------------------- internal/storage/local_url_test.go | 35 ++++++++++----------- 3 files changed, 38 insertions(+), 64 deletions(-) diff --git a/docs/authorization.md b/docs/authorization.md index af9372e..3f550cd 100644 --- a/docs/authorization.md +++ b/docs/authorization.md @@ -158,16 +158,13 @@ repository permissions change rarely or when the mapping is used for content whose access is primarily stable. Shorter TTLs keep permission changes visible sooner at the cost of more probes to the upstream Drupal URL. -Triplet caches successful probes, 401, 403, and 404 responses for the -configured TTL. Other upstream errors are not cached. - -Negative auth-probe caching is conservative. For 401, 403, and 404 probe -responses, Triplet checks the upstream `Last-Modified` header before caching the -denial. If `Last-Modified` parses and is newer than -5 minutes ago, the denial is not cached. This avoids holding a stale denial -while repository access rules or file publication are still settling. If -`Last-Modified` is absent, unparseable, or older than that minimum age window, -the denial can be cached for the configured auth TTL. +Triplet caches successful probes for the configured TTL. It does not cache +401/403 authorization denials, because those commonly happen while Drupal is +still publishing or recalculating access. Other upstream errors are not cached. + +It also does not cache 404 not-found responses. A 404 can happen while Drupal +or Fedora is still publishing a just-created file, so Triplet rechecks upstream +on the next request instead of holding a stale miss. The image cache invalidation route also clears matching auth-probe entries when the source backend supports it. diff --git a/internal/storage/local_url.go b/internal/storage/local_url.go index c8ead04..74eed11 100644 --- a/internal/storage/local_url.go +++ b/internal/storage/local_url.go @@ -44,13 +44,12 @@ type LocalURLFallback struct { // LocalURLMapping maps a URL identifier prefix to a local file source. type LocalURLMapping struct { - Prefix string - File *FileOpener - OCFL bool - AuthProbe bool - AuthCacheTTL time.Duration - AuthErrorCacheMinAge time.Duration - AuthCacheMaxEntries int + Prefix string + File *FileOpener + OCFL bool + AuthProbe bool + AuthCacheTTL time.Duration + AuthCacheMaxEntries int } type authCacheEntry struct { @@ -65,10 +64,9 @@ var errAuthProbeHeadUnsupported = errors.New("auth probe head unsupported") type authCacheTier string const ( - authCacheTierAnonymous authCacheTier = "anonymous" - authCacheTierAuthenticated authCacheTier = "authenticated" - defaultAuthErrorCacheMinAge = 5 * time.Minute - defaultAuthCacheMaxEntries = 4096 + authCacheTierAnonymous authCacheTier = "anonymous" + authCacheTierAuthenticated authCacheTier = "authenticated" + defaultAuthCacheMaxEntries = 4096 ) // Open implements Opener. @@ -315,7 +313,7 @@ func (l *LocalURLFallback) authorize(ctx context.Context, identifier string, map } return l.authorizeAuthenticated(ctx, identifier, mapping, headers) } - anonErr := l.probeCached(ctx, anonKey, identifier, nil, mapping.anonymousAuthTTL(), mapping.authErrorCacheMinAge(), mapping.authCacheMaxEntries()) + anonErr := l.probeCached(ctx, anonKey, identifier, nil, mapping.anonymousAuthTTL(), mapping.authCacheMaxEntries()) if anonErr == nil || !hasAuthHeaders(headers) { return anonErr } @@ -330,10 +328,10 @@ func (l *LocalURLFallback) authorizeAuthenticated(ctx context.Context, identifie if err, ok := l.cachedAuth(key); ok { return err } - return l.probeCached(ctx, key, identifier, headers, mapping.authenticatedAuthTTL(), mapping.authErrorCacheMinAge(), mapping.authCacheMaxEntries()) + return l.probeCached(ctx, key, identifier, headers, mapping.authenticatedAuthTTL(), mapping.authCacheMaxEntries()) } -func (l *LocalURLFallback) probeCached(ctx context.Context, key, identifier string, headers http.Header, ttl, errorMinAge time.Duration, maxEntries int) error { +func (l *LocalURLFallback) probeCached(ctx context.Context, key, identifier string, headers http.Header, ttl time.Duration, maxEntries int) error { if wait, ok := l.beginAuthProbe(key); ok { l.debug(ctx, "local url auth probe waiting on in-flight probe", identifier, "cache_key", redact.Hash(key)) select { @@ -344,8 +342,8 @@ func (l *LocalURLFallback) probeCached(ctx context.Context, key, identifier stri return ctx.Err() } } - respHeader, err := l.probe(ctx, identifier, headers) - cacheable := cacheableAuthProbeResult(err) && cacheableAuthProbeResponse(err, respHeader, errorMinAge, time.Now()) + _, err := l.probe(ctx, identifier, headers) + cacheable := cacheableAuthProbeResult(err) l.debug(ctx, "local url auth probe completed", identifier, "cache_key", redact.Hash(key), "cacheable", cacheable, "ttl", ttl.String(), "err", err) if ttl > 0 && cacheable { l.storeAuth(key, authCacheScope(key), identifier, err, ttl, maxEntries) @@ -388,18 +386,7 @@ func hasAuthHeaders(headers http.Header) bool { } func cacheableAuthProbeResult(err error) bool { - return err == nil || errors.Is(err, ErrForbidden) || errors.Is(err, ErrNotFound) -} - -func cacheableAuthProbeResponse(err error, header http.Header, errorMinAge time.Duration, now time.Time) bool { - if err == nil || (!errors.Is(err, ErrForbidden) && !errors.Is(err, ErrNotFound)) || errorMinAge <= 0 { - return true - } - modified, parseErr := http.ParseTime(header.Get("Last-Modified")) - if parseErr != nil { - return true - } - return !modified.After(now.Add(-errorMinAge)) + return err == nil } func (m LocalURLMapping) anonymousAuthTTL() time.Duration { @@ -410,13 +397,6 @@ func (m LocalURLMapping) authenticatedAuthTTL() time.Duration { return m.AuthCacheTTL } -func (m LocalURLMapping) authErrorCacheMinAge() time.Duration { - if m.AuthErrorCacheMinAge > 0 { - return m.AuthErrorCacheMinAge - } - return defaultAuthErrorCacheMinAge -} - func (m LocalURLMapping) authCacheMaxEntries() int { if m.AuthCacheMaxEntries > 0 { return m.AuthCacheMaxEntries diff --git a/internal/storage/local_url_test.go b/internal/storage/local_url_test.go index 285d065..6e19794 100644 --- a/internal/storage/local_url_test.go +++ b/internal/storage/local_url_test.go @@ -574,7 +574,7 @@ func TestLocalURLFallbackAuthenticatedAuthProbeTTL(t *testing.T) { time.Sleep(ttl - time.Nanosecond) synctest.Wait() openAndClose(t, op, ctx, identifier) - if got := anonProbes.Load(); got != 1 { + if got := anonProbes.Load(); got != 2 { t.Fatalf("anonymous probes before auth ttl = %d", got) } if got := authProbes.Load(); got != 1 { @@ -584,7 +584,7 @@ func TestLocalURLFallbackAuthenticatedAuthProbeTTL(t *testing.T) { time.Sleep(2 * time.Nanosecond) synctest.Wait() openAndClose(t, op, ctx, identifier) - if got := anonProbes.Load(); got != 2 { + if got := anonProbes.Load(); got != 3 { t.Fatalf("anonymous probes after auth ttl = %d", got) } if got := authProbes.Load(); got != 2 { @@ -636,7 +636,7 @@ func TestLocalURLFallbackAuthProbeAnonymousSucceedsAndCaches(t *testing.T) { } } -func TestLocalURLFallbackAuthProbeDoesNotCacheRecentError(t *testing.T) { +func TestLocalURLFallbackAuthProbeDoesNotCacheForbidden(t *testing.T) { root := t.TempDir() if err := os.WriteFile(filepath.Join(root, "private.jp2"), []byte("private"), 0o600); err != nil { t.Fatal(err) @@ -654,11 +654,10 @@ func TestLocalURLFallbackAuthProbeDoesNotCacheRecentError(t *testing.T) { } op := &LocalURLFallback{ Mappings: []LocalURLMapping{{ - Prefix: srv.URL + "/system/files", - File: fileOp, - AuthProbe: true, - AuthCacheTTL: time.Minute, - AuthErrorCacheMinAge: time.Hour, + Prefix: srv.URL + "/system/files", + File: fileOp, + AuthProbe: true, + AuthCacheTTL: time.Minute, }}, Fallback: errOpener{}, AuthFallback: testAuthHTTP(t, srv), @@ -675,7 +674,7 @@ func TestLocalURLFallbackAuthProbeDoesNotCacheRecentError(t *testing.T) { } } -func TestLocalURLFallbackAuthProbeCachesOldError(t *testing.T) { +func TestLocalURLFallbackAuthProbeDoesNotCacheNotFound(t *testing.T) { root := t.TempDir() if err := os.WriteFile(filepath.Join(root, "private.jp2"), []byte("private"), 0o600); err != nil { t.Fatal(err) @@ -683,8 +682,7 @@ func TestLocalURLFallbackAuthProbeCachesOldError(t *testing.T) { var probes atomic.Int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { probes.Add(1) - w.Header().Set("Last-Modified", time.Now().Add(-2*time.Hour).UTC().Format(http.TimeFormat)) - w.WriteHeader(http.StatusForbidden) + w.WriteHeader(http.StatusNotFound) })) defer srv.Close() fileOp, err := NewFileOpener(root) @@ -693,11 +691,10 @@ func TestLocalURLFallbackAuthProbeCachesOldError(t *testing.T) { } op := &LocalURLFallback{ Mappings: []LocalURLMapping{{ - Prefix: srv.URL + "/system/files", - File: fileOp, - AuthProbe: true, - AuthCacheTTL: time.Minute, - AuthErrorCacheMinAge: time.Hour, + Prefix: srv.URL + "/system/files", + File: fileOp, + AuthProbe: true, + AuthCacheTTL: time.Minute, }}, Fallback: errOpener{}, AuthFallback: testAuthHTTP(t, srv), @@ -705,11 +702,11 @@ func TestLocalURLFallbackAuthProbeCachesOldError(t *testing.T) { for i := 0; i < 2; i++ { _, _, err := op.Open(context.Background(), srv.URL+"/system/files/private.jp2") - if !errors.Is(err, ErrForbidden) { + if !errors.Is(err, ErrNotFound) { t.Fatalf("err = %v", err) } } - if got := probes.Load(); got != 1 { + if got := probes.Load(); got != 2 { t.Fatalf("probes = %d", got) } } @@ -753,7 +750,7 @@ func TestLocalURLFallbackAuthProbeFallsBackToCredentialedCache(t *testing.T) { } _ = rc.Close() } - if got := probes.Load(); got != 2 { + if got := probes.Load(); got != 3 { t.Fatalf("probes = %d", got) } }