fix(logging): reduce verbose logging for security. Fixes #12293 (#12295)

add back additionalinfo as optional



make k8s error logs more generic to protect against xss



remove verbose logging for minio get artifact handler



sanitize rather than remove resource names + revert backend error logs



fix typo + add invalid resource name tests



add valid namespace check for tensorboard



update pipeline upload server responses + log verbose error messages



remove error wrap from backend error logs



minor updates to satisfy pre commit checks



update tensor board file to ensure namespace is a str



update frontend code format + backend test cases



update pipeline upload test cases to pass pre commit



fix minor merge typo



fix minor merge typo

Signed-off-by: JerT33 <[email protected]>

Author: JerT33

backend/src/apiserver/server/pipeline_upload_server.go

@@ -12,6 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+/*
+Package server contains the implementation of the pipeline upload server.
+*/
 package server
 
 import (
@@ -65,7 +68,7 @@ var (
 
 type PipelineUploadServerOptions struct {
 	CollectMetrics bool `json:"collect_metrics,omitempty"`
-	// ApiVersion       string `default:"v2beta1" json:"api_version,omitempty"`
+	// ApiVersion       string `default:"v2beta1" json:"apiVersion,omitempty"`
 	// DefaultNamespace string `default:"" json:"default_namespace,omitempty"`
 }
 
@@ -89,7 +92,7 @@ func (s *PipelineUploadServer) UploadPipeline(w http.ResponseWriter, r *http.Req
 // endpoint to the HTTP endpoint.
 // See https://github.com/grpc-ecosystem/grpc-gateway/issues/500
 // Thus we create the HTTP endpoint directly and using swagger to auto generate the HTTP client.
-func (s *PipelineUploadServer) uploadPipeline(api_version string, w http.ResponseWriter, r *http.Request) {
+func (s *PipelineUploadServer) uploadPipeline(apiVersion string, w http.ResponseWriter, r *http.Request) {
 	if s.options.CollectMetrics {
 		uploadPipelineRequests.Inc()
 		uploadPipelineVersionRequests.Inc()
@@ -98,14 +101,16 @@ func (s *PipelineUploadServer) uploadPipeline(api_version string, w http.Respons
 	glog.Infof("Upload pipeline called")
 	file, header, err := r.FormFile(FormFileKey)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusBadRequest, util.Wrap(err, "Failed to read pipeline from file"))
+		glog.Errorf("Failed to read pipeline from file: %v", err)
+		s.writeErrorToResponse(w, http.StatusBadRequest, errors.New("Failed to read pipeline from file"))
 		return
 	}
 	defer file.Close()
 
 	pipelineFile, err := ReadPipelineFile(header.Filename, file, common.MaxFileLength)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusBadRequest, util.Wrap(err, "Failed to read a pipeline spec file"))
+		glog.Errorf("Failed to read a pipeline spec file: %v", err)
+		s.writeErrorToResponse(w, http.StatusBadRequest, errors.New("Failed to read a pipeline spec file"))
 		return
 	}
 
@@ -126,7 +131,8 @@ func (s *PipelineUploadServer) uploadPipeline(api_version string, w http.Respons
 	}
 	err = s.canUploadVersionedPipeline(r, "", resourceAttributes)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusBadRequest, util.Wrap(err, "Failed to create a pipeline due to authorization error"))
+		glog.Errorf("Failed to create a pipeline due to authorization error: %v", err)
+		s.writeErrorToResponse(w, http.StatusBadRequest, errors.New("Failed to create a pipeline"))
 		return
 	}
 
@@ -162,10 +168,12 @@ func (s *PipelineUploadServer) uploadPipeline(api_version string, w http.Respons
 	newPipeline, newPipelineVersion, err := s.resourceManager.CreatePipelineAndPipelineVersion(pipeline, pipelineVersion)
 	if err != nil {
 		if util.IsUserErrorCodeMatch(err, codes.AlreadyExists) {
-			s.writeErrorToResponse(w, http.StatusConflict, util.Wrap(err, "Failed to create a pipeline and a pipeline version. The pipeline already exists."))
+			glog.Errorf("Failed to create a pipeline and a pipeline version. The pipeline already exists: %v", err)
+			s.writeErrorToResponse(w, http.StatusConflict, errors.New("Failed to create a pipeline and a pipeline version"))
 			return
 		}
-		s.writeErrorToResponse(w, http.StatusInternalServerError, util.Wrap(err, "Failed to create a pipeline and a pipeline version"))
+		glog.Errorf("Failed to create a pipeline and a pipeline version: %v", err)
+		s.writeErrorToResponse(w, http.StatusInternalServerError, errors.New("Failed to create a pipeline and a pipeline version"))
 		return
 	}
 
@@ -174,13 +182,14 @@ func (s *PipelineUploadServer) uploadPipeline(api_version string, w http.Respons
 	}
 
 	var messageToMarshal proto.Message
-	switch api_version {
+	switch apiVersion {
 	case "v1beta1":
 		messageToMarshal = toApiPipelineV1(newPipeline, newPipelineVersion)
 	case "v2beta1":
 		messageToMarshal = toApiPipeline(newPipeline)
 	default:
-		s.writeErrorToResponse(w, http.StatusInternalServerError, util.Wrap(err, "Failed to create a pipeline. Invalid API version"))
+		glog.Errorf("Failed to create a pipeline. Invalid API version: %v", apiVersion)
+		s.writeErrorToResponse(w, http.StatusInternalServerError, errors.New("Failed to create a pipeline"))
 		return
 	}
 
@@ -191,12 +200,14 @@ func (s *PipelineUploadServer) uploadPipeline(api_version string, w http.Respons
 	}
 	data, err := marshaler.Marshal(messageToMarshal)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusInternalServerError, util.Wrap(err, "Failed to create a pipeline. Marshaling error"))
+		glog.Errorf("Failed to create a pipeline. Marshaling error: %v", err)
+		s.writeErrorToResponse(w, http.StatusInternalServerError, errors.New("Failed to create a pipeline"))
 		return
 	}
 	_, err = w.Write(data)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusInternalServerError, util.Wrap(err, "Failed to create a pipeline. Write error."))
+		glog.Errorf("Failed to create a pipeline. Write error: %v", err)
+		s.writeErrorToResponse(w, http.StatusInternalServerError, errors.New("Failed to create a pipeline"))
 		return
 	}
 }
@@ -216,27 +227,30 @@ func (s *PipelineUploadServer) UploadPipelineVersion(w http.ResponseWriter, r *h
 // endpoint to the HTTP endpoint.
 // See https://github.com/grpc-ecosystem/grpc-gateway/issues/500
 // Thus we create the HTTP endpoint directly and using swagger to auto generate the HTTP client.
-func (s *PipelineUploadServer) uploadPipelineVersion(api_version string, w http.ResponseWriter, r *http.Request) {
+func (s *PipelineUploadServer) uploadPipelineVersion(apiVersion string, w http.ResponseWriter, r *http.Request) {
 	if s.options.CollectMetrics {
 		uploadPipelineVersionRequests.Inc()
 	}
 
 	glog.Infof("Upload pipeline version called")
 	file, header, err := r.FormFile(FormFileKey)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusBadRequest, util.Wrap(err, "Failed to create a pipeline version due to error parsing pipeline spec filename"))
+		glog.Errorf("Failed to create a pipeline version. Error parsing pipeline spec filename: %v", err)
+		s.writeErrorToResponse(w, http.StatusBadRequest, errors.New("Failed to create a pipeline version"))
 		return
 	}
 	defer file.Close()
 
 	pipelineFile, err := ReadPipelineFile(header.Filename, file, common.MaxFileLength)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusBadRequest, util.Wrap(err, "Failed to create a pipeline version due to error reading pipeline spec file"))
+		glog.Errorf("Failed to create a pipeline version. Error reading pipeline spec file: %v", err)
+		s.writeErrorToResponse(w, http.StatusBadRequest, errors.New("Failed to create a pipeline version"))
 		return
 	}
-	pipelineId := r.URL.Query().Get(PipelineKey)
-	if pipelineId == "" {
-		s.writeErrorToResponse(w, http.StatusBadRequest, errors.New("Failed to create a pipeline version due to error reading pipeline id"))
+	pipelineID := r.URL.Query().Get(PipelineKey)
+	if pipelineID == "" {
+		glog.Errorf("Failed to create a pipeline version. Error reading pipeline id")
+		s.writeErrorToResponse(w, http.StatusBadRequest, errors.New("Failed to create a pipeline version"))
 		return
 	}
 
@@ -255,9 +269,10 @@ func (s *PipelineUploadServer) uploadPipelineVersion(api_version string, w http.
 		return
 	}
 
-	namespace, err := s.resourceManager.FetchNamespaceFromPipelineId(pipelineId)
+	namespace, err := s.resourceManager.FetchNamespaceFromPipelineId(pipelineID)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusBadRequest, util.Wrap(err, "Failed to create a pipeline version due to error reading namespace"))
+		glog.Errorf("Failed to create a pipeline version. Error reading namespace: %v", err)
+		s.writeErrorToResponse(w, http.StatusBadRequest, errors.New("Failed to create a pipeline version"))
 		return
 	}
 
@@ -271,9 +286,10 @@ func (s *PipelineUploadServer) uploadPipelineVersion(api_version string, w http.
 		Namespace: namespace,
 		Verb:      common.RbacResourceVerbCreate,
 	}
-	err = s.canUploadVersionedPipeline(r, pipelineId, resourceAttributes)
+	err = s.canUploadVersionedPipeline(r, pipelineID, resourceAttributes)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusBadRequest, util.Wrap(err, "Failed to create a pipeline version due to authorization error"))
+		glog.Errorf("Failed to create a pipeline version. Authorization error: %v", err)
+		s.writeErrorToResponse(w, http.StatusBadRequest, errors.New("Failed to create a pipeline version"))
 		return
 	}
 
@@ -284,27 +300,30 @@ func (s *PipelineUploadServer) uploadPipelineVersion(api_version string, w http.
 			Name:         pipelineVersionName,
 			DisplayName:  displayName,
 			Description:  model.LargeText(r.URL.Query().Get(DescriptionQueryStringKey)),
-			PipelineId:   pipelineId,
+			PipelineId:   pipelineID,
 			PipelineSpec: model.LargeText(pipelineFile),
 		},
 	)
 	if err != nil {
 		if util.IsUserErrorCodeMatch(err, codes.AlreadyExists) {
-			s.writeErrorToResponse(w, http.StatusConflict, util.Wrap(err, "Failed to create a pipeline version. The pipeline already exists."))
+			glog.Errorf("Failed to create a pipeline version. The pipeline already exists: %v", err)
+			s.writeErrorToResponse(w, http.StatusConflict, errors.New("Failed to create a pipeline version"))
 			return
 		}
-		s.writeErrorToResponse(w, http.StatusInternalServerError, util.Wrap(err, "Failed to create a pipeline version"))
+		glog.Errorf("Failed to create a pipeline version: %v", err)
+		s.writeErrorToResponse(w, http.StatusInternalServerError, errors.New("Failed to create a pipeline version"))
 		return
 	}
 
 	var messageToMarshal proto.Message
-	switch api_version {
+	switch apiVersion {
 	case "v1beta1":
 		messageToMarshal = toApiPipelineVersionV1(newPipelineVersion)
 	case "v2beta1":
 		messageToMarshal = toApiPipelineVersion(newPipelineVersion)
 	default:
-		s.writeErrorToResponse(w, http.StatusInternalServerError, util.Wrap(err, "Failed to create a pipeline version. Invalid API version"))
+		glog.Errorf("Failed to create a pipeline version. Invalid API version: %v", apiVersion)
+		s.writeErrorToResponse(w, http.StatusInternalServerError, errors.New("Failed to create a pipeline version"))
 		return
 	}
 	// Marshal the message to bytes
@@ -314,12 +333,14 @@ func (s *PipelineUploadServer) uploadPipelineVersion(api_version string, w http.
 	}
 	data, err := marshaler.Marshal(messageToMarshal)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusInternalServerError, util.Wrap(err, "Failed to create a pipeline version. Marshaling error"))
+		glog.Errorf("Failed to create a pipeline version. Marshaling error: %v", err)
+		s.writeErrorToResponse(w, http.StatusInternalServerError, errors.New("Failed to create a pipeline version"))
 		return
 	}
 	_, err = w.Write(data)
 	if err != nil {
-		s.writeErrorToResponse(w, http.StatusInternalServerError, util.Wrap(err, "Failed to create a pipeline version. Write error."))
+		glog.Errorf("Failed to create a pipeline version. Write error: %v", err)
+		s.writeErrorToResponse(w, http.StatusInternalServerError, errors.New("Failed to create a pipeline version"))
 		return
 	}
 
@@ -328,13 +349,13 @@ func (s *PipelineUploadServer) uploadPipelineVersion(api_version string, w http.
 	}
 }
 
-func (s *PipelineUploadServer) canUploadVersionedPipeline(r *http.Request, pipelineId string, resourceAttributes *authorizationv1.ResourceAttributes) error {
+func (s *PipelineUploadServer) canUploadVersionedPipeline(r *http.Request, pipelineID string, resourceAttributes *authorizationv1.ResourceAttributes) error {
 	if !common.IsMultiUserMode() {
 		// Skip authorization if not multi-user mode.
 		return nil
 	}
-	if len(pipelineId) > 0 {
-		namespace, err := s.resourceManager.FetchNamespaceFromPipelineId(pipelineId)
+	if len(pipelineID) > 0 {
+		namespace, err := s.resourceManager.FetchNamespaceFromPipelineId(pipelineID)
 		if err != nil {
 			return util.Wrap(err, "Failed to authorize with the Pipeline ID")
 		}

backend/src/apiserver/server/pipeline_upload_server_test.go

@@ -49,31 +49,31 @@ func TestUploadPipeline(t *testing.T) {
 	tt := []struct {
 		name        string
 		spec        []byte
-		api_version string
+		apiVersion string
 	}{{
 		name:        "upload argo workflow YAML",
 		spec:        []byte("apiVersion: argoproj.io/v1alpha1\nkind: Workflow"),
-		api_version: "v1beta1",
+		apiVersion: "v1beta1",
 	}, {
 		name:        "upload argo workflow YAML",
 		spec:        []byte("apiVersion: argoproj.io/v1alpha1\nkind: Workflow"),
-		api_version: "v2beta1",
+		apiVersion: "v2beta1",
 	}, {
 		name:        "upload pipeline v2 job in proto yaml",
 		spec:        []byte(v2SpecHelloWorld),
-		api_version: "v1beta1",
+		apiVersion: "v1beta1",
 	}, {
 		name:        "upload pipeline v2 job in proto yaml",
 		spec:        []byte(v2SpecHelloWorld),
-		api_version: "v2beta1",
+		apiVersion: "v2beta1",
 	}}
 	for _, test := range tt {
 		t.Run(test.name, func(t *testing.T) {
 			clientManager, server := setupClientManagerAndServer()
 			bytesBuffer, writer := setupWriter("")
 			setWriterWithBuffer("uploadfile", "hello-world.yaml", string(test.spec), writer)
 			var response *httptest.ResponseRecorder
-			switch test.api_version {
+			switch test.apiVersion {
 			case "v1beta1":
 				response = uploadPipeline("/apis/v1beta1/pipelines/upload",
 					bytes.NewReader(bytesBuffer.Bytes()), writer, server.UploadPipelineV1)
@@ -100,7 +100,7 @@ func TestUploadPipeline(t *testing.T) {
 			assert.Equal(t, "1970-01-01T00:00:01Z", parsedResponse.CreatedAt)
 
 			// Verify v1 API returns v1 object while v2 API returns v2 object.
-			switch test.api_version {
+			switch test.apiVersion {
 			case "v1beta1":
 				assert.Equal(t, "123e4567-e89b-12d3-a456-426655440000", parsedResponse.ID)
 				assert.Equal(t, "", parsedResponse.PipelineID)
@@ -235,19 +235,19 @@ func TestUploadPipelineV2_NameValidation(t *testing.T) {
 			name:    "invalid - capitalized",
 			spec:    []byte(invalidV2specCapitalized),
 			wantErr: true,
-			errMsg:  "pipeline's name must contain only lowercase alphanumeric characters or '-' and must start with alphanumeric characters",
+			errMsg:  "Failed to create a pipeline and a pipeline version",
 		},
 		{
 			name:    "invalid - dot",
 			spec:    []byte(invalidV2specDot),
 			wantErr: true,
-			errMsg:  "pipeline's name must contain only lowercase alphanumeric characters or '-' and must start with alphanumeric characters",
+			errMsg:  "Failed to create a pipeline and a pipeline version",
 		},
 		{
 			name:    "invalid - too long",
 			spec:    []byte(invalidV2specLong),
 			wantErr: true,
-			errMsg:  "pipeline's name must contain no more than 128 characters",
+			errMsg:  "Failed to create a pipeline and a pipeline version",
 		},
 	}
 	for _, test := range tt {
@@ -574,7 +574,7 @@ func TestUploadPipelineVersion_GetFromFileError(t *testing.T) {
 	response = uploadPipeline("/apis/v1beta1/pipelines/upload_version?name="+fakeVersionName+"&pipelineid="+DefaultFakeUUID,
 		bytes.NewReader(bytesBuffer.Bytes()), writer, server.UploadPipelineVersion)
 	assert.Equal(t, 400, response.Code)
-	assert.Contains(t, response.Body.String(), "error parsing pipeline spec filename")
+	assert.Contains(t, response.Body.String(), "Failed to create a pipeline version")
 }
 
 func TestUploadPipelineVersion_NameTooLong(t *testing.T) {

frontend/server/app.test.ts

@@ -295,11 +295,32 @@ describe('UIServer apis', () => {
       const spyError = jest.spyOn(console, 'error').mockImplementation(() => null);
       request
         .get('/k8s/pod?podname=test-pod&podnamespace=test-ns')
-        .expect(500, 'Could not get pod test-pod in namespace test-ns: pod not found', () => {
+        .expect(500, 'Could not get pod test-pod in namespace test-ns', () => {
           expect(spyError).toHaveBeenCalledTimes(1);
           done();
         });
     });
+
+    it('responds with error when invalid resource name', done => {
+      const readPodSpy = jest.spyOn(K8S_TEST_EXPORT.k8sV1Client, 'readNamespacedPod');
+      readPodSpy.mockImplementation(() =>
+        Promise.reject({
+          body: {
+            message: 'pod not found',
+            code: 404,
+          },
+        } as any),
+      );
+      const spyError = jest.spyOn(console, 'error').mockImplementation(() => null);
+      request
+        .get(
+          '/k8s/pod?podname=test-pod-name&podnamespace=test-namespace%7d%7dt93g1%3Cscript%3Ealert(1)%3C%2fscript%3Ej66h',
+        )
+        .expect(500, 'Invalid resource name', err => {
+          expect(spyError).toHaveBeenCalledTimes(1);
+          done(err);
+        });
+    });
   });
 
   describe('/k8s/pod/events', () => {
@@ -353,14 +374,31 @@ describe('UIServer apis', () => {
       const spyError = jest.spyOn(console, 'error').mockImplementation(() => null);
       request
         .get('/k8s/pod/events?podname=test-pod&podnamespace=test-ns')
-        .expect(
-          500,
-          'Error when listing pod events for pod "test-pod" in "test-ns" namespace: no events',
-          err => {
-            expect(spyError).toHaveBeenCalledTimes(1);
-            done(err);
+        .expect(500, 'Error when listing pod events for pod test-pod in namespace test-ns', err => {
+          expect(spyError).toHaveBeenCalledTimes(1);
+          done(err);
+        });
+    });
+
+    it('responds with error when invalid resource name', done => {
+      const listEventSpy = jest.spyOn(K8S_TEST_EXPORT.k8sV1Client, 'listNamespacedEvent');
+      listEventSpy.mockImplementation(() =>
+        Promise.reject({
+          body: {
+            message: 'no events',
+            code: 404,
           },
-        );
+        } as any),
+      );
+      const spyError = jest.spyOn(console, 'error').mockImplementation(() => null);
+      request
+        .get(
+          '/k8s/pod/events?podname=test-pod-name&podnamespace=test-namespace%7d%7dt93g1%3Cscript%3Ealert(1)%3C%2fscript%3Ej66h',
+        )
+        .expect(500, 'Invalid resource name', err => {
+          expect(spyError).toHaveBeenCalledTimes(1);
+          done(err);
+        });
     });
   });