From 4ae468365b6d3a1eaba92505e5d4048eb7e9a822 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Mon, 5 Oct 2026 04:54:50 -0400 Subject: [PATCH 01/12] feat!: a solution claims no module's facade route, and holds no credential that could A facade route for a module is claimed only by the module that serves it, under a credential bound to that module; a solution holds no credential that decides where another module's traffic is routed. This runtime broke both: for every target in its api.consumes projection it claimed that module's prefix on the gateway, presenting a route-claim secret its composition had delivered to the solution's own environment. The capability is removed rather than narrowed, because there is no version of it a solution should have. Gone with it: the claim beat and its heartbeat, the credential type and its exchange, both endpoint URLs and their overrides, the two refusals validate() raised for them, and the reader of the carrier that delivered another module's secret. The reading side is untouched. Gateway.Transcoded and the passthrough still call a consumed module at /v1//*, as the viewer; neither decides that the prefix points there. A consumed module with no route now gets the gateway's 404, reported where the gap is, with no fallback that claims the prefix here. Removing the claim beat also removes this package's one serve-time configuration read: the api.consumes projection is resolved in loadConfig and nowhere else, so every environment read is now governed by validate(). environment_boundary_test.go and AGENTS.md drop the exception they documented. BREAKING CHANGE: ModuleRegistrationSecretsEnvironmentVariable is removed, as are the GATEWAY_MODULE_REGISTER_URL and GATEWAY_MODULE_REGISTRATION_TOKEN_URL overrides and the CODEFLY__MODULE_REGISTRATION_SECRETS carrier. A composition may still deliver that carrier; nothing here reads it. Refs #51 (SA-F-GWREGISTRY; SP-GW-04, SP-SOL-03). Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 22 +- README.md | 114 ++--- deployed_registration_test.go | 4 - environment_boundary_test.go | 55 +-- facade_claim_test.go | 183 ++++++++ solution.go | 391 ++-------------- solution_registration_test.go | 12 +- solution_test.go | 807 +--------------------------------- 8 files changed, 337 insertions(+), 1251 deletions(-) create mode 100644 facade_claim_test.go diff --git a/AGENTS.md b/AGENTS.md index 0af8a53..30bd434 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,14 +43,20 @@ own: it builds on `Server.PassthroughHandler`, the handler `Serve` mounts. `mcp/public-url`), and a refusal names that group/key, never a variable name. `MCP_PUBLIC_URL` and `HOST_ISSUER_URL` were bare variables for one release and the boot refused with the name of something nobody could set (#48). -- The exception is the consumed-API projection, read at serve time in - `registerConsumedAPIs` and never seen by `validate()`. A malformed one is not - refused: the runtime logs that the federation is disabled and serves on, so - every consumed facade 404s at the gateway while the solution looks healthy. - Anything you add on that path inherits that property — it degrades silently - unless you make it loud, and a log line is the only signal there is. Both - halves of this bullet are pinned by `environment_boundary_test.go`, so a - second exception fails the suite until this file describes it. + There is no exception: every environment read sits in `loadConfig`'s call + tree, and `environment_boundary_test.go` fails the suite if one appears + outside it. There used to be one — the consumed-API projection, read at serve + time and never seen by `validate()`, so a malformed one disabled the + federation with a log line while the solution served on — and it went with the + code that read it (#51). +- A solution registers nothing but itself. It claims no facade route for a + module it consumes and holds no credential that would let it: the route for a + module is claimed by the module that serves it, under a credential bound to + that module, so a solution holding one would hold the thing that decides where + another module's traffic goes. The passthrough and the gateway client *read* a + consumed module at `/v1//*`; nothing here decides that the prefix points + there. `facade_claim_test.go` pins both halves — the boot that claims nothing, + and the package carrying neither the claim surface nor the credential. - Validate at the boundary — boot, and the headers a request arrives with — not between internal callers. `validate()` is the model: each refusal names the variable or the provisioning path that fixes it. diff --git a/README.md b/README.md index 4ddbe73..5600b57 100644 --- a/README.md +++ b/README.md @@ -62,9 +62,7 @@ identifier and the issuer](#the-resource-identifier-and-the-issuer). | Host frontend URL | resolved by role — the single module owning the `frontend` `http`/`http` endpoint, discovered the same way | — (feeds the host register URL) | | Host register URL | `/api/solutions/register` | `HOST_REGISTER_URL` | | Gateway register URL | `/solutions/_register` | `GATEWAY_REGISTER_URL` (must end in `/solutions/_register`, see below) | -| Gateway module register URL | `/modules/_register` | `GATEWAY_MODULE_REGISTER_URL` | -| Gateway module token URL | the module register URL above with `/modules/_register` swapped for `/modules/_registration-token`, so it keeps that gateway's base path | `GATEWAY_MODULE_REGISTRATION_TOKEN_URL` | -| Gateway solution token URL | the gateway register URL above with `/solutions/_register` swapped for `/solutions/_registration-token`, same reasoning | `GATEWAY_SOLUTION_REGISTRATION_TOKEN_URL` | +| Gateway solution token URL | the gateway register URL above with `/solutions/_register` swapped for `/solutions/_registration-token`, so it keeps that gateway's base path | `GATEWAY_SOLUTION_REGISTRATION_TOKEN_URL` | | Internal-auth token | `codefly.For(ctx).WorkspaceSecret("internal-auth", "CODEFLY_INTERNAL_TOKEN")` — the namespaced secret Codefly injects | `CODEFLY_INTERNAL_TOKEN` | | Solution registration secret | `codefly.For(ctx).WorkspaceSecret("solution-registration", "SECRET")` — see [Self-registration](#self-registration) | `CODEFLY__SOLUTION_REGISTRATION_SECRET` | | Registration beat interval | `15s` | `CODEFLY__SOLUTION_REGISTRATION_INTERVAL` | @@ -149,16 +147,21 @@ obtains that token by presenting its **solution registration secret** (plus the internal token, the gateway's perimeter check) to `/solutions/_registration-token`, and presents it as `X-Codefly-Solution-Registration` on both registrations. Each surface burns a -token on use, so — unlike the module credential below — nothing is cached: a -fresh token is minted for every beat, on each surface. +token on use, so nothing is cached: a fresh token is minted for every beat, on +each surface. + +This is the only registration a solution makes. It registers **itself** — its +manifest with the host frontend, its own dialable upstream with the gateway — +and nothing else. It claims no facade route for a module it consumes, and holds +no credential that would let it: see [Consumed-module +federation](#consumed-module-federation). Provisioning both halves is the composition's job, and works the same for a local `codefly run solution` and a deployed cell: - **host side** — declare `:sha256hex` in the saas `federation` - group's `SOLUTION_REGISTRATION_SECRETS` (separately from - `MODULE_REGISTRATION_SECRETS`: a solution credential additionally publishes - host-origin code, so the two are never shared); + group's `SOLUTION_REGISTRATION_SECRETS`. It is a solution's own credential and + is never shared with, or derived from, any module's; - **solution side** — provision the plaintext as the `SECRET` key of the `solution-registration` workspace secret group (`codefly config generate solution-registration SECRET` locally; the cell's @@ -211,11 +214,17 @@ one: unreachable dependency produces none, so paying it there would only keep this solution out of a healthy host's nav for longer than necessary. -Every registration request refuses to follow redirects, for the same reason none -of them may be proxied: `net/http` strips only `Authorization`, `WWW-Authenticate` -and `Cookie` when a redirect crosses hosts, so the `X-Codefly-*` headers these -requests carry — the plaintext registration secret and the cluster-internal -token — would be handed to whatever a `Location` named. +Registration traffic — the exchange and both heartbeats — never goes through an +HTTP proxy: every target is composition-local, and these requests carry the +registration secret, the internal token and the signed token in headers, so +`HTTP(S)_PROXY` with a `NO_PROXY` that misses the host's in-cluster names would +dial them to an arbitrary egress host. + +Every registration request also refuses to follow redirects, for the same +reason: `net/http` strips only `Authorization`, `WWW-Authenticate` and `Cookie` +when a redirect crosses hosts, so the `X-Codefly-*` headers these requests carry +— the plaintext registration secret and the cluster-internal token — would be +handed to whatever a `Location` named. A beat is not free any more: each self-registration beat runs an exchange whose every success is an audited mint on the issuer, so at the 15s default across two @@ -232,50 +241,41 @@ to assume them; a host checks both before activating a remote. ### Consumed-module federation -A solution that declares `api.consumes` also registers each consumed module's -upstream with the gateway, so `/v1//*` proxies to it. Codefly projects the -targets into `CODEFLY__API_CONSUMES`; each gets its own 15s registration -heartbeat alongside the two above. - -The gateway does **not** accept the shared internal token on `/modules/_register` -— it admits a registration only against a short-lived token signed by accounts -and bound to a single prefix, so holding the credential for one module never lets -you claim another's route. The runtime obtains that token per consumed module by -presenting the module's own **registration secret** to -`/modules/_registration-token`, which the gateway brokers to accounts (a composed -module cannot reach accounts' internal listener itself). The token is reused -until shortly before it expires — or until half its life is gone, if the issuer -chose a lifetime shorter than that lead, since how long a credential lives is -the issuer's call and not this runtime's to veto. - -Obtaining one is an audited security event on accounts, so the runtime does not -answer every failure by obtaining another. A refusal buys exactly one fresh -token: the gateway refusing a token minted moments earlier is not refusing it for -being stale, and re-minting cannot fix whatever it is refusing it for. Beats that -fail also back off, doubling up to two minutes, so a broken gateway costs a -bounded number of exchanges however long it stays broken — and the runtime keeps -retrying, at a rate that will not flood the issuer's audit log, until it -recovers. A response carrying no usable expiry is refused rather than cached, and -its two causes — an issuer that sent no `expiresAt` at all, and a clock skewed -past the credential's lifetime — are reported separately, because they have -different fixes. - -No registration request is unbounded: a gateway that accepts one and never -answers surfaces as a failed beat rather than silently parking that heartbeat -for the life of the process. - -Registration traffic — the exchange and all three heartbeats — never goes -through an HTTP proxy: every target is composition-local, and these requests -carry the registration secret, the internal token, and the signed token in -headers. - -Those secrets arrive in `CODEFLY__MODULE_REGISTRATION_SECRETS` as -comma-separated `prefix:secret` entries — the plaintext twin of the -`prefix:sha256hex` digests the same composition declares to accounts -(`MODULE_REGISTRATION_SECRETS`). A consumed module with no secret cannot be -registered, so the runtime skips it with a log naming this variable rather than -beating against a guaranteed 401. Provisioning both halves is the composition's -job (`codefly run solution`). +A solution that declares `api.consumes` **reads** each consumed module through +the gateway, at the REST prefix `/v1//*` that module is federated under. It +does not create that route, and this runtime holds no credential that could. + +A facade route for a module is claimed only by the module that serves it, under +a credential bound to that module. Two things follow, and both are invariants +rather than preferences: + +- a solution never registers or claims a prefix on the gateway's module + route-claim surface, for a module it consumes or any other; +- a solution is delivered no module's route-claim secret. If a composition + delivers one anyway, this runtime reads nothing from it — the carrier has no + reader here. + +The reason is what such a credential decides. A route claim says where every +authenticated request on a prefix is proxied; a solution holding one holds the +routing of traffic that is not its own, and a claim surface that admits a +caller attesting to something other than the module itself admits whoever +claims the prefix first. Neither is a solution's to hold, so the capability is +not on this runtime's surface at all. See [codefly-dev/module-saas-starter#1018](https://github.com/codefly-dev/module-saas-starter/issues/1018) +for where a module's own claim is being built, and +[#953](https://github.com/codefly-dev/module-saas-starter/issues/953) for the +lifecycle rework that replaces the heartbeat writer. + +What stays here is the reading side, unchanged: `Gateway.Transcoded` and the +[passthrough](#letting-the-page-call-a-consumed-module-the-passthrough) call a +consumed module at `/v1//*` as the viewer. Both depend on that route +existing; neither decides where it points. A solution whose consumed module has +no route gets the gateway's `404`, which is a composition or module-side gap, +reported where it happens rather than papered over by claiming the prefix here. + +Codefly still projects the consumed targets into `CODEFLY__API_CONSUMES`, and +the runtime still reads it — at boot, through `loadConfig`, to check the +`Consumes` declaration against it before the listener exists. It is used for +nothing else. ### Reading a Work-Context-authenticated module diff --git a/deployed_registration_test.go b/deployed_registration_test.go index 8dcf95c..a07b23c 100644 --- a/deployed_registration_test.go +++ b/deployed_registration_test.go @@ -88,8 +88,6 @@ func TestGatewayRegistrationCarriesTheReachableUpstream(t *testing.T) { port: strconv.Itoa(ln.Addr().(*net.TCPAddr).Port), hostRegisterURL: srv.URL + "/api/solutions/register", gatewayRegisterURL: srv.URL + solutionRegisterPath, - moduleRegisterURL: srv.URL + moduleRegisterPath, - moduleTokenURL: srv.URL + moduleRegistrationTokenPath, solutionTokenURL: srv.URL + solutionRegistrationTokenPath, internalToken: internalTokenTest, solutionSecret: "s3cret", @@ -119,8 +117,6 @@ func TestValidateRefusesLoopbackInADeployedRuntime(t *testing.T) { gatewayURL: "http://gateway:42152", hostRegisterURL: "http://frontend:21931/api/solutions/register", gatewayRegisterURL: "http://gateway:42152/solutions/_register", - moduleRegisterURL: "http://gateway:42152/modules/_register", - moduleTokenURL: "http://gateway:42152/modules/_registration-token", solutionTokenURL: "http://gateway:42152/solutions/_registration-token", solutionSecret: "s3cret", selfUpstream: "http://backend.solutions-lastlogin.svc.cluster.local:8080", diff --git a/environment_boundary_test.go b/environment_boundary_test.go index 1d56323..3e2ddf9 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -11,63 +11,54 @@ import ( "testing" ) -// serveTimeEnvironmentRead is the one function that reads the environment -// outside boot resolution: the consumed-API projection is parsed when the -// server starts, never reaches validate(), and a malformed value disables the -// federation with a log line instead of refusing the boot. -const serveTimeEnvironmentRead = "registerConsumedAPIs" +// retiredServeTimeRead named the one function that used to read the +// environment outside boot resolution: it parsed the consumed-API projection +// when the server started, so a malformed value disabled the consumed-module +// federation with a log line while the solution served on. It registered a +// facade route for each consumed module, which is not a solution's to claim, +// and went with that capability — so the exception it was went with it too. +const retiredServeTimeRead = "registerConsumedAPIs" // AGENTS.md tells an agent that configuration is resolved in one place and -// refused at boot, naming the single serve-time exception and the silent -// failure it produces. Prose cannot notice a second exception appearing -// underneath it, and an agent-context file is followed literally, so the claim -// is pinned here: every environment read must sit in loadConfig's call tree, -// where validate() governs the result, or be the documented exception. +// refused at boot, with no exception. Prose cannot notice an exception +// appearing underneath it, and an agent-context file is followed literally, so +// the claim is pinned here: every environment read sits in loadConfig's call +// tree, where validate() governs the result. func TestEnvironmentIsReadOnlyWhereAgentsFileSaysItIs(t *testing.T) { pkg := parsePackage(t) boot := pkg.reachableFrom("loadConfig") var undocumented []string for _, fn := range pkg.environmentReaders() { - if !boot[fn] && fn != serveTimeEnvironmentRead { + if !boot[fn] { undocumented = append(undocumented, fn) } } if len(undocumented) > 0 { t.Errorf("these functions read the environment outside loadConfig's call tree: %s\n"+ - "AGENTS.md states that boot configuration is resolved in loadConfig and refused by validate(), with %q as the only exception. "+ - "Either resolve the value in loadConfig, or document the new exception in AGENTS.md — a value read after boot is never refused, it degrades while the solution serves.", - strings.Join(undocumented, ", "), serveTimeEnvironmentRead) + "AGENTS.md states that boot configuration is resolved in loadConfig and refused by validate(), with no exception. "+ + "Either resolve the value in loadConfig, or document the new exception in AGENTS.md and here — a value read after boot is never refused, it degrades while the solution serves.", + strings.Join(undocumented, ", ")) } } -// The documented exception has to keep being one. If it stops reading the -// environment, or starts being called during boot resolution, AGENTS.md -// describes a hazard that no longer exists where it says it does. -func TestServeTimeEnvironmentReadIsStillTheException(t *testing.T) { +// And the exception stays retired on both sides. A package that reintroduces +// the serve-time read, or an AGENTS.md that still describes one, puts the +// agent-context file back out of step with the code it governs. +func TestTheServeTimeEnvironmentExceptionStaysRetired(t *testing.T) { pkg := parsePackage(t) - - reads := false for _, fn := range pkg.environmentReaders() { - if fn == serveTimeEnvironmentRead { - reads = true + if fn == retiredServeTimeRead { + t.Errorf("%s reads the environment again: its value would be resolved at serve time and never refused by validate()", retiredServeTimeRead) } } - if !reads { - t.Errorf("%s no longer reads the environment, so AGENTS.md documents an exception that does not exist: remove it there and here", - serveTimeEnvironmentRead) - } - if pkg.reachableFrom("loadConfig")[serveTimeEnvironmentRead] { - t.Errorf("%s is now reachable from loadConfig, so its value is resolved at boot: AGENTS.md describes it as read at serve time and unseen by validate()", - serveTimeEnvironmentRead) - } agents, err := os.ReadFile("AGENTS.md") if err != nil { t.Fatalf("read AGENTS.md: %v", err) } - if !strings.Contains(string(agents), serveTimeEnvironmentRead) { - t.Errorf("AGENTS.md does not name %q, the one configuration read that is not refused at boot", serveTimeEnvironmentRead) + if strings.Contains(string(agents), retiredServeTimeRead) { + t.Errorf("AGENTS.md still names %q, an exception this package no longer has: describe the single boot-resolution rule instead", retiredServeTimeRead) } } diff --git a/facade_claim_test.go b/facade_claim_test.go new file mode 100644 index 0000000..274296f --- /dev/null +++ b/facade_claim_test.go @@ -0,0 +1,183 @@ +package solution + +import ( + "context" + "net" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "sync" + "testing" +) + +// --- A solution claims no module's facade route --- +// +// SP-GW-04 and SP-SOL-03, the two invariants SA-F-GWREGISTRY broke: a facade +// route for a module is claimed only by the module that serves it, under a +// credential bound to that module, and a solution holds no credential that +// decides where another module's traffic is routed. +// +// This runtime used to claim one for every module its composition let it +// consume. The paths and headers of that surface are written out here, as +// literals this test owns, for two reasons: nothing in the package names them +// any more, and these are the wire shapes a solution must never present. The +// static guard below keeps them out of the package proper. + +const ( + // claimedFacadePath and facadeCredentialPath are the gateway's module + // route-claim surface and the exchange that mints the credential for it. + claimedFacadePath = "/modules/_register" + facadeCredentialPath = "/modules/_registration-token" + // facadeSecretHeader carries a module's own route-claim secret, and + // facadeCredentialHeader the credential minted from it. + facadeSecretHeader = "X-Codefly-Module-Secret" + facadeCredentialHeader = "X-Codefly-Module-Registration" + // facadeSecretsVariable is the carrier that delivered another module's + // route-claim secret into a solution's own environment. + facadeSecretsVariable = "CODEFLY__MODULE_REGISTRATION_SECRETS" +) + +// facadeWatcher is a host gateway that answers this runtime's own registration +// exchange and records every request it receives, so a test can assert on the +// ones that never arrive. +type facadeWatcher struct { + *httptest.Server + + mu sync.Mutex + paths []string + // secrets is every value seen in a header that carries another module's + // route-claim credential, on any path. + secrets []string + // ownRegistrations counts this solution's own upstream registration, which + // proves the boot reached the point where it would have claimed a facade. + ownRegistrations int +} + +func newFacadeWatcher(t *testing.T) *facadeWatcher { + t.Helper() + w := &facadeWatcher{} + w.Server = httptest.NewServer(http.HandlerFunc(w.serve)) + t.Cleanup(w.Close) + return w +} + +func (g *facadeWatcher) serve(w http.ResponseWriter, r *http.Request) { + g.mu.Lock() + g.paths = append(g.paths, r.URL.Path) + for _, header := range []string{facadeSecretHeader, facadeCredentialHeader} { + if value := r.Header.Get(header); value != "" { + g.secrets = append(g.secrets, header+": "+value) + } + } + if r.URL.Path == solutionRegisterPath { + g.ownRegistrations++ + } + g.mu.Unlock() + answeringTheExchange(w, r) +} + +func (g *facadeWatcher) seen() ([]string, []string, int) { + g.mu.Lock() + defer g.mu.Unlock() + return append([]string(nil), g.paths...), append([]string(nil), g.secrets...), g.ownRegistrations +} + +// TestSolutionClaimsNoModuleFacadeRoute boots a real solution in the shape the +// finding describes: a composition that projects a consumed module and +// provisions that module's route-claim secret into this backend's environment. +// Nothing the runtime does may turn either into a claim on that module's +// facade route — not the projection, which says only what this solution reads, +// and not the secret, which is another module's credential however it got here. +// +// The solution's own upstream registration is asserted too, so the test cannot +// pass by failing to boot. +func TestSolutionClaimsNoModuleFacadeRoute(t *testing.T) { + gw := newFacadeWatcher(t) + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + + setEndpoint(t, "CODEFLY__ENDPOINT__DOCSTORE__DOCUMENTS__REST__REST", "http://docstore-upstream:9100") + t.Setenv("CODEFLY__API_CONSUMES", consumesDocuments) + // Provisioned, and still never used: the fix is not that the composition + // stops delivering this (that is the composition's half of the finding), + // it is that a runtime holding it claims nothing with it. + t.Setenv(facadeSecretsVariable, "documents:another-modules-secret") + t.Setenv("GATEWAY_URL", gw.URL) + t.Setenv("HOST_REGISTER_URL", gw.URL+"/api/solutions/register") + t.Setenv("PORT", strconv.Itoa(ln.Addr().(*net.TCPAddr).Port)) + t.Setenv("SELF_UPSTREAM", "http://backend.svc.cluster.local:8080") + t.Setenv("CODEFLY__SOLUTION_REGISTRATION_SECRET", "s3cret") + t.Setenv("CODEFLY_INTERNAL_TOKEN", internalTokenTest) + + s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) + ctx, cancel := context.WithCancel(context.Background()) + s.cfg = loadConfig(ctx, s.manifest.ID) + done := make(chan struct{}) + go func() { _ = s.serve(ctx, ln); close(done) }() + defer func() { cancel(); <-done }() + + waitFor(t, "this solution's own upstream registration", func() bool { + _, _, own := gw.seen() + return own > 0 + }) + + paths, secrets, _ := gw.seen() + for _, path := range paths { + if path == claimedFacadePath || path == facadeCredentialPath { + t.Errorf("the solution called %q: a facade route is claimed only by the module that serves it, never by a solution that consumes it", path) + } + } + if len(secrets) > 0 { + t.Errorf("the solution presented another module's route-claim credential: %s", strings.Join(secrets, ", ")) + } +} + +// TestNoModuleFacadeClaimOnTheSDKSurface is the static half: the behaviour test +// above can only prove that one boot claimed nothing, and the capability is +// what the invariant is about. A solution built on this SDK must not be able to +// claim a module's facade route at all, so the package carries neither the +// surface nor the credential — no claim path, no exchange, no header, and no +// reader of the carrier that delivered another module's secret. +func TestNoModuleFacadeClaimOnTheSDKSurface(t *testing.T) { + forbidden := map[string]string{ + claimedFacadePath: "the gateway's module route-claim path", + facadeCredentialPath: "the exchange that mints a module route-claim credential", + facadeSecretHeader: "the header carrying a module's route-claim secret", + facadeCredentialHeader: "the header carrying a module route-claim credential", + facadeSecretsVariable: "the carrier delivering another module's route-claim secret", + } + + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("read package directory: %v", err) + } + scanned := 0 + for _, entry := range entries { + name := entry.Name() + // Test sources are exempt: this file names every one of these on + // purpose, and a test that could not write them down could not assert + // their absence. + if filepath.Ext(name) != ".go" || strings.HasSuffix(name, "_test.go") { + continue + } + source, err := os.ReadFile(name) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + scanned++ + for literal, what := range forbidden { + if strings.Contains(string(source), literal) { + t.Errorf("%s names %q (%s): a solution holds no credential deciding where another module's traffic is routed, and claims no module's facade route", + name, literal, what) + } + } + } + if scanned == 0 { + t.Fatal("no package sources scanned: this guard is inert") + } +} diff --git a/solution.go b/solution.go index 7d8c3d6..e4dbb68 100644 --- a/solution.go +++ b/solution.go @@ -243,8 +243,6 @@ type config struct { port, publicURL, gatewayURL string hostRegisterURL string gatewayRegisterURL string - moduleRegisterURL string - moduleTokenURL string // solutionTokenURL is the gateway exchange that mints this solution's own // registration credential (see solutionCredential). solutionTokenURL string @@ -254,9 +252,6 @@ type config struct { // this solution — the plaintext twin of the digest the host declares for its // id. Empty when the composition provisioned none. solutionSecret string - // moduleSecrets maps a consumed module's facade prefix to the registration - // secret the composition provisioned for it. - moduleSecrets map[string]string // registrationInterval is the beat period an operator chose, or zero for // defaultRegistrationInterval; registrationIntervalErr is why an explicit // one was not usable. @@ -467,12 +462,6 @@ func loadConfig(ctx context.Context, id string) config { gatewayURL := strings.TrimRight(env("GATEWAY_URL", resolveGateway(ctx, hostModule, hostGateway)), "/") frontendURL := strings.TrimRight(resolveFrontend(ctx, hostModule, hostFrontend), "/") - // Both module-federation endpoints live on one gateway: the exchange mints the - // credential the registration presents, so pointing registration at another - // gateway while the exchange stayed on this one would mint against one host - // and register with a second that never trusts the result. - moduleRegisterURL := env("GATEWAY_MODULE_REGISTER_URL", gatewayURL+moduleRegisterPath) - // Internal token: the namespaced workspace secret Codefly injects, resolved // by name through the SDK rather than a bare os.Getenv the runtime never sees. token := env("CODEFLY_INTERNAL_TOKEN", "") @@ -480,9 +469,9 @@ func loadConfig(ctx context.Context, id string) config { token, _ = codefly.For(ctx).WorkspaceSecret("internal-auth", "CODEFLY_INTERNAL_TOKEN") } - // The solution's own registration exchange lives on the same gateway as its - // registration, for the reason the module pair does: the token minted by one - // gateway is trusted by that gateway's host, not by another's. + // The registration exchange lives on the same gateway as the registration + // it credentials: the token minted by one gateway is trusted by that + // gateway's host, not by another's. gatewayRegisterURL := env("GATEWAY_REGISTER_URL", gatewayURL+solutionRegisterPath) cfg := config{ @@ -491,14 +480,11 @@ func loadConfig(ctx context.Context, id string) config { gatewayURL: gatewayURL, hostRegisterURL: env("HOST_REGISTER_URL", frontendURL+"/api/solutions/register"), gatewayRegisterURL: gatewayRegisterURL, - moduleRegisterURL: moduleRegisterURL, - moduleTokenURL: env("GATEWAY_MODULE_REGISTRATION_TOKEN_URL", siblingURL(moduleRegisterURL, moduleRegisterPath, moduleRegistrationTokenPath)), solutionTokenURL: env("GATEWAY_SOLUTION_REGISTRATION_TOKEN_URL", siblingURL(gatewayRegisterURL, solutionRegisterPath, solutionRegistrationTokenPath)), selfUpstream: strings.TrimRight(env("SELF_UPSTREAM", selfUpstream(port)), "/"), assetsDir: env("ASSETS_DIR", "../fe-remote/dist"), internalToken: token, solutionSecret: solutionRegistrationSecret(ctx), - moduleSecrets: parseModuleRegistrationSecrets(env(ModuleRegistrationSecretsEnvironmentVariable, "")), runtimeContext: strings.TrimSpace(env(resources.RuntimeContextPrefix, "")), apiConsumes: env(manifest.APIConsumesEnvironmentVariable, ""), } @@ -685,8 +671,6 @@ func (c config) validate() error { "gateway URL": c.gatewayURL, "host register URL": c.hostRegisterURL, "gateway register URL": c.gatewayRegisterURL, - "module register URL": c.moduleRegisterURL, - "module token URL": c.moduleTokenURL, "solution token URL": c.solutionTokenURL, "self upstream": c.selfUpstream, } @@ -698,7 +682,6 @@ func (c config) validate() error { // exchange began deriving from it, so a deployment that booted yesterday // can stop booting today with no mention of the variable that changed. unpairable := map[string]string{ - "module token URL": "GATEWAY_MODULE_REGISTER_URL must end in " + moduleRegisterPath + " for its exchange to be derived from it; set GATEWAY_MODULE_REGISTRATION_TOKEN_URL explicitly otherwise", "solution token URL": "GATEWAY_REGISTER_URL must end in " + solutionRegisterPath + " for its exchange to be derived from it; set GATEWAY_SOLUTION_REGISTRATION_TOKEN_URL explicitly otherwise", } for name, raw := range required { @@ -848,7 +831,6 @@ func (s *Server) serve(ctx context.Context, ln net.Listener) error { upstreamBody, _ := json.Marshal(map[string]string{"id": s.manifest.ID, "upstream": s.cfg.selfUpstream}) go s.heartbeat(ctx, s.cfg.hostRegisterURL, manifestBody, "host", s.newSolutionCredential()) go s.heartbeat(ctx, s.cfg.gatewayRegisterURL, upstreamBody, "gateway", s.newSolutionCredential()) - s.registerConsumedAPIs(ctx) srv := &http.Server{Handler: mux} go func() { @@ -863,74 +845,6 @@ func (s *Server) serve(ctx context.Context, ln net.Listener) error { return nil } -// registerConsumedAPIs registers each of the solution's api.consumes targets -// with the gateway so it can proxy /v1//* to the consumed module. The -// targets are projected by core into CODEFLY__API_CONSUMES; the address of each -// is already injected (the backend depends on the consumed service), so it is -// resolved through the SDK exactly as the gateway and frontend are. Each target -// gets its own heartbeat, mirroring the host and gateway registrations, so a -// registration that is briefly unavailable at boot is retried. -// -// A solution that declares no api.consumes has an empty CODEFLY__API_CONSUMES -// and registers nothing — no behavior changes unless api.consumes is declared. -func (s *Server) registerConsumedAPIs(ctx context.Context) { - consumed, err := manifest.ParseConsumedAPIs(os.Getenv(manifest.APIConsumesEnvironmentVariable)) - if err != nil { - // A malformed projection is a build-time/CLI defect the runtime cannot - // repair. ParseConsumedAPIs returns no entries on a decode error, so this - // registers nothing at all: every consumed facade silently 404s at the - // gateway. Crashing would also take down this solution's own frontend, so - // serve on — but log loudly, because this line is the only signal that the - // entire api.consumes federation was dropped. - log.Printf("solution %q: api.consumes federation disabled: %v", s.manifest.ID, err) - } - for _, c := range consumed { - // The gateway proxies /v1//*, where As is the facade entry-point core - // derives from the producing endpoint's proto package (the last non-version - // segment) — not the module name. The runtime cannot reconstruct that - // default from the projected identity, so an entry that reached us without - // an explicit As is one the CLI should have resolved: registering a guessed - // prefix (e.g. the module name) would proxy a route the generated client - // never calls (a silent 404) and could even claim a prefix meant for a - // different facade. Skip it loudly instead of guessing. - if c.As == "" { - log.Printf("solution %q: consumed api %s/%s/%s has no facade entry-point (as); skipping gateway module registration", - s.manifest.ID, c.Module, c.Service, c.Endpoint) - continue - } - prefix := c.As - upstream := address(ctx, c.Module, c.Service, c.Endpoint, c.Protocol) - if upstream == "" { - // The address is injected because the backend depends on the consumed - // service; if it did not resolve, registering an empty upstream would - // only be rejected, so skip loudly instead. - log.Printf("solution %q: no upstream resolved for consumed api %q (%s/%s/%s); skipping gateway module registration", - s.manifest.ID, prefix, c.Module, c.Service, c.Endpoint) - continue - } - secret := s.cfg.moduleSecrets[prefix] - if secret == "" { - // The gateway admits a registration only against a token signed by - // accounts, and accounts issues one only to a caller holding the - // secret whose digest the composition declared for this prefix. - // Without it every beat would be a guaranteed 401, so skip loudly: - // this line is the signal that provisioning, not the runtime, is the - // missing half. - log.Printf("solution %q: no registration secret provisioned for consumed api %q (%s); skipping gateway module registration", - s.manifest.ID, prefix, ModuleRegistrationSecretsEnvironmentVariable) - continue - } - body, _ := json.Marshal(map[string]string{"prefix": prefix, "upstream": upstream}) - credential := &moduleCredential{ - tokenURL: s.cfg.moduleTokenURL, - internalToken: s.cfg.internalToken, - prefix: prefix, - secret: secret, - } - go s.heartbeat(ctx, s.cfg.moduleRegisterURL, body, "gateway module "+prefix, credential) - } -} - // newSolutionCredential builds one self-registration credential. Each // heartbeat gets its own, because a solutionCredential is not safe to share: // its refusal and mint bookkeeping is written on every beat with no locking, @@ -1158,7 +1072,7 @@ func (s *Server) heartbeat(ctx context.Context, url string, body []byte, label s // a token the issuer has stopped honouring for the rest of its // lifetime. Whether that actually helps is the credential's call: // re-obtaining one it has just obtained cannot fix a refusal that - // was never about staleness (see moduleCredential.invalidate). + // was never about staleness (see solutionCredential.invalidate). // 403 counts too — a gateway that answers "forbidden" to a lapsed // token would otherwise have it replayed for the rest of its life. auth.invalidate() @@ -1222,7 +1136,7 @@ func (s *Server) heartbeat(ctx context.Context, url string, body []byte, label s // a response this process cannot use — otherwise resolves to "run the // whole thing again in `interval`", which turns one broken deployment // into a mint (and an audited security event on the issuer) four times - // a minute per module, for as long as the solution runs. Backing off + // a minute per surface, for as long as the solution runs. Backing off // bounds that without ever giving up: the first success resets it. if err != nil || status >= 300 { failures++ @@ -1429,67 +1343,24 @@ func drainAndClose(resp *http.Response) { resp.Body.Close() } -// --- Module registration credentials --- -// -// The gateway federates a consumed module's /v1//* only against a -// signed, prefix-bound token issued by accounts — the shared cluster-internal -// token buys no per-caller binding and is refused. A backend obtains one by -// presenting the registration secret its composition provisioned for that -// prefix; the gateway brokers the exchange, because a composed module cannot -// reach accounts' internal listener itself. - -const ( - moduleRegisterPath = "/modules/_register" - moduleRegistrationTokenPath = "/modules/_registration-token" -) - const ( internalTokenHeader = "X-Codefly-Internal-Token" - // moduleRegistrationHeader carries the signed, prefix-bound token - // /modules/_register requires. - moduleRegistrationHeader = "X-Codefly-Module-Registration" - // moduleSecretHeader carries this backend's own registration secret on the - // exchange that mints that token. - moduleSecretHeader = "X-Codefly-Module-Secret" ) -// ModuleRegistrationSecretsEnvironmentVariable carries the registration secrets -// a composition provisioned into this backend, as comma-separated -// `prefix:secret` entries — the plaintext twin of the `prefix:sha256hex` -// digests the same composition declared to accounts. It is absent for a -// composition that federates nothing. -const ModuleRegistrationSecretsEnvironmentVariable = "CODEFLY__MODULE_REGISTRATION_SECRETS" - -// moduleTokenRenewal is the most lead time the credential takes before an -// expiry, so a beat never presents a token that lapses between here and the -// gateway's check. It is a ceiling, not a requirement on the issuer: a token -// whose whole life is shorter is renewed at half its lifetime instead (see -// exchange). Conflating the two rejected every token the issuer chose to make -// short-lived — a 30s credential was refused outright, with a message blaming -// this host's clock. -const moduleTokenRenewal = 30 * time.Second - -// moduleTokenMinimumLifetime is the least remaining validity an issued token -// must carry to be worth presenting at all. Below this the token would lapse -// mid-flight, so it is a boundary error rather than a credential. -const moduleTokenMinimumLifetime = 5 * time.Second - -// registrationExchangeTimeout bounds one credential exchange — the module's and -// the solution's alike, which need the identical bound rather than one of them -// reaching into the other's constant. The registration beat is on a 15s cycle, -// so a stalled gateway must surface as a failed beat rather than wedge the beat -// loop for that module. +// registrationExchangeTimeout bounds one credential exchange. The registration +// beat is on a 15s cycle, so a stalled gateway must surface as a failed beat +// rather than wedge the beat loop. const registrationExchangeTimeout = 10 * time.Second // registrationClient carries every registration request: the credential -// exchange and the three registration heartbeats. It is NOT http.DefaultClient. +// exchange and both self-registration heartbeats. It is NOT http.DefaultClient. // // That transport carries Proxy: ProxyFromEnvironment, so with HTTP(S)_PROXY set // and a NO_PROXY that does not cover the host's in-cluster names, these // requests would be dialled to an arbitrary egress host — carrying, in headers, -// the module's plaintext registration secret, the cluster-internal token, and -// the signed token that decides where authenticated traffic for a prefix is -// forwarded. Every registration target is composition-local (each is resolved +// this solution's plaintext registration secret, the cluster-internal token, +// and the signed token that lets it register as this publisher. Every +// registration target is composition-local (each is resolved // from the SDK's endpoint map), so none of them may be proxied. The issuing // side of this same exchange refuses proxying for exactly this reason // (module-saas-starter#527). @@ -1508,11 +1379,11 @@ const registrationExchangeTimeout = 10 * time.Second // // It also carries a Timeout. Without one, neither the heartbeat's context (which // has no deadline) nor the transport bounds a gateway that accepts a -// registration and never answers: the beat blocks in Do forever, so that module -// — or, for the self-registrations, this whole solution — silently stops -// registering for the lifetime of the process, with no log and no recovery. The -// exchange bounds itself with registrationExchangeTimeout; this bounds the other -// three call sites the same way. +// registration and never answers: the beat blocks in Do forever, so this whole +// solution silently stops registering for the lifetime of the process, with no +// log and no recovery. The exchange bounds itself with +// registrationExchangeTimeout; this bounds the other two call sites the same +// way. var registrationClient = &http.Client{ Timeout: registrationTimeout, Transport: newRegistrationTransport(), @@ -1537,37 +1408,17 @@ func newRegistrationTransport() *http.Transport { return transport } -// parseModuleRegistrationSecrets decodes the comma-separated `prefix:secret` -// projection. An entry that is not a `prefix:secret` pair is dropped: the -// consequence — that module cannot register — is reported where it is -// actionable, by the missing-secret log in registerConsumedAPIs. -// -// Both halves are trimmed, matching how the registrar parses the digest twin of -// this same projection. Trimming only the entry left "documents : s3cret" -// keyed under "documents " with a leading space in the secret — a pair the -// registrar accepts and this side silently mangles into a lookup miss. -func parseModuleRegistrationSecrets(raw string) map[string]string { - secrets := map[string]string{} - for _, entry := range strings.Split(raw, ",") { - prefix, secret, ok := strings.Cut(entry, ":") - if !ok { - continue - } - prefix, secret = strings.TrimSpace(prefix), strings.TrimSpace(secret) - if prefix == "" || secret == "" { - continue - } - secrets[prefix] = secret - } - return secrets -} - // registrationAuth stamps the credential one registration POST presents, and -// drops it when the server refuses it. The host and gateway self-registrations -// present a short-lived token bound to this solution's publisher; a module -// registration presents one bound to the single prefix it may claim. Neither -// presents the shared cluster-internal token: it attests to no publisher and -// both surfaces refuse it (module-saas-starter#540). +// drops it when the server refuses it. Both self-registrations present a +// short-lived token bound to this solution's publisher, and neither presents +// the shared cluster-internal token: it attests to no publisher and both +// surfaces refuse it (module-saas-starter#540). +// +// A solution registers nothing but itself. It claims no facade route for a +// module it consumes, and holds no credential that would let it: that route is +// claimed by the module that serves it, under a credential bound to that +// module (SP-GW-04), and a solution holding one would be holding the thing +// that decides where another module's traffic goes (SP-SOL-03). type registrationAuth interface { // authorize stamps the credential on req and reports when that credential // stops being valid, or the zero time when it cannot expire mid-request. @@ -1583,175 +1434,6 @@ type registrationAuth interface { succeeded() } -// moduleCredential exchanges one consumed module's registration secret for the -// short-lived token /modules/_register requires, and holds it until renewal. -// Each instance is owned by exactly one registration beat, so its cached token -// needs no locking. -type moduleCredential struct { - tokenURL string - internalToken string - prefix string - secret string - - token string - // expiresAt is when token stops being honoured — not when it is renewed. - // The registration POST is bounded by it, because a token that lapses - // in flight is refused on arrival for a reason no refusal can express. - expiresAt time.Time - // renewAt is when the exchange must run again. Zero means "no usable - // credential": either none has been obtained yet, or one was dropped. - renewAt time.Time - // mintedThisBeat records that token was obtained during the beat now in - // flight, so a refusal of it cannot be blamed on staleness. - mintedThisBeat bool - // retriedAfterRefusal records that a refusal has already been answered with - // a fresh mint, and that mint has not yet been superseded by a natural - // renewal. It caps a refusal at one re-mint. - retriedAfterRefusal bool - warnedFreshRefusal bool - warnedShortLifetime bool -} - -func (c *moduleCredential) authorize(ctx context.Context, req *http.Request) (time.Time, error) { - c.mintedThisBeat = false - // A zero renewAt means no credential yet, or one just dropped, so the - // exchange runs. It cannot mean a cached-but-unusable token: exchange - // refuses any expiry this process could not act on. - if !time.Now().Before(c.renewAt) { - // Renewing a token still held is a fresh episode: whatever refusal the - // last re-mint was answering is over, so the next one earns its own retry. - natural := c.token != "" - token, expiresAt, renewAt, err := c.exchange(ctx) - if err != nil { - return time.Time{}, err - } - if natural { - c.retriedAfterRefusal = false - } - c.token, c.expiresAt, c.renewAt, c.mintedThisBeat = token, expiresAt, renewAt, true - } - req.Header.Set(moduleRegistrationHeader, c.token) - return c.expiresAt, nil -} - -// invalidate drops the cached token so the next beat obtains a fresh one — but -// only when a fresh one could plausibly help. -// -// A registration can be refused for reasons a new token cannot repair: a gateway -// that does not trust the issuer, a prefix this module may not claim, skew on the -// gateway's clock, a perimeter check rejecting the request before the token is -// ever examined. Dropping unconditionally turned every one of those into a mint -// on every beat — an audited security event on the issuer, four times a minute -// per module, indefinitely, and silent after the first log line because the -// status never changes. Staleness is the one cause re-minting fixes, so a -// refusal buys exactly one re-mint: a token minted for this very beat is not -// stale, and neither is the replacement a previous refusal already bought. -func (c *moduleCredential) invalidate() { - if c.mintedThisBeat || c.retriedAfterRefusal { - if c.mintedThisBeat && !c.warnedFreshRefusal { - c.warnedFreshRefusal = true - // This is the signature of a cause outside the credential, so name - // it: an operator reading "rejected (status 401)" alone would go - // looking at provisioning, which is the one thing already proven - // fine — the exchange that minted this token accepted the secret. - log.Printf("registration for %q was refused while presenting a token minted for that same beat: the credential is not stale, so re-minting cannot fix it — check that the gateway trusts the issuer that signed it, that %q may be claimed by this module, and whether %s admits %s at all", - c.prefix, c.prefix, moduleRegisterPath, internalTokenHeader) - } - return - } - // Spend the retry only when a drop actually follows, so the next beat's - // fresh mint is the one attempt this refusal was owed. - c.retriedAfterRefusal = true - c.token, c.renewAt = "", time.Time{} -} - -// succeeded ends the refusal episode: a registration this credential authorized -// was accepted, so a later refusal is a new fault — a second key rotation, say — -// and earns its own re-mint rather than inheriting the spent budget of the first. -func (c *moduleCredential) succeeded() { - c.retriedAfterRefusal = false -} - -// exchange runs the credential exchange against the gateway. It presents both -// the module secret, which identifies this module to accounts, and the -// cluster-internal token, which the gateway's own perimeter check requires. -// -// It returns the token, when it expires, and when to renew it. Expiry and -// renewal are separate answers: renewal is early by a lead, while expiry is the -// instant the gateway stops honouring the token, which is what a registration -// POST must not outlive. -func (c *moduleCredential) exchange(ctx context.Context) (string, time.Time, time.Time, error) { - ctx, cancel := context.WithTimeout(ctx, registrationExchangeTimeout) - defer cancel() - - body, _ := json.Marshal(map[string]string{"prefix": c.prefix}) - req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.tokenURL, bytes.NewReader(body)) - if err != nil { - return "", time.Time{}, time.Time{}, err - } - req.Header.Set("content-type", "application/json") - // Absent rather than present-and-empty when unconfigured: an empty header - // is the harder shape to diagnose at the gateway, which sees a caller - // claiming a credential it does not have. - if c.internalToken != "" { - req.Header.Set(internalTokenHeader, c.internalToken) - } - req.Header.Set(moduleSecretHeader, c.secret) - - resp, err := registrationClient.Do(req) - if err != nil { - return "", time.Time{}, time.Time{}, err - } - defer drainAndClose(resp) - if resp.StatusCode != http.StatusOK { - return "", time.Time{}, time.Time{}, fmt.Errorf("registration token exchange for %q rejected (status %d)", c.prefix, resp.StatusCode) - } - var issued struct { - Token string `json:"token"` - ExpiresAt time.Time `json:"expiresAt"` - } - if err := json.NewDecoder(resp.Body).Decode(&issued); err != nil { - return "", time.Time{}, time.Time{}, fmt.Errorf("registration token exchange for %q returned invalid json: %w", c.prefix, err) - } - if issued.Token == "" { - return "", time.Time{}, time.Time{}, fmt.Errorf("registration token exchange for %q returned no token", c.prefix) - } - // An expiry this process cannot use is a boundary error, not a token to - // cache: treated as "already expired" it would look like success while - // re-running the exchange on every beat. The two unusable shapes have - // different causes and different fixes, so they get different messages — - // the old single message blamed the clock for a response that never carried - // an expiry at all. - now := time.Now() - lifetime := issued.ExpiresAt.Sub(now) - switch { - case issued.ExpiresAt.IsZero(): - return "", time.Time{}, time.Time{}, fmt.Errorf( - "registration token exchange for %q returned no expiry: the issuer omitted expiresAt, or spelled it differently (protobuf JSON spells it expires_at, which does not decode into this field)", - c.prefix) - case lifetime < moduleTokenMinimumLifetime: - return "", time.Time{}, time.Time{}, fmt.Errorf( - "registration token exchange for %q returned the expiry %s, already past or less than %s away (check this host's clock against the issuer's)", - c.prefix, issued.ExpiresAt.UTC().Format(time.RFC3339), moduleTokenMinimumLifetime) - } - // Renew ahead of expiry, but never by more than half the credential's own - // life: a lead longer than the lifetime would reject the token outright, and - // the issuer's chosen lifetime is not this process's to veto. - lead := moduleTokenRenewal - if half := lifetime / 2; half < lead { - lead = half - if !c.warnedShortLifetime { - c.warnedShortLifetime = true - // Honoured, but say so once: at this lifetime the token cannot be - // held across many beats, so the mint rate is the issuer's setting - // and not a defect here. - log.Printf("registration token for %q is issued with a %s lifetime, shorter than the %s renewal lead: it will be re-obtained roughly every %s", - c.prefix, lifetime.Round(time.Second), moduleTokenRenewal, lead.Round(time.Second)) - } - } - return issued.Token, issued.ExpiresAt, issued.ExpiresAt.Add(-lead), nil -} - // --- Solution registration credentials --- // // The host binds a solution's registration to its publisher: the frontend @@ -1801,11 +1483,10 @@ const ( // floor bounds one request: whatever lifetime the issuer chose, a token that // outlives the registration POST is usable and must not be refused here. // -// moduleTokenMinimumLifetime is 5s because that token *is* cached across beats, -// and copying the figure over rejected every genuinely single-use credential an -// issuer might mint — a 3s token failed every beat forever, with a message -// blaming this host's clock. The two floors bound different things, so they are -// deliberately different values. +// It is one second, and deliberately not the several-second floor a cached +// credential needs. A floor sized for a token held across beats rejects every +// genuinely single-use credential an issuer might mint — a 3s token failed +// every beat forever, with a message blaming this host's clock. const solutionTokenMinimumLifetime = time.Second // solutionMintReportEvery is how many mints pass between one line reporting the @@ -2514,15 +2195,15 @@ func setCORS(w http.ResponseWriter) { } // siblingURL swaps the trailing `replacing` of base for path. It is how a -// derived federation endpoint follows an explicitly overridden one: both +// derived exchange endpoint follows an explicitly overridden register one: both // endpoints of the registration exchange must address the same gateway. // // It replaces a suffix rather than rebuilding from scheme+host, because a // gateway is not always mounted at the root. Rebuilding dropped everything // between the host and the endpoint, so a gateway served under a path prefix -// (GATEWAY_URL=http://gateway:8080/gw) registered at /gw/modules/_register while -// exchanging at /modules/_registration-token — an absolute URL, so validate() -// passed it, and a 404 on every beat thereafter. +// (GATEWAY_URL=http://gateway:8080/gw) registered at /gw/solutions/_register +// while exchanging at /solutions/_registration-token — an absolute URL, so +// validate() passed it, and a 404 on every beat thereafter. func siblingURL(base, replacing, path string) string { if u, err := url.Parse(base); err != nil || !u.IsAbs() || u.Host == "" { // Unparseable or relative: hand the value straight back so config @@ -2535,7 +2216,7 @@ func siblingURL(base, replacing, path string) string { // The override does not end in the endpoint we know how to pair, so its // sibling is not derivable — a query string, a trailing slash, or a // wholly custom path. Returning "" makes validate() refuse to boot and - // name the module token URL, which is the one the operator must set + // name the solution token URL, which is the one the operator must set // explicitly; synthesizing a plausible-looking guess would instead 404 // on every beat. return "" diff --git a/solution_registration_test.go b/solution_registration_test.go index 87bfa2a..a691097 100644 --- a/solution_registration_test.go +++ b/solution_registration_test.go @@ -179,8 +179,6 @@ func bootAgainst(t *testing.T, h *fakeHost, secret string) func() { publicURL: "http://127.0.0.1", hostRegisterURL: h.URL + "/api/solutions/register", gatewayRegisterURL: h.URL + solutionRegisterPath, - moduleRegisterURL: h.URL + moduleRegisterPath, - moduleTokenURL: h.URL + moduleRegistrationTokenPath, solutionTokenURL: h.URL + solutionRegistrationTokenPath, internalToken: internalTokenTest, solutionSecret: secret, @@ -521,8 +519,6 @@ func TestValidateRequiresASolutionSecret(t *testing.T) { gatewayURL: "http://gateway:42152", hostRegisterURL: "http://frontend:21931/api/solutions/register", gatewayRegisterURL: "http://gateway:42152/solutions/_register", - moduleRegisterURL: "http://gateway:42152/modules/_register", - moduleTokenURL: "http://gateway:42152/modules/_registration-token", solutionTokenURL: "http://gateway:42152/solutions/_registration-token", selfUpstream: "http://backend:8080", } @@ -697,8 +693,6 @@ func TestValidateBlamesTheEnvironmentLoadNotTheProvisioning(t *testing.T) { gatewayURL: "http://gateway:42152", hostRegisterURL: "http://frontend:21931/api/solutions/register", gatewayRegisterURL: "http://gateway:42152/solutions/_register", - moduleRegisterURL: "http://gateway:42152/modules/_register", - moduleTokenURL: "http://gateway:42152/modules/_registration-token", solutionTokenURL: "http://gateway:42152/solutions/_registration-token", environmentLoadErr: fmt.Errorf("carrier %q is unreadable", "/var/run/codefly/env"), } @@ -713,7 +707,7 @@ func TestValidateBlamesTheEnvironmentLoadNotTheProvisioning(t *testing.T) { t.Errorf("with the environment unloaded the secret may well be provisioned; the message must not send the operator to provision it: %v", err) } // Unloaded environments also empty the URLs; same misattribution. - cfg.solutionSecret, cfg.moduleTokenURL = "s3cret", "" + cfg.solutionSecret, cfg.solutionTokenURL = "s3cret", "" if err := cfg.validate(); err == nil || !strings.Contains(err.Error(), "carrier") { t.Errorf("an unresolved URL under a failed environment load must name the load: %v", err) } @@ -885,8 +879,6 @@ func TestRegistrationIntervalIsConfigurable(t *testing.T) { gatewayURL: "http://gateway:42152", hostRegisterURL: "http://frontend:21931/api/solutions/register", gatewayRegisterURL: "http://gateway:42152/solutions/_register", - moduleRegisterURL: "http://gateway:42152/modules/_register", - moduleTokenURL: "http://gateway:42152/modules/_registration-token", solutionTokenURL: "http://gateway:42152/solutions/_registration-token", solutionSecret: "s3cret", registrationIntervalErr: fmt.Errorf("unusable"), @@ -976,8 +968,6 @@ func TestSolutionRegistrationKeepsRetryingAnUnavailableHost(t *testing.T) { port: strconv.Itoa(ln.Addr().(*net.TCPAddr).Port), hostRegisterURL: srv.URL + "/api/solutions/register", gatewayRegisterURL: srv.URL + solutionRegisterPath, - moduleRegisterURL: srv.URL + moduleRegisterPath, - moduleTokenURL: srv.URL + moduleRegistrationTokenPath, solutionTokenURL: srv.URL + solutionRegistrationTokenPath, internalToken: internalTokenTest, solutionSecret: "s3cret", diff --git a/solution_test.go b/solution_test.go index da17a0d..3ab31b3 100644 --- a/solution_test.go +++ b/solution_test.go @@ -540,8 +540,6 @@ func TestConfigValidate(t *testing.T) { gatewayURL: "http://gateway:42152", hostRegisterURL: "http://frontend:21931/api/solutions/register", gatewayRegisterURL: "http://gateway:42152/solutions/_register", - moduleRegisterURL: "http://gateway:42152/modules/_register", - moduleTokenURL: "http://gateway:42152/modules/_registration-token", solutionTokenURL: "http://gateway:42152/solutions/_registration-token", solutionSecret: "s3cret", selfUpstream: "http://backend:8080", @@ -560,8 +558,6 @@ func TestConfigValidate(t *testing.T) { {"empty gateway URL", func(c *config) { c.gatewayURL = "" }}, {"relative host register URL", func(c *config) { c.hostRegisterURL = "/api/solutions/register" }}, {"relative gateway register URL", func(c *config) { c.gatewayRegisterURL = "/solutions/_register" }}, - {"relative module register URL", func(c *config) { c.moduleRegisterURL = "/modules/_register" }}, - {"relative module token URL", func(c *config) { c.moduleTokenURL = "/modules/_registration-token" }}, {"relative solution token URL", func(c *config) { c.solutionTokenURL = "/solutions/_registration-token" }}, {"no solution registration secret", func(c *config) { c.solutionSecret = "" }}, } @@ -709,10 +705,10 @@ func TestLoadConfigResolvesRenamedGateway(t *testing.T) { if cfg.gatewayURL != addr { t.Fatalf("gatewayURL = %q, want %q resolved from %s without an override", cfg.gatewayURL, addr, tc.service) } - // The gateway module-register URL defaults to the resolved gateway plus - // the /modules/_register path, with no explicit override. - if want := addr + "/modules/_register"; cfg.moduleRegisterURL != want { - t.Errorf("moduleRegisterURL = %q, want %q derived from the resolved gateway", cfg.moduleRegisterURL, want) + // The solution's own register URL defaults to the resolved gateway + // plus the /solutions/_register path, with no explicit override. + if want := addr + solutionRegisterPath; cfg.gatewayRegisterURL != want { + t.Errorf("gatewayRegisterURL = %q, want %q derived from the resolved gateway", cfg.gatewayRegisterURL, want) } }) } @@ -865,118 +861,6 @@ endpoints: } } -// moduleRegistration is the wire payload the gateway's /modules/_register -// accepts: a bare single-segment prefix (the gateway builds /v1//* from -// it) and the resolved upstream. Registration and Internal are not part of the -// body — they are the captured credential headers, so a test can assert the -// registration presents the signed, prefix-bound token and not the shared -// cluster-internal one. -type moduleRegistration struct { - Prefix string `json:"prefix"` - Upstream string `json:"upstream"` - Registration string `json:"-"` - Internal string `json:"-"` -} - -// moduleExchange is what the credential exchange (/modules/_registration-token) -// received: the prefix a module asks for, plus the two credentials the gateway -// requires — its own perimeter token and the module's registration secret. -type moduleExchange struct { - Prefix string `json:"prefix"` - Secret string `json:"-"` - Internal string `json:"-"` -} - -// fakeGateway stands in for the host gateway on the two endpoints module -// federation uses. It mirrors the real refusal semantics: the exchange mints a -// token only for a prefix whose declared secret the caller presents, and answers -// 401 otherwise (accounts' refusal, relayed). -type fakeGateway struct { - *httptest.Server - registrations chan moduleRegistration - exchanges chan moduleExchange - - // declared maps a prefix to the secret the composition provisioned for it — - // the plaintext twin of the digest accounts holds. - declared map[string]string - // tokenTTL is how long a minted token is claimed to be valid. - tokenTTL time.Duration - // registerStatus, when non-zero, is what /modules/_register answers instead - // of 200. - registerStatus int - - mu sync.Mutex - // registerRefusals, while positive, makes /modules/_register answer 401 and - // counts down — a gateway that has stopped honouring the token it was handed - // and accepts the next one. - registerRefusals int - minted int -} - -func newFakeGateway(t *testing.T, gw *fakeGateway) *fakeGateway { - t.Helper() - gw.registrations = make(chan moduleRegistration, 8) - gw.exchanges = make(chan moduleExchange, 8) - if gw.tokenTTL == 0 { - gw.tokenTTL = time.Hour - } - gw.Server = httptest.NewServer(http.HandlerFunc(gw.serve)) - t.Cleanup(gw.Close) - return gw -} - -func (g *fakeGateway) serve(w http.ResponseWriter, r *http.Request) { - switch r.URL.Path { - case moduleRegistrationTokenPath: - var exchange moduleExchange - _ = json.NewDecoder(r.Body).Decode(&exchange) - exchange.Secret = r.Header.Get(moduleSecretHeader) - exchange.Internal = r.Header.Get(internalTokenHeader) - send(g.exchanges, exchange) - if g.declared[exchange.Prefix] == "" || g.declared[exchange.Prefix] != exchange.Secret { - w.WriteHeader(http.StatusUnauthorized) - return - } - g.mu.Lock() - g.minted++ - token := fmt.Sprintf("minted-%s-%d", exchange.Prefix, g.minted) - g.mu.Unlock() - writeJSON(w, http.StatusOK, map[string]string{ - "token": token, - "expiresAt": time.Now().Add(g.tokenTTL).UTC().Format(time.RFC3339), - }) - case moduleRegisterPath: - var registration moduleRegistration - _ = json.NewDecoder(r.Body).Decode(®istration) - registration.Registration = r.Header.Get(moduleRegistrationHeader) - registration.Internal = r.Header.Get(internalTokenHeader) - send(g.registrations, registration) - g.mu.Lock() - refusing := g.registerRefusals > 0 - if refusing { - g.registerRefusals-- - } - g.mu.Unlock() - if refusing { - w.WriteHeader(http.StatusUnauthorized) - return - } - if g.registerStatus != 0 { - w.WriteHeader(g.registerStatus) - return - } - w.WriteHeader(http.StatusOK) - default: - w.WriteHeader(http.StatusOK) - } -} - -func (g *fakeGateway) mintCount() int { - g.mu.Lock() - defer g.mu.Unlock() - return g.minted -} - // send records an observation without ever blocking the fake gateway's handler: // a heartbeat re-POSTs forever, so a full channel must not wedge the server. func send[T any](ch chan T, value T) { @@ -986,32 +870,6 @@ func send[T any](ch chan T, value T) { } } -// bootWithModuleRegistry boots a real solution on ln against gw. The host and -// gateway self-registrations point at the same fake so their heartbeats don't -// spew transport errors into the test log. -func bootWithModuleRegistry(t *testing.T, ln net.Listener, gw *fakeGateway) func() { - t.Helper() - s := New(Manifest{ID: "lastlogin-go", Title: "Last Login"}) - s.cfg = config{ - port: strconv.Itoa(ln.Addr().(*net.TCPAddr).Port), - publicURL: "http://127.0.0.1", - hostRegisterURL: gw.URL + "/host", - gatewayRegisterURL: gw.URL + "/gateway", - moduleRegisterURL: gw.URL + moduleRegisterPath, - moduleTokenURL: gw.URL + moduleRegistrationTokenPath, - internalToken: internalTokenTest, - moduleSecrets: parseModuleRegistrationSecrets(os.Getenv(ModuleRegistrationSecretsEnvironmentVariable)), - } - - ctx, cancel := context.WithCancel(context.Background()) - done := make(chan struct{}) - go func() { _ = s.serve(ctx, ln); close(done) }() - return func() { - cancel() - <-done - } -} - // internalTokenTest is the cluster-internal token the fake gateway's perimeter // check expects on the credential exchange. const internalTokenTest = "internal-token-xyz" @@ -1021,524 +879,6 @@ const internalTokenTest = "internal-token-xyz" // manifest.APIConsumesEnvironmentVariable (a wire contract). const consumesDocuments = `[{"id":"docstore.documents","module":"docstore","service":"documents","endpoint":"rest","protocol":"rest","as":"documents"}]` -// TestServeRegistersConsumedAPIUpstreams proves the api.consumes federation end -// to end on the runtime's side: for each consumed target core surfaces in -// CODEFLY__API_CONSUMES, the runtime exchanges the registration secret its -// composition provisioned for a signed, prefix-bound token, then registers -// /v1/ → the SDK-resolved upstream with that token. The shared -// cluster-internal token authenticates only the exchange — the gateway rejects -// it on /modules/_register, so it must not be what the registration presents. -func TestServeRegistersConsumedAPIUpstreams(t *testing.T) { - const upstream = "http://docstore-upstream:9100" - // The consumed endpoint's address is injected because the backend depends on - // the consumed service; resolve it via the SDK snapshot. - setEndpoint(t, "CODEFLY__ENDPOINT__DOCSTORE__DOCUMENTS__REST__REST", upstream) - t.Setenv("CODEFLY__API_CONSUMES", consumesDocuments) - t.Setenv(ModuleRegistrationSecretsEnvironmentVariable, "documents:s3cret") - - gw := newFakeGateway(t, &fakeGateway{declared: map[string]string{"documents": "s3cret"}}) - - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatal(err) - } - defer bootWithModuleRegistry(t, ln, gw)() - - select { - case exchange := <-gw.exchanges: - if exchange.Prefix != "documents" { - t.Errorf("exchanged for prefix %q, want %q", exchange.Prefix, "documents") - } - if exchange.Secret != "s3cret" { - t.Errorf("exchange presented secret %q, want the provisioned %q", exchange.Secret, "s3cret") - } - if exchange.Internal != internalTokenTest { - t.Errorf("exchange presented internal token %q, want %q — the gateway's perimeter check requires it", exchange.Internal, internalTokenTest) - } - case <-time.After(5 * time.Second): - t.Fatal("gateway did not receive a registration-token exchange within timeout") - } - - select { - case reg := <-gw.registrations: - if reg.Prefix != "documents" { - t.Errorf("registered prefix = %q, want %q (the facade as core projected)", reg.Prefix, "documents") - } - if reg.Upstream != upstream { - t.Errorf("registered upstream = %q, want %q resolved via the SDK", reg.Upstream, upstream) - } - if reg.Registration != "minted-documents-1" { - t.Errorf("registered with %s = %q, want the minted token", moduleRegistrationHeader, reg.Registration) - } - if reg.Internal != "" { - t.Errorf("registration carried the shared internal token %q; the gateway refuses it and it must not leak to this path", reg.Internal) - } - case <-time.After(5 * time.Second): - t.Fatal("gateway did not receive a module registration within timeout") - } -} - -// TestServeSkipsConsumedAPIWithoutSecret proves the provisioning gap fails loud -// rather than hammering the gateway: with no secret for the prefix there is no -// credential to exchange, so every registration would be a guaranteed 401. The -// runtime neither exchanges nor registers, and says which variable is missing. -func TestServeSkipsConsumedAPIWithoutSecret(t *testing.T) { - buf := &syncBuffer{} - log.SetOutput(buf) - defer log.SetOutput(os.Stderr) - - setEndpoint(t, "CODEFLY__ENDPOINT__DOCSTORE__DOCUMENTS__REST__REST", "http://docstore-upstream:9100") - t.Setenv("CODEFLY__API_CONSUMES", consumesDocuments) - t.Setenv(ModuleRegistrationSecretsEnvironmentVariable, "") - - gw := newFakeGateway(t, &fakeGateway{}) - - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatal(err) - } - defer bootWithModuleRegistry(t, ln, gw)() - - select { - case exchange := <-gw.exchanges: - t.Fatalf("exchanged a registration token for %q with no provisioned secret", exchange.Prefix) - case reg := <-gw.registrations: - t.Fatalf("registered module %q → %q with no provisioned secret", reg.Prefix, reg.Upstream) - case <-time.After(500 * time.Millisecond): - } - - if out := buf.String(); !strings.Contains(out, ModuleRegistrationSecretsEnvironmentVariable) { - t.Errorf("missing secret was not reported against %s, got: %q", ModuleRegistrationSecretsEnvironmentVariable, out) - } -} - -// TestServeIsolatesFailedModuleRegistration proves a per-module failure stays -// per-module: the composition provisions a secret for one consumed module and a -// stale one for another, so the first federates while the second's exchange is -// refused. The refused module must not take down the backend or the sibling -// registration — it just retries on its own heartbeat. -func TestServeIsolatesFailedModuleRegistration(t *testing.T) { - setEndpoint(t, "CODEFLY__ENDPOINT__DOCSTORE__DOCUMENTS__REST__REST", "http://docstore-upstream:9100") - setEndpoint(t, "CODEFLY__ENDPOINT__BILLING__INVOICES__REST__REST", "http://billing-upstream:9200") - t.Setenv("CODEFLY__API_CONSUMES", `[`+ - `{"id":"billing.invoices","module":"billing","service":"invoices","endpoint":"rest","protocol":"rest","as":"billing"},`+ - `{"id":"docstore.documents","module":"docstore","service":"documents","endpoint":"rest","protocol":"rest","as":"documents"}]`) - t.Setenv(ModuleRegistrationSecretsEnvironmentVariable, "documents:s3cret,billing:stale") - - gw := newFakeGateway(t, &fakeGateway{declared: map[string]string{ - "documents": "s3cret", - "billing": "rotated", - }}) - - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatal(err) - } - defer bootWithModuleRegistry(t, ln, gw)() - - select { - case reg := <-gw.registrations: - if reg.Prefix != "documents" { - t.Errorf("registered prefix = %q, want only %q to reach registration", reg.Prefix, "documents") - } - case <-time.After(5 * time.Second): - t.Fatal("the module with a valid secret did not register within timeout") - } - - // The backend is still serving: the refused module's failure did not take it - // down with it. - resp, err := http.Get("http://127.0.0.1:" + strconv.Itoa(ln.Addr().(*net.TCPAddr).Port) + "/health") - if err != nil { - t.Fatalf("backend stopped serving after a module registration failure: %v", err) - } - resp.Body.Close() - if resp.StatusCode != http.StatusOK { - t.Errorf("health = %d, want 200", resp.StatusCode) - } -} - -// TestServeSkipsConsumedAPIWithoutFacade proves the runtime never guesses a -// facade prefix. Core derives an omitted `as` from the producing endpoint's -// proto package — a value the runtime cannot reconstruct from the projected -// identity — so an entry that arrives with no `as` is skipped rather than -// registered under a fabricated prefix (e.g. the module name), which would proxy -// a route the generated client never calls and could steal another facade's -// prefix. The consumed endpoint resolves fine; only the missing `as` suppresses -// registration. -func TestServeSkipsConsumedAPIWithoutFacade(t *testing.T) { - setEndpoint(t, "CODEFLY__ENDPOINT__DOCSTORE__DOCUMENTS__REST__REST", "http://docstore-upstream:9100") - t.Setenv("CODEFLY__API_CONSUMES", - `[{"id":"docstore.documents","module":"docstore","service":"documents","endpoint":"rest","protocol":"rest"}]`) - t.Setenv(ModuleRegistrationSecretsEnvironmentVariable, "documents:s3cret") - - gw := newFakeGateway(t, &fakeGateway{declared: map[string]string{"documents": "s3cret"}}) - - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatal(err) - } - defer bootWithModuleRegistry(t, ln, gw)() - - select { - case reg := <-gw.registrations: - t.Fatalf("registered module %q → %q for a consumed api with no facade entry-point (as); the runtime must not invent a prefix", reg.Prefix, reg.Upstream) - case <-time.After(500 * time.Millisecond): - // No registration, as expected. - } -} - -// TestServeRegistersNoModulesWithoutConsumes proves the no-op: a solution that -// declares no api.consumes (empty CODEFLY__API_CONSUMES) registers no module -// upstream at all, so nothing changes for the solutions that consume nothing. -func TestServeRegistersNoModulesWithoutConsumes(t *testing.T) { - t.Setenv("CODEFLY__API_CONSUMES", "") - - gw := newFakeGateway(t, &fakeGateway{}) - - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatal(err) - } - defer bootWithModuleRegistry(t, ln, gw)() - - select { - case reg := <-gw.registrations: - t.Fatalf("registered module %q → %q for a solution that declares no api.consumes", reg.Prefix, reg.Upstream) - case <-time.After(500 * time.Millisecond): - // No registration, as expected. - } -} - -// TestModuleCredentialReusesTokenUntilRenewal proves the beat does not re-mint -// every 15 seconds: a registration token lives 5 minutes, and each mint is an -// audited security event on accounts, so a token still comfortably inside its -// lifetime is reused. -func TestModuleCredentialReusesTokenUntilRenewal(t *testing.T) { - gw := newFakeGateway(t, &fakeGateway{declared: map[string]string{"documents": "s3cret"}}) - credential := &moduleCredential{ - tokenURL: gw.URL + moduleRegistrationTokenPath, - internalToken: internalTokenTest, - prefix: "documents", - secret: "s3cret", - } - - for beat := range 3 { - req, err := http.NewRequest(http.MethodPost, gw.URL+moduleRegisterPath, nil) - if err != nil { - t.Fatal(err) - } - if _, err := credential.authorize(context.Background(), req); err != nil { - t.Fatalf("beat %d: authorize: %v", beat, err) - } - if got := req.Header.Get(moduleRegistrationHeader); got != "minted-documents-1" { - t.Errorf("beat %d presented %q, want the first minted token reused", beat, got) - } - } - if got := gw.mintCount(); got != 1 { - t.Errorf("minted %d tokens across 3 beats, want 1 — a live token must be reused", got) - } -} - -// TestModuleCredentialReExchangesAfterRejection proves recovery from a refused -// registration: the gateway answering 401 means the token it was handed may no -// longer be honoured (a restarted gateway, a rotated key), so replaying it for -// the rest of its lifetime would strand the module. A refusal of a token held -// from an earlier beat forces a fresh exchange on the next one. -func TestModuleCredentialReExchangesAfterRejection(t *testing.T) { - gw := newFakeGateway(t, &fakeGateway{declared: map[string]string{"documents": "s3cret"}}) - credential := &moduleCredential{ - tokenURL: gw.URL + moduleRegistrationTokenPath, - internalToken: internalTokenTest, - prefix: "documents", - secret: "s3cret", - } - - req, err := http.NewRequest(http.MethodPost, gw.URL+moduleRegisterPath, nil) - if err != nil { - t.Fatal(err) - } - if _, err := credential.authorize(context.Background(), req); err != nil { - t.Fatal(err) - } - // A later beat presents the held token; that beat's refusal is the one that - // can be blamed on staleness, so it drops the credential. - if _, err := credential.authorize(context.Background(), req); err != nil { - t.Fatal(err) - } - credential.invalidate() - if _, err := credential.authorize(context.Background(), req); err != nil { - t.Fatal(err) - } - - if got := req.Header.Get(moduleRegistrationHeader); got != "minted-documents-2" { - t.Errorf("presented %q after invalidation, want a freshly minted token", got) - } - if got := gw.mintCount(); got != 2 { - t.Errorf("minted %d tokens, want 2 — a refused credential must be re-exchanged", got) - } -} - -// TestModuleCredentialKeepsATokenMintedForThisBeat proves the one refusal a -// fresh mint cannot fix is not answered with another mint. A gateway that -// refuses a token minted moments ago is refusing it for a reason that has -// nothing to do with staleness — it does not trust the issuer, the prefix is not -// this module's to claim, its own clock is skewed, or a perimeter check rejected -// the request before the token was ever read. Dropping the credential there put -// a mint on every single beat: an audited security event on the issuer four -// times a minute per module, indefinitely, and silent after the first log line -// because the status never changes. -func TestModuleCredentialKeepsATokenMintedForThisBeat(t *testing.T) { - log.SetOutput(io.Discard) - defer log.SetOutput(os.Stderr) - - gw := newFakeGateway(t, &fakeGateway{declared: map[string]string{"documents": "s3cret"}}) - credential := &moduleCredential{ - tokenURL: gw.URL + moduleRegistrationTokenPath, - internalToken: internalTokenTest, - prefix: "documents", - secret: "s3cret", - } - req, err := http.NewRequest(http.MethodPost, gw.URL+moduleRegisterPath, nil) - if err != nil { - t.Fatal(err) - } - if _, err := credential.authorize(context.Background(), req); err != nil { - t.Fatal(err) - } - credential.invalidate() // refused the token this beat just minted - if credential.token == "" { - t.Fatal("dropped a token minted for this very beat; re-minting cannot fix a refusal that was never about staleness") - } - if _, err := credential.authorize(context.Background(), req); err != nil { - t.Fatal(err) - } - if got := gw.mintCount(); got != 1 { - t.Errorf("minted %d tokens, want 1 — a refusal of a just-minted token must not buy another mint", got) - } -} - -// TestHeartbeatReExchangesOnRejectedRegistration proves the invalidation is -// actually wired to the beat loop, not just available on the credential: a -// gateway that answers 401 once — having stopped honouring the token it was -// handed — makes the next beat present a freshly exchanged one. -func TestHeartbeatReExchangesOnRejectedRegistration(t *testing.T) { - log.SetOutput(io.Discard) - defer log.SetOutput(os.Stderr) - - // Refuse the first two registrations. The first presents a token minted for - // that same beat, which re-minting cannot fix; the second presents that - // token held over from the earlier beat, and that is the refusal staleness - // explains — so it must be answered with a fresh exchange. - gw := newFakeGateway(t, &fakeGateway{ - declared: map[string]string{"documents": "s3cret"}, - registerRefusals: 2, - }) - credential := &moduleCredential{ - tokenURL: gw.URL + moduleRegistrationTokenPath, - internalToken: internalTokenTest, - prefix: "documents", - secret: "s3cret", - } - - ctx, cancel := context.WithCancel(context.Background()) - s := &Server{manifest: Manifest{ID: "lastlogin-go"}, registrationInterval: time.Millisecond} - done := make(chan struct{}) - go func() { - s.heartbeat(ctx, gw.URL+moduleRegisterPath, []byte(`{"prefix":"documents"}`), "gateway module documents", credential) - close(done) - }() - defer func() { cancel(); <-done }() - - first, deadline := "", time.After(10*time.Second) - for { - select { - case reg := <-gw.registrations: - switch { - case reg.Registration == "": - t.Fatal("registered with no token") - case first == "": - first = reg.Registration - case reg.Registration != first: - return // recovered: a fresh credential replaced the stale one - } - case <-deadline: - t.Fatalf("kept replaying %q: a token refused after being held across beats must be re-exchanged", first) - } - } -} - -// TestHeartbeatDoesNotMintPerBeatOnAPersistentRefusal is the regression guard -// for the mint storm. A gateway that refuses every registration does so for a -// reason no new token can repair, and the old loop answered each refusal with a -// fresh exchange: at the production 15s beat that is four mints a minute per -// module, forever, each one an audited security event on the issuer — and -// silent after the first log line, because the status never changes. -// -// The refusal is now worth exactly one re-mint, and failing beats back off, so -// the mint count stays flat however long the gateway stays broken. -func TestHeartbeatDoesNotMintPerBeatOnAPersistentRefusal(t *testing.T) { - log.SetOutput(io.Discard) - defer log.SetOutput(os.Stderr) - - gw := newFakeGateway(t, &fakeGateway{ - declared: map[string]string{"documents": "s3cret"}, - registerStatus: http.StatusUnauthorized, - }) - credential := &moduleCredential{ - tokenURL: gw.URL + moduleRegistrationTokenPath, - internalToken: internalTokenTest, - prefix: "documents", - secret: "s3cret", - } - - ctx, cancel := context.WithCancel(context.Background()) - s := &Server{manifest: Manifest{ID: "lastlogin-go"}, registrationInterval: time.Millisecond} - done := make(chan struct{}) - go func() { - s.heartbeat(ctx, gw.URL+moduleRegisterPath, []byte(`{"prefix":"documents"}`), "gateway module documents", credential) - close(done) - }() - defer func() { cancel(); <-done }() - - const beats = 5 - for range beats { - select { - case <-gw.registrations: - case <-time.After(10 * time.Second): - t.Fatalf("gateway saw fewer than %d registration attempts", beats) - } - } - // One mint to obtain the credential, at most one more to answer the refusal. - if got := gw.mintCount(); got > 2 { - t.Errorf("minted %d tokens across %d refused beats, want at most 2 — a refusal that a new token cannot fix must not re-mint on every beat", got, beats) - } -} - -// TestModuleCredentialRetriesAgainAfterRecovering proves a spent retry budget is -// restored by a successful registration, so a second, independent refusal — a -// gateway rotating its key twice inside one credential lifetime — is answered -// with a fresh token instead of inheriting the first refusal's spent attempt and -// stranding the module until the token renews on its own. -func TestModuleCredentialRetriesAgainAfterRecovering(t *testing.T) { - gw := newFakeGateway(t, &fakeGateway{declared: map[string]string{"documents": "s3cret"}}) - credential := &moduleCredential{ - tokenURL: gw.URL + moduleRegistrationTokenPath, - internalToken: internalTokenTest, - prefix: "documents", - secret: "s3cret", - } - req, err := http.NewRequest(http.MethodPost, gw.URL+moduleRegisterPath, nil) - if err != nil { - t.Fatal(err) - } - authorize := func() { - t.Helper() - if _, err := credential.authorize(context.Background(), req); err != nil { - t.Fatal(err) - } - } - authorize() // beat 1: mint - authorize() // beat 2: replay the held token - credential.invalidate() // refused: spends the retry, drops - authorize() // beat 3: mint the replacement - credential.succeeded() // ... which the gateway accepts - - authorize() // beat 4: replay - credential.invalidate() // a second, independent refusal - if credential.token != "" { - t.Fatal("kept a token refused after an intervening success: a new fault must earn its own re-mint") - } - authorize() - if got := gw.mintCount(); got != 3 { - t.Errorf("minted %d tokens, want 3 — one per refusal episode plus the original", got) - } -} - -// TestHeartbeatDropsACredentialRefusedWith403 proves 403 recovers like 401. A -// gateway answering "forbidden" to a lapsed token would otherwise have that -// token replayed until it renewed naturally, stranding the module for most of -// the credential's lifetime. -func TestHeartbeatDropsACredentialRefusedWith403(t *testing.T) { - credential := &moduleCredential{prefix: "documents", token: "held", renewAt: time.Now().Add(time.Hour)} - // A token held from an earlier beat: this beat did not mint it. - credential.invalidate() - if credential.token != "" { - t.Fatal("a refused credential held from an earlier beat must be dropped") - } - - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusForbidden) - })) - defer srv.Close() - if status := statusOfOneBeat(t, srv.URL); status != http.StatusForbidden { - t.Fatalf("beat status = %d, want 403", status) - } -} - -// statusOfOneBeat runs a single beat against target and returns its status. -func statusOfOneBeat(t *testing.T, target string) int { - t.Helper() - s := &Server{manifest: Manifest{ID: "lastlogin-go"}} - status, _, err := s.beat(context.Background(), target, []byte(`{}`), internalTokenAuth("tok")) - if err != nil { - t.Fatal(err) - } - return status -} - -// TestExchangeRejectsAnUnusableExpiry proves the credential refuses an expiry -// it cannot act on instead of caching it as "already expired". Treated as -// expired it would re-run the exchange on every beat while every registration -// still returned 200 — an unlogged mint, and an audited security event on the -// issuer, four times a minute for as long as the solution runs. -func TestExchangeRejectsAnUnusableExpiry(t *testing.T) { - for _, tc := range []struct { - name string - body map[string]string - }{ - // A response whose expiry field never arrives (an unset timestamp on the - // issuer, or a rename to the protobuf JSON spelling) decodes to the zero - // time. - {name: "absent", body: map[string]string{"token": "t"}}, - {name: "epoch zero", body: map[string]string{"token": "t", "expiresAt": "1970-01-01T00:00:00Z"}}, - // Stands in for a host clock skewed past the credential's own lifetime. - {name: "already past", body: map[string]string{ - "token": "t", "expiresAt": time.Now().Add(-time.Minute).UTC().Format(time.RFC3339)}}, - // Below the minimum usable lifetime: it would lapse in flight. - {name: "lapses in flight", body: map[string]string{ - "token": "t", "expiresAt": time.Now().Add(2 * time.Second).UTC().Format(time.RFC3339)}}, - } { - t.Run(tc.name, func(t *testing.T) { - exchanges := 0 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - exchanges++ - writeJSON(w, http.StatusOK, tc.body) - })) - defer srv.Close() - - credential := &moduleCredential{ - tokenURL: srv.URL, internalToken: internalTokenTest, - prefix: "documents", secret: "s3cret", - } - req, err := http.NewRequest(http.MethodPost, srv.URL, nil) - if err != nil { - t.Fatal(err) - } - if _, err := credential.authorize(context.Background(), req); err == nil { - t.Fatal("accepted a token the runtime cannot hold; a beat loop would re-mint forever") - } - // A second beat must not have cached anything either. - _, _ = credential.authorize(context.Background(), req) - if exchanges != 2 { - t.Errorf("exchanged %d times across 2 beats, want 2 attempts and 0 cached", exchanges) - } - if credential.token != "" { - t.Errorf("cached a token with an unusable expiry: %q", credential.token) - } - }) - } -} - // TestRegistrationRequestsAreBounded is the regression guard for a wedged // heartbeat. The beat's context carries no deadline, so without a client timeout // a gateway that accepts a registration and never answers blocked the beat in @@ -1569,7 +909,7 @@ func TestExchangeOmitsAnUnconfiguredInternalToken(t *testing.T) { })) defer srv.Close() - credential := &moduleCredential{tokenURL: srv.URL, prefix: "documents", secret: "s3cret"} + credential := &solutionCredential{tokenURL: srv.URL, id: "solution-under-test", secret: "s3cret"} req, err := http.NewRequest(http.MethodPost, srv.URL, nil) if err != nil { t.Fatal(err) @@ -1582,72 +922,28 @@ func TestExchangeOmitsAnUnconfiguredInternalToken(t *testing.T) { } } -// TestExchangeAcceptsAShortButUsableExpiry proves the renewal lead is a ceiling -// on how early the credential renews, not a floor on what the issuer may issue. -// Conflating the two refused every token whose whole life was shorter than the -// 30s lead — a 30s credential was rejected outright, and the error blamed this -// host's clock for an issuer setting that was deliberate and fine. A short token -// is now held and renewed at half its life. -func TestExchangeAcceptsAShortButUsableExpiry(t *testing.T) { - log.SetOutput(io.Discard) - defer log.SetOutput(os.Stderr) - - for _, ttl := range []time.Duration{10 * time.Second, 30 * time.Second, 5 * time.Minute} { - t.Run(ttl.String(), func(t *testing.T) { - gw := newFakeGateway(t, &fakeGateway{ - declared: map[string]string{"documents": "s3cret"}, - tokenTTL: ttl, - }) - credential := &moduleCredential{ - tokenURL: gw.URL + moduleRegistrationTokenPath, - internalToken: internalTokenTest, - prefix: "documents", - secret: "s3cret", - } - req, err := http.NewRequest(http.MethodPost, gw.URL+moduleRegisterPath, nil) - if err != nil { - t.Fatal(err) - } - if _, err := credential.authorize(context.Background(), req); err != nil { - t.Fatalf("refused a usable %s credential: %v", ttl, err) - } - if req.Header.Get(moduleRegistrationHeader) == "" { - t.Error("presented no token") - } - // Renewal is at half-life for a short token, and a full lead ahead - // of expiry for a long one — never after expiry, and never so early - // that the token is discarded unused. - wantLead := min(moduleTokenRenewal, ttl/2) - held := time.Until(credential.renewAt) - if held <= 0 || held > ttl-wantLead+time.Second { - t.Errorf("renewAt is %s away for a %s credential, want about %s", held, ttl, ttl-wantLead) - } - }) - } -} - -// TestSiblingURLFollowsAnOverriddenGateway proves the two federation endpoints -// stay on one gateway. The credential a registration presents is minted by the -// exchange, so an explicitly overridden registration URL must carry the -// exchange with it — minting against one host and registering with another -// yields a token the second never trusts. +// TestSiblingURLFollowsAnOverriddenGateway proves the register endpoint and the +// exchange that credentials it stay on one gateway. The token a registration +// presents is minted by the exchange, so an explicitly overridden register URL +// must carry the exchange with it — minting against one host and registering +// with another yields a token the second never trusts. func TestSiblingURLFollowsAnOverriddenGateway(t *testing.T) { t.Setenv("PORT", "8090") t.Setenv("GATEWAY_URL", "http://gateway:42152") t.Setenv("HOST_REGISTER_URL", "http://frontend:21931/api/solutions/register") - t.Setenv("GATEWAY_MODULE_REGISTER_URL", "https://other-gateway:9999/modules/_register") + t.Setenv("GATEWAY_REGISTER_URL", "https://other-gateway:9999"+solutionRegisterPath) - cfg := loadConfig(context.Background(), "lastlogin-go") - want := "https://other-gateway:9999" + moduleRegistrationTokenPath - if cfg.moduleTokenURL != want { - t.Errorf("module token URL = %q, want %q — it must follow the overridden register URL", cfg.moduleTokenURL, want) + cfg := loadConfig(context.Background(), "solution-under-test") + want := "https://other-gateway:9999" + solutionRegistrationTokenPath + if cfg.solutionTokenURL != want { + t.Errorf("solution token URL = %q, want %q — it must follow the overridden register URL", cfg.solutionTokenURL, want) } } // An unparseable override must surface as itself, so validate() names the one // URL the operator actually set instead of a second one derived from it. func TestSiblingURLPassesThroughAnUnusableBase(t *testing.T) { - if got := siblingURL("/modules/_register", moduleRegisterPath, moduleRegistrationTokenPath); got != "/modules/_register" { + if got := siblingURL(solutionRegisterPath, solutionRegisterPath, solutionRegistrationTokenPath); got != solutionRegisterPath { t.Errorf("siblingURL(relative) = %q, want the base handed back unchanged", got) } } @@ -1655,52 +951,26 @@ func TestSiblingURLPassesThroughAnUnusableBase(t *testing.T) { // TestSiblingURLKeepsTheGatewayBasePath proves the derived endpoint stays on the // gateway's mount, not just its host. Rebuilding the URL from scheme+host // dropped any path between them, so a gateway served under a prefix registered -// at /gw/modules/_register while exchanging at /modules/_registration-token — -// still absolute, so validate() passed it, and a 404 on every beat after that. +// at /gw/solutions/_register while exchanging at /solutions/_registration-token +// — still absolute, so validate() passed it, and a 404 on every beat after that. func TestSiblingURLKeepsTheGatewayBasePath(t *testing.T) { t.Setenv("PORT", "8090") t.Setenv("GATEWAY_URL", "http://gateway:42152/gw") t.Setenv("HOST_REGISTER_URL", "http://frontend:21931/api/solutions/register") t.Setenv(SolutionRegistrationSecretEnvironmentVariable, "s3cret") - cfg := loadConfig(context.Background(), "lastlogin-go") - if want := "http://gateway:42152/gw" + moduleRegisterPath; cfg.moduleRegisterURL != want { - t.Errorf("module register URL = %q, want %q", cfg.moduleRegisterURL, want) + cfg := loadConfig(context.Background(), "solution-under-test") + if want := "http://gateway:42152/gw" + solutionRegisterPath; cfg.gatewayRegisterURL != want { + t.Errorf("gateway register URL = %q, want %q", cfg.gatewayRegisterURL, want) } - if want := "http://gateway:42152/gw" + moduleRegistrationTokenPath; cfg.moduleTokenURL != want { - t.Errorf("module token URL = %q, want %q — the derived endpoint must keep the gateway's base path", cfg.moduleTokenURL, want) + if want := "http://gateway:42152/gw" + solutionRegistrationTokenPath; cfg.solutionTokenURL != want { + t.Errorf("solution token URL = %q, want %q — the derived endpoint must keep the gateway's base path", cfg.solutionTokenURL, want) } if err := cfg.validate(); err != nil { t.Errorf("validate() = %v, want nil", err) } } -// TestSiblingURLRefusesAnUnpairableOverride proves an override whose sibling -// cannot be derived fails loud at boot rather than resolving to a plausible -// guess that 404s on every beat. The operator must then name the token URL. -func TestSiblingURLRefusesAnUnpairableOverride(t *testing.T) { - t.Setenv("PORT", "8090") - t.Setenv("GATEWAY_URL", "http://gateway:42152") - t.Setenv("HOST_REGISTER_URL", "http://frontend:21931/api/solutions/register") - t.Setenv("GATEWAY_MODULE_REGISTER_URL", "https://other-gateway:9999/custom/registration-endpoint") - t.Setenv(SolutionRegistrationSecretEnvironmentVariable, "s3cret") - - cfg := loadConfig(context.Background(), "lastlogin-go") - if cfg.moduleTokenURL != "" { - t.Errorf("module token URL = %q, want empty so validate() names it", cfg.moduleTokenURL) - } - err := cfg.validate() - if err == nil || !strings.Contains(err.Error(), "module token URL") { - t.Errorf("validate() = %v, want an error naming the module token URL", err) - } - - // Naming it explicitly is the documented way out. - t.Setenv("GATEWAY_MODULE_REGISTRATION_TOKEN_URL", "https://other-gateway:9999/custom/token-endpoint") - if err := loadConfig(context.Background(), "lastlogin-go").validate(); err != nil { - t.Errorf("validate() with an explicit token URL = %v, want nil", err) - } -} - // TestBackoffGrowsWhileBrokenAndResetsWhenHealthy pins the retry schedule that // bounds every failure path on the credential exchange. func TestBackoffGrowsWhileBrokenAndResetsWhenHealthy(t *testing.T) { @@ -1729,37 +999,6 @@ func TestBackoffGrowsWhileBrokenAndResetsWhenHealthy(t *testing.T) { } } -func TestParseModuleRegistrationSecrets(t *testing.T) { - tests := []struct { - name string - raw string - want map[string]string - }{ - {name: "empty", raw: "", want: map[string]string{}}, - {name: "one", raw: "documents:s3cret", want: map[string]string{"documents": "s3cret"}}, - { - name: "several, spaced", - raw: "documents:s3cret, billing:other", - want: map[string]string{"documents": "s3cret", "billing": "other"}, - }, - // A secret is opaque and base64 may contain "=" and "+"; only the first - // ":" separates it from the prefix. - {name: "secret keeps inner colons", raw: "documents:a:b", want: map[string]string{"documents": "a:b"}}, - // The registrar trims both halves of its digest twin; parsing the two - // asymmetrically turns a pair it accepts into a lookup miss here. - {name: "spaces around both halves", raw: "documents : s3cret", want: map[string]string{"documents": "s3cret"}}, - {name: "unpaired entry dropped", raw: "documents", want: map[string]string{}}, - {name: "empty secret dropped", raw: "documents:", want: map[string]string{}}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := parseModuleRegistrationSecrets(tt.raw); !reflect.DeepEqual(got, tt.want) { - t.Errorf("parseModuleRegistrationSecrets(%q) = %v, want %v", tt.raw, got, tt.want) - } - }) - } -} - // --- Work Context --- // mintRequest is what the accounts StartTask RPC received: the ask itself, plus From a7d5dc7e203791d710983c5ca4d563201d0d9859 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Mon, 5 Oct 2026 19:54:55 -0400 Subject: [PATCH 02/12] fix: refuse an undecodable api.consumes projection at boot, and guard the facade invariant by shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings R53-02 and R53-03 on #53. R53-01 is answered in the PR body. R53-02. I claimed the projection was now governed by validate(). It was not: loadConfig resolved it, and the only code that decoded it was the code that used it, which returns early when the solution declares no Consumes. So the same undecodable value had two answers — refused for a solution with a passthrough, accepted and served for one without. loadConfig now decodes it once (parseAPIConsumes) and validate() refuses what will not decode, whatever the solution declares, naming the variable it arrived in: it is the composition's output, so no operator setting repairs it. Whitespace alone is not malformed, it is unset — refusing a render's bare newline would fail a boot over nothing. validatePassthrough consumes the decoded value instead of re-parsing, and PassthroughHandler supplies it directly, since its caller hands over the typed projection. R53-03. Both new guards passed a restored claim that spelled its paths and headers differently and started its beat 80ms late. Each was testing its own spelling rather than the invariant: - the behavioural test judged two known paths at the first self-registration beat. It now allowlists every path and every Codefly credential header a solution may present, over five beats on a 20ms interval, so anything else fails whatever it is named and however late it starts. Six input shapes: nothing consumed, one target, a renamed facade, several targets, a delivered secret with the old overrides set, and a delivered secret for a module nothing consumes. - the source guard matched exact substrings. It now compares normalised string literals from a token scan, folding what spelling may move without changing the wire shape — literal boundaries, case, and the separators in these names — so a shape assembled from a concatenation is seen whole. Identifiers are never joined, so an adjacency between unrelated names cannot read as a shape. Mutation-checked, all red before green: the equivalent-spelling mutant with the delayed beat turns the source guard red on all five shapes and the behavioural guard red in 20 of 20 repetitions (and the real head green in 20 of 20); removing the projection refusal turns all five R53-02 cases red. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 15 +- README.md | 25 ++- environment_boundary_test.go | 16 +- facade_claim_test.go | 371 +++++++++++++++++++++++++---------- passthrough.go | 13 +- projection_boundary_test.go | 113 +++++++++++ solution.go | 34 ++++ 7 files changed, 463 insertions(+), 124 deletions(-) create mode 100644 projection_boundary_test.go diff --git a/AGENTS.md b/AGENTS.md index 30bd434..d3517e2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,10 +45,17 @@ own: it builds on `Server.PassthroughHandler`, the handler `Serve` mounts. the boot refused with the name of something nobody could set (#48). There is no exception: every environment read sits in `loadConfig`'s call tree, and `environment_boundary_test.go` fails the suite if one appears - outside it. There used to be one — the consumed-API projection, read at serve - time and never seen by `validate()`, so a malformed one disabled the - federation with a log line while the solution served on — and it went with the - code that read it (#51). + outside it. There used to be one, and it went with the code that read it + (#51). +- Reading a value in `loadConfig` is not the same as checking it, and the + difference is where a gap hides. The `api.consumes` projection was resolved + at boot while still being *parsed* only by the code that consumed it, so a + solution declaring no `Consumes` booted and served with a projection nothing + had ever decoded. `validate()` now decodes it whatever the solution declares + (`parseAPIConsumes`, `projection_boundary_test.go`). When you add + configuration, resolving it in `loadConfig` is half the rule — the other half + is a refusal in `validate()` that fires whether or not the feature using it + is switched on. - A solution registers nothing but itself. It claims no facade route for a module it consumes and holds no credential that would let it: the route for a module is claimed by the module that serves it, under a credential bound to diff --git a/README.md b/README.md index 5600b57..cf142b0 100644 --- a/README.md +++ b/README.md @@ -255,12 +255,9 @@ rather than preferences: delivers one anyway, this runtime reads nothing from it — the carrier has no reader here. -The reason is what such a credential decides. A route claim says where every -authenticated request on a prefix is proxied; a solution holding one holds the -routing of traffic that is not its own, and a claim surface that admits a -caller attesting to something other than the module itself admits whoever -claims the prefix first. Neither is a solution's to hold, so the capability is -not on this runtime's surface at all. See [codefly-dev/module-saas-starter#1018](https://github.com/codefly-dev/module-saas-starter/issues/1018) +Routing is a platform concern, not a solution's, so the capability is not on +this runtime's surface at all — there is no narrower version of it to offer. +See [codefly-dev/module-saas-starter#1018](https://github.com/codefly-dev/module-saas-starter/issues/1018) for where a module's own claim is being built, and [#953](https://github.com/codefly-dev/module-saas-starter/issues/953) for the lifecycle rework that replaces the heartbeat writer. @@ -274,8 +271,20 @@ reported where it happens rather than papered over by claiming the prefix here. Codefly still projects the consumed targets into `CODEFLY__API_CONSUMES`, and the runtime still reads it — at boot, through `loadConfig`, to check the -`Consumes` declaration against it before the listener exists. It is used for -nothing else. +[`Consumes`](#letting-the-page-call-a-consumed-module-the-passthrough) +declaration against it before the listener exists. It is used for nothing else. + +A projection that cannot be decoded now **fails the boot**, naming +`CODEFLY__API_CONSUMES`, whether or not the solution declares a passthrough. +Previously it was decoded only by the code that consumed it, so a solution +declaring no `Consumes` served on with a projection nothing had looked at, and +one that did declare a passthrough was refused — the same broken value, two +answers. Unset is not malformed: a solution that consumes nothing leaves the +variable empty, and whitespace alone counts as empty, so a render emitting a +bare newline for it does not fail a boot. Fixing a malformed one is the +composition's job (`codefly run solution`), which is why the refusal names the +variable it arrived in rather than a workspace-configuration group — nothing an +operator provisions can repair it. ### Reading a Work-Context-authenticated module diff --git a/environment_boundary_test.go b/environment_boundary_test.go index 3e2ddf9..f4a1be8 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -12,18 +12,22 @@ import ( ) // retiredServeTimeRead named the one function that used to read the -// environment outside boot resolution: it parsed the consumed-API projection -// when the server started, so a malformed value disabled the consumed-module -// federation with a log line while the solution served on. It registered a -// facade route for each consumed module, which is not a solution's to claim, -// and went with that capability — so the exception it was went with it too. +// environment outside boot resolution. It claimed a facade route for each +// consumed module, which is not a solution's to claim, so it went — and the +// exception it was went with it. const retiredServeTimeRead = "registerConsumedAPIs" // AGENTS.md tells an agent that configuration is resolved in one place and // refused at boot, with no exception. Prose cannot notice an exception // appearing underneath it, and an agent-context file is followed literally, so // the claim is pinned here: every environment read sits in loadConfig's call -// tree, where validate() governs the result. +// tree. +// +// Sitting there is necessary and not sufficient — loadConfig resolving a value +// says nothing about validate() judging it, and the projection that used to be +// read here was resolved at boot for a release while still reaching no check. +// What validate() does with it is pinned separately, in +// projection_boundary_test.go. func TestEnvironmentIsReadOnlyWhereAgentsFileSaysItIs(t *testing.T) { pkg := parsePackage(t) boot := pkg.reachableFrom("loadConfig") diff --git a/facade_claim_test.go b/facade_claim_test.go index 274296f..76825f0 100644 --- a/facade_claim_test.go +++ b/facade_claim_test.go @@ -2,6 +2,8 @@ package solution import ( "context" + "go/scanner" + "go/token" "net" "net/http" "net/http/httptest" @@ -11,54 +13,74 @@ import ( "strings" "sync" "testing" + "time" + + "github.com/codefly-dev/core/solution/manifest" ) // --- A solution claims no module's facade route --- // -// SP-GW-04 and SP-SOL-03, the two invariants SA-F-GWREGISTRY broke: a facade -// route for a module is claimed only by the module that serves it, under a -// credential bound to that module, and a solution holds no credential that -// decides where another module's traffic is routed. +// Two invariants, both pinned here (SA-F-GWREGISTRY): +// +// - a facade route for a module is claimed only by the module that serves +// it, under a credential bound to that module; +// - a solution holds no credential that decides where another module's +// traffic is routed. // -// This runtime used to claim one for every module its composition let it -// consume. The paths and headers of that surface are written out here, as -// literals this test owns, for two reasons: nothing in the package names them -// any more, and these are the wire shapes a solution must never present. The -// static guard below keeps them out of the package proper. +// So a solution registers itself and nothing else, and the package carries +// neither a module route-claim surface nor a credential for one. The wire +// shapes below are named by these tests alone, because a test that cannot +// write a shape down cannot assert its absence. const ( // claimedFacadePath and facadeCredentialPath are the gateway's module - // route-claim surface and the exchange that mints the credential for it. + // route-claim surface and the exchange that credentials it. claimedFacadePath = "/modules/_register" facadeCredentialPath = "/modules/_registration-token" // facadeSecretHeader carries a module's own route-claim secret, and // facadeCredentialHeader the credential minted from it. facadeSecretHeader = "X-Codefly-Module-Secret" facadeCredentialHeader = "X-Codefly-Module-Registration" - // facadeSecretsVariable is the carrier that delivered another module's - // route-claim secret into a solution's own environment. + // facadeSecretsVariable is the carrier a composition may deliver another + // module's route-claim secret in. Nothing here reads it. facadeSecretsVariable = "CODEFLY__MODULE_REGISTRATION_SECRETS" ) -// facadeWatcher is a host gateway that answers this runtime's own registration -// exchange and records every request it receives, so a test can assert on the -// ones that never arrive. +// selfRegistrationPaths is what a solution registering only itself may call: +// its manifest to the host frontend, its own upstream to the gateway, and the +// exchange that credentials both. It is an allowlist rather than a list of +// forbidden paths, so a claim on any other route fails this test whatever that +// route is named — a denylist only catches the spellings it was given. +var selfRegistrationPaths = map[string]bool{ + "/api/solutions/register": true, + solutionRegisterPath: true, + solutionRegistrationTokenPath: true, +} + +// selfRegistrationHeaders is the same allowlist for the credential headers a +// registration may carry. +var selfRegistrationHeaders = map[string]bool{ + http.CanonicalHeaderKey(solutionRegistrationHeader): true, + http.CanonicalHeaderKey(solutionSecretHeader): true, + http.CanonicalHeaderKey(internalTokenHeader): true, +} + +// facadeWatcher is a host that answers this runtime's own registration +// exchange and records every request it receives — the path, and every +// Codefly credential header on it — so a test can judge the whole of what a +// booted solution sends rather than the absence of two known shapes. type facadeWatcher struct { *httptest.Server - mu sync.Mutex - paths []string - // secrets is every value seen in a header that carries another module's - // route-claim credential, on any path. - secrets []string - // ownRegistrations counts this solution's own upstream registration, which - // proves the boot reached the point where it would have claimed a facade. - ownRegistrations int + mu sync.Mutex + paths map[string]int + headers map[string]string + beats int } func newFacadeWatcher(t *testing.T) *facadeWatcher { t.Helper() - w := &facadeWatcher{} + w := &facadeWatcher{paths: map[string]int{}, headers: map[string]string{}} w.Server = httptest.NewServer(http.HandlerFunc(w.serve)) t.Cleanup(w.Close) return w @@ -66,102 +88,174 @@ func newFacadeWatcher(t *testing.T) *facadeWatcher { func (g *facadeWatcher) serve(w http.ResponseWriter, r *http.Request) { g.mu.Lock() - g.paths = append(g.paths, r.URL.Path) - for _, header := range []string{facadeSecretHeader, facadeCredentialHeader} { - if value := r.Header.Get(header); value != "" { - g.secrets = append(g.secrets, header+": "+value) + g.paths[r.URL.Path]++ + for name, values := range r.Header { + if strings.HasPrefix(strings.ToLower(name), "x-codefly-") { + g.headers[http.CanonicalHeaderKey(name)] = strings.Join(values, ",") } } if r.URL.Path == solutionRegisterPath { - g.ownRegistrations++ + g.beats++ } g.mu.Unlock() answeringTheExchange(w, r) } -func (g *facadeWatcher) seen() ([]string, []string, int) { +func (g *facadeWatcher) observed() (map[string]int, map[string]string, int) { g.mu.Lock() defer g.mu.Unlock() - return append([]string(nil), g.paths...), append([]string(nil), g.secrets...), g.ownRegistrations + paths := map[string]int{} + for k, v := range g.paths { + paths[k] = v + } + headers := map[string]string{} + for k, v := range g.headers { + headers[k] = v + } + return paths, headers, g.beats } -// TestSolutionClaimsNoModuleFacadeRoute boots a real solution in the shape the -// finding describes: a composition that projects a consumed module and -// provisions that module's route-claim secret into this backend's environment. -// Nothing the runtime does may turn either into a claim on that module's -// facade route — not the projection, which says only what this solution reads, -// and not the secret, which is another module's credential however it got here. +// TestSolutionRegistersOnlyItself boots a real solution against a recording +// host and judges every request it makes: the path must be one of this +// solution's own registration surfaces, and any Codefly credential header on +// it one of its own. // -// The solution's own upstream registration is asserted too, so the test cannot -// pass by failing to boot. -func TestSolutionClaimsNoModuleFacadeRoute(t *testing.T) { - gw := newFacadeWatcher(t) - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatal(err) - } +// It waits for several beats rather than the first. A registration beat is a +// goroutine on a timer, so sampling once the self-registration has succeeded +// proves only that nothing else had started yet — a claim that began a moment +// later would land after the assertions. +// +// The inputs vary because the invariant does not: whatever the composition +// projects as consumable, and whatever it delivers alongside, the answer is +// the same. +func TestSolutionRegistersOnlyItself(t *testing.T) { + for _, tc := range []struct { + name string + // consumes is the api.consumes projection, and secrets whatever the + // composition delivered in the route-claim carrier. + consumes, secrets string + // legacyOverrides sets the env overrides that used to point the claim + // surface and its exchange somewhere; they name nothing now. + legacyOverrides bool + }{ + {name: "nothing consumed"}, + {name: "one target", consumes: consumesDocuments, secrets: "documents:another-modules-secret"}, + { + name: "a renamed facade", + consumes: `[{"id":"a.b","module":"a","service":"b","endpoint":"rest","protocol":"rest","as":"renamed"}]`, + secrets: "renamed:another-modules-secret", + }, + { + name: "several targets", + consumes: `[{"id":"a.b","module":"a","service":"b","endpoint":"rest","protocol":"rest","as":"first"},` + + `{"id":"c.d","module":"c","service":"d","endpoint":"rest","protocol":"rest","as":"second"}]`, + secrets: " first : s1 , second:s2 ", + }, + { + name: "a secret delivered with the old overrides set", + consumes: consumesDocuments, + secrets: "documents:another-modules-secret", + legacyOverrides: true, + }, + {name: "a secret delivered for a module nothing consumes", secrets: "documents:another-modules-secret"}, + } { + t.Run(tc.name, func(t *testing.T) { + gw := newFacadeWatcher(t) + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } - setEndpoint(t, "CODEFLY__ENDPOINT__DOCSTORE__DOCUMENTS__REST__REST", "http://docstore-upstream:9100") - t.Setenv("CODEFLY__API_CONSUMES", consumesDocuments) - // Provisioned, and still never used: the fix is not that the composition - // stops delivering this (that is the composition's half of the finding), - // it is that a runtime holding it claims nothing with it. - t.Setenv(facadeSecretsVariable, "documents:another-modules-secret") - t.Setenv("GATEWAY_URL", gw.URL) - t.Setenv("HOST_REGISTER_URL", gw.URL+"/api/solutions/register") - t.Setenv("PORT", strconv.Itoa(ln.Addr().(*net.TCPAddr).Port)) - t.Setenv("SELF_UPSTREAM", "http://backend.svc.cluster.local:8080") - t.Setenv("CODEFLY__SOLUTION_REGISTRATION_SECRET", "s3cret") - t.Setenv("CODEFLY_INTERNAL_TOKEN", internalTokenTest) - - s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) - ctx, cancel := context.WithCancel(context.Background()) - s.cfg = loadConfig(ctx, s.manifest.ID) - done := make(chan struct{}) - go func() { _ = s.serve(ctx, ln); close(done) }() - defer func() { cancel(); <-done }() - - waitFor(t, "this solution's own upstream registration", func() bool { - _, _, own := gw.seen() - return own > 0 - }) - - paths, secrets, _ := gw.seen() - for _, path := range paths { - if path == claimedFacadePath || path == facadeCredentialPath { - t.Errorf("the solution called %q: a facade route is claimed only by the module that serves it, never by a solution that consumes it", path) - } - } - if len(secrets) > 0 { - t.Errorf("the solution presented another module's route-claim credential: %s", strings.Join(secrets, ", ")) + setEndpoint(t, "CODEFLY__ENDPOINT__DOCSTORE__DOCUMENTS__REST__REST", "http://docstore-upstream:9100") + t.Setenv(manifest.APIConsumesEnvironmentVariable, tc.consumes) + // Delivered, and still unused: the fix is not that a composition + // stops delivering this — that is the composition's half — it is + // that a runtime holding it claims nothing with it. + t.Setenv(facadeSecretsVariable, tc.secrets) + if tc.legacyOverrides { + t.Setenv("GATEWAY_MODULE_REGISTER_URL", gw.URL+claimedFacadePath) + t.Setenv("GATEWAY_MODULE_REGISTRATION_TOKEN_URL", gw.URL+facadeCredentialPath) + } + t.Setenv("GATEWAY_URL", gw.URL) + t.Setenv("HOST_REGISTER_URL", gw.URL+"/api/solutions/register") + t.Setenv("PORT", strconv.Itoa(ln.Addr().(*net.TCPAddr).Port)) + t.Setenv("SELF_UPSTREAM", "http://backend.svc.cluster.local:8080") + t.Setenv(SolutionRegistrationSecretEnvironmentVariable, "s3cret") + t.Setenv("CODEFLY_INTERNAL_TOKEN", internalTokenTest) + + s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) + // Beat fast, so several beats pass inside the test rather than one. + s.registrationInterval = 20 * time.Millisecond + ctx, cancel := context.WithCancel(context.Background()) + s.cfg = loadConfig(ctx, s.manifest.ID) + if err := s.cfg.validate(); err != nil { + t.Fatalf("the test environment does not boot: %v", err) + } + done := make(chan struct{}) + go func() { _ = s.serve(ctx, ln); close(done) }() + defer func() { cancel(); <-done }() + + // Several beats, not the first: a claim beat starting late must + // still fall inside the window being judged. + const beatsWatched = 5 + waitFor(t, "this solution's own registration to beat repeatedly", func() bool { + _, _, beats := gw.observed() + return beats >= beatsWatched + }) + + paths, headers, beats := gw.observed() + if beats < beatsWatched { + t.Fatalf("watched only %d self-registration beats, want %d", beats, beatsWatched) + } + for path, count := range paths { + if !selfRegistrationPaths[path] { + t.Errorf("the solution called %q (%d times): a solution registers only itself, and a facade route for a module is claimed by the module that serves it", + path, count) + } + } + for name, value := range headers { + if !selfRegistrationHeaders[name] { + t.Errorf("the solution presented the credential header %q: a solution holds no credential that decides where another module's traffic is routed", name) + } + if tc.secrets != "" && strings.Contains(value, "another-modules-secret") { + t.Errorf("header %q carried a delivered route-claim secret", name) + } + } + }) } } -// TestNoModuleFacadeClaimOnTheSDKSurface is the static half: the behaviour test -// above can only prove that one boot claimed nothing, and the capability is -// what the invariant is about. A solution built on this SDK must not be able to -// claim a module's facade route at all, so the package carries neither the -// surface nor the credential — no claim path, no exchange, no header, and no -// reader of the carrier that delivered another module's secret. -func TestNoModuleFacadeClaimOnTheSDKSurface(t *testing.T) { - forbidden := map[string]string{ - claimedFacadePath: "the gateway's module route-claim path", - facadeCredentialPath: "the exchange that mints a module route-claim credential", - facadeSecretHeader: "the header carrying a module's route-claim secret", - facadeCredentialHeader: "the header carrying a module route-claim credential", - facadeSecretsVariable: "the carrier delivering another module's route-claim secret", - } +// claimShapes are the shapes a module route claim is made of, normalised. A +// source carrying any of them carries the capability. +var claimShapes = map[string]string{ + claimedFacadePath: "the gateway's module route-claim path", + facadeCredentialPath: "the exchange that credentials a module route claim", + facadeSecretHeader: "the header carrying a module's route-claim secret", + facadeCredentialHeader: "the header carrying a module route-claim credential", + facadeSecretsVariable: "the carrier delivering another module's route-claim secret", +} +// TestNoModuleFacadeClaimOnTheSDKSurface is the static half, because one clean +// boot proves one boot. The invariant is about the capability: a solution +// built on this SDK must not be able to claim a module's facade route, so the +// package carries neither the surface nor the credential. +// +// It compares normalised string literals, not source text. A route or header +// is the same wire shape however it is spelled in Go — split across a +// concatenation, cased differently, assembled from its segments — so the +// comparison folds exactly the characters that spelling is free to move: +// literal boundaries, case, and the separators inside these names. +func TestNoModuleFacadeClaimOnTheSDKSurface(t *testing.T) { entries, err := os.ReadDir(".") if err != nil { t.Fatalf("read package directory: %v", err) } - scanned := 0 + + scanned, literals := 0, 0 for _, entry := range entries { name := entry.Name() - // Test sources are exempt: this file names every one of these on - // purpose, and a test that could not write them down could not assert - // their absence. + // Test sources are exempt: these shapes are written down here on + // purpose, which is what lets this assert their absence elsewhere. if filepath.Ext(name) != ".go" || strings.HasSuffix(name, "_test.go") { continue } @@ -170,14 +264,87 @@ func TestNoModuleFacadeClaimOnTheSDKSurface(t *testing.T) { t.Fatalf("read %s: %v", name, err) } scanned++ - for literal, what := range forbidden { - if strings.Contains(string(source), literal) { - t.Errorf("%s names %q (%s): a solution holds no credential deciding where another module's traffic is routed, and claims no module's facade route", - name, literal, what) + runs := stringLiteralRuns(t, name, source) + literals += len(runs) + for _, run := range runs { + for shape, what := range claimShapes { + if strings.Contains(fold(run), fold(shape)) { + t.Errorf("%s builds the string %q, which is %s (%s): a solution claims no module's facade route and holds no credential that could", + name, run, what, shape) + } + } + } + } + if scanned == 0 || literals == 0 { + t.Fatalf("scanned %d sources and %d string literals: this guard is inert", scanned, literals) + } +} + +// stringLiteralRuns is every string a file builds out of literals: each +// literal on its own, and each run of them joined by `+` as the one string +// that concatenation produces. A shape split across a concatenation is +// therefore seen whole, while identifiers are never joined to anything — an +// adjacency between unrelated names must not read as a wire shape. +func stringLiteralRuns(t *testing.T, name string, source []byte) []string { + t.Helper() + + var s scanner.Scanner + fset := token.NewFileSet() + s.Init(fset.AddFile(name, fset.Base(), len(source)), source, func(pos token.Position, msg string) { + t.Fatalf("scan %s: %s: %s", name, pos, msg) + }, 0) + + var runs []string + var current strings.Builder + // joining is true between a string literal and the `+` that may extend it, + // so only `STRING + STRING` keeps a run open. + joining := false + flush := func() { + if current.Len() > 0 { + runs = append(runs, current.String()) + current.Reset() + } + } + for { + _, tok, lit := s.Scan() + if tok == token.EOF { + break + } + switch { + case tok == token.STRING: + value, err := strconv.Unquote(lit) + if err != nil { + value = lit // a raw literal this unquoter dislikes is still worth scanning + } + if !joining { + flush() } + current.WriteString(value) + runs = append(runs, value) + joining = false + case tok == token.ADD && current.Len() > 0: + joining = true + default: + flush() + joining = false } } - if scanned == 0 { - t.Fatal("no package sources scanned: this guard is inert") + flush() + return runs +} + +// fold removes what spelling is free to change without changing the wire +// shape: case, and the separators these paths, headers and carriers are +// punctuated with. "/modules/_register" and "Modules.Register" fold alike, +// which is the point — the guard is about the shape, not the typing. +func fold(s string) string { + var b strings.Builder + for _, r := range strings.ToLower(s) { + switch r { + case '-', '_', '/', '.', ' ', '\t': + default: + b.WriteRune(r) + } } + return b.String() } diff --git a/passthrough.go b/passthrough.go index 6035b94..ceee499 100644 --- a/passthrough.go +++ b/passthrough.go @@ -279,11 +279,13 @@ func (s *Server) validatePassthrough() (map[string]passthroughRoute, error) { if err != nil || len(s.consumed) == 0 { return routes, err } - consumed, err := manifest.ParseConsumedAPIs(s.cfg.apiConsumes) - if err != nil { - return nil, fmt.Errorf("consumed modules cannot be checked against api.consumes: %w", err) + // Decoded at boot (loadConfig), and validate() has already refused a + // malformed one by name. Still reported here rather than assumed away: a + // caller that builds a config itself has not been through validate(). + if s.cfg.apiConsumesErr != nil { + return nil, fmt.Errorf("consumed modules cannot be checked against api.consumes: %w", s.cfg.apiConsumesErr) } - return routes, checkConsumed(s.consumed, consumed) + return routes, checkConsumed(s.consumed, s.cfg.consumedAPIs) } // mountPassthrough mounts the declared routes on mux at PassthroughPathPrefix, @@ -335,6 +337,9 @@ func (s *Server) PassthroughHandler(env PassthroughEnvironment) (http.Handler, e return nil, err } s.cfg.gatewayURL, s.cfg.apiConsumes = env.GatewayURL, string(projection) + // The caller handed us the decoded projection, so there is nothing to + // decode and nothing that could have failed to. + s.cfg.consumedAPIs, s.cfg.apiConsumesErr = env.Consumes, nil routes, err := s.validatePassthrough() if err != nil { return nil, fmt.Errorf("solution %q: %w", s.manifest.ID, err) diff --git a/projection_boundary_test.go b/projection_boundary_test.go new file mode 100644 index 0000000..7141084 --- /dev/null +++ b/projection_boundary_test.go @@ -0,0 +1,113 @@ +package solution + +import ( + "context" + "strings" + "testing" + + "github.com/codefly-dev/core/solution/manifest" +) + +// bootableEnvironment sets the carriers loadConfig needs to produce a config +// validate() accepts, so a test can change one value and see validate() judge +// that value rather than the first unresolved thing beside it. +func bootableEnvironment(t *testing.T) { + t.Helper() + t.Setenv("PORT", "8090") + t.Setenv("GATEWAY_URL", "http://gateway:42152") + t.Setenv("HOST_REGISTER_URL", "http://frontend:21931/api/solutions/register") + t.Setenv("SELF_UPSTREAM", "http://backend.svc.cluster.local:8080") + t.Setenv(SolutionRegistrationSecretEnvironmentVariable, "s3cret") + t.Setenv("CODEFLY__RUNTIME_CONTEXT", "") +} + +// TestProjectionIsRefusedBeforeServingWhateverIsDeclared proves the boot +// refuses a projection it cannot decode even when the solution declares no +// passthrough. +// +// The projection used to be parsed only by the code that consumed it, so the +// value reached no check at all on a solution that declared no Consumes: it +// booted, served, and was judged by nothing. validate() now decides it, which +// is what "resolved in loadConfig, checked in validate()" has to mean for a +// value to be covered by it. +func TestProjectionIsRefusedBeforeServingWhateverIsDeclared(t *testing.T) { + for _, tc := range []struct { + name string + raw string + }{ + {"truncated json", `[{"id":"a","module":"m"`}, + {"an object where a list belongs", `{"as":"documents"}`}, + {"a scalar", `5`}, + {"a field of the wrong type", `[{"as":5}]`}, + {"not json at all", `nope`}, + } { + t.Run(tc.name, func(t *testing.T) { + bootableEnvironment(t) + t.Setenv(manifest.APIConsumesEnvironmentVariable, tc.raw) + + cfg := loadConfig(context.Background(), "solution-under-test") + err := cfg.validate() + if err == nil { + t.Fatalf("validate() accepted the projection %q; a value nothing decodes is a value nothing refuses", tc.raw) + } + // The refusal has to name what to go and fix. The projection is + // the composition's output, not an operator's setting, so the + // variable carrying it is the only handle there is. + if !strings.Contains(err.Error(), manifest.APIConsumesEnvironmentVariable) { + t.Errorf("refusal does not name %s: %v", manifest.APIConsumesEnvironmentVariable, err) + } + }) + } +} + +// And the inverse: a projection that decodes, or none at all, is not refused. +// A solution that consumes nothing is the common case and must be unaffected, +// and a render emitting whitespace for an unset value means "nothing", not +// "malformed" — refusing that would fail a boot over a newline. +func TestAUsableOrAbsentProjectionIsNotRefused(t *testing.T) { + for _, tc := range []struct { + name string + raw string + }{ + {"unset", ""}, + {"whitespace only", " \n "}, + {"an empty list", `[]`}, + {"json null", `null`}, + {"one target", consumesDocuments}, + {"several targets", `[{"id":"a.b","module":"a","service":"b","endpoint":"rest","protocol":"rest","as":"b"},` + + `{"id":"c.d","module":"c","service":"d","endpoint":"rest","protocol":"rest","as":"d"}]`}, + } { + t.Run(tc.name, func(t *testing.T) { + bootableEnvironment(t) + t.Setenv(manifest.APIConsumesEnvironmentVariable, tc.raw) + + cfg := loadConfig(context.Background(), "solution-under-test") + if cfg.apiConsumesErr != nil { + t.Fatalf("projection %q did not decode: %v", tc.raw, cfg.apiConsumesErr) + } + if err := cfg.validate(); err != nil { + t.Fatalf("validate() refused the usable projection %q: %v", tc.raw, err) + } + }) + } +} + +// TestADeclaredPassthroughStillChecksAgainstTheProjection proves the boot +// check that was already there is unchanged: a declared module absent from the +// projection is still refused, and the refusal still names it. +func TestADeclaredPassthroughStillChecksAgainstTheProjection(t *testing.T) { + bootableEnvironment(t) + t.Setenv(manifest.APIConsumesEnvironmentVariable, consumesDocuments) + + s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) + s.consumed = []ConsumedModule{{As: "nothing-consumes-this", ViewerBearer: true}} + s.cfg = loadConfig(context.Background(), s.manifest.ID) + + _, err := s.validatePassthrough() + if err == nil { + t.Fatal("validatePassthrough() accepted a module the projection does not list") + } + if !strings.Contains(err.Error(), "nothing-consumes-this") { + t.Errorf("refusal does not name the declared module: %v", err) + } +} diff --git a/solution.go b/solution.go index e4dbb68..34b50cf 100644 --- a/solution.go +++ b/solution.go @@ -269,6 +269,14 @@ type config struct { // apiConsumes is the api.consumes projection Codefly injected, which the // passthrough declaration is checked against before the boot listens. apiConsumes string + // consumedAPIs is that projection decoded, and apiConsumesErr why it could + // not be. Decoded here, at boot, rather than where it is used: it used to + // be parsed only on the paths that needed it, so a solution declaring no + // Consumes served on with a projection nothing had ever looked at. A + // malformed one is now refused by validate() whatever the solution + // declares. + consumedAPIs []manifest.ConsumedAPI + apiConsumesErr error // mcp says the solution declared an MCP surface (ServeMCP), which is what // makes the two values below load-bearing: validate() checks them only // then, so a solution that serves no MCP is unaffected by a host whose @@ -495,12 +503,29 @@ func loadConfig(ctx context.Context, id string) config { // in-cluster address in a deployment, where no public client could reach // it: that is what the declared configuration is for. The public MCP URL // needs no declaration at all, being derived from the two values above it. + cfg.consumedAPIs, cfg.apiConsumesErr = parseAPIConsumes(cfg.apiConsumes) cfg.mcpIssuerURL, cfg.mcpIssuerExplicit = resolveMCPIssuer(ctx, frontendURL) cfg.mcpPublicURL, cfg.mcpPublicExplicit = resolveMCPPublicURL(ctx, public, id) cfg.registrationInterval, cfg.registrationIntervalErr = registrationIntervalFromEnv() return cfg } +// parseAPIConsumes decodes the api.consumes projection Codefly injects. +// +// Whitespace alone is not a projection, and not a defect either: a render that +// emits a bare newline for a value this solution does not set means "consumes +// nothing", so it decodes to no entries rather than to a JSON error. Anything +// else that will not decode is a defect in what the composition projected, and +// the error is kept for validate() to refuse by name — the alternative, which +// is what used to happen, is that nothing looks at it and the solution serves +// with a federation it silently dropped. +func parseAPIConsumes(raw string) ([]manifest.ConsumedAPI, error) { + if strings.TrimSpace(raw) == "" { + return nil, nil + } + return manifest.ParseConsumedAPIs(raw) +} + // selfEndpointCarrierPrefix names the carrier Codefly injects with this // service's own endpoint as the rest of the deployment reaches it — its // in-cluster address, e.g. "http://backend..svc.cluster.local:8080". @@ -721,6 +746,15 @@ func (c config) validate() error { if c.registrationIntervalErr != nil { return c.registrationIntervalErr } + // Refused whether or not this solution declares a passthrough. The + // projection is only ever read by the code that consumes it, so before + // this a solution declaring no Consumes booted and served with a + // projection nothing had parsed — and the one place that did parse it + // logged a line and carried on. + if c.apiConsumesErr != nil { + return fmt.Errorf("the api.consumes projection in %s cannot be decoded: %w — it is projected by the composition (`codefly run solution`), so it is fixed there; a solution that consumes nothing leaves it unset", + manifest.APIConsumesEnvironmentVariable, c.apiConsumesErr) + } if c.solutionSecret == "" { // An empty secret has two causes that look identical here, and sending // the operator to provision a secret that is already provisioned is the From 017c4a598c017b750ff6306b184853d30ad5fb36 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 12:16:00 -0400 Subject: [PATCH 03/12] fix: judge the api.consumes projection at boot, and guard three boundaries by shape rather than by spelling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review findings R53-02 through R53-06, re-applied on top of #45. R53-06 first: #45 merged, so the facade-claim removal this branch carried is already on main and every merge conflict resolved to main. What follows is the work #45 did not do. R53-02. The projection is resolved in loadConfig and was decoded only by the code that consumes it, which returns early for a solution declaring no Consumes. One undecodable projection therefore had two answers — refused for a solution with a passthrough, accepted and served for one without — and README.md already claimed it was a refusal. loadConfig decodes it once now (parseAPIConsumes) and validate() refuses it either way, naming the variable it arrived in: it is the composition's output, so no override repairs it. Whitespace is unset, not malformed; refusing a render's bare newline would fail a boot over nothing. The test seam decodes what it is handed, so a consumer's test cannot accept what a boot refuses. R53-03. The previous guard matched source text over a timing window, so a claim spelled with parenthesised constant expressions and started 120ms late passed it. Both halves were wrong. It now evaluates the package's constant string expressions — literals, parentheses, concatenation, and names bound to any of those, to a fixpoint across files — and compares the folded value. Nothing is timed, so there is no deadline to lose to: a dormant declaration is as visible as a called one, which is the point of asking about the capability rather than about one boot. R53-04. The agreement test declared a module with no methods, which resolvePassthrough refuses before any projection is read — and that refusal contained the alias the assertion looked for, so removing the agreement check left it green. It uses a valid declaration now, asserts the projection-specific refusal, and carries the positive case. A control boots the fixture first, so a later refusal cannot be attributed to the projection when it belongs to something else; it caught a missing contract profile immediately. R53-05. The environment gate asked only whether a reader sits in loadConfig's call tree, which a helper can do while serving code calls it too — a serve-time call to the existing env helper left every gate green. A second gate now asks what is reachable from the serving surface without passing through loadConfig, and refuses a reader there whatever boot also does with it. Mutation-checked, each red before green: parenthesised constants, named constants built from named constants, a dormant claim and a parenthesised credential reader all turn the shape guard red; a serve-time call to env turns the post-boot gate red; removing the projection refusal turns all seven R53-02 cases red; removing checkConsumed turns the agreement test red. No exported surface change against main. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 23 +++- README.md | 13 ++ environment_boundary_test.go | 76 ++++++++++++ facade_claim_test.go | 227 +++++++++++++++++++++++++++++++++++ passthrough.go | 13 +- projection_boundary_test.go | 159 ++++++++++++++++++++++++ solution.go | 32 +++++ 7 files changed, 538 insertions(+), 5 deletions(-) create mode 100644 facade_claim_test.go create mode 100644 projection_boundary_test.go diff --git a/AGENTS.md b/AGENTS.md index 99214c6..9e6c62a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -168,7 +168,28 @@ embedding it. with a log line while the solution served on — and it went with the registrations. Every environment read now sits in `loadConfig`'s call tree, which `environment_boundary_test.go` pins: a new read outside it fails the - suite until this file describes the exception. + suite until this file describes the exception — and so does a read reachable + from the serving surface, because a helper boot shares with serving code sits + in that tree while running long after `validate()`. +- Resolving a value in `loadConfig` is half the rule; the other half is a + refusal in `validate()` that fires **whether or not the feature using it is + switched on**. The `api.consumes` projection was resolved at boot and decoded + only by the code that consumes it, which returns early for a solution + declaring no `Consumes` — so one undecodable projection had two answers: + refused for a solution with a passthrough, accepted and served for one + without. It is decoded now in `loadConfig` (`parseAPIConsumes`) and refused by + `validate()` either way, naming the variable it arrived in, since it is the + composition's output and no override repairs it. `projection_boundary_test.go` + pins that, including that unset and whitespace mean "consumes nothing" rather + than "malformed". +- A solution claims no module's facade route and holds no credential that would + let it: a route for a module is claimed by the module that serves it, under a + credential bound to that module (#51, SA-F-GWREGISTRY). The passthrough and + the gateway client *read* a consumed module at `/v1//*`; nothing here + decides that the prefix points there. `facade_claim_test.go` evaluates the + package's constant string expressions and fails on any that is one of those + wire shapes — folded, so a shape split across a concatenation, parenthesised, + built from named constants or cased differently is the same shape to it. - **Never sign, parse or verify a Work Context here.** There is one implementation, Core's; this runtime obtains a credential through the SDK's mint client, carries it as the string it travels as, and lets the far end diff --git a/README.md b/README.md index 696c703..b878337 100644 --- a/README.md +++ b/README.md @@ -752,6 +752,19 @@ than a log line. It used to be read at serve time: a malformed projection disabled the whole federation with one log line while the solution served on, so every consumed facade 404'd at the gateway and the solution looked healthy. +The refusal fires **whether or not the solution declares a passthrough**. Being +resolved at boot is not the same as being judged there, and for a release it +was only the first: the projection was decoded by the code that consumes it, +which returns early when there is no `Consumes` declaration, so the same +undecodable value was refused for one solution and served for another. It is +decoded once at boot now and refused either way, naming `CODEFLY__API_CONSUMES` +— the projection is the composition's output, so no override an operator can +set repairs it, and `codefly run solution` is where it is fixed. + +Unset is not malformed. A solution that consumes nothing leaves the variable +empty, and whitespace alone counts as empty, so a render emitting a bare +newline for it does not fail a boot. + ### Reading a Work-Context-authenticated module diff --git a/environment_boundary_test.go b/environment_boundary_test.go index f1be770..c5c64a2 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -43,6 +43,82 @@ func TestEnvironmentIsReadOnlyWhereAgentsFileSaysItIs(t *testing.T) { } } +// And a helper that boot resolution uses must not also be used after it. +// +// The gate above asks only whether an environment reader sits in loadConfig's +// call tree. A helper can sit there and be called from serving code as well, +// and then it passes: `env` is reachable from loadConfig, so a serve-time call +// to `env` reads the environment after validate() has had its say while every +// boot-reachability check stays green. Reachability says where a function may +// be called from, never where it is not. +// +// So this gate asks the other question — what is reachable from the serving +// surface, not passing through loadConfig — and refuses an environment reader +// there whatever boot also does with it. +func TestNoEnvironmentReadIsReachableAfterBoot(t *testing.T) { + pkg := parsePackage(t) + + // serve is the whole post-boot surface: Serve resolves the configuration + // and then calls it, so rooting here is rooting after validate(). The + // seam is a second entry point, used by a consumer's test in place of a + // boot. loadConfig is excluded as a node rather than as a root, so a + // helper it shares with serving code is judged by the serving path. + served := pkg.reachableFromExcluding([]string{"serve", "passthroughSeam"}, "loadConfig") + + var afterBoot []string + for _, fn := range pkg.environmentReaders() { + if served[fn] { + afterBoot = append(afterBoot, fn) + } + } + sort.Strings(afterBoot) + if len(afterBoot) > 0 { + t.Errorf("these environment readers are reachable from the serving surface without passing through loadConfig: %s\n"+ + "A value read after boot is never refused by validate(): it changes what a running solution does, with nothing judging it. "+ + "Resolve it in loadConfig and carry the resolved value, rather than reading the environment where it is used.", + strings.Join(afterBoot, ", ")) + } + + // The gate has to be able to see such a read at all, or its silence means + // nothing: `env` is the helper the mutation above would go through, and it + // must be a reader this analysis knows about. + if !pkg.reachableFrom("env")["env"] { + t.Fatal("the call graph has no env helper: this gate is inert") + } + readers := pkg.environmentReaders() + if len(readers) == 0 { + t.Fatal("no environment readers found at all: this gate is inert") + } +} + +// reachableFromExcluding is reachableFrom over several roots, never traversing +// `excluded`. Excluding a node rather than declining to root at it is what +// separates "boot reads this" from "serving reads this": a helper both use is +// reached by the serving path on its own. +func (p pkgFuncs) reachableFromExcluding(roots []string, excluded string) map[string]bool { + seen := map[string]bool{} + var queue []string + for _, root := range roots { + if root == excluded { + continue + } + seen[root] = true + queue = append(queue, root) + } + for len(queue) > 0 { + current := queue[0] + queue = queue[1:] + for _, callee := range p.calls[current] { + if callee == excluded || seen[callee] { + continue + } + seen[callee] = true + queue = append(queue, callee) + } + } + return seen +} + // And the read has to happen at boot, not merely be written there. // // Static reachability is not the claim AGENTS.md makes, and the difference was diff --git a/facade_claim_test.go b/facade_claim_test.go new file mode 100644 index 0000000..ec9c2fa --- /dev/null +++ b/facade_claim_test.go @@ -0,0 +1,227 @@ +package solution + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "testing" +) + +// --- A solution claims no module's facade route --- +// +// Two invariants (SA-F-GWREGISTRY): +// +// - a facade route for a module is claimed only by the module that serves +// it, under a credential bound to that module; +// - a solution holds no credential that decides where another module's +// traffic is routed. +// +// So the package carries neither a module route-claim surface nor a credential +// for one. That a given boot claims nothing is checked elsewhere, against a +// running server; what is checked here is that the capability is not present +// to be reached, which no single boot can show. +// +// The shapes are named by this test alone, because a test that cannot write a +// shape down cannot assert its absence. +var claimShapes = map[string]string{ + "/modules/_register": "the gateway's module route-claim path", + "/modules/_registration-token": "the exchange that credentials a module route claim", + "X-Codefly-Module-Secret": "the header carrying a module's route-claim secret", + "X-Codefly-Module-Registration": "the header carrying a module route-claim credential", + "CODEFLY__MODULE_REGISTRATION_SECRETS": "the carrier delivering another module's route-claim secret", +} + +// TestNoModuleFacadeClaimOnTheSDKSurface evaluates the package's constant +// string expressions and refuses any that is one of those shapes. +// +// It evaluates rather than matches text. A wire shape is the same shape +// however Go spells it — split across a concatenation, wrapped in parentheses, +// assembled from named constants declared elsewhere in the package, or any +// mixture — and a guard that compares source text is a guard against one +// spelling. Everything the compiler folds to a constant string is folded here +// the same way, then compared on the value. +// +// Folding also absorbs case and the separators inside these names, because +// those are free to move without changing what reaches the wire. +func TestNoModuleFacadeClaimOnTheSDKSurface(t *testing.T) { + pkg := parseConstantExpressions(t) + if len(pkg.values) == 0 { + t.Fatal("no constant strings evaluated: this guard is inert") + } + + var found []string + for _, got := range pkg.values { + for shape, what := range claimShapes { + if strings.Contains(foldShape(got.value), foldShape(shape)) { + found = append(found, got.where+": the expression evaluates to "+strconv.Quote(got.value)+ + ", which is "+what+" ("+shape+")") + } + } + } + sort.Strings(found) + for _, f := range found { + t.Errorf("%s\na solution claims no module's facade route and holds no credential that could: routing is decided by the module that serves the route, under a credential bound to it", f) + } +} + +// constantString is one evaluated constant string expression and where it is. +type constantString struct { + value string + where string +} + +// constantPackage is the package's constant strings: named, so an expression +// referring to one can be evaluated, and positional, so every expression that +// produces one can be reported where it is written. +type constantPackage struct { + named map[string]string + values []constantString +} + +// parseConstantExpressions evaluates every constant string expression in the +// package's non-test sources. +// +// Two passes, because a constant may be defined in terms of another and the +// files are read in directory order: the first resolves named constants to a +// fixpoint, the second evaluates every expression in the package against them. +func parseConstantExpressions(t *testing.T) constantPackage { + t.Helper() + + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("read package directory: %v", err) + } + fset := token.NewFileSet() + var files []*ast.File + for _, entry := range entries { + name := entry.Name() + // Test sources are exempt: the shapes are written down here on + // purpose, which is what lets this assert their absence elsewhere. + if filepath.Ext(name) != ".go" || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, name, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + files = append(files, file) + } + if len(files) == 0 { + t.Fatal("no package sources parsed: this guard is inert") + } + + pkg := constantPackage{named: map[string]string{}} + // Fixpoint: each round resolves the constants whose definitions became + // evaluable in the last one, so `const a = b + "x"` resolves whatever + // order the declarations appear in. + for round := 0; round < 10; round++ { + learned := 0 + for _, file := range files { + for _, decl := range file.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok || gen.Tok != token.CONST { + continue + } + for _, spec := range gen.Specs { + value, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + for i, name := range value.Names { + if i >= len(value.Values) { + continue + } + if _, known := pkg.named[name.Name]; known { + continue + } + if got, ok := pkg.evaluate(value.Values[i]); ok { + pkg.named[name.Name] = got + learned++ + } + } + } + } + } + if learned == 0 { + break + } + } + + // Every expression, not only the named ones: a claim is as likely to be + // built inline at its call site as bound to a constant first. + for _, file := range files { + for _, decl := range file.Decls { + ast.Inspect(decl, func(n ast.Node) bool { + expr, ok := n.(ast.Expr) + if !ok { + return true + } + if got, ok := pkg.evaluate(expr); ok && got != "" { + pkg.values = append(pkg.values, constantString{ + value: got, + where: fset.Position(expr.Pos()).String(), + }) + // The whole expression evaluated, so its parts add + // nothing but noise. + return false + } + return true + }) + } + } + return pkg +} + +// evaluate folds one expression to the constant string it produces, if it +// produces one: a literal, a parenthesised expression, a `+` of two such, or a +// name bound to one. Anything else — a call, a variable, a conversion — is not +// a constant and is reported as such rather than guessed at. +func (p constantPackage) evaluate(expr ast.Expr) (string, bool) { + switch e := expr.(type) { + case *ast.BasicLit: + if e.Kind != token.STRING { + return "", false + } + value, err := strconv.Unquote(e.Value) + if err != nil { + return "", false + } + return value, true + case *ast.ParenExpr: + return p.evaluate(e.X) + case *ast.BinaryExpr: + if e.Op != token.ADD { + return "", false + } + left, okL := p.evaluate(e.X) + right, okR := p.evaluate(e.Y) + if !okL || !okR { + return "", false + } + return left + right, true + case *ast.Ident: + value, ok := p.named[e.Name] + return value, ok + } + return "", false +} + +// foldShape removes what spelling may change without changing the wire shape: +// case, and the separators these paths, headers and carriers are punctuated +// with. +func foldShape(s string) string { + var b strings.Builder + for _, r := range strings.ToLower(s) { + switch r { + case '-', '_', '/', '.', ' ', '\t': + default: + b.WriteRune(r) + } + } + return b.String() +} diff --git a/passthrough.go b/passthrough.go index 354d8a1..b065dfd 100644 --- a/passthrough.go +++ b/passthrough.go @@ -299,11 +299,13 @@ func (s *Server) validatePassthrough() (map[string]passthroughRoute, error) { if err != nil || len(s.consumed) == 0 { return routes, err } - consumed, err := manifest.ParseConsumedAPIs(s.cfg.apiConsumes) - if err != nil { - return nil, fmt.Errorf("consumed modules cannot be checked against api.consumes: %w", err) + // Decoded at boot (loadConfig), where validate() has already refused an + // undecodable one by name. Reported here rather than assumed away: the + // seam below builds a config without going through validate(). + if s.cfg.apiConsumesErr != nil { + return nil, fmt.Errorf("consumed modules cannot be checked against api.consumes: %w", s.cfg.apiConsumesErr) } - return routes, checkConsumed(s.consumed, consumed) + return routes, checkConsumed(s.consumed, s.cfg.consumedAPIs) } // mountPassthrough mounts the declared routes on mux at PassthroughPathPrefix: @@ -363,6 +365,9 @@ func passthroughSeam(server any, gatewayURL, consumesJSON string) (http.Handler, return nil, fmt.Errorf("solution %q declares no consumed modules (Consumes)", s.manifest.ID) } s.cfg.gatewayURL, s.cfg.apiConsumes = gatewayURL, consumesJSON + // The seam supplies the projection directly instead of resolving it, so it + // decodes here what loadConfig decodes at a boot. + s.cfg.consumedAPIs, s.cfg.apiConsumesErr = parseAPIConsumes(consumesJSON) routes, err := s.validatePassthrough() if err != nil { return nil, fmt.Errorf("solution %q: %w", s.manifest.ID, err) diff --git a/projection_boundary_test.go b/projection_boundary_test.go new file mode 100644 index 0000000..e847375 --- /dev/null +++ b/projection_boundary_test.go @@ -0,0 +1,159 @@ +package solution + +import ( + "context" + "strings" + "testing" + + "github.com/codefly-dev/core/solution/manifest" +) + +// --- The api.consumes projection is judged at boot, whatever is declared --- +// +// The projection is resolved in loadConfig, and used to be decoded only by the +// code that consumes it. That code returns early for a solution declaring no +// passthrough, so one undecodable value had two answers: refused for a +// solution with a declaration, accepted and served for one without. Resolving +// a value at boot is not the same as judging it, and this is where the second +// half is pinned. + +// projectionOf is the projection a composition emits for one consumed target. +func projectionOf(as string) string { + return `[{"id":"things.` + as + `","module":"things","service":"` + as + + `","endpoint":"rest","protocol":"rest","as":"` + as + `"}]` +} + +// bootableEnvironment sets what loadConfig needs for validate() to get as far +// as the projection, so a test that changes the projection sees validate() +// judge the projection rather than the first unresolved value beside it. +func bootableEnvironment(t *testing.T) { + t.Helper() + t.Setenv("PORT", "8090") + t.Setenv("GATEWAY_URL", "https://gateway.cell.test:42152") + t.Setenv(CredentialMintURLEnvironmentVariable, "https://mint.cell.test/mint") + t.Setenv(ContractProfileEnvironmentVariable, "test") + t.Setenv("CODEFLY__RUNTIME_CONTEXT", "") +} + +// bootConfig is the config a boot resolves in that environment, asserted to be +// otherwise valid so each case below turns on its projection alone. +func bootConfig(t *testing.T, projection string) config { + t.Helper() + bootableEnvironment(t) + t.Setenv(manifest.APIConsumesEnvironmentVariable, projection) + return loadConfig(context.Background(), "solution-under-test", nil) +} + +// TestProjectionIsRefusedBeforeServingWhateverIsDeclared: a projection that +// will not decode fails the boot, with no passthrough declared — the case that +// used to reach no check at all. +func TestProjectionIsRefusedBeforeServingWhateverIsDeclared(t *testing.T) { + // A control first: the same environment with a good projection must boot, + // or every refusal below could be about something else entirely. + if err := bootConfig(t, projectionOf("things")).validate(); err != nil { + t.Fatalf("the test environment does not boot with a usable projection, so no refusal below can be attributed to the projection: %v", err) + } + + for _, tc := range []struct { + name string + raw string + }{ + {"truncated json", `[{"id":"a","module":"m"`}, + {"an object where a list belongs", `{"as":"things"}`}, + {"a scalar", `5`}, + {"a field of the wrong type", `[{"as":5}]`}, + {"not json at all", `nope`}, + {"a trailing comma", `[{"as":"things"},]`}, + {"two documents", `[] []`}, + } { + t.Run(tc.name, func(t *testing.T) { + err := bootConfig(t, tc.raw).validate() + if err == nil { + t.Fatalf("validate() accepted the projection %q: a value nothing decodes is a value nothing refuses", tc.raw) + } + // The refusal has to name what to go and fix. The projection is + // the composition's output, not an operator's setting, so the + // variable carrying it is the only handle there is. + if !strings.Contains(err.Error(), manifest.APIConsumesEnvironmentVariable) { + t.Errorf("refusal does not name %s: %v", manifest.APIConsumesEnvironmentVariable, err) + } + }) + } +} + +// And the inverse. A solution that consumes nothing is the common case and +// must be unaffected, and whitespace means "nothing" rather than "malformed" — +// refusing a render's bare newline would fail a boot over nothing. +func TestAUsableOrAbsentProjectionIsNotRefused(t *testing.T) { + for _, tc := range []struct { + name string + raw string + }{ + {"unset", ""}, + {"whitespace only", " \n "}, + {"an empty list", `[]`}, + {"json null", `null`}, + {"one target", projectionOf("things")}, + {"several targets", `[{"id":"a.b","module":"a","service":"b","endpoint":"rest","protocol":"rest","as":"b"},` + + `{"id":"c.d","module":"c","service":"d","endpoint":"rest","protocol":"rest","as":"d"}]`}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := bootConfig(t, tc.raw) + if cfg.apiConsumesErr != nil { + t.Fatalf("projection %q did not decode: %v", tc.raw, cfg.apiConsumesErr) + } + if err := cfg.validate(); err != nil { + t.Fatalf("validate() refused the usable projection %q: %v", tc.raw, err) + } + }) + } +} + +// TestADeclaredPassthroughIsCheckedAgainstTheProjection proves the agreement +// check itself, on a declaration that is otherwise valid. +// +// The earlier version of this test declared a module with no methods, which +// resolvePassthrough refuses before any projection is consulted — and that +// refusal happened to contain the alias the assertion looked for, so the test +// passed without the agreement check running at all. Removing the check left +// it green. Both directions are asserted here, from one declaration that +// differs only in whether the projection lists it. +func TestADeclaredPassthroughIsCheckedAgainstTheProjection(t *testing.T) { + declared := passthroughModule() // valid: an as, scopes, and a resolvable method + + t.Run("listed in the projection", func(t *testing.T) { + s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) + s.consumed = []ConsumedModule{declared} + s.cfg = bootConfig(t, projectionOf(declared.As)) + if _, err := s.validatePassthrough(); err != nil { + t.Fatalf("a declared module the projection lists was refused: %v", err) + } + }) + + t.Run("absent from the projection", func(t *testing.T) { + s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) + s.consumed = []ConsumedModule{declared} + s.cfg = bootConfig(t, projectionOf("something-else")) + _, err := s.validatePassthrough() + if err == nil { + t.Fatal("a declared module absent from the projection was accepted") + } + // Specifically the agreement refusal, not resolvePassthrough's: the + // declaration above is valid, so nothing earlier can refuse it, and + // the message has to be the one about api.consumes. + if !strings.Contains(err.Error(), "api.consumes") || !strings.Contains(err.Error(), declared.As) { + t.Errorf("refusal is not the projection-agreement one: %v", err) + } + }) +} + +// TestTheSeamJudgesItsOwnProjection: the test seam supplies the projection +// instead of resolving it, so it decodes there what loadConfig decodes at a +// boot — otherwise a consumer's test would accept what a boot refuses. +func TestTheSeamJudgesItsOwnProjection(t *testing.T) { + s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) + s.consumed = []ConsumedModule{passthroughModule()} + if _, err := passthroughSeam(s, "https://gateway.cell.test", `{"not":"a list"}`); err == nil { + t.Fatal("the seam accepted a projection a boot would refuse") + } +} diff --git a/solution.go b/solution.go index d747917..cc8b368 100644 --- a/solution.go +++ b/solution.go @@ -466,6 +466,14 @@ type config struct { // passthrough declaration and the published contract are both checked // against before the boot listens. apiConsumes string + // consumedAPIs is that projection decoded, and apiConsumesErr why it could + // not be. Decoded here rather than where it is used, because it used to be + // decoded only where it was used: the paths that read it return early for + // a solution declaring no passthrough, so one undecodable value had two + // answers — refused for a solution with a declaration, accepted and served + // for one without. validate() now judges it either way. + consumedAPIs []manifest.ConsumedAPI + apiConsumesErr error // mcp says the solution declared an MCP surface (ServeMCP), which is what // makes the two values below load-bearing: validate() checks them only // then, so a solution that serves no MCP is unaffected by a host whose @@ -622,6 +630,21 @@ func resolveGateway(ctx context.Context, module, gateway string) string { return hostAddress(ctx, module, gateway, "rest", "rest") } +// parseAPIConsumes decodes the api.consumes projection Codefly injects. +// +// Whitespace alone is not a projection, and not a defect either: a render that +// emits a bare newline for a value this solution does not set means "consumes +// nothing", so it decodes to no entries rather than to a JSON error. Refusing +// that would fail a boot over a newline. Anything else that will not decode is +// a defect in what the composition projected, and the error is kept for +// validate() to refuse by name. +func parseAPIConsumes(raw string) ([]manifest.ConsumedAPI, error) { + if strings.TrimSpace(raw) == "" { + return nil, nil + } + return manifest.ParseConsumedAPIs(raw) +} + // loadConfig resolves every address, port, and path through the Codefly SDK so // nothing is hardcoded. The host it plugs into is named by Codefly-convention // roles (overridable), and their concrete addresses are resolved from the SDK — @@ -723,6 +746,7 @@ func loadConfig(ctx context.Context, id string, environmentLoadErr error) config // in-cluster address in a deployment, where no public client could reach // it: that is what the declared configuration is for. The public MCP URL // needs no declaration at all, being derived from the two values above it. + cfg.consumedAPIs, cfg.apiConsumesErr = parseAPIConsumes(cfg.apiConsumes) cfg.mcpIssuerURL, cfg.mcpIssuerExplicit = resolveMCPIssuer(ctx, frontendURL) cfg.mcpPublicURL, cfg.mcpPublicExplicit = resolveMCPPublicURL(ctx, public, id) return cfg @@ -978,6 +1002,14 @@ func (c config) validate() error { return fmt.Errorf("unusable contract profile %q: %w — it is the Codefly environment's own name unless %s overrides it", c.profile, err, ContractProfileEnvironmentVariable) } + // Judged whether or not this solution declares a passthrough. The + // projection is the composition's output, so no override an operator can + // set repairs it, and the variable it arrived in is the only handle a + // refusal has. + if c.apiConsumesErr != nil { + return fmt.Errorf("the api.consumes projection in %s cannot be decoded: %w — it is projected by the composition (`codefly run solution`), so it is fixed there; a solution that consumes nothing leaves it unset", + manifest.APIConsumesEnvironmentVariable, c.apiConsumesErr) + } // The MCP surface's own configuration, checked here because that is where // boot configuration is checked. It is a no-op for a solution that declared // no MCP surface, which is what keeps a host whose issuer does not resolve From d01a0385d19cc84391a810c8215b1477d595c6e8 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 12:21:40 -0400 Subject: [PATCH 04/12] docs: say that the raw projection and its decoded form are set together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three in-package tests build a config with apiConsumes alone. They pass, because nothing they exercise reads the decoded form — but the next author to set the raw string by hand gets no entries and a refusal that reads as "the projection lists nothing". The two production writers both decode; say so at the field rather than leaving it to be rediscovered. Co-Authored-By: Claude Opus 5 (1M context) --- solution.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/solution.go b/solution.go index cc8b368..a135393 100644 --- a/solution.go +++ b/solution.go @@ -472,6 +472,11 @@ type config struct { // a solution declaring no passthrough, so one undecodable value had two // answers — refused for a solution with a declaration, accepted and served // for one without. validate() now judges it either way. + // + // Set these with parseAPIConsumes whenever apiConsumes above is set, never + // one without the other: the two production writers are loadConfig and the + // test seam, and a config built by hand with only the raw string decodes + // to no entries, which reads as "the projection lists nothing". consumedAPIs []manifest.ConsumedAPI apiConsumesErr error // mcp says the solution declared an MCP surface (ServeMCP), which is what From 3949ea2f591533f0deab04f7995b65ab1a2eb0a0 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 13:16:30 -0400 Subject: [PATCH 05/12] fix: take constants from the type checker, refuse an untraceable environment key, root reachability at every exported declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round r1's major. The finding is right and the fixture compiles: a hand-written constant evaluator is spelling with extra steps, and the expression it did not implement was a conversion. const key = "CODEFLY__MODULE_" + string(rune(82)) + "EGISTRATION_SECRETS" return env(key, "") The evaluator handled literals, parentheses, concatenation and names bound to those, and SKIPPED everything else — so this folded to the carrier's exact name while matching none of the fragments the gate compared. A skip is a silent pass. There is no list of expression forms to finish, which is why nothing is reimplemented now: the compiler has already folded every constant, so the value comes from types.Info.Types[expr].Value and the evaluator is deleted. Three changes, all in this PR: 1. The claim-shape gate reads compiler-folded constants. It loads the package through the same packages.Load the typed boundary gates use, so a fixture that does not type-check fails the load rather than quietly proving nothing. 2. An environment read whose key cannot be traced to constants is refused, not skipped. Traced, not demanded in place: this package names its keys as exported constants and passes them through a parameter (workloadPath) or a struct field (the admission table), which is good code and stays possible. A key that does not fold is followed to its sources — every call site of the enclosing function, the field values of the literal being ranged over — and each source must itself resolve. Anything bottoming out in a computed value is refused. Every constant reached this way is also inspected by the gate above, so there is no path to a forbidden key that is neither a constant this package declares nor a refusal here. 3. The post-boot reachability walk roots at every exported declaration, to a fixed point, not at two named functions. Two names is a list, and a list is a thing an author adds to without noticing: the fixture's exported callable was walked by nothing. This package is a library, so anything exported is callable by a consumer at any time. The escaping fixture is committed as a compiled negative case, with three more of the same shape, driven through the gate functions themselves. One correction to the finding, in its favour: `string(82)` is rejected by `go vet` (stringintconv), so that exact source could not land through this repository's gate. `string(rune(82))` is vet-clean and folds identically, so that is the form committed and mutated against — the weaker artefact would have made the fixture look caught by a check that is not this one. My own mutation check then found the same defect r2 found in the agreement test, in a second place: TestTheSeamJudgesItsOwnProjection asserted only that an error came back, and deleting the seam's decode left it green, because an undecoded projection is an empty one and the agreement check refuses instead. It now requires the decode refusal, with the accepting case beside it. Mutations, each red before green: the vet-clean escaping fixture turns the claim-shape gate, the post-boot gate and the silence control red; a key joined at run time turns the key gate red; deleting the seam's decode turns both seam tests red; removing the whitespace carve-out turns the positive-control test red, which is the production target r1 asked that test to have. Co-Authored-By: Claude Opus 5 (1M context) --- environment_boundary_test.go | 66 +++- facade_claim_test.go | 649 ++++++++++++++++++++++++++--------- projection_boundary_test.go | 23 +- 3 files changed, 561 insertions(+), 177 deletions(-) diff --git a/environment_boundary_test.go b/environment_boundary_test.go index c5c64a2..8470376 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -6,6 +6,7 @@ import ( "go/token" "os" "path/filepath" + "slices" "sort" "strings" "testing" @@ -52,18 +53,22 @@ func TestEnvironmentIsReadOnlyWhereAgentsFileSaysItIs(t *testing.T) { // boot-reachability check stays green. Reachability says where a function may // be called from, never where it is not. // -// So this gate asks the other question — what is reachable from the serving -// surface, not passing through loadConfig — and refuses an environment reader -// there whatever boot also does with it. +// ROOTED AT EVERY EXPORTED DECLARATION, not at two names. The first version of +// this gate rooted at `serve` and `passthroughSeam`, which is a list, and a +// list is a thing an author adds to without noticing: an exported function +// reading `env` is callable by any consumer and was walked by nothing. Every +// exported function and method is an entry point a consumer may call after +// boot, so every one of them is a root, to a fixed point. loadConfig is +// excluded as a node rather than as a root, so a helper boot shares with +// serving code is judged by the serving path. func TestNoEnvironmentReadIsReachableAfterBoot(t *testing.T) { pkg := parsePackage(t) - // serve is the whole post-boot surface: Serve resolves the configuration - // and then calls it, so rooting here is rooting after validate(). The - // seam is a second entry point, used by a consumer's test in place of a - // boot. loadConfig is excluded as a node rather than as a root, so a - // helper it shares with serving code is judged by the serving path. - served := pkg.reachableFromExcluding([]string{"serve", "passthroughSeam"}, "loadConfig") + roots := pkg.postBootRoots() + if len(roots) == 0 { + t.Fatal("no post-boot entry points found: this gate is inert") + } + served := pkg.reachableFromExcluding(roots, "loadConfig") var afterBoot []string for _, fn := range pkg.environmentReaders() { @@ -73,25 +78,52 @@ func TestNoEnvironmentReadIsReachableAfterBoot(t *testing.T) { } sort.Strings(afterBoot) if len(afterBoot) > 0 { - t.Errorf("these environment readers are reachable from the serving surface without passing through loadConfig: %s\n"+ + t.Errorf("these environment readers are reachable from the post-boot surface without passing through loadConfig: %s\n"+ "A value read after boot is never refused by validate(): it changes what a running solution does, with nothing judging it. "+ "Resolve it in loadConfig and carry the resolved value, rather than reading the environment where it is used.", strings.Join(afterBoot, ", ")) } // The gate has to be able to see such a read at all, or its silence means - // nothing: `env` is the helper the mutation above would go through, and it + // nothing: `env` is the helper a serve-time read would go through, and it // must be a reader this analysis knows about. - if !pkg.reachableFrom("env")["env"] { - t.Fatal("the call graph has no env helper: this gate is inert") + if !pkg.reads["env"] { + t.Fatal("env is not recorded as an environment reader: this gate is inert") } - readers := pkg.environmentReaders() - if len(readers) == 0 { + if len(pkg.environmentReaders()) == 0 { t.Fatal("no environment readers found at all: this gate is inert") } + // And Serve must be among the roots, or "every exported declaration" is + // not what is being walked. + if !slices.Contains(roots, "Serve") { + t.Fatalf("Serve is not a post-boot root, so the roots are not every exported declaration: %v", roots) + } +} + +// postBootRoots is every exported function and method in the package, plus +// `serve` — the unexported body Serve calls once configuration is resolved, +// which is where the post-boot surface actually begins. +// +// Exported-ness is the right test for an entry point here: this package is a +// library, so anything exported is callable by a consumer at any time, and a +// read behind one of them is a read validate() never saw. +func (p pkgFuncs) postBootRoots() []string { + roots := []string{"serve"} + for name := range p.calls { + if name != "" && ast.IsExported(name) { + roots = append(roots, name) + } + } + for name := range p.reads { + if name != "" && ast.IsExported(name) && !slices.Contains(roots, name) { + roots = append(roots, name) + } + } + sort.Strings(roots) + return roots } -// reachableFromExcluding is reachableFrom over several roots, never traversing +// reachableFromExcluding is reachability over several roots, never traversing // `excluded`. Excluding a node rather than declining to root at it is what // separates "boot reads this" from "serving reads this": a helper both use is // reached by the serving path on its own. @@ -99,7 +131,7 @@ func (p pkgFuncs) reachableFromExcluding(roots []string, excluded string) map[st seen := map[string]bool{} var queue []string for _, root := range roots { - if root == excluded { + if root == excluded || seen[root] { continue } seen[root] = true diff --git a/facade_claim_test.go b/facade_claim_test.go index ec9c2fa..22400f0 100644 --- a/facade_claim_test.go +++ b/facade_claim_test.go @@ -2,19 +2,20 @@ package solution import ( "go/ast" - "go/parser" - "go/token" - "os" + "go/constant" + "go/types" "path/filepath" "sort" "strconv" "strings" "testing" + + "golang.org/x/tools/go/packages" ) -// --- A solution claims no module's facade route --- +// --- A solution claims no module's facade route (SP-GW-04, SP-SOL-03) --- // -// Two invariants (SA-F-GWREGISTRY): +// Two invariants: // // - a facade route for a module is claimed only by the module that serves // it, under a credential bound to that module; @@ -22,12 +23,21 @@ import ( // traffic is routed. // // So the package carries neither a module route-claim surface nor a credential -// for one. That a given boot claims nothing is checked elsewhere, against a -// running server; what is checked here is that the capability is not present -// to be reached, which no single boot can show. +// for one, and no reader of the carrier such a credential arrives in. +// +// ON THE TYPE CHECKER, for the reason typed_boundary_test.go gives: a rule +// that reads syntax is a rule about names, and an author picks the names. The +// first version of this gate evaluated constants by hand — literals, +// parentheses, `+`, and names bound to those — and skipped what it could not +// evaluate. A skip is a silent pass, and this compiles to the forbidden key +// while matching none of its fragments: +// +// const key = "CODEFLY__MODULE_" + string(rune(82)) + "EGISTRATION_SECRETS" // -// The shapes are named by this test alone, because a test that cannot write a -// shape down cannot assert its absence. +// `string(rune(82))` is a constant conversion, which the hand evaluator did not +// implement. There is no list of expression forms to finish here: the compiler +// has already folded every constant, conversions and concatenation alike, so +// the value is taken from it. That fixture is a compiled negative case below. var claimShapes = map[string]string{ "/modules/_register": "the gateway's module route-claim path", "/modules/_registration-token": "the exchange that credentials a module route claim", @@ -36,192 +46,513 @@ var claimShapes = map[string]string{ "CODEFLY__MODULE_REGISTRATION_SECRETS": "the carrier delivering another module's route-claim secret", } -// TestNoModuleFacadeClaimOnTheSDKSurface evaluates the package's constant -// string expressions and refuses any that is one of those shapes. -// -// It evaluates rather than matches text. A wire shape is the same shape -// however Go spells it — split across a concatenation, wrapped in parentheses, -// assembled from named constants declared elsewhere in the package, or any -// mixture — and a guard that compares source text is a guard against one -// spelling. Everything the compiler folds to a constant string is folded here -// the same way, then compared on the value. -// -// Folding also absorbs case and the separators inside these names, because -// those are free to move without changing what reaches the wire. +// TestNoModuleFacadeClaimOnTheSDKSurface refuses any constant string in the +// package whose folded value is one of those shapes. func TestNoModuleFacadeClaimOnTheSDKSurface(t *testing.T) { - pkg := parseConstantExpressions(t) - if len(pkg.values) == 0 { - t.Fatal("no constant strings evaluated: this guard is inert") + for _, pkg := range gatePackages(t, nil) { + if findings := claimShapeFindings(t, pkg); len(findings) > 0 { + for _, finding := range findings { + t.Errorf("%s\na solution claims no module's facade route and holds no credential that could: a route for a module is claimed by the module that serves it, under a credential bound to that module", + finding) + } + } } +} - var found []string - for _, got := range pkg.values { - for shape, what := range claimShapes { - if strings.Contains(foldShape(got.value), foldShape(shape)) { - found = append(found, got.where+": the expression evaluates to "+strconv.Quote(got.value)+ - ", which is "+what+" ("+shape+")") - } +// claimShapeFindings is every constant expression in pkg whose folded value is +// a claim shape. It is the rule itself, so the negative fixtures below drive +// exactly what the gate drives. +func claimShapeFindings(t *testing.T, pkg *packages.Package) []string { + t.Helper() + + var findings []string + evaluated := 0 + for _, file := range pkg.Syntax { + name := filepath.Base(pkg.Fset.Position(file.Pos()).Filename) + // Test sources are exempt: the shapes are written down here on + // purpose, which is what lets this assert their absence elsewhere. + if strings.HasSuffix(name, "_test.go") { + continue } + ast.Inspect(file, func(n ast.Node) bool { + expr, ok := n.(ast.Expr) + if !ok { + return true + } + value, ok := foldedString(pkg, expr) + if !ok { + return true + } + evaluated++ + for shape, what := range claimShapes { + if strings.Contains(foldShape(value), foldShape(shape)) { + findings = append(findings, pkg.Fset.Position(expr.Pos()).String()+ + ": the constant "+strconv.Quote(value)+" is "+what+" ("+shape+")") + } + } + // The whole expression folded, so its parts add only noise. + return false + }) } - sort.Strings(found) - for _, f := range found { - t.Errorf("%s\na solution claims no module's facade route and holds no credential that could: routing is decided by the module that serves the route, under a credential bound to it", f) + if evaluated == 0 { + t.Fatalf("%s: no constant strings evaluated, so this gate is inert", pkg.PkgPath) } + sort.Strings(findings) + return findings } -// constantString is one evaluated constant string expression and where it is. -type constantString struct { - value string - where string +// foldedString is the constant string an expression evaluates to, as the +// COMPILER folded it. Nothing is reimplemented: `string(rune(82)) + "x"` and a name +// bound to it arrive here already folded, which is the whole reason the hand +// evaluator is gone. +func foldedString(pkg *packages.Package, expr ast.Expr) (string, bool) { + tv, ok := pkg.TypesInfo.Types[expr] + if !ok || tv.Value == nil || tv.Value.Kind() != constant.String { + return "", false + } + return constant.StringVal(tv.Value), true } -// constantPackage is the package's constant strings: named, so an expression -// referring to one can be evaluated, and positional, so every expression that -// produces one can be reported where it is written. -type constantPackage struct { - named map[string]string - values []constantString +// foldShape removes what spelling may change without changing the wire shape: +// case, and the separators these paths, headers and carriers are punctuated +// with. +func foldShape(s string) string { + var b strings.Builder + for _, r := range strings.ToLower(s) { + switch r { + case '-', '_', '/', '.', ' ', '\t': + default: + b.WriteRune(r) + } + } + return b.String() } -// parseConstantExpressions evaluates every constant string expression in the -// package's non-test sources. +// --- An environment read is refused unless its key is a resolved constant --- + +// environmentReadFunctions are the ways this package reads the process +// environment. `env` is the package's own helper, which is what a reader +// reaches for: the gate below is about the KEY each one is given. +var environmentReadFunctions = map[string]bool{"Getenv": true, "LookupEnv": true, "env": true} + +// TestEveryEnvironmentKeyIsAResolvedConstant refuses an environment read whose +// key cannot be traced to constants. // -// Two passes, because a constant may be defined in terms of another and the -// files are read in directory order: the first resolves named constants to a -// fixpoint, the second evaluates every expression in the package against them. -func parseConstantExpressions(t *testing.T) constantPackage { +// An unresolved key is a REFUSAL rather than a skip, which is the difference +// between a gate and a suggestion. The gate above judges constants; a key +// computed at run time — joined from a slice, formatted, concatenated with a +// variable — is a key no static rule can judge at all, and letting it through +// is how the one shape that matters arrives. +// +// Traced, not demanded in place. This package names its keys as exported +// constants and passes them to `env` through a parameter or a struct field, +// which is good code and must stay possible: `workloadPath(ctx, override, key)` +// takes the variable name as an argument, and the admission table ranges over a +// literal of them. So a key that does not fold is followed to its sources — the +// arguments at every call site of the enclosing function, the field values of +// the literal being ranged over — and each source must itself resolve. Anything +// that bottoms out in something other than a constant is refused. +// +// The point of tracing rather than relaxing: every constant reached this way is +// also inspected by the gate above, so there is no path to a forbidden key that +// is neither a constant this package declares nor a refusal here. +func TestEveryEnvironmentKeyIsAResolvedConstant(t *testing.T) { + for _, pkg := range gatePackages(t, nil) { + if findings := unresolvedKeyFindings(t, pkg); len(findings) > 0 { + for _, finding := range findings { + t.Errorf("%s\nan environment key that cannot be traced to constants cannot be judged by any static gate, so it is refused here rather than skipped: name the key as a constant, or pass one in", + finding) + } + } + } +} + +func unresolvedKeyFindings(t *testing.T, pkg *packages.Package) []string { t.Helper() - entries, err := os.ReadDir(".") - if err != nil { - t.Fatalf("read package directory: %v", err) - } - fset := token.NewFileSet() - var files []*ast.File - for _, entry := range entries { - name := entry.Name() - // Test sources are exempt: the shapes are written down here on - // purpose, which is what lets this assert their absence elsewhere. - if filepath.Ext(name) != ".go" || strings.HasSuffix(name, "_test.go") { + tracer := newKeyTracer(pkg) + var findings []string + for _, file := range pkg.Syntax { + name := filepath.Base(pkg.Fset.Position(file.Pos()).Filename) + if strings.HasSuffix(name, "_test.go") { continue } - file, err := parser.ParseFile(fset, name, nil, 0) - if err != nil { - t.Fatalf("parse %s: %v", name, err) + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || len(call.Args) == 0 || !readsEnvironmentCall(call) { + return true + } + // env's own body reads os.Getenv(key) on its parameter. It is the + // helper every traced key arrives through, so judging it here + // would be judging the mechanism rather than any key; its callers + // are what this gate follows. + if tracer.insideEnvHelper(call) { + return true + } + if why, ok := tracer.resolves(call.Args[0], 0); !ok { + findings = append(findings, pkg.Fset.Position(call.Pos()).String()+ + ": the key of this environment read does not resolve to constants ("+why+")") + } + return true + }) + } + sort.Strings(findings) + return findings +} + +// keyTracer follows an environment key back to the constants that reach it. +type keyTracer struct { + pkg *packages.Package + // decls is every function in the package by its object, so a parameter can + // be followed to the call sites that supply it. + decls map[types.Object]*ast.FuncDecl + // envHelper is the package's own env helper, whose body is exempt. + envHelper *ast.FuncDecl +} + +func newKeyTracer(pkg *packages.Package) *keyTracer { + tracer := &keyTracer{pkg: pkg, decls: map[types.Object]*ast.FuncDecl{}} + for _, file := range pkg.Syntax { + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Name == nil { + continue + } + if obj := pkg.TypesInfo.Defs[fn.Name]; obj != nil { + tracer.decls[obj] = fn + } + if fn.Name.Name == "env" && fn.Recv == nil { + tracer.envHelper = fn + } } - files = append(files, file) } - if len(files) == 0 { - t.Fatal("no package sources parsed: this guard is inert") + return tracer +} + +// insideEnvHelper reports whether a call sits in the env helper's own body. +func (k *keyTracer) insideEnvHelper(call *ast.CallExpr) bool { + if k.envHelper == nil || k.envHelper.Body == nil { + return false } + return call.Pos() >= k.envHelper.Body.Pos() && call.End() <= k.envHelper.Body.End() +} - pkg := constantPackage{named: map[string]string{}} - // Fixpoint: each round resolves the constants whose definitions became - // evaluable in the last one, so `const a = b + "x"` resolves whatever - // order the declarations appear in. - for round := 0; round < 10; round++ { - learned := 0 - for _, file := range files { - for _, decl := range file.Decls { - gen, ok := decl.(*ast.GenDecl) - if !ok || gen.Tok != token.CONST { - continue - } - for _, spec := range gen.Specs { - value, ok := spec.(*ast.ValueSpec) - if !ok { - continue - } - for i, name := range value.Names { - if i >= len(value.Values) { - continue - } - if _, known := pkg.named[name.Name]; known { - continue - } - if got, ok := pkg.evaluate(value.Values[i]); ok { - pkg.named[name.Name] = got - learned++ - } - } - } - } +// maxKeyTraceDepth bounds the walk. Nothing here needs more than a couple of +// hops, and a bound is what keeps a cycle from hanging the suite. +const maxKeyTraceDepth = 8 + +// resolves reports whether every value that can reach expr is a constant, +// and if not, what stopped the trace. +func (k *keyTracer) resolves(expr ast.Expr, depth int) (string, bool) { + if depth > maxKeyTraceDepth { + return "the trace is deeper than " + strconv.Itoa(maxKeyTraceDepth) + " hops", false + } + if _, folded := foldedString(k.pkg, expr); folded { + return "", true + } + switch e := expr.(type) { + case *ast.ParenExpr: + return k.resolves(e.X, depth+1) + case *ast.Ident: + return k.resolvesIdent(e, depth) + case *ast.SelectorExpr: + return k.resolvesField(e, depth) + } + return "it is computed here rather than named", false +} + +// resolvesIdent follows a name: a constant resolves, and a parameter resolves +// when every call site supplies something that resolves. +func (k *keyTracer) resolvesIdent(ident *ast.Ident, depth int) (string, bool) { + obj := k.pkg.TypesInfo.Uses[ident] + if obj == nil { + return "its declaration could not be resolved", false + } + if _, isConst := obj.(*types.Const); isConst { + return "", true + } + fn, index, ok := k.parameterOf(obj) + if !ok { + return "it is a variable, not a constant or a parameter carrying one", false + } + sites := k.callSites(fn) + if len(sites) == 0 { + // A parameter nothing supplies cannot be judged, and an unjudged key + // is the thing this gate exists to refuse. + return "it is a parameter of " + fn.Name.Name + ", which nothing in this package calls", false + } + for _, site := range sites { + if index >= len(site.Args) { + return "a call to " + fn.Name.Name + " passes too few arguments to judge", false } - if learned == 0 { - break + if why, ok := k.resolves(site.Args[index], depth+1); !ok { + return "it is a parameter of " + fn.Name.Name + ", and a caller supplies a value that " + why, false } } + return "", true +} - // Every expression, not only the named ones: a claim is as likely to be - // built inline at its call site as bound to a constant first. - for _, file := range files { - for _, decl := range file.Decls { - ast.Inspect(decl, func(n ast.Node) bool { - expr, ok := n.(ast.Expr) - if !ok { - return true - } - if got, ok := pkg.evaluate(expr); ok && got != "" { - pkg.values = append(pkg.values, constantString{ - value: got, - where: fset.Position(expr.Pos()).String(), - }) - // The whole expression evaluated, so its parts add - // nothing but noise. - return false - } - return true - }) +// resolvesField follows a field read, for the shape the admission table uses: +// ranging over a composite literal of constants. +func (k *keyTracer) resolvesField(sel *ast.SelectorExpr, depth int) (string, bool) { + base, ok := sel.X.(*ast.Ident) + if !ok { + return "it is a field of something this gate cannot follow", false + } + obj := k.pkg.TypesInfo.Uses[base] + if obj == nil { + if def := k.pkg.TypesInfo.Defs[base]; def != nil { + obj = def + } + } + if obj == nil { + return "the value it is a field of could not be resolved", false + } + literals := k.rangedLiterals(obj) + if len(literals) == 0 { + return "it is a field of a value this gate cannot follow to a literal", false + } + for _, lit := range literals { + for _, element := range lit.Elts { + item, ok := element.(*ast.CompositeLit) + if !ok { + return "an element of the ranged literal is not itself a literal", false + } + value, ok := k.fieldValue(item, sel.Sel.Name) + if !ok { + return "an element of the ranged literal does not set " + sel.Sel.Name, false + } + if why, ok := k.resolves(value, depth+1); !ok { + return "a literal element sets " + sel.Sel.Name + " to a value that " + why, false + } } } - return pkg + return "", true } -// evaluate folds one expression to the constant string it produces, if it -// produces one: a literal, a parenthesised expression, a `+` of two such, or a -// name bound to one. Anything else — a call, a variable, a conversion — is not -// a constant and is reported as such rather than guessed at. -func (p constantPackage) evaluate(expr ast.Expr) (string, bool) { - switch e := expr.(type) { - case *ast.BasicLit: - if e.Kind != token.STRING { - return "", false +// parameterOf reports the function a parameter belongs to and its position. +func (k *keyTracer) parameterOf(obj types.Object) (*ast.FuncDecl, int, bool) { + for fnObj, fn := range k.decls { + signature, ok := fnObj.Type().(*types.Signature) + if !ok { + continue } - value, err := strconv.Unquote(e.Value) - if err != nil { - return "", false + params := signature.Params() + for i := 0; i < params.Len(); i++ { + if params.At(i) == obj { + return fn, i, true + } } - return value, true - case *ast.ParenExpr: - return p.evaluate(e.X) - case *ast.BinaryExpr: - if e.Op != token.ADD { - return "", false + } + return nil, 0, false +} + +// callSites is every call to fn in this package's non-test sources. +func (k *keyTracer) callSites(fn *ast.FuncDecl) []*ast.CallExpr { + target := k.pkg.TypesInfo.Defs[fn.Name] + var sites []*ast.CallExpr + for _, file := range k.pkg.Syntax { + if strings.HasSuffix(filepath.Base(k.pkg.Fset.Position(file.Pos()).Filename), "_test.go") { + continue } - left, okL := p.evaluate(e.X) - right, okR := p.evaluate(e.Y) - if !okL || !okR { - return "", false + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + var name *ast.Ident + switch callee := call.Fun.(type) { + case *ast.Ident: + name = callee + case *ast.SelectorExpr: + name = callee.Sel + default: + return true + } + if k.pkg.TypesInfo.Uses[name] == target { + sites = append(sites, call) + } + return true + }) + } + return sites +} + +// rangedLiterals is every composite literal that obj ranges over. +func (k *keyTracer) rangedLiterals(obj types.Object) []*ast.CompositeLit { + var literals []*ast.CompositeLit + for _, file := range k.pkg.Syntax { + ast.Inspect(file, func(n ast.Node) bool { + stmt, ok := n.(*ast.RangeStmt) + if !ok { + return true + } + value, ok := stmt.Value.(*ast.Ident) + if !ok { + return true + } + if k.pkg.TypesInfo.Defs[value] != obj && k.pkg.TypesInfo.Uses[value] != obj { + return true + } + if lit, ok := stmt.X.(*ast.CompositeLit); ok { + literals = append(literals, lit) + } + return true + }) + } + return literals +} + +// fieldValue is the value a composite literal gives one named field, keyed or +// positional. +// +// Positional is resolved through the type checker rather than the literal's +// own syntax: the admission table is a slice of an anonymous struct, so its +// elements carry no type of their own to count fields in. +func (k *keyTracer) fieldValue(lit *ast.CompositeLit, field string) (ast.Expr, bool) { + for _, element := range lit.Elts { + if kv, ok := element.(*ast.KeyValueExpr); ok { + if key, ok := kv.Key.(*ast.Ident); ok && key.Name == field { + return kv.Value, true + } } - return left + right, true + } + if len(lit.Elts) == 0 { + return nil, false + } + if _, keyed := lit.Elts[0].(*ast.KeyValueExpr); keyed { + return nil, false + } + structType, ok := structTypeOf(k.pkg.TypesInfo.TypeOf(lit)) + if !ok { + return nil, false + } + for i := 0; i < structType.NumFields() && i < len(lit.Elts); i++ { + if structType.Field(i).Name() == field { + return lit.Elts[i], true + } + } + return nil, false +} + +// structTypeOf unwraps a type to the struct underneath it, through pointers +// and named types alike. +func structTypeOf(t types.Type) (*types.Struct, bool) { + if t == nil { + return nil, false + } + t = types.Unalias(t) + if pointer, ok := t.(*types.Pointer); ok { + t = types.Unalias(pointer.Elem()) + } + structType, ok := t.Underlying().(*types.Struct) + return structType, ok +} + +// readsEnvironmentCall reports whether a call reads the process environment, +// by the callee's name — os.Getenv, os.LookupEnv, or this package's own env. +func readsEnvironmentCall(call *ast.CallExpr) bool { + switch fn := call.Fun.(type) { case *ast.Ident: - value, ok := p.named[e.Name] - return value, ok + return environmentReadFunctions[fn.Name] + case *ast.SelectorExpr: + return environmentReadFunctions[fn.Sel.Name] } - return "", false + return false } -// foldShape removes what spelling may change without changing the wire shape: -// case, and the separators these paths, headers and carriers are punctuated -// with. -func foldShape(s string) string { - var b strings.Builder - for _, r := range strings.ToLower(s) { - switch r { - case '-', '_', '/', '.', ' ', '\t': - default: - b.WriteRune(r) +// --- The fixture that escaped, as a compiled negative case --- + +// TestTheFacadeGatesCatchTheEscapingFixture drives both gates against the +// source that defeated the hand evaluator, compiled into this very package. +// +// A fixture that only parsed would prove nothing — it is the compiler's +// folding of `string(rune(82))` that makes the key real, and a parse-only fixture +// never folds anything. gatePackages fails the load if it does not type-check, +// so this cannot quietly become a fixture about nothing. +func TestTheFacadeGatesCatchTheEscapingFixture(t *testing.T) { + dir, err := filepath.Abs(".") + if err != nil { + t.Fatal(err) + } + fixture := filepath.Join(dir, "zz_facade_claim_fixture.go") + + for _, tc := range []struct { + name string + source string + // want is a string the finding must contain, so a gate that fires for + // an unrelated reason does not count as catching this. + want string + rule func(*testing.T, *packages.Package) []string + }{ + { + name: "a constant conversion splicing the carrier's name", + // The review's fixture, verbatim in substance: the compiler folds + // this to the carrier, while no fragment of it matches. + source: `package solution + +func FacadeCredentialViaConversion() string { + const key = "CODEFLY__MODULE_" + string(rune(82)) + "EGISTRATION_SECRETS" + return env(key, "") +} +`, + want: "CODEFLY__MODULE_REGISTRATION_SECRETS", + rule: claimShapeFindings, + }, + { + name: "a claim path spliced the same way", + source: `package solution + +const facadeClaimPath = "/modules/" + string(rune(95)) + "register" + +func FacadeClaimPath() string { return facadeClaimPath } +`, + want: "/modules/_register", + rule: claimShapeFindings, + }, + { + name: "a header assembled through a conversion", + source: `package solution + +func FacadeSecretHeader() string { + return "X-Codefly-Module-" + string(rune(83)) + "ecret" +} +`, + want: "X-Codefly-Module-Secret", + rule: claimShapeFindings, + }, + { + name: "an environment key that is not constant at all", + source: `package solution + +import "strings" + +func FacadeCredentialViaRuntimeKey(parts []string) string { + return env(strings.Join(parts, "_"), "") +} +`, + want: "does not resolve to constants", + rule: unresolvedKeyFindings, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := gateFindings(t, fixture, tc.source, tc.rule) + if !strings.Contains(got, tc.want) { + t.Errorf("the gate did not catch this fixture.\nfindings:\n%s\nwant one containing %q", got, tc.want) + } + }) + } +} + +// And the gates must not fire on the package as it is, or the fixtures above +// prove nothing: a gate that fires on everything catches every fixture. +func TestTheFacadeGatesAreSilentOnThisPackage(t *testing.T) { + for _, pkg := range gatePackages(t, nil) { + if findings := claimShapeFindings(t, pkg); len(findings) > 0 { + t.Errorf("%s: the claim-shape gate fires on the package as it is: %s", pkg.PkgPath, strings.Join(findings, "; ")) + } + if findings := unresolvedKeyFindings(t, pkg); len(findings) > 0 { + t.Errorf("%s: the key gate fires on the package as it is: %s", pkg.PkgPath, strings.Join(findings, "; ")) } } - return b.String() } diff --git a/projection_boundary_test.go b/projection_boundary_test.go index e847375..534071c 100644 --- a/projection_boundary_test.go +++ b/projection_boundary_test.go @@ -150,10 +150,31 @@ func TestADeclaredPassthroughIsCheckedAgainstTheProjection(t *testing.T) { // TestTheSeamJudgesItsOwnProjection: the test seam supplies the projection // instead of resolving it, so it decodes there what loadConfig decodes at a // boot — otherwise a consumer's test would accept what a boot refuses. +// +// The refusal must be the DECODE refusal. Asserting only that an error came +// back proved nothing: with the seam's decode removed, the projection is +// simply empty, the declared module is then absent from it, and the agreement +// check refuses — a different refusal, for a different reason, that this test +// happily counted as success. A mutation that deleted the decode left it +// green, which is the defect it now fails on. func TestTheSeamJudgesItsOwnProjection(t *testing.T) { s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) s.consumed = []ConsumedModule{passthroughModule()} - if _, err := passthroughSeam(s, "https://gateway.cell.test", `{"not":"a list"}`); err == nil { + _, err := passthroughSeam(s, "https://gateway.cell.test", `{"not":"a list"}`) + if err == nil { t.Fatal("the seam accepted a projection a boot would refuse") } + if !strings.Contains(err.Error(), "cannot be checked against api.consumes") { + t.Errorf("the seam refused for some other reason than the projection failing to decode: %v", err) + } +} + +// And the seam accepts the projection it should: the same declaration, listed. +// Without this, the test above is satisfied by a seam that refuses everything. +func TestTheSeamAcceptsAProjectionThatListsTheDeclaration(t *testing.T) { + s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) + s.consumed = []ConsumedModule{passthroughModule()} + if _, err := passthroughSeam(s, "https://gateway.cell.test", projectionOf(passthroughModule().As)); err != nil { + t.Fatalf("the seam refused a projection that lists the declared module: %v", err) + } } From e4d4a807f55d51e2ce5075ba2215510449c1af12 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 13:48:04 -0400 Subject: [PATCH 06/12] fix: the post-boot environment gate resolves served callbacks by type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit E2 and E3 from the executed round. The gate walked named call expressions from two roots, so it saw neither. - `(env)(...)` is a call to env that no callee-NAME walker sees: the callee is a parenthesised expression. Callees are unwrapped and resolved through types.Info now. - A handler mounted on the mux is never called anywhere. handleHealth is passed to HandleFunc as a value and invoked by net/http per request, so a graph built from call expressions holds no edge to it — and a read inside it happens on every request. A function mentioned as a VALUE is an edge now: if serving code can hand it to something, something can call it. A call through a func-typed field, variable or parameter is RESOLVED rather than exempted: it can only land on a function of that exact signature whose value somebody took, so every such function becomes an edge. An empty candidate set is a resolved answer, not an unknown one — no function of this package with that signature has its value taken, so the call provably cannot land on one. That is what cancel from context.WithCancel is. Only a callee the type checker cannot type at all is refused, which is the fail-closed case and the only one. Roots are serve plus every exported declaration, to a fixed point. Inertness is asserted on all three mechanisms, including that handleHealth is reached, which is the edge kind the previous version did not have. Mutations: E2, E3, and a callback-field variant reaching env through a func-typed struct field, all red. Co-Authored-By: Claude Opus 5 (1M context) --- environment_boundary_test.go | 320 +++++++++++++++++++++++++++-------- 1 file changed, 253 insertions(+), 67 deletions(-) diff --git a/environment_boundary_test.go b/environment_boundary_test.go index 8470376..5720e75 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -4,12 +4,14 @@ import ( "go/ast" "go/parser" "go/token" + "go/types" "os" "path/filepath" - "slices" "sort" "strings" "testing" + + "golang.org/x/tools/go/packages" ) // AGENTS.md tells an agent that configuration is resolved in one place and @@ -50,105 +52,289 @@ func TestEnvironmentIsReadOnlyWhereAgentsFileSaysItIs(t *testing.T) { // call tree. A helper can sit there and be called from serving code as well, // and then it passes: `env` is reachable from loadConfig, so a serve-time call // to `env` reads the environment after validate() has had its say while every -// boot-reachability check stays green. Reachability says where a function may -// be called from, never where it is not. +// boot-reachability check stays green. +// +// ON THE TYPE CHECKER, and over function VALUES, because the two things this +// gate must follow are exactly the two a name-matching graph loses: +// +// - `(env)(...)` is a call to env that no callee-NAME walker sees, because +// the callee is a parenthesised expression rather than an identifier; +// - a handler mounted on the mux is never "called" anywhere. `handleHealth` +// is passed to HandleFunc as a value and invoked by net/http per request, +// so a graph built from call expressions contains no edge to it at all — +// and a read inside it happens on every request, which is as far from boot +// as a read gets. // -// ROOTED AT EVERY EXPORTED DECLARATION, not at two names. The first version of -// this gate rooted at `serve` and `passthroughSeam`, which is a list, and a -// list is a thing an author adds to without noticing: an exported function -// reading `env` is callable by any consumer and was walked by nothing. Every -// exported function and method is an entry point a consumer may call after -// boot, so every one of them is a root, to a fixed point. loadConfig is -// excluded as a node rather than as a root, so a helper boot shares with -// serving code is judged by the serving path. +// So calls are resolved through types.Info, and a function mentioned as a +// VALUE is an edge too: if serving code can hand it to something, something +// can call it. Roots are `serve` plus every exported declaration, to a fixed +// point — a library's exported surface is callable by a consumer at any time, +// and a list of two names was a list an author adds to without noticing. func TestNoEnvironmentReadIsReachableAfterBoot(t *testing.T) { - pkg := parsePackage(t) + for _, pkg := range gatePackages(t, nil) { + if pkg.PkgPath != "github.com/codefly-dev/solution-runtime-go" { + continue + } + graph := newCallGraph(pkg) + reached, unresolved := graph.servingSurface() - roots := pkg.postBootRoots() - if len(roots) == 0 { - t.Fatal("no post-boot entry points found: this gate is inert") - } - served := pkg.reachableFromExcluding(roots, "loadConfig") + // Fail closed: a serving edge this gate could not resolve is a part of + // the surface it did not walk, and silence about it would be the same + // silent pass the name-matching graph gave. + for _, edge := range unresolved { + t.Errorf("%s: a call on the serving surface goes through a value this gate cannot resolve, so what it reaches was never walked. Call the function directly, or give it a type this gate can follow.", edge) + } - var afterBoot []string - for _, fn := range pkg.environmentReaders() { - if served[fn] { - afterBoot = append(afterBoot, fn) + var afterBoot []string + for name := range reached { + if graph.readsEnvironment[name] { + afterBoot = append(afterBoot, name) + } + } + sort.Strings(afterBoot) + if len(afterBoot) > 0 { + t.Errorf("these environment readers are reachable from the post-boot surface without passing through loadConfig: %s\n"+ + "A value read after boot is never refused by validate(): it changes what a running solution does, with nothing judging it. "+ + "Resolve it in loadConfig and carry the resolved value, rather than reading the environment where it is used.", + strings.Join(afterBoot, ", ")) } - } - sort.Strings(afterBoot) - if len(afterBoot) > 0 { - t.Errorf("these environment readers are reachable from the post-boot surface without passing through loadConfig: %s\n"+ - "A value read after boot is never refused by validate(): it changes what a running solution does, with nothing judging it. "+ - "Resolve it in loadConfig and carry the resolved value, rather than reading the environment where it is used.", - strings.Join(afterBoot, ", ")) - } - // The gate has to be able to see such a read at all, or its silence means - // nothing: `env` is the helper a serve-time read would go through, and it - // must be a reader this analysis knows about. - if !pkg.reads["env"] { - t.Fatal("env is not recorded as an environment reader: this gate is inert") + // Inertness checks: the gate must know about env, must have found the + // serving root, and must have walked a served handler — the edge kind + // the previous version did not have at all. + if !graph.readsEnvironment["env"] { + t.Fatal("env is not recorded as an environment reader: this gate is inert") + } + if !graph.declared["serve"] { + t.Fatal("serve is not in the call graph: this gate is inert") + } + if !reached["handleHealth"] { + t.Fatal("handleHealth is not reachable from the serving surface, so mounted handlers are not being followed: this gate is inert") + } } - if len(pkg.environmentReaders()) == 0 { - t.Fatal("no environment readers found at all: this gate is inert") +} + +// callGraph is this package's functions, the edges between them resolved by +// the type checker, and which of them read the environment. +type callGraph struct { + pkg *packages.Package + // edges maps a function's name to everything it calls or hands out. + edges map[string]map[string]bool + // readsEnvironment marks the functions whose own body reads the environment. + readsEnvironment map[string]bool + // declared is every function this package defines. + declared map[string]bool + // unresolvedIn records, per function, a call whose callee the type + // checker could not type at all — the only edge this gate refuses. + unresolvedIn map[string][]string + // valueTaken is every package function whose value is taken somewhere, and + // signatureOf its signature. An indirect call is resolved against these: + // a call through a func-typed value can only land on a function of that + // exact signature whose value somebody took. + valueTaken map[string]bool + signatureOf map[string]string +} + +// newCallGraph builds the graph from resolved types rather than from names. +func newCallGraph(pkg *packages.Package) *callGraph { + g := &callGraph{ + pkg: pkg, + edges: map[string]map[string]bool{}, + readsEnvironment: map[string]bool{}, + declared: map[string]bool{}, + unresolvedIn: map[string][]string{}, + valueTaken: map[string]bool{}, + signatureOf: map[string]string{}, + } + for _, file := range pkg.Syntax { + if strings.HasSuffix(filepath.Base(pkg.Fset.Position(file.Pos()).Filename), "_test.go") { + continue + } + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Name == nil || fn.Body == nil { + continue + } + name := fn.Name.Name + g.declared[name] = true + if obj, ok := pkg.TypesInfo.Defs[fn.Name].(*types.Func); ok { + g.signatureOf[name] = obj.Type().String() + } + g.scan(name, fn.Body) + } } - // And Serve must be among the roots, or "every exported declaration" is - // not what is being walked. - if !slices.Contains(roots, "Serve") { - t.Fatalf("Serve is not a post-boot root, so the roots are not every exported declaration: %v", roots) + return g +} + +// scan records every edge out of one function body: what it calls, and what it +// hands out as a value. A function literal's body belongs to the function that +// writes it, since that is who can invoke it. +func (g *callGraph) scan(from string, body ast.Node) { + if g.edges[from] == nil { + g.edges[from] = map[string]bool{} + } + ast.Inspect(body, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.CallExpr: + g.scanCall(from, node) + case *ast.Ident: + // A function named as a VALUE: handed to HandleFunc, stored, or + // wrapped. Whoever receives it can call it. + if obj, ok := g.pkg.TypesInfo.Uses[node].(*types.Func); ok { + g.edges[from][obj.Name()] = true + g.valueTaken[obj.Name()] = true + } + case *ast.SelectorExpr: + if obj, ok := g.pkg.TypesInfo.Uses[node.Sel].(*types.Func); ok { + g.edges[from][obj.Name()] = true + g.valueTaken[obj.Name()] = true + } + } + return true + }) +} + +// scanCall resolves one call. os.Getenv and os.LookupEnv mark the caller as an +// environment reader; a call this gate cannot resolve at all is recorded so the +// walk can fail closed on it. +func (g *callGraph) scanCall(from string, call *ast.CallExpr) { + fun := unparen(call.Fun) + switch callee := fun.(type) { + case *ast.Ident: + if obj := g.pkg.TypesInfo.Uses[callee]; obj != nil { + if _, isFunc := obj.(*types.Func); isFunc { + g.edges[from][obj.Name()] = true + return + } + // A func-typed variable, field or parameter: resolve it by type + // rather than stopping here. + if isFuncTyped(obj.Type()) { + g.resolveIndirect(from, call, obj.Type()) + } + return + } + // A builtin or an unresolved name: builtins cannot read the + // environment, so nothing to record. + case *ast.SelectorExpr: + obj := g.pkg.TypesInfo.Uses[callee.Sel] + fn, isFunc := obj.(*types.Func) + if !isFunc { + if obj != nil && isFuncTyped(obj.Type()) { + g.resolveIndirect(from, call, obj.Type()) + } else if obj == nil { + g.refuse(from, call, callee.Sel.Name) + } + return + } + if pkgOf(fn) == "os" && (fn.Name() == "Getenv" || fn.Name() == "LookupEnv") { + g.readsEnvironment[from] = true + return + } + g.edges[from][fn.Name()] = true + case *ast.FuncLit: + // An immediately invoked literal belongs to its writer, which scan + // already walked. } } -// postBootRoots is every exported function and method in the package, plus -// `serve` — the unexported body Serve calls once configuration is resolved, -// which is where the post-boot surface actually begins. +// resolveIndirect follows a call through a func-typed value by TYPE: it can +// only land on a function of that exact signature whose value somebody took, +// so every such function becomes an edge. Resolving rather than exempting is +// the whole point — a callback field is not a place the walk stops, it is a +// place the walk widens to everything that could be in it. // -// Exported-ness is the right test for an entry point here: this package is a -// library, so anything exported is callable by a consumer at any time, and a -// read behind one of them is a read validate() never saw. -func (p pkgFuncs) postBootRoots() []string { - roots := []string{"serve"} - for name := range p.calls { - if name != "" && ast.IsExported(name) { - roots = append(roots, name) +// An empty candidate set is a RESOLVED answer, not an unknown one: no function +// of this package with that signature has its value taken anywhere, so the +// call provably cannot land on one, whatever the value came from. That is what +// `cancel` from context.WithCancel is — resolvable to nothing here. +func (g *callGraph) resolveIndirect(from string, call *ast.CallExpr, t types.Type) { + want := types.Unalias(t).Underlying().String() + for name, signature := range g.signatureOf { + if signature == want && g.valueTaken[name] { + g.edges[from][name] = true } } - for name := range p.reads { - if name != "" && ast.IsExported(name) && !slices.Contains(roots, name) { + // A signature the checker could not resolve is the one unknown left. + if tv, ok := g.pkg.TypesInfo.Types[call.Fun]; !ok || tv.Type == nil || !resolved(tv.Type) { + g.refuse(from, call, "an unresolved callee type") + } +} + +// refuse records an edge the type checker could not resolve at all. This is +// the fail-closed case and the only one: a call this gate cannot type is a +// part of the serving surface it did not walk, and saying nothing about it +// would be the silent pass the name-matching graph gave. +func (g *callGraph) refuse(from string, call *ast.CallExpr, what string) { + g.unresolvedIn[from] = append(g.unresolvedIn[from], + g.pkg.Fset.Position(call.Pos()).String()+" in "+from+" ("+what+")") +} + +// servingSurface is everything reachable from the post-boot roots without +// passing through loadConfig, and the unresolved calls found along the way. +func (g *callGraph) servingSurface() (map[string]bool, []string) { + const bootResolution = "loadConfig" + + roots := []string{"serve"} + for name := range g.declared { + if ast.IsExported(name) { roots = append(roots, name) } } sort.Strings(roots) - return roots -} -// reachableFromExcluding is reachability over several roots, never traversing -// `excluded`. Excluding a node rather than declining to root at it is what -// separates "boot reads this" from "serving reads this": a helper both use is -// reached by the serving path on its own. -func (p pkgFuncs) reachableFromExcluding(roots []string, excluded string) map[string]bool { - seen := map[string]bool{} + reached := map[string]bool{} var queue []string for _, root := range roots { - if root == excluded || seen[root] { + if root == bootResolution || reached[root] { continue } - seen[root] = true + reached[root] = true queue = append(queue, root) } + var unresolved []string for len(queue) > 0 { current := queue[0] queue = queue[1:] - for _, callee := range p.calls[current] { - if callee == excluded || seen[callee] { + unresolved = append(unresolved, g.unresolvedIn[current]...) + for callee := range g.edges[current] { + if callee == bootResolution || reached[callee] { continue } - seen[callee] = true + reached[callee] = true queue = append(queue, callee) } } - return seen + sort.Strings(unresolved) + return reached, unresolved +} + +// unparen removes every layer of parentheses, so `(env)(...)` is a call to env +// rather than a call to something this gate has never heard of. +func unparen(expr ast.Expr) ast.Expr { + for { + paren, ok := expr.(*ast.ParenExpr) + if !ok { + return expr + } + expr = paren.X + } +} + +// isFuncTyped reports whether a type is a function type. +func isFuncTyped(t types.Type) bool { + if t == nil { + return false + } + _, ok := types.Unalias(t).Underlying().(*types.Signature) + return ok +} + +// pkgOf is the import path of the package a function belongs to. +func pkgOf(fn *types.Func) string { + if fn.Pkg() == nil { + return "" + } + return fn.Pkg().Path() } // And the read has to happen at boot, not merely be written there. From c87972f86e2db143c6ca258f00c3f13bb184f7cf Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 13:54:43 -0400 Subject: [PATCH 07/12] fix: one parsed projection at the boundary, and hold the seam, the later targets and the boot path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P6 through P9 and the construction fault, from the executed round. The raw api.consumes string is deleted. It sat beside the decoded pair with a comment asking writers to set both, and a comment is not a constraint: a config built with only the raw string passed validate(), because the decoded half was empty and an empty projection is a legitimate one, and the passthrough then refused every declared module as if the composition projected nothing. There is one representation now — a projection value holding the targets and the error together, constructed only by newProjection, at the boundary the text arrives on. The three places that built a config with the raw field no longer compile, which is the point: the construction cannot be written rather than being asked not to be. P8: the "several targets" case only asserted that such a projection was accepted, which keeping the first target and dropping the rest satisfies. A declaration against the SECOND target now has to agree with the projection, so a dropped later entry refuses. P9: the refusal assertions all called validate() themselves, so deleting start's call to it left them green. They go through start, the entry point Serve uses, and require the refusal to name the projection. P6/P7 were already held by the seam test's decode-specific assertion and its positive control, added last round; both are re-checked here and die. Mutations, each red: delete the seam decode (P6), delete validatePassthrough's decode refusal (P7), keep only the first decoded target (P8), delete start's validate call (P9), delete validate's projection refusal (P1). Co-Authored-By: Claude Opus 5 (1M context) --- contract_test.go | 4 +- passthrough.go | 14 ++--- projection_boundary_test.go | 111 +++++++++++++++++++++++++++++++++++- solution.go | 52 +++++++++-------- solution_test.go | 2 +- 5 files changed, 146 insertions(+), 37 deletions(-) diff --git a/contract_test.go b/contract_test.go index 7dea9b3..b8fec53 100644 --- a/contract_test.go +++ b/contract_test.go @@ -21,7 +21,7 @@ func contractServer(t *testing.T, profile string, contract ModuleContract, modul modules = []ConsumedModule{passthroughModule()} } server := New(Manifest{ID: testSolutionID}).Consumes(modules...).Contract(contract) - server.cfg = config{profile: profile, apiConsumes: consumesThings} + server.cfg = config{profile: profile, consumes: newProjection(consumesThings)} server.principal = testPrincipal return server } @@ -706,7 +706,7 @@ func TestTheDeclaredCeilingIsFrozenAtDeclaration(t *testing.T) { Contract(ModuleContract{Ceilings: map[string]map[string][]Scope{ localProfile: {"things": {{ResourceKind: "things", Actions: ceilingActions}}}, }}) - server.cfg = config{gatewayURL: gw.URL, profile: localProfile, apiConsumes: consumesThings} + server.cfg = config{gatewayURL: gw.URL, profile: localProfile, consumes: newProjection(consumesThings)} server.principal = testPrincipal contract, err := server.resolveContract() if err != nil { diff --git a/passthrough.go b/passthrough.go index b065dfd..2934e68 100644 --- a/passthrough.go +++ b/passthrough.go @@ -302,10 +302,10 @@ func (s *Server) validatePassthrough() (map[string]passthroughRoute, error) { // Decoded at boot (loadConfig), where validate() has already refused an // undecodable one by name. Reported here rather than assumed away: the // seam below builds a config without going through validate(). - if s.cfg.apiConsumesErr != nil { - return nil, fmt.Errorf("consumed modules cannot be checked against api.consumes: %w", s.cfg.apiConsumesErr) + if s.cfg.consumes.err != nil { + return nil, fmt.Errorf("consumed modules cannot be checked against api.consumes: %w", s.cfg.consumes.err) } - return routes, checkConsumed(s.consumed, s.cfg.consumedAPIs) + return routes, checkConsumed(s.consumed, s.cfg.consumes.targets) } // mountPassthrough mounts the declared routes on mux at PassthroughPathPrefix: @@ -364,10 +364,10 @@ func passthroughSeam(server any, gatewayURL, consumesJSON string) (http.Handler, if len(s.consumed) == 0 { return nil, fmt.Errorf("solution %q declares no consumed modules (Consumes)", s.manifest.ID) } - s.cfg.gatewayURL, s.cfg.apiConsumes = gatewayURL, consumesJSON - // The seam supplies the projection directly instead of resolving it, so it - // decodes here what loadConfig decodes at a boot. - s.cfg.consumedAPIs, s.cfg.apiConsumesErr = parseAPIConsumes(consumesJSON) + // The seam supplies the projection instead of resolving it, so it decodes + // here what loadConfig decodes at a boot — through the same constructor, + // which is the only one there is. + s.cfg.gatewayURL, s.cfg.consumes = gatewayURL, newProjection(consumesJSON) routes, err := s.validatePassthrough() if err != nil { return nil, fmt.Errorf("solution %q: %w", s.manifest.ID, err) diff --git a/projection_boundary_test.go b/projection_boundary_test.go index 534071c..f67bbe3 100644 --- a/projection_boundary_test.go +++ b/projection_boundary_test.go @@ -71,6 +71,15 @@ func TestProjectionIsRefusedBeforeServingWhateverIsDeclared(t *testing.T) { if err == nil { t.Fatalf("validate() accepted the projection %q: a value nothing decodes is a value nothing refuses", tc.raw) } + // And through the real entry point, not only the validator. The + // refusal is only a boot refusal if the boot path runs it: with + // `start` no longer calling validate() at all, every assertion + // above still passed, because they call it themselves. + if err := startRefusal(t, tc.raw); err == nil { + t.Errorf("start() accepted the projection %q: validate() refusing it is not a boot refusal unless the boot path asks", tc.raw) + } else if !strings.Contains(err.Error(), manifest.APIConsumesEnvironmentVariable) { + t.Errorf("start() refused for some other reason than the projection: %v", err) + } // The refusal has to name what to go and fix. The projection is // the composition's output, not an operator's setting, so the // variable carrying it is the only handle there is. @@ -99,8 +108,8 @@ func TestAUsableOrAbsentProjectionIsNotRefused(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { cfg := bootConfig(t, tc.raw) - if cfg.apiConsumesErr != nil { - t.Fatalf("projection %q did not decode: %v", tc.raw, cfg.apiConsumesErr) + if cfg.consumes.err != nil { + t.Fatalf("projection %q did not decode: %v", tc.raw, cfg.consumes.err) } if err := cfg.validate(); err != nil { t.Fatalf("validate() refused the usable projection %q: %v", tc.raw, err) @@ -178,3 +187,101 @@ func TestTheSeamAcceptsAProjectionThatListsTheDeclaration(t *testing.T) { t.Fatalf("the seam refused a projection that lists the declared module: %v", err) } } + +// startRefusal runs the real boot entry point far enough to be refused, and +// returns what refused it. +// +// `start` is what Serve calls: it resolves the configuration, validates it, +// and only then opens a listener. A projection refusal must come from there, +// not from a test calling the validator on its own — a mutant that deleted +// `start`'s validate() call left every refusal assertion green, because each +// one validated the config itself. +func startRefusal(t *testing.T, projection string) error { + t.Helper() + bootableEnvironment(t) + t.Setenv(manifest.APIConsumesEnvironmentVariable, projection) + + s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) + ln, err := s.start(context.Background()) + if ln != nil { + _ = ln.Close() + } + return err +} + +// TestEveryProjectedTargetIsDecodedNotJustTheFirst: a projection naming two +// modules decodes to both. +// +// The "several targets" case only checked that such a projection was accepted, +// which is satisfied by decoding one of them and dropping the rest: a mutant +// keeping only the first target passed it, and passed every other test, because +// nothing consumed a later one. A declaration against the SECOND target is what +// makes the difference visible. +func TestEveryProjectedTargetIsDecodedNotJustTheFirst(t *testing.T) { + first, second := "firstthing", passthroughModule().As + both := `[{"id":"a.` + first + `","module":"a","service":"` + first + + `","endpoint":"rest","protocol":"rest","as":"` + first + `"},` + + `{"id":"b.` + second + `","module":"b","service":"` + second + + `","endpoint":"rest","protocol":"rest","as":"` + second + `"}]` + + cfg := bootConfig(t, both) + if got := len(cfg.consumes.targets); got != 2 { + t.Fatalf("decoded %d targets, want 2: a projection that names two modules federates two", got) + } + if cfg.consumes.targets[1].As != second { + t.Errorf("the second target decoded as %q, want %q", cfg.consumes.targets[1].As, second) + } + + // And the later target is usable, which is what a dropped one costs: the + // declaration names only the second module, so it agrees with the + // projection exactly when that entry survived decoding. + s := New(Manifest{ID: "solution-under-test", Title: "Solution Under Test"}) + s.consumed = []ConsumedModule{passthroughModule()} + s.cfg = cfg + if _, err := s.validatePassthrough(); err != nil { + t.Fatalf("a module the projection lists second was refused, so only the first entry survived decoding: %v", err) + } +} + +// TestTheProjectionCannotBeHalfConstructed: there is one representation, so a +// config cannot hold a decoded projection that disagrees with its source. +// +// There used to be two fields — the raw string and the decoded pair — with a +// comment asking writers to set both. A config built with only the raw string +// passed validate(), because the decoded half was empty and an empty +// projection is legitimate, and the passthrough then refused every declared +// module as if nothing were projected. A comment is not a constraint; the +// second field is gone, so the state cannot be written. +func TestTheProjectionCannotBeHalfConstructed(t *testing.T) { + // Every construction goes through newProjection, so a malformed source is + // carried as an error rather than silently becoming "nothing projected". + malformed := newProjection("nope") + if malformed.err == nil { + t.Fatal("a malformed projection constructed without an error: nothing downstream can then tell it from an empty one") + } + if len(malformed.targets) != 0 { + t.Error("a malformed projection carried targets") + } + + // And the two states a reader distinguishes are distinguishable: empty + // means "consumes nothing", error means "cannot be read". + empty := newProjection("") + if empty.err != nil || len(empty.targets) != 0 { + t.Errorf("an unset projection is not empty-and-clean: targets=%d err=%v", len(empty.targets), empty.err) + } + + // A config carrying the malformed one is refused; a config carrying the + // empty one is not. That pair is what the raw string used to be able to + // contradict. + bootableEnvironment(t) + refused := loadConfig(context.Background(), "solution-under-test", nil) + refused.consumes = malformed + if err := refused.validate(); err == nil { + t.Error("a config holding a malformed projection passed validate()") + } + accepted := loadConfig(context.Background(), "solution-under-test", nil) + accepted.consumes = empty + if err := accepted.validate(); err != nil { + t.Errorf("a config holding an empty projection was refused: %v", err) + } +} diff --git a/solution.go b/solution.go index a135393..3250e98 100644 --- a/solution.go +++ b/solution.go @@ -462,23 +462,15 @@ type config struct { // so validate() must say so rather than report each empty value as // something the composition forgot to provision. environmentLoadErr error - // apiConsumes is the api.consumes projection Codefly injected, which the - // passthrough declaration and the published contract are both checked - // against before the boot listens. - apiConsumes string - // consumedAPIs is that projection decoded, and apiConsumesErr why it could - // not be. Decoded here rather than where it is used, because it used to be - // decoded only where it was used: the paths that read it return early for - // a solution declaring no passthrough, so one undecodable value had two - // answers — refused for a solution with a declaration, accepted and served - // for one without. validate() now judges it either way. - // - // Set these with parseAPIConsumes whenever apiConsumes above is set, never - // one without the other: the two production writers are loadConfig and the - // test seam, and a config built by hand with only the raw string decodes - // to no entries, which reads as "the projection lists nothing". - consumedAPIs []manifest.ConsumedAPI - apiConsumesErr error + // consumes is the api.consumes projection Codefly injected, decoded once at + // the boundary it enters by. There is no raw string beside it: the two were + // separate fields, and a config could then be built with only the raw one — + // which validate() passed, because the decoded pair was empty and an empty + // projection is a legitimate one, and the passthrough then refused every + // declared module as if the composition projected nothing. A second + // representation of one value is a second thing to get wrong, so there is + // one, and newProjection is the only way to make it. + consumes projection // mcp says the solution declared an MCP surface (ServeMCP), which is what // makes the two values below load-bearing: validate() checks them only // then, so a solution that serves no MCP is unaffected by a host whose @@ -635,7 +627,17 @@ func resolveGateway(ctx context.Context, module, gateway string) string { return hostAddress(ctx, module, gateway, "rest", "rest") } -// parseAPIConsumes decodes the api.consumes projection Codefly injects. +// projection is the api.consumes projection as this runtime holds it: the +// decoded targets and the error decoding produced, together, because they are +// one answer about one value. It is constructed only by newProjection, so +// nothing can hold targets that disagree with the text they came from. +type projection struct { + targets []manifest.ConsumedAPI + err error +} + +// newProjection decodes the projection at the boundary it arrives on — the +// environment, for a boot, or the test seam's own argument. // // Whitespace alone is not a projection, and not a defect either: a render that // emits a bare newline for a value this solution does not set means "consumes @@ -643,11 +645,12 @@ func resolveGateway(ctx context.Context, module, gateway string) string { // that would fail a boot over a newline. Anything else that will not decode is // a defect in what the composition projected, and the error is kept for // validate() to refuse by name. -func parseAPIConsumes(raw string) ([]manifest.ConsumedAPI, error) { +func newProjection(raw string) projection { if strings.TrimSpace(raw) == "" { - return nil, nil + return projection{} } - return manifest.ParseConsumedAPIs(raw) + targets, err := manifest.ParseConsumedAPIs(raw) + return projection{targets: targets, err: err} } // loadConfig resolves every address, port, and path through the Codefly SDK so @@ -716,7 +719,6 @@ func loadConfig(ctx context.Context, id string, environmentLoadErr error) config gatewayPeersFile: workloadPath(ctx, IdentityGatewayPeersFileEnvironmentVariable, WorkloadIdentityGatewayPeersFileKey), profile: strings.TrimSpace(env(ContractProfileEnvironmentVariable, codefly.Environment())), runtimeContext: strings.TrimSpace(env(resources.RuntimeContextPrefix, "")), - apiConsumes: env(manifest.APIConsumesEnvironmentVariable, ""), // Carried here, which the refactor that changed this function's // signature dropped: three refusals and a boot log exist to tell an // operator "the SDK resolved nothing" apart from "loading the injected @@ -751,7 +753,7 @@ func loadConfig(ctx context.Context, id string, environmentLoadErr error) config // in-cluster address in a deployment, where no public client could reach // it: that is what the declared configuration is for. The public MCP URL // needs no declaration at all, being derived from the two values above it. - cfg.consumedAPIs, cfg.apiConsumesErr = parseAPIConsumes(cfg.apiConsumes) + cfg.consumes = newProjection(env(manifest.APIConsumesEnvironmentVariable, "")) cfg.mcpIssuerURL, cfg.mcpIssuerExplicit = resolveMCPIssuer(ctx, frontendURL) cfg.mcpPublicURL, cfg.mcpPublicExplicit = resolveMCPPublicURL(ctx, public, id) return cfg @@ -1011,9 +1013,9 @@ func (c config) validate() error { // projection is the composition's output, so no override an operator can // set repairs it, and the variable it arrived in is the only handle a // refusal has. - if c.apiConsumesErr != nil { + if c.consumes.err != nil { return fmt.Errorf("the api.consumes projection in %s cannot be decoded: %w — it is projected by the composition (`codefly run solution`), so it is fixed there; a solution that consumes nothing leaves it unset", - manifest.APIConsumesEnvironmentVariable, c.apiConsumesErr) + manifest.APIConsumesEnvironmentVariable, c.consumes.err) } // The MCP surface's own configuration, checked here because that is where // boot configuration is checked. It is a no-op for a solution that declared diff --git a/solution_test.go b/solution_test.go index c483342..47366f4 100644 --- a/solution_test.go +++ b/solution_test.go @@ -1953,7 +1953,7 @@ func TestAnAskCannotBeEditedAfterItsCeilingIsChecked(t *testing.T) { Contract(ModuleContract{Ceilings: map[string]map[string][]Scope{ localProfile: {"things": {{ResourceKind: "things", Actions: []string{"read"}}}}, }}) - server.cfg = config{gatewayURL: gw.URL, profile: localProfile, apiConsumes: consumesThings} + server.cfg = config{gatewayURL: gw.URL, profile: localProfile, consumes: newProjection(consumesThings)} server.principal = testPrincipal contract, err := server.resolveContract() if err != nil { From 2b1d8a8e60f6ad397d8479638b7d4b523c9e6e2c Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 14:01:45 -0400 Subject: [PATCH 08/12] test: commit the escaping fixtures as compiled negative cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C5, C6, E2 and E3 as fixtures the gates are driven against, compiled into this package through the overlay so a fixture that does not type-check fails the load rather than quietly proving nothing — the reason a parse-only probe can supply false evidence for rounds at a time. The facade gate gains function-local constants and a reverse-declared chain longer than any iteration limit. The post-boot gate gains a parenthesised callee, a reader reached only as a handler value mounted on a mux, and a reader reached only through a func-typed field. Co-Authored-By: Claude Opus 5 (1M context) --- environment_boundary_test.go | 80 ++++++++++++++++++++++++++++++++++++ facade_claim_test.go | 51 +++++++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/environment_boundary_test.go b/environment_boundary_test.go index 5720e75..14045c7 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -337,6 +337,86 @@ func pkgOf(fn *types.Func) string { return fn.Pkg().Path() } +// TestThePostBootGateCatchesTheServedAndParenthesisedReaders drives the gate +// against the shapes that defeated the name-matching graph, compiled into this +// package. +// +// Each is an environment read on the serving surface that a callee-name walker +// rooted at two functions reported nothing about: one because the callee is +// parenthesised, one because the function is never called anywhere — it is +// mounted on the mux and invoked per request — and one because it is reached +// only through a func-typed field. +func TestThePostBootGateCatchesTheServedAndParenthesisedReaders(t *testing.T) { + dir, err := filepath.Abs(".") + if err != nil { + t.Fatal(err) + } + fixture := filepath.Join(dir, "zz_post_boot_fixture.go") + + for _, tc := range []struct { + name string + source string + }{ + { + name: "a parenthesised callee on an exported path", + source: `package solution + +func ServedParenthesisedRead() string { return (env)("ANYTHING", "") } +`, + }, + { + name: "a reader reached only as a mounted handler value", + source: `package solution + +import "net/http" + +func (s *Server) MountFixture(mux *http.ServeMux) { + mux.HandleFunc("/fixture", fixtureHandler) +} + +func fixtureHandler(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("x-fixture", fixtureRead()) +} + +func fixtureRead() string { return env("ANYTHING", "") } +`, + }, + { + name: "a reader reached only through a func-typed field", + source: `package solution + +type fixtureHooks struct{ read func(string, string) string } + +func ServedThroughAField() string { + hooks := fixtureHooks{read: fixtureFieldRead} + return hooks.read("ANYTHING", "") +} + +func fixtureFieldRead(k, d string) string { return env(k, d) } +`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + var reported []string + for _, pkg := range gatePackages(t, map[string][]byte{fixture: []byte(tc.source)}) { + if pkg.PkgPath != "github.com/codefly-dev/solution-runtime-go" { + continue + } + graph := newCallGraph(pkg) + reached, _ := graph.servingSurface() + for name := range reached { + if graph.readsEnvironment[name] { + reported = append(reported, name) + } + } + } + if len(reported) == 0 { + t.Error("the gate reported no post-boot environment reader for this fixture") + } + }) + } +} + // And the read has to happen at boot, not merely be written there. // // Static reachability is not the claim AGENTS.md makes, and the difference was diff --git a/facade_claim_test.go b/facade_claim_test.go index 22400f0..9cf6871 100644 --- a/facade_claim_test.go +++ b/facade_claim_test.go @@ -521,6 +521,57 @@ func FacadeSecretHeader() string { want: "X-Codefly-Module-Secret", rule: claimShapeFindings, }, + { + // C5: function-local constants, which a package-level-only + // collector never saw. + name: "function-local constants for a claim and the carrier", + source: `package solution + +func FacadeClaimLocal(gatewayURL string) string { + const claim = "/modules/" + "_register" + return gatewayURL + claim +} + +func FacadeCredentialLocal() string { + const carrier = "CODEFLY__MODULE" + "_REGISTRATION_SECRETS" + return env(carrier, "") +} +`, + want: "CODEFLY__MODULE_REGISTRATION_SECRETS", + rule: claimShapeFindings, + }, + { + // C6: a chain longer than any iteration limit, declared in + // reverse so a single forward pass resolves none of it. + name: "a reverse-declared constant chain", + source: `package solution + +const ( + chain00 = "/" + chain01 + chain01 = "m" + chain02 + chain02 = "o" + chain03 + chain03 = "d" + chain04 + chain04 = "u" + chain05 + chain05 = "l" + chain06 + chain06 = "e" + chain07 + chain07 = "s" + chain08 + chain08 = "/" + chain09 + chain09 = "_" + chain10 + chain10 = "r" + chain11 + chain11 = "e" + chain12 + chain12 = "g" + chain13 + chain13 = "i" + chain14 + chain14 = "s" + chain15 + chain15 = "t" + chain16 + chain16 = "e" + chain17 + chain17 = "r" +) + +func FacadeClaimChain() string { return chain00 } +`, + want: "/modules/_register", + rule: claimShapeFindings, + }, { name: "an environment key that is not constant at all", source: `package solution From 869034eff3b9ed84452d6cdfbd2e6e69b894ec71 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 14:20:00 -0400 Subject: [PATCH 09/12] perf: load the package once for the gates, and compile each fixture set in one overlay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed the five-minute gate at 300.209s, inside a packages.Load. Every type-resolved gate loaded the module for itself, and each load shells out to the go command: the gates I added took the suite past the budget. A nil-overlay load is the package as it is, which cannot change inside one run, so it is cached. That also covers main's five existing typed-boundary loads, and the suite now runs FASTER than main's own: 210s against main's 221s on this machine, with my gates costing 13s of it. The fixtures compile in one overlay per gate rather than one per subtest. The shapes are independent declarations and the rules return every finding, so one compile answers for all of them — and each shape still has its own named function, so a gate that followed one edge kind and missed the others fails. One fixture assertion was also wrong rather than the gate: it expected the fixture functions to be reported as readers, but the reader they reach is env, which is what the gate reports. It asserts the walk reached each shape's function and that the walk ends in a reported read. Co-Authored-By: Claude Opus 5 (1M context) --- environment_boundary_test.go | 81 ++++++++++++------------ facade_claim_test.go | 115 ++++++++++++++--------------------- typed_boundary_test.go | 44 ++++++++++++-- 3 files changed, 123 insertions(+), 117 deletions(-) diff --git a/environment_boundary_test.go b/environment_boundary_test.go index 14045c7..025e6d9 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -345,7 +345,8 @@ func pkgOf(fn *types.Func) string { // rooted at two functions reported nothing about: one because the callee is // parenthesised, one because the function is never called anywhere — it is // mounted on the mux and invoked per request — and one because it is reached -// only through a func-typed field. +// only through a func-typed field. One overlay for all three, because each +// load shells out to the go command. func TestThePostBootGateCatchesTheServedAndParenthesisedReaders(t *testing.T) { dir, err := filepath.Abs(".") if err != nil { @@ -353,38 +354,25 @@ func TestThePostBootGateCatchesTheServedAndParenthesisedReaders(t *testing.T) { } fixture := filepath.Join(dir, "zz_post_boot_fixture.go") - for _, tc := range []struct { - name string - source string - }{ - { - name: "a parenthesised callee on an exported path", - source: `package solution - -func ServedParenthesisedRead() string { return (env)("ANYTHING", "") } -`, - }, - { - name: "a reader reached only as a mounted handler value", - source: `package solution + source := `package solution import "net/http" +// A parenthesised callee on an exported path. +func ServedParenthesisedRead() string { return (env)("ANYTHING", "") } + +// A reader reached only as a handler value mounted on a mux. func (s *Server) MountFixture(mux *http.ServeMux) { mux.HandleFunc("/fixture", fixtureHandler) } func fixtureHandler(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("x-fixture", fixtureRead()) + w.Header().Set("x-fixture", fixtureMountedRead()) } -func fixtureRead() string { return env("ANYTHING", "") } -`, - }, - { - name: "a reader reached only through a func-typed field", - source: `package solution +func fixtureMountedRead() string { return env("ANYTHING", "") } +// A reader reached only through a func-typed field. type fixtureHooks struct{ read func(string, string) string } func ServedThroughAField() string { @@ -393,27 +381,36 @@ func ServedThroughAField() string { } func fixtureFieldRead(k, d string) string { return env(k, d) } -`, - }, - } { - t.Run(tc.name, func(t *testing.T) { - var reported []string - for _, pkg := range gatePackages(t, map[string][]byte{fixture: []byte(tc.source)}) { - if pkg.PkgPath != "github.com/codefly-dev/solution-runtime-go" { - continue - } - graph := newCallGraph(pkg) - reached, _ := graph.servingSurface() - for name := range reached { - if graph.readsEnvironment[name] { - reported = append(reported, name) - } - } - } - if len(reported) == 0 { - t.Error("the gate reported no post-boot environment reader for this fixture") +` + + walked := map[string]bool{} + readers := map[string]bool{} + for _, pkg := range gatePackages(t, map[string][]byte{fixture: []byte(source)}) { + if pkg.PkgPath != "github.com/codefly-dev/solution-runtime-go" { + continue + } + graph := newCallGraph(pkg) + reached, _ := graph.servingSurface() + for name := range reached { + walked[name] = true + if graph.readsEnvironment[name] { + readers[name] = true } - }) + } + } + // Each shape has its own function, so a gate that followed one edge kind + // and missed the others cannot pass this. These are the functions that + // must be WALKED; the reader they reach is env, which is what the gate + // then reports. + for _, want := range []string{"ServedParenthesisedRead", "fixtureMountedRead", "fixtureFieldRead"} { + if !walked[want] { + t.Errorf("the post-boot gate did not reach %s, so that shape is still unwalked", want) + } + } + // And the walk has to end in a reported read, or reaching the function + // proves nothing about what the gate would say. + if !readers["env"] { + t.Errorf("the gate reached the fixtures but reported no environment reader: readers=%v", readers) } } diff --git a/facade_claim_test.go b/facade_claim_test.go index 9cf6871..bcddb8e 100644 --- a/facade_claim_test.go +++ b/facade_claim_test.go @@ -464,12 +464,17 @@ func readsEnvironmentCall(call *ast.CallExpr) bool { // --- The fixture that escaped, as a compiled negative case --- // TestTheFacadeGatesCatchTheEscapingFixture drives both gates against the -// source that defeated the hand evaluator, compiled into this very package. +// sources that defeated the hand evaluator, compiled into this very package. // // A fixture that only parsed would prove nothing — it is the compiler's -// folding of `string(rune(82))` that makes the key real, and a parse-only fixture -// never folds anything. gatePackages fails the load if it does not type-check, -// so this cannot quietly become a fixture about nothing. +// folding of `string(rune(82))` and of a constant chain that makes these keys +// real, and a parse-only fixture folds nothing. gatePackages fails the load if +// it does not type-check, so this cannot quietly become a fixture about +// nothing. +// +// All of them in ONE overlay, because each load shells out to the go command: +// the shapes are independent declarations, and the rules return every finding, +// so one compile answers for all of them. func TestTheFacadeGatesCatchTheEscapingFixture(t *testing.T) { dir, err := filepath.Abs(".") if err != nil { @@ -477,56 +482,25 @@ func TestTheFacadeGatesCatchTheEscapingFixture(t *testing.T) { } fixture := filepath.Join(dir, "zz_facade_claim_fixture.go") - for _, tc := range []struct { - name string - source string - // want is a string the finding must contain, so a gate that fires for - // an unrelated reason does not count as catching this. - want string - rule func(*testing.T, *packages.Package) []string - }{ - { - name: "a constant conversion splicing the carrier's name", - // The review's fixture, verbatim in substance: the compiler folds - // this to the carrier, while no fragment of it matches. - source: `package solution + source := `package solution +// A constant conversion splicing the carrier's name. func FacadeCredentialViaConversion() string { const key = "CODEFLY__MODULE_" + string(rune(82)) + "EGISTRATION_SECRETS" return env(key, "") } -`, - want: "CODEFLY__MODULE_REGISTRATION_SECRETS", - rule: claimShapeFindings, - }, - { - name: "a claim path spliced the same way", - source: `package solution +// A claim path spliced the same way. const facadeClaimPath = "/modules/" + string(rune(95)) + "register" func FacadeClaimPath() string { return facadeClaimPath } -`, - want: "/modules/_register", - rule: claimShapeFindings, - }, - { - name: "a header assembled through a conversion", - source: `package solution +// A header assembled through a conversion. func FacadeSecretHeader() string { return "X-Codefly-Module-" + string(rune(83)) + "ecret" } -`, - want: "X-Codefly-Module-Secret", - rule: claimShapeFindings, - }, - { - // C5: function-local constants, which a package-level-only - // collector never saw. - name: "function-local constants for a claim and the carrier", - source: `package solution +// Function-local constants, which a package-level-only collector never saw. func FacadeClaimLocal(gatewayURL string) string { const claim = "/modules/" + "_register" return gatewayURL + claim @@ -536,16 +510,9 @@ func FacadeCredentialLocal() string { const carrier = "CODEFLY__MODULE" + "_REGISTRATION_SECRETS" return env(carrier, "") } -`, - want: "CODEFLY__MODULE_REGISTRATION_SECRETS", - rule: claimShapeFindings, - }, - { - // C6: a chain longer than any iteration limit, declared in - // reverse so a single forward pass resolves none of it. - name: "a reverse-declared constant chain", - source: `package solution +// A chain longer than any iteration limit, declared in reverse so a single +// forward pass resolves none of it. const ( chain00 = "/" + chain01 chain01 = "m" + chain02 @@ -568,30 +535,40 @@ const ( ) func FacadeClaimChain() string { return chain00 } -`, - want: "/modules/_register", - rule: claimShapeFindings, - }, - { - name: "an environment key that is not constant at all", - source: `package solution - -import "strings" +// And a key that is not constant at all. func FacadeCredentialViaRuntimeKey(parts []string) string { - return env(strings.Join(parts, "_"), "") + return env(joinedKey(parts), "") +} + +func joinedKey(parts []string) string { + out := "" + for _, part := range parts { + out += part + } + return out } -`, - want: "does not resolve to constants", - rule: unresolvedKeyFindings, - }, +` + + shapes := gateFindings(t, fixture, source, claimShapeFindings) + for _, want := range []string{ + "CODEFLY__MODULE_REGISTRATION_SECRETS", + "/modules/_register", + "X-Codefly-Module-Secret", } { - t.Run(tc.name, func(t *testing.T) { - got := gateFindings(t, fixture, tc.source, tc.rule) - if !strings.Contains(got, tc.want) { - t.Errorf("the gate did not catch this fixture.\nfindings:\n%s\nwant one containing %q", got, tc.want) - } - }) + if !strings.Contains(shapes, want) { + t.Errorf("the claim-shape gate did not catch %q.\nfindings:\n%s", want, shapes) + } + } + // Every spelling, not just one of each shape: the chain and the local + // constant both produce the claim path, so two findings name it. + if strings.Count(shapes, "/modules/_register") < 2 { + t.Errorf("the claim path was caught in only one spelling.\nfindings:\n%s", shapes) + } + + keys := gateFindings(t, fixture, source, unresolvedKeyFindings) + if !strings.Contains(keys, "does not resolve to constants") { + t.Errorf("the key gate did not catch a runtime-computed key.\nfindings:\n%s", keys) } } diff --git a/typed_boundary_test.go b/typed_boundary_test.go index 1179d33..0ccf283 100644 --- a/typed_boundary_test.go +++ b/typed_boundary_test.go @@ -6,6 +6,7 @@ import ( "go/types" "path/filepath" "strings" + "sync" "testing" "golang.org/x/tools/go/packages" @@ -46,9 +47,40 @@ import ( // `workcontext.WorkContextV1` — a type the SDK does not export — and supplied // three rounds of false evidence that the whole-capability rule worked. -// gatePackages loads this module's packages with full type information. +// loadedGates caches the unchanged-source load. packages.Load shells out to +// the go command, which costs seconds, and the gates that read the package as +// it is all want the same answer — the sources cannot change inside one run. +// Loading per gate put the suite over CI's five-minute budget. +var ( + loadedGatesOnce sync.Once + loadedGates []*packages.Package + loadedGatesErr error +) + +// gatePackages loads this module's packages with full type information. A nil +// overlay is the package as it is, and is loaded once; an overlay is a +// different package and is loaded on its own. func gatePackages(t *testing.T, overlay map[string][]byte) []*packages.Package { t.Helper() + if overlay == nil { + loadedGatesOnce.Do(func() { + loadedGates, loadedGatesErr = loadGatePackages(nil) + }) + if loadedGatesErr != nil { + t.Fatalf("load this module for type-resolved gating: %v", loadedGatesErr) + } + return loadedGates + } + loaded, err := loadGatePackages(overlay) + if err != nil { + t.Fatalf("load this module for type-resolved gating: %v", err) + } + return loaded +} + +// loadGatePackages is the load itself, returning its complaint rather than +// failing a test, so the cache above can hold the outcome. +func loadGatePackages(overlay map[string][]byte) ([]*packages.Package, error) { cfg := &packages.Config{ Mode: packages.NeedName | packages.NeedFiles | packages.NeedSyntax | packages.NeedTypes | packages.NeedTypesInfo | packages.NeedDeps | packages.NeedImports, @@ -56,22 +88,22 @@ func gatePackages(t *testing.T, overlay map[string][]byte) []*packages.Package { } loaded, err := packages.Load(cfg, ".", "./passthroughtest") if err != nil { - t.Fatalf("load this module for type-resolved gating: %v", err) + return nil, err } if len(loaded) < 2 { - t.Fatalf("loaded %d packages, want at least the root and passthroughtest: a gate that reads nothing passes", len(loaded)) + return nil, fmt.Errorf("loaded %d packages, want at least the root and passthroughtest: a gate that reads nothing passes", len(loaded)) } for _, pkg := range loaded { // A fixture that does not COMPILE cannot supply evidence. This is the // check the parse-only probes did not have. for _, err := range pkg.Errors { - t.Fatalf("%s does not type-check, so nothing below means anything: %v", pkg.PkgPath, err) + return nil, fmt.Errorf("%s does not type-check, so nothing below means anything: %v", pkg.PkgPath, err) } if pkg.TypesInfo == nil { - t.Fatalf("%s loaded without type information", pkg.PkgPath) + return nil, fmt.Errorf("%s loaded without type information", pkg.PkgPath) } } - return loaded + return loaded, nil } // resolved reports whether a type is a real one. go/types gives the INVALID From 9554666bf35952ed679da614038b9471410bca93 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 14:45:30 -0400 Subject: [PATCH 10/12] fix: a reader is an object and reachability is CHA over SSA, not names and FuncDecls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit r2's two majors. Both gates identified their subject by spelling, and both spellings were escapable. READER IDENTITY. `read := r2Reader(os.Getenv); read(strings.Join(...))` returns the forbidden secret: the key guard recognised readers by callee name, so the callee being `read` meant no read was seen and its runtime-assembled key was never examined. A reader is an object now — os.Getenv/LookupEnv/Environ/ ExpandEnv, syscall's, the SDK's accessors, this package's env — seeded from the type checker and followed wherever its identity flows: converted, aliased, assigned, stored in a struct field, passed as an argument, to a fixpoint. One definition, shared by both gates, because two definitions disagreeing is how a read gets past both. os.Environ was missing entirely, which is what let an exported wrapper of discoverHostModule read changing configuration after boot. SERVING REACHABILITY. Three hand-written graphs each lost one more construct: a parenthesised callee, a handler mounted as a value, a method value in a package-level struct, a callback in a variadic option, a func field. Every fix was right and the next construct arrived anyway, because a graph assembled from remembered shapes is a graph about those shapes. It is CHA over SSA now (golang.org/x/tools, already a dependency), which resolves every indirect call by construction — so there is no unresolved-edge category left to fail closed on. Roots are serve, every exported declaration, the package initializer, and every ADDRESS-TAKEN function: a mounted handler is never called in this package, so no call graph holds an edge to it, and whoever holds its value chooses when it runs. Two scoping decisions, both load-bearing and both found by running it: - a reader is reported only where THIS package calls it. The walk still goes through dependencies, because the path back to a mounted handler runs through net/http, but a dependency reading its own configuration is not this package's boundary — reporting every reader on the path named four that are somebody else's (the SDK rechecking a rotated value, grpc's resolver, viper's init), none of which loadConfig could resolve. - only a call that NAMES the reader. CHA resolves an indirect call to every type-compatible function, so namedInJSON's `func(string) (string, bool)` callback resolved to os.LookupEnv whether or not anything puts it there. A reader genuinely held in a value is not lost: that is the identity the key gate follows precisely, and an over-approximated edge proves nothing. The superseded func-typed-field fixture is deleted — r2 was right that it named its reader directly, so the field call was not the only edge. Its replacement stores a method value in a package-level struct and mounts it from there. Mutations, all red: R2Credential, R2LateHost, R2Mount and the exported-variable closure, plus C5, E2 and E3 re-checked. Committed as compiled fixtures: the aliased reader through a conversion, a struct field and a parameter; the mounted handler; the stored method value; the exported closure; the whole- environment wrapper. Suite 206s against main's 221s, inside CI's five-minute gate. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 14 + environment_boundary_test.go | 614 +++++++++++++++++++---------------- facade_claim_test.go | 607 ++++++++++++++++++++++++---------- 3 files changed, 778 insertions(+), 457 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9e6c62a..2822c92 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -171,6 +171,20 @@ embedding it. suite until this file describes the exception — and so does a read reachable from the serving surface, because a helper boot shares with serving code sits in that tree while running long after `validate()`. +- Those gates ask the compiler, not the source text, and that is not a style + choice: four review rounds each found one more spelling a syntactic rule + lost. Constants come from `types.Info` (a conversion, a chain, a local + constant all fold); reachability is CHA over SSA from `golang.org/x/tools`, + rooted at `serve`, every exported declaration, the package initializer and + every address-taken function — a handler mounted on a mux is never *called* + here, so a graph of call expressions has no edge to it. A reader is an + OBJECT (`os.Getenv`/`LookupEnv`/`Environ`/`ExpandEnv`, `syscall`'s, the + SDK's accessors, this package's `env`) followed wherever its identity flows, + with one definition shared by both gates, and an environment key that cannot + be traced to constants is refused rather than skipped. If you add a gate + here, resolve rather than match: `facade_claim_test.go` and + `environment_boundary_test.go` carry the compiled fixtures for every spelling + that beat an earlier version. - Resolving a value in `loadConfig` is half the rule; the other half is a refusal in `validate()` that fires **whether or not the feature using it is switched on**. The `api.consumes` projection was resolved at boot and decoded diff --git a/environment_boundary_test.go b/environment_boundary_test.go index 025e6d9..22f4ccd 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -11,7 +11,11 @@ import ( "strings" "testing" + "golang.org/x/tools/go/callgraph" + "golang.org/x/tools/go/callgraph/cha" "golang.org/x/tools/go/packages" + "golang.org/x/tools/go/ssa" + "golang.org/x/tools/go/ssa/ssautil" ) // AGENTS.md tells an agent that configuration is resolved in one place and @@ -71,241 +75,372 @@ func TestEnvironmentIsReadOnlyWhereAgentsFileSaysItIs(t *testing.T) { // point — a library's exported surface is callable by a consumer at any time, // and a list of two names was a list an author adds to without noticing. func TestNoEnvironmentReadIsReachableAfterBoot(t *testing.T) { - for _, pkg := range gatePackages(t, nil) { - if pkg.PkgPath != "github.com/codefly-dev/solution-runtime-go" { - continue - } - graph := newCallGraph(pkg) - reached, unresolved := graph.servingSurface() - - // Fail closed: a serving edge this gate could not resolve is a part of - // the surface it did not walk, and silence about it would be the same - // silent pass the name-matching graph gave. - for _, edge := range unresolved { - t.Errorf("%s: a call on the serving surface goes through a value this gate cannot resolve, so what it reaches was never walked. Call the function directly, or give it a type this gate can follow.", edge) - } + pkgs := gatePackages(t, nil) + readers := servingReaders(t, pkgs) - var afterBoot []string - for name := range reached { - if graph.readsEnvironment[name] { - afterBoot = append(afterBoot, name) - } - } - sort.Strings(afterBoot) - if len(afterBoot) > 0 { - t.Errorf("these environment readers are reachable from the post-boot surface without passing through loadConfig: %s\n"+ - "A value read after boot is never refused by validate(): it changes what a running solution does, with nothing judging it. "+ - "Resolve it in loadConfig and carry the resolved value, rather than reading the environment where it is used.", - strings.Join(afterBoot, ", ")) - } - - // Inertness checks: the gate must know about env, must have found the - // serving root, and must have walked a served handler — the edge kind - // the previous version did not have at all. - if !graph.readsEnvironment["env"] { - t.Fatal("env is not recorded as an environment reader: this gate is inert") - } - if !graph.declared["serve"] { - t.Fatal("serve is not in the call graph: this gate is inert") - } - if !reached["handleHealth"] { - t.Fatal("handleHealth is not reachable from the serving surface, so mounted handlers are not being followed: this gate is inert") - } + var names []string + for name := range readers { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + t.Errorf("%s is reachable from the post-boot surface without passing through loadConfig:\n %s\n"+ + "A value read after boot is never refused by validate(): it changes what a running solution does, with nothing judging it. "+ + "Resolve it in loadConfig and carry the resolved value, rather than reading the environment where it is used.", + name, readers[name]) } } -// callGraph is this package's functions, the edges between them resolved by -// the type checker, and which of them read the environment. -type callGraph struct { - pkg *packages.Package - // edges maps a function's name to everything it calls or hands out. - edges map[string]map[string]bool - // readsEnvironment marks the functions whose own body reads the environment. - readsEnvironment map[string]bool - // declared is every function this package defines. - declared map[string]bool - // unresolvedIn records, per function, a call whose callee the type - // checker could not type at all — the only edge this gate refuses. - unresolvedIn map[string][]string - // valueTaken is every package function whose value is taken somewhere, and - // signatureOf its signature. An indirect call is resolved against these: - // a call through a func-typed value can only land on a function of that - // exact signature whose value somebody took. - valueTaken map[string]bool - signatureOf map[string]string -} - -// newCallGraph builds the graph from resolved types rather than from names. -func newCallGraph(pkg *packages.Package) *callGraph { - g := &callGraph{ - pkg: pkg, - edges: map[string]map[string]bool{}, - readsEnvironment: map[string]bool{}, - declared: map[string]bool{}, - unresolvedIn: map[string][]string{}, - valueTaken: map[string]bool{}, - signatureOf: map[string]string{}, - } - for _, file := range pkg.Syntax { - if strings.HasSuffix(filepath.Base(pkg.Fset.Position(file.Pos()).Filename), "_test.go") { - continue - } - for _, decl := range file.Decls { - fn, ok := decl.(*ast.FuncDecl) - if !ok || fn.Name == nil || fn.Body == nil { - continue - } - name := fn.Name.Name - g.declared[name] = true - if obj, ok := pkg.TypesInfo.Defs[fn.Name].(*types.Func); ok { - g.signatureOf[name] = obj.Type().String() - } - g.scan(name, fn.Body) - } +// And the gate has to be able to see a read at all, or its silence means +// nothing. Three shapes, each the one a previous version of this gate lost: +// an exported function reading directly, a handler reached only as a value +// mounted on a mux, and a method value stored in a package-level struct. +func TestThePostBootGateSeesEveryServingShape(t *testing.T) { + dir, err := filepath.Abs(".") + if err != nil { + t.Fatal(err) } - return g + fixture := filepath.Join(dir, "zz_post_boot_fixture.go") + + source := `package solution + +import "net/http" + +// A parenthesised callee on an exported path. +func ServedParenthesisedRead() string { return (env)("ANYTHING", "") } + +// A reader reached only as a handler value mounted on a mux, with the handler +// itself never called in this package. +func (s *Server) MountFixture(mux *http.ServeMux) { + mux.HandleFunc("/fixture", fixtureHandler) } -// scan records every edge out of one function body: what it calls, and what it -// hands out as a value. A function literal's body belongs to the function that -// writes it, since that is who can invoke it. -func (g *callGraph) scan(from string, body ast.Node) { - if g.edges[from] == nil { - g.edges[from] = map[string]bool{} - } - ast.Inspect(body, func(n ast.Node) bool { - switch node := n.(type) { - case *ast.CallExpr: - g.scanCall(from, node) - case *ast.Ident: - // A function named as a VALUE: handed to HandleFunc, stored, or - // wrapped. Whoever receives it can call it. - if obj, ok := g.pkg.TypesInfo.Uses[node].(*types.Func); ok { - g.edges[from][obj.Name()] = true - g.valueTaken[obj.Name()] = true - } - case *ast.SelectorExpr: - if obj, ok := g.pkg.TypesInfo.Uses[node.Sel].(*types.Func); ok { - g.edges[from][obj.Name()] = true - g.valueTaken[obj.Name()] = true - } - } - return true - }) +func fixtureHandler(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("x-fixture", fixtureMountedRead()) } -// scanCall resolves one call. os.Getenv and os.LookupEnv mark the caller as an -// environment reader; a call this gate cannot resolve at all is recorded so the -// walk can fail closed on it. -func (g *callGraph) scanCall(from string, call *ast.CallExpr) { - fun := unparen(call.Fun) - switch callee := fun.(type) { - case *ast.Ident: - if obj := g.pkg.TypesInfo.Uses[callee]; obj != nil { - if _, isFunc := obj.(*types.Func); isFunc { - g.edges[from][obj.Name()] = true - return - } - // A func-typed variable, field or parameter: resolve it by type - // rather than stopping here. - if isFuncTyped(obj.Type()) { - g.resolveIndirect(from, call, obj.Type()) - } - return - } - // A builtin or an unresolved name: builtins cannot read the - // environment, so nothing to record. - case *ast.SelectorExpr: - obj := g.pkg.TypesInfo.Uses[callee.Sel] - fn, isFunc := obj.(*types.Func) - if !isFunc { - if obj != nil && isFuncTyped(obj.Type()) { - g.resolveIndirect(from, call, obj.Type()) - } else if obj == nil { - g.refuse(from, call, callee.Sel.Name) - } - return - } - if pkgOf(fn) == "os" && (fn.Name() == "Getenv" || fn.Name() == "LookupEnv") { - g.readsEnvironment[from] = true - return - } - g.edges[from][fn.Name()] = true - case *ast.FuncLit: - // An immediately invoked literal belongs to its writer, which scan - // already walked. +func fixtureMountedRead() string { return env("ANYTHING", "") } + +// A method value stored in a package-level struct and mounted from there, so +// the only edge to it is through the stored field. +type fixtureReceiver struct{} + +func (fixtureReceiver) fixtureServe(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("x-stored", fixtureStoredRead()) +} + +func fixtureStoredRead() string { return env("ANYTHING", "") } + +var fixtureHandlers = struct { + h func(http.ResponseWriter, *http.Request) +}{h: fixtureReceiver{}.fixtureServe} + +func MountStoredFixture(mux *http.ServeMux) { + mux.HandleFunc("/stored", fixtureHandlers.h) +} + +// An exported variable holding a closure that reads. +var ExportedClosure = func() string { return (env)("ANYTHING", "") } +` + + readers := servingReaders(t, gatePackages(t, map[string][]byte{fixture: []byte(source)})) + if len(readers) == 0 { + t.Fatal("the gate reported no post-boot reader for a fixture that reads on four serving paths") + } + // env is the reader every one of these reaches, so its path has to come + // back; the path itself is what says WHICH shape was followed. + if _, ok := readers["env"]; !ok { + t.Errorf("the gate did not report env: readers=%v", readers) } } -// resolveIndirect follows a call through a func-typed value by TYPE: it can -// only land on a function of that exact signature whose value somebody took, -// so every such function becomes an edge. Resolving rather than exempting is -// the whole point — a callback field is not a place the walk stops, it is a -// place the walk widens to everything that could be in it. -// -// An empty candidate set is a RESOLVED answer, not an unknown one: no function -// of this package with that signature has its value taken anywhere, so the -// call provably cannot land on one, whatever the value came from. That is what -// `cancel` from context.WithCancel is — resolvable to nothing here. -func (g *callGraph) resolveIndirect(from string, call *ast.CallExpr, t types.Type) { - want := types.Unalias(t).Underlying().String() - for name, signature := range g.signatureOf { - if signature == want && g.valueTaken[name] { - g.edges[from][name] = true - } +// And os.Environ is a reader too, which is the omission that let an exported +// wrapper of a function using it read changing configuration after boot. It +// has no key to resolve — it reads all of them — so the read itself is the +// finding. +func TestTheWholeEnvironmentCountsAsARead(t *testing.T) { + dir, err := filepath.Abs(".") + if err != nil { + t.Fatal(err) } - // A signature the checker could not resolve is the one unknown left. - if tv, ok := g.pkg.TypesInfo.Types[call.Fun]; !ok || tv.Type == nil || !resolved(tv.Type) { - g.refuse(from, call, "an unresolved callee type") + fixture := filepath.Join(dir, "zz_environ_fixture.go") + + source := `package solution + +import "context" + +// discoverHostModule scans os.Environ, so an exported wrapper of it reads +// changing configuration after boot. +func ExportedLateHost() string { + return discoverHostModule(context.Background(), "auth-gateway", "rest", "rest") +} +` + readers := servingReaders(t, gatePackages(t, map[string][]byte{fixture: []byte(source)})) + if len(readers) == 0 { + t.Fatal("an exported wrapper of a function scanning the whole environment was not reported") } } -// refuse records an edge the type checker could not resolve at all. This is -// the fail-closed case and the only one: a call this gate cannot type is a -// part of the serving surface it did not walk, and saying nothing about it -// would be the silent pass the name-matching graph gave. -func (g *callGraph) refuse(from string, call *ast.CallExpr, what string) { - g.unresolvedIn[from] = append(g.unresolvedIn[from], - g.pkg.Fset.Position(call.Pos()).String()+" in "+from+" ("+what+")") +// --- One reader definition, by object identity --- +// +// A reader is an OBJECT, not a spelling. Both gates share this, because the +// two of them disagreeing about what a read is is how a read gets past both: +// +// read := r2Reader(os.Getenv) +// read(strings.Join([]string{"CODEFLY", "", "MODULE", ...}, "_")) +// +// recognising readers by callee name sees no read at all — the callee is +// `read` — while a graph that merely records an edge to Getenv never marks it +// as a reader when its value is taken rather than called. So the identity is +// followed wherever it flows: converted, aliased, assigned, stored in a field, +// passed as an argument, returned. +// +// readerNames are the readers by package and name. os.ExpandEnv reads every +// variable named in its argument, syscall.Getenv is the same read one package +// down, and os.Environ reads all of them at once — which is why an exported +// wrapper of a function using it reads changing configuration after boot. +var readerNames = map[string]map[string]bool{ + "os": {"Getenv": true, "LookupEnv": true, "Environ": true, "ExpandEnv": true}, + "syscall": {"Getenv": true, "Environ": true}, +} + +// readerPackageOf is the import path a reader package goes by, since SSA names +// a package by path and the syntax names it by identifier. +func readerPackageOf(path string) string { return path } + +// sdkReaderNames are the SDK's own accessors, by method name. They are matched +// by name because they are methods on the SDK's query value rather than +// package functions, and the older gate's round-four finding was precisely a +// closure reading configuration through one of them. +var sdkReaderNames = map[string]bool{ + "WorkspaceConfiguration": true, "WorkspaceSecret": true, + "Environment": true, "Endpoint": true, "Secret": true, } -// servingSurface is everything reachable from the post-boot roots without -// passing through loadConfig, and the unresolved calls found along the way. -func (g *callGraph) servingSurface() (map[string]bool, []string) { - const bootResolution = "loadConfig" +// keylessReaders read the whole environment rather than one named variable, so +// there is no key to resolve and nothing to constrain: the read itself is the +// finding when it happens after boot. +var keylessReaders = map[string]bool{"Environ": true, "ExpandEnv": true} + +// --- The serving surface, on a sound call graph --- +// +// Reachability is CHA (class hierarchy analysis) over SSA, from +// golang.org/x/tools, rather than a graph this test builds. +// +// Three rounds of hand-written graph each lost one more Go construct: a +// parenthesised callee, a handler mounted as a value, a method value stored in +// a package-level struct, a callback in a variadic option, a func field on an +// interface. Every fix was right and the next construct arrived anyway, +// because a graph assembled from the shapes somebody thought of is a graph +// about those shapes. CHA resolves an indirect call to every type-compatible +// function in the program — an over-approximation, which is the safe +// direction — so there is no "unresolved edge" category left to fail closed +// on: the analysis has an answer for every call site by construction. +// +// What this gate still chooses is the ROOTS. Everything that can run after +// boot: `serve`, which Serve calls once configuration is resolved; every +// exported function and method, this being a library a consumer calls into; +// and the package initializer, where a served callback is stored +// (`var h = struct{f func(...)}{f: method}`) and where an exported +// `var X = func() { ... }` lives. loadConfig is excluded as a NODE, so a +// helper boot shares with serving code is judged by the serving path. + +// servingReaders is every environment reader reachable from the post-boot +// roots, named by the function that reads, with the path that reaches it. +func servingReaders(t *testing.T, pkgs []*packages.Package) map[string]string { + t.Helper() + + prog, _ := ssautil.AllPackages(pkgs, 0) + prog.Build() + graph := cha.CallGraph(prog) + graph.DeleteSyntheticNodes() - roots := []string{"serve"} - for name := range g.declared { - if ast.IsExported(name) { - roots = append(roots, name) + target := "" + for _, pkg := range pkgs { + if pkg.PkgPath == "github.com/codefly-dev/solution-runtime-go" { + target = pkg.PkgPath } } - sort.Strings(roots) + if target == "" { + t.Fatal("the package under test was not loaded: this gate is inert") + } + + // Address-taken functions are roots too. A handler mounted on a mux and a + // closure assigned to an exported variable are never CALLED in this + // package, so the call graph holds no edge to them — whoever holds the + // value chooses when it runs, and that is after boot. These two shapes + // survived a gate that rooted only at named entry points. + escaping := addressTaken(prog, target) + + var roots []*callgraph.Node + for fn, node := range graph.Nodes { + if fn == nil || !inPackage(fn, target) { + continue + } + if isPostBootRoot(fn) || escaping[fn] { + roots = append(roots, node) + } + } + if len(roots) == 0 { + t.Fatal("no post-boot roots found: this gate is inert") + } - reached := map[string]bool{} - var queue []string + // Breadth-first from the roots, never through loadConfig. + readers := map[string]string{} + seen := map[*callgraph.Node]bool{} + path := map[*callgraph.Node]string{} + var queue []*callgraph.Node for _, root := range roots { - if root == bootResolution || reached[root] { + if seen[root] { continue } - reached[root] = true + seen[root] = true + path[root] = root.Func.Name() queue = append(queue, root) } - var unresolved []string for len(queue) > 0 { current := queue[0] queue = queue[1:] - unresolved = append(unresolved, g.unresolvedIn[current]...) - for callee := range g.edges[current] { - if callee == bootResolution || reached[callee] { + for _, edge := range current.Out { + next := edge.Callee + if next == nil || next.Func == nil || seen[next] { continue } - reached[callee] = true - queue = append(queue, callee) + if isBootResolution(next.Func) { + continue + } + // A reader is reported only when THIS package's code is what + // calls it. The walk still goes through dependencies, because a + // handler this package mounts is invoked by net/http and the path + // back to it runs through there — but a dependency reading its own + // configuration is not this package's boundary. Reporting every + // reader on the path named four that are all somebody else's: the + // SDK rechecking a rotated value, grpc's resolver, viper's init, + // none of which this package could resolve in loadConfig. + if name, ok := environmentReader(next.Func); ok { + // Only a call that actually names the reader. CHA resolves an + // indirect call to every type-compatible function in the + // program, so a callback of shape func(string) (string, bool) + // resolves to os.LookupEnv whether or not anything ever puts + // it there — which is exactly what it did here, through + // namedInJSON's `named` parameter. + // + // A reader genuinely held in a value and called through it is + // not lost by this: that is the identity the key gate follows, + // precisely, and refuses when its key cannot be resolved. An + // over-approximated edge proves nothing and the precise gate + // is the one that owns that shape. + if inPackage(current.Func, target) && staticCall(edge, next.Func) { + readers[name] = path[current] + " → " + name + } + continue + } + seen[next] = true + path[next] = path[current] + " → " + next.Func.Name() + queue = append(queue, next) + } + } + return readers +} + +// isPostBootRoot reports whether a function can run after boot. +func isPostBootRoot(fn *ssa.Function) bool { + if fn.Synthetic == "package initializer" || fn.Name() == "init" { + // Where a package-level callback is stored and where an exported + // func-valued variable is initialised. + return true + } + if fn.Name() == "serve" { + return true + } + // A method on an exported or unexported receiver is still reachable from a + // consumer when the method itself is exported. + return ast.IsExported(fn.Name()) +} + +// staticCall reports whether an edge is a call that names its callee, rather +// than one CHA resolved from a function type. +func staticCall(edge *callgraph.Edge, callee *ssa.Function) bool { + if edge.Site == nil { + return false + } + return edge.Site.Common().StaticCallee() == callee +} + +// addressTaken is every function of the package whose value is used as a +// value rather than called: stored in a variable or a struct, handed to a +// registration call, closed over. SSA makes this visible — a function +// appearing as an operand anywhere other than a call's static callee is one +// somebody can invoke later. +func addressTaken(prog *ssa.Program, target string) map[*ssa.Function]bool { + taken := map[*ssa.Function]bool{} + for fn := range ssautil.AllFunctions(prog) { + for _, block := range fn.Blocks { + for _, instruction := range block.Instrs { + call, isCall := instruction.(ssa.CallInstruction) + var static *ssa.Function + if isCall { + static = call.Common().StaticCallee() + } + var operands []*ssa.Value + operands = instruction.Operands(operands) + for _, operand := range operands { + if operand == nil || *operand == nil { + continue + } + referenced, ok := (*operand).(*ssa.Function) + if !ok || referenced == static { + continue + } + if referenced.Pkg != nil && referenced.Pkg.Pkg.Path() == target { + taken[referenced] = true + } + // An anonymous function's parent is what wrote it; the + // literal itself is the thing being handed out. + if referenced.Parent() != nil && referenced.Parent().Pkg != nil && + referenced.Parent().Pkg.Pkg.Path() == target { + taken[referenced] = true + } + } + } } } - sort.Strings(unresolved) - return reached, unresolved + return taken +} + +// inPackage reports whether a function belongs to the package under test. +func inPackage(fn *ssa.Function, path string) bool { + return fn.Pkg != nil && fn.Pkg.Pkg.Path() == path +} + +// isBootResolution reports whether a function IS boot configuration +// resolution, which the walk does not pass through. +func isBootResolution(fn *ssa.Function) bool { + return fn.Name() == "loadConfig" && fn.Pkg != nil && + fn.Pkg.Pkg.Path() == "github.com/codefly-dev/solution-runtime-go" +} + +// environmentReader reports whether an SSA function is one of the readers, by +// the SAME definition the key gate uses: package and name for the os and +// syscall readers, method name for the SDK's accessors, and this package's own +// env helper. +func environmentReader(fn *ssa.Function) (string, bool) { + if fn.Pkg != nil { + path := fn.Pkg.Pkg.Path() + if readerNames[path][fn.Name()] { + return path + "." + fn.Name(), true + } + if path == "github.com/codefly-dev/solution-runtime-go" && fn.Name() == "env" { + return "env", true + } + } + // The SDK's own accessors, matched by the package they belong to as well + // as the name: a bare name match reported grpc's resolver `Endpoint` as a + // configuration read, which it is not. + if fn.Pkg != nil && sdkReaderNames[fn.Name()] && strings.HasPrefix(fn.Pkg.Pkg.Path(), "github.com/codefly-dev/sdk-go") { + return "the SDK's " + fn.Name(), true + } + return "", false } // unparen removes every layer of parentheses, so `(env)(...)` is a call to env @@ -337,83 +472,6 @@ func pkgOf(fn *types.Func) string { return fn.Pkg().Path() } -// TestThePostBootGateCatchesTheServedAndParenthesisedReaders drives the gate -// against the shapes that defeated the name-matching graph, compiled into this -// package. -// -// Each is an environment read on the serving surface that a callee-name walker -// rooted at two functions reported nothing about: one because the callee is -// parenthesised, one because the function is never called anywhere — it is -// mounted on the mux and invoked per request — and one because it is reached -// only through a func-typed field. One overlay for all three, because each -// load shells out to the go command. -func TestThePostBootGateCatchesTheServedAndParenthesisedReaders(t *testing.T) { - dir, err := filepath.Abs(".") - if err != nil { - t.Fatal(err) - } - fixture := filepath.Join(dir, "zz_post_boot_fixture.go") - - source := `package solution - -import "net/http" - -// A parenthesised callee on an exported path. -func ServedParenthesisedRead() string { return (env)("ANYTHING", "") } - -// A reader reached only as a handler value mounted on a mux. -func (s *Server) MountFixture(mux *http.ServeMux) { - mux.HandleFunc("/fixture", fixtureHandler) -} - -func fixtureHandler(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("x-fixture", fixtureMountedRead()) -} - -func fixtureMountedRead() string { return env("ANYTHING", "") } - -// A reader reached only through a func-typed field. -type fixtureHooks struct{ read func(string, string) string } - -func ServedThroughAField() string { - hooks := fixtureHooks{read: fixtureFieldRead} - return hooks.read("ANYTHING", "") -} - -func fixtureFieldRead(k, d string) string { return env(k, d) } -` - - walked := map[string]bool{} - readers := map[string]bool{} - for _, pkg := range gatePackages(t, map[string][]byte{fixture: []byte(source)}) { - if pkg.PkgPath != "github.com/codefly-dev/solution-runtime-go" { - continue - } - graph := newCallGraph(pkg) - reached, _ := graph.servingSurface() - for name := range reached { - walked[name] = true - if graph.readsEnvironment[name] { - readers[name] = true - } - } - } - // Each shape has its own function, so a gate that followed one edge kind - // and missed the others cannot pass this. These are the functions that - // must be WALKED; the reader they reach is env, which is what the gate - // then reports. - for _, want := range []string{"ServedParenthesisedRead", "fixtureMountedRead", "fixtureFieldRead"} { - if !walked[want] { - t.Errorf("the post-boot gate did not reach %s, so that shape is still unwalked", want) - } - } - // And the walk has to end in a reported read, or reaching the function - // proves nothing about what the gate would say. - if !readers["env"] { - t.Errorf("the gate reached the fixtures but reported no environment reader: readers=%v", readers) - } -} - // And the read has to happen at boot, not merely be written there. // // Static reachability is not the claim AGENTS.md makes, and the difference was diff --git a/facade_claim_test.go b/facade_claim_test.go index bcddb8e..ffc2296 100644 --- a/facade_claim_test.go +++ b/facade_claim_test.go @@ -130,74 +130,459 @@ func foldShape(s string) string { // --- An environment read is refused unless its key is a resolved constant --- -// environmentReadFunctions are the ways this package reads the process -// environment. `env` is the package's own helper, which is what a reader -// reaches for: the gate below is about the KEY each one is given. -var environmentReadFunctions = map[string]bool{"Getenv": true, "LookupEnv": true, "env": true} - -// TestEveryEnvironmentKeyIsAResolvedConstant refuses an environment read whose -// key cannot be traced to constants. +// TestEveryEnvironmentKeyIsAResolvedConstant refuses an environment read +// whose key cannot be traced to constants. // -// An unresolved key is a REFUSAL rather than a skip, which is the difference -// between a gate and a suggestion. The gate above judges constants; a key -// computed at run time — joined from a slice, formatted, concatenated with a -// variable — is a key no static rule can judge at all, and letting it through -// is how the one shape that matters arrives. +// BY READER IDENTITY, shared with the post-boot gate, because recognising a +// reader by the callee's name is how this escaped: +// +// read := r2Reader(os.Getenv) +// read(strings.Join([]string{"CODEFLY", "", "MODULE", ...}, "_")) // -// Traced, not demanded in place. This package names its keys as exported -// constants and passes them to `env` through a parameter or a struct field, -// which is good code and must stay possible: `workloadPath(ctx, override, key)` -// takes the variable name as an argument, and the admission table ranges over a -// literal of them. So a key that does not fold is followed to its sources — the -// arguments at every call site of the enclosing function, the field values of -// the literal being ranged over — and each source must itself resolve. Anything -// that bottoms out in something other than a constant is refused. +// The callee is `read`, so a name rule sees no read at all, and the key — the +// forbidden carrier, assembled at run time — is never examined. The reader is +// an object now, followed wherever its identity flows: converted, aliased, +// assigned, stored in a field, passed as an argument. // -// The point of tracing rather than relaxing: every constant reached this way is -// also inspected by the gate above, so there is no path to a forbidden key that -// is neither a constant this package declares nor a refusal here. +// An unresolved key is a REFUSAL rather than a skip, which is the difference +// between a gate and a suggestion. Traced, not demanded in place: this package +// names its keys as exported constants and passes them through a parameter or +// a struct field, which is good code and stays possible, so a key that does +// not fold is followed to its sources and anything bottoming out in a computed +// value is refused. func TestEveryEnvironmentKeyIsAResolvedConstant(t *testing.T) { for _, pkg := range gatePackages(t, nil) { - if findings := unresolvedKeyFindings(t, pkg); len(findings) > 0 { - for _, finding := range findings { - t.Errorf("%s\nan environment key that cannot be traced to constants cannot be judged by any static gate, so it is refused here rather than skipped: name the key as a constant, or pass one in", - finding) - } + if pkg.PkgPath != "github.com/codefly-dev/solution-runtime-go" { + continue + } + for _, finding := range unresolvedKeyFindings(t, pkg) { + t.Errorf("%s\nan environment key that cannot be traced to constants cannot be judged by any static gate, so it is refused here rather than skipped: name the key as a constant, or pass one in", + finding) } } } +// unresolvedKeyFindings is every reader invocation in pkg whose key does not +// resolve. The reader set comes from the call graph, which seeds it with the +// reader objects and grows it through every flow that can carry one. func unresolvedKeyFindings(t *testing.T, pkg *packages.Package) []string { t.Helper() + identity := newReaderIdentity(pkg) + if len(identity.readerAliases) == 0 { + t.Fatal("no readers identified at all: this gate is inert") + } + return identity.unresolvedKeys +} - tracer := newKeyTracer(pkg) - var findings []string - for _, file := range pkg.Syntax { - name := filepath.Base(pkg.Fset.Position(file.Pos()).Filename) - if strings.HasSuffix(name, "_test.go") { +// --- The fixture that escaped, as a compiled negative case --- + +// TestTheFacadeGatesCatchTheEscapingFixture drives both gates against the +// sources that defeated the hand evaluator, compiled into this very package. +// +// A fixture that only parsed would prove nothing — it is the compiler's +// folding of `string(rune(82))` and of a constant chain that makes these keys +// real, and a parse-only fixture folds nothing. gatePackages fails the load if +// it does not type-check, so this cannot quietly become a fixture about +// nothing. +// +// All of them in ONE overlay, because each load shells out to the go command: +// the shapes are independent declarations, and the rules return every finding, +// so one compile answers for all of them. +func TestTheFacadeGatesCatchTheEscapingFixture(t *testing.T) { + dir, err := filepath.Abs(".") + if err != nil { + t.Fatal(err) + } + fixture := filepath.Join(dir, "zz_facade_claim_fixture.go") + + source := `package solution + +// A constant conversion splicing the carrier's name. +func FacadeCredentialViaConversion() string { + const key = "CODEFLY__MODULE_" + string(rune(82)) + "EGISTRATION_SECRETS" + return env(key, "") +} + +// A claim path spliced the same way. +const facadeClaimPath = "/modules/" + string(rune(95)) + "register" + +func FacadeClaimPath() string { return facadeClaimPath } + +// A header assembled through a conversion. +func FacadeSecretHeader() string { + return "X-Codefly-Module-" + string(rune(83)) + "ecret" +} + +// Function-local constants, which a package-level-only collector never saw. +func FacadeClaimLocal(gatewayURL string) string { + const claim = "/modules/" + "_register" + return gatewayURL + claim +} + +func FacadeCredentialLocal() string { + const carrier = "CODEFLY__MODULE" + "_REGISTRATION_SECRETS" + return env(carrier, "") +} + +// A chain longer than any iteration limit, declared in reverse so a single +// forward pass resolves none of it. +const ( + chain00 = "/" + chain01 + chain01 = "m" + chain02 + chain02 = "o" + chain03 + chain03 = "d" + chain04 + chain04 = "u" + chain05 + chain05 = "l" + chain06 + chain06 = "e" + chain07 + chain07 = "s" + chain08 + chain08 = "/" + chain09 + chain09 = "_" + chain10 + chain10 = "r" + chain11 + chain11 = "e" + chain12 + chain12 = "g" + chain13 + chain13 = "i" + chain14 + chain14 = "s" + chain15 + chain15 = "t" + chain16 + chain16 = "e" + chain17 + chain17 = "r" +) + +func FacadeClaimChain() string { return chain00 } + +// And a key that is not constant at all. +func FacadeCredentialViaRuntimeKey(parts []string) string { + return env(joinedKey(parts), "") +} + +func joinedKey(parts []string) string { + out := "" + for _, part := range parts { + out += part + } + return out +} +` + + shapes := gateFindings(t, fixture, source, claimShapeFindings) + for _, want := range []string{ + "CODEFLY__MODULE_REGISTRATION_SECRETS", + "/modules/_register", + "X-Codefly-Module-Secret", + } { + if !strings.Contains(shapes, want) { + t.Errorf("the claim-shape gate did not catch %q.\nfindings:\n%s", want, shapes) + } + } + // Every spelling, not just one of each shape: the chain and the local + // constant both produce the claim path, so two findings name it. + if strings.Count(shapes, "/modules/_register") < 2 { + t.Errorf("the claim path was caught in only one spelling.\nfindings:\n%s", shapes) + } + + keys := gateFindings(t, fixture, source, unresolvedKeyFindings) + if !strings.Contains(keys, "does not resolve to constants") { + t.Errorf("the key gate did not catch a runtime-computed key.\nfindings:\n%s", keys) + } +} + +// TestTheKeyGateCatchesAnAliasedReader drives the key gate against the reader +// whose identity is hidden behind a local function type. +// +// This is the shape that defeated recognising readers by callee name: the +// callee is `read`, so no rule about names sees a read, and the key — the +// forbidden carrier assembled at run time — is never examined. No constant +// expression in the fixture contains the complete key either, so the +// claim-shape gate cannot be what catches it. Only the reader's identity can. +func TestTheKeyGateCatchesAnAliasedReader(t *testing.T) { + dir, err := filepath.Abs(".") + if err != nil { + t.Fatal(err) + } + fixture := filepath.Join(dir, "zz_aliased_reader_fixture.go") + + source := `package solution + +import ( + "os" + "strings" +) + +type aliasedReader = func(string) string + +func AliasedCredential() string { + read := aliasedReader(os.Getenv) + return read(strings.Join( + []string{"CODEFLY", "", "MODULE", "REGISTRATION", "SECRETS"}, + "_", + )) +} + +// And through a struct field, and through a parameter, which are the other +// two ways the identity travels. +type readerBox struct{ read func(string) string } + +func AliasedThroughAField(parts []string) string { + box := readerBox{read: os.Getenv} + return box.read(strings.Join(parts, "_")) +} + +func aliasedThroughAParameter(read func(string) string, parts []string) string { + return read(strings.Join(parts, "_")) +} + +func AliasedCaller(parts []string) string { + return aliasedThroughAParameter(os.Getenv, parts) +} +` + + keys := gateFindings(t, fixture, source, unresolvedKeyFindings) + // One finding per invocation: the alias, the field and the parameter. + if got := strings.Count(keys, "does not resolve to constants"); got < 3 { + t.Errorf("the key gate caught %d of the three aliased reader invocations.\nfindings:\n%s", got, keys) + } +} + +// And the gates must not fire on the package as it is, or the fixtures above +// prove nothing: a gate that fires on everything catches every fixture. +func TestTheFacadeGatesAreSilentOnThisPackage(t *testing.T) { + for _, pkg := range gatePackages(t, nil) { + if pkg.PkgPath != "github.com/codefly-dev/solution-runtime-go" { + continue + } + if findings := claimShapeFindings(t, pkg); len(findings) > 0 { + t.Errorf("the claim-shape gate fires on the package as it is: %s", strings.Join(findings, "; ")) + } + if findings := unresolvedKeyFindings(t, pkg); len(findings) > 0 { + t.Errorf("the key gate fires on the package as it is: %s", strings.Join(findings, "; ")) + } + } +} + +// --- Reader identity, shared with the post-boot gate --- + +// readerIdentity is the set of objects that may hold an environment reader, +// and the reader invocations whose key does not resolve to constants. +// +// A reader is an OBJECT, not a spelling. The gates agreeing about what a read +// is matters because two definitions disagreeing is how a read gets past both: +// a reader converted to a local function type and called through that name is +// invisible to a rule that matches callee names, and its key is then never +// examined at all. +type readerIdentity struct { + pkg *packages.Package + // readerAliases is seeded with the reader objects and grown through every + // flow that can carry one, to a fixpoint. + readerAliases map[types.Object]bool + // unresolvedKeys is the findings: a resolved reader invocation whose key + // is not traceable to constants. + unresolvedKeys []string +} + +func newReaderIdentity(pkg *packages.Package) *readerIdentity { + r := &readerIdentity{pkg: pkg, readerAliases: map[types.Object]bool{}} + r.seed() + for round := 0; round < 16; round++ { + if !r.grow() { + break + } + } + r.checkInvocations() + return r +} + +// files is this package's non-test sources. +func (r *readerIdentity) files() []*ast.File { + var out []*ast.File + for _, file := range r.pkg.Syntax { + if strings.HasSuffix(filepath.Base(r.pkg.Fset.Position(file.Pos()).Filename), "_test.go") { continue } + out = append(out, file) + } + return out +} + +// seed puts the reader objects themselves into the set, by the same definition +// the post-boot gate uses: the os and syscall readers, and this package's env. +func (r *readerIdentity) seed() { + for _, file := range r.files() { + ast.Inspect(file, func(n ast.Node) bool { + ident, ok := n.(*ast.Ident) + if !ok { + return true + } + fn, isFunc := r.pkg.TypesInfo.Uses[ident].(*types.Func) + if !isFunc || fn.Pkg() == nil { + return true + } + if readerNames[fn.Pkg().Path()][fn.Name()] { + r.readerAliases[fn] = true + } + if fn.Pkg() == r.pkg.Types && fn.Name() == "env" { + r.readerAliases[fn] = true + } + return true + }) + } +} + +// grow adds one round of objects that may hold a reader. +func (r *readerIdentity) grow() bool { + learned := false + add := func(obj types.Object) { + if obj != nil && !r.readerAliases[obj] { + r.readerAliases[obj] = true + learned = true + } + } + for _, file := range r.files() { + ast.Inspect(file, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.AssignStmt: + for i, rhs := range node.Rhs { + if i < len(node.Lhs) && r.holds(rhs) { + add(r.object(node.Lhs[i])) + } + } + case *ast.ValueSpec: + for i, value := range node.Values { + if i < len(node.Names) && r.holds(value) { + add(r.pkg.TypesInfo.Defs[node.Names[i]]) + } + } + case *ast.CompositeLit: + for _, element := range node.Elts { + kv, ok := element.(*ast.KeyValueExpr) + if !ok || !r.holds(kv.Value) { + continue + } + if key, ok := kv.Key.(*ast.Ident); ok { + add(r.field(node, key.Name)) + } + } + case *ast.CallExpr: + r.intoParameters(node, add) + } + return true + }) + } + return learned +} + +// intoParameters adds a callee's parameters that receive a reader. +func (r *readerIdentity) intoParameters(call *ast.CallExpr, add func(types.Object)) { + fn, ok := r.object(call.Fun).(*types.Func) + if !ok || fn.Pkg() != r.pkg.Types { + return + } + decl := r.decl(fn) + if decl == nil || decl.Type.Params == nil { + return + } + index := 0 + for _, field := range decl.Type.Params.List { + for _, name := range field.Names { + if index < len(call.Args) && r.holds(call.Args[index]) { + add(r.pkg.TypesInfo.Defs[name]) + } + index++ + } + } +} + +// holds reports whether an expression evaluates to a reader: the object, a +// parenthesisation, a CONVERSION of one — which is the same function wearing a +// different type name — or an object already known to hold one. +func (r *readerIdentity) holds(expr ast.Expr) bool { + switch e := expr.(type) { + case *ast.ParenExpr: + return r.holds(e.X) + case *ast.Ident, *ast.SelectorExpr: + return r.readerAliases[r.object(expr)] + case *ast.CallExpr: + if tv, ok := r.pkg.TypesInfo.Types[e.Fun]; ok && tv.IsType() && len(e.Args) == 1 { + return r.holds(e.Args[0]) + } + } + return false +} + +// checkInvocations requires every resolved reader invocation's key to be +// traceable to constants. A keyless reader reads the whole environment, so +// there is no key: that read is the post-boot gate's finding, not this one's. +func (r *readerIdentity) checkInvocations() { + tracer := newKeyTracer(r.pkg) + for _, file := range r.files() { ast.Inspect(file, func(n ast.Node) bool { call, ok := n.(*ast.CallExpr) - if !ok || len(call.Args) == 0 || !readsEnvironmentCall(call) { + if !ok { + return true + } + if tv, ok := r.pkg.TypesInfo.Types[call.Fun]; ok && tv.IsType() { + return true // a conversion, not a call + } + callee := r.object(call.Fun) + if callee == nil || !r.readerAliases[callee] { return true } - // env's own body reads os.Getenv(key) on its parameter. It is the - // helper every traced key arrives through, so judging it here - // would be judging the mechanism rather than any key; its callers - // are what this gate follows. - if tracer.insideEnvHelper(call) { + if fn, ok := callee.(*types.Func); ok && keylessReaders[fn.Name()] { + return true + } + if len(call.Args) == 0 || tracer.insideEnvHelper(call) { return true } if why, ok := tracer.resolves(call.Args[0], 0); !ok { - findings = append(findings, pkg.Fset.Position(call.Pos()).String()+ - ": the key of this environment read does not resolve to constants ("+why+")") + r.unresolvedKeys = append(r.unresolvedKeys, + r.pkg.Fset.Position(call.Pos()).String()+": the key of this environment read does not resolve to constants ("+why+")") } return true }) } - sort.Strings(findings) - return findings + sort.Strings(r.unresolvedKeys) +} + +// object resolves an expression to the object it names. +func (r *readerIdentity) object(expr ast.Expr) types.Object { + switch e := unparen(expr).(type) { + case *ast.Ident: + if obj := r.pkg.TypesInfo.Uses[e]; obj != nil { + return obj + } + return r.pkg.TypesInfo.Defs[e] + case *ast.SelectorExpr: + if selection, ok := r.pkg.TypesInfo.Selections[e]; ok { + return selection.Obj() + } + return r.pkg.TypesInfo.Uses[e.Sel] + } + return nil +} + +// field is the struct field a composite literal key names. +func (r *readerIdentity) field(lit *ast.CompositeLit, name string) types.Object { + structType, ok := structTypeOf(r.pkg.TypesInfo.TypeOf(lit)) + if !ok { + return nil + } + for i := 0; i < structType.NumFields(); i++ { + if structType.Field(i).Name() == name { + return structType.Field(i) + } + } + return nil +} + +// decl is the declaration of one of this package's functions. +func (r *readerIdentity) decl(fn *types.Func) *ast.FuncDecl { + for _, file := range r.files() { + for _, d := range file.Decls { + if decl, ok := d.(*ast.FuncDecl); ok && decl.Name != nil && + r.pkg.TypesInfo.Defs[decl.Name] == fn { + return decl + } + } + } + return nil } // keyTracer follows an environment key back to the constants that reach it. @@ -448,139 +833,3 @@ func structTypeOf(t types.Type) (*types.Struct, bool) { structType, ok := t.Underlying().(*types.Struct) return structType, ok } - -// readsEnvironmentCall reports whether a call reads the process environment, -// by the callee's name — os.Getenv, os.LookupEnv, or this package's own env. -func readsEnvironmentCall(call *ast.CallExpr) bool { - switch fn := call.Fun.(type) { - case *ast.Ident: - return environmentReadFunctions[fn.Name] - case *ast.SelectorExpr: - return environmentReadFunctions[fn.Sel.Name] - } - return false -} - -// --- The fixture that escaped, as a compiled negative case --- - -// TestTheFacadeGatesCatchTheEscapingFixture drives both gates against the -// sources that defeated the hand evaluator, compiled into this very package. -// -// A fixture that only parsed would prove nothing — it is the compiler's -// folding of `string(rune(82))` and of a constant chain that makes these keys -// real, and a parse-only fixture folds nothing. gatePackages fails the load if -// it does not type-check, so this cannot quietly become a fixture about -// nothing. -// -// All of them in ONE overlay, because each load shells out to the go command: -// the shapes are independent declarations, and the rules return every finding, -// so one compile answers for all of them. -func TestTheFacadeGatesCatchTheEscapingFixture(t *testing.T) { - dir, err := filepath.Abs(".") - if err != nil { - t.Fatal(err) - } - fixture := filepath.Join(dir, "zz_facade_claim_fixture.go") - - source := `package solution - -// A constant conversion splicing the carrier's name. -func FacadeCredentialViaConversion() string { - const key = "CODEFLY__MODULE_" + string(rune(82)) + "EGISTRATION_SECRETS" - return env(key, "") -} - -// A claim path spliced the same way. -const facadeClaimPath = "/modules/" + string(rune(95)) + "register" - -func FacadeClaimPath() string { return facadeClaimPath } - -// A header assembled through a conversion. -func FacadeSecretHeader() string { - return "X-Codefly-Module-" + string(rune(83)) + "ecret" -} - -// Function-local constants, which a package-level-only collector never saw. -func FacadeClaimLocal(gatewayURL string) string { - const claim = "/modules/" + "_register" - return gatewayURL + claim -} - -func FacadeCredentialLocal() string { - const carrier = "CODEFLY__MODULE" + "_REGISTRATION_SECRETS" - return env(carrier, "") -} - -// A chain longer than any iteration limit, declared in reverse so a single -// forward pass resolves none of it. -const ( - chain00 = "/" + chain01 - chain01 = "m" + chain02 - chain02 = "o" + chain03 - chain03 = "d" + chain04 - chain04 = "u" + chain05 - chain05 = "l" + chain06 - chain06 = "e" + chain07 - chain07 = "s" + chain08 - chain08 = "/" + chain09 - chain09 = "_" + chain10 - chain10 = "r" + chain11 - chain11 = "e" + chain12 - chain12 = "g" + chain13 - chain13 = "i" + chain14 - chain14 = "s" + chain15 - chain15 = "t" + chain16 - chain16 = "e" + chain17 - chain17 = "r" -) - -func FacadeClaimChain() string { return chain00 } - -// And a key that is not constant at all. -func FacadeCredentialViaRuntimeKey(parts []string) string { - return env(joinedKey(parts), "") -} - -func joinedKey(parts []string) string { - out := "" - for _, part := range parts { - out += part - } - return out -} -` - - shapes := gateFindings(t, fixture, source, claimShapeFindings) - for _, want := range []string{ - "CODEFLY__MODULE_REGISTRATION_SECRETS", - "/modules/_register", - "X-Codefly-Module-Secret", - } { - if !strings.Contains(shapes, want) { - t.Errorf("the claim-shape gate did not catch %q.\nfindings:\n%s", want, shapes) - } - } - // Every spelling, not just one of each shape: the chain and the local - // constant both produce the claim path, so two findings name it. - if strings.Count(shapes, "/modules/_register") < 2 { - t.Errorf("the claim path was caught in only one spelling.\nfindings:\n%s", shapes) - } - - keys := gateFindings(t, fixture, source, unresolvedKeyFindings) - if !strings.Contains(keys, "does not resolve to constants") { - t.Errorf("the key gate did not catch a runtime-computed key.\nfindings:\n%s", keys) - } -} - -// And the gates must not fire on the package as it is, or the fixtures above -// prove nothing: a gate that fires on everything catches every fixture. -func TestTheFacadeGatesAreSilentOnThisPackage(t *testing.T) { - for _, pkg := range gatePackages(t, nil) { - if findings := claimShapeFindings(t, pkg); len(findings) > 0 { - t.Errorf("%s: the claim-shape gate fires on the package as it is: %s", pkg.PkgPath, strings.Join(findings, "; ")) - } - if findings := unresolvedKeyFindings(t, pkg); len(findings) > 0 { - t.Errorf("%s: the key gate fires on the package as it is: %s", pkg.PkgPath, strings.Join(findings, "; ")) - } - } -} From ea3e0f4d62b21d03a3cd491e552220bf599aa4f3 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 14:53:36 -0400 Subject: [PATCH 11/12] fix: one reader model answers both gates, so an aliased reader with a constant key is seen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The executed r2's two survivors, N5 and X1. X1 — a parenthesised `(env)(...)` with a runtime-assembled key — already died on the reader-identity key gate. N5 did not, and it is the more interesting one: type reviewEnvReader = func(string) string var reviewRead reviewEnvReader = os.Getenv func reviewAliasRead() string { return reviewRead("REVIEW_GATE_VALUE") } Called from handleHealth, so it runs per request. It passed both gates at once and neither was wrong on its own: the key is a perfectly good constant, so the key gate had nothing to say, and the call is indirect, so the CHA edge into os.Getenv was an over-approximation the serving gate discards — it has to, because that same over-approximation resolved namedInJSON's callback to os.LookupEnv. The two gates were asking different questions about what a read IS. Now one model answers both: which calls are reads comes from the identity model, which resolves the alias exactly, and the serving graph only says whether that call site runs after boot. Each invocation is attributed to the innermost reachable function containing it, so a read inside a closure is the closure's. Mutations, all red: N5, X1, R2Credential, R2LateHost, R2Mount, the exported closure, E2. Committed as fixtures: the aliased reader on a served path with a constant key, and the parenthesised runtime key beside the conversion, field and parameter cases. Co-Authored-By: Claude Opus 5 (1M context) --- environment_boundary_test.go | 107 +++++++++++++++++++++++++++++------ facade_claim_test.go | 37 +++++++++++- 2 files changed, 126 insertions(+), 18 deletions(-) diff --git a/environment_boundary_test.go b/environment_boundary_test.go index 22f4ccd..7d0bed7 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -154,6 +154,48 @@ var ExportedClosure = func() string { return (env)("ANYTHING", "") } } } +// And an aliased reader on a serving path, with a perfectly constant key. +// +// This is the shape that survived both gates at once: the key resolves, so the +// key gate has nothing to say about it, and the call is indirect, so the CHA +// edge into os.Getenv is an over-approximation the post-boot gate discards. +// Neither gate was wrong on its own — they were asking different questions +// about what a read is. One model answers both: the identity model resolves +// the alias exactly, and the serving graph says whether that call site runs +// after boot. +func TestThePostBootGateSeesAnAliasedReaderWithAConstantKey(t *testing.T) { + dir, err := filepath.Abs(".") + if err != nil { + t.Fatal(err) + } + fixture := filepath.Join(dir, "zz_aliased_serving_fixture.go") + + source := `package solution + +import ( + "net/http" + "os" +) + +type servingEnvReader = func(string) string + +var servingRead servingEnvReader = os.Getenv + +func servingAliasRead() string { return servingRead("ANYTHING") } + +// On a served path, so it runs per request rather than at boot. +func (s *Server) MountAliasFixture(mux *http.ServeMux) { + mux.HandleFunc("/alias", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("x-alias", servingAliasRead()) + }) +} +` + readers := servingReaders(t, gatePackages(t, map[string][]byte{fixture: []byte(source)})) + if len(readers) == 0 { + t.Fatal("an aliased reader with a constant key, called on a served path, was not reported") + } +} + // And os.Environ is a reader too, which is the omission that let an exported // wrapper of a function using it read changing configuration after boot. It // has no key to resolve — it reads all of them — so the read itself is the @@ -317,22 +359,14 @@ func servingReaders(t *testing.T, pkgs []*packages.Package) map[string]string { // reader on the path named four that are all somebody else's: the // SDK rechecking a rotated value, grpc's resolver, viper's init, // none of which this package could resolve in loadConfig. - if name, ok := environmentReader(next.Func); ok { - // Only a call that actually names the reader. CHA resolves an - // indirect call to every type-compatible function in the - // program, so a callback of shape func(string) (string, bool) - // resolves to os.LookupEnv whether or not anything ever puts - // it there — which is exactly what it did here, through - // namedInJSON's `named` parameter. - // - // A reader genuinely held in a value and called through it is - // not lost by this: that is the identity the key gate follows, - // precisely, and refuses when its key cannot be resolved. An - // over-approximated edge proves nothing and the precise gate - // is the one that owns that shape. - if inPackage(current.Func, target) && staticCall(edge, next.Func) { - readers[name] = path[current] + " → " + name - } + if _, isReader := environmentReader(next.Func); isReader { + // Reader edges are not taken from this graph. CHA resolves an + // indirect call to every type-compatible function, so a + // callback of shape func(string) (string, bool) resolves to + // os.LookupEnv whether or not anything puts it there — which + // is what it did, through namedInJSON's `named` parameter. + // Which calls are reads is the identity model's answer, below, + // and it is the same model the key gate uses. continue } seen[next] = true @@ -340,9 +374,50 @@ func servingReaders(t *testing.T, pkgs []*packages.Package) map[string]string { queue = append(queue, next) } } + + // Which calls are reads comes from the reader identity model, not from + // this graph: an aliased reader — `var read reviewEnvReader = os.Getenv`, + // called through `read` — is an indirect call CHA can only guess at, while + // the identity model resolves it exactly. Each invocation is attributed to + // the innermost function containing it, and reported when that function is + // reachable after boot. + for _, pkg := range pkgs { + if pkg.PkgPath != target { + continue + } + identity := newReaderIdentity(pkg) + for _, site := range identity.invocations { + holder := innermostContaining(seen, site.pos) + if holder == nil { + continue + } + readers[site.name] = path[holder] + " → " + site.name + } + } return readers } +// innermostContaining is the reachable function whose source extent contains a +// position most tightly, so a read inside a closure is attributed to the +// closure rather than to whatever encloses it. +func innermostContaining(reached map[*callgraph.Node]bool, pos token.Pos) *callgraph.Node { + var best *callgraph.Node + var bestSize token.Pos + for node := range reached { + syntax := node.Func.Syntax() + if syntax == nil { + continue + } + if pos < syntax.Pos() || pos >= syntax.End() { + continue + } + if size := syntax.End() - syntax.Pos(); best == nil || size < bestSize { + best, bestSize = node, size + } + } + return best +} + // isPostBootRoot reports whether a function can run after boot. func isPostBootRoot(fn *ssa.Function) bool { if fn.Synthetic == "package initializer" || fn.Name() == "init" { diff --git a/facade_claim_test.go b/facade_claim_test.go index ffc2296..aea63dd 100644 --- a/facade_claim_test.go +++ b/facade_claim_test.go @@ -3,6 +3,7 @@ package solution import ( "go/ast" "go/constant" + "go/token" "go/types" "path/filepath" "sort" @@ -330,6 +331,12 @@ func aliasedThroughAParameter(read func(string) string, parts []string) string { return read(strings.Join(parts, "_")) } +// And parenthesised, which returned false from a rule reading the immediate +// spelling and so skipped the key check entirely. +func AliasedThroughParentheses(parts []string) string { + return (env)(strings.Join(parts, "_"), "") +} + func AliasedCaller(parts []string) string { return aliasedThroughAParameter(os.Getenv, parts) } @@ -337,8 +344,8 @@ func AliasedCaller(parts []string) string { keys := gateFindings(t, fixture, source, unresolvedKeyFindings) // One finding per invocation: the alias, the field and the parameter. - if got := strings.Count(keys, "does not resolve to constants"); got < 3 { - t.Errorf("the key gate caught %d of the three aliased reader invocations.\nfindings:\n%s", got, keys) + if got := strings.Count(keys, "does not resolve to constants"); got < 4 { + t.Errorf("the key gate caught %d of the four aliased reader invocations.\nfindings:\n%s", got, keys) } } @@ -376,6 +383,17 @@ type readerIdentity struct { // unresolvedKeys is the findings: a resolved reader invocation whose key // is not traceable to constants. unresolvedKeys []string + // invocations is every resolved reader invocation, wherever it is. The + // post-boot gate uses these rather than its own notion of a reader call: + // one model, so an alias the key gate resolves is an alias that gate sees + // too. + invocations []readerSite +} + +// readerSite is one resolved reader invocation. +type readerSite struct { + pos token.Pos + name string } func newReaderIdentity(pkg *packages.Package) *readerIdentity { @@ -525,6 +543,7 @@ func (r *readerIdentity) checkInvocations() { if callee == nil || !r.readerAliases[callee] { return true } + r.invocations = append(r.invocations, readerSite{pos: call.Pos(), name: readerDisplayName(callee)}) if fn, ok := callee.(*types.Func); ok && keylessReaders[fn.Name()] { return true } @@ -541,6 +560,20 @@ func (r *readerIdentity) checkInvocations() { sort.Strings(r.unresolvedKeys) } +// readerDisplayName names a reader for a finding. +func readerDisplayName(obj types.Object) string { + if fn, ok := obj.(*types.Func); ok && fn.Pkg() != nil { + if fn.Pkg().Path() == "github.com/codefly-dev/solution-runtime-go" { + return fn.Name() + } + return fn.Pkg().Name() + "." + fn.Name() + } + if obj != nil { + return obj.Name() + } + return "an environment reader" +} + // object resolves an expression to the object it names. func (r *readerIdentity) object(expr ast.Expr) types.Object { switch e := unparen(expr).(type) { From e354557b7087368d43030a3e6d9e6651a688a2e7 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Tue, 6 Oct 2026 15:15:43 -0400 Subject: [PATCH 12/12] perf: build SSA for this package only, and compile each fixture set in one overlay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed the five-minute gate at 300.306s. The two new gates were the slowest tests in the suite — 24.4s and 16.3s under -race — because each built SSA for the whole dependency closure. That bought nothing. The graph is used for in-package reachability, and re-entry into this package from a dependency always happens through a function whose address was taken: net/http invoking a mounted handler IS that, and address-taken functions are already roots. Reader identification never came from this graph. So SSA is built for this package alone and the walk stays in-package: 4.7s and 5.0s, with every mutant still dying — N5, X1, R2Mount, the exported closure, R2LateHost, E2, E3 and C5 all re-checked after the narrowing, because a smaller graph is exactly the change that could have quietly lost one. The fixtures also compile in one overlay per gate rather than one per shape, each load being a go command invocation: seven serving shapes in one file, five untraceable-key spellings in another, both rules run against a single load. Suite 199s, against main's own 221s on this machine. The margin matters: main is close enough to the gate that the next test anyone adds will trip it, and this branch now leaves more room than it found. Co-Authored-By: Claude Opus 5 (1M context) --- environment_boundary_test.go | 122 ++++++++++++++--------------------- facade_claim_test.go | 116 ++++++++++++++------------------- 2 files changed, 96 insertions(+), 142 deletions(-) diff --git a/environment_boundary_test.go b/environment_boundary_test.go index 7d0bed7..d4ab3e0 100644 --- a/environment_boundary_test.go +++ b/environment_boundary_test.go @@ -92,9 +92,11 @@ func TestNoEnvironmentReadIsReachableAfterBoot(t *testing.T) { } // And the gate has to be able to see a read at all, or its silence means -// nothing. Three shapes, each the one a previous version of this gate lost: -// an exported function reading directly, a handler reached only as a value -// mounted on a mux, and a method value stored in a package-level struct. +// nothing. Every serving shape a previous version of this gate lost, in ONE +// compiled fixture: each load shells out to the go command and builds SSA for +// the whole program, so one overlay answers for all of them. Each shape has +// its own reader, and each is asserted by name, so a gate that followed one +// edge kind and missed the others fails. func TestThePostBootGateSeesEveryServingShape(t *testing.T) { dir, err := filepath.Abs(".") if err != nil { @@ -104,7 +106,11 @@ func TestThePostBootGateSeesEveryServingShape(t *testing.T) { source := `package solution -import "net/http" +import ( + "context" + "net/http" + "os" +) // A parenthesised callee on an exported path. func ServedParenthesisedRead() string { return (env)("ANYTHING", "") } @@ -119,17 +125,17 @@ func fixtureHandler(w http.ResponseWriter, _ *http.Request) { w.Header().Set("x-fixture", fixtureMountedRead()) } -func fixtureMountedRead() string { return env("ANYTHING", "") } +func fixtureMountedRead() string { return os.Getenv("ANYTHING") } // A method value stored in a package-level struct and mounted from there, so -// the only edge to it is through the stored field. +// the only edge to it is the stored field. type fixtureReceiver struct{} func (fixtureReceiver) fixtureServe(w http.ResponseWriter, _ *http.Request) { w.Header().Set("x-stored", fixtureStoredRead()) } -func fixtureStoredRead() string { return env("ANYTHING", "") } +func fixtureStoredRead() string { return os.LookupEnvValue("ANYTHING") } var fixtureHandlers = struct { h func(http.ResponseWriter, *http.Request) @@ -141,85 +147,46 @@ func MountStoredFixture(mux *http.ServeMux) { // An exported variable holding a closure that reads. var ExportedClosure = func() string { return (env)("ANYTHING", "") } -` - - readers := servingReaders(t, gatePackages(t, map[string][]byte{fixture: []byte(source)})) - if len(readers) == 0 { - t.Fatal("the gate reported no post-boot reader for a fixture that reads on four serving paths") - } - // env is the reader every one of these reaches, so its path has to come - // back; the path itself is what says WHICH shape was followed. - if _, ok := readers["env"]; !ok { - t.Errorf("the gate did not report env: readers=%v", readers) - } -} - -// And an aliased reader on a serving path, with a perfectly constant key. -// -// This is the shape that survived both gates at once: the key resolves, so the -// key gate has nothing to say about it, and the call is indirect, so the CHA -// edge into os.Getenv is an over-approximation the post-boot gate discards. -// Neither gate was wrong on its own — they were asking different questions -// about what a read is. One model answers both: the identity model resolves -// the alias exactly, and the serving graph says whether that call site runs -// after boot. -func TestThePostBootGateSeesAnAliasedReaderWithAConstantKey(t *testing.T) { - dir, err := filepath.Abs(".") - if err != nil { - t.Fatal(err) - } - fixture := filepath.Join(dir, "zz_aliased_serving_fixture.go") - - source := `package solution - -import ( - "net/http" - "os" -) +// An ALIASED reader with a perfectly constant key, on a served path. This is +// the shape that passed both gates at once: the key resolves, so the key gate +// has nothing to say, and the call is indirect, so a CHA edge into os.Getenv +// is an over-approximation the serving graph discards. type servingEnvReader = func(string) string var servingRead servingEnvReader = os.Getenv func servingAliasRead() string { return servingRead("ANYTHING") } -// On a served path, so it runs per request rather than at boot. func (s *Server) MountAliasFixture(mux *http.ServeMux) { mux.HandleFunc("/alias", func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("x-alias", servingAliasRead()) }) } -` - readers := servingReaders(t, gatePackages(t, map[string][]byte{fixture: []byte(source)})) - if len(readers) == 0 { - t.Fatal("an aliased reader with a constant key, called on a served path, was not reported") - } -} - -// And os.Environ is a reader too, which is the omission that let an exported -// wrapper of a function using it read changing configuration after boot. It -// has no key to resolve — it reads all of them — so the read itself is the -// finding. -func TestTheWholeEnvironmentCountsAsARead(t *testing.T) { - dir, err := filepath.Abs(".") - if err != nil { - t.Fatal(err) - } - fixture := filepath.Join(dir, "zz_environ_fixture.go") - - source := `package solution - -import "context" +// And the whole environment at once, through a function boot also uses: // discoverHostModule scans os.Environ, so an exported wrapper of it reads -// changing configuration after boot. +// changing configuration after boot. os.Environ was missing from the reader +// set entirely. func ExportedLateHost() string { return discoverHostModule(context.Background(), "auth-gateway", "rest", "rest") } ` + // os.LookupEnvValue does not exist; the fixture must compile, so use the + // real reader. + source = strings.ReplaceAll(source, "os.LookupEnvValue(", "os.Getenv(") + readers := servingReaders(t, gatePackages(t, map[string][]byte{fixture: []byte(source)})) if len(readers) == 0 { - t.Fatal("an exported wrapper of a function scanning the whole environment was not reported") + t.Fatal("the gate reported no post-boot reader for a fixture that reads on seven serving paths") + } + // Both readers these shapes reach, so a gate that saw one kind of read and + // not the other cannot pass: env for the in-package helper, os.Getenv for + // the direct and aliased ones, os.Environ for the whole-environment scan. + for _, want := range []string{"env", "os.Getenv", "os.Environ"} { + if _, ok := readers[want]; !ok { + t.Errorf("the gate did not report %s: readers=%v", want, readers) + } } } @@ -292,7 +259,14 @@ var keylessReaders = map[string]bool{"Environ": true, "ExpandEnv": true} func servingReaders(t *testing.T, pkgs []*packages.Package) map[string]string { t.Helper() - prog, _ := ssautil.AllPackages(pkgs, 0) + // SSA for THIS package only, not the whole dependency closure. Building + // everything cost 40s of the suite under -race and bought nothing: the + // graph is used for in-package reachability, and re-entry into this + // package from a dependency always happens through a function whose + // address was taken — net/http invoking a mounted handler is exactly that + // — which is already a root. Reader identification does not come from + // this graph at all. + prog, _ := ssautil.Packages(pkgs, 0) prog.Build() graph := cha.CallGraph(prog) graph.DeleteSyntheticNodes() @@ -359,14 +333,12 @@ func servingReaders(t *testing.T, pkgs []*packages.Package) map[string]string { // reader on the path named four that are all somebody else's: the // SDK rechecking a rotated value, grpc's resolver, viper's init, // none of which this package could resolve in loadConfig. - if _, isReader := environmentReader(next.Func); isReader { - // Reader edges are not taken from this graph. CHA resolves an - // indirect call to every type-compatible function, so a - // callback of shape func(string) (string, bool) resolves to - // os.LookupEnv whether or not anything puts it there — which - // is what it did, through namedInJSON's `named` parameter. - // Which calls are reads is the identity model's answer, below, - // and it is the same model the key gate uses. + // Which calls are reads is the identity model's answer, below, + // and it is the same model the key gate uses: CHA resolves an + // indirect call to every type-compatible function, so a callback + // of shape func(string) (string, bool) resolves to os.LookupEnv + // whether or not anything puts it there. + if !inPackage(next.Func, target) { continue } seen[next] = true diff --git a/facade_claim_test.go b/facade_claim_test.go index aea63dd..b742b4f 100644 --- a/facade_claim_test.go +++ b/facade_claim_test.go @@ -198,6 +198,8 @@ func TestTheFacadeGatesCatchTheEscapingFixture(t *testing.T) { source := `package solution +import "os" + // A constant conversion splicing the carrier's name. func FacadeCredentialViaConversion() string { const key = "CODEFLY__MODULE_" + string(rune(82)) + "EGISTRATION_SECRETS" @@ -255,6 +257,38 @@ func FacadeCredentialViaRuntimeKey(parts []string) string { return env(joinedKey(parts), "") } +// The reader's identity hidden behind a local function type: the callee is +// ` + "`read`" + `, so no rule about callee names sees a read at all, and the key is +// never examined. +type aliasedReader = func(string) string + +func AliasedCredential(parts []string) string { + read := aliasedReader(osGetenv) + return read(joinedKey(parts)) +} + +// Through a struct field, through a parameter, and parenthesised. +type readerBox struct{ read func(string) string } + +func AliasedThroughAField(parts []string) string { + box := readerBox{read: osGetenv} + return box.read(joinedKey(parts)) +} + +func aliasedThroughAParameter(read func(string) string, parts []string) string { + return read(joinedKey(parts)) +} + +func AliasedCaller(parts []string) string { + return aliasedThroughAParameter(osGetenv, parts) +} + +func AliasedThroughParentheses(parts []string) string { + return (env)(joinedKey(parts), "") +} + +var osGetenv = os.Getenv + func joinedKey(parts []string) string { out := "" for _, part := range parts { @@ -264,7 +298,16 @@ func joinedKey(parts []string) string { } ` - shapes := gateFindings(t, fixture, source, claimShapeFindings) + // One load for both rules: each shells out to the go command, and the + // fixture is the same package either way. + shapes, keys := "", "" + for _, pkg := range gatePackages(t, map[string][]byte{fixture: []byte(source)}) { + if pkg.PkgPath != "github.com/codefly-dev/solution-runtime-go" { + continue + } + shapes = strings.Join(claimShapeFindings(t, pkg), "\n") + keys = strings.Join(unresolvedKeyFindings(t, pkg), "\n") + } for _, want := range []string{ "CODEFLY__MODULE_REGISTRATION_SECRETS", "/modules/_register", @@ -280,72 +323,11 @@ func joinedKey(parts []string) string { t.Errorf("the claim path was caught in only one spelling.\nfindings:\n%s", shapes) } - keys := gateFindings(t, fixture, source, unresolvedKeyFindings) - if !strings.Contains(keys, "does not resolve to constants") { - t.Errorf("the key gate did not catch a runtime-computed key.\nfindings:\n%s", keys) - } -} - -// TestTheKeyGateCatchesAnAliasedReader drives the key gate against the reader -// whose identity is hidden behind a local function type. -// -// This is the shape that defeated recognising readers by callee name: the -// callee is `read`, so no rule about names sees a read, and the key — the -// forbidden carrier assembled at run time — is never examined. No constant -// expression in the fixture contains the complete key either, so the -// claim-shape gate cannot be what catches it. Only the reader's identity can. -func TestTheKeyGateCatchesAnAliasedReader(t *testing.T) { - dir, err := filepath.Abs(".") - if err != nil { - t.Fatal(err) - } - fixture := filepath.Join(dir, "zz_aliased_reader_fixture.go") - - source := `package solution - -import ( - "os" - "strings" -) - -type aliasedReader = func(string) string - -func AliasedCredential() string { - read := aliasedReader(os.Getenv) - return read(strings.Join( - []string{"CODEFLY", "", "MODULE", "REGISTRATION", "SECRETS"}, - "_", - )) -} - -// And through a struct field, and through a parameter, which are the other -// two ways the identity travels. -type readerBox struct{ read func(string) string } - -func AliasedThroughAField(parts []string) string { - box := readerBox{read: os.Getenv} - return box.read(strings.Join(parts, "_")) -} - -func aliasedThroughAParameter(read func(string) string, parts []string) string { - return read(strings.Join(parts, "_")) -} - -// And parenthesised, which returned false from a rule reading the immediate -// spelling and so skipped the key check entirely. -func AliasedThroughParentheses(parts []string) string { - return (env)(strings.Join(parts, "_"), "") -} - -func AliasedCaller(parts []string) string { - return aliasedThroughAParameter(os.Getenv, parts) -} -` - - keys := gateFindings(t, fixture, source, unresolvedKeyFindings) - // One finding per invocation: the alias, the field and the parameter. - if got := strings.Count(keys, "does not resolve to constants"); got < 4 { - t.Errorf("the key gate caught %d of the four aliased reader invocations.\nfindings:\n%s", got, keys) + // Four aliased reader invocations: through a conversion, a struct field, + // a parameter, and parenthesised — plus the runtime-joined key. Each is a + // spelling that defeated recognising readers by callee name. + if got := strings.Count(keys, "does not resolve to constants"); got < 5 { + t.Errorf("the key gate caught %d of the five untraceable-key invocations.\nfindings:\n%s", got, keys) } }