fix(oauth2): refuse client authentication with an empty secret

[euskadi31]

Closes #94.

Problem

Secret-based client authentication succeeded without a secret, through two
independent holes:

• client_secret_basic never checked the password. decodeBasic turns
  Authorization: Basic base64("<client_id>:") into an empty secret, and
  Authenticate went straight from the decode to LoadClient and the
  client's SecretMatcher (Basic base64(":") even looked up the client
  "").
• DefaultClient.SecretMatches matched two empty strings.
  subtle.ConstantTimeCompare returns 1 for two empty slices, so an unset
  Secret matched the empty password.

client_secret_post already refused an empty client_secret before the
store lookup, so the two secret-based methods disagreed. Anyone knowing the
client_id of a client registered without a secret — a public client, or a
confidential or untyped one left without one — authenticated as it:

Endpoint	Impact
/token	tokens for any grant the client may use — client_credentials mints a token with sub = the client, whatever its type
/revoke	revocation of that client's tokens the caller presents
/introspect	for a confidential client, the validity and claims of any token the caller presents: #81's confidential-only check let it through (public and untyped clients were already refused by #81)

An empty secret is no credential. RFC 6749 §2.3 lets the authorization
server accept "any form of client authentication meeting its security
requirements" and forbids relying on a public client's authentication to
identify it; §10.1 forbids issuing client passwords to native or
user-agent-based clients; OAuth 2.1 §2.1 calls clients without credentials
"public clients". RFC 6749 §2.3.1 lets a client omit an empty
client_secret from the body, but requires Basic only for clients "that
were issued a client password", and a client registered without a secret
was issued none.

Reproductions — the red phase of the committed tests, run on d8ccbb5 with
an empty Basic password:

	Request	Observed
(a)	/token client_credentials as a confidential, public or untyped client without a secret	200 and a minted access token
(b)	/revoke of the client's own token, same clients	200, the token deleted
(c)	/introspect as a confidential client without a secret	200 {"active":true,…} with the token's claims
(d)	an unknown client, then a client with a secret	unknown client, then client authentication failed: the answer depended on the client

Fix

• Guard before LoadClient (oauth2/clientauth/basic.go). Right after
  the decode, id == "" || secret == "" answers 401 invalid_client
missing client_id or client_secret — the guard and description
  client_secret_post's Authenticate already had. No store lookup
  happens, so the answer is byte-identical whether the client exists, has a
  secret or has none (no existence oracle), and a third-party
  SecretMatcher that would wrongly accept "" is never consulted. The
  check holds under every profile, Profile20 included: RFC 6749 permits
  the refusal, and nothing in the repository relied on an empty secret.
  decodeBasic stays a pure decoder and Match a cheap header check.
• DefaultClient.SecretMatches returns false when Secret is empty;
  a non-empty Secret is still compared with subtle.ConstantTimeCompare.
  This covers code that calls the matcher directly. The early return leaks
  nothing: ConstantTimeCompare already returns early on a length
  mismatch.
• oauth2.SecretMatcher contract (godoc): an empty secret is never a
  credential — implementations MUST NOT match it, not even against a hash
  of the empty string, nor anything when no secret is registered. The
  NewBasic, SecretMatches and DefaultClient.Secret godoc say the same.

No signature change and no new exported identifier. none,
client_secret_post, #81's confidential-only check, /authorize and the
metadata are unchanged. The refusal is an *oauth2.Error whose description
stays within the RFC 6749 §5.2 charset; the 401 carries
WWW-Authenticate: Basic realm="oauth2" and reaches OnError as
invalid_client.

Docs: CHANGELOG.md (Security), MIGRATION.md (new #94 section with a
before/after table), LIMITATIONS.md (RFC 6749 §4.4 is not enforced yet),
docs/security-considerations.md (client-authentication and introspection
bullets, a new operator-checklist item).

Breaking change / migration

Nothing is tagged yet, but code tracking master is affected — see
MIGRATION.md.

• Public clients that sent an empty Basic password must use none:
  client_id in the form body, no Authorization header and no
  client_secret (RFC 6749 §3.2.1). Register them with
  TypeValue: oauth2.ClientPublic, add clientauth.NewNone() to
  ServerConfig.ClientAuth (examples/oauth2 does not wire it) and, if the
  client sets AuthMethodValues, include "none".
• Confidential or untyped clients registered without a secret can no
  longer authenticate through client_secret_basic or client_secret_post
  (and none refuses non-public clients): give each one a high-entropy
  secret, or authenticate it with a custom ClientAuthenticator using
  another credential (e.g. private_key_jwt, mTLS).
• Custom SecretMatchers must return false when no secret is
  registered or the presented one is empty: a bcrypt hash of "" matches
  "".
• Wire — Basic base64("<client_id>:") and Basic base64(":<secret>")
  answer 401 invalid_client missing client_id or client_secret at
  /token, /revoke and /introspect, whatever the client. The two oauth2: add explicit caller authorization for token introspection #81
  /introspect rows that used an empty Basic password now get this
  description instead of
  introspection requires confidential client authentication.
• DefaultClient{Secret: ""}.SecretMatches("") is now false.

Public clients still authenticate with none, by design. Through it, a
public client can still use any grant its GrantTypeValues allow,
client_credentials included: RFC 6749 §4.4 is not enforced yet (now listed
in LIMITATIONS.md, follow-up (a) below). Restrict GrantTypeValues for
public clients — an empty list allows every grant.

Tests

• TestDefaultClient (values_test.go, extended) — a DefaultClient
  without a Secret matches neither "" nor "s3cr3t".
• TestSecretMethodsRefuseMissingCredentials (clientauth_test.go) —
  Basic and Post × six cases: a client with a secret, a confidential and a
  public client without one, an unknown client (each with an empty secret),
  an empty client_id with and without a secret. Each case asserts no
  client, invalid_client, the exact description and zero LoadClient
  calls (a new countingStore fake, one per subtest).
• TestEmptyBasicPasswordRefusedAtEveryEndpoint (new
  empty_client_secret_test.go) — /token (client_credentials),
  /revoke and /introspect on a Profile20BCP server with a pinned
  clock, Basic + Post + none, a client with a secret and confidential,
  public and untyped clients without one, each holding an active access
  token. The unknown client's answer is pinned (401, the Basic challenge,
  the exact JSON body, the §5.2 charset), and every registered client's
  answer must be the same status, headers and bytes; every client's token
  is still in the store afterwards — nothing revoked or disclosed.
• TestIntrospectCallerAuthentication — its two empty-password rows now
  expect missing client_id or client_secret: Basic refuses them before
  oauth2: add explicit caller authorization for token introspection #81's confidential-only check, which stays covered by the none public,
  public-with-secret (Basic and Post) and untyped-with-secret rows.

Red phase — the tests were written first and run on d8ccbb5:
TestDefaultClient (SecretMatches("") was true); the six Basic rows (the
confidential and public clients without a secret authenticated, the
others got client authentication failed or unknown client after a store
lookup), while the six Post rows passed as consistency guards; the three
endpoint subtests (reproductions (a)–(d)); the two introspection rows.

Mutation — my runs on a scratch copy killed: the Basic guard removed,
reduced to secret == "", reduced to id == "", the DefaultClient guard
removed, and both removed (the original bug). The independent adversarial
review ran 16 mutants on basic.go, client.go, post.go and
introspect_endpoint.go (guard dropped or reduced to one term, && instead
of ||, moved after LoadClient or after the nil-client check, bare
sentinel, invalid_request code, DefaultClient guard dropped, inverted or
weakened, the Post guard dropped, the confidential-only check dropped or
turned into a public-only denylist): 14 killed; the two survivors are
equivalent — SecretMatches guarding on the presented secret instead of the
registered one, and a non-constant-time comparison, neither observable by a
test.

Probe — the review also sent 159 Basic header variants (53 per endpoint:
Basic/basic/BASIC/bAsIc, unpadded and extra padding, double and
trailing spaces, the URL-safe alphabet, a %00 password — not
percent-decoded, so not empty —, : alone, an empty client_id with and
without a secret) for an unknown client, a client with a secret and the
confidential, public and untyped clients without one, to /token, /revoke
and /introspect: every one got 401 invalid_client with
WWW-Authenticate: Basic realm="oauth2", none authenticated. With Basic +
form mixes and Post with an empty client_secret, that is 165 refusals; the
only 200 was a public client posting an empty client_secret, i.e. none,
as designed.

Checks

• make build — OK.

• make test — every package passes with -race:

  Package	Before	After
  oauth2	93.1%	93.6%
  oauth2/clientauth	100%	100%

  The changed functions, basicAuth.Authenticate and
  DefaultClient.SecretMatches, are 100% covered.

• make lint — 0 issues.

• gofmt -l — clean on every changed production file and on the new test
  file. values_test.go and clientauth_test.go keep the gofmt drift they
  already have on master (struct-field alignment unrelated to this PR); no
  new hunk was added.

Follow-ups (out of scope)

• (a) RFC 6749 §4.4 — client_credentials "MUST only be used by
  confidential clients", but grant/client_credentials.go only checks
  GrantTypeValues: a public client authenticated with none can mint
  client_credentials tokens. Documented in LIMITATIONS.md for now.
• (b) Basic credentials are not form-urldecoded (RFC 6749 §2.3.1,
  Appendix B), which matters for interoperability (e.g. golang.org/x/oauth2
  AuthStyleInHeader encodes them). Keep the empty check on the decoded
  values when fixing it.
• (c) basic.Match claims any Authorization header starting with B
  (e.g. Bearer …); it could be folded into (b).
• (d) With a non-empty secret, the descriptions still tell whether the
  client exists (unknown client vs client authentication failed,
  method not allowed for client, client cannot verify secret).
• (e) A request carrying several client-authentication methods (Basic
  and a form client_secret) is silently resolved to Basic, while RFC 6749
  §2.3 says a client MUST NOT use more than one method per request
  (invalid_request, §5.2).
• (f) The Client.GrantTypes godoc says values are compared with
  strings.EqualFold, but grantTypeAllowed uses an exact
  slices.Contains (fails closed).

Notes for the maintainer

• The id == "" term goes one step beyond the issue text: it mirrors
  client_secret_post and RFC 6749 §2.3.1 makes client_id REQUIRED.
  Dropping it only changes the description an empty-client_id Basic
  header gets (unknown client again, after a store lookup).
• Clients pinned to header-only authentication that send Basic <id>: for
  a public client break; golang.org/x/oauth2's auto-detection falls back to
  form parameters and omits an empty client_secret, which lands on none.

[coveralls]

Coverage Report for CI Build 38048066397

Coverage increased (+0.1%) to 94.753%

Details

• Coverage increased (+0.1%) from the base build.
• Patch coverage: 6 of 6 lines across 2 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	5108
Covered Lines:	4840
Line Coverage:	94.75%
Coverage Strength:	75.86 hits per line

---

💛 - Coveralls

[coveralls]

Coverage Report for CI Build 38048066397

Coverage increased (+0.1%) to 94.753%

Details

• Coverage increased (+0.1%) from the base build.
• Patch coverage: 6 of 6 lines across 2 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	5108
Covered Lines:	4840
Line Coverage:	94.75%
Coverage Strength:	75.86 hits per line

---

💛 - Coveralls