From e10c78ea7c2b8028aa10703950def4888681cc99 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 2 Jun 2017 11:29:19 +0200 Subject: [PATCH] apiserver: return BadRequest 400 for invalid query params Kubernetes-commit: 4846c0d16700bb7cb3c3e02fa3919f2de36d4685 --- pkg/endpoints/apiserver_test.go | 46 +++++++++++++++++++++++++++++++++ pkg/endpoints/handlers/rest.go | 7 +++++ 2 files changed, 53 insertions(+) diff --git a/pkg/endpoints/apiserver_test.go b/pkg/endpoints/apiserver_test.go index 467e7dc57..4d6d58970 100644 --- a/pkg/endpoints/apiserver_test.go +++ b/pkg/endpoints/apiserver_test.go @@ -1157,6 +1157,52 @@ func TestList(t *testing.T) { } } +func TestRequestsWithInvalidQuery(t *testing.T) { + storage := map[string]rest.Storage{} + + storage["simple"] = &SimpleRESTStorage{expectedResourceNamespace: "default"} + storage["withoptions"] = GetWithOptionsRESTStorage{} + + var handler = handleInternal(storage, admissionControl, selfLinker, nil) + server := httptest.NewServer(handler) + defer server.Close() + + for i, test := range []struct { + postfix string + method string + }{ + {"/simple?labelSelector=", http.MethodGet}, + {"/simple/foo?gracePeriodSeconds=", http.MethodDelete}, + // {"/simple?labelSelector=", http.MethodDelete}, TODO: implement DeleteCollection in SimpleRESTStorage + // {"/simple/foo?export=", http.MethodGet}, TODO: there is no invalid bool in conversion. Should we be more strict? + // {"/simple/foo?resourceVersion=", http.MethodGet}, TODO: there is no invalid resourceVersion. Should we be more strict? + // {"/withoptions?labelSelector=", http.MethodGet}, TODO: SimpleGetOptions is always valid. Add more validation that can fail. + } { + baseURL := server.URL + "/" + grouplessPrefix + "/" + grouplessGroupVersion.Version + "/namespaces/default" + url := baseURL + test.postfix + r, err := http.NewRequest(test.method, url, nil) + if err != nil { + t.Errorf("%d: unexpected error: %v", i, err) + continue + } + resp, err := http.DefaultClient.Do(r) + if err != nil { + t.Errorf("%d: unexpected error: %v", i, err) + continue + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("%d: unexpected status: %d from url %s, Expected: %d, %#v", i, resp.StatusCode, url, http.StatusBadRequest, resp) + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + t.Errorf("%d: unexpected error: %v", i, err) + continue + } + t.Logf("%d: body: %s", i, string(body)) + } + } +} + func TestLogs(t *testing.T) { handler := handle(map[string]rest.Storage{}) server := httptest.NewServer(handler) diff --git a/pkg/endpoints/handlers/rest.go b/pkg/endpoints/handlers/rest.go index e57b694b6..03d02bcb4 100644 --- a/pkg/endpoints/handlers/rest.go +++ b/pkg/endpoints/handlers/rest.go @@ -155,6 +155,7 @@ func GetResource(r rest.Getter, e rest.Exporter, scope RequestScope) http.Handle if values := req.URL.Query(); len(values) > 0 { exports := metav1.ExportOptions{} if err := metainternalversion.ParameterCodec.DecodeParameters(values, scope.MetaGroupVersion, &exports); err != nil { + err = errors.NewBadRequest(err.Error()) return nil, err } if exports.Export { @@ -164,6 +165,7 @@ func GetResource(r rest.Getter, e rest.Exporter, scope RequestScope) http.Handle return e.Export(ctx, name, exports) } if err := metainternalversion.ParameterCodec.DecodeParameters(values, scope.MetaGroupVersion, &options); err != nil { + err = errors.NewBadRequest(err.Error()) return nil, err } } @@ -181,6 +183,7 @@ func GetResourceWithOptions(r rest.GetterWithOptions, scope RequestScope, isSubr opts, subpath, subpathKey := r.NewGetOptions() trace.Step("About to process Get options") if err := getRequestOptions(req, scope, opts, subpath, subpathKey, isSubresource); err != nil { + err = errors.NewBadRequest(err.Error()) return nil, err } if trace != nil { @@ -227,6 +230,7 @@ func ConnectResource(connecter rest.Connecter, scope RequestScope, admit admissi ctx = request.WithNamespace(ctx, namespace) opts, subpath, subpathKey := connecter.NewConnectOptions() if err := getRequestOptions(req, scope, opts, subpath, subpathKey, isSubresource); err != nil { + err = errors.NewBadRequest(err.Error()) scope.err(err, w, req) return } @@ -293,6 +297,7 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope RequestScope, forceWatch opts := metainternalversion.ListOptions{} if err := metainternalversion.ParameterCodec.DecodeParameters(req.URL.Query(), scope.MetaGroupVersion, &opts); err != nil { + err = errors.NewBadRequest(err.Error()) scope.err(err, w, req) return } @@ -968,6 +973,7 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope RequestSco } else { if values := req.URL.Query(); len(values) > 0 { if err := metainternalversion.ParameterCodec.DecodeParameters(values, scope.MetaGroupVersion, options); err != nil { + err = errors.NewBadRequest(err.Error()) scope.err(err, w, req) return } @@ -1065,6 +1071,7 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope RequestSco listOptions := metainternalversion.ListOptions{} if err := metainternalversion.ParameterCodec.DecodeParameters(req.URL.Query(), scope.MetaGroupVersion, &listOptions); err != nil { + err = errors.NewBadRequest(err.Error()) scope.err(err, w, req) return }