🐛 fix(placement): correct debug permission checks and reject bad methods (#1623)

Map GET/POST debug requests to get/create SAR verbs and return 403 on
denial. Reject unsupported HTTP methods with 405 instead of treating
them as GET. Update the GET RBAC integration test Role to grant get.

Also free unused disk on e2e runners before image builds to reduce
"no space left on device" flakes.

Fixes open-cluster-management-io/ocm#1622

Signed-off-by: Roke Jung <roke@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

.github/scripts/free-disk-space.sh

@@ -0,0 +1,23 @@
+#!/usr/bin/env bash
+# Reclaim disk on GitHub-hosted Ubuntu runners before heavy image builds.
+set -euo pipefail
+
+echo "::group::Disk before"
+df -h /
+echo "::endgroup::"
+
+sudo rm -rf \
+  /usr/share/dotnet \
+  /usr/local/lib/android \
+  /opt/ghc \
+  /opt/hostedtoolcache/CodeQL \
+  /usr/local/share/boost \
+  /usr/share/swift \
+  /usr/local/.ghcup || true
+
+sudo docker image prune -af || true
+sudo docker builder prune -af || true
+
+echo "::group::Disk after"
+df -h /
+echo "::endgroup::"

.github/workflows/e2e.yml

@@ -32,6 +32,8 @@ jobs:
         if: ${{ env.ACT }} # this step only runs locally when using the https://github.com/nektos/act to debug the e2e
       - name: Checkout
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
+      - name: Free disk space
+        run: bash .github/scripts/free-disk-space.sh
       - name: Setup Go
         uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6
         with:
@@ -57,6 +59,8 @@ jobs:
         if: ${{ env.ACT }} # this step only runs locally when using the https://github.com/nektos/act to debug the e2e
       - name: Checkout
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
+      - name: Free disk space
+        run: bash .github/scripts/free-disk-space.sh
       - name: Setup Go
         uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6
         with:
@@ -82,6 +86,8 @@ jobs:
         if: ${{ env.ACT }} # this step only runs locally when using the https://github.com/nektos/act to debug the e2e
       - name: Checkout
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
+      - name: Free disk space
+        run: bash .github/scripts/free-disk-space.sh
       - name: Setup Go
         uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6
         with:
@@ -107,6 +113,8 @@ jobs:
         if: ${{ env.ACT }} # this step only runs locally when using the https://github.com/nektos/act to debug the e2e
       - name: Checkout
         uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
+      - name: Free disk space
+        run: bash .github/scripts/free-disk-space.sh
       - name: Setup Go
         uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6
         with:

pkg/placement/debugger/debugger.go

@@ -80,16 +80,15 @@ func (d *Debugger) Handler(w http.ResponseWriter, r *http.Request) {
 	var placement *clusterv1beta1.Placement
 	var err error
 
-	// Support both GET (fetch from API) and POST (accept JSON body)
-	if r.Method == http.MethodPost {
+	switch r.Method {
+	case http.MethodPost:
 		// POST: Parse Placement from request body
 		placement, err = d.parsePlacementFromBody(r)
 		if err != nil {
 			d.reportErr(w, http.StatusBadRequest, err)
 			return
 		}
-	} else {
-		// GET: Fetch Placement from API (original behavior)
+	case http.MethodGet:
 		namespace, name, err := d.parsePath(r.URL.Path)
 		if err != nil {
 			d.reportErr(w, http.StatusBadRequest, err)
@@ -109,9 +108,12 @@ func (d *Debugger) Handler(w http.ResponseWriter, r *http.Request) {
 			}
 			return
 		}
+	default:
+		d.reportErr(w, http.StatusMethodNotAllowed, fmt.Errorf("method %s not allowed", r.Method))
+		return
 	}
 
-	// Check if user has permission to create placements in this namespace
+	// Check if user has permission to access placements in this namespace
 	if err := d.checkPermission(r, placement.Namespace); err != nil {
 		d.reportPermissionErr(w, err)
 		return
@@ -209,11 +211,24 @@ func (d *Debugger) reportPermissionErr(w http.ResponseWriter, err error) {
 		statusCode = http.StatusUnauthorized
 	case strings.Contains(msg, "does not have permission"):
 		statusCode = http.StatusForbidden
+	case strings.Contains(msg, "unsupported method"):
+		statusCode = http.StatusMethodNotAllowed
 	}
 	d.reportErr(w, statusCode, err)
 }
 
-// checkPermission checks if the user has permission to create placements in the namespace using SAR
+func placementPermissionVerb(r *http.Request) (string, error) {
+	switch r.Method {
+	case http.MethodPost:
+		return "create", nil
+	case http.MethodGet:
+		return "get", nil
+	default:
+		return "", fmt.Errorf("unsupported method %s", r.Method)
+	}
+}
+
+// checkPermission checks if the user has permission to access placements in the namespace using SAR
 func (d *Debugger) checkPermission(r *http.Request, namespace string) error {
 	// Get user from request context (authenticated by GenericAPIServer)
 	ctx := r.Context()
@@ -229,7 +244,12 @@ func (d *Debugger) checkPermission(r *http.Request, namespace string) error {
 		extra[k] = authorizationv1.ExtraValue(v)
 	}
 
-	// Create SubjectAccessReview to check if user can create placements
+	verb, err := placementPermissionVerb(r)
+	if err != nil {
+		return err
+	}
+
+	// Create SubjectAccessReview to check if user can access placements
 	sar := &authorizationv1.SubjectAccessReview{
 		Spec: authorizationv1.SubjectAccessReviewSpec{
 			User:   username,
@@ -237,7 +257,7 @@ func (d *Debugger) checkPermission(r *http.Request, namespace string) error {
 			Extra:  extra,
 			ResourceAttributes: &authorizationv1.ResourceAttributes{
 				Namespace: namespace,
-				Verb:      "create",
+				Verb:      verb,
 				Group:     "cluster.open-cluster-management.io",
 				Version:   "v1beta1",
 				Resource:  "placements",
@@ -254,8 +274,8 @@ func (d *Debugger) checkPermission(r *http.Request, namespace string) error {
 	}
 
 	if !result.Status.Allowed {
-		return fmt.Errorf("user does not have permission to create placements in namespace %s: %s",
-			namespace, result.Status.Reason)
+		return fmt.Errorf("user does not have permission to %s placements in namespace %s: %s",
+			verb, namespace, result.Status.Reason)
 	}
 
 	return nil

pkg/placement/debugger/debugger_test.go

@@ -230,10 +230,20 @@ func TestReportPermissionErr(t *testing.T) {
 			expectStatus: http.StatusUnauthorized,
 		},
 		{
-			name:         "forbidden",
+			name:         "forbidden create",
 			err:          fmt.Errorf("user does not have permission to create placements in namespace test: denied"),
 			expectStatus: http.StatusForbidden,
 		},
+		{
+			name:         "forbidden get",
+			err:          fmt.Errorf("user does not have permission to get placements in namespace test: denied"),
+			expectStatus: http.StatusForbidden,
+		},
+		{
+			name:         "unsupported method",
+			err:          fmt.Errorf("unsupported method DELETE"),
+			expectStatus: http.StatusMethodNotAllowed,
+		},
 		{
 			name:         "internal",
 			err:          fmt.Errorf("failed to check permissions: boom"),
@@ -301,6 +311,27 @@ func TestDebuggerHandlerReportErr(t *testing.T) {
 			expectStatus:  http.StatusNotFound,
 			errorContains: "does-not-exist",
 		},
+		{
+			name:          "DELETE method not allowed",
+			method:        http.MethodDelete,
+			pathSuffix:    placementNamespace + "/" + placementName,
+			expectStatus:  http.StatusMethodNotAllowed,
+			errorContains: "method DELETE not allowed",
+		},
+		{
+			name:          "PUT method not allowed",
+			method:        http.MethodPut,
+			pathSuffix:    placementNamespace + "/" + placementName,
+			expectStatus:  http.StatusMethodNotAllowed,
+			errorContains: "method PUT not allowed",
+		},
+		{
+			name:          "PATCH method not allowed",
+			method:        http.MethodPatch,
+			pathSuffix:    placementNamespace + "/" + placementName,
+			expectStatus:  http.StatusMethodNotAllowed,
+			errorContains: "method PATCH not allowed",
+		},
 		{
 			name:       "GET placement lister failure",
 			initObjs:   validObjs,
@@ -769,6 +800,8 @@ func TestDebuggerPermissionCheck(t *testing.T) {
 
 	cases := []struct {
 		name             string
+		method           string
+		body             []byte
 		injectUser       bool
 		userInfo         *user.DefaultInfo
 		sarAllowed       bool
@@ -793,12 +826,33 @@ func TestDebuggerPermissionCheck(t *testing.T) {
 			expectStatusCode: http.StatusOK,
 		},
 		{
-			name:             "User without permission - should reject",
+			name:             "GET user without permission - should reject with get verb",
+			method:           http.MethodGet,
 			injectUser:       true,
 			sarAllowed:       false,
 			expectError:      true,
 			expectStatusCode: http.StatusForbidden,
-			errorContains:    "does not have permission",
+			errorContains:    "does not have permission to get placements",
+			verifySARRequest: func(t *testing.T, sar *authorizationv1.SubjectAccessReview) {
+				if sar.Spec.ResourceAttributes.Verb != "get" {
+					t.Errorf("Expected SAR verb get, got %s", sar.Spec.ResourceAttributes.Verb)
+				}
+			},
+		},
+		{
+			name:             "POST user without permission - should reject with create verb",
+			method:           http.MethodPost,
+			body:             []byte(`{"apiVersion":"cluster.open-cluster-management.io/v1beta1","kind":"Placement","metadata":{"name":"test-placement","namespace":"test-ns"},"spec":{"numberOfClusters":1}}`),
+			injectUser:       true,
+			sarAllowed:       false,
+			expectError:      true,
+			expectStatusCode: http.StatusForbidden,
+			errorContains:    "does not have permission to create placements",
+			verifySARRequest: func(t *testing.T, sar *authorizationv1.SubjectAccessReview) {
+				if sar.Spec.ResourceAttributes.Verb != "create" {
+					t.Errorf("Expected SAR verb create, got %s", sar.Spec.ResourceAttributes.Verb)
+				}
+			},
 		},
 		{
 			name:             "SAR API call fails",
@@ -899,8 +953,31 @@ func TestDebuggerPermissionCheck(t *testing.T) {
 			defer server.Close()
 
 			// Send request
-			url := fmt.Sprintf("%s%s%s/%s", server.URL, DebugPath, placementNamespace, placementName)
-			res, err := http.Get(url)
+			method := c.method
+			if method == "" {
+				method = http.MethodGet
+			}
+
+			var url string
+			var body io.Reader
+			if method == http.MethodPost {
+				url = fmt.Sprintf("%s%s", server.URL, DebugPath)
+				if len(c.body) > 0 {
+					body = bytes.NewBuffer(c.body)
+				}
+			} else {
+				url = fmt.Sprintf("%s%s%s/%s", server.URL, DebugPath, placementNamespace, placementName)
+			}
+
+			req, err := http.NewRequest(method, url, body)
+			if err != nil {
+				t.Fatalf("Failed to create request: %v", err)
+			}
+			if method == http.MethodPost {
+				req.Header.Set("Content-Type", "application/json")
+			}
+
+			res, err := http.DefaultClient.Do(req)
 			if err != nil {
 				t.Fatalf("Unexpected error: %v", err)
 			}

test/integration/placement/debugger_test.go

@@ -92,7 +92,7 @@ var _ = ginkgo.Describe("DebugService", func() {
 			_, err := kubeClient.CoreV1().ServiceAccounts(namespace).Create(context.Background(), sa, metav1.CreateOptions{})
 			gomega.Expect(err).ToNot(gomega.HaveOccurred())
 
-			// Create Role with permission to create placements
+			// Create Role with permission to get placements (GET debug endpoint checks get verb)
 			role := &rbacv1.Role{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      roleName,
@@ -102,7 +102,7 @@ var _ = ginkgo.Describe("DebugService", func() {
 					{
 						APIGroups: []string{"cluster.open-cluster-management.io"},
 						Resources: []string{"placements"},
-						Verbs:     []string{"create"},
+						Verbs:     []string{"get"},
 					},
 				},
 			}
@@ -258,8 +258,9 @@ var _ = ginkgo.Describe("DebugService", func() {
 			gomega.Expect(err).ToNot(gomega.HaveOccurred())
 
 			ginkgo.By("Verify request was rejected due to lack of permission")
+			gomega.Expect(resp.StatusCode).To(gomega.Equal(http.StatusForbidden))
 			gomega.Expect(result.Error).ToNot(gomega.BeEmpty(), "Should have permission error")
-			gomega.Expect(result.Error).To(gomega.ContainSubstring("does not have permission"))
+			gomega.Expect(result.Error).To(gomega.ContainSubstring("does not have permission to get placements"))
 		})
 
 		ginkgo.It("Should accept POST request with placement JSON and valid token", func() {