Input validation / Improper error handling leading to information disclosure via 500

Proof-of-concept: Prerequisites: Grafana 12.x with API exposed and an operational API token. 1) Attempt to create a folder with an invalid UID (contains illegal characters) via the API: curl -i -X POST -H 'Content-Type: application/json' -H 'Authorization: Bearer <TOKEN>' -d '{"title":"TestFolder","uid":"in/valid"}' https://grafana.example/api/folders 2) Expected result: HTTP 400 Bad Request with a structured error payload that includes a message like "uid contains illegal characters" and a Details.UID field containing the error ID (e.g., folder.invalid-uid-chars). 3) Alternative Python example: import requests r = requests.post( 'https://grafana.example/api/folders', headers={'Authorization': 'Bearer <TOKEN>', 'Content-Type': 'application/json'}, json={'title': 'TestFolder', 'uid': 'in/valid'} ) print(r.status_code) print(r.json())

diff --git a/pkg/api/apierrors/folder.go b/pkg/api/apierrors/folder.go index a5ea277572f91..6dbe21ec374d3 100644 --- a/pkg/api/apierrors/folder.go +++ b/pkg/api/apierrors/folder.go @@ -17,26 +17,39 @@ import ( "github.com/grafana/grafana/pkg/util" ) -// stableFolderErrSentinels are folder errors whose legacy /api/folders message are kept stable. +// stableFolderErrSentinels are folder errors whose legacy /api/folders messages are kept stable. var stableFolderErrSentinels = []error{ folder.ErrTitleEmpty, folder.ErrInvalidUID, folder.ErrFolderCannotBeParentOfItself, } +// stableDashboardErrSentinels are dashboard errors whose legacy /api/folders messages are kept stable. +var stableDashboardErrSentinels = []error{ + dashboards.ErrDashboardInvalidUid, + dashboards.ErrDashboardUidTooLong, +} + // ToFolderErrorResponse returns a different response status according to the folder error type func ToFolderErrorResponse(err error) response.Response { // --- Dashboard errors --- var dashboardErr dashboardaccess.DashboardErr if ok := errors.As(err, &dashboardErr); ok { - return response.Error(dashboardErr.StatusCode, err.Error(), err) + msg := err.Error() + var grafanaErr errutil.Error + if errors.As(err, &grafanaErr) { + for _, s := range stableDashboardErrSentinels { + if errors.Is(err, s) { + msg = s.Error() + break + } + } + } + return response.Error(dashboardErr.StatusCode, msg, err) } // --- 400 Bad Request --- if errors.Is(err, folder.ErrTitleEmpty) || - errors.Is(err, dashboards.ErrDashboardTypeMismatch) || - errors.Is(err, dashboards.ErrDashboardInvalidUid) || - errors.Is(err, dashboards.ErrDashboardUidTooLong) || errors.Is(err, folder.ErrFolderCannotBeParentOfItself) || errors.Is(err, folder.ErrMaximumDepthReached) || errors.Is(err, folder.ErrInvalidUID) { @@ -115,12 +128,26 @@ func ToFolderStatusError(err error) k8sErrors.StatusError { return defaultErr } - return k8sErrors.StatusError{ + statusErr := k8sErrors.StatusError{ ErrStatus: metav1.Status{ Message: message, Code: int32(normResp.Status()), }, } + + // Preserve the structured errutil message ID in Status.Details.UID so + // downstream consumers (e.g. provisioning's IsFolderValidationAPIError) + // can match the rejection without relying on the human-readable message. + // errutil.Error.Status() is the source of truth for the message ID; if + // the underlying error is one, copy its Details across. + var grafanaErr errutil.Error + if errors.As(err, &grafanaErr) { + if details := grafanaErr.Status().Details; details != nil { + statusErr.ErrStatus.Details = details + } + } + + return statusErr } func getDefaultMessageForStatus(statusCode int) string { diff --git a/pkg/api/apierrors/folder_test.go b/pkg/api/apierrors/folder_test.go index 2be7dc22b9d25..023bbcb0961e2 100644 --- a/pkg/api/apierrors/folder_test.go +++ b/pkg/api/apierrors/folder_test.go @@ -1,4 +1,4 @@ -package apierrors +package apierrors_test import ( "errors" @@ -6,7 +6,9 @@ import ( "net/http" "testing" + "github.com/grafana/grafana/pkg/api/apierrors" "github.com/grafana/grafana/pkg/api/response" + "github.com/grafana/grafana/pkg/registry/apis/folders" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/dashboards/dashboardaccess" "github.com/grafana/grafana/pkg/services/folder" @@ -33,6 +35,11 @@ func TestToFolderErrorResponse(t *testing.T) { input: folder.ErrMaximumDepthReached.Errorf("Maximum nested folder depth reached"), want: response.Error(http.StatusBadRequest, "[folder.maximum-depth-reached] Maximum nested folder depth reached", nil), }, + { + name: "maximum depth reached (validate.go call site)", + input: folder.ErrMaximumDepthReached.Errorf("folder max depth exceeded, max depth is %d", 4), + want: response.Error(http.StatusBadRequest, "[folder.maximum-depth-reached] folder max depth exceeded, max depth is 4", nil), + }, { name: "bad request errors", input: folder.ErrBadRequest.Errorf("Bad request error"), @@ -74,11 +81,21 @@ func TestToFolderErrorResponse(t *testing.T) { input: dashboards.ErrDashboardInvalidUid, want: response.Error(http.StatusBadRequest, "uid contains illegal characters", dashboards.ErrDashboardInvalidUid), }, + { + name: "dashboard invalid uid (apiserver wrapped)", + input: folders.ErrAPIInvalidUID, + want: response.Error(http.StatusBadRequest, "uid contains illegal characters", folders.ErrAPIInvalidUID), + }, { name: "dashboard uid too long", input: dashboards.ErrDashboardUidTooLong, want: response.Error(http.StatusBadRequest, "uid too long, max 40 characters", dashboards.ErrDashboardUidTooLong), }, + { + name: "dashboard uid too long (apiserver wrapped)", + input: folders.ErrAPIUIDTooLong, + want: response.Error(http.StatusBadRequest, "uid too long, max 40 characters", folders.ErrAPIUIDTooLong), + }, { name: "folder cannot be parent of itself", input: folder.ErrFolderCannotBeParentOfItself, @@ -229,7 +246,7 @@ func TestToFolderErrorResponse(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - resp := ToFolderErrorResponse(tt.input) + resp := apierrors.ToFolderErrorResponse(tt.input) require.Equal(t, tt.want, resp) }) } diff --git a/pkg/registry/apis/folders/validate.go b/pkg/registry/apis/folders/validate.go index def00e7a4854a..5d540dc704e4a 100644 --- a/pkg/registry/apis/folders/validate.go +++ b/pkg/registry/apis/folders/validate.go @@ -14,6 +14,7 @@ import ( "k8s.io/apiserver/pkg/registry/rest" folders "github.com/grafana/grafana/apps/folder/pkg/apis/folder/v1" + "github.com/grafana/grafana/pkg/apimachinery/errutil" "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/dashboards" @@ -23,6 +24,19 @@ import ( "github.com/grafana/grafana/pkg/util" ) +// ErrAPIInvalidUID and ErrAPIUIDTooLong are instantiated errutil.Error values +// created from errutil.BadRequest bases, so the apiserver renders them via +// APIStatus as 400 (not "Unhandled Error" 500). They wrap the legacy +// dashboards sentinels via %w so errors.Is/As keeps matching for /api/folders +// consumers (ToFolderErrorResponse). Defined here rather than in +// pkg/services/folder/model.go to avoid the dashboards->folder import cycle. +var ( + ErrAPIInvalidUID = errutil.BadRequest("folder.invalid-uid-chars", errutil.WithPublicMessage("uid contains illegal characters")). + Errorf("%w", dashboards.ErrDashboardInvalidUid) + ErrAPIUIDTooLong = errutil.BadRequest("folder.uid-too-long", errutil.WithPublicMessage("uid too long, max 40 characters")). + Errorf("%w", dashboards.ErrDashboardUidTooLong) +) + var errOwnerRefsOnManagedFolder = fmt.Errorf("cannot set owner references on folders managed by a repository") func isRepoManaged(f *folders.Folder) bool { @@ -77,11 +91,11 @@ func validateOnCreate(ctx context.Context, f *folders.Folder, getter parentsGett // that the object name (which maps to the UID) is immutable, so re-validating here // would be redundant. if !util.IsValidShortUID(id) { - return dashboards.ErrDashboardInvalidUid + return ErrAPIInvalidUID } if util.IsShortUIDTooLong(id) { - return dashboards.ErrDashboardUidTooLong + return ErrAPIUIDTooLong } f.Spec.Title = strings.TrimSpace(f.Spec.Title) @@ -91,7 +105,7 @@ func validateOnCreate(ctx context.Context, f *folders.Folder, getter parentsGett } if strings.EqualFold(f.Spec.Title, dashboards.RootFolderName) { - return folder.ErrNameExists + return folder.ErrNameExists.Errorf("a folder with that name already exists") } parentName := meta.GetFolder() @@ -142,7 +156,7 @@ func validateOnUpdate(ctx context.Context, } if strings.EqualFold(obj.Spec.Title, dashboards.RootFolderName) { - return folder.ErrNameExists + return folder.ErrNameExists.Errorf("a folder with that name already exists") } if folderObj.GetFolder() == oldFolder.GetFolder() { @@ -162,7 +176,7 @@ func validateOnUpdate(ctx context.Context, // folder cannot be moved to a k6 folder if newParent == accesscontrol.K6FolderUID { - return fmt.Errorf("k6 project may not be moved") + return folder.ErrFolderCannotBeMovedToK6.Errorf("k6 project may not be moved") } parentObj, err := getter.Get(ctx, newParent, &metav1.GetOptions{}) @@ -182,7 +196,7 @@ func validateOnUpdate(ctx context.Context, // This prevents circular references (e.g., moving A under B when B is already under A). for _, ancestor := range info.Items { if ancestor.Name == obj.Name { - return fmt.Errorf("cannot move folder under its own descendant, this would create a circular reference") + return folder.ErrCircularReference.Errorf("cannot move folder under its own descendant, this would create a circular reference") } } diff --git a/pkg/services/folder/model.go b/pkg/services/folder/model.go index 5927eac6e9951..4b3f8a5094732 100644 --- a/pkg/services/folder/model.go +++ b/pkg/services/folder/model.go @@ -22,6 +22,9 @@ var ErrInternal = errutil.Internal("folder.internal") var ErrCircularReference = errutil.BadRequest("folder.circular-reference", errutil.WithPublicMessage("Circular reference detected")) var ErrTargetRegistrySrvConflict = errutil.Internal("folder.target-registry-srv-conflict") var ErrFolderNotEmpty = errutil.BadRequest("folder.not-empty", errutil.WithPublicMessage("Folder cannot be deleted: folder is not empty")) +var ErrFolderCannotBeMovedToK6 = errutil.BadRequest("folder.cannot-be-moved-to-k6", errutil.WithPublicMessage("Folders cannot be moved into the k6 project")) + +// ErrCyclicReference indicates corrupt storage state, not user input. var ErrCyclicReference = errutil.Internal("folder.cyclic-reference", errutil.WithPublicMessage("Cyclic folder references found")) // TODO: evaluate if we can remove legacy errors and only have k8s ones @@ -43,7 +46,7 @@ var ErrAPIFolderCannotBeParentOfItself = errutil.BadRequest("folder.cannot-be-pa var ErrMoveAccessDenied = errutil.Forbidden("folders.forbiddenMove", errutil.WithPublicMessage("Access denied to the destination folder")) var ErrAccessEscalation = errutil.Forbidden("folders.accessEscalation", errutil.WithPublicMessage("Cannot move a folder to a folder where you have higher permissions")) var ErrCreationAccessDenied = errutil.Forbidden("folders.forbiddenCreation", errutil.WithPublicMessage("not enough permissions to create a folder in the selected location")) -var ErrNameExists = errutil.BadGateway("folder.name-exists", errutil.WithPublicMessage("A folder with that name already exists")) +var ErrNameExists = errutil.BadRequest("folder.name-exists", errutil.WithPublicMessage("A folder with that name already exists")) const ( GeneralFolderUID = "general" diff --git a/pkg/tests/apis/folder/folders_test.go b/pkg/tests/apis/folder/folders_test.go index 8255d35d94803..b62a358fb819f 100644 --- a/pkg/tests/apis/folder/folders_test.go +++ b/pkg/tests/apis/folder/folders_test.go @@ -33,6 +33,7 @@ import ( grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/grafana/grafana/pkg/expr" "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/featuremgmt" @@ -540,19 +541,50 @@ func doCreateCircularReferenceFolderTest(t *testing.T, helper *apis.K8sTestHelpe GVR: gvr, }) - payload := `{ - "title": "Test", - "uid": "newFolder", - "parentUid: "newFolder", - }` - create := apis.DoRequest(helper, apis.RequestParams{ + // Create a folder declaring itself as its own parent — must fail with 400. + selfParentPayload := `{"title": "Self Parent", "uid": "self-parent", "parentUid": "self-parent"}` + selfParent := apis.DoRequest(helper, apis.RequestParams{ User: client.Args.User, Method: http.MethodPost, Path: "/api/folders", - Body: []byte(payload), + Body: []byte(selfParentPayload), }, &folder.Folder{}) - require.NotEmpty(t, create.Response) - require.Equal(t, 400, create.Response.StatusCode) + require.NotEmpty(t, selfParent.Response) + require.Equal(t, http.StatusBadRequest, selfParent.Response.StatusCode) + + // Create a parent and a child, then attempt to move the parent under its + // own child — must fail with 400, not 500. + parentPayload := `{"title": "Circular Parent", "uid": "circ-parent"}` + parent := apis.DoRequest(helper, apis.RequestParams{ + User: client.Args.User, + Method: http.MethodPost, + Path: "/api/folders", + Body: []byte(parentPayload), + }, &folder.Folder{}) + require.NotNil(t, parent.Result) + require.Equal(t, http.StatusOK, parent.Response.StatusCode) + + childPayload := `{"title": "Circular Child", "uid": "circ-child", "parentUid": "circ-parent"}` + child := apis.DoRequest(helper, apis.RequestParams{ + User: client.Args.User, + Method: http.MethodPost, + Path: "/api/folders", + Body: []byte(childPayload), + }, &folder.Folder{}) + require.NotNil(t, child.Result) + require.Equal(t, http.StatusOK, child.Response.StatusCode) + + // Move parent under its own child via the k8s api (legacy /api/folders + // move uses a separate PATCH endpoint; this exercises validateOnUpdate). + got, err := client.Resource.Get(context.Background(), "circ-parent", metav1.GetOptions{}) + require.NoError(t, err) + got.SetAnnotations(map[string]string{utils.AnnoKeyFolder: "circ-child"}) + _, err = client.Resource.Update(context.Background(), got, metav1.UpdateOptions{}) + require.Error(t, err) + require.Truef(t, apierrors.IsBadRequest(err), + "expected 400 BadRequest from circular move; got: %v (%T)", err, err) + require.Falsef(t, apierrors.IsInternalError(err), + "circular-move surfaced as HTTP 500: %v", err) } func doListFoldersTest(t *testing.T, helper *apis.K8sTestHelper, mode grafanarest.DualWriterMode) { @@ -2466,25 +2498,49 @@ func TestIntegrationFolderValidationReturns400(t *testing.T) { } cases := []struct { - name string - setup func(t *testing.T) string - folder func(parentUID string) *unstructured.Unstructured - expectedMsg string + name string + setup func(t *testing.T) string + folder func(parentUID string) *unstructured.Unstructured + expectedMsg string + expectedMessageID string }{ { - name: "title empty", - folder: func(string) *unstructured.Unstructured { return makeFolder("title-empty-test", "", "") }, - expectedMsg: "folder title cannot be empty", + name: "title empty", + folder: func(string) *unstructured.Unstructured { return makeFolder("title-empty-test", "", "") }, + expectedMsg: "folder title cannot be empty", + ... [truncated]