feat(08-03): implement exact authorize validation and pending creation in wristband

- Server.Authorize ports OAuthAuthorizeController::authorize's exact validation order: usable client, exact redirect, response_type=code, code_challenge_method=S256, challenge length, scope parsing/ceiling truncation, resource check, then opaque pending-request creation - Unknown client/unregistered redirect are local text/plain 400s with no Location; every later failure is an ordered RFC3986 redirect with error/error_description/iss[/state], built via a dedicated encoder (never url.Values.Encode, which sorts keys and space-encodes as '+') - Options gains Resource and PendingRequestTTL (both PHP-parity defaults) so authorize's resource check and 600s pending expiry are configurable
2026-09-23 20:08:37 +02:00
parent 787e612ab3
commit 90752beb87
3 changed files with 875 additions and 6 deletions
--- a/wristband/authorize_test.go
+++ b/wristband/authorize_test.go
@@ -117,3 +117,614 @@ func TestPhase8RedAuthorize(t *testing.T) {
 		t.Fatalf("PHASE8_RED:authorize: Cache-Control = %q, want \"no-store\"", cc)
 	}
 }
+
+const authorizeTestRedirect = "https://chatgpt.com/connector/oauth/cb"
+
+func newAuthorizeTestServer() (*Server, *memoryBackend) {
+	backend := newMemoryBackend()
+	return newTestServer(backend), backend
+}
+
+func pendingCount(backend *memoryBackend) int {
+	backend.mu.Lock()
+	defer backend.mu.Unlock()
+	return len(backend.codes)
+}
+
+func TestAuthorizeUnknownClientReturnsLocal400NoLocation(t *testing.T) {
+	srv, _ := newAuthorizeTestServer()
+	req := authorizeRequest(map[string]string{
+		"client_id":    "does-not-exist",
+		"redirect_uri": authorizeTestRedirect,
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want %d (body=%s)", rec.Code, http.StatusBadRequest, rec.Body.String())
+	}
+	if loc := rec.Header().Get("Location"); loc != "" {
+		t.Fatalf("Location = %q, want none", loc)
+	}
+	if ct := rec.Header().Get("Content-Type"); ct != "text/plain; charset=UTF-8" {
+		t.Fatalf("Content-Type = %q, want text/plain; charset=UTF-8", ct)
+	}
+	if body := rec.Body.String(); body != "Unknown client." {
+		t.Fatalf("body = %q, want %q", body, "Unknown client.")
+	}
+	if cc := rec.Header().Get("Cache-Control"); cc != "no-store" {
+		t.Fatalf("Cache-Control = %q, want no-store", cc)
+	}
+}
+
+func TestAuthorizeRevokedClientIsUnknown(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	rec := insertAuthorizeTestClient(backend, "cli-revoked", []string{authorizeTestRedirect}, nil)
+	now := time.Now()
+	rec.RevokedAt = &now
+
+	req := authorizeRequest(map[string]string{
+		"client_id":    "cli-revoked",
+		"redirect_uri": authorizeTestRedirect,
+	})
+	w := httptest.NewRecorder()
+	srv.Authorize(w, req)
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want %d", w.Code, http.StatusBadRequest)
+	}
+}
+
+func TestAuthorizeUnregisteredRedirectURIReturnsLocal400NoLocation(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-redirect", []string{authorizeTestRedirect}, nil)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":    "cli-redirect",
+		"redirect_uri": "https://evil.test/cb",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest)
+	}
+	if loc := rec.Header().Get("Location"); loc != "" {
+		t.Fatalf("Location = %q, want none", loc)
+	}
+	if strings.Contains(rec.Body.String(), "https://evil.test/cb") {
+		t.Fatal("body echoes the untrusted redirect URI")
+	}
+}
+
+func TestAuthorizeTrailingSlashMismatchIsUnregistered(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-slash", []string{authorizeTestRedirect}, nil)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":    "cli-slash",
+		"redirect_uri": authorizeTestRedirect + "/",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest)
+	}
+	if loc := rec.Header().Get("Location"); loc != "" {
+		t.Fatalf("Location = %q, want none", loc)
+	}
+}
+
+func TestAuthorizeUnsupportedResponseTypeRedirectsWithIssAndState(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-resptype", []string{authorizeTestRedirect}, nil)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-resptype",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "token",
+		"code_challenge":        strings.Repeat("b", 43),
+		"code_challenge_method": "S256",
+		"state":                 "iss-state",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, want %d", rec.Code, http.StatusFound)
+	}
+	loc := rec.Header().Get("Location")
+	if !strings.HasPrefix(loc, authorizeTestRedirect) {
+		t.Fatalf("Location = %q, want prefix %q", loc, authorizeTestRedirect)
+	}
+	q := queryOf(t, loc)
+	if q["error"] != "unsupported_response_type" {
+		t.Fatalf("error = %q, want unsupported_response_type", q["error"])
+	}
+	if q["iss"] != "https://plytarium.com" {
+		t.Fatalf("iss = %q, want https://plytarium.com", q["iss"])
+	}
+	if q["state"] != "iss-state" {
+		t.Fatalf("state = %q, want iss-state", q["state"])
+	}
+}
+
+func TestPKCEChallengeMethodMustBeS256(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-plain", []string{authorizeTestRedirect}, nil)
+	state := "state-plain"
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-plain",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        strings.Repeat("a", 43),
+		"code_challenge_method": "plain",
+		"state":                 state,
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, want %d", rec.Code, http.StatusFound)
+	}
+	q := queryOf(t, rec.Header().Get("Location"))
+	if q["error"] != "invalid_request" {
+		t.Fatalf("error = %q, want invalid_request", q["error"])
+	}
+	if q["state"] != state {
+		t.Fatalf("state = %q, want %q", q["state"], state)
+	}
+	if q["iss"] != "https://plytarium.com" {
+		t.Fatalf("iss = %q, want https://plytarium.com", q["iss"])
+	}
+	if cc := rec.Header().Get("Cache-Control"); cc != "no-store" {
+		t.Fatalf("Cache-Control = %q, want no-store", cc)
+	}
+}
+
+func TestPKCEChallengeLengthBounds(t *testing.T) {
+	cases := []struct {
+		name      string
+		challenge string
+	}{
+		{"too-short", strings.Repeat("a", 42)},
+		{"too-long", strings.Repeat("a", 129)},
+		{"empty", ""},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			srv, backend := newAuthorizeTestServer()
+			insertAuthorizeTestClient(backend, "cli-len-"+tc.name, []string{authorizeTestRedirect}, nil)
+			req := authorizeRequest(map[string]string{
+				"client_id":             "cli-len-" + tc.name,
+				"redirect_uri":          authorizeTestRedirect,
+				"response_type":         "code",
+				"code_challenge":        tc.challenge,
+				"code_challenge_method": "S256",
+			})
+			rec := httptest.NewRecorder()
+			srv.Authorize(rec, req)
+			if rec.Code != http.StatusFound {
+				t.Fatalf("status = %d, want %d", rec.Code, http.StatusFound)
+			}
+			q := queryOf(t, rec.Header().Get("Location"))
+			if q["error"] != "invalid_request" {
+				t.Fatalf("error = %q, want invalid_request", q["error"])
+			}
+			if q["error_description"] != "code_challenge is required" {
+				t.Fatalf("error_description = %q, want %q", q["error_description"], "code_challenge is required")
+			}
+		})
+	}
+}
+
+func TestAuthorizeValidRequestRedirectsToConnectWithOpaqueHandleOnly(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-valid", []string{authorizeTestRedirect}, nil)
+	_, challenge := s256Pair(t)
+	state := "must-not-appear-on-our-url"
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-valid",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+		"scope":                 "read write",
+		"state":                 state,
+		"resource":              "https://mcp.plytarium.com/mcp",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, want %d (body=%s)", rec.Code, http.StatusFound, rec.Body.String())
+	}
+	loc := rec.Header().Get("Location")
+	if !strings.HasPrefix(loc, "https://plytarium.com/connect?") {
+		t.Fatalf("Location = %q, want prefix https://plytarium.com/connect?", loc)
+	}
+	q := queryOf(t, loc)
+	if q["request"] == "" {
+		t.Fatal("Location missing non-empty request param")
+	}
+	if _, has := q["code"]; has {
+		t.Fatal("Location leaks a code")
+	}
+	if _, has := q["state"]; has {
+		t.Fatal("Location leaks state")
+	}
+	if _, has := q["client_secret"]; has {
+		t.Fatal("Location leaks client_secret")
+	}
+	if cc := rec.Header().Get("Cache-Control"); cc != "no-store" {
+		t.Fatalf("Cache-Control = %q, want no-store", cc)
+	}
+
+	backend.mu.Lock()
+	defer backend.mu.Unlock()
+	if len(backend.codes) != 1 {
+		t.Fatalf("pending rows = %d, want 1", len(backend.codes))
+	}
+	pending := backend.codes[0]
+	if pending.RequestID == nil || *pending.RequestID != q["request"] {
+		t.Fatalf("pending.RequestID = %v, want %q", pending.RequestID, q["request"])
+	}
+	if pending.CodeHash != nil {
+		t.Fatal("pending row already has a code hash")
+	}
+	if pending.UserID != nil {
+		t.Fatal("pending row already has a user id")
+	}
+	if pending.State == nil || *pending.State != state {
+		t.Fatalf("pending.State = %v, want %q", pending.State, state)
+	}
+	if pending.ExpiresAt.Before(time.Now().Add(500*time.Second)) || pending.ExpiresAt.After(time.Now().Add(700*time.Second)) {
+		t.Fatalf("pending.ExpiresAt = %v, want ~600s from now", pending.ExpiresAt)
+	}
+}
+
+func TestAuthorizeIssIsPresentOnEveryErrorRedirect(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-iss", []string{authorizeTestRedirect}, nil)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-iss",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "token",
+		"code_challenge":        strings.Repeat("b", 43),
+		"code_challenge_method": "S256",
+		"state":                 "iss-state",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	q := queryOf(t, rec.Header().Get("Location"))
+	if q["iss"] != "https://plytarium.com" {
+		t.Fatalf("iss = %q, want https://plytarium.com", q["iss"])
+	}
+	if q["state"] != "iss-state" {
+		t.Fatalf("state = %q, want iss-state", q["state"])
+	}
+	if q["error"] != "unsupported_response_type" {
+		t.Fatalf("error = %q, want unsupported_response_type", q["error"])
+	}
+}
+
+func TestAuthorizeScopeDefaultsToRead(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-default-scope", []string{authorizeTestRedirect}, nil)
+	_, challenge := s256Pair(t)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-default-scope",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, want %d", rec.Code, http.StatusFound)
+	}
+
+	backend.mu.Lock()
+	defer backend.mu.Unlock()
+	if len(backend.codes) != 1 {
+		t.Fatalf("pending rows = %d, want 1", len(backend.codes))
+	}
+	if got := backend.codes[0].Scopes; len(got) != 1 || got[0] != "read" {
+		t.Fatalf("scopes = %v, want [read]", got)
+	}
+}
+
+func TestAuthorizeScopeInvalidValueRedirectsInvalidScope(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-bad-scope", []string{authorizeTestRedirect}, nil)
+	_, challenge := s256Pair(t)
+	before := pendingCount(backend)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-bad-scope",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+		"scope":                 "read bogus",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, want %d", rec.Code, http.StatusFound)
+	}
+	q := queryOf(t, rec.Header().Get("Location"))
+	if q["error"] != "invalid_scope" {
+		t.Fatalf("error = %q, want invalid_scope", q["error"])
+	}
+	if q["error_description"] != "scope contains an unsupported value" {
+		t.Fatalf("error_description = %q", q["error_description"])
+	}
+	if got := pendingCount(backend); got != before {
+		t.Fatalf("pending rows = %d, want unchanged %d", got, before)
+	}
+}
+
+func TestAuthorizeNullCeilingPreservesAiScope(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-null-ceiling", []string{authorizeTestRedirect}, nil)
+	_, challenge := s256Pair(t)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-null-ceiling",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+		"scope":                 "read write ai",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, body=%s", rec.Code, rec.Body.String())
+	}
+
+	backend.mu.Lock()
+	defer backend.mu.Unlock()
+	got := backend.codes[len(backend.codes)-1].Scopes
+	want := []string{"read", "write", "ai"}
+	if len(got) != len(want) {
+		t.Fatalf("scopes = %v, want %v", got, want)
+	}
+	for i := range want {
+		if got[i] != want[i] {
+			t.Fatalf("scopes = %v, want %v", got, want)
+		}
+	}
+}
+
+func TestAuthorizeCeilingTruncatesAiWithoutError(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-ceiling-trunc", []string{authorizeTestRedirect}, []string{"read", "write"})
+	_, challenge := s256Pair(t)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-ceiling-trunc",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+		"scope":                 "read write ai",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, body=%s", rec.Code, rec.Body.String())
+	}
+
+	backend.mu.Lock()
+	defer backend.mu.Unlock()
+	got := backend.codes[len(backend.codes)-1].Scopes
+	if stringSliceContains(got, "ai") {
+		t.Fatalf("scopes = %v, still contains ai", got)
+	}
+	want := []string{"read", "write"}
+	if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] {
+		t.Fatalf("scopes = %v, want %v", got, want)
+	}
+}
+
+func TestAuthorizeCeilingPreservesOfflineAccessFlag(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-ceiling-offline", []string{authorizeTestRedirect}, []string{"read", "write"})
+	_, challenge := s256Pair(t)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-ceiling-offline",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+		"scope":                 "read write ai offline_access",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, body=%s", rec.Code, rec.Body.String())
+	}
+
+	backend.mu.Lock()
+	defer backend.mu.Unlock()
+	pending := backend.codes[len(backend.codes)-1]
+	want := []string{"read", "write"}
+	if len(pending.Scopes) != len(want) || pending.Scopes[0] != want[0] || pending.Scopes[1] != want[1] {
+		t.Fatalf("scopes = %v, want %v", pending.Scopes, want)
+	}
+	if !pending.OfflineAccess {
+		t.Fatal("OfflineAccess = false, want true")
+	}
+}
+
+func TestAuthorizeCeilingRejectsAiOnlyAsInvalidScopeOnTrustedRedirect(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-ceiling-ai-only", []string{authorizeTestRedirect}, []string{"read", "write"})
+	_, challenge := s256Pair(t)
+	state := "state-ai-only"
+	before := pendingCount(backend)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-ceiling-ai-only",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+		"scope":                 "ai",
+		"state":                 state,
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, body=%s", rec.Code, rec.Body.String())
+	}
+	loc := rec.Header().Get("Location")
+	if !strings.HasPrefix(loc, authorizeTestRedirect) {
+		t.Fatalf("Location = %q, want prefix %q", loc, authorizeTestRedirect)
+	}
+	q := queryOf(t, loc)
+	if q["error"] != "invalid_scope" {
+		t.Fatalf("error = %q, want invalid_scope", q["error"])
+	}
+	if q["state"] != state {
+		t.Fatalf("state = %q, want %q", q["state"], state)
+	}
+	if q["iss"] != "https://plytarium.com" {
+		t.Fatalf("iss = %q, want https://plytarium.com", q["iss"])
+	}
+	if got := pendingCount(backend); got != before {
+		t.Fatalf("pending rows = %d, want unchanged %d", got, before)
+	}
+}
+
+func TestAuthorizeCeilingRejectsAiPlusOfflineAccessAsInvalidScope(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-ceiling-ai-offline", []string{authorizeTestRedirect}, []string{"read", "write"})
+	_, challenge := s256Pair(t)
+	state := "state-ai-offline"
+	before := pendingCount(backend)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-ceiling-ai-offline",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+		"scope":                 "ai offline_access",
+		"state":                 state,
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, body=%s", rec.Code, rec.Body.String())
+	}
+	q := queryOf(t, rec.Header().Get("Location"))
+	if q["error"] != "invalid_scope" {
+		t.Fatalf("error = %q, want invalid_scope", q["error"])
+	}
+	if q["state"] != state {
+		t.Fatalf("state = %q, want %q", q["state"], state)
+	}
+	if got := pendingCount(backend); got != before {
+		t.Fatalf("pending rows = %d, want unchanged %d", got, before)
+	}
+}
+
+func TestAuthorizeResourceMismatchRedirectsInvalidTarget(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-resource", []string{authorizeTestRedirect}, nil)
+	_, challenge := s256Pair(t)
+	before := pendingCount(backend)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-resource",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+		"resource":              "https://wrong.example.test/mcp",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, body=%s", rec.Code, rec.Body.String())
+	}
+	q := queryOf(t, rec.Header().Get("Location"))
+	if q["error"] != "invalid_target" {
+		t.Fatalf("error = %q, want invalid_target", q["error"])
+	}
+	if q["error_description"] != "resource does not match this server" {
+		t.Fatalf("error_description = %q", q["error_description"])
+	}
+	if got := pendingCount(backend); got != before {
+		t.Fatalf("pending rows = %d, want unchanged %d", got, before)
+	}
+}
+
+func TestAuthorizeResourceOmittedIsAccepted(t *testing.T) {
+	srv, backend := newAuthorizeTestServer()
+	insertAuthorizeTestClient(backend, "cli-resource-omitted", []string{authorizeTestRedirect}, nil)
+	_, challenge := s256Pair(t)
+
+	req := authorizeRequest(map[string]string{
+		"client_id":             "cli-resource-omitted",
+		"redirect_uri":          authorizeTestRedirect,
+		"response_type":         "code",
+		"code_challenge":        challenge,
+		"code_challenge_method": "S256",
+	})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusFound {
+		t.Fatalf("status = %d, body=%s", rec.Code, rec.Body.String())
+	}
+}
+
+// TestOrderedRedirectQueryEncodingIsRFC3986 proves the redirect query
+// encoder differs from net/url.Values.Encode exactly where PHP
+// http_build_query(..., PHP_QUERY_RFC3986) does: spaces become %20 (not
+// '+'), reserved characters are percent-encoded, and key order is
+// preserved (not sorted) — Pitfall 5.
+func TestOrderedRedirectQueryEncodingIsRFC3986(t *testing.T) {
+	got := buildOrderedQuery([][2]string{
+		{"error", "invalid_scope"},
+		{"error_description", "requested scope is outside this client's ceiling"},
+		{"iss", "https://plytarium.com"},
+		{"state", "a b&c=d"},
+	})
+	want := "error=invalid_scope&error_description=requested%20scope%20is%20outside%20this%20client%27s%20ceiling&iss=https%3A%2F%2Fplytarium.com&state=a%20b%26c%3Dd"
+	if got != want {
+		t.Fatalf("got:  %s\nwant: %s", got, want)
+	}
+}
+
+func TestOrderedRedirectAppendsToExistingQuery(t *testing.T) {
+	got := appendOrderedQuery("https://client.example.test/cb?already=here", [][2]string{
+		{"error", "invalid_request"},
+		{"iss", "https://plytarium.com"},
+	})
+	want := "https://client.example.test/cb?already=here&error=invalid_request&iss=https%3A%2F%2Fplytarium.com"
+	if got != want {
+		t.Fatalf("got:  %s\nwant: %s", got, want)
+	}
+}
+
+func TestAuthorizeBackendUnavailableIsOpaque500(t *testing.T) {
+	opts := DefaultOptions()
+	opts.Issuer = "https://plytarium.com"
+	srv := NewServer(opts)
+	req := authorizeRequest(map[string]string{"client_id": "anything"})
+	rec := httptest.NewRecorder()
+	srv.Authorize(rec, req)
+	if rec.Code != http.StatusInternalServerError {
+		t.Fatalf("status = %d, want %d", rec.Code, http.StatusInternalServerError)
+	}
+}