mirror of
https://tangled.org/evan.jarrett.net/at-container-registry
synced 2026-09-21 01:34:16 +00:00
A user with an unresolvable defaultHold (did:web:localhost%3A8080) got HTTP
500 with a body of literally {} on every request in their namespace, and
crane retried it three times because 500 is retryable.
The cause is in the distribution library. handlers/app.go:755 switches on the
error type returned by Repository() with cases for ErrRepositoryUnknown,
ErrRepositoryNameInvalid and errcode.Error, and no default. A bare fmt.Errorf
matches none of them, so context.Errors stays empty; ServeJSON then finds no
ErrorCoder, leaves sc == 0, falls through to 500, and Errors.MarshalJSON
renders the nil slice as {} via omitempty.
So every error leaving Repository() must be coded. Four were not:
hold URL unresolvable -> 404 NAME_UNKNOWN when errors.Is
atproto.ErrHoldDIDPermanent, else 503 UNAVAILABLE
no hold DID configured -> 500 UNKNOWN, but with a body and a log line
invalid image name -> 400 NAME_INVALID
name missing an owner -> 400 NAME_INVALID
The permanent/transient split is the point: a DNS blip must stay retryable,
but a did:web that can never resolve must not be retried at all. Stored user
data that cannot resolve is a 4xx condition, not a server fault, and the
NAME_UNKNOWN message now names the hold so the owner can fix their profile.
The hold DID is already world-readable in their sailor profile record.
Tests assert through a helper that replays distribution's exact type switch,
so an uncoded error still surfaces as 500 {} and the assertions bind to the
real behaviour rather than to the constructors.
Does not address the logrus line at app.go:757, which fires unconditionally
before the type switch and cannot be avoided by any returned error type.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
489 lines
16 KiB
Go
489 lines
16 KiB
Go
package middleware
|
|
|
|
import (
|
|
"context"
|
|
"encoding/json"
|
|
"errors"
|
|
"fmt"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/distribution/distribution/v3"
|
|
"github.com/distribution/distribution/v3/registry/api/errcode"
|
|
"github.com/distribution/reference"
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
|
|
"atcr.io/pkg/atproto"
|
|
)
|
|
|
|
// mockNamespace is a mock implementation of distribution.Namespace
|
|
type mockNamespace struct {
|
|
distribution.Namespace
|
|
repositories map[string]distribution.Repository
|
|
}
|
|
|
|
func (m *mockNamespace) Repository(ctx context.Context, name reference.Named) (distribution.Repository, error) {
|
|
if m.repositories == nil {
|
|
return nil, fmt.Errorf("repository not found: %s", name.Name())
|
|
}
|
|
if repo, ok := m.repositories[name.Name()]; ok {
|
|
return repo, nil
|
|
}
|
|
return nil, fmt.Errorf("repository not found: %s", name.Name())
|
|
}
|
|
|
|
func (m *mockNamespace) Repositories(ctx context.Context, repos []string, last string) (int, error) {
|
|
// Return empty result for mock
|
|
return 0, nil
|
|
}
|
|
|
|
func (m *mockNamespace) Blobs() distribution.BlobEnumerator {
|
|
return nil
|
|
}
|
|
|
|
func (m *mockNamespace) BlobStatter() distribution.BlobStatter {
|
|
return nil
|
|
}
|
|
|
|
func TestSetGlobalRefresher(t *testing.T) {
|
|
// Test that SetGlobalRefresher doesn't panic
|
|
SetGlobalRefresher(nil)
|
|
// If we get here without panic, test passes
|
|
}
|
|
|
|
func TestSetGlobalDatabase(t *testing.T) {
|
|
SetGlobalDatabase(nil)
|
|
// If we get here without panic, test passes
|
|
}
|
|
|
|
func TestSetGlobalAuthorizer(t *testing.T) {
|
|
SetGlobalAuthorizer(nil)
|
|
// If we get here without panic, test passes
|
|
}
|
|
|
|
// TestInitATProtoResolver tests the initialization function
|
|
func TestInitATProtoResolver(t *testing.T) {
|
|
ctx := context.Background()
|
|
mockNS := &mockNamespace{}
|
|
|
|
tests := []struct {
|
|
name string
|
|
options map[string]any
|
|
wantErr bool
|
|
}{
|
|
{
|
|
name: "with default hold DID",
|
|
options: map[string]any{
|
|
"default_hold_did": "did:web:hold01.atcr.io",
|
|
"base_url": "https://atcr.io",
|
|
"test_mode": false,
|
|
},
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "with test mode enabled",
|
|
options: map[string]any{
|
|
"default_hold_did": "did:web:hold01.atcr.io",
|
|
"base_url": "https://atcr.io",
|
|
"test_mode": true,
|
|
},
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "without options",
|
|
options: map[string]any{},
|
|
wantErr: false,
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
ns, err := initATProtoResolver(ctx, mockNS, nil, tt.options)
|
|
if tt.wantErr {
|
|
assert.Error(t, err)
|
|
return
|
|
}
|
|
|
|
require.NoError(t, err)
|
|
assert.NotNil(t, ns)
|
|
|
|
resolver, ok := ns.(*NamespaceResolver)
|
|
require.True(t, ok, "expected NamespaceResolver type")
|
|
|
|
if holdDID, ok := tt.options["default_hold_did"].(string); ok {
|
|
assert.Equal(t, holdDID, resolver.defaultHoldDID)
|
|
}
|
|
if baseURL, ok := tt.options["base_url"].(string); ok {
|
|
assert.Equal(t, baseURL, resolver.baseURL)
|
|
}
|
|
if testMode, ok := tt.options["test_mode"].(bool); ok {
|
|
assert.Equal(t, testMode, resolver.testMode)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// TestAuthErrorMessage tests the error message formatting
|
|
func TestAuthErrorMessage(t *testing.T) {
|
|
resolver := &NamespaceResolver{
|
|
baseURL: "https://atcr.io",
|
|
}
|
|
|
|
err := resolver.authErrorMessage("OAuth session expired")
|
|
assert.Contains(t, err.Error(), "OAuth session expired")
|
|
assert.Contains(t, err.Error(), "https://atcr.io/auth/oauth/login")
|
|
}
|
|
|
|
// TestFindHoldDID_DefaultFallback tests default hold DID fallback
|
|
func TestFindHoldDID_DefaultFallback(t *testing.T) {
|
|
// Start a mock PDS server that returns 404 for profile and empty list for holds
|
|
mockPDS := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
if r.URL.Path == "/xrpc/com.atproto.repo.getRecord" {
|
|
// Profile not found
|
|
w.WriteHeader(http.StatusNotFound)
|
|
return
|
|
}
|
|
if r.URL.Path == "/xrpc/com.atproto.repo.listRecords" {
|
|
// Empty hold records
|
|
w.Header().Set("Content-Type", "application/json")
|
|
json.NewEncoder(w).Encode(map[string]any{
|
|
"records": []any{},
|
|
})
|
|
return
|
|
}
|
|
w.WriteHeader(http.StatusNotFound)
|
|
}))
|
|
defer mockPDS.Close()
|
|
|
|
resolver := &NamespaceResolver{
|
|
defaultHoldDID: "did:web:default.atcr.io",
|
|
}
|
|
|
|
ctx := context.Background()
|
|
holdDID, _ := resolver.findHoldDIDAndProfile(ctx, "did:plc:test123", mockPDS.URL)
|
|
|
|
assert.Equal(t, "did:web:default.atcr.io", holdDID, "should fall back to default hold DID")
|
|
}
|
|
|
|
// TestFindHoldDID_SailorProfile tests hold discovery from sailor profile
|
|
func TestFindHoldDID_SailorProfile(t *testing.T) {
|
|
// Start a mock PDS server that returns a sailor profile
|
|
mockPDS := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
if r.URL.Path == "/xrpc/com.atproto.repo.getRecord" {
|
|
// Return sailor profile with defaultHold
|
|
profile := atproto.NewSailorProfileRecord("did:web:user.hold.io")
|
|
w.Header().Set("Content-Type", "application/json")
|
|
json.NewEncoder(w).Encode(map[string]any{
|
|
"value": profile,
|
|
})
|
|
return
|
|
}
|
|
w.WriteHeader(http.StatusNotFound)
|
|
}))
|
|
defer mockPDS.Close()
|
|
|
|
resolver := &NamespaceResolver{
|
|
defaultHoldDID: "did:web:default.atcr.io",
|
|
testMode: false,
|
|
}
|
|
|
|
ctx := context.Background()
|
|
holdDID, _ := resolver.findHoldDIDAndProfile(ctx, "did:plc:test123", mockPDS.URL)
|
|
|
|
assert.Equal(t, "did:web:user.hold.io", holdDID, "should use sailor profile's defaultHold")
|
|
}
|
|
|
|
// TestFindHoldDID_Priority tests the priority order
|
|
func TestFindHoldDID_Priority(t *testing.T) {
|
|
// Start a mock PDS server that returns both profile and hold records
|
|
mockPDS := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
if r.URL.Path == "/xrpc/com.atproto.repo.getRecord" {
|
|
// Return sailor profile with defaultHold (highest priority)
|
|
profile := atproto.NewSailorProfileRecord("did:web:profile.hold.io")
|
|
w.Header().Set("Content-Type", "application/json")
|
|
json.NewEncoder(w).Encode(map[string]any{
|
|
"value": profile,
|
|
})
|
|
return
|
|
}
|
|
w.WriteHeader(http.StatusNotFound)
|
|
}))
|
|
defer mockPDS.Close()
|
|
|
|
resolver := &NamespaceResolver{
|
|
defaultHoldDID: "did:web:default.atcr.io",
|
|
}
|
|
|
|
ctx := context.Background()
|
|
holdDID, _ := resolver.findHoldDIDAndProfile(ctx, "did:plc:test123", mockPDS.URL)
|
|
|
|
// Profile should take priority over hold records and default
|
|
assert.Equal(t, "did:web:profile.hold.io", holdDID, "should prioritize sailor profile over hold records")
|
|
}
|
|
|
|
// TestFindHoldDID_TestModeFallback tests test mode fallback when hold unreachable
|
|
func TestFindHoldDID_TestModeFallback(t *testing.T) {
|
|
// Start a mock PDS server that returns a profile with unreachable hold
|
|
mockPDS := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
if r.URL.Path == "/xrpc/com.atproto.repo.getRecord" {
|
|
// Return sailor profile with an unreachable hold
|
|
profile := atproto.NewSailorProfileRecord("did:web:unreachable.hold.io")
|
|
w.Header().Set("Content-Type", "application/json")
|
|
json.NewEncoder(w).Encode(map[string]any{
|
|
"value": profile,
|
|
})
|
|
return
|
|
}
|
|
w.WriteHeader(http.StatusNotFound)
|
|
}))
|
|
defer mockPDS.Close()
|
|
|
|
resolver := &NamespaceResolver{
|
|
defaultHoldDID: "did:web:default.atcr.io",
|
|
testMode: true, // Test mode enabled
|
|
}
|
|
|
|
ctx := context.Background()
|
|
holdDID, _ := resolver.findHoldDIDAndProfile(ctx, "did:plc:test123", mockPDS.URL)
|
|
|
|
// In test mode with unreachable hold, should fall back to default
|
|
assert.Equal(t, "did:web:default.atcr.io", holdDID, "should fall back to default in test mode when hold unreachable")
|
|
}
|
|
|
|
// TestIsHoldReachable tests the hold reachability check
|
|
func TestIsHoldReachable(t *testing.T) {
|
|
// Mock hold server with DID document
|
|
mockHold := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
if r.URL.Path == "/.well-known/did.json" {
|
|
w.Header().Set("Content-Type", "application/json")
|
|
json.NewEncoder(w).Encode(map[string]any{
|
|
"id": "did:web:reachable.hold.io",
|
|
})
|
|
return
|
|
}
|
|
w.WriteHeader(http.StatusNotFound)
|
|
}))
|
|
defer mockHold.Close()
|
|
|
|
resolver := &NamespaceResolver{}
|
|
|
|
ctx := context.Background()
|
|
|
|
t.Run("reachable hold", func(t *testing.T) {
|
|
// Use URL format directly — DID resolution requires real identity directory
|
|
reachable := resolver.isHoldReachable(ctx, mockHold.URL)
|
|
assert.True(t, reachable, "should detect reachable hold")
|
|
})
|
|
|
|
t.Run("unreachable hold", func(t *testing.T) {
|
|
reachable := resolver.isHoldReachable(ctx, "did:web:nonexistent.example.com")
|
|
assert.False(t, reachable, "should detect unreachable hold")
|
|
})
|
|
}
|
|
|
|
// TestRepositoryCaching tests that repositories are cached by DID+name
|
|
func TestRepositoryCaching(t *testing.T) {
|
|
// This test requires integration with actual repository resolution
|
|
// For now, we test that the cache key format is correct
|
|
did := "did:plc:test123"
|
|
repoName := "myapp"
|
|
expectedKey := "did:plc:test123:myapp"
|
|
|
|
cacheKey := did + ":" + repoName
|
|
assert.Equal(t, expectedKey, cacheKey, "cache key should be DID:reponame")
|
|
}
|
|
|
|
// TestNamespaceResolver_Repositories tests delegation to underlying namespace
|
|
func TestNamespaceResolver_Repositories(t *testing.T) {
|
|
mockNS := &mockNamespace{}
|
|
resolver := &NamespaceResolver{
|
|
Namespace: mockNS,
|
|
}
|
|
|
|
ctx := context.Background()
|
|
repos := []string{}
|
|
|
|
// Test delegation (mockNamespace doesn't implement this, so it will return 0, nil)
|
|
n, err := resolver.Repositories(ctx, repos, "")
|
|
assert.NoError(t, err)
|
|
assert.Equal(t, 0, n)
|
|
}
|
|
|
|
// TestNamespaceResolver_Blobs tests delegation to underlying namespace
|
|
func TestNamespaceResolver_Blobs(t *testing.T) {
|
|
mockNS := &mockNamespace{}
|
|
resolver := &NamespaceResolver{
|
|
Namespace: mockNS,
|
|
}
|
|
|
|
// Should not panic
|
|
blobs := resolver.Blobs()
|
|
assert.Nil(t, blobs, "mockNamespace returns nil")
|
|
}
|
|
|
|
// TestNamespaceResolver_BlobStatter tests delegation to underlying namespace
|
|
func TestNamespaceResolver_BlobStatter(t *testing.T) {
|
|
mockNS := &mockNamespace{}
|
|
resolver := &NamespaceResolver{
|
|
Namespace: mockNS,
|
|
}
|
|
|
|
// Should not panic
|
|
statter := resolver.BlobStatter()
|
|
assert.Nil(t, statter, "mockNamespace returns nil")
|
|
}
|
|
|
|
// serveAsDistributionWould replays distribution's dispatcher behaviour
|
|
// (registry/handlers/app.go, the type switch after app.registry.Repository())
|
|
// and reports the status code and JSON body a client would actually see.
|
|
//
|
|
// The switch has no default branch, so an error that is not an errcode.Error
|
|
// leaves the error list empty and errcode.ServeJSON falls through to a
|
|
// bodyless `500 {}`. Asserting through this helper is what proves each path
|
|
// produces a real response rather than that empty envelope.
|
|
func serveAsDistributionWould(t *testing.T, err error) (int, string) {
|
|
t.Helper()
|
|
|
|
var errs errcode.Errors
|
|
switch e := err.(type) {
|
|
case distribution.ErrRepositoryUnknown:
|
|
errs = append(errs, errcode.ErrorCodeNameUnknown.WithDetail(e))
|
|
case distribution.ErrRepositoryNameInvalid:
|
|
errs = append(errs, errcode.ErrorCodeNameInvalid.WithDetail(e))
|
|
case errcode.Error:
|
|
errs = append(errs, e)
|
|
}
|
|
|
|
rec := httptest.NewRecorder()
|
|
require.NoError(t, errcode.ServeJSON(rec, errs))
|
|
return rec.Code, strings.TrimSpace(rec.Body.String())
|
|
}
|
|
|
|
// TestHoldResolutionError_PermanentVsTransient is the core of the fix: a hold
|
|
// DID that can never resolve must terminate the client, while a transient
|
|
// failure must stay retryable.
|
|
func TestHoldResolutionError_PermanentVsTransient(t *testing.T) {
|
|
const holdDID = "did:web:localhost%3A8080"
|
|
|
|
t.Run("permanent", func(t *testing.T) {
|
|
wrapped := fmt.Errorf("failed to resolve hold DID %s: %w: %w",
|
|
holdDID, atproto.ErrHoldDIDPermanent, errors.New("directory rejected host"))
|
|
|
|
err := holdResolutionError(holdDID, wrapped)
|
|
|
|
var coded errcode.Error
|
|
require.True(t, errors.As(err, &coded), "must be an errcode.Error")
|
|
assert.Equal(t, errcode.ErrorCodeNameUnknown, coded.Code)
|
|
|
|
status, body := serveAsDistributionWould(t, err)
|
|
assert.Equal(t, http.StatusNotFound, status, "permanent hold failure must not be retryable")
|
|
assert.Contains(t, body, "NAME_UNKNOWN")
|
|
assert.Contains(t, body, holdDID, "message must name the unresolvable hold DID")
|
|
assert.NotEqual(t, "{}", body, "must not be the bodyless envelope")
|
|
})
|
|
|
|
t.Run("transient", func(t *testing.T) {
|
|
wrapped := fmt.Errorf("failed to resolve hold DID %s: %w", holdDID, errors.New("dial tcp: i/o timeout"))
|
|
|
|
err := holdResolutionError(holdDID, wrapped)
|
|
|
|
var coded errcode.Error
|
|
require.True(t, errors.As(err, &coded), "must be an errcode.Error")
|
|
assert.Equal(t, errcode.ErrorCodeUnavailable, coded.Code)
|
|
|
|
status, body := serveAsDistributionWould(t, err)
|
|
assert.Equal(t, http.StatusServiceUnavailable, status, "a DNS or PLC blip must stay retryable")
|
|
assert.Contains(t, body, "UNAVAILABLE")
|
|
assert.Contains(t, body, holdDID)
|
|
})
|
|
}
|
|
|
|
// TestHoldResolutionError_FromRealResolver verifies the classification against
|
|
// errors that atproto.ResolveHoldURL actually produces, not hand-built ones,
|
|
// so the permanent/transient split can't silently drift from the resolver.
|
|
func TestHoldResolutionError_FromRealResolver(t *testing.T) {
|
|
// testMode short-circuits did:web resolution to a derived URL, which would
|
|
// hide the permanent case entirely.
|
|
prevTestMode := atproto.IsTestMode()
|
|
atproto.SetTestMode(false)
|
|
t.Cleanup(func() { atproto.SetTestMode(prevTestMode) })
|
|
|
|
t.Run("stale defaultHold is permanent", func(t *testing.T) {
|
|
const holdDID = "did:web:localhost%3A8080"
|
|
|
|
_, err := atproto.ResolveHoldURL(context.Background(), holdDID)
|
|
require.Error(t, err)
|
|
require.ErrorIs(t, err, atproto.ErrHoldDIDPermanent,
|
|
"a did:web naming a port-qualified localhost can never resolve")
|
|
|
|
status, _ := serveAsDistributionWould(t, holdResolutionError(holdDID, err))
|
|
assert.Equal(t, http.StatusNotFound, status)
|
|
})
|
|
|
|
t.Run("interrupted lookup is transient", func(t *testing.T) {
|
|
const holdDID = "did:plc:aaaaaaaaaaaaaaaaaaaaaaaa"
|
|
|
|
ctx, cancel := context.WithCancel(context.Background())
|
|
cancel() // the lookup fails the way a network fault would, without a network
|
|
|
|
_, err := atproto.ResolveHoldURL(ctx, holdDID)
|
|
require.Error(t, err)
|
|
require.NotErrorIs(t, err, atproto.ErrHoldDIDPermanent,
|
|
"an interrupted lookup says nothing about the DID itself")
|
|
|
|
status, _ := serveAsDistributionWould(t, holdResolutionError(holdDID, err))
|
|
assert.Equal(t, http.StatusServiceUnavailable, status)
|
|
})
|
|
}
|
|
|
|
// TestRepository_MissingOwnerComponent covers the name-without-owner path.
|
|
func TestRepository_MissingOwnerComponent(t *testing.T) {
|
|
resolver := &NamespaceResolver{Namespace: &mockNamespace{}}
|
|
|
|
ref, err := reference.WithName("myimage")
|
|
require.NoError(t, err)
|
|
|
|
_, err = resolver.Repository(context.Background(), ref)
|
|
require.Error(t, err)
|
|
|
|
var coded errcode.Error
|
|
require.True(t, errors.As(err, &coded))
|
|
assert.Equal(t, errcode.ErrorCodeNameInvalid, coded.Code)
|
|
|
|
status, body := serveAsDistributionWould(t, err)
|
|
assert.Equal(t, http.StatusBadRequest, status)
|
|
assert.Contains(t, body, "NAME_INVALID")
|
|
assert.Contains(t, body, "myimage")
|
|
}
|
|
|
|
// TestRepository_UnresolvableIdentity pins the existing identity precedent that
|
|
// the hold-DID handling is modelled on.
|
|
func TestRepository_UnresolvableIdentity(t *testing.T) {
|
|
resolver := &NamespaceResolver{Namespace: &mockNamespace{}}
|
|
|
|
ref, err := reference.WithName("definitely-not-a-real-handle.example/foo")
|
|
require.NoError(t, err)
|
|
|
|
ctx, cancel := context.WithCancel(context.Background())
|
|
cancel() // resolution fails without depending on the network
|
|
|
|
_, err = resolver.Repository(ctx, ref)
|
|
require.Error(t, err)
|
|
|
|
status, body := serveAsDistributionWould(t, err)
|
|
assert.Equal(t, http.StatusNotFound, status)
|
|
assert.Contains(t, body, "NAME_UNKNOWN")
|
|
}
|
|
|
|
// TestRepository_NoHoldConfigured checks the misconfiguration path still 5xxes
|
|
// but carries a body an operator can act on.
|
|
func TestRepository_NoHoldConfigured(t *testing.T) {
|
|
status, body := serveAsDistributionWould(t, noHoldConfiguredError())
|
|
assert.Equal(t, http.StatusInternalServerError, status)
|
|
assert.Contains(t, body, "default_hold_did")
|
|
assert.NotEqual(t, "{}", body, "the whole point is that a 500 stops being bodyless")
|
|
}
|