From a01b08b924427f19817ce786ea745fb704769f8a Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Fri, 11 Sep 2026 10:53:27 -0500 Subject: [PATCH] atproto: gate local indigo behavior behind a testmode build tag indigo's identity directory refuses HTTP and IP-hosted did:web, and its OAuth client is growing an SSRF-guarded transport that refuses loopback and private addresses. Local development and the test suites need both, and the workarounds were scattered: two did:web fallbacks in the resolver, a hand-rolled appview key fetch on the hold, and the OAuth client left on indigo's defaults so any test driving it against an httptest server depended on the transport staying permissive. Move every departure from indigo's defaults into one file pair in pkg/atproto: indigo_prod.go (!testmode) returns indigo's directory and OAuth client unchanged; indigo_local.go (testmode) wraps the directory so a did:web naming an IP, localhost, or a host with a port resolves over plain HTTP, and gives the OAuth client plain HTTP clients. All six identity and OAuth constructor call sites go through NewDirectory and NewOAuthClientApp. The resolver fallbacks, DIDWebToURL, and the hold's scheme-guessing key fetch are gone; the hold resolves the appview key through the directory, preferring #appview, and purges and retries once on a signature failure so a re-keyed appview is not masked by the 24-hour cache. There is no runtime switch for this: a production binary cannot be configured to resolve local DIDs. The runtime test_mode flag still gates the remaining behavioral branches only. Tests, the harness, make dev, Air, Dockerfile.dev, and docker-compose build with the tag; fixtures that need loopback did:web fail fast naming it. Test hold servers now serve a did.json via pkg/testpds so they resolve as real holds under the tag. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01UwYzaG3Yy7uA8FbZ5qk3tQ --- .air.hold.toml | 3 +- .air.labeler.toml | 3 +- .air.toml | 3 +- CLAUDE.md | 36 +++- Dockerfile.dev | 6 + Makefile | 32 ++-- cmd/relay-compare/main.go | 3 +- docker-compose.yml | 4 + docs/DEVELOPMENT.md | 26 ++- internal/testharness/harness.go | 8 + pkg/appview/authgate/testhelpers_test.go | 29 ++-- pkg/appview/holdclient/tier_update_test.go | 62 +++---- pkg/appview/jetstream/processor.go | 3 +- pkg/appview/middleware/registry_test.go | 11 +- pkg/atproto/directory.go | 9 +- pkg/atproto/indigo_local.go | 181 ++++++++++++++++++++ pkg/atproto/indigo_local_test.go | 149 ++++++++++++++++ pkg/atproto/indigo_prod.go | 33 ++++ pkg/atproto/indigo_prod_test.go | 25 +++ pkg/atproto/resolver.go | 29 +--- pkg/atproto/resolver_test.go | 53 +----- pkg/auth/hold_remote_captain_verify_test.go | 25 ++- pkg/auth/oauth/client.go | 6 +- pkg/billing/tier_resolution_test.go | 31 +++- pkg/billing/webhook_retry_test.go | 8 +- pkg/hold/admin/admin.go | 3 +- pkg/hold/pds/appview_token_test.go | 38 ++-- pkg/hold/pds/auth.go | 84 ++++----- pkg/labeler/server.go | 3 +- pkg/testpds/didweb.go | 33 ++++ 30 files changed, 709 insertions(+), 230 deletions(-) create mode 100644 pkg/atproto/indigo_local.go create mode 100644 pkg/atproto/indigo_local_test.go create mode 100644 pkg/atproto/indigo_prod.go create mode 100644 pkg/atproto/indigo_prod_test.go create mode 100644 pkg/testpds/didweb.go diff --git a/.air.hold.toml b/.air.hold.toml index 0b8a723..8450205 100644 --- a/.air.hold.toml +++ b/.air.hold.toml @@ -3,7 +3,8 @@ tmp_dir = "tmp" [build] pre_cmd = ["go generate ./pkg/hold/..."] -cmd = "go build -buildvcs=false -o ./tmp/atcr-hold ./cmd/hold" +# GO_TAGS (set by Dockerfile.dev / `make dev`) supplies build tags, e.g. testmode. +cmd = "go build ${GO_TAGS:+-tags $GO_TAGS} -buildvcs=false -o ./tmp/atcr-hold ./cmd/hold" entrypoint = ["./tmp/atcr-hold", "serve", "--config", "config-hold.example.yaml"] include_ext = ["go", "html", "css", "js"] exclude_dir = ["bin", "tmp", "vendor", "deploy", "docs", ".git", "dist", "node_modules", "scanner", "pkg/appview", "pkg/labeler"] diff --git a/.air.labeler.toml b/.air.labeler.toml index ed460a7..b0646df 100644 --- a/.air.labeler.toml +++ b/.air.labeler.toml @@ -2,7 +2,8 @@ root = "." tmp_dir = "tmp" [build] -cmd = "go build -buildvcs=false -o ./tmp/atcr-labeler ./cmd/labeler" +# GO_TAGS (set by Dockerfile.dev / `make dev`) supplies build tags, e.g. testmode. +cmd = "go build ${GO_TAGS:+-tags $GO_TAGS} -buildvcs=false -o ./tmp/atcr-labeler ./cmd/labeler" entrypoint = ["./tmp/atcr-labeler", "serve", "--config", "config-labeler.example.yaml"] include_ext = ["go", "html", "css", "js"] exclude_dir = ["bin", "tmp", "vendor", "deploy", "docs", ".git", "dist", "node_modules", "scanner", "pkg/appview", "pkg/hold"] diff --git a/.air.toml b/.air.toml index af8f69c..fba3ff4 100644 --- a/.air.toml +++ b/.air.toml @@ -7,7 +7,8 @@ poll = true poll_interval = 500 # Pre-build: generate assets if missing (each string is a shell command) pre_cmd = ["go generate ./pkg/appview/..."] -cmd = "go build -tags billing -buildvcs=false -o ./tmp/atcr-appview ./cmd/appview" +# GO_TAGS (set by Dockerfile.dev / `make dev`) appends build tags, e.g. testmode. +cmd = "go build -tags billing${GO_TAGS:+,$GO_TAGS} -buildvcs=false -o ./tmp/atcr-appview ./cmd/appview" entrypoint = ["./tmp/atcr-appview", "serve", "--config", "config-appview.example.yaml"] include_ext = ["go", "html", "css", "js"] exclude_dir = ["bin", "tmp", "vendor", "deploy", "docs", ".git", "dist", "node_modules", "scanner", "pkg/hold", "pkg/labeler"] diff --git a/CLAUDE.md b/CLAUDE.md index 03a49c8..8a015bf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,11 +39,16 @@ cd cmd/credential-helper/atcr && go build -o ../../../bin/docker-credential-atcr # Build appview with billing support (optional build tag; billing lives in pkg/billing/, appview-side only) go build -tags billing -o bin/atcr-appview ./cmd/appview -# Tests -go test ./... # all tests -go test ./pkg/atproto/... # specific package -go test -run TestManifestStore ./pkg/atproto/... # specific test -go test -race ./... # race detector +# Tests (run as a `testmode` build, see below) +go test -tags testmode ./... # all tests +go test -tags testmode ./pkg/atproto/... # specific package +go test -tags testmode -run TestManifestStore ./pkg/atproto/... # specific test +go test -tags testmode -race ./... # race detector +make test # same, every workspace module + +# Local run against a loopback hold / PDS: build with the testmode tag +go build -tags testmode -o bin/atcr-appview ./cmd/appview +go build -tags testmode -o bin/atcr-hold ./cmd/hold # Docker docker build -f Dockerfile.appview -t atcr.io/appview:latest . @@ -71,6 +76,27 @@ go run ./cmd/s3-test # S3 connectivity test go run ./cmd/healthcheck # HTTP health check (for Docker) ``` +### The `testmode` build tag + +`pkg/atproto/indigo_prod.go` (`!testmode`) and `pkg/atproto/indigo_local.go` +(`testmode`) are the only place ATCR departs from indigo's defaults. Both define +`NewDirectory()`, `NewOAuthClientApp()`, and the constant `TestModeBuild`; all +identity and OAuth construction in the repo goes through them. Production builds +get indigo's hardened directory and HTTP clients (SSRF-guarded, HTTPS-only +did:web). A `-tags testmode` build wraps the directory so a did:web naming an IP, +localhost, or any host with a port resolves over plain HTTP, and gives the OAuth +client plain HTTP clients so it can reach a PDS on loopback. + +- Tests, the integration harness, `make dev`, and docker-compose all build with + the tag (`make test`, `TEST_TAGS` in the Makefile, `GO_TAGS` in Air/Dockerfile.dev). +- Production Dockerfiles and `make build-trixie` never set it. There is no + runtime switch: a production binary cannot be configured to resolve local DIDs. +- Tests that need loopback did:web resolution call `t.Fatal` naming the tag + when `atproto.TestModeBuild` is false, so an untagged `go test` says why. +- `server.test_mode` (runtime) still gates the remaining behavioral branches + (hold issuer-mismatch tolerance, backfill warning suppression, registry + fall-back-to-default-hold); it no longer affects DID resolution. + ## Architecture Overview ATCR uses **distribution/distribution** as a library, extending it via middleware to route content to different backends: diff --git a/Dockerfile.dev b/Dockerfile.dev index 48d25cb..e9cf7aa 100644 --- a/Dockerfile.dev +++ b/Dockerfile.dev @@ -4,9 +4,15 @@ FROM mirror.gcr.io/library/golang:1.26.7-trixie ARG AIR_CONFIG=.air.toml +# Extra Go build tags for Air's build command (comma-separated). docker-compose +# passes `testmode` so the dev stack can resolve its loopback did:web +# identities. Production images (Dockerfile.appview, Dockerfile.hold) never +# set this and never carry the tag. +ARG GO_TAGS="" ENV DEBIAN_FRONTEND=noninteractive ENV AIR_CONFIG=${AIR_CONFIG} +ENV GO_TAGS=${GO_TAGS} RUN apt-get update && \ apt-get install -y --no-install-recommends sqlite3 libsqlite3-dev curl nodejs npm && \ diff --git a/Makefile b/Makefile index ed1209f..91487fe 100644 --- a/Makefile +++ b/Makefile @@ -143,33 +143,40 @@ define test-submodules done endef +# Tests run as a `testmode` build. The tag compiles pkg/atproto/indigo_local.go +# in place of indigo_prod.go, which is what lets a did:web on a loopback port +# (every test server, the integration harness, docker-compose) resolve over +# plain HTTP. Production binaries never carry the tag; there is no runtime +# switch. Tests that need it fail fast with a message naming the tag. +TEST_TAGS := testmode + test: test-billing ## Run all tests (every workspace module) @echo "→ Running tests..." - go test -cover ./... - $(call test-submodules,-cover) + go test -tags $(TEST_TAGS) -cover ./... + $(call test-submodules,-tags $(TEST_TAGS) -cover) # pkg/billing is behind the `billing` build tag, so `go test ./...` never # compiles it, let alone runs it. Its tests covered the money path and had # never executed in this target or in CI. test-billing: ## Run the billing-tagged tests (skipped by plain `go test ./...`) @echo "→ Running billing-tagged tests..." - go test -tags billing -cover ./pkg/billing/... + go test -tags billing,$(TEST_TAGS) -cover ./pkg/billing/... test-race: ## Run tests with race detector (every workspace module) @echo "→ Running tests with race detector..." - go test -race ./... + go test -race -tags $(TEST_TAGS) ./... @echo "→ Running billing-tagged tests with race detector..." - go test -race -tags billing ./pkg/billing/... - $(call test-submodules,-race) + go test -race -tags billing,$(TEST_TAGS) ./pkg/billing/... + $(call test-submodules,-race -tags $(TEST_TAGS)) test-verbose: ## Run tests with verbose output (every workspace module) @echo "→ Running tests with verbose output..." - go test -v ./... - $(call test-submodules,-v) + go test -v -tags $(TEST_TAGS) ./... + $(call test-submodules,-v -tags $(TEST_TAGS)) integration-test: ## Run in-process smoke test (no docker, fake PDS + gofakes3 + hold + appview) @echo "→ Running integration smoke test..." - go test -tags=integration -count=1 -race -timeout=120s ./test/integration/... + go test -tags=integration,$(TEST_TAGS) -count=1 -race -timeout=120s ./test/integration/... stripe-integration-test: ## Run Stripe sandbox-backed billing tests (needs STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET, STRIPE_TEST_PRICE_MONTHLY, STRIPE_TEST_PRICE_YEARLY) @echo "→ Running Stripe sandbox integration tests..." @@ -202,6 +209,8 @@ lint: check-golangci-lint ## Run golangci-lint golangci-lint run ./... @echo "→ Running golangci-lint (billing tag)..." golangci-lint run --build-tags=billing ./pkg/billing/... + @echo "→ Running golangci-lint (testmode tag)..." + golangci-lint run --build-tags=testmode ./pkg/atproto/... lex-lint: ## Lint ATProto lexicon schemas goat lex lint ./lexicons/ @@ -215,7 +224,10 @@ install-credential-helper: build-credential-helper ## Install credential helper ##@ Development Targets -dev: $(GENERATED_ASSETS) ## Run AppView locally with Air hot reload +# Air's build command appends $GO_TAGS to its -tags list; a local run needs +# `testmode` for the same reason the tests do (see TEST_TAGS above). +dev: export GO_TAGS ?= testmode +dev: $(GENERATED_ASSETS) ## Run AppView locally with Air hot reload (testmode build) @which air > /dev/null || (echo "→ Installing Air..." && go install github.com/air-verse/air@latest) air -c .air.toml diff --git a/cmd/relay-compare/main.go b/cmd/relay-compare/main.go index f1b5c38..f0d2e7d 100644 --- a/cmd/relay-compare/main.go +++ b/cmd/relay-compare/main.go @@ -19,6 +19,7 @@ import ( "sync" "time" + "atcr.io/pkg/atproto" "github.com/bluesky-social/indigo/atproto/identity" "github.com/bluesky-social/indigo/atproto/syntax" "github.com/bluesky-social/indigo/xrpc" @@ -137,7 +138,7 @@ func main() { ctx, cancel := context.WithTimeout(context.Background(), *timeout) defer cancel() - dir = identity.DefaultDirectory() + dir = atproto.NewDirectory() // Short display names for each relay names := make([]string, len(relays)) diff --git a/docker-compose.yml b/docker-compose.yml index 6665b7c..4f34c34 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -3,6 +3,8 @@ services: build: context: . dockerfile: Dockerfile.dev + args: + GO_TAGS: testmode image: atcr-appview-dev:latest container_name: atcr-appview # Option B: share the hold's network namespace so "localhost:8080" reaches the @@ -98,6 +100,7 @@ services: dockerfile: Dockerfile.dev args: AIR_CONFIG: .air.hold.toml + GO_TAGS: testmode image: atcr-hold-dev:latest container_name: atcr-hold ports: @@ -146,6 +149,7 @@ services: dockerfile: Dockerfile.dev args: AIR_CONFIG: .air.labeler.toml + GO_TAGS: testmode image: atcr-labeler-dev:latest container_name: atcr-labeler ports: diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 85bf320..0ddd948 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -128,7 +128,8 @@ poll = true poll_interval = 500 # Pre-build: generate assets if missing (each string is a shell command) pre_cmd = ["go generate ./pkg/appview/..."] -cmd = "go build -tags billing -buildvcs=false -o ./tmp/atcr-appview ./cmd/appview" +# GO_TAGS (set by Dockerfile.dev / `make dev`) appends build tags, e.g. testmode. +cmd = "go build -tags billing${GO_TAGS:+,$GO_TAGS} -buildvcs=false -o ./tmp/atcr-appview ./cmd/appview" entrypoint = ["./tmp/atcr-appview", "serve", "--config", "config-appview.example.yaml"] include_ext = ["go", "html", "css", "js"] exclude_dir = ["bin", "tmp", "vendor", "deploy", "docs", ".git", "dist", "node_modules", "scanner", "pkg/hold", "pkg/labeler"] @@ -145,7 +146,15 @@ Key points that differ from a naive config: - `pre_cmd` runs `go generate ./pkg/appview/...`, which regenerates and re-embeds CSS/JS/icons before the build. - `cmd` builds with `-tags billing` (AppView dev runs with billing support) and - `-buildvcs=false`. + `-buildvcs=false`. Air runs the command through `sh -c`, so `${GO_TAGS:+,$GO_TAGS}` + appends whatever `GO_TAGS` holds. docker-compose passes `GO_TAGS: testmode` as a + build arg to `Dockerfile.dev`, and `make dev` exports the same, so every dev + build is a **testmode build**: `pkg/atproto/indigo_local.go` replaces + `indigo_prod.go`, letting a did:web on an IP, `localhost`, or any port (the + hold's `did:web:localhost%3A8080`, the appview's `did:web:127.0.0.1%3A5000`, + the labeler's `did:web:172.28.0.4%3A5002`) resolve over plain HTTP, and letting + the OAuth client reach a PDS on loopback. Production images never set the tag + and cannot be configured to resolve local DIDs at runtime. - `entrypoint` is the full argv for the built binary: it runs `serve --config config-appview.example.yaml`. (The example config is the dev base config; env vars in `docker-compose.yml` override it.) @@ -273,14 +282,21 @@ builds the generated assets, and runs `air -c .air.toml`. You can also run Air directly, or skip hot reload entirely: ```bash -# Air, AppView config -air -c .air.toml +# Air, AppView config (GO_TAGS makes it a testmode build, as `make dev` does) +GO_TAGS=testmode air -c .air.toml # No hot reload — build and run once -go build -tags billing -o bin/atcr-appview ./cmd/appview +go build -tags billing,testmode -o bin/atcr-appview ./cmd/appview ./bin/atcr-appview serve --config config-appview.example.yaml ``` +Leave `testmode` off only when the appview talks exclusively to public +identities (a real PDS, a hold on a public HTTPS hostname); with it off, any +did:web naming an IP, `localhost`, or a port fails to resolve, exactly as in +production. Tests need the tag too: `make test` sets it, and a bare +`go test ./...` fails fast in the tests that depend on it with a message naming +the tag. + Running on the host requires a working toolchain for the build: Go 1.26.7 (see `go.work`), Node/npm (for the `go generate` asset step), and SQLite headers. Override config values with the `ATCR_*` env vars listed above, diff --git a/internal/testharness/harness.go b/internal/testharness/harness.go index 2c5f734..0b77724 100644 --- a/internal/testharness/harness.go +++ b/internal/testharness/harness.go @@ -117,6 +117,14 @@ func New(t *testing.T, opts ...Option) *Harness { } h := &Harness{t: t} + // The stack's holds and appview identify themselves by did:web on loopback + // ports. Only a `-tags testmode` build resolves those (pkg/atproto's + // indigo_local.go); a production build would fail every lookup with an + // opaque dial error, so say why up front. + if !atproto.TestModeBuild { + t.Fatal("the integration harness needs a `-tags testmode` build: go test -tags integration,testmode ./test/integration/...") + } + // 1. Fake PDS. h.PDS = testpds.New(t) atproto.SetDirectory(h.PDS.Directory()) diff --git a/pkg/appview/authgate/testhelpers_test.go b/pkg/appview/authgate/testhelpers_test.go index 5f2e7a9..7e85134 100644 --- a/pkg/appview/authgate/testhelpers_test.go +++ b/pkg/appview/authgate/testhelpers_test.go @@ -5,12 +5,12 @@ import ( "database/sql" "net/http" "net/http/httptest" - "strings" "testing" "atcr.io/pkg/appview/db" "atcr.io/pkg/atproto" "atcr.io/pkg/auth" + "atcr.io/pkg/testpds" ) // newTestDB returns an in-memory libsql DB with the full appview schema @@ -72,26 +72,33 @@ type quotaServerResult struct { // quotaServer spins up an httptest.Server that responds to every request // with the given status + body, records hit count + last URL, and returns -// both the server URL and the did:web:HOST form that resolves to it under -// atproto.SetTestMode(true). +// both the server URL and the did:web:HOST form that resolves to it. The +// server also serves its own did:web document (not counted in hits) so the +// test-mode identity directory can resolve that DID back to the server. func quotaServer(t *testing.T, status int, body string) *quotaServerResult { t.Helper() + if !atproto.TestModeBuild { + t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") + } res := "aServerResult{} - res.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) + }) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { res.hits++ res.lastURL = r.URL.String() w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) _, _ = w.Write([]byte(body)) - })) + }) + res.server = httptest.NewServer(mux) t.Cleanup(res.server.Close) - // httptest.Server.URL has the form "http://127.0.0.1:PORT". The - // did:web equivalent percent-encodes the colon; didWebToURL reverses - // the encoding and re-derives the http://host:port form, which is - // what we need for atproto.SetTestMode(true) to route requests. - host := strings.TrimPrefix(res.server.URL, "http://") - res.holdDID = "did:web:" + strings.Replace(host, ":", "%3A", 1) + // httptest.Server.URL has the form "http://127.0.0.1:PORT"; did:web + // percent-encodes the port colon. + res.holdDID = testpds.DIDWebForURL(res.server.URL) return res } diff --git a/pkg/appview/holdclient/tier_update_test.go b/pkg/appview/holdclient/tier_update_test.go index 134a5fa..184e206 100644 --- a/pkg/appview/holdclient/tier_update_test.go +++ b/pkg/appview/holdclient/tier_update_test.go @@ -10,6 +10,7 @@ import ( "time" "atcr.io/pkg/atproto" + "atcr.io/pkg/testpds" "github.com/bluesky-social/indigo/atproto/atcrypto" ) @@ -89,14 +90,25 @@ func TestUpdateCrewTierOnHold_PostsToEndpoint(t *testing.T) { // function the Stripe webhook actually calls, and the one whose error decides // whether a paid upgrade is retried or dropped — had no test at all. -// didFor turns an httptest server URL into a did:web that ResolveHoldDIDToURL -// maps straight back to it. The port makes DIDWebToURL choose http, and test -// mode is what lets a did:web that no directory can resolve fall back to being -// decoded from the DID itself. -func didFor(t *testing.T, serverURL string) string { +// holdServer starts an httptest server standing in for a hold and returns it +// with the did:web that ResolveHoldDIDToURL maps back to it. The server serves +// its own DID document at /.well-known/did.json (always promptly, whatever the +// hold handler does), and handler gets every other request. Resolving a +// loopback did:web is what a `-tags testmode` build provides. +func holdServer(t *testing.T, handler http.HandlerFunc) (*httptest.Server, string) { t.Helper() - host := strings.TrimPrefix(serverURL, "http://") - return "did:web:" + strings.ReplaceAll(host, ":", "%3A") + if !atproto.TestModeBuild { + t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") + } + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) + }) + mux.Handle("/", handler) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv, testpds.DIDWebForURL(srv.URL) } func testKey(t *testing.T) *atcrypto.PrivateKeyP256 { @@ -116,20 +128,15 @@ func TestUpdateCrewTierOnAllHolds_JoinedErrorNamesEveryFailingHold(t *testing.T) atproto.SetTestMode(true) t.Cleanup(func() { atproto.SetTestMode(false) }) - ok := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, okDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) - })) - defer ok.Close() - bad1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + }) + _, bad1DID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { http.Error(w, "down", http.StatusServiceUnavailable) - })) - defer bad1.Close() - bad2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + }) + _, bad2DID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { http.Error(w, "broken", http.StatusInternalServerError) - })) - defer bad2.Close() - - okDID, bad1DID, bad2DID := didFor(t, ok.URL), didFor(t, bad1.URL), didFor(t, bad2.URL) + }) err := UpdateCrewTierOnAllHolds(context.Background(), []string{okDID, bad1DID, bad2DID}, "did:plc:user", 1, testKey(t), "did:web:appview") @@ -159,25 +166,23 @@ func TestUpdateCrewTierOnAllHolds_SlowHoldDoesNotStarveOthers(t *testing.T) { t.Cleanup(func() { atproto.SetTestMode(false) }) release := make(chan struct{}) - slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, slowDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { <-release - })) - defer slow.Close() + }) defer close(release) var healthyHits atomic.Int32 - healthy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, healthyDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { healthyHits.Add(1) w.WriteHeader(http.StatusOK) - })) - defer healthy.Close() + }) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() // Slow hold first: serial contact would spend the entire budget on it. err := UpdateCrewTierOnAllHolds(ctx, - []string{didFor(t, slow.URL), didFor(t, healthy.URL)}, + []string{slowDID, healthyDID}, "did:plc:user", 1, testKey(t), "did:web:appview") if err == nil { @@ -203,11 +208,10 @@ func TestUpdateCrewTierOnAllHolds_DeadlineCutsRetriesShort(t *testing.T) { release := make(chan struct{}) var attempts atomic.Int32 - hung := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, hungDID := holdServer(t, func(w http.ResponseWriter, r *http.Request) { attempts.Add(1) <-release - })) - defer hung.Close() + }) defer close(release) // Deadline deliberately shorter than tierUpdateMaxAttempts would need. @@ -215,7 +219,7 @@ func TestUpdateCrewTierOnAllHolds_DeadlineCutsRetriesShort(t *testing.T) { defer cancel() start := time.Now() - err := UpdateCrewTierOnAllHolds(ctx, []string{didFor(t, hung.URL)}, + err := UpdateCrewTierOnAllHolds(ctx, []string{hungDID}, "did:plc:user", 1, testKey(t), "did:web:appview") elapsed := time.Since(start) diff --git a/pkg/appview/jetstream/processor.go b/pkg/appview/jetstream/processor.go index 93ffcec..f129bb7 100644 --- a/pkg/appview/jetstream/processor.go +++ b/pkg/appview/jetstream/processor.go @@ -11,7 +11,6 @@ import ( "atcr.io/pkg/appview/db" "atcr.io/pkg/atproto" atpdata "github.com/bluesky-social/indigo/atproto/atdata" - "github.com/bluesky-social/indigo/atproto/identity" "github.com/bluesky-social/indigo/atproto/lexicon" ) @@ -43,7 +42,7 @@ func (p *Processor) SetWebhookDispatcher(d WebhookDispatcher) { // statsCache: shared stats cache for aggregating across holds (nil to skip stats processing) func NewProcessor(database db.DBTX, useCache bool, statsCache *StatsCache) *Processor { // Create lexicon catalog for debug validation logging - dir := identity.DefaultDirectory() + dir := atproto.NewDirectory() catalog := lexicon.NewResolvingCatalog() catalog.Directory = dir diff --git a/pkg/appview/middleware/registry_test.go b/pkg/appview/middleware/registry_test.go index d19afe1..4df30e9 100644 --- a/pkg/appview/middleware/registry_test.go +++ b/pkg/appview/middleware/registry_test.go @@ -405,14 +405,11 @@ func TestHoldResolutionError_PermanentVsTransient(t *testing.T) { // 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" + // Port 1 (tcpmux) rather than 8080: a `-tags testmode` build really + // does try http://localhost:PORT/.well-known/did.json, and a dev hold + // listening on 8080 would turn this into a successful resolution. + const holdDID = "did:web:localhost%3A1" _, err := atproto.ResolveHoldURL(context.Background(), holdDID) require.Error(t, err) diff --git a/pkg/atproto/directory.go b/pkg/atproto/directory.go index 9ad06e3..d2c9660 100644 --- a/pkg/atproto/directory.go +++ b/pkg/atproto/directory.go @@ -39,8 +39,11 @@ func SetDirectory(d identity.Directory) { } // GetDirectory returns the shared identity.Directory. On first call (and if -// SetDirectory has not been used), it constructs an indigo cached directory -// with a 24h TTL backed by Jetstream event-driven invalidation. +// SetDirectory has not been used), it constructs the directory via +// NewDirectory: indigo's cached directory (24h TTL, Jetstream-driven +// invalidation) in production builds, and the same wrapped with plain-HTTP +// resolution of local did:web identifiers in `-tags testmode` builds. See +// indigo_prod.go and indigo_local.go. // // Using a shared instance ensures all identity lookups across the application // use the same cache, which is more memory-efficient and provides better cache @@ -49,7 +52,7 @@ func GetDirectory() identity.Directory { directoryMu.Lock() defer directoryMu.Unlock() if sharedDirectory == nil { - sharedDirectory = identity.DefaultDirectory() + sharedDirectory = NewDirectory() } return sharedDirectory } diff --git a/pkg/atproto/indigo_local.go b/pkg/atproto/indigo_local.go new file mode 100644 index 0000000..888a09c --- /dev/null +++ b/pkg/atproto/indigo_local.go @@ -0,0 +1,181 @@ +//go:build testmode + +package atproto + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net" + "net/http" + "strings" + "time" + + "github.com/bluesky-social/indigo/atproto/auth/oauth" + "github.com/bluesky-social/indigo/atproto/identity" + "github.com/bluesky-social/indigo/atproto/syntax" +) + +// TestModeBuild reports whether this binary was compiled with `-tags testmode`. +// +// This file is the only place ATCR departs from indigo's defaults, and it is +// compiled out of production binaries entirely. A test-mode build resolves +// did:web identifiers that name loopback, private, or port-qualified hosts over +// plain HTTP, and lets the OAuth client talk to a PDS on such an address. Every +// other lookup goes through indigo exactly as it would in production. +const TestModeBuild = true + +// maxLocalDIDDocSize bounds a local did:web document read. Real documents are +// a few hundred bytes. +const maxLocalDIDDocSize = 1 << 20 + +// NewDirectory returns the identity directory used for every DID and handle +// lookup in this process. Test-mode builds wrap indigo's directory so that +// local did:web identifiers (an IP literal, localhost, or any host with a port) +// resolve by fetching http://host[:port]/.well-known/did.json directly. Those +// lookups are not cached: a dev hold or appview restarts often and its +// document is one loopback GET away. +func NewDirectory() identity.Directory { + base := identity.BaseDirectory{ + PLCURL: identity.DefaultPLCURL, + // No Transport means http.DefaultTransport: no SSRF dialer, so a + // test-mode build can also resolve well-known handles served locally. + HTTPClient: http.Client{Timeout: 10 * time.Second}, + PLCClient: &http.Client{Timeout: 10 * time.Second}, + Resolver: net.Resolver{ + Dial: func(ctx context.Context, network, address string) (net.Conn, error) { + d := net.Dialer{Timeout: 3 * time.Second} + return d.DialContext(ctx, network, address) + }, + }, + TryAuthoritativeDNS: true, + SkipDNSDomainSuffixes: []string{".bsky.social"}, + UserAgent: "atcr-identity/testmode", + } + inner := identity.NewCacheDirectory(&base, 250_000, 24*time.Hour, 2*time.Minute, 5*time.Minute) + return &localDirectory{ + inner: inner, + client: &http.Client{Timeout: 10 * time.Second}, + } +} + +// NewOAuthClientApp constructs an indigo OAuth client app for ATCR. Test-mode +// builds replace the HTTP clients indigo uses for PAR, token, and auth-server +// metadata requests with plain timeout-only clients, so the flow can reach a +// PDS on loopback or a private address. The identity directory is the shared +// process-wide one, which in this build already understands local did:web. +func NewOAuthClientApp(cfg *oauth.ClientConfig, store oauth.ClientAuthStore) *oauth.ClientApp { + app := oauth.NewClientApp(cfg, store) + app.Client = &http.Client{Timeout: 30 * time.Second} + app.Resolver.Client = &http.Client{Timeout: 10 * time.Second} + app.Dir = GetDirectory() + return app +} + +// localDirectory implements identity.Directory. It handles did:web identifiers +// that name a local host itself and delegates everything else to inner. +type localDirectory struct { + inner identity.Directory + client *http.Client +} + +var _ identity.Directory = (*localDirectory)(nil) + +func (d *localDirectory) LookupDID(ctx context.Context, did syntax.DID) (*identity.Identity, error) { + if docURL, ok := localDIDWebURL(did); ok { + return d.resolveLocalDIDWeb(ctx, did, docURL) + } + return d.inner.LookupDID(ctx, did) +} + +func (d *localDirectory) LookupHandle(ctx context.Context, handle syntax.Handle) (*identity.Identity, error) { + return d.inner.LookupHandle(ctx, handle) +} + +func (d *localDirectory) Lookup(ctx context.Context, atid syntax.AtIdentifier) (*identity.Identity, error) { + if did, err := atid.AsDID(); err == nil { + return d.LookupDID(ctx, did) + } + return d.inner.Lookup(ctx, atid) +} + +func (d *localDirectory) Purge(ctx context.Context, atid syntax.AtIdentifier) error { + return d.inner.Purge(ctx, atid) +} + +// localDIDWebURL reports whether did is a did:web naming a local host, and if +// so returns the plain-HTTP URL of its DID document. +// +// did:web encodes a port as %3A and uses a bare ':' to separate optional path +// segments, so "did:web:localhost%3A8080" is http://localhost:8080 and +// "did:web:127.0.0.1:hold" would be http://127.0.0.1/hold/did.json. A host is +// local when it is an IP literal, localhost (or a *.localhost name), or carries +// a port at all: indigo rejects every one of those, and none of them can be +// anything but a development endpoint. +func localDIDWebURL(did syntax.DID) (string, bool) { + if did.Method() != "web" { + return "", false + } + segments := strings.Split(did.Identifier(), ":") + hostPort := strings.NewReplacer("%3A", ":", "%3a", ":").Replace(segments[0]) + if hostPort == "" { + return "", false + } + + hostname := hostPort + hasPort := false + if h, _, err := net.SplitHostPort(hostPort); err == nil { + hostname = h + hasPort = true + } + local := hasPort || + hostname == "localhost" || + strings.HasSuffix(hostname, ".localhost") || + net.ParseIP(hostname) != nil + if !local { + return "", false + } + + path := "/.well-known/did.json" + if len(segments) > 1 { + path = "/" + strings.Join(segments[1:], "/") + "/did.json" + } + return "http://" + hostPort + path, true +} + +// resolveLocalDIDWeb fetches and parses a local did:web document. The handle +// is left as handle.invalid: nothing on a loopback address has a verifiable +// handle, and no caller in this codebase reads it for a hold or appview DID. +func (d *localDirectory) resolveLocalDIDWeb(ctx context.Context, did syntax.DID, docURL string) (*identity.Identity, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, docURL, nil) + if err != nil { + return nil, fmt.Errorf("%w: local did:web request for %s: %w", identity.ErrDIDResolutionFailed, did, err) + } + resp, err := d.client.Do(req) + if err != nil { + return nil, fmt.Errorf("%w: local did:web fetch %s: %w", identity.ErrDIDResolutionFailed, docURL, err) + } + defer resp.Body.Close() + + switch { + case resp.StatusCode == http.StatusNotFound: + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, maxLocalDIDDocSize)) + return nil, fmt.Errorf("%w: local did:web HTTP 404 for %s", identity.ErrDIDNotFound, did) + case resp.StatusCode != http.StatusOK: + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, maxLocalDIDDocSize)) + return nil, fmt.Errorf("%w: local did:web HTTP %d for %s", identity.ErrDIDResolutionFailed, resp.StatusCode, did) + } + + var doc identity.DIDDocument + if err := json.NewDecoder(io.LimitReader(resp.Body, maxLocalDIDDocSize)).Decode(&doc); err != nil { + return nil, fmt.Errorf("%w: local did:web document for %s: %w", identity.ErrDIDResolutionFailed, did, err) + } + // ParseIdentity keeps only keys whose controller matches the document's own + // id, so a document that omits both still yields its keys. Either way the + // identity is for the DID that was asked for. + ident := identity.ParseIdentity(&doc) + ident.DID = did + ident.Handle = syntax.HandleInvalid + return &ident, nil +} diff --git a/pkg/atproto/indigo_local_test.go b/pkg/atproto/indigo_local_test.go new file mode 100644 index 0000000..c65cad2 --- /dev/null +++ b/pkg/atproto/indigo_local_test.go @@ -0,0 +1,149 @@ +//go:build testmode + +package atproto + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/bluesky-social/indigo/atproto/atcrypto" + "github.com/bluesky-social/indigo/atproto/identity" + "github.com/bluesky-social/indigo/atproto/syntax" +) + +func TestLocalDIDWebURL(t *testing.T) { + cases := []struct { + did string + want string + local bool + }{ + {"did:web:localhost%3A8080", "http://localhost:8080/.well-known/did.json", true}, + {"did:web:127.0.0.1%3A5000", "http://127.0.0.1:5000/.well-known/did.json", true}, + {"did:web:172.28.0.4%3A5002", "http://172.28.0.4:5002/.well-known/did.json", true}, + {"did:web:localhost", "http://localhost/.well-known/did.json", true}, + {"did:web:hold.localhost", "http://hold.localhost/.well-known/did.json", true}, + {"did:web:10.0.0.7", "http://10.0.0.7/.well-known/did.json", true}, + {"did:web:example.com%3A8443", "http://example.com:8443/.well-known/did.json", true}, + {"did:web:127.0.0.1:hold", "http://127.0.0.1/hold/did.json", true}, + {"did:web:hold01.atcr.io", "", false}, + {"did:web:atcr.io:path", "", false}, + {"did:plc:aaaaaaaaaaaaaaaaaaaaaaaa", "", false}, + } + for _, tc := range cases { + t.Run(tc.did, func(t *testing.T) { + did, err := syntax.ParseDID(tc.did) + if err != nil { + t.Fatalf("ParseDID(%q): %v", tc.did, err) + } + got, local := localDIDWebURL(did) + if local != tc.local { + t.Fatalf("localDIDWebURL(%q) local = %v, want %v", tc.did, local, tc.local) + } + if got != tc.want { + t.Errorf("localDIDWebURL(%q) = %q, want %q", tc.did, got, tc.want) + } + }) + } +} + +// TestLocalDirectoryResolvesLoopbackDIDWeb is the end-to-end check for the +// test-mode seam: a did:web naming a loopback host with a port resolves through +// the shared directory by fetching its document over plain HTTP, and the +// resolver helpers built on the directory need no fallback of their own. +func TestLocalDirectoryResolvesLoopbackDIDWeb(t *testing.T) { + priv, err := atcrypto.GeneratePrivateKeyP256() + if err != nil { + t.Fatalf("generate P-256: %v", err) + } + pub, err := priv.PublicKey() + if err != nil { + t.Fatalf("public key: %v", err) + } + + mux := http.NewServeMux() + var srv *httptest.Server + mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { + did := "did:web:" + strings.Replace(strings.TrimPrefix(srv.URL, "http://"), ":", "%3A", 1) + _ = json.NewEncoder(w).Encode(map[string]any{ + "id": did, + "verificationMethod": []map[string]string{{ + "id": did + "#appview", + "type": "Multikey", + "controller": did, + "publicKeyMultibase": pub.Multibase(), + }}, + "service": []map[string]string{ + {"id": "#atproto_pds", "type": "AtprotoPersonalDataServer", "serviceEndpoint": srv.URL}, + {"id": "#atcr_hold", "type": "AtcrHoldService", "serviceEndpoint": srv.URL}, + }, + }) + }) + srv = httptest.NewServer(mux) + t.Cleanup(srv.Close) + + did := "did:web:" + strings.Replace(strings.TrimPrefix(srv.URL, "http://"), ":", "%3A", 1) + + SetDirectory(NewDirectory()) + t.Cleanup(func() { SetDirectory(nil) }) + ctx := context.Background() + + ident, err := GetDirectory().LookupDID(ctx, syntax.DID(did)) + if err != nil { + t.Fatalf("LookupDID(%q): %v", did, err) + } + if ident.DID.String() != did { + t.Errorf("identity DID = %q, want %q", ident.DID, did) + } + if ident.Handle != syntax.HandleInvalid { + t.Errorf("identity handle = %q, want handle.invalid (no verification on loopback)", ident.Handle) + } + if _, err := ident.GetPublicKey("appview"); err != nil { + t.Errorf("GetPublicKey(appview): %v", err) + } + + url, err := ResolveHoldDIDToURL(ctx, did) + if err != nil { + t.Fatalf("ResolveHoldDIDToURL(%q): %v", did, err) + } + if url != srv.URL { + t.Errorf("ResolveHoldDIDToURL = %q, want %q", url, srv.URL) + } + + isHold, err := HasHoldService(ctx, did) + if err != nil { + t.Fatalf("HasHoldService(%q): %v", did, err) + } + if !isHold { + t.Error("HasHoldService = false, want true: document declares #atcr_hold") + } + + // Lookup by at-identifier takes the same path. + atid, _ := syntax.ParseAtIdentifier(did) + if _, err := GetDirectory().Lookup(ctx, atid); err != nil { + t.Errorf("Lookup(%q): %v", did, err) + } +} + +func TestLocalDirectoryMissingDocumentIsNotFound(t *testing.T) { + srv := httptest.NewServer(http.NotFoundHandler()) + t.Cleanup(srv.Close) + did := "did:web:" + strings.Replace(strings.TrimPrefix(srv.URL, "http://"), ":", "%3A", 1) + + _, err := NewDirectory().LookupDID(context.Background(), syntax.DID(did)) + if !errors.Is(err, identity.ErrDIDNotFound) { + t.Fatalf("LookupDID on a 404 = %v, want ErrDIDNotFound", err) + } +} + +func TestLocalDirectoryUnreachableHostIsResolutionFailure(t *testing.T) { + // Port 1 (tcpmux) is never bound on a developer machine. + _, err := NewDirectory().LookupDID(context.Background(), syntax.DID("did:web:127.0.0.1%3A1")) + if !errors.Is(err, identity.ErrDIDResolutionFailed) { + t.Fatalf("LookupDID on an unreachable host = %v, want ErrDIDResolutionFailed", err) + } +} diff --git a/pkg/atproto/indigo_prod.go b/pkg/atproto/indigo_prod.go new file mode 100644 index 0000000..a5bb16c --- /dev/null +++ b/pkg/atproto/indigo_prod.go @@ -0,0 +1,33 @@ +//go:build !testmode + +package atproto + +import ( + "github.com/bluesky-social/indigo/atproto/auth/oauth" + "github.com/bluesky-social/indigo/atproto/identity" +) + +// TestModeBuild reports whether this binary was compiled with `-tags testmode`. +// +// A production build is never a test-mode build. The identity directory and +// OAuth client below are indigo's hardened defaults, which refuse loopback and +// private-network addresses and require HTTPS on port 443 for did:web. There +// is deliberately no runtime switch that can relax them. +const TestModeBuild = false + +// NewDirectory returns the identity directory used for every DID and handle +// lookup in this process. Production builds use indigo's default directory +// unchanged: PLC + did:web over HTTPS with SSRF protection, cached 24h. +func NewDirectory() identity.Directory { + return identity.DefaultDirectory() +} + +// NewOAuthClientApp constructs an indigo OAuth client app for ATCR. Production +// builds keep indigo's hardened HTTP clients for PAR, token, and auth-server +// metadata requests; only the identity directory is swapped for the shared +// process-wide one so OAuth lookups hit the same cache as everything else. +func NewOAuthClientApp(cfg *oauth.ClientConfig, store oauth.ClientAuthStore) *oauth.ClientApp { + app := oauth.NewClientApp(cfg, store) + app.Dir = GetDirectory() + return app +} diff --git a/pkg/atproto/indigo_prod_test.go b/pkg/atproto/indigo_prod_test.go new file mode 100644 index 0000000..a859edb --- /dev/null +++ b/pkg/atproto/indigo_prod_test.go @@ -0,0 +1,25 @@ +//go:build !testmode + +package atproto + +import ( + "context" + "testing" + + "github.com/bluesky-social/indigo/atproto/syntax" +) + +// TestProdDirectoryRejectsLocalDIDWeb pins the production contract: without +// `-tags testmode` there is no plain-HTTP path for a port-qualified or loopback +// did:web, so such an identifier fails before any network request is made +// (indigo rejects the identifier as not a plain hostname). +func TestProdDirectoryRejectsLocalDIDWeb(t *testing.T) { + if TestModeBuild { + t.Fatal("TestModeBuild must be false without -tags testmode") + } + for _, did := range []string{"did:web:localhost%3A8080", "did:web:127.0.0.1%3A5000"} { + if _, err := NewDirectory().LookupDID(context.Background(), syntax.DID(did)); err == nil { + t.Errorf("LookupDID(%q) succeeded in a production build; local did:web must not resolve", did) + } + } +} diff --git a/pkg/atproto/resolver.go b/pkg/atproto/resolver.go index 03447e9..9d35dda 100644 --- a/pkg/atproto/resolver.go +++ b/pkg/atproto/resolver.go @@ -29,6 +29,10 @@ var ErrHoldDIDPermanent = errors.New("hold DID is not resolvable") // error prose, is what lets ResolveHoldDIDToURL classify the failure as // permanent. Conservative by design: anything not clearly hopeless is left to // the transient path, so an unfamiliar failure still surfaces loudly. +// +// A `-tags testmode` build resolves exactly these identifiers over plain HTTP +// (see indigo_local.go), so there a failure means the local service is down +// rather than unresolvable; the quieter classification is harmless in dev. func didWebHostUnusable(did string) bool { host, ok := strings.CutPrefix(did, "did:web:") if !ok { @@ -138,12 +142,6 @@ func ResolveHoldDIDToURL(ctx context.Context, did string) (string, error) { ident, err := directory.LookupDID(ctx, didParsed) if err != nil { - // In test mode, fall back to deriving URL directly from did:web. - // The indigo directory hardcodes HTTPS and rejects IPs/ports, - // so local dev (HTTP, IP:port) always needs this fallback. - if testMode && strings.HasPrefix(did, "did:web:") { - return DIDWebToURL(did), nil - } // A missing DID document or a structurally unusable did:web will fail // identically on every retry, so mark it permanent and let callers pick // a quieter log level. @@ -180,12 +178,6 @@ func HasHoldService(ctx context.Context, did string) (bool, error) { ident, err := GetDirectory().LookupDID(ctx, didParsed) if err != nil { - // In test mode, local did:web identifiers (HTTP, IP:port) are not - // resolvable by the indigo directory at all — trust them, matching - // the ResolveHoldDIDToURL fallback. - if testMode && strings.HasPrefix(did, "did:web:") { - return true, nil - } return false, fmt.Errorf("failed to resolve DID %s: %w", did, err) } @@ -207,19 +199,6 @@ func NormalizeDID(did string) string { return "did:web:" + host } -// DIDWebToURL converts a did:web DID to its base URL. -// did:web:example.com → https://example.com -// did:web:172.28.0.3%3A8080 → http://172.28.0.3:8080 -func DIDWebToURL(did string) string { - host := strings.TrimPrefix(did, "did:web:") - host = strings.ReplaceAll(host, "%3A", ":") - scheme := "https" - if strings.Contains(host, ":") { - scheme = "http" - } - return scheme + "://" + host -} - // ResolveDIDToPDS resolves a DID to its PDS endpoint. // Uses the shared identity directory with cache TTL and event-driven invalidation. func ResolveDIDToPDS(ctx context.Context, did string) (string, error) { diff --git a/pkg/atproto/resolver_test.go b/pkg/atproto/resolver_test.go index 7d4f881..f0fc297 100644 --- a/pkg/atproto/resolver_test.go +++ b/pkg/atproto/resolver_test.go @@ -92,54 +92,13 @@ func TestHasHoldService(t *testing.T) { t.Error("HasHoldService(unresolvable) expected error, got nil") } }) -} -// TestHasHoldServiceTestMode covers the test-mode fallback: local did:web -// identifiers (HTTP, IP:port) that the indigo directory cannot resolve are -// trusted, matching the ResolveHoldDIDToURL fallback. Resolvable DIDs are -// still checked normally, and the fallback never applies to other methods. -func TestHasHoldServiceTestMode(t *testing.T) { - pdsOnlyDID := "did:plc:ordinaryaccount" - - SetDirectory(&stubDirectory{byDID: map[string]*identity.Identity{ - pdsOnlyDID: { - DID: syntax.DID(pdsOnlyDID), - Services: map[string]identity.ServiceEndpoint{ - "atproto_pds": {Type: "AtprotoPersonalDataServer", URL: "https://pds.example.com"}, - }, - }, - }}) - defer SetDirectory(nil) - - SetTestMode(true) - defer SetTestMode(false) - - ctx := context.Background() - - t.Run("trusts unresolvable did:web", func(t *testing.T) { - localDID := "did:web:172.28.0.3%3A8080" - isHold, err := HasHoldService(ctx, localDID) - if err != nil { - t.Fatalf("HasHoldService(%q) unexpected error: %v", localDID, err) - } - if !isHold { - t.Errorf("HasHoldService(%q) = false, want true (test-mode did:web fallback)", localDID) - } - }) - - t.Run("rejects unresolvable did:plc", func(t *testing.T) { - if _, err := HasHoldService(ctx, "did:plc:unknown000000000000"); err == nil { - t.Error("HasHoldService(unresolvable did:plc) expected error even in test mode, got nil") - } - }) - - t.Run("still checks resolvable DIDs", func(t *testing.T) { - isHold, err := HasHoldService(ctx, pdsOnlyDID) - if err != nil { - t.Fatalf("HasHoldService(%q) unexpected error: %v", pdsOnlyDID, err) - } - if isHold { - t.Errorf("HasHoldService(%q) = true, want false (fallback must not bypass a successful lookup)", pdsOnlyDID) + // Local did:web identifiers are no exception: resolving them is the + // identity directory's job (indigo_local.go under -tags testmode), and + // HasHoldService itself never trusts a DID it could not resolve. + t.Run("unresolvable local did:web", func(t *testing.T) { + if _, err := HasHoldService(ctx, "did:web:172.28.0.3%3A8080"); err == nil { + t.Error("HasHoldService(unresolvable did:web) expected error, got nil") } }) } diff --git a/pkg/auth/hold_remote_captain_verify_test.go b/pkg/auth/hold_remote_captain_verify_test.go index d304212..fed1896 100644 --- a/pkg/auth/hold_remote_captain_verify_test.go +++ b/pkg/auth/hold_remote_captain_verify_test.go @@ -10,6 +10,7 @@ import ( "time" "atcr.io/pkg/atproto" + "atcr.io/pkg/testpds" ) // 69307c0 verified captain records against the publishing DID's atcr_hold @@ -23,9 +24,18 @@ import ( // captainServer serves a captain record for any repo, with allowAllCrew set — // the value that makes a row visible to every user rather than just its author. +// +// It also serves its own did:web document so the test-mode identity directory +// can resolve the DID derived from its URL (see didFromServer) back to it. func captainServer(t *testing.T) *httptest.Server { t.Helper() - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requireTestModeBuild(t) + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) + }) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { _ = json.NewEncoder(w).Encode(map[string]any{ "uri": "at://x/io.atcr.hold.captain/self", "cid": "bafytest", @@ -36,11 +46,22 @@ func captainServer(t *testing.T) *httptest.Server { "allowAllCrew": true, }, }) - })) + }) + srv := httptest.NewServer(mux) t.Cleanup(srv.Close) return srv } +// requireTestModeBuild fails fast, with the reason, when the binary cannot +// resolve a loopback did:web. Without it these tests die on an opaque dial +// error from indigo's hardened directory. +func requireTestModeBuild(t *testing.T) { + t.Helper() + if !atproto.TestModeBuild { + t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") + } +} + func didFromServer(url string) string { return "did:web:" + strings.ReplaceAll(strings.TrimPrefix(url, "http://"), ":", "%3A") } diff --git a/pkg/auth/oauth/client.go b/pkg/auth/oauth/client.go index 36ddd8b..a4e01d0 100644 --- a/pkg/auth/oauth/client.go +++ b/pkg/auth/oauth/client.go @@ -100,8 +100,7 @@ func NewClientApp(baseURL string, store oauth.ClientAuthStore, scopes []string, slog.Info("Using public OAuth client (localhost development)") } - clientApp := oauth.NewClientApp(&config, store) - clientApp.Dir = atproto.GetDirectory() + clientApp := atproto.NewOAuthClientApp(&config, store) return clientApp, nil } @@ -132,8 +131,7 @@ func NewClientAppWithKey(baseURL string, store oauth.ClientAuthStore, scopes []s slog.Info("Using public OAuth client (localhost development)") } - clientApp := oauth.NewClientApp(&config, store) - clientApp.Dir = atproto.GetDirectory() + clientApp := atproto.NewOAuthClientApp(&config, store) return clientApp, nil } diff --git a/pkg/billing/tier_resolution_test.go b/pkg/billing/tier_resolution_test.go index 1803a3b..921e2d9 100644 --- a/pkg/billing/tier_resolution_test.go +++ b/pkg/billing/tier_resolution_test.go @@ -7,11 +7,11 @@ import ( "fmt" "net/http" "net/http/httptest" - "strings" "testing" "time" "atcr.io/pkg/atproto" + "atcr.io/pkg/testpds" "github.com/bluesky-social/indigo/atproto/atcrypto" "github.com/stripe/stripe-go/v84" ) @@ -100,6 +100,27 @@ func TestResolveTier_UnknownEverythingIsUnresolved(t *testing.T) { } } +// managedHoldServer starts an httptest server standing in for a managed hold +// and returns it with the did:web the tier fan-out resolves back to it. The +// server serves its own DID document at /.well-known/did.json; handler gets +// every other request. Resolving a loopback did:web needs a `-tags testmode` +// build, so the test fails fast with that message otherwise. +func managedHoldServer(t *testing.T, handler http.HandlerFunc) (*httptest.Server, string) { + t.Helper() + if !atproto.TestModeBuild { + t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") + } + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + testpds.HoldDIDDocumentHandler(testpds.DIDWebForURL(base), base)(w, r) + }) + mux.Handle("/", handler) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv, testpds.DIDWebForURL(srv.URL) +} + // TestHandleSubscriptionChange_GrandfatheredSubscriberKeepsTier runs the same // property through the real webhook path, so it covers the payload shape too: // Stripe sends price.product as a bare ID string, and this fails if that is @@ -126,15 +147,13 @@ func TestHandleSubscriptionChange_GrandfatheredSubscriberKeepsTier(t *testing.T) TierRank int `json:"tierRank"` } pushes := make(chan tierPush, 4) - hold := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, holdDID := managedHoldServer(t, func(w http.ResponseWriter, r *http.Request) { var got tierPush _ = json.NewDecoder(r.Body).Decode(&got) pushes <- got w.WriteHeader(http.StatusOK) - })) - defer hold.Close() - m.managedHolds = []string{"did:web:" + strings.ReplaceAll( - strings.TrimPrefix(hold.URL, "http://"), ":", "%3A")} + }) + m.managedHolds = []string{holdDID} priv, err := atcrypto.GeneratePrivateKeyP256() if err != nil { diff --git a/pkg/billing/webhook_retry_test.go b/pkg/billing/webhook_retry_test.go index 4169f6c..af8c83b 100644 --- a/pkg/billing/webhook_retry_test.go +++ b/pkg/billing/webhook_retry_test.go @@ -307,12 +307,10 @@ func TestHandleSubscriptionChange_HoldFanoutFailureIsRetryable(t *testing.T) { stripeAPIReturning(t, http.StatusOK, `{"id":"cus_fanout","object":"customer","metadata":{"user_did":"did:plc:fanoutuser"}}`) - hold := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, holdDID := managedHoldServer(t, func(w http.ResponseWriter, r *http.Request) { http.Error(w, "hold is down", http.StatusServiceUnavailable) - })) - defer hold.Close() - m.managedHolds = []string{"did:web:" + strings.ReplaceAll( - strings.TrimPrefix(hold.URL, "http://"), ":", "%3A")} + }) + m.managedHolds = []string{holdDID} priv, err := atcrypto.GeneratePrivateKeyP256() if err != nil { diff --git a/pkg/hold/admin/admin.go b/pkg/hold/admin/admin.go index e00f7c6..d302795 100644 --- a/pkg/hold/admin/admin.go +++ b/pkg/hold/admin/admin.go @@ -153,8 +153,7 @@ func NewAdminUI(ctx context.Context, holdPDS *pds.HoldPDS, quotaMgr *quota.Manag "redirect_uri", redirectURI) } - clientApp := indigooauth.NewClientApp(&oauthConfig, oauthStore) - clientApp.Dir = atproto.GetDirectory() + clientApp := atproto.NewOAuthClientApp(&oauthConfig, oauthStore) // Parse templates templates, err := parseTemplates() diff --git a/pkg/hold/pds/appview_token_test.go b/pkg/hold/pds/appview_token_test.go index 66f686e..8b8157b 100644 --- a/pkg/hold/pds/appview_token_test.go +++ b/pkg/hold/pds/appview_token_test.go @@ -4,12 +4,12 @@ import ( "encoding/json" "net/http" "net/http/httptest" - "net/url" "strings" "testing" "atcr.io/pkg/atproto" "atcr.io/pkg/auth" + "atcr.io/pkg/testpds" "github.com/bluesky-social/indigo/atproto/atcrypto" ) @@ -41,33 +41,33 @@ func newAppviewTestEnvWithKey(t *testing.T, priv atcrypto.PrivateKey) *appviewTe t.Fatalf("public key: %v", err) } - doc := map[string]any{ - "@context": []string{"https://www.w3.org/ns/did/v1"}, - "verificationMethod": []map[string]string{{ - "id": "#atproto", - "type": "Multikey", - "publicKeyMultibase": pub.Multibase(), - }}, + // fetchAppviewPublicKey resolves the appview DID through the identity + // directory; only a `-tags testmode` build resolves a loopback did:web. + if !atproto.TestModeBuild { + t.Fatal("this test resolves a did:web on 127.0.0.1 and needs a `-tags testmode` build") } + // The document mirrors what the real appview serves: the key is + // controlled by the document's own DID, so the directory keeps it. mux := http.NewServeMux() mux.HandleFunc("/.well-known/did.json", func(w http.ResponseWriter, r *http.Request) { + did := testpds.DIDWebForURL("http://" + r.Host) w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(doc) + _ = json.NewEncoder(w).Encode(map[string]any{ + "@context": []string{"https://www.w3.org/ns/did/v1"}, + "id": did, + "verificationMethod": []map[string]string{{ + "id": did + "#appview", + "type": "Multikey", + "controller": did, + "publicKeyMultibase": pub.Multibase(), + }}, + }) }) server := httptest.NewServer(mux) t.Cleanup(server.Close) - u, err := url.Parse(server.URL) - if err != nil { - t.Fatalf("parse server URL: %v", err) - } - // did:web requires percent-encoded ":" for ports - appviewDID := "did:web:" + strings.ReplaceAll(u.Host, ":", "%3A") - - // fetchAppviewPublicKey downgrades to http when test mode is on - atproto.SetTestMode(true) - t.Cleanup(func() { atproto.SetTestMode(false) }) + appviewDID := testpds.DIDWebForURL(server.URL) // Reset shared jti cache so other tests don't pollute this one and vice versa. *sharedAppviewJTICache = *newJTIReplayCache() diff --git a/pkg/hold/pds/auth.go b/pkg/hold/pds/auth.go index 0eca3dd..b92a1a3 100644 --- a/pkg/hold/pds/auth.go +++ b/pkg/hold/pds/auth.go @@ -695,7 +695,21 @@ func ValidateAppviewToken(r *http.Request, appviewDID, holdDID string) (string, } if err := pubKey.HashAndVerifyLenient(signedData, signature); err != nil { - return "", fmt.Errorf("signature verification failed: %w", err) + // The identity directory caches the appview's DID document for up to + // 24h. If the appview re-keyed since (a fresh database, a rotated + // OAuth key), the cached key is stale and every token fails until the + // entry expires. Purge and try once more against a fresh document + // before rejecting. + if perr := atproto.InvalidateIdentity(r.Context(), appviewDID); perr != nil { + return "", fmt.Errorf("signature verification failed: %w", err) + } + pubKey, ferr := fetchAppviewPublicKey(r.Context(), appviewDID) + if ferr != nil { + return "", fmt.Errorf("signature verification failed: %w", err) + } + if err := pubKey.HashAndVerifyLenient(signedData, signature); err != nil { + return "", fmt.Errorf("signature verification failed: %w", err) + } } // Replay check: when `jti` is present, refuse to honour the same one @@ -714,60 +728,46 @@ func ValidateAppviewToken(r *http.Request, appviewDID, holdDID string) (string, return subject, nil } -// fetchAppviewPublicKey fetches the appview's verification key from its -// did:web DID document. Accepts any Multikey atcrypto can parse (P-256 or -// K-256). Returns the first one found. +// appviewKeyFragments lists the verification-method fragments an appview may +// publish its signing key under, in preference order. The appview's own DID +// document uses #appview; #atproto is the conventional fragment and what the +// test fixtures serve. +var appviewKeyFragments = []string{"appview", "atproto"} + +// fetchAppviewPublicKey resolves the appview's DID document through the shared +// identity directory and returns its verification key. Accepts any key the +// directory can parse (P-256 or K-256): a known fragment first, then whatever +// else the document declares. +// +// The directory is what makes this work in every build: production resolves +// did:web over HTTPS with indigo's hardening, and a `-tags testmode` build +// resolves a port-qualified or loopback did:web over plain HTTP. func fetchAppviewPublicKey(ctx context.Context, did string) (atcrypto.PublicKey, error) { - if !strings.HasPrefix(did, "did:web:") { - return nil, fmt.Errorf("only did:web is supported for appview DID, got %s", did) - } - - host := strings.TrimPrefix(did, "did:web:") - host = strings.ReplaceAll(host, "%3A", ":") - scheme := "https" - if atproto.IsTestMode() { - scheme = "http" - } - didDocURL := fmt.Sprintf("%s://%s/.well-known/did.json", scheme, host) - - req, err := http.NewRequestWithContext(ctx, "GET", didDocURL, nil) + didParsed, err := syntax.ParseDID(did) if err != nil { - return nil, fmt.Errorf("failed to create request: %w", err) + return nil, fmt.Errorf("invalid appview DID %q: %w", did, err) } - resp, err := http.DefaultClient.Do(req) + ident, err := atproto.GetDirectory().LookupDID(ctx, didParsed) if err != nil { - return nil, fmt.Errorf("failed to fetch DID document: %w", err) - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("DID document fetch returned status %d", resp.StatusCode) + return nil, fmt.Errorf("failed to resolve appview DID %s: %w", did, err) } - var doc struct { - VerificationMethod []struct { - ID string `json:"id"` - Type string `json:"type"` - PublicKeyMultibase string `json:"publicKeyMultibase"` - } `json:"verificationMethod"` + for _, fragment := range appviewKeyFragments { + if pubKey, err := ident.GetPublicKey(fragment); err == nil { + return pubKey, nil + } } - if err := json.NewDecoder(resp.Body).Decode(&doc); err != nil { - return nil, fmt.Errorf("failed to decode DID document: %w", err) - } - - for _, vm := range doc.VerificationMethod { - if vm.Type != "Multikey" || vm.PublicKeyMultibase == "" { + for fragment := range ident.Keys { + if slices.Contains(appviewKeyFragments, fragment) { continue } - pubKey, err := atcrypto.ParsePublicMultibase(vm.PublicKeyMultibase) - if err != nil { - continue + if pubKey, err := ident.GetPublicKey(fragment); err == nil { + return pubKey, nil } - return pubKey, nil } - return nil, fmt.Errorf("no Multikey verification method found in DID document for %s", did) + return nil, fmt.Errorf("no usable verification key in DID document for %s", did) } // fetchPublicKeyFromDID fetches the public key from a DID document diff --git a/pkg/labeler/server.go b/pkg/labeler/server.go index 28a7e1b..82cb2dc 100644 --- a/pkg/labeler/server.go +++ b/pkg/labeler/server.go @@ -79,8 +79,7 @@ func NewServer(cfg *Config) (*Server, error) { oauthConfig = indigooauth.NewPublicConfig(clientID, redirectURI, scopes) } - clientApp := indigooauth.NewClientApp(&oauthConfig, oauthStore) - clientApp.Dir = atproto.GetDirectory() + clientApp := atproto.NewOAuthClientApp(&oauthConfig, oauthStore) auth := NewAuth(cfg.Labeler.OwnerDID) diff --git a/pkg/testpds/didweb.go b/pkg/testpds/didweb.go new file mode 100644 index 0000000..6a20b93 --- /dev/null +++ b/pkg/testpds/didweb.go @@ -0,0 +1,33 @@ +package testpds + +import ( + "encoding/json" + "net/http" +) + +// DIDWebForURL returns the did:web identifier for a local http://host:port +// URL, percent-encoding the port colon as did:web requires. It is the DID a +// `-tags testmode` build resolves back to that URL by fetching +// http://host:port/.well-known/did.json. +func DIDWebForURL(rawURL string) string { + return "did:web:" + didWebForHost(rawURL) +} + +// HoldDIDDocumentHandler serves a minimal did:web document for a test server +// standing in for a hold: both the #atproto_pds and #atcr_hold services point +// at baseURL, and no keys are declared. Mount it at /.well-known/did.json so +// the test-mode identity directory can resolve the server's DID. +func HoldDIDDocumentHandler(did, baseURL string) http.HandlerFunc { + doc := map[string]any{ + "@context": []string{"https://www.w3.org/ns/did/v1"}, + "id": did, + "service": []map[string]string{ + {"id": "#atproto_pds", "type": "AtprotoPersonalDataServer", "serviceEndpoint": baseURL}, + {"id": "#atcr_hold", "type": "AtcrHoldService", "serviceEndpoint": baseURL}, + }, + } + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/did+ld+json") + _ = json.NewEncoder(w).Encode(doc) + } +}