From 6c16f88c1c9ece6b184cb9d6196d1f14de4162d8 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 9 Nov 2026 13:23:31 +0800 Subject: [PATCH 1/2] ci: add a weekly mutation check for the security checks' tests Add mutation.yml, which re-applies a curated list of mutations to the library's security checks and fails if any mutant survives, so a refactor can't quietly remove the test protection a check has. - .github/mutation/mutants.json lists each mutant: the check's file, a literal snippet that must occur exactly once there (never a line number), its replacement, the packages and tests that must fail, and what the check guards. - .github/scripts/mutation-check.go (go run, build-tagged ignore so it stays out of the module's packages) runs a baseline of every listed test unmutated, checks each run filter names a real test, then applies each mutant, runs its tests and restores the file from the copy read before mutating it. A stale anchor, a mutant that doesn't compile or one that survives fails the run; the job summary gets a table of every result. - The list covers the checks given tests in #808, #809 and #810 (with boundary mutants where those tests pin a boundary), the security checks already protected (signature checks, caller key bounds, sealed-cookie key ID and expiry, code-exchange grant revocation, opaque-token issuer, notifier hardening, vouched keys at later chain hops) and sqlstore's PostgreSQL concurrency checks, against a PostgreSQL service container. - Runs weekly, on demand, and on pull requests that change the list or the runner. It isn't a required check. - check-postgres-pin.sh now checks every workflow's Postgres service image against .github/postgres/Dockerfile, mutation.yml's included. - CONTRIBUTING.md says a change moving an anchored check updates its entry, and how to run the list locally. Co-Authored-By: Claude Opus 5.5 --- .github/mutation/mutants.json | 658 ++++++++++++++++++++++++++ .github/postgres/Dockerfile | 10 +- .github/scripts/check-postgres-pin.sh | 40 +- .github/scripts/mutation-check.go | 406 ++++++++++++++++ .github/workflows/ci.yml | 12 +- .github/workflows/mutation.yml | 81 ++++ CONTRIBUTING.md | 8 + 7 files changed, 1187 insertions(+), 28 deletions(-) create mode 100644 .github/mutation/mutants.json create mode 100644 .github/scripts/mutation-check.go create mode 100644 .github/workflows/mutation.yml diff --git a/.github/mutation/mutants.json b/.github/mutation/mutants.json new file mode 100644 index 00000000..7570806d --- /dev/null +++ b/.github/mutation/mutants.json @@ -0,0 +1,658 @@ +[ + { + "id": "fed-leaf-vouched-jwks", + "file": "federation/resolver.go", + "old": "belowClaims, err := r.verifyAgainstJWKS(st.belowStmt, sup.aboveStmt.ClaimedJWKS(),", + "new": "belowClaims, err := r.verifyAgainstJWKS(st.belowStmt, func() json.RawMessage { if hop == 0 { return st.belowStmt.ClaimedJWKS() }; return sup.aboveStmt.ClaimedJWKS() }(),", + "module": "root", + "packages": [ + "./federation" + ], + "run": "^TestResolveRejectsLeafNotSignedByIntermediateVouchedKey$", + "why": "a leaf's Entity Configuration verifies against the keys its Intermediate vouches for, not its own" + }, + { + "id": "fed-later-hop-vouched-jwks", + "file": "federation/resolver.go", + "old": "belowClaims, err := r.verifyAgainstJWKS(st.belowStmt, sup.aboveStmt.ClaimedJWKS(),", + "new": "belowClaims, err := r.verifyAgainstJWKS(st.belowStmt, func() json.RawMessage { if hop > 0 { return st.belowStmt.ClaimedJWKS() }; return sup.aboveStmt.ClaimedJWKS() }(),", + "module": "root", + "packages": [ + "./federation" + ], + "run": "^(TestChainExpiresWithAnIntermediateSubordinateStatement|TestResolveMaxPathLengthCountsSuperiors)$", + "why": "a Subordinate Statement above the leaf verifies against the keys its superior vouches for" + }, + { + "id": "fed-superior-self-signature", + "file": "federation/resolver.go", + "old": "claims, err := r.verifySelfSigned(config, hint, now)\n\tif err != nil {", + "new": "claims, err := r.verifySelfSigned(config, hint, now)\n\tif err != nil && false {", + "module": "root", + "packages": [ + "./federation" + ], + "run": "^TestResolveRejectsSuperiorEntityConfigurationWithBadSelfSignature$", + "why": "a superior's Entity Configuration must verify against its own JWKS" + }, + { + "id": "fed-superior-metadata-undeclared-type", + "file": "federation/resolver.go", + "old": "override, ok := superior[entityType]\n\t\tif !ok {", + "new": "override, ok := superior[entityType]\n\t\tif !ok && false {", + "module": "root", + "packages": [ + "./federation" + ], + "run": "^TestResolveIgnoresSuperiorMetadataForUndeclaredEntityType$", + "why": "superior metadata for an Entity Type the leaf doesn't declare is ignored" + }, + { + "id": "fed-tls-client-auth-usable-key", + "file": "federation/automatic_registration.go", + "old": "if len(thumbprints) == 0 {\n\t\treturn nil, fmt.Errorf(\"the RP JWK Set has no usable key", + "new": "if false {\n\t\treturn nil, fmt.Errorf(\"the RP JWK Set has no usable key", + "module": "root", + "packages": [ + "./federation" + ], + "run": "^TestRegisteredClientConfigFromMetadataRejectsTLSClientAuthWithoutUsableKey$", + "why": "a tls_client_auth RP must publish a signing key its certificate can carry" + }, + { + "id": "fed-json-depth-cap", + "file": "internal/federation/claims_validation.go", + "old": "depth >= maxJSONDepth", + "new": "false", + "module": "root", + "packages": [ + "./internal/federation" + ], + "run": "^TestParseRefusesStatementsSection32Rejects$", + "why": "entity statement JSON nesting is capped at maxJSONDepth" + }, + { + "id": "fed-json-depth-cap-boundary", + "file": "internal/federation/claims_validation.go", + "old": "depth >= maxJSONDepth", + "new": "depth > maxJSONDepth", + "module": "root", + "packages": [ + "./internal/federation" + ], + "run": "^TestParseRefusesStatementsSection32Rejects$", + "why": "the JSON nesting cap applies at exactly maxJSONDepth" + }, + { + "id": "fed-json-depth-cap-below", + "file": "internal/federation/claims_validation.go", + "old": "depth >= maxJSONDepth", + "new": "depth >= maxJSONDepth-1", + "module": "root", + "packages": [ + "./internal/federation" + ], + "run": "^TestParseRefusesStatementsSection32Rejects$", + "why": "JSON nested one level below maxJSONDepth is accepted" + }, + { + "id": "rar-unregistered-granted-type", + "file": "extension/rar.go", + "old": "def, ok := r.byType[typ]\n\t\tif !ok {\n\t\t\treturn fmt.Errorf(\"%w: %q\", ErrRARUnregisteredType, typ)", + "new": "def, ok := r.byType[typ]\n\t\tif !ok && false {\n\t\t\treturn fmt.Errorf(\"%w: %q\", ErrRARUnregisteredType, typ)", + "module": "root", + "packages": [ + "./extension" + ], + "run": "^TestRARRegistryValidateRefusesUnregisteredGrantedType$", + "why": "ValidateGrant/ValidateNarrowing refuse a granted RAR type the registry never registered" + }, + { + "id": "extension-definition-validate", + "file": "extension/definition.go", + "old": "if err := d.Validate(v); err != nil {", + "new": "if err := d.Validate(v); err != nil && false {", + "module": "root", + "packages": [ + "./extension" + ], + "run": "^TestRegistryParseRefusesValueValidateRejects$", + "why": "Registry.Parse applies the Definition's own Validate (an integrator's business limit)" + }, + { + "id": "sqlstore-strictjson-lone-surrogate", + "file": "sqlstore/strictjson.go", + "old": "high && i > 0", + "new": "false", + "module": "sqlstore", + "packages": [ + "." + ], + "run": "^TestParseClientConfigRefusals$", + "why": "a high surrogate escape followed by text is refused" + }, + { + "id": "srv-cc-scope-allowlist", + "file": "server/client_credentials.go", + "old": "if !s.clientAllowsScope(client, tok) {", + "new": "if false {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestRequestClientCredentialsTokenRejectsUnregisteredScope$", + "why": "client_credentials only issues scopes registered for the client" + }, + { + "id": "srv-par-request-object-alg", + "file": "server/par.go", + "old": "if !s.cfg.Algorithms.RequestObject.Contains(alg) {", + "new": "if false {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestPushAuthorizationRequestRejectsRequestObjectAlgorithmNotEnabledServerWide$", + "why": "PAR refuses a request-object algorithm the server hasn't enabled" + }, + { + "id": "srv-ciba-request-object-alg", + "file": "server/backchannel_authentication.go", + "old": "if !s.cfg.Algorithms.BackchannelAuthenticationRequest.Contains(alg) {", + "new": "if false {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestBeginBackchannelAuthenticationRejectsAlgorithmNotEnabledServerWide$", + "why": "CIBA refuses a signed-request algorithm the server hasn't enabled" + }, + { + "id": "srv-idtoken-enc-content-alg", + "file": "server/idtoken_encryption.go", + "old": "if !s.cfg.Algorithms.IDTokenEncryptionContentEncryption.Contains(contentEncryption) {", + "new": "if false {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestExchangeAuthorizationCodeRejectsIDTokenContentEncryptionNotPermittedByServer$", + "why": "ID token encryption refuses an enc the server hasn't enabled" + }, + { + "id": "srv-userinfo-enc-content-alg", + "file": "server/userinfo.go", + "old": "if !s.cfg.Algorithms.UserInfoEncryptionContentEncryption.Contains(contentEncryption) {", + "new": "if false {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestSignUserInfoResponseRejectsContentEncryptionNotPermittedByServer$", + "why": "UserInfo encryption refuses an enc the server hasn't enabled" + }, + { + "id": "srv-ciba-dpop-nonce", + "file": "server/backchannel_authentication.go", + "old": "if s.deps.Nonces != nil {\n\t\tif challenge := s.checkDPoPNonce(", + "new": "if s.deps.Nonces != nil && false {\n\t\tif challenge := s.checkDPoPNonce(", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestBackchannelAuthenticationChallengesDPoPNonce$", + "why": "the backchannel authentication endpoint enforces DPoP nonces" + }, + { + "id": "srv-access-token-size-cap", + "file": "server/accesstoken.go", + "old": "s.cfg.Limits.MaxAccessTokenBytes(); size > limit {", + "new": "s.cfg.Limits.MaxAccessTokenBytes(); false {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestIssuedAccessTokenBoundedForTheResourceServer$", + "why": "an issued access token over Limits.MaxAccessTokenBytes is refused" + }, + { + "id": "srv-access-token-size-cap-boundary", + "file": "server/accesstoken.go", + "old": "s.cfg.Limits.MaxAccessTokenBytes(); size > limit {", + "new": "s.cfg.Limits.MaxAccessTokenBytes(); size >= limit {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestIssuedAccessTokenBoundedForTheResourceServer$", + "why": "an access token exactly at Limits.MaxAccessTokenBytes is issued" + }, + { + "id": "srv-interaction-expiry", + "file": "server/complete_authorization.go", + "old": "if !now.Before(completed.ExpiresAt) {", + "new": "if _ = now; false {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestCompleteAuthorizationRejectsHandleExpiredByServerClock$", + "why": "an interaction handle past its expiry by the server clock is refused" + }, + { + "id": "srv-interaction-expiry-boundary", + "file": "server/complete_authorization.go", + "old": "if !now.Before(completed.ExpiresAt) {", + "new": "if now.After(completed.ExpiresAt) {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestCompleteAuthorizationRejectsHandleExpiredByServerClock$", + "why": "an interaction handle is refused at exactly its expiry" + }, + { + "id": "srv-mtls-dn-non-string", + "file": "server/client_auth_mtls.go", + "old": "if _, ok := atv.Value.(string); !ok {", + "new": "if _, ok := atv.Value.(string); !ok && false {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestMatchesRegisteredSubjectDNNonStringValueAnyToolchain$", + "why": "a certificate subject attribute that isn't a string isn't matched through String()" + }, + { + "id": "srv-code-grant-revoked", + "file": "server/token.go", + "old": "if revErr := s.checkGrantNotRevoked(ctx, grant); revErr != nil {\n\t\treturn grantRecord{}, revErr", + "new": "if revErr := s.checkGrantNotRevoked(ctx, grant); false && revErr != nil {\n\t\treturn grantRecord{}, revErr", + "module": "root", + "packages": [ + "./fapitest" + ], + "run": "^TestRevokeGrantBeforeTheCodeIsRedeemed$", + "why": "a code whose grant was revoked before redemption is refused" + }, + { + "id": "srv-opaque-token-issuer", + "file": "server/accesstoken.go", + "old": "if p.Issuer == \"\" {", + "new": "if false {", + "module": "root", + "packages": [ + "./resource" + ], + "run": "^TestServerOpaqueAccessTokensRequiresIssuer$", + "why": "opaque access tokens are refused without an issuer" + }, + { + "id": "srv-signedclaims-signature", + "file": "internal/token/validate.go", + "old": "if err := t.compact.Verify(pub, algorithm); err != nil {\n\t\treturn AccessTokenClaims{}", + "new": "if err := t.compact.Verify(pub, algorithm); err != nil && false {\n\t\treturn AccessTokenClaims{}", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestRevokeTokenAnswersInvalidTokensWith200$", + "why": "AccessToken.SignedClaims checks the signature" + }, + { + "id": "srv-client-attestation-signature", + "file": "internal/clientattestation/attestation.go", + "old": "if err := a.compact.Verify(pub, alg); err != nil {", + "new": "if err := a.compact.Verify(pub, alg); err != nil && false {", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestClientAttestationTriesEveryMatchingAttesterKey$", + "why": "a client attestation's signature is checked against the attester key" + }, + { + "id": "srv-caller-certificate-key", + "file": "internal/jose/callerkey.go", + "old": "return checkCallerKeyMaterial(pub)\n}", + "new": "return nil\n}", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestX5CAttesterChainRefusesUnboundedRSAKeys$", + "why": "ValidateCallerCertificateKey bounds caller-supplied certificate keys" + }, + { + "id": "backchannel-outbound-hardened", + "file": "backchannelhttp/notifier.go", + "old": "OutboundHardened: n.hardened}", + "new": "OutboundHardened: true}", + "module": "root", + "packages": [ + "./server" + ], + "run": "^TestNewProductionRefusesLoopbackOutboundExceptions$", + "why": "a notifier with a caller transport doesn't claim outbound hardening" + }, + { + "id": "token-id-aud-trusted-only", + "file": "internal/token/validate.go", + "old": "if !sawExpectedAudience {", + "new": "if _ = sawExpectedAudience; false {", + "module": "root", + "packages": [ + "./internal/token" + ], + "run": "^TestIDTokenValidateRejectsOnlyTrustedAudiences$", + "why": "an ID token whose aud names only trusted audiences, never the client, is refused" + }, + { + "id": "client-jarm-verify", + "file": "client/authorization_response.go", + "old": "\tif verifyErr != nil {\n\t\treturn nil, \"\", newError(ErrorInvalidResponse, \"authorization response verification failed\", verifyErr)\n\t}", + "new": "\tif verifyErr != nil {\n\t\tp := map[string]json.RawMessage{}\n\t\tfor _, k := range []string{\"code\", \"state\", \"error\", \"error_description\", \"iss\"} {\n\t\t\tif v, ok := resp.Parameter(k); ok {\n\t\t\t\tp[k] = v\n\t\t\t}\n\t\t}\n\t\treturn p, responseModeJARM, nil\n\t}", + "module": "root", + "packages": [ + "./client" + ], + "run": "^TestHandleAuthorizationResponseRejectsUnverifiableJARM$", + "why": "a JARM response that fails verification (key, iss, aud, exp) is refused" + }, + { + "id": "client-ciba-token-type", + "file": "client/backchannel.go", + "old": "if !strings.EqualFold(raw.TokenType, wantTokenType) {", + "new": "if false {", + "module": "root", + "packages": [ + "./client" + ], + "run": "^TestPollBackchannelAuthenticationRejectsBearerUnderDPoP$", + "why": "a CIBA token response's token_type must match the sender constraint" + }, + { + "id": "client-key-custody-durable", + "file": "client/assurance.go", + "old": "if !asserter.KeyCustody().Durable {", + "new": "if _ = asserter; false {", + "module": "root", + "packages": [ + "./client" + ], + "run": "^TestNewProductionRequires(Client|Decryption)KeyCustody$", + "why": "AssuranceProduction requires Durable custody for Keys and Decryption" + }, + { + "id": "client-idtoken-content-enc-valid", + "file": "client/client.go", + "old": "if !cfg.Algorithms.IDTokenContentEncryption.IsValid() {", + "new": "if false {", + "module": "root", + "packages": [ + "./client" + ], + "run": "^TestNewNamesEachEncryptionAndMessageSigningRefusal$", + "why": "an invalid ID token content-encryption algorithm is refused" + }, + { + "id": "client-userinfo-key-mgmt-valid", + "file": "client/client.go", + "old": "if !cfg.Algorithms.UserInfoKeyManagement.IsValid() {", + "new": "if false {", + "module": "root", + "packages": [ + "./client" + ], + "run": "^TestNewNamesEachEncryptionAndMessageSigningRefusal$", + "why": "an invalid UserInfo key-management algorithm is refused" + }, + { + "id": "client-jarm-alg-required", + "file": "client/client.go", + "old": "if !cfg.Algorithms.JARM.IsValid() {", + "new": "if false {", + "module": "root", + "packages": [ + "./client" + ], + "run": "^TestNewNamesEachEncryptionAndMessageSigningRefusal$", + "why": "Message Signing requires a JARM algorithm" + }, + { + "id": "client-request-object-lifetime-max", + "file": "client/client.go", + "old": "under ProfileFAPISecurityWithMessageSigning\")\n\t}\n\tif cfg.Limits.RequestObjectLifetime > maxRequestObjectLifetime {", + "new": "under ProfileFAPISecurityWithMessageSigning\")\n\t}\n\tif false {", + "module": "root", + "packages": [ + "./client" + ], + "run": "^TestNewNamesEachEncryptionAndMessageSigningRefusal$", + "why": "Message Signing caps the request object lifetime at 60 minutes" + }, + { + "id": "client-request-object-lifetime-max-boundary", + "file": "client/client.go", + "old": "under ProfileFAPISecurityWithMessageSigning\")\n\t}\n\tif cfg.Limits.RequestObjectLifetime > maxRequestObjectLifetime {", + "new": "under ProfileFAPISecurityWithMessageSigning\")\n\t}\n\tif cfg.Limits.RequestObjectLifetime >= maxRequestObjectLifetime {", + "module": "root", + "packages": [ + "./client" + ], + "run": "^TestNewNamesEachEncryptionAndMessageSigningRefusal$", + "why": "a request object lifetime of exactly 60 minutes is accepted" + }, + { + "id": "client-jarm-lifetime-positive", + "file": "client/client.go", + "old": "if cfg.Limits.MaxJARMResponseLifetime <= 0 {", + "new": "if false {", + "module": "root", + "packages": [ + "./client" + ], + "run": "^TestNewNamesEachEncryptionAndMessageSigningRefusal$", + "why": "Message Signing requires a positive MaxJARMResponseLifetime" + }, + { + "id": "resource-pairwise-empty-subject", + "file": "resource/verify.go", + "old": "case internal == \"\":", + "new": "case internal == \"\" && false:", + "module": "root", + "packages": [ + "./resource" + ], + "run": "^TestVerifyResolvesPairwiseSubjects$", + "why": "a pairwise lookup returning no subject fails closed with a 500" + }, + { + "id": "resource-code-grant-batch-revocation", + "file": "resource/verify.go", + "old": "var keys []string\n\tfor _, c := range grantRevocationClaims {\n\t\traw, ok := claims[c.claim]\n\t\tif !ok {\n\t\t\tcontinue", + "new": "var keys []string\n\tfor _, c := range grantRevocationClaims {\n\t\traw, ok := claims[c.claim]\n\t\tif !ok {\n\t\t\treturn keys, true", + "module": "root", + "packages": [ + "./resource" + ], + "run": "^TestVerifyChecksTheCodeGrantRevocation$", + "why": "a code_grant_id-only token's revoked code grant is refused on the batch path too" + }, + { + "id": "fapihttp-no-addresses", + "file": "fapihttp/client.go", + "old": "if len(ips) == 0 {\n\t\treturn ErrSSRFBlocked", + "new": "if false {\n\t\treturn ErrSSRFBlocked", + "module": "root", + "packages": [ + "./fapihttp" + ], + "run": "^TestFetchRejectsHostResolvingToNoAddresses$", + "why": "a host that resolves to no addresses is refused before any round trip" + }, + { + "id": "requestobject-nbf-future", + "file": "internal/requestobject/verify.go", + "old": "if policy.Now.Before(c.NotBefore.Add(-policy.MaxClockSkew)) {", + "new": "if false {", + "module": "root", + "packages": [ + "./internal/requestobject" + ], + "run": "^TestVerifyBoundsFutureNotBefore$", + "why": "a request object nbf more than MaxClockSkew ahead is refused" + }, + { + "id": "requestobject-nbf-future-skew", + "file": "internal/requestobject/verify.go", + "old": "if policy.Now.Before(c.NotBefore.Add(-policy.MaxClockSkew)) {", + "new": "if policy.Now.Before(c.NotBefore) {", + "module": "root", + "packages": [ + "./internal/requestobject" + ], + "run": "^TestVerifyBoundsFutureNotBefore$", + "why": "a request object nbf within MaxClockSkew is accepted" + }, + { + "id": "jarm-nbf-future", + "file": "internal/jarm/verify.go", + "old": "policy.Now.Before(c.NotBefore.Add(-policy.MaxClockSkew))", + "new": "false", + "module": "root", + "packages": [ + "./internal/jarm" + ], + "run": "^TestVerifyBoundsFutureNotBefore$", + "why": "a JARM response nbf more than MaxClockSkew ahead is refused" + }, + { + "id": "requestobject-empty-string-claim", + "file": "internal/requestobject/claims.go", + "old": "if err := json.Unmarshal(raw, &s); err != nil || s == \"\" {\n\t\treturn \"\", false,", + "new": "if err := json.Unmarshal(raw, &s); err != nil {\n\t\treturn \"\", false,", + "module": "root", + "packages": [ + "./internal/requestobject" + ], + "run": "^TestParseClaimsRejectsMalformedOptionalClaims$", + "why": "an empty jti or sub is malformed, so the replay check can't be skipped" + }, + { + "id": "jwe-cbc-iv-size", + "file": "internal/jwe/cbchmac.go", + "old": "return nil, fmt.Errorf(\"jwe: a256cbc-hs512: %w\", err)\n\t}\n\tif len(iv) != block.BlockSize() {", + "new": "return nil, fmt.Errorf(\"jwe: a256cbc-hs512: %w\", err)\n\t}\n\tif false {", + "module": "root", + "packages": [ + "./internal/jwe" + ], + "run": "^TestOpenCBCHMACRejectsWrongIVSizeWithValidTag$", + "why": "a wrong-length CBC IV is a decryption failure, not a panic" + }, + { + "id": "sealedcookie-key-id", + "file": "internal/sealedcookie/sealedcookie.go", + "old": "if k.id != id || len(rest)", + "new": "if _ = id; len(rest)", + "module": "root", + "packages": [ + "./client/sessioncookie", + "./server/interactioncookie" + ], + "run": "^TestKeyRotation$", + "why": "a sealed cookie is opened only with the key its key ID names" + }, + { + "id": "sealedcookie-expiry", + "file": "internal/sealedcookie/sealedcookie.go", + "old": "json.Unmarshal(plaintext, &e) != nil || !now.Before(time.Unix(e.ExpiresAt, 0))", + "new": "json.Unmarshal(plaintext, &e) != nil", + "module": "root", + "packages": [ + "./client/sessioncookie", + "./server/interactioncookie" + ], + "run": "^TestReadRefuses$", + "why": "an expired sealed cookie is refused" + }, + { + "id": "sqlstore-pg-poll-for-update", + "file": "sqlstore/backchannel.go", + "old": "pollRead + \" FOR UPDATE\"", + "new": "pollRead", + "module": "sqlstore", + "packages": [ + "." + ], + "run": "^TestPostgresPollWaitsForAConcurrentPoll$", + "postgres": true, + "why": "a CIBA poll locks the row so concurrent polls can't both win" + }, + { + "id": "sqlstore-pg-serialization-retry", + "file": "sqlstore/db.go", + "old": "serializationFailure(err)", + "new": "false", + "module": "sqlstore", + "packages": [ + "." + ], + "run": "^TestPostgresSingleStatementRaces$", + "postgres": true, + "why": "a single-statement write is rerun after a serialization failure" + }, + { + "id": "sqlstore-pg-isolation-pin", + "file": "sqlstore/db.go", + "old": "!strings.EqualFold(level, \"read committed\")", + "new": "false && strings.EqualFold(level, \"\")", + "module": "sqlstore", + "packages": [ + "." + ], + "run": "^TestPostgresIsolationCheckFailureKeepsPin$", + "postgres": true, + "why": "a connection not at READ COMMITTED stays pinned to explicit transactions" + }, + { + "id": "sqlstore-pg-sweep-lock", + "file": "sqlstore/sweep.go", + "old": "!locked", + "new": "false", + "module": "sqlstore", + "packages": [ + "." + ], + "run": "^TestPostgresSweepSkipsWhileAnotherSweeps$", + "postgres": true, + "why": "a sweep skips while another holds the advisory lock" + }, + { + "id": "sqlstore-pg-migration-lock", + "file": "sqlstore/migrate.go", + "old": "d.lockMigration(ctx, q)", + "new": "error(nil)", + "module": "sqlstore", + "packages": [ + "." + ], + "run": "^TestPostgresMigrationLockIsPerSchema$", + "postgres": true, + "why": "migrations take the per-schema migration lock" + }, + { + "id": "sqlstore-pg-read-committed", + "file": "sqlstore/db.go", + "old": "d.db.BeginTx(ctx, readCommitted)", + "new": "d.db.BeginTx(ctx, nil)", + "module": "sqlstore", + "packages": [ + "." + ], + "run": "^TestPostgresPollWaitsForAConcurrentPoll$", + "postgres": true, + "why": "storage transactions run at READ COMMITTED" + } +] diff --git a/.github/postgres/Dockerfile b/.github/postgres/Dockerfile index b595ebbb..764577f5 100644 --- a/.github/postgres/Dockerfile +++ b/.github/postgres/Dockerfile @@ -1,9 +1,9 @@ -# Never built. ci.yml's sqlstore job runs this image as a service -# container, and Dependabot doesn't update a workflow's service images, -# but it does update a Dockerfile's (the docker ecosystem in +# Never built. ci.yml's sqlstore job and mutation.yml run this image as +# a service container, and Dependabot doesn't update a workflow's service +# images, but it does update a Dockerfile's (the docker ecosystem in # .github/dependabot.yml). So the pin lives here too, and ci.yml's lint -# job (.github/scripts/check-postgres-pin.sh) fails until the two match: -# when Dependabot moves this digest, copy it to ci.yml. +# job (.github/scripts/check-postgres-pin.sh) fails until they all +# match: when Dependabot moves this digest, copy it to both workflows. FROM postgres:17-alpine@sha256:266d3c257594e195b309ae9d1805e54c788f1d7b375f56bf6c367aac6c917e7b # The image's server itself runs as postgres (its entrypoint drops root); # naming it here only keeps this never-built file from defaulting to root. diff --git a/.github/scripts/check-postgres-pin.sh b/.github/scripts/check-postgres-pin.sh index 76548d11..89dcd6cc 100755 --- a/.github/scripts/check-postgres-pin.sh +++ b/.github/scripts/check-postgres-pin.sh @@ -1,16 +1,16 @@ #!/usr/bin/env bash -# Checks that every Postgres service image in .github/workflows/ci.yml -# is exactly the image .github/postgres/Dockerfile pins (tag and digest). -# Dependabot can't update a workflow's service image but does update a -# Dockerfile's, so a Dependabot pull request moving the Dockerfile's -# digest fails this until ci.yml is moved to match. +# Checks that every Postgres service image in .github/workflows/ (the +# sqlstore job in ci.yml, and mutation.yml) is exactly the image +# .github/postgres/Dockerfile pins (tag and digest). Dependabot can't +# update a workflow's service image but does update a Dockerfile's, so a +# Dependabot pull request moving the Dockerfile's digest fails this +# until the workflows are moved to match. # # Usage: check-postgres-pin.sh [repo-root] set -euo pipefail root=${1:-.} dockerfile=$root/.github/postgres/Dockerfile -workflow=$root/.github/workflows/ci.yml want=$(sed -nE 's/^FROM[[:space:]]+([^[:space:]]+).*/\1/p' "$dockerfile") if [[ "$(printf '%s\n' "$want" | grep -c .)" -ne 1 || $want != postgres:*@sha256:* ]]; then @@ -18,18 +18,22 @@ if [[ "$(printf '%s\n' "$want" | grep -c .)" -ne 1 || $want != postgres:*@sha256 exit 1 fi -found=$(sed -nE 's/^[[:space:]]*image:[[:space:]]*(postgres[:@][^[:space:]]*).*/\1/p' "$workflow") -if [[ -z $found ]]; then - echo "$workflow: no Postgres service image found" >&2 +status=0 +seen=0 +for workflow in "$root"/.github/workflows/*.yml; do + found=$(sed -nE 's/^[[:space:]]*image:[[:space:]]*(postgres[:@][^[:space:]]*).*/\1/p' "$workflow") + [[ -z $found ]] && continue + while IFS= read -r got; do + seen=$((seen + 1)) + if [[ $got != "$want" ]]; then + echo "$workflow: Postgres service image $got doesn't match $dockerfile's $want; copy the Dockerfile's pin there" >&2 + status=1 + fi + done <<<"$found" +done +if [[ $seen -eq 0 ]]; then + echo "$root/.github/workflows: no Postgres service image found" >&2 exit 1 fi - -status=0 -while IFS= read -r got; do - if [[ $got != "$want" ]]; then - echo "$workflow: Postgres service image $got doesn't match $dockerfile's $want; copy the Dockerfile's pin to ci.yml" >&2 - status=1 - fi -done <<<"$found" -[[ $status -eq 0 ]] && echo "Postgres service image matches $dockerfile: $want" +[[ $status -eq 0 ]] && echo "All $seen Postgres service images match $dockerfile: $want" exit "$status" diff --git a/.github/scripts/mutation-check.go b/.github/scripts/mutation-check.go new file mode 100644 index 00000000..781fa60a --- /dev/null +++ b/.github/scripts/mutation-check.go @@ -0,0 +1,406 @@ +//go:build ignore + +// Command mutation-check is mutation.yml's runner. It applies each +// mutant in .github/mutation/mutants.json to a security check, runs +// the tests that should catch it, restores the file and fails if any +// mutant survives: a check the tests no longer protect. +// +// Each mutant names a literal snippet ("old") that must occur exactly +// once in its file and the text that replaces it ("new"), so an entry +// is anchored by text, never by line number. A refactor that moves or +// rewrites an anchored check makes its anchor stale, and the run fails +// until the entry is updated to match. +// +// Every mutated file is restored from the copy read before mutating it, +// never from git, so uncommitted work in the tree is safe. An interrupt +// restores it too. +// +// Run from the repository root: +// +// go run .github/scripts/mutation-check.go # every mutant +// go run .github/scripts/mutation-check.go S1 C3 # just these +// +// The mutants marked "postgres" need FAPIGO_SQLSTORE_POSTGRES_DSN (see +// sqlstore/README.md). Without it they're skipped locally; under CI a +// missing DSN fails the run. +package main + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "os" + "os/exec" + "os/signal" + "path/filepath" + "regexp" + "sort" + "strings" + "sync" + "syscall" + "time" +) + +const listPath = ".github/mutation/mutants.json" + +type mutant struct { + ID string `json:"id"` + File string `json:"file"` + Old string `json:"old"` + New string `json:"new"` + Module string `json:"module"` + Packages []string `json:"packages"` + Run string `json:"run,omitempty"` + Postgres bool `json:"postgres,omitempty"` + Why string `json:"why"` +} + +type result struct { + m mutant + status string + detail string + took time.Duration +} + +// The module directories, relative to the repository root. +var modules = map[string]string{"root": ".", "sqlstore": "sqlstore"} + +// restore holds the file being mutated and its original contents, for +// the interrupt handler. +var restore struct { + sync.Mutex + path string + data []byte +} + +func main() { + if err := run(os.Args[1:]); err != nil { + fmt.Fprintln(os.Stderr, "mutation-check:", err) + os.Exit(1) + } +} + +func run(only []string) error { + mutants, err := load(only) + if err != nil { + return err + } + watchInterrupt() + + ci := os.Getenv("CI") != "" + havePG := os.Getenv("FAPIGO_SQLSTORE_POSTGRES_DSN") != "" + if !havePG && ci && needsPostgres(mutants) { + return errors.New("FAPIGO_SQLSTORE_POSTGRES_DSN is unset under CI: the postgres mutants must run there") + } + + if err := baseline(mutants, havePG); err != nil { + return err + } + + start := time.Now() + var results []result + for _, m := range mutants { + r := apply(m, havePG) + fmt.Printf("%-45s %-12s %6.1fs %s\n", m.ID, r.status, r.took.Seconds(), r.detail) + results = append(results, r) + } + failed := report(results, time.Since(start)) + if failed > 0 { + return fmt.Errorf("%d of %d mutants not killed (see the table above)", failed, len(results)) + } + return nil +} + +func load(only []string) ([]mutant, error) { + data, err := os.ReadFile(listPath) + if err != nil { + return nil, fmt.Errorf("run from the repository root: %w", err) + } + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + var all []mutant + if err := dec.Decode(&all); err != nil { + return nil, fmt.Errorf("%s: %w", listPath, err) + } + seen := map[string]bool{} + for _, m := range all { + switch { + case m.ID == "" || seen[m.ID]: + return nil, fmt.Errorf("%s: missing or duplicate id %q", listPath, m.ID) + case modules[m.Module] == "": + return nil, fmt.Errorf("%s: %s: module must be root or sqlstore", listPath, m.ID) + case !filepath.IsLocal(m.File) || filepath.Clean(m.File) != m.File: + return nil, fmt.Errorf("%s: %s: file must be a clean path inside the repository", listPath, m.ID) + case m.Module == "sqlstore" && !strings.HasPrefix(m.File, "sqlstore/"): + return nil, fmt.Errorf("%s: %s: a sqlstore mutant's file must be under sqlstore/", listPath, m.ID) + case m.Old == "" || m.Old == m.New || len(m.Packages) == 0 || m.Why == "": + return nil, fmt.Errorf("%s: %s: needs old, a different new, packages and why", listPath, m.ID) + } + if _, err := regexp.Compile(m.Run); err != nil { + return nil, fmt.Errorf("%s: %s: run: %w", listPath, m.ID, err) + } + seen[m.ID] = true + } + if len(only) == 0 { + return all, nil + } + var picked []mutant + for _, id := range only { + if !seen[id] { + return nil, fmt.Errorf("no mutant %q in %s", id, listPath) + } + for _, m := range all { + if m.ID == id { + picked = append(picked, m) + } + } + } + return picked, nil +} + +func needsPostgres(mutants []mutant) bool { + for _, m := range mutants { + if m.Postgres { + return true + } + } + return false +} + +// baseline runs, unmutated, every test a mutant below will run, once per +// package: a test that already fails would kill its mutant for the +// wrong reason. It also checks each mutant's run filter names a test in +// its packages, or the mutant would survive with nothing run. +func baseline(mutants []mutant, havePG bool) error { + type pkgKey struct{ module, pkg string } + runs := map[pkgKey][]string{} + whole := map[pkgKey]bool{} + for _, m := range mutants { + if m.Postgres && !havePG { + continue + } + for _, p := range m.Packages { + k := pkgKey{m.Module, p} + if m.Run == "" { + whole[k] = true + } else { + runs[k] = append(runs[k], m.Run) + } + } + } + keys := make([]pkgKey, 0, len(runs)+len(whole)) + for k := range runs { + keys = append(keys, k) + } + for k := range whole { + if _, ok := runs[k]; !ok { + keys = append(keys, k) + } + } + sort.Slice(keys, func(i, j int) bool { + return keys[i].module+keys[i].pkg < keys[j].module+keys[j].pkg + }) + start := time.Now() + ran := map[pkgKey][]string{} + for _, k := range keys { + filter := "" + if !whole[k] { + filter = strings.Join(runs[k], "|") + } + out, err := goTest(k.module, []string{k.pkg}, filter, true) + if err != nil { + fmt.Fprint(os.Stderr, tail(out, 60)) + return fmt.Errorf("baseline: %s tests fail before any mutation: fix them first", k.pkg) + } + for _, match := range runRE.FindAllStringSubmatch(out, -1) { + if !strings.Contains(match[1], "/") { + ran[k] = append(ran[k], match[1]) + } + } + } + for _, m := range mutants { + if m.Run == "" || (m.Postgres && !havePG) { + continue + } + re := regexp.MustCompile(m.Run) + found := false + for _, p := range m.Packages { + for _, name := range ran[pkgKey{m.Module, p}] { + found = found || re.MatchString(name) + } + } + if !found { + return fmt.Errorf("%s: run filter %q matches no test in %s: update %s", m.ID, m.Run, strings.Join(m.Packages, " "), listPath) + } + } + fmt.Printf("baseline: %d packages pass unmutated (%.0fs)\n", len(keys), time.Since(start).Seconds()) + return nil +} + +var runRE = regexp.MustCompile(`(?m)^=== RUN\s+(\S+)`) + +var failRE = regexp.MustCompile(`--- FAIL: (\S+)`) + +func apply(m mutant, havePG bool) result { + r := result{m: m} + if m.Postgres && !havePG { + r.status = "SKIPPED" + r.detail = "needs FAPIGO_SQLSTORE_POSTGRES_DSN" + return r + } + orig, err := os.ReadFile(m.File) + if err != nil { + r.status = "STALE" + r.detail = err.Error() + return r + } + if n := bytes.Count(orig, []byte(m.Old)); n != 1 { + r.status = "STALE" + r.detail = fmt.Sprintf("anchor for %s no longer matches %s (%d occurrences, want 1): update %s", m.ID, m.File, n, listPath) + return r + } + mutated := bytes.Replace(orig, []byte(m.Old), []byte(m.New), 1) + + restore.Lock() + restore.path, restore.data = m.File, orig + restore.Unlock() + defer func() { + restore.Lock() + defer restore.Unlock() + if err := writeBack(m.File, orig); err != nil { + fmt.Fprintf(os.Stderr, "mutation-check: RESTORE %s BY HAND: %v\n", m.File, err) + os.Exit(2) + } + restore.path = "" + }() + if err := writeBack(m.File, mutated); err != nil { + r.status = "STALE" + r.detail = err.Error() + return r + } + + start := time.Now() + out, err := goTest(m.Module, m.Packages, m.Run, false) + r.took = time.Since(start) + switch { + case strings.Contains(out, "[build failed]") || strings.Contains(out, "[setup failed]"): + r.status = "BUILD-FAILED" + r.detail = "the mutant must compile: " + firstLine(out) + case err == nil: + r.status = "SURVIVED" + r.detail = "no test failed: " + m.Why + default: + r.status = "KILLED" + names := uniq(failRE.FindAllStringSubmatch(out, -1)) + if len(names) == 0 { + r.detail = firstLine(out) + } else { + r.detail = strings.Join(names, ", ") + } + } + return r +} + +// writeBack replaces the contents of path, a source file that already +// exists, so its mode is kept (WriteFile's applies only to a new file). +func writeBack(path string, data []byte) error { + return os.WriteFile(path, data, 0o600) //nolint:gosec // path is a mutant's file, checked local to the repository by load +} + +func goTest(module string, pkgs []string, filter string, verbose bool) (string, error) { + args := []string{"test", "-count=1", "-vet=off", "-timeout=10m"} + if verbose { + args = append(args, "-v") + } + if filter != "" { + args = append(args, "-run", filter) + } + args = append(args, pkgs...) + cmd := exec.Command("go", args...) + cmd.Dir = modules[module] + out, err := cmd.CombinedOutput() + return string(out), err +} + +// report prints the results as a markdown table, to stdout and to the +// job summary, and returns how many mutants weren't killed. +func report(results []result, took time.Duration) int { + var b strings.Builder + failed, skipped := 0, 0 + for _, r := range results { + switch r.status { + case "KILLED": + case "SKIPPED": + skipped++ + default: + failed++ + } + } + fmt.Fprintf(&b, "## Mutation check\n\n%d mutants: %d killed, %d not killed, %d skipped (%.0fs).\n\n", + len(results), len(results)-failed-skipped, failed, skipped, took.Seconds()) + b.WriteString("| Mutant | Result | File | Guards | Detail |\n|---|---|---|---|---|\n") + for _, r := range results { + fmt.Fprintf(&b, "| %s | %s | %s | %s | %s |\n", r.m.ID, r.status, r.m.File, cell(r.m.Why), cell(r.detail)) + } + fmt.Print("\n" + b.String()) + if path := os.Getenv("GITHUB_STEP_SUMMARY"); path != "" { + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) //nolint:gosec // the runner's own job summary file + if err == nil { + _, _ = f.WriteString(b.String()) + _ = f.Close() + } + } + return failed +} + +func watchInterrupt() { + ch := make(chan os.Signal, 1) + signal.Notify(ch, os.Interrupt, syscall.SIGTERM) + go func() { + <-ch + restore.Lock() + if restore.path != "" { + _ = writeBack(restore.path, restore.data) + fmt.Fprintln(os.Stderr, "\nmutation-check: interrupted; restored", restore.path) + } + os.Exit(130) + }() +} + +func uniq(matches [][]string) []string { + seen := map[string]bool{} + var out []string + for _, m := range matches { + // Report the top-level test; its subtests' names add nothing. + name, _, _ := strings.Cut(m[1], "/") + if !seen[name] { + seen[name] = true + out = append(out, name) + } + } + return out +} + +func firstLine(out string) string { + for _, l := range strings.Split(out, "\n") { + l = strings.TrimSpace(l) + if l != "" && !strings.HasPrefix(l, "#") && !strings.HasPrefix(l, "ok ") { + return l + } + } + return "" +} + +func tail(out string, n int) string { + lines := strings.Split(out, "\n") + if len(lines) > n { + lines = lines[len(lines)-n:] + } + return strings.Join(lines, "\n") +} + +func cell(s string) string { + s = strings.ReplaceAll(s, "|", `\|`) + return strings.ReplaceAll(s, "\n", " ") +} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ea76f0a9..e61d4089 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -170,8 +170,9 @@ jobs: postgres: # Pinned by digest. Dependabot can't update a workflow's service # image, but it does update .github/postgres/Dockerfile's copy of - # this pin, and the lint job fails until the two match: when a - # Dependabot pull request moves that one, copy it here. + # this pin, and the lint job fails until they match: when a + # Dependabot pull request moves that one, copy it here and to + # mutation.yml. image: postgres:17-alpine@sha256:266d3c257594e195b309ae9d1805e54c788f1d7b375f56bf6c367aac6c917e7b env: # A throwaway server reachable only from this job: no password. @@ -309,9 +310,10 @@ jobs: - name: Check fuzz.yml lists every fuzz target run: .github/scripts/check-fuzz-matrix.sh - # Dependabot can't update the sqlstore job's Postgres service image, - # so .github/postgres/Dockerfile carries the same pin where it can, - # and this fails until a digest it moves is copied to ci.yml. + # Dependabot can't update a workflow's Postgres service image (the + # sqlstore job's, mutation.yml's), so .github/postgres/Dockerfile + # carries the same pin where it can, and this fails until a digest + # it moves is copied to both. - name: Check the Postgres service image matches .github/postgres/Dockerfile run: .github/scripts/check-postgres-pin.sh diff --git a/.github/workflows/mutation.yml b/.github/workflows/mutation.yml new file mode 100644 index 00000000..810ac7f1 --- /dev/null +++ b/.github/workflows/mutation.yml @@ -0,0 +1,81 @@ +name: Mutation + +# Re-applies a curated list of mutations to the library's security +# checks (.github/mutation/mutants.json: delete a check, invert it, or +# move a boundary by one) and runs the tests that should catch each one +# (.github/scripts/mutation-check.go). A mutant that survives — no test +# fails with the check broken — means a refactor has quietly removed +# the protection a test used to give that check, while `go test ./...` +# stays green. The mutants each run a few named tests, so the whole list +# takes minutes, but it guards against slow drift rather than anything +# a single diff does, so it runs weekly (Sunday 9am UTC, clear of +# conformance.yml's and fuzz.yml's daily 6am and 7am runs) and on +# demand. It also runs on a pull request that changes the list or the +# runner, so an edited entry is checked before it lands. +# +# Each entry anchors its check by a literal snippet that must occur +# exactly once in its file, never by line number. A refactor that moves +# or rewrites an anchored check fails this run until its entry is +# updated, in the same PR when it's known. To run it locally (from the +# repository root; the PostgreSQL mutants need +# FAPIGO_SQLSTORE_POSTGRES_DSN, as in sqlstore/README.md, or are +# skipped): +# +# go run .github/scripts/mutation-check.go [mutant-id ...] +# +# Informational only: not a required check in branch protection, and it +# should stay that way, since it runs on pull requests only when the +# list or runner changes. The job summary has a table of every mutant +# and the tests that killed it. + +on: + schedule: + - cron: '0 9 * * 0' # weekly, Sunday 9am UTC + workflow_dispatch: {} + pull_request: + paths: + - .github/mutation/** + - .github/scripts/mutation-check.go + - .github/workflows/mutation.yml + +permissions: + contents: read + +jobs: + mutation: + runs-on: ubuntu-latest + timeout-minutes: 45 + services: + postgres: + # The same pin as ci.yml's sqlstore job, checked against + # .github/postgres/Dockerfile by ci.yml's lint job. + image: postgres:17-alpine@sha256:266d3c257594e195b309ae9d1805e54c788f1d7b375f56bf6c367aac6c917e7b + env: + # A throwaway server reachable only from this job: no password. + POSTGRES_HOST_AUTH_METHOD: trust + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U postgres" + --health-interval 2s + --health-timeout 5s + --health-retries 30 + env: + FAPIGO_SQLSTORE_POSTGRES_DSN: postgres://postgres@localhost:5432/postgres?sslmode=disable + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + check-latest: true + # See ci.yml: no go.sum at the root, and sqlstore's own for its + # drivers. + cache-dependency-path: | + go.mod + sqlstore/go.sum + + - name: Mutation check + run: go run .github/scripts/mutation-check.go diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2f856d83..b72f6b70 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -92,6 +92,14 @@ PostgreSQL as well as SQLite: see [sqlstore/README.md](sqlstore/README.md). to the root module), so a change to the library can break one. CI vets, tests (`-race`) and lints every one; run `go vet ./...` and `go test -race ./...` in each demo your change could affect. +- [`.github/mutation/mutants.json`](.github/mutation/mutants.json) + lists security checks together with a mutation of each (the check + deleted, inverted or moved by one) and the tests that must fail + under it. `mutation.yml` re-applies them weekly and fails if one + survives. Each entry finds its check by a literal snippet of the + source, so a change that moves or rewrites an anchored check updates + its entry in the same PR. Run them locally from the repository root + with `go run .github/scripts/mutation-check.go [mutant-id ...]`. - A breaking change (`feat!:`/`fix!:`) adds its own section to [UPGRADING.md](UPGRADING.md), under the version it will ship in: who's affected, why, and what to change. From 34f2a38cf43bd53ecf0b4d2df79f679501378c7e Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Mon, 9 Nov 2026 13:37:13 +0800 Subject: [PATCH 2/2] ci: split the mutation runner's checks and run go by absolute path Split load and baseline into smaller functions to bring their cognitive complexity under Sonar's limit, run the go command from runtime.GOROOT() rather than a PATH lookup, and exclude .github/scripts from Sonar's coverage measure: it's CI tooling that mutation.yml exercises, never imported by the library. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/mutation-check.go | 149 ++++++++++++++++++------------ sonar-project.properties | 6 +- 2 files changed, 94 insertions(+), 61 deletions(-) diff --git a/.github/scripts/mutation-check.go b/.github/scripts/mutation-check.go index 781fa60a..cd9c3803 100644 --- a/.github/scripts/mutation-check.go +++ b/.github/scripts/mutation-check.go @@ -35,6 +35,8 @@ import ( "os/signal" "path/filepath" "regexp" + "runtime" + "slices" "sort" "strings" "sync" @@ -94,7 +96,7 @@ func run(only []string) error { return errors.New("FAPIGO_SQLSTORE_POSTGRES_DSN is unset under CI: the postgres mutants must run there") } - if err := baseline(mutants, havePG); err != nil { + if err := baseline(runnable(mutants, havePG)); err != nil { return err } @@ -123,42 +125,48 @@ func load(only []string) ([]mutant, error) { if err := dec.Decode(&all); err != nil { return nil, fmt.Errorf("%s: %w", listPath, err) } - seen := map[string]bool{} + byID := map[string]mutant{} for _, m := range all { - switch { - case m.ID == "" || seen[m.ID]: + if _, dup := byID[m.ID]; dup || m.ID == "" { return nil, fmt.Errorf("%s: missing or duplicate id %q", listPath, m.ID) - case modules[m.Module] == "": - return nil, fmt.Errorf("%s: %s: module must be root or sqlstore", listPath, m.ID) - case !filepath.IsLocal(m.File) || filepath.Clean(m.File) != m.File: - return nil, fmt.Errorf("%s: %s: file must be a clean path inside the repository", listPath, m.ID) - case m.Module == "sqlstore" && !strings.HasPrefix(m.File, "sqlstore/"): - return nil, fmt.Errorf("%s: %s: a sqlstore mutant's file must be under sqlstore/", listPath, m.ID) - case m.Old == "" || m.Old == m.New || len(m.Packages) == 0 || m.Why == "": - return nil, fmt.Errorf("%s: %s: needs old, a different new, packages and why", listPath, m.ID) } - if _, err := regexp.Compile(m.Run); err != nil { - return nil, fmt.Errorf("%s: %s: run: %w", listPath, m.ID, err) + if err := check(m); err != nil { + return nil, fmt.Errorf("%s: %s: %w", listPath, m.ID, err) } - seen[m.ID] = true + byID[m.ID] = m } if len(only) == 0 { return all, nil } - var picked []mutant + picked := make([]mutant, 0, len(only)) for _, id := range only { - if !seen[id] { + m, ok := byID[id] + if !ok { return nil, fmt.Errorf("no mutant %q in %s", id, listPath) } - for _, m := range all { - if m.ID == id { - picked = append(picked, m) - } - } + picked = append(picked, m) } return picked, nil } +// check reports what's wrong with an entry of the list, if anything. +func check(m mutant) error { + switch { + case modules[m.Module] == "": + return errors.New("module must be root or sqlstore") + case !filepath.IsLocal(m.File) || filepath.Clean(m.File) != m.File: + return errors.New("file must be a clean path inside the repository") + case m.Module == "sqlstore" && !strings.HasPrefix(m.File, "sqlstore/"): + return errors.New("a sqlstore mutant's file must be under sqlstore/") + case m.Old == "" || m.Old == m.New || len(m.Packages) == 0 || m.Why == "": + return errors.New("needs old, a different new, packages and why") + } + if _, err := regexp.Compile(m.Run); err != nil { + return fmt.Errorf("run: %w", err) + } + return nil +} + func needsPostgres(mutants []mutant) bool { for _, m := range mutants { if m.Postgres { @@ -168,47 +176,61 @@ func needsPostgres(mutants []mutant) bool { return false } -// baseline runs, unmutated, every test a mutant below will run, once per -// package: a test that already fails would kill its mutant for the -// wrong reason. It also checks each mutant's run filter names a test in -// its packages, or the mutant would survive with nothing run. -func baseline(mutants []mutant, havePG bool) error { - type pkgKey struct{ module, pkg string } +// pkgKey is one package of one module. +type pkgKey struct{ module, pkg string } + +// runnable is mutants without the postgres ones when there's no +// PostgreSQL to run them against. +func runnable(mutants []mutant, havePG bool) []mutant { + var out []mutant + for _, m := range mutants { + if havePG || !m.Postgres { + out = append(out, m) + } + } + return out +} + +// baselineFilters is the -run filter for each package mutants test: the +// union of their filters, or "" when one of them runs the whole package. +func baselineFilters(mutants []mutant) map[pkgKey]string { runs := map[pkgKey][]string{} whole := map[pkgKey]bool{} for _, m := range mutants { - if m.Postgres && !havePG { - continue - } for _, p := range m.Packages { k := pkgKey{m.Module, p} - if m.Run == "" { - whole[k] = true - } else { - runs[k] = append(runs[k], m.Run) - } + whole[k] = whole[k] || m.Run == "" + runs[k] = append(runs[k], m.Run) } } - keys := make([]pkgKey, 0, len(runs)+len(whole)) - for k := range runs { - keys = append(keys, k) - } - for k := range whole { - if _, ok := runs[k]; !ok { - keys = append(keys, k) + filters := make(map[pkgKey]string, len(runs)) + for k, r := range runs { + if whole[k] { + filters[k] = "" + } else { + filters[k] = strings.Join(r, "|") } } + return filters +} + +// baseline runs, unmutated, every test a mutant below will run, once per +// package: a test that already fails would kill its mutant for the +// wrong reason. It also checks each mutant's run filter names a test in +// its packages, or the mutant would survive with nothing run. +func baseline(mutants []mutant) error { + filters := baselineFilters(mutants) + keys := make([]pkgKey, 0, len(filters)) + for k := range filters { + keys = append(keys, k) + } sort.Slice(keys, func(i, j int) bool { return keys[i].module+keys[i].pkg < keys[j].module+keys[j].pkg }) start := time.Now() ran := map[pkgKey][]string{} for _, k := range keys { - filter := "" - if !whole[k] { - filter = strings.Join(runs[k], "|") - } - out, err := goTest(k.module, []string{k.pkg}, filter, true) + out, err := goTest(k.module, []string{k.pkg}, filters[k], true) if err != nil { fmt.Fprint(os.Stderr, tail(out, 60)) return fmt.Errorf("baseline: %s tests fail before any mutation: fix them first", k.pkg) @@ -220,17 +242,7 @@ func baseline(mutants []mutant, havePG bool) error { } } for _, m := range mutants { - if m.Run == "" || (m.Postgres && !havePG) { - continue - } - re := regexp.MustCompile(m.Run) - found := false - for _, p := range m.Packages { - for _, name := range ran[pkgKey{m.Module, p}] { - found = found || re.MatchString(name) - } - } - if !found { + if !filterMatches(m, ran) { return fmt.Errorf("%s: run filter %q matches no test in %s: update %s", m.ID, m.Run, strings.Join(m.Packages, " "), listPath) } } @@ -238,6 +250,21 @@ func baseline(mutants []mutant, havePG bool) error { return nil } +// filterMatches reports whether m's run filter, if it has one, matches +// a top-level test the baseline ran in one of its packages. +func filterMatches(m mutant, ran map[pkgKey][]string) bool { + if m.Run == "" { + return true + } + re := regexp.MustCompile(m.Run) + for _, p := range m.Packages { + if slices.ContainsFunc(ran[pkgKey{m.Module, p}], re.MatchString) { + return true + } + } + return false +} + var runRE = regexp.MustCompile(`(?m)^=== RUN\s+(\S+)`) var failRE = regexp.MustCompile(`--- FAIL: (\S+)`) @@ -317,7 +344,9 @@ func goTest(module string, pkgs []string, filter string, verbose bool) (string, args = append(args, "-run", filter) } args = append(args, pkgs...) - cmd := exec.Command("go", args...) + // The go command running this one, by absolute path, so the tests + // run on the same toolchain whatever PATH holds. + cmd := exec.Command(filepath.Join(runtime.GOROOT(), "bin", "go"), args...) cmd.Dir = modules[module] out, err := cmd.CombinedOutput() return string(out), err diff --git a/sonar-project.properties b/sonar-project.properties index 439e330f..360524c3 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -85,7 +85,11 @@ sonar.go.tests.reportPaths=report.json # **/testdata/** holds test fixtures and the independent scripts that # generate them (pairwise/testdata/reversible_vectors.py), which no Go # test measures. -sonar.coverage.exclusions=conformance/**/*.py,conformance/**/*.go,cmd/conformance-client/**/*.go,cmd/conformance-as/main.go,cmd/conformance-federation-trust-anchor/**/*.go,examples/**,**/testdata/** +# +# .github/scripts/** is CI tooling, never imported by the library: +# mutation-check.go is verified by mutation.yml running it (its baseline +# and every mutant's result), not by unit tests. +sonar.coverage.exclusions=conformance/**/*.py,conformance/**/*.go,cmd/conformance-client/**/*.go,cmd/conformance-as/main.go,cmd/conformance-federation-trust-anchor/**/*.go,examples/**,**/testdata/**,.github/scripts/** # internal/jose/header.go and internal/jwe/header.go's parseHeader # functions are structurally identical for ~10 lines (decode, then