diff --git a/src/internal/config/config_test.go b/src/internal/config/config_test.go index 286e4e2..3c28665 100644 --- a/src/internal/config/config_test.go +++ b/src/internal/config/config_test.go @@ -551,7 +551,6 @@ func TestValidate_ServiceIdentityFieldsRejectedOnBrowserClients(t *testing.T) { mutate func(*config.ClientConfig) want string }{ - "tenant": {func(c *config.ClientConfig) { c.Tenant = "tenant:platform" }, "tenant"}, "serviceSubject": {func(c *config.ClientConfig) { c.ServiceSubject = "service:approver" }, "serviceSubject"}, "roles": {func(c *config.ClientConfig) { c.Roles = []string{"approver"} }, "roles"}, } @@ -580,13 +579,22 @@ func TestValidate_ServiceIdentityFieldsRejectedOnBrowserClients(t *testing.T) { cfg := validConfig(writeTempFile(t, "key")) client := browserClient() client.GrantTypes = nil - client.Tenant = "tenant:platform" - if errs := config.ValidateConfig(cfg); len(errs) != 0 { - t.Fatalf("baseline config invalid: %v", errs) - } + client.ServiceSubject = "service:approver" cfg.Clients = append(cfg.Clients, client) if errs := config.ValidateConfig(cfg); len(errs) == 0 { - t.Fatal("expected tenant to be rejected on an implicit authorization-code client") + t.Fatal("expected serviceSubject to be rejected on an implicit authorization-code client") + } + }) + + // A browser client MAY declare a tenant: humanTenant resolves it against the + // directory (KEY-WP-0013-T05). Rejecting it here would break that. + t.Run("tenant is allowed on a browser client", func(t *testing.T) { + cfg := validConfig(writeTempFile(t, "key")) + client := browserClient() + client.Tenant = "tenant:platform" + cfg.Clients = append(cfg.Clients, client) + if errs := config.ValidateConfig(cfg); len(errs) != 0 { + t.Fatalf("browser client tenant rejected: %v", errs) } }) diff --git a/src/internal/config/validate.go b/src/internal/config/validate.go index b9954a4..a787173 100644 --- a/src/internal/config/validate.go +++ b/src/internal/config/validate.go @@ -93,18 +93,16 @@ func ValidateConfig(cfg *Config) []string { if c.TokenLifetime != "" { errs = append(errs, prefix+": tokenLifetime is only supported for client_credentials clients") } - // Service-identity fields are read only on the client_credentials - // path. On a browser client they are silently ignored, and the - // tenant one is a trap worth failing on: an approver client - // registered with tenant: tenant:platform starts cleanly and then - // issues tokens carrying the directory tenant, which a resource - // server comparing tenant exactly will refuse. That surfaces as a - // mysterious failed approval, not as a registration defect - // (KEY-WP-0028). Human tenant comes from the directory user; see - // docs/approval-engine-auth-contract.md. - if c.Tenant != "" { - errs = append(errs, prefix+": tenant is only read for client_credentials clients; a browser client's tenant claim comes from the directory user, so this value would be silently ignored") - } + // serviceSubject and roles are read only on the client_credentials + // path, so on a browser client they are silently ignored: the + // subject and roles come from the directory user. Rejecting them + // turns a registration that looks effective into a startup error + // (KEY-WP-0028). + // + // tenant is deliberately NOT in this list. A browser client may + // declare one, and humanTenant resolves it against the directory -- + // see docs/tenant-claim-contract.md, "How a human token's tenant is + // resolved". if c.ServiceSubject != "" { errs = append(errs, prefix+": serviceSubject is only read for client_credentials clients; a browser client's subject is the directory user") } diff --git a/workplans/KEY-WP-0028-browser-client-field-validation.md b/workplans/KEY-WP-0028-browser-client-field-validation.md new file mode 100644 index 0000000..83fc122 --- /dev/null +++ b/workplans/KEY-WP-0028-browser-client-field-validation.md @@ -0,0 +1,66 @@ +--- +id: KEY-WP-0028 +type: workplan +title: "Reject service-identity fields that a browser client silently ignores" +domain: infotech +repo: key-cape +status: finished +owner: claude +topic_slug: browser-client-field-validation +created: "2026-09-09" +updated: "2026-09-09" +--- + +`serviceSubject` and `roles` are read only on the `client_credentials` path. On a +browser client they are accepted at startup and then ignored: the subject and +roles come from the directory user. A registration that looks effective and is +not fails later, somewhere else, as a downstream rejection rather than as a +registration defect. + +`tokenLifetime` was already rejected this way for non-service clients, so the +rule exists — it was just incomplete. + +## Reject the ignored fields + +```task +id: KEY-WP-0028-T01 +status: done +priority: medium +``` + +Config validation now rejects `serviceSubject` and `roles` on any client that +does not use `client_credentials`, naming where the value actually comes from. +Nothing in `config/dev-config.yaml`, the example fixture or the live deployment +sets either field on a browser client, so this breaks no existing configuration — +checked against all three rather than assumed. + +**`tenant` is deliberately excluded from the rule**, and a test pins that. A +browser client may declare a tenant; `humanTenant` resolves it against the +directory and refuses issuance when the two disagree (KEY-WP-0013-T05). An +earlier version of this change rejected `tenant` too, which would have made that +feature unusable. + +## Correction + +```task +id: KEY-WP-0028-T02 +status: done +priority: medium +``` + +This work started from KEY-WP-0013-T05's blocker paragraph, which described a +human token as unable to carry `tenant:platform`. That was true when written and +had already been fixed in a concurrent session by the time this task began; the +paragraph was read as current state rather than as a dated record. + +Two consequences, both corrected within the session: + +- the `tenant` rejection was removed before it was committed, and a test now + asserts a browser client carrying `tenant: tenant:platform` validates cleanly; +- a hub decision (`0145ab57`) recording the tenant question as open and + unimplemented was withdrawn with a rationale pointing at the implementation. + Nothing was built on it. + +The lesson worth keeping: in a repository where several sessions work at once, +blocker prose ages faster than the code it describes. Re-read the source before +acting on a workplan's description of it.