fix: show unsalted hash in error messages

[tyler-french]

Errors are not helpful if the key can't be connected to the original blob:

❯ grpcurl -H "x-buildbuddy-api-key: $API_KEY" \
  -d '{"blob_digest": {"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "size_bytes": 10}, "digest_function": "SHA256"}' \
  chunking.buildbuddy.dev:443 \
  build.bazel.remote.execution.v2.ContentAddressableStorage/SplitBlob
ERROR:
  Code: NotFound
  Message: Exhausted all peers attempting to read "f8b01741c4a85c18cde29fb1e72eb6408e244efe40647fef9c36be0dfe74a674".

First we can sanitize (remove salted key), and also add in blob key:

❯ grpcurl -plaintext -H "x-buildbuddy-api-key: $API_KEY" \
  -d '{"blob_digest": {"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "size_bytes": 10}, "digest_function": "SHA256"}' \
  localhost:1985\
  build.bazel.remote.execution.v2.ContentAddressableStorage/SplitBlob
ERROR:
  Code: NotFound
  Message: key "PTdefault/4416723b85b45334d6eb586e90de2456cb430e40556014e8977074f9fda39116/1/ac/v5" not found (blob e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855)

[Copilot]

Pull request overview

This PR adds functionality to sanitize error messages from the chunking package to prevent leaking internal salted hashes to callers. When the cache uses a salt for AC (Action Cache) keys, the internal salted hash should not be exposed in error messages; instead, the original blob digest should be shown.

Changes:

• Added sanitizeManifestError function to replace salted hashes with original blob hashes in error messages
• Updated Store and LoadManifest methods to sanitize errors before returning them
• Added test TestLoadWithoutManifest_SaltedHashNotLeaked to verify error message sanitization

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
server/remote_cache/chunking/chunking.go	Implements error message sanitization logic and applies it to Store and LoadManifest error paths
server/remote_cache/chunking/chunking_test.go	Adds test case to verify that salted hashes are not leaked in error messages from LoadManifest
server/remote_cache/chunking/BUILD	Adds gRPC status dependency required for the new error handling code

Comments suppressed due to low confidence (4)

server/remote_cache/chunking/chunking.go:405

• When creating a new gRPC status error, the function does not preserve any error details that may be attached to the original error. gRPC status errors can have details (such as PreconditionFailure, BadRequest, etc.) that provide additional context about the error. These details should be preserved when sanitizing the error message to maintain the full error information for callers.

	newMsg := strings.ReplaceAll(grpcStatus.Message(), saltedHash, blobHash)
	return gstatus.Error(grpcStatus.Code(), newMsg)

server/remote_cache/chunking/chunking.go:405

• Using strings.ReplaceAll could potentially replace the hash in unintended parts of the error message if the salted hash happens to appear in other contexts (e.g., in error metadata, stack traces, or other diagnostic information). While unlikely, this could lead to confusing error messages. Consider using a more targeted approach that only replaces the hash in the specific resource name format, or document this limitation.

		return fmt.Errorf("%s", strings.ReplaceAll(err.Error(), saltedHash, blobHash))
	}
	newMsg := strings.ReplaceAll(grpcStatus.Message(), saltedHash, blobHash)
	return gstatus.Error(grpcStatus.Code(), newMsg)

server/remote_cache/chunking/chunking_test.go:238

• The test only validates that LoadManifest sanitizes error messages to not leak the salted hash. However, the Store method also has error sanitization logic (line 275 in chunking.go). Consider adding a test case that validates Store also properly sanitizes error messages when cache.Set fails, to ensure consistent behavior across both operations.

func TestLoadWithoutManifest_SaltedHashNotLeaked(t *testing.T) {
	const salt = "test-salt"
	flags.Set(t, "cache.chunking.ac_key_salt", salt)

	ctx := context.Background()
	te := testenv.GetTestEnv(t)
	ctx, err := prefix.AttachUserPrefixToContext(ctx, te.GetAuthenticator())
	require.NoError(t, err)
	cache := te.GetCache()

	blobRN, _ := testdigest.RandomCASResourceBuf(t, 500)
	blobDigest := blobRN.GetDigest()

	saltedDigest, err := digest.Compute(bytes.NewReader([]byte(salt+":"+blobDigest.GetHash())), repb.DigestFunction_SHA256)
	require.NoError(t, err)

	_, err = chunking.LoadManifest(ctx, cache, blobDigest, "", repb.DigestFunction_SHA256)

	require.Error(t, err)
	require.True(t, status.IsNotFoundError(err))
	assert.Contains(t, err.Error(), blobDigest.GetHash())
	assert.NotContains(t, err.Error(), saltedDigest.GetHash())
}

server/remote_cache/chunking/chunking.go:402

• When the error is not a gRPC status error, creating a new error with fmt.Errorf loses the original error's type and breaks error unwrapping. This means status.IsNotFoundError and similar checks may fail for non-gRPC errors. Consider using fmt.Errorf with %w to preserve the error chain, but note that this would also preserve the original error message. A better approach might be to wrap the error in a custom type that preserves both the original error for unwrapping and provides the sanitized message.

		return fmt.Errorf("%s", strings.ReplaceAll(err.Error(), saltedHash, blobHash))

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.