Repository: linkerd/linkerd2
Pull request #7664 (2022-01-21 21:03:37)

Fix log forgery issues in production-facing code
CodeQL has caught several instances where we may be susceptible to [log
forgery][cql].

This change ensures that we strip newlines from log messages that
include potentially user-supplied strings. Several redundant error logs
are removed--we should generally not log an error when returning an
error. Errors should be logged where they are handled.

This change also properly escapes URL when constructing them from
protobuf messages.

Signed-off-by: Oliver Gould <ver@buoyant.io>

[cql]: https://codeql.github.com/codeql-query-help/go/go-log-injection/

<!--  Thanks for sending a pull request!

If you already have a well-structured git commit message, chances are GitHub
set the title and description of this PR to the git commit message subject and
body, respectively. If so, you may delete these instructions and submit your PR.

If this is your first time, please read our contributor guide:
https://github.com/linkerd/linkerd2/blob/main/CONTRIBUTING.md

The title and description of your Pull Request should match the git commit
subject and body, respectively. Git commit messages are structured as follows:

```
Subject

Problem

Solution

Validation

Fixes #[GitHub issue ID]

DCO Sign off
```

Example git commit message:

```
Introduce Pull Request Template

GitHub's community guidelines recommend a pull request template, the repo was
lacking one.

Introduce a `PULL_REQUEST_TEMPLATE.md` file.

Once merged, the
[Community profile checklist](https://github.com/linkerd/linkerd2/community)
should indicate the repo now provides a pull request template.

Fixes #3321

Signed-off-by: Jane Smith <jane.smith@example.com>
```

Note the git commit message subject becomes the pull request title.

For more details around git commits, see the section on Committing in our
contributor guide:
https://github.com/linkerd/linkerd2/blob/main/CONTRIBUTING.md#committing
-->


olix0r: Note that, even though this change should fix this issue, codeql appears to have [a bug](https://github.com/github/codeql-go/issues/635) that falsely flags this issue

Patch:
diff --git a/viz/metrics-api/http_server.go b/viz/metrics-api/http_server.go
index d098c5e81..4d6f95be5 100644
--- a/viz/metrics-api/http_server.go
+++ b/viz/metrics-api/http_server.go
@@ -29,9 +29,8 @@ type handler struct {
 }
 
 func (h *handler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
-	log.WithFields(log.Fields{
-		"req.Method": req.Method, "req.URL": req.URL, "req.Form": req.Form,
-	}).Debugf("Serving %s %s", req.Method, req.URL.Path)
+	log.Debugf("Serving %s %q", req.Method, req.URL.Path)
+
 	// Validate request method
 	if req.Method != http.MethodPost {
 		protohttp.WriteErrorToHTTPResponse(w, fmt.Errorf("POST required"))
diff --git a/viz/metrics-api/prometheus.go b/viz/metrics-api/prometheus.go
index 0f05b7a09..2961caf65 100644
--- a/viz/metrics-api/prometheus.go
+++ b/viz/metrics-api/prometheus.go
@@ -60,7 +60,7 @@ func extractSampleValue(sample *model.Sample) uint64 {
 }
 
 func (s *grpcServer) queryProm(ctx context.Context, query string) (model.Vector, error) {
-	log.Debugf("Query request:\n\t%+v", query)
+	log.Debugf("Query request: %q", query)
 
 	_, span := trace.StartSpan(ctx, "query.prometheus")
 	defer span.End()
@@ -73,8 +73,7 @@ func (s *grpcServer) queryProm(ctx context.Context, query string) (model.Vector,
 	// single data point (aka summary) query
 	res, warn, err := s.prometheusAPI.Query(ctx, query, time.Time{})
 	if err != nil {
-		log.Errorf("Query(%+v) failed with: %+v", query, err)
-		return nil, err
+		return nil, fmt.Errorf("Query failed: %q: %w", query, err)
 	}
 	if warn != nil {
 		log.Warnf("%v", warn)
@@ -82,9 +81,7 @@ func (s *grpcServer) queryProm(ctx context.Context, query string) (model.Vector,
 	log.Debugf("Query response:\n\t%+v", res)
 
 	if res.Type() != model.ValVector {
-		err = fmt.Errorf("Unexpected query result type (expected Vector): %s", res.Type())
-		log.Error(err)
-		return nil, err
+		return nil, fmt.Errorf("Unexpected query result type (expected Vector): %s", res.Type())
 	}
 
 	return res.(model.Vector), nil
diff --git a/viz/metrics-api/stat_summary.go b/viz/metrics-api/stat_summary.go
index d58332526..c15a72a72 100644
--- a/viz/metrics-api/stat_summary.go
+++ b/viz/metrics-api/stat_summary.go
@@ -392,7 +392,7 @@ func (s *grpcServer) serviceResourceQuery(ctx context.Context, req *pb.StatSumma
 			}] = weightedDst.Weight.String()
 		}
 	} else if !kerrors.IsNotFound(err) {
-		log.Errorf("Failed to get weights from ServiceProfile %s: %v", spName, err)
+		log.Errorf("Failed to get weights from ServiceProfile %q: %v", spName, err)
 	}
 
 	for k, weight := range weights {
diff --git a/viz/tap/api/grpc_server.go b/viz/tap/api/grpc_server.go
index 2f3d2b38c..651b99d52 100644
--- a/viz/tap/api/grpc_server.go
+++ b/viz/tap/api/grpc_server.go
@@ -102,7 +102,7 @@ func (s *GRPCTapServer) TapByResource(req *tapPb.TapByResourceRequest, stream ta
 
 	if len(pods) == 0 {
 		var errs strings.Builder
-		fmt.Fprintf(&errs, "no pods to tap for %s/%s\n", res.GetType(), res.GetName())
+		fmt.Fprintf(&errs, "no pods to tap for type=%q name=%q\n", res.GetType(), res.GetName())
 		if len(tapDisabled) > 0 {
 			fmt.Fprintf(&errs, "%d pods found with tap disabled via the %s annotation:\n", len(tapDisabled), vizLabels.VizTapDisabled)
 			for _, pod := range tapDisabled {
@@ -120,7 +120,7 @@ func (s *GRPCTapServer) TapByResource(req *tapPb.TapByResourceRequest, stream ta
 		return status.Errorf(codes.NotFound, errs.String())
 	}
 
-	log.Infof("Tapping %d pods for target: %s", len(pods), res.String())
+	log.Infof("Tapping %d pods for target: %q", len(pods), res.String())
 
 	events := make(chan *tapPb.TapEvent)
 
diff --git a/viz/tap/api/handlers.go b/viz/tap/api/handlers.go
index 4a9b3085a..a9bb0ce83 100644
--- a/viz/tap/api/handlers.go
+++ b/viz/tap/api/handlers.go
@@ -114,7 +114,7 @@ func (h *handler) handleTap(w http.ResponseWriter, req *http.Request, p httprout
 	} else if len(path) == 10 {
 		resource = path[7]
 	} else {
-		err := fmt.Errorf("invalid path: %s", req.URL.Path)
+		err := fmt.Errorf("invalid path: %q", req.URL.Path)
 		h.log.Error(err)
 		renderJSONError(w, err, http.StatusBadRequest)
 		return
@@ -140,7 +140,7 @@ func (h *handler) handleTap(w http.ResponseWriter, req *http.Request, p httprout
 		req.Header.Values(h.groupHeader),
 	)
 	if err != nil {
-		err = fmt.Errorf("tap authorization failed (%s), visit %s for more information", err, pkg.TapRbacURL)
+		err = fmt.Errorf("tap authorization failed (%w), visit %s for more information", err, pkg.TapRbacURL)
 		h.log.Error(err)
 		renderJSONError(w, err, http.StatusForbidden)
 		return
@@ -149,7 +149,7 @@ func (h *handler) handleTap(w http.ResponseWriter, req *http.Request, p httprout
 	tapReq := pb.TapByResourceRequest{}
 	err = protohttp.HTTPRequestToProto(req, &tapReq)
 	if err != nil {
-		err = fmt.Errorf("Error decoding Tap Request proto: %s", err)
+		err = fmt.Errorf("Error decoding Tap Request proto: %w", err)
 		h.log.Error(err)
 		protohttp.WriteErrorToHTTPResponse(w, err)
 		return
@@ -157,7 +157,7 @@ func (h *handler) handleTap(w http.ResponseWriter, req *http.Request, p httprout
 
 	url := pkg.TapReqToURL(&tapReq)
 	if url != req.URL.Path {
-		err = fmt.Errorf("tap request body did not match APIServer URL: %+v != %+v", url, req.URL.Path)
+		err = fmt.Errorf("tap request body did not match APIServer URL: %+v != %q", url, req.URL.Path)
 		h.log.Error(err)
 		protohttp.WriteErrorToHTTPResponse(w, err)
 		return
diff --git a/viz/tap/pkg/protohttp.go b/viz/tap/pkg/protohttp.go
index 26ec53542..ceff84c59 100644
--- a/viz/tap/pkg/protohttp.go
+++ b/viz/tap/pkg/protohttp.go
@@ -2,6 +2,7 @@ package pkg
 
 import (
 	"fmt"
+	"net/url"
 
 	"github.com/linkerd/linkerd2/pkg/k8s"
 	tapPb "github.com/linkerd/linkerd2/viz/tap/gen/tap"
@@ -16,13 +17,17 @@ func TapReqToURL(req *tapPb.TapByResourceRequest) string {
 	if res.GetType() == k8s.Namespace {
 		return fmt.Sprintf(
 			"/apis/tap.linkerd.io/v1alpha1/watch/namespaces/%s/tap",
-			res.GetName(),
+			url.PathEscape(res.GetName()),
 		)
 	}
 
 	// namespaced
 	return fmt.Sprintf(
 		"/apis/tap.linkerd.io/v1alpha1/watch/namespaces/%s/%s/%s/tap",
-		res.GetNamespace(), res.GetType()+"s", res.GetName(),
+		url.PathEscape(res.GetNamespace()),
+		// FIXME(olix0r): This pluralization is probably not correct for all
+		// resource types.
+		url.PathEscape(res.GetType()+"s"),
+		url.PathEscape(res.GetName()),
 	)
 }
diff --git a/web/app/package.json b/web/app/package.json
index f3dae7420..459fb7fdf 100644
--- a/web/app/package.json
+++ b/web/app/package.json
@@ -9,7 +9,7 @@
     "@fortawesome/free-regular-svg-icons": "5.15.4",
     "@fortawesome/free-solid-svg-icons": "5.15.4",
     "@fortawesome/react-fontawesome": "0.1.16",
-    "@lingui/core": "^3.13.0",
+    "@lingui/core": "^3.13.2",
     "@lingui/macro": "3.13.1",
     "@lingui/react": "3.13.0",
     "@material-ui/core": "4.12.3",
@@ -41,7 +41,7 @@
     "whatwg-fetch": "3.6.2"
   },
   "devDependencies": {
-    "@babel/core": "^7.16.7",
+    "@babel/core": "^7.16.12",
     "@babel/eslint-parser": "^7.16.5",
     "@babel/plugin-proposal-class-properties": "^7.16.7",
     "@babel/preset-env": "^7.16.11",
diff --git a/web/app/yarn.lock b/web/app/yarn.lock
index 28eacd371..7dddd9a56 100644
--- a/web/app/yarn.lock
+++ b/web/app/yarn.lock
@@ -14,20 +14,20 @@
   resolved "https://registry.yarnpkg.com/@babel/compat-data/-/compat-data-7.16.8.tgz#31560f9f29fdf1868de8cb55049538a1b9732a60"
   integrity sha512-m7OkX0IdKLKPpBlJtF561YJal5y/jyI5fNfWbPxh2D/nbzzGI4qRyrD8xO2jB24u7l+5I2a43scCG2IrfjC50Q==
 
-"@babel/core@^7.1.0", "@babel/core@^7.12.3", "@babel/core@^7.16.7", "@babel/core@^7.7.2", "@babel/core@^7.8.0":
-  version "7.16.7"
-  resolved "https://registry.yarnpkg.com/@babel/core/-/core-7.16.7.tgz#db990f931f6d40cb9b87a0dc7d2adc749f1dcbcf"
-  integrity sha512-aeLaqcqThRNZYmbMqtulsetOQZ/5gbR/dWruUCJcpas4Qoyy+QeagfDsPdMrqwsPRDNxJvBlRiZxxX7THO7qtA==
+"@babel/core@^7.1.0", "@babel/core@^7.12.3", "@babel/core@^7.16.12", "@babel/core@^7.7.2", "@babel/core@^7.8.0":
+  version "7.16.12"
+  resolved "https://registry.yarnpkg.com/@babel/core/-/core-7.16.12.tgz#5edc53c1b71e54881315923ae2aedea2522bb784"
+  integrity sha512-dK5PtG1uiN2ikk++5OzSYsitZKny4wOCD0nrO4TqnW4BVBTQ2NGS3NgilvT/TEyxTST7LNyWV/T4tXDoD3fOgg==
   dependencies:
     "@babel/code-frame" "^7.16.7"
-    "@babel/generator" "^7.16.7"
+    "@babel/generator" "^7.16.8"
     "@babel/helper-compilation-targets" "^7.16.7"
     "@babel/helper-module-transforms" "^7.16.7"
     "@babel/helpers" "^7.16.7"
-    "@babel/parser" "^7.16.7"
+    "@babel/parser" "^7.16.12"
     "@babel/template" "^7.16.7"
-    "@babel/traverse" "^7.16.7"
-    "@babel/types" "^7.16.7"
+    "@babel/traverse" "^7.16.10"
+    "@babel/types" "^7.16.8"
     convert-source-map "^1.7.0"
     debug "^4.1.0"
     gensync "^1.0.0-beta.2"
@@ -51,7 +51,7 @@
   dependencies:
     eslint-rule-composer "^0.3.0"
 
-"@babel/generator@^7.11.6", "@babel/generator@^7.16.7", "@babel/generator@^7.16.8", "@babel/generator@^7.7.2":
+"@babel/generator@^7.11.6", "@babel/generator@^7.16.8", "@babel/generator@^7.7.2":
   version "7.16.8"
   resolved "https://registry.yarnpkg.com/@babel/generator/-/generator-7.16.8.tgz#359d44d966b8cd059d543250ce79596f792f2ebe"
   integrity sha512-1ojZwE9+lOXzcWdWmO6TbUzDfqLD39CmEhN8+2cX9XkDo5yW1OpgfejfliysR2AWLpMamTiOiAp/mtroaymhpw==
@@ -310,10 +310,10 @@
     chalk "^2.0.0"
     js-tokens "^4.0.0"
 
-"@babel/parser@^7.1.0", "@babel/parser@^7.11.5", "@babel/parser@^7.14.7", "@babel/parser@^7.16.7", "@babel/parser@^7.16.8":
-  version "7.16.8"
-  resolved "https://registry.yarnpkg.com/@babel/parser/-/parser-7.16.8.tgz#61c243a3875f7d0b0962b0543a33ece6ff2f1f17"
-  integrity sha512-i7jDUfrVBWc+7OKcBzEe5n7fbv3i2fWtxKzzCvOjnzSxMfWMigAhtfJ7qzZNGFNMsCCd67+uz553dYKWXPvCKw==
+"@babel/parser@^7.1.0", "@babel/parser@^7.11.5", "@babel/parser@^7.14.7", "@babel/parser@^7.16.10", "@babel/parser@^7.16.12", "@babel/parser@^7.16.7":
+  version "7.16.12"
+  resolved "https://registry.yarnpkg.com/@babel/parser/-/parser-7.16.12.tgz#9474794f9a650cf5e2f892444227f98e28cdf8b6"
+  integrity sha512-VfaV15po8RiZssrkPweyvbGVSe4x2y+aciFCgn0n0/SJMR22cwofRV1mtnJQYcSB1wUTaA/X1LnA3es66MCO5A==
 
 "@babel/plugin-bugfix-safari-id-destructuring-collision-in-function-expression@^7.16.7":
   version "7.16.7"
@@ -995,26 +995,10 @@
     "@babel/parser" "^7.16.7"
     "@babel/types" "^7.16.7"
 
-"@babel/traverse@^7.13.0", "@babel/traverse@^7.16.7", "@babel/traverse@^7.7.2":
-  version "7.16.7"
-  resolved "https://registry.yarnpkg.com/@babel/traverse/-/traverse-7.16.7.tgz#dac01236a72c2560073658dd1a285fe4e0865d76"
-  integrity sha512-8KWJPIb8c2VvY8AJrydh6+fVRo2ODx1wYBU2398xJVq0JomuLBZmVQzLPBblJgHIGYG4znCpUZUZ0Pt2vdmVYQ==
-  dependencies:
-    "@babel/code-frame" "^7.16.7"
-    "@babel/generator" "^7.16.7"
-    "@babel/helper-environment-visitor" "^7.16.7"
-    "@babel/helper-function-name" "^7.16.7"
-    "@babel/helper-hoist-variables" "^7.16.7"
-    "@babel/helper-split-export-declaration" "^7.16.7"
-    "@babel/parser" "^7.16.7"
-    "@babel/types" "^7.16.7"
-    debug "^4.1.0"
-    globals "^11.1.0"
-
-"@babel/traverse@^7.16.8":
-  version "7.16.8"
-  resolved "https://registry.yarnpkg.com/@babel/traverse/-/traverse-7.16.8.tgz#bab2f2b09a5fe8a8d9cad22cbfe3ba1d126fef9c"
-  integrity sha512-xe+H7JlvKsDQwXRsBhSnq1/+9c+LlQcCK3Tn/l5sbx02HYns/cn7ibp9+RV1sIUqu7hKg91NWsgHurO9dowITQ==
+"@babel/traverse@^7.13.0", "@babel/traverse@^7.16.10", "@babel/traverse@^7.16.7", "@babel/traverse@^7.16.8", "@babel/traverse@^7.7.2":
+  version "7.16.10"
+  resolved "https://registry.yarnpkg.com/@babel/traverse/-/traverse-7.16.10.tgz#448f940defbe95b5a8029975b051f75993e8239f"
+  integrity sha512-yzuaYXoRJBGMlBhsMJoUW7G1UmSb/eXr/JHYM/MsOJgavJibLwASijW7oXBdw3NQ6T0bW7Ty5P/VarOs9cHmqw==
   dependencies:
     "@babel/code-frame" "^7.16.7"
     "@babel/generator" "^7.16.8"
@@ -1022,7 +1006,7 @@
     "@babel/helper-function-name" "^7.16.7"
     "@babel/helper-hoist-variables" "^7.16.7"
     "@babel/helper-split-export-declaration" "^7.16.7"
-    "@babel/parser" "^7.16.8"
+    "@babel/parser" "^7.16.10"
     "@babel/types" "^7.16.8"
     debug "^4.1.0"
     globals "^11.1.0"
@@ -1370,7 +1354,7 @@
     pseudolocale "^1.1.0"
     ramda "^0.27.1"
 
-"@lingui/conf@^3.13.0", "@lingui/conf@^3.13.1":
+"@lingui/conf@^3.13.1":
   version "3.13.1"
   resolved "https://registry.yarnpkg.com/@lingui/conf/-/conf-3.13.1.tgz#06d9ada1b9bd6a428e07a39ed5a6d0a9933abbe5"
   integrity sha512-xWJVdgNiGqYzkYIxHg4c6KqSNleN5/96M2ly+GzBOmA5H75r4hz27DfQpxuC4FuBUAPC+Mk1HTBO9/aFO0BCZg==
@@ -1382,10 +1366,10 @@
     jest-validate "^26.5.2"
     lodash.get "^4.4.2"
 
-"@lingui/core@^3.13.0":
-  version "3.13.0"
-  resolved "https://registry.yarnpkg.com/@lingui/core/-/core-3.13.0.tgz#0b83ba400b0b4abb24a86eb9e24cf9148cb2d84b"
-  integrity sha512-UDmI8UL59rLmQDDjBK8JFMX0+i3+pncl3fWG+tD2cXNJkN+MEBrhECTQ2lsM1tCk09AfiATglPPXm1e0tLxxOw==
+"@lingui/core@^3.13.0", "@lingui/core@^3.13.2":
+  version "3.13.2"
+  resolved "https://registry.yarnpkg.com/@lingui/core/-/core-3.13.2.tgz#1c8b264c7eb5d7fa11c59916542bf5fc8e1b9309"
+  integrity sha512-ic1uC+bS7hpfM4oVugHKel8UhlLAwykSv2TYw3oyeEQhvCS232nSzh+PFbP0BXK1hZ0UdHve7mODpgADuwHIQA==
   dependencies:
     "@babel/runtime" "^7.11.2"
     make-plural "^6.2.2"


Test patch:
diff --git a/test/integration/tap/tap_test.go b/test/integration/tap/tap_test.go
index 7643ef0fd..3dc836bb4 100644
--- a/test/integration/tap/tap_test.go
+++ b/test/integration/tap/tap_test.go
@@ -140,7 +140,7 @@ func TestCliTap(t *testing.T) {
 			if stderr == "" {
 				testutil.Fatal(t, "expected an error, got none")
 			}
-			expectedErr := `no pods to tap for deployment/t4
+			expectedErr := `no pods to tap for type="deployment" name="t4"
 1 pods found with tap disabled via the viz.linkerd.io/disable-tap annotation:`
 			split := strings.Split(stderr, "\n")
 			actualErr := strings.Join(split[:2], "\n")
diff --git a/viz/tap/api/grpc_server_test.go b/viz/tap/api/grpc_server_test.go
index d82f625c4..e8099b759 100644
--- a/viz/tap/api/grpc_server_test.go
+++ b/viz/tap/api/grpc_server_test.go
@@ -92,7 +92,7 @@ status:
 			},
 		},
 		{
-			err: status.Errorf(codes.NotFound, "no pods to tap for pod/emojivoto-not-meshed\n"),
+			err: status.Errorf(codes.NotFound, "no pods to tap for type=\"pod\" name=\"emojivoto-not-meshed\"\n"),
 			k8sRes: []string{`
 apiVersion: v1
 kind: Pod
@@ -158,7 +158,7 @@ status:
 			},
 		},
 		{
-			err: status.Errorf(codes.NotFound, "no pods to tap for pod/emojivoto-meshed\n"),
+			err: status.Errorf(codes.NotFound, "no pods to tap for type=\"pod\" name=\"emojivoto-meshed\"\n"),
 			k8sRes: []string{`
 apiVersion: v1
 kind: Pod
@@ -186,7 +186,7 @@ status:
 			},
 		},
 		{
-			err: status.Errorf(codes.NotFound, `no pods to tap for pod/emojivoto-meshed-tap-disabled
+			err: status.Errorf(codes.NotFound, `no pods to tap for type="pod" name="emojivoto-meshed-tap-disabled"
 1 pods found with tap disabled via the viz.linkerd.io/disable-tap annotation:
 	* emojivoto-meshed-tap-disabled
 remove this annotation to make these pods valid tap targets
@@ -224,7 +224,7 @@ status:
 			},
 		},
 		{
-			err: status.Errorf(codes.NotFound, `no pods to tap for pod/emojivoto-meshed-tap-not-enabled
+			err: status.Errorf(codes.NotFound, `no pods to tap for type="pod" name="emojivoto-meshed-tap-not-enabled"
 1 pods found with tap not enabled:
 	* emojivoto-meshed-tap-not-enabled
 restart these pods to enable tap and make them valid tap targets
diff --git a/viz/tap/api/handlers_test.go b/viz/tap/api/handlers_test.go
index 650160a07..ea68feda1 100644
--- a/viz/tap/api/handlers_test.go
+++ b/viz/tap/api/handlers_test.go
@@ -29,7 +29,7 @@ func TestHandleTap(t *testing.T) {
 			},
 			code:   http.StatusBadRequest,
 			header: http.Header{"Content-Type": []string{"application/json"}},
-			body:   `{"error":"invalid path: /apis"}`,
+			body:   `{"error":"invalid path: \"/apis\""}`,
 		},
 		{
 			req: &http.Request{