🤖 fix: check a CoderAIProvider's provider ID before Coder calls, and trust only Coder's own delete 404

internal/controller/coderaiprovider_delete.go

@@ -2,6 +2,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -26,6 +27,32 @@ const (
 	aiProviderCreateSettle = 2 * time.Minute
 )
 
+// coderNotFoundMessage is the fixed message of Coder's resource 404, which
+// the provider routes answer (coderd/httpapi/httpapi.go:149-155 and
+// coderd/ai_providers.go:636-638 in v2.37.3). A 404 without it, for example
+// an HTML page of a proxy or "Route not found." for a wrong base URL, does
+// not prove that Coder has no such provider (#323).
+const coderNotFoundMessage = "Resource not found or you do not have access to this resource"
+
+// isCoderResourceNotFound reports a 404 that Coder itself answered.
+func isCoderResourceNotFound(err error) bool {
+	var sdkErr *codersdk.Error
+	return isCoderNotFound(err) && errors.As(err, &sdkErr) && sdkErr.Message == coderNotFoundMessage
+}
+
+// checkAIProviderID fails unless status.providerID is a canonical UUID. The
+// controller puts it into /api/v2/ai/providers/{id}, which also accepts a
+// name, and the SDK resolves ".." segments, so any other value could send
+// the operator token to another Coder route (#323). Only the controller
+// writes it, so another value is impossible.
+func checkAIProviderID(provider *coderv1alpha1.CoderAIProvider) error {
+	id, err := uuid.Parse(provider.Status.ProviderID)
+	if err != nil || id == uuid.Nil || id.String() != provider.Status.ProviderID {
+		return fmt.Errorf("assertion failed: AI provider %s/%s has status.providerID %q, which is not a canonical UUID", provider.Namespace, provider.Name, provider.Status.ProviderID)
+	}
+	return nil
+}
+
 // deleteStep handles an object that is being deleted. It deletes in Coder
 // only a provider that this object proved it created (plan S7), and it
 // returns release when the finalizer can go. Otherwise the step says why
@@ -81,10 +108,14 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co
 // only the nonce and the type prove that the provider is this object's.
 func (r *CoderAIProviderReconciler) deleteInCoder(ctx context.Context, sdk *codersdk.Client, provider *coderv1alpha1.CoderAIProvider) (*aiProviderStep, error) {
 	id := provider.Status.ProviderID
-	if id == "" {
+	if id != "" {
+		if err := checkAIProviderID(provider); err != nil {
+			return nil, err
+		}
+	} else {
 		found, err := sdk.AIProvider(ctx, provider.Name)
 		switch {
-		case isCoderNotFound(err):
+		case isCoderResourceNotFound(err):
 			// A 404 proves absence only for a site owner. A role change in
 			// Coder has no watch, so the wait uses the delete poll.
 			if step := requireSiteOwner(ctx, sdk); step != nil {
@@ -115,7 +146,7 @@ func (r *CoderAIProviderReconciler) deleteInCoder(ctx context.Context, sdk *code
 		}
 		id = found.ID.String()
 	}
-	if err := sdk.DeleteAIProvider(ctx, id); err != nil && !isCoderNotFound(err) {
+	if err := sdk.DeleteAIProvider(ctx, id); err != nil && !isCoderResourceNotFound(err) {
 		return aiProviderWait(aiProviderDeletePoll, "CoderUnavailable", "delete provider %s (ID %s): %s", provider.Name, id, coderErrorSummary(err)), nil
 	}
 	return nil, nil

internal/controller/coderaiprovider_delete_test.go

@@ -326,3 +326,92 @@ func TestCoderAIProviderDeleteWithControlPlaneGoing(t *testing.T) {
 		require.Equal(t, requests, env.fake.totalRequests(), "no Coder request")
 	})
 }
+
+// #323: status.providerID goes into /api/v2/ai/providers/{id}, which also
+// takes a name, and the SDK resolves "..". A value that is not a canonical
+// UUID fails an assertion before any Coder request, on the delete, sync and
+// disable paths.
+func TestCoderAIProviderForgedProviderID(t *testing.T) {
+	t.Parallel()
+	forged := map[string]string{
+		"path traversal": "../../users/5d0c6c2e-3a4d-4c55-9d2f-3f1b1f0c9a11",
+		"name":           "openai",
+		"braces":         "{5d0c6c2e-3a4d-4c55-9d2f-3f1b1f0c9a11}",
+		"upper case":     "5D0C6C2E-3A4D-4C55-9D2F-3F1B1F0C9A11",
+		"nil UUID":       "00000000-0000-0000-0000-000000000000",
+	}
+	for _, path := range []string{"delete", "sync", "disable"} {
+		for name, id := range forged {
+			t.Run(path+"/"+name, func(t *testing.T) {
+				t.Parallel()
+				ctx := context.Background()
+				env, secret, provider := newReadyAIProvider(ctx, t)
+				provider.Status.ProviderID = id
+				require.NoError(t, k8sClient.Status().Update(ctx, provider))
+				switch path {
+				case "delete":
+					require.NoError(t, k8sClient.Delete(ctx, provider))
+				case "disable":
+					editSecret(ctx, t, secret, func(s *corev1.Secret) { delete(s.Labels, "coder.com/credential") })
+				}
+				requests := env.fake.totalRequests()
+				_, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(provider)})
+				require.ErrorContains(t, err, "assertion failed: AI provider "+env.namespace+"/openai has status.providerID")
+				require.ErrorContains(t, err, "which is not a canonical UUID")
+				require.Equal(t, requests, env.fake.totalRequests(), "no Coder request")
+				got := &coderv1alpha1.CoderAIProvider{}
+				require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(provider), got))
+				require.Equal(t, []string{coderv1alpha1.CoderAIProviderCleanupFinalizer}, got.Finalizers)
+			})
+		}
+	}
+}
+
+// #323: a delete answered 404 without Coder's JSON error, for example an
+// HTML page of a proxy, does not prove that the provider is gone. The
+// finalizer stays and the delete is retried.
+func TestCoderAIProviderDeleteNotFoundFromProxy(t *testing.T) {
+	t.Parallel()
+	for name, fault := range map[string]fakeFault{
+		"HTML page":     {Status: http.StatusNotFound, HTML: "<html>404 Not Found</html>"},
+		"other message": {Status: http.StatusNotFound, Message: "Route not found."},
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			ctx := context.Background()
+			env, _, provider := newReadyAIProvider(ctx, t)
+			env.fake.failNext(routeDeleteAIProvider, fault)
+			got, result := env.deleteAIProvider(ctx, t, provider)
+			require.NotNil(t, got)
+			requireAIProviderStep(t, got, false, "CoderUnavailable")
+			require.Equal(t, []string{coderv1alpha1.CoderAIProviderCleanupFinalizer}, got.Finalizers)
+			require.Equal(t, 60*time.Second, result.RequeueAfter)
+			_, found := env.fake.aiProvider("openai")
+			require.True(t, found)
+
+			got, _ = env.reconcileDeleting(ctx, t, got)
+			require.Nil(t, got, "Coder's own answer releases the finalizer")
+			_, found = env.fake.aiProvider("openai")
+			require.False(t, found)
+		})
+	}
+}
+
+// #323: with only a create marker, a lookup 404 that Coder did not answer
+// proves nothing either.
+func TestCoderAIProviderDeletePendingCreateNotFoundFromProxy(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	env, _, provider := newAIProviderCreateEnv(ctx, t)
+	provider.Finalizers = []string{coderv1alpha1.CoderAIProviderCleanupFinalizer}
+	require.NoError(t, k8sClient.Update(ctx, provider))
+	provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now().Add(-3 * time.Minute)}
+	require.NoError(t, k8sClient.Status().Update(ctx, provider))
+	env.fake.failNext(routeAIProvider, fakeFault{Status: http.StatusNotFound, HTML: "<html>404 Not Found</html>"})
+	got, _ := env.deleteAIProvider(ctx, t, provider)
+	require.NotNil(t, got)
+	requireAIProviderStep(t, got, false, "CoderUnavailable")
+	require.Equal(t, []string{coderv1alpha1.CoderAIProviderCleanupFinalizer}, got.Finalizers)
+	got, _ = env.reconcileDeleting(ctx, t, got)
+	require.Nil(t, got, "Coder's own 404 releases the finalizer")
+}

internal/controller/coderaiprovider_disable.go

@@ -33,6 +33,9 @@ func (r *CoderAIProviderReconciler) disableWithoutConsent(
 	if provider.Status.ProviderID == "" || credErr == nil {
 		return nil, fmt.Errorf("assertion failed: AI provider %s/%s needs a provider ID and a credential failure to be disabled", provider.Namespace, provider.Name)
 	}
+	if err := checkAIProviderID(provider); err != nil {
+		return nil, err
+	}
 	// Status messages are cut at 256 characters, so the new fact goes first.
 	// No Kubernetes event follows a dashboard enable, so the stall resyncs.
 	stalled := func(format string, args ...any) *aiProviderStep {

internal/controller/coderaiprovider_sync.go

@@ -34,6 +34,9 @@ func (r *CoderAIProviderReconciler) syncAIProvider(
 	if cred == nil {
 		return nil, false, fmt.Errorf("assertion failed: AI provider %s/%s has a provider ID without a credential", provider.Namespace, provider.Name)
 	}
+	if err := checkAIProviderID(provider); err != nil {
+		return nil, false, err
+	}
 	applied := provider.Status.AppliedGeneration == provider.Generation
 	found, err := sdk.AIProvider(ctx, provider.Status.ProviderID)
 	switch {