Repository: buchgr/bazel-remote
Pull request #515 (2021-12-27 15:54:09)

Validate input and output ActionResults more thoroughly


Patch:
diff --git a/cache/disk/BUILD.bazel b/cache/disk/BUILD.bazel
index 7bde4ea..7250d8e 100644
--- a/cache/disk/BUILD.bazel
+++ b/cache/disk/BUILD.bazel
@@ -16,6 +16,7 @@ go_library(
         "//cache/disk/casblob:go_default_library",
         "//genproto/build/bazel/remote/execution/v2:go_default_library",
         "//utils/tempfile:go_default_library",
+        "//utils/validate:go_default_library",
         "@com_github_djherbis_atime//:go_default_library",
         "@com_github_prometheus_client_golang//prometheus:go_default_library",
         "@org_golang_google_grpc//codes:go_default_library",
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index 778cb9e..6ad7222 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -23,6 +23,7 @@ import (
 	"github.com/buchgr/bazel-remote/cache"
 	"github.com/buchgr/bazel-remote/cache/disk/casblob"
 	"github.com/buchgr/bazel-remote/utils/tempfile"
+	"github.com/buchgr/bazel-remote/utils/validate"
 
 	"github.com/djherbis/atime"
 
@@ -36,8 +37,6 @@ var tfc = tempfile.NewCreator()
 
 var emptyZstdBlob = []byte{40, 181, 47, 253, 32, 0, 1, 0, 0}
 
-var hashKeyRegex = regexp.MustCompile("^[a-f0-9]{64}$")
-
 type Cache interface {
 	Get(ctx context.Context, kind cache.EntryKind, hash string, size int64, offset int64) (io.ReadCloser, int64, error)
 	GetValidatedActionResult(ctx context.Context, hash string) (*pb.ActionResult, []byte, error)
@@ -332,7 +331,7 @@ func migrateDirectory(baseDir string, kind cache.EntryKind) error {
 					continue
 				}
 
-				if !hashKeyRegex.MatchString(oldName) {
+				if !validate.HashKeyRegex.MatchString(oldName) {
 					log.Println("Warning: skipping unexpected file:", oldNamePath)
 					continue
 				}
@@ -391,7 +390,7 @@ func migrateV1Subdir(oldDir string, destDir string, kind cache.EntryKind) error
 
 			oldPath := path.Join(oldDir, item.Name())
 
-			if !hashKeyRegex.MatchString(item.Name()) {
+			if !validate.HashKeyRegex.MatchString(item.Name()) {
 				return fmt.Errorf("Unexpected file: %s", oldPath)
 			}
 
@@ -409,7 +408,7 @@ func migrateV1Subdir(oldDir string, destDir string, kind cache.EntryKind) error
 	for _, item := range listing {
 		oldPath := path.Join(oldDir, item.Name())
 
-		if !hashKeyRegex.MatchString(item.Name()) {
+		if !validate.HashKeyRegex.MatchString(item.Name()) {
 			return fmt.Errorf("Unexpected file: %s %s", oldPath, item.Name())
 		}
 
@@ -1105,20 +1104,14 @@ func (c *diskCache) GetValidatedActionResult(ctx context.Context, hash string) (
 		return nil, nil, err
 	}
 
-	for _, f := range result.OutputFiles {
-		if f == nil {
-			return nil, nil, fmt.Errorf("nil output file")
-		}
-		if f.Path == "" {
-			return nil, nil, fmt.Errorf("empty path: %q", f.Path)
-		}
-		if strings.HasPrefix(f.Path, "/") {
-			return nil, nil, fmt.Errorf("absolute path in output file: %q", f.Path)
-		}
-		if f.Digest == nil {
-			return nil, nil, fmt.Errorf("nil digest for path: %q", f.Path)
-		}
+	// Validate the ActionResult's immediate fields, but don't check for dependent blobs.
+	err = validate.ActionResult(result)
+	if err != nil {
+		return nil, nil, err // Should we return "not found" instead of an error?
+	}
 
+	for _, f := range result.OutputFiles {
+		// f was validated in validate.ActionResult but blobs were not checked for existence
 		if len(f.Contents) == 0 {
 			found, _ := c.Contains(ctx, cache.CAS, f.Digest.Hash, f.Digest.SizeBytes)
 			if !found {
@@ -1128,16 +1121,7 @@ func (c *diskCache) GetValidatedActionResult(ctx context.Context, hash string) (
 	}
 
 	for _, d := range result.OutputDirectories {
-		if d == nil {
-			return nil, nil, fmt.Errorf("nil output directory")
-		}
-		if strings.HasPrefix(d.Path, "/") {
-			return nil, nil, fmt.Errorf("absolute path in output directory: %q", d.Path)
-		}
-		if d.TreeDigest == nil {
-			return nil, nil, fmt.Errorf("nil tree digest for path: %q", d.Path)
-		}
-
+		// d was validated in validate.ActionResult but blobs were not checked for existence
 		r, size, err := c.Get(ctx, cache.CAS, d.TreeDigest.Hash, d.TreeDigest.SizeBytes, 0)
 		if r == nil {
 			return nil, nil, err // aka "not found", or an err if non-nil
diff --git a/server/BUILD.bazel b/server/BUILD.bazel
index 0b8c3c3..b2ecae4 100644
--- a/server/BUILD.bazel
+++ b/server/BUILD.bazel
@@ -22,6 +22,7 @@ go_library(
         "//genproto/build/bazel/remote/execution/v2:go_default_library",
         "//genproto/build/bazel/semver:go_default_library",
         "//utils/idle:go_default_library",
+        "//utils/validate:go_default_library",
         "//utils/zstdpool:go_default_library",
         "@com_github_abbot_go_http_auth//:go_default_library",
         "@com_github_klauspost_compress//zstd:go_default_library",
diff --git a/server/grpc.go b/server/grpc.go
index 9801420..4966785 100644
--- a/server/grpc.go
+++ b/server/grpc.go
@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"regexp"
 
 	"google.golang.org/genproto/googleapis/bytestream"
 	"google.golang.org/grpc"
@@ -21,6 +20,7 @@ import (
 
 	"github.com/buchgr/bazel-remote/cache"
 	"github.com/buchgr/bazel-remote/cache/disk"
+	"github.com/buchgr/bazel-remote/utils/validate"
 
 	_ "github.com/mostynb/go-grpc-compression/snappy" // Register snappy
 	_ "github.com/mostynb/go-grpc-compression/zstd"   // and zstd support.
@@ -31,11 +31,6 @@ const (
 	emptySha256   = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
 )
 
-var (
-	// Cache keys must be lower case asciified SHA256 sums.
-	hashKeyRegex = regexp.MustCompile("^[a-f0-9]{64}$")
-)
-
 type grpcServer struct {
 	cache                    disk.Cache
 	accessLogger             cache.Logger
@@ -141,7 +136,7 @@ func (s *grpcServer) validateHash(hash string, size int64, logPrefix string) err
 		return status.Error(codes.InvalidArgument, msg)
 	}
 
-	if !hashKeyRegex.MatchString(hash) {
+	if !validate.HashKeyRegex.MatchString(hash) {
 		msg := "Malformed hash"
 		s.accessLogger.Printf("%s %s: %s", logPrefix, hash, msg)
 		return status.Error(codes.InvalidArgument, msg)
diff --git a/server/grpc_ac.go b/server/grpc_ac.go
index 6cfd631..9a0b543 100644
--- a/server/grpc_ac.go
+++ b/server/grpc_ac.go
@@ -11,13 +11,15 @@ import (
 	"net"
 	"strings"
 
+	"github.com/buchgr/bazel-remote/cache"
+	"github.com/buchgr/bazel-remote/utils/validate"
+
 	pb "github.com/buchgr/bazel-remote/genproto/build/bazel/remote/execution/v2"
+
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/peer"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
-
-	"github.com/buchgr/bazel-remote/cache"
 )
 
 var (
@@ -82,6 +84,13 @@ func (s *grpcServer) GetActionResult(ctx context.Context,
 			return nil, status.Error(codes.Unknown, err.Error())
 		}
 
+		// This doesn't check deps, but does check for invalid fields.
+		err = validate.ActionResult(result)
+		if err != nil {
+			s.accessLogger.Printf("%s %s %s", logPrefix, req.ActionDigest.Hash, err)
+			return nil, status.Error(codes.Internal, err.Error())
+		}
+
 		s.accessLogger.Printf("%s %s OK", logPrefix, req.ActionDigest.Hash)
 		return result, nil
 	}
@@ -208,6 +217,12 @@ func (s *grpcServer) UpdateActionResult(ctx context.Context,
 		return nil, err
 	}
 
+	// Validate the ActionResult's immediate fields, but don't check for dependent blobs.
+	err = validate.ActionResult(req.ActionResult)
+	if err != nil {
+		return nil, err
+	}
+
 	// Ensure that the serialized ActionResult has non-zero length.
 	addWorkerMetadataGRPC(ctx, req.ActionResult)
 
diff --git a/server/http.go b/server/http.go
index 1ebca4d..39fe8e5 100644
--- a/server/http.go
+++ b/server/http.go
@@ -17,7 +17,10 @@ import (
 
 	"github.com/buchgr/bazel-remote/cache"
 	"github.com/buchgr/bazel-remote/cache/disk"
+	"github.com/buchgr/bazel-remote/utils/validate"
+
 	pb "github.com/buchgr/bazel-remote/genproto/build/bazel/remote/execution/v2"
+
 	"github.com/klauspost/compress/zstd"
 	"google.golang.org/protobuf/encoding/protojson"
 	"google.golang.org/protobuf/proto"
@@ -349,16 +352,30 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 			}
 
 			// Ensure that the serialized ActionResult has non-zero length.
-			data, code, err := addWorkerMetadataHTTP(r.RemoteAddr, r.Header.Get("Content-Type"), data)
+			ar, code, err := addWorkerMetadataHTTP(r.RemoteAddr, r.Header.Get("Content-Type"), data)
 			if err != nil {
 				http.Error(w, err.Error(), code)
 				h.errorLogger.Printf("PUT %s: %s", path(kind, hash), err.Error())
 				return
 			}
-			contentLength = int64(len(data))
 
-			// Note: we do not currently verify that the blobs exist
-			// in the CAS.
+			// Note: we do not currently verify that the blobs exist in the CAS.
+			err = validate.ActionResult(ar)
+			if err != nil {
+				http.Error(w, err.Error(), http.StatusBadRequest)
+				h.errorLogger.Printf("PUT %s: %s", path(kind, hash), err.Error())
+				return
+			}
+
+			data, err = proto.Marshal(ar)
+			if err != nil {
+				msg := "Failed to marshal ActionResult"
+				http.Error(w, msg, http.StatusInternalServerError)
+				h.errorLogger.Printf("PUT %s: %s", path(kind, hash), msg)
+				return
+			}
+
+			contentLength = int64(len(data))
 
 			rdr = bytes.NewReader(data)
 		}
@@ -424,7 +441,7 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func addWorkerMetadataHTTP(addr string, ct string, orig []byte) (data []byte, code int, err error) {
+func addWorkerMetadataHTTP(addr string, ct string, orig []byte) (actionResult *pb.ActionResult, code int, err error) {
 	ar := &pb.ActionResult{}
 	if ct == "application/json" {
 		err = protojson.Unmarshal(orig, ar)
@@ -432,13 +449,13 @@ func addWorkerMetadataHTTP(addr string, ct string, orig []byte) (data []byte, co
 		err = proto.Unmarshal(orig, ar)
 	}
 	if err != nil {
-		return orig, http.StatusBadRequest, err
+		return nil, http.StatusBadRequest, err
 	}
 
 	if ar.ExecutionMetadata == nil {
 		ar.ExecutionMetadata = &pb.ExecutedActionMetadata{}
 	} else if ar.ExecutionMetadata.Worker != "" {
-		return orig, http.StatusOK, nil
+		return ar, http.StatusOK, nil
 	}
 
 	worker := addr
@@ -451,12 +468,7 @@ func addWorkerMetadataHTTP(addr string, ct string, orig []byte) (data []byte, co
 
 	ar.ExecutionMetadata.Worker = worker
 
-	data, err = proto.Marshal(ar)
-	if err != nil {
-		return orig, http.StatusInternalServerError, err
-	}
-
-	return data, http.StatusOK, nil
+	return ar, http.StatusOK, nil
 }
 
 // Produce a debugging page with some stats about the cache.
diff --git a/utils/validate/BUILD.bazel b/utils/validate/BUILD.bazel
new file mode 100644
index 0000000..8c31aaa
--- /dev/null
+++ b/utils/validate/BUILD.bazel
@@ -0,0 +1,9 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["action_result.go"],
+    importpath = "github.com/buchgr/bazel-remote/utils/validate",
+    visibility = ["//visibility:public"],
+    deps = ["//genproto/build/bazel/remote/execution/v2:go_default_library"],
+)
diff --git a/utils/validate/action_result.go b/utils/validate/action_result.go
new file mode 100644
index 0000000..ccfe666
--- /dev/null
+++ b/utils/validate/action_result.go
@@ -0,0 +1,151 @@
+package validate
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+
+	pb "github.com/buchgr/bazel-remote/genproto/build/bazel/remote/execution/v2"
+)
+
+var (
+	errNilActionResult = fmt.Errorf("nil *ActionResult")
+
+	errNegativeDigest = fmt.Errorf("Digest has negative SizeBytes")
+
+	errNilOutputFile = fmt.Errorf("nil output file")
+	errEmptyPath     = fmt.Errorf("empty path")
+	errNilOutputDir  = fmt.Errorf("nil output directory")
+
+	errNilOuputFileSymlink           = fmt.Errorf("nil *OutputSymlink in OutputFileSymlinks")
+	errEmptyOutputFileSymlinksPath   = fmt.Errorf("empty path in OutputFileSymlinks")
+	errEmptyOutputFileSymlinksTarget = fmt.Errorf("empty target in OutputFileSymlinks")
+
+	errNilOuputSymlink           = fmt.Errorf("nil *OutputSymlink in OuputSymlinks")
+	errEmptyOutputSymlinksPath   = fmt.Errorf("empty path in OutputSymlinks")
+	errEmptyOutputSymlinksTarget = fmt.Errorf("empty target in OutputSymlinks")
+
+	errNilOuputDirSymlink           = fmt.Errorf("nil *OutputSymlink in OutputDirectorySymlinks")
+	errEmptyOutputDirSymlinksPath   = fmt.Errorf("empty path in OutputDirectorySymlinks")
+	errEmptyOutputDirSymlinksTarget = fmt.Errorf("empty target in OutputDirectorySymlinks")
+)
+
+// Cache keys must be lower case asciified SHA256 sums.
+var HashKeyRegex = regexp.MustCompile("^[a-f0-9]{64}$")
+
+// Validate the immediate fields in ar, but don't verify ar's
+// dependent blobs.
+func ActionResult(ar *pb.ActionResult) error {
+	if ar == nil {
+		return errNilActionResult
+	}
+
+	var err error
+
+	for _, f := range ar.OutputFiles {
+		if f == nil {
+			return errNilOutputFile
+		}
+		if f.Path == "" {
+			return errEmptyPath
+		}
+		if strings.HasPrefix(f.Path, "/") {
+			return fmt.Errorf("absolute path in output file: %q", f.Path)
+		}
+		if f.Digest == nil {
+			return fmt.Errorf("nil Digest for path %q", f.Path)
+		}
+		err = maybeNilDigest(f.Digest) // No need to re-check for nil.
+		if err != nil {
+			return fmt.Errorf("invalid Digest for path %q: %w", f.Path, err)
+		}
+	}
+
+	for _, d := range ar.OutputDirectories {
+		if d == nil {
+			return errNilOutputDir
+		}
+		if strings.HasPrefix(d.Path, "/") {
+			return fmt.Errorf("absolute path in output directory: %q", d.Path)
+		}
+		if d.TreeDigest == nil {
+			return fmt.Errorf("nil tree digest pointer for output directory: %q", d.Path)
+		}
+		err = maybeNilDigest(d.TreeDigest) // No need to re-check for nil.
+		if err != nil {
+			return fmt.Errorf("Invalid TreeDigest for path %q: %w", d.Path, err)
+		}
+	}
+
+	for _, s := range ar.OutputFileSymlinks {
+		if s == nil {
+			return errNilOuputFileSymlink
+		}
+		if s.Path == "" {
+			return errEmptyOutputFileSymlinksPath
+		}
+		if s.Target == "" {
+			return errEmptyOutputFileSymlinksTarget
+		}
+		if strings.HasPrefix(s.Path, "/") {
+			return fmt.Errorf("absolute path in output file symlink: %q", s.Path)
+		}
+	}
+
+	for _, s := range ar.OutputSymlinks {
+		if s == nil {
+			return errNilOuputSymlink
+		}
+		if s.Path == "" {
+			return errEmptyOutputSymlinksPath
+		}
+		if s.Target == "" {
+			return errEmptyOutputSymlinksTarget
+		}
+		if strings.HasPrefix(s.Path, "/") {
+			return fmt.Errorf("absolute path in output symlink: %q", s.Path)
+		}
+	}
+
+	for _, s := range ar.OutputDirectorySymlinks {
+		if s == nil {
+			return errNilOuputDirSymlink
+		}
+		if s.Path == "" {
+			return errEmptyOutputDirSymlinksPath
+		}
+		if s.Target == "" {
+			return errEmptyOutputDirSymlinksTarget
+		}
+		if strings.HasPrefix(s.Path, "/") {
+			return fmt.Errorf("absolute path in output directory symlink: %q", s.Path)
+		}
+	}
+
+	err = maybeNilDigest(ar.StdoutDigest)
+	if err != nil {
+		return fmt.Errorf("invalid StdoutDigest: %w", err)
+	}
+	err = maybeNilDigest(ar.StderrDigest)
+	if err != nil {
+		return fmt.Errorf("invalid StderrDigest: %w", err)
+	}
+
+	return nil
+}
+
+// Verify that The digest hash and size are valid, if it is non-nil.
+func maybeNilDigest(d *pb.Digest) error {
+	if d == nil {
+		return nil
+	}
+
+	if d.SizeBytes < 0 {
+		return errNegativeDigest
+	}
+	if !HashKeyRegex.MatchString(d.Hash) {
+		return fmt.Errorf("Invalid hash: %q", d.Hash)
+	}
+
+	return nil
+}


Test patch:
diff --git a/server/grpc_test.go b/server/grpc_test.go
index 1e67ec1..c2bea53 100644
--- a/server/grpc_test.go
+++ b/server/grpc_test.go
@@ -1398,22 +1398,368 @@ func TestBadUpdateActionResultRequest(t *testing.T) {
 		SizeBytes: 1,
 	}
 
-	upACReq := pb.UpdateActionResultRequest{
-		ActionDigest: &digest,
-		ActionResult: &pb.ActionResult{
-			OutputFiles: []*pb.OutputFile{
-				{
-					Path:     "foo/bar",
-					Contents: []byte{0, 1, 2, 3, 4},
-					// Note: nil digest.
+	// Each of these ActionResults should have exactly one invalid field,
+	// and none of them should be accepted by UpdateActioResult.
+	tcs := []struct {
+		description  string // What makes the ActionResult invalid.
+		actionResult *pb.ActionResult
+	}{
+		{
+			description: "nil *OutputFile",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{nil},
+			},
+		},
+		{
+			description: "nil OutputFile digest",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{
+					{
+						Path:     "foo/bar",
+						Contents: []byte{0, 1, 2, 3, 4},
+						// Note: nil digest.
+					},
+				},
+			},
+		},
+		{
+			description: "empty OutputFile path",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{
+					{
+						// Note: empty path
+						Contents: []byte{0, 1, 2, 3, 4},
+						Digest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283d",
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "absolute OutputFile path",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{
+					{
+						Path:     "/foo/bar", // Note: absolute path.
+						Contents: []byte{0, 1, 2, 3, 4},
+						Digest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283d",
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "OutputFile negative digest",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{
+					{
+						Path:     "foo/bar",
+						Contents: []byte{0, 1, 2, 3, 4},
+						Digest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283d",
+							SizeBytes: -5, // Note: negative.
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "OutputFile long digest",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{
+					{
+						Path:     "foo/bar",
+						Contents: []byte{0, 1, 2, 3, 4},
+						Digest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283ddddddddd", // Note: too long.
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "OutputFile short digest",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{
+					{
+						Path:     "foo/bar",
+						Contents: []byte{0, 1, 2, 3, 4},
+						Digest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283", // Note: too short.
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "OutputFile uppercase digest",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{
+					{
+						Path:     "foo/bar",
+						Contents: []byte{0, 1, 2, 3, 4},
+						Digest: &pb.Digest{
+							Hash:      "08BB5E5D6EAAC1049EDE0893D30ED022B1A4D9B5B48DB414871F51C9CB35283D", // Note: uppercase chars.
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "OutputFile non-hex digest",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{
+					{
+						Path:     "foo/bar",
+						Contents: []byte{0, 1, 2, 3, 4},
+						Digest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283g", // Note: non-hex chars.
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "nil *OutputDirectory",
+			actionResult: &pb.ActionResult{
+				OutputDirectories: []*pb.OutputDirectory{nil},
+			},
+		},
+		{
+			description: "absolute OutputDirectory path",
+			actionResult: &pb.ActionResult{
+				OutputDirectories: []*pb.OutputDirectory{
+					{
+						Path: "/foo/bar", // Note: absolute path.
+						TreeDigest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283d",
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "nil OutputDirectory TreeDigest",
+			actionResult: &pb.ActionResult{
+				OutputDirectories: []*pb.OutputDirectory{
+					{
+						Path: "foo/bar",
+						// Note: nil TreeDigest.
+					},
+				},
+			},
+		},
+		{
+			description: "OutputDirectory negative tree digest",
+			actionResult: &pb.ActionResult{
+				OutputDirectories: []*pb.OutputDirectory{
+					{
+						Path: "foo/bar",
+						TreeDigest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283d",
+							SizeBytes: -5, // Note: negative.
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "OutputDirectory long tree digest",
+			actionResult: &pb.ActionResult{
+				OutputDirectories: []*pb.OutputDirectory{
+					{
+						Path: "foo/bar",
+						TreeDigest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283dd", // Note: too long.
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "OutputDirectory short tree digest",
+			actionResult: &pb.ActionResult{
+				OutputDirectories: []*pb.OutputDirectory{
+					{
+						Path: "foo/bar",
+						TreeDigest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283", // Note: too short.
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "OutputDirectory uppercase tree digest",
+			actionResult: &pb.ActionResult{
+				OutputDirectories: []*pb.OutputDirectory{
+					{
+						Path: "foo/bar",
+						TreeDigest: &pb.Digest{
+							Hash:      "08BB5E5D6EAAC1049EDE0893D30ED022B1A4D9B5B48DB414871F51C9CB35283D", // Note: uppercase chars.
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "OutputDirectory non-hex digest",
+			actionResult: &pb.ActionResult{
+				OutputDirectories: []*pb.OutputDirectory{
+					{
+						Path: "foo/bar",
+						TreeDigest: &pb.Digest{
+							Hash:      "08bb5e5d6eaac1049ede0893d30ed022b1a4d9b5b48db414871f51c9cb35283g", // Note: non-hex chars.
+							SizeBytes: 5,
+						},
+					},
+				},
+			},
+		},
+
+		{
+			description: "nil pointer in OutputFileSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputFileSymlinks: []*pb.OutputSymlink{nil},
+			},
+		},
+		{
+			description: "absolute path in OutputFileSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputFileSymlinks: []*pb.OutputSymlink{
+					{
+						Path:   "/foo/bar", // Note: absolute path.
+						Target: "grok",
+					},
+				},
+			},
+		},
+		{
+			description: "empty path in OutputFileSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputFileSymlinks: []*pb.OutputSymlink{
+					{
+						// Note: empty path.
+						Target: "grok",
+					},
+				},
+			},
+		},
+		{
+			description: "empty target in OutputFileSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputFileSymlinks: []*pb.OutputSymlink{
+					{
+						Path: "foo/bar",
+						// Note: empty target.
+					},
+				},
+			},
+		},
+
+		{
+			description: "nil pointer in OutputSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputSymlinks: []*pb.OutputSymlink{nil},
+			},
+		},
+		{
+			description: "absolute path in OutputSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputSymlinks: []*pb.OutputSymlink{
+					{
+						Path:   "/foo/bar", // Note: absolute path.
+						Target: "grok",
+					},
+				},
+			},
+		},
+		{
+			description: "empty path in OutputSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputSymlinks: []*pb.OutputSymlink{
+					{
+						// Note: empty path.
+						Target: "grok",
+					},
+				},
+			},
+		},
+		{
+			description: "empty target in OutputSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputSymlinks: []*pb.OutputSymlink{
+					{
+						Path: "foo/bar",
+						// Note: empty target.
+					},
+				},
+			},
+		},
+
+		{
+			description: "nil pointer in OutputDirectorySymlinks",
+			actionResult: &pb.ActionResult{
+				OutputDirectorySymlinks: []*pb.OutputSymlink{nil},
+			},
+		},
+		{
+			description: "absolute path in OutputDirectorySymlinks",
+			actionResult: &pb.ActionResult{
+				OutputDirectorySymlinks: []*pb.OutputSymlink{
+					{
+						Path:   "/foo/bar", // Note: absolute path.
+						Target: "grok",
+					},
+				},
+			},
+		},
+		{
+			description: "empty path in OutputDirectorySymlinks",
+			actionResult: &pb.ActionResult{
+				OutputDirectorySymlinks: []*pb.OutputSymlink{
+					{
+						// Note: empty path.
+						Target: "grok",
+					},
+				},
+			},
+		},
+		{
+			description: "empty target in OutputDirectorySymlinks",
+			actionResult: &pb.ActionResult{
+				OutputDirectorySymlinks: []*pb.OutputSymlink{
+					{
+						Path: "foo/bar",
+						// Note: empty target.
+					},
 				},
 			},
 		},
 	}
 
-	_, err := acClient.UpdateActionResult(ctx, &upACReq)
-	if err != nil {
-		t.Fatal(err)
+	for _, tc := range tcs {
+		upACReq := pb.UpdateActionResultRequest{
+			ActionDigest: &digest,
+			ActionResult: tc.actionResult,
+		}
+
+		_, err := acClient.UpdateActionResult(ctx, &upACReq)
+		if err == nil {
+			t.Error("invalid ActionResult accepted:", tc.description)
+		}
 	}
 }
 
diff --git a/utils/validate/action_result_test.go b/utils/validate/action_result_test.go
new file mode 100644
index 0000000..2b3b137
--- /dev/null
+++ b/utils/validate/action_result_test.go
@@ -0,0 +1,61 @@
+package validate
+
+import (
+	"testing"
+
+	pb "github.com/buchgr/bazel-remote/genproto/build/bazel/remote/execution/v2"
+)
+
+func TestValidateNilPointers(t *testing.T) {
+
+	// The nil pointers in OutputFileSymlinks etc in TestBadUpdateActionResultRequest
+	// get replaced by empty structs somewhere in the protobuf library or bindings.
+	// So test those cases specifically here.
+
+	tcs := []struct {
+		description  string // What makes the ActionResult invalid.
+		actionResult *pb.ActionResult
+		expected     error
+	}{
+		{
+			description:  "nil *ActionResult",
+			actionResult: nil,
+			expected:     errNilActionResult,
+		},
+		{
+			description: "nil *OutputFile",
+			actionResult: &pb.ActionResult{
+				OutputFiles: []*pb.OutputFile{nil},
+			},
+			expected: errNilOutputFile,
+		},
+		{
+			description: "nil *OutputSymlink in OutputFileSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputFileSymlinks: []*pb.OutputSymlink{nil},
+			},
+			expected: errNilOuputFileSymlink,
+		},
+		{
+			description: "nil *OutputSymlink in OutputSymlinks",
+			actionResult: &pb.ActionResult{
+				OutputSymlinks: []*pb.OutputSymlink{nil},
+			},
+			expected: errNilOuputSymlink,
+		},
+		{
+			description: "nil *OutputSymlink in OutputDirectorySymlinks",
+			actionResult: &pb.ActionResult{
+				OutputDirectorySymlinks: []*pb.OutputSymlink{nil},
+			},
+			expected: errNilOuputDirSymlink,
+		},
+	}
+
+	for _, tc := range tcs {
+		err := ActionResult(tc.actionResult)
+		if err != tc.expected {
+			t.Error("invalid ActionResult accepted:", tc.description)
+		}
+	}
+}