Tanya/errors

[tanyav2]

Summary by cubic

Standardized EHBP error handling with consistent status mapping and fail-closed behavior. Servers return 422 application/problem+json for key-config mismatches; both Go and JS clients surface typed errors (no auto-retry), require Ehbp-Response-Nonce, and never fall back to plaintext.

• New Features
  • Spec: defines failure classes, 400/422/500 mapping, RFC 7807 problem type, missing/invalid nonce handling, and safe rekey-and-resend guidance.
  • Server: middleware maps protocol errors, probes early decryption, and emits application/problem+json for 422 key-config mismatches.
  • Go client: detects 422 key-config problem+json and returns an error (no retry); enforces Ehbp-Response-Nonce; better error classification.
  • JS client: adds typed errors (KeyConfigMismatchError, ProtocolError, DecryptionError); detects 422 problem+json and throws without retry; enforces Ehbp-Response-Nonce.
  • Tests: cover server 422 problem responses, Go key-mismatch error, JS error behavior, and end-to-end encrypted round trips.

^{Written for commit 9e51846. Summary will update on new commits.}

[cubic-dev-ai]

8 issues found across 10 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="client/client.go">

<violation number="1" location="client/client.go:149">
P2: Rule violated: **Code Smell Detector**

Inconsistent error wrapping: this is the only `fmt.Errorf` in the file that uses `%w` (error wrapping). All other error formatting uses `%v`. Use `%v` here for consistency, or migrate the rest of the file to `%w` if wrapping is desired.</violation>

<violation number="2" location="client/client.go:210">
P2: Rule violated: **Code Smell Detector**

Inconsistent `resp.Body.Close()` style: some calls use `_ = resp.Body.Close()` (explicit discard) while others use bare `resp.Body.Close()` (implicit discard). Pick one pattern within the function for consistency — the explicit `_ =` form is generally preferred.</violation>

<violation number="3" location="client/client.go:216">
P2: Rule violated: **Code Smell Detector**

Duplicated drain-and-close block: `io.Copy(io.Discard, resp.Body)` + `resp.Body.Close()` is repeated verbatim in both branches of the `if attempt < maxConfigRetries` check. Hoist the cleanup above the conditional since both paths need it before proceeding.</violation>

<violation number="4" location="client/client.go:220">
P1: Rule violated: **Check System Design and Architectural Patterns**

`Transport` implements `http.RoundTripper`, which must be safe for concurrent use, but the new retry path calls `t.syncServerPublicKey()` — mutating `t.serverIdentity` — without any synchronization. Concurrent `RoundTrip` calls will race on `t.serverIdentity` (one goroutine writing via `syncServerPublicKey`, another reading via `EncryptRequestWithContext`). Add a `sync.RWMutex` to `Transport` and acquire a write lock around `syncServerPublicKey`, and a read lock around access to `t.serverIdentity`.</violation>
</file>

<file name="identity/middleware.go">

<violation number="1" location="identity/middleware.go:34">
P2: Rule violated: **Code Smell Detector**

Repeated boilerplate: `statusForProtocolError` is only ever called as a preamble to `sendError`, creating a 2-line pattern repeated 3 times. Since the status is always derived from the error, consolidate both into a single helper like `sendProtocolError(w http.ResponseWriter, err error, text string)` that internally calls `statusForProtocolError` and then handles the response. This eliminates the repetitive two-step dance at every call site.</violation>

<violation number="2" location="identity/middleware.go:92">
P2: Rule violated: **Code Smell Detector**

Duplicate error message `"failed to decrypt request"` is used for two distinct failure points: the `DecryptRequestWithContext` call (header/context decryption) and the probe byte read (body stream decryption). These represent different failure modes but are indistinguishable to the caller. Consider using a distinct message for the probe failure, e.g., `"failed to read decrypted request body"`.</violation>

<violation number="3" location="identity/middleware.go:109">
P2: Inconsistent Content-Length header cleanup: the `n > 0` branch deletes the stale `Content-Length` header, but the empty-body (`readErr == io.EOF`) branch sets `r.ContentLength = 0` without also clearing the header. This leaves a stale encrypted-body-size in the header, which could confuse downstream middleware that reads the raw header.</violation>
</file>

<file name="js/src/client.ts">

<violation number="1" location="js/src/client.ts:153">
P2: Retry with a `ReadableStream` body will throw. When `input` is a URL/string and `init.body` is a `ReadableStream`, the stream is consumed on the first attempt. The second attempt (after key config mismatch) will fail because the stream is already locked/disturbed. Consider draining the body to an `ArrayBuffer` before the loop, similar to how the `Request` input path already does.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P2: Rule violated: Code Smell Detector

Duplicate error message "failed to decrypt request" is used for two distinct failure points: the DecryptRequestWithContext call (header/context decryption) and the probe byte read (body stream decryption). These represent different failure modes but are indistinguishable to the caller. Consider using a distinct message for the probe failure, e.g., "failed to read decrypted request body".

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At identity/middleware.go, line 92:

<comment>Duplicate error message `"failed to decrypt request"` is used for two distinct failure points: the `DecryptRequestWithContext` call (header/context decryption) and the probe byte read (body stream decryption). These represent different failure modes but are indistinguishable to the caller. Consider using a distinct message for the probe failure, e.g., `"failed to read decrypted request body"`.</comment>

<file context>
@@ -37,21 +78,43 @@ func (i *Identity) Middleware() func(http.Handler) http.Handler {
+			n, readErr := r.Body.Read(probe)
+			if readErr != nil && readErr != io.EOF {
+				status := statusForProtocolError(readErr)
+				sendError(w, readErr, "failed to decrypt request", status)
+				_ = r.Body.Close()
+				return
</file context>

Suggested change

	sendError(w, readErr, "failed to decrypt request", status)
	sendError(w, readErr, "failed to read decrypted request body", status)

[cubic-dev-ai]

P2: Rule violated: Code Smell Detector

Repeated boilerplate: statusForProtocolError is only ever called as a preamble to sendError, creating a 2-line pattern repeated 3 times. Since the status is always derived from the error, consolidate both into a single helper like sendProtocolError(w http.ResponseWriter, err error, text string) that internally calls statusForProtocolError and then handles the response. This eliminates the repetitive two-step dance at every call site.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At identity/middleware.go, line 34:

<comment>Repeated boilerplate: `statusForProtocolError` is only ever called as a preamble to `sendError`, creating a 2-line pattern repeated 3 times. Since the status is always derived from the error, consolidate both into a single helper like `sendProtocolError(w http.ResponseWriter, err error, text string)` that internally calls `statusForProtocolError` and then handles the response. This eliminates the repetitive two-step dance at every call site.</comment>

<file context>
@@ -1,17 +1,58 @@
 	http.Error(w, text, status)
 }
 
+func statusForProtocolError(err error) int {
+	if IsKeyConfigError(err) {
+		return http.StatusUnprocessableEntity
</file context>

[cubic-dev-ai]

P2: Inconsistent Content-Length header cleanup: the n > 0 branch deletes the stale Content-Length header, but the empty-body (readErr == io.EOF) branch sets r.ContentLength = 0 without also clearing the header. This leaves a stale encrypted-body-size in the header, which could confuse downstream middleware that reads the raw header.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At identity/middleware.go, line 109:

<comment>Inconsistent Content-Length header cleanup: the `n > 0` branch deletes the stale `Content-Length` header, but the empty-body (`readErr == io.EOF`) branch sets `r.ContentLength = 0` without also clearing the header. This leaves a stale encrypted-body-size in the header, which could confuse downstream middleware that reads the raw header.</comment>

<file context>
@@ -37,21 +78,43 @@ func (i *Identity) Middleware() func(http.Handler) http.Handler {
+			} else if readErr == io.EOF {
+				_ = originalBody.Close()
+				r.Body = http.NoBody
+				r.ContentLength = 0
+			} else {
+				r.Body = originalBody
</file context>

Suggested change

	r.ContentLength = 0
	r.ContentLength = 0
	r.Header.Del("Content-Length")

[cubic-dev-ai]

1 issue found across 6 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="js/src/client.ts">

<violation number="1" location="js/src/client.ts:156">
P2: Resource leak: the response body stream is not cancelled before throwing `KeyConfigMismatchError`. The old code explicitly called `response.body?.cancel()`. Consider cancelling the body within `checkKeyConfigMismatch` or wrapping the call with cleanup.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P2: Resource leak: the response body stream is not cancelled before throwing KeyConfigMismatchError. The old code explicitly called response.body?.cancel(). Consider cancelling the body within checkKeyConfigMismatch or wrapping the call with cleanup.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At js/src/client.ts, line 156:

<comment>Resource leak: the response body stream is not cancelled before throwing `KeyConfigMismatchError`. The old code explicitly called `response.body?.cancel()`. Consider cancelling the body within `checkKeyConfigMismatch` or wrapping the call with cleanup.</comment>

<file context>
@@ -148,52 +132,37 @@ export class Transport {
-        throw new Error(`Missing ${PROTOCOL.RESPONSE_NONCE_HEADER} header`);
-      }
+    // Throws KeyConfigMismatchError if server returned 422 key-config mismatch
+    await Transport.checkKeyConfigMismatch(response);
 
-      // Decrypt response
</file context>

Suggested change

	await Transport.checkKeyConfigMismatch(response);
	try {
	await Transport.checkKeyConfigMismatch(response);
	} catch (err) {
	await response.body?.cancel();
	throw err;
	}