Add opt-in digest validation header for stale parameter detection#957
Add opt-in digest validation header for stale parameter detection#957bitcoin-coder-bob wants to merge 9 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements opt-in digest validation: server stores GetInfo digest via a DigestService; gRPC interceptors validate client-sent Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Interceptor
participant DigestService as DigestService
participant Handler
rect rgba(100,150,200,0.5)
Note over Client,Handler: Initial GetInfo (exempt from check)
Client->>Interceptor: GetInfo (no x-ark-digest)
Interceptor->>Handler: invoke GetInfo
Handler->>Handler: compute digest
Handler->>DigestService: SetDigest(digest)
Handler->>Client: return GetInfo + digest
end
rect rgba(100,200,150,0.5)
Note over Client,Handler: Subsequent request with x-ark-digest
Client->>Interceptor: RegisterIntent + x-ark-digest
Interceptor->>DigestService: checkDigest(header)
alt digest matches
DigestService->>Interceptor: valid
Interceptor->>Handler: invoke RegisterIntent
Handler->>Client: return response
else digest mismatch
DigestService->>Interceptor: DIGEST_MISMATCH (FailedPrecondition)
Interceptor->>Client: return error with current_digest
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Review: Digest Validation HeaderClean implementation. Opt-in design is the right call — no breaking changes for existing clients. A few observations: Architecture
Potential Issues1. Digest only set on GetInfo call, not on startup
Is this intentional? If the digest is deterministic from server config, you could seed it at startup in 2. Stream interceptor catches mismatch only at connection time 3. Test CoverageThorough — unit tests cover all skip paths (GetInfo, non-public, no stored digest, no header, nil metadata) plus match/mismatch/update scenarios. E2E tests verify the full gRPC stack. Stream mismatch e2e handles both the "error on dial" and "error on Recv" cases. Cross-Repo ImpactSDK clients ( Overall: Solid. The lazy initialization question (point 1) is the main thing worth addressing. |
🔍 Arkana PR Review — Digest Validation HeaderScope: 558 additions, 5 deletions across 7 files — opt-in Security & Protocol Correctness
Potential Concerns
Test Coverage ✅
Cross-Repo Impact
VerdictClean implementation of an important protocol safety feature. The opt-in design and backward compatibility are well handled. Please verify the error code assertion in tests (48 vs 49) — this appears to be a bug in the test expectations. Otherwise, no blocking issues. |
🔍 Arkana PR ReviewPR: Add opt-in digest validation header for stale parameter detection ArchitectureClean design: 🐛 Bug: Error Code Mismatch in Tests
var DIGEST_MISMATCH = Code[DigestMismatchMetadata]{49, "DIGEST_MISMATCH", grpccodes.FailedPrecondition}But the unit tests in require.Equal(t, int32(48), errDetails.Code) // ← should be 49This appears in two test cases:
These tests will fail. Should be Security Review
Observations
Verdict |
🔍 Arkana PR Review —
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/test/e2e/digest_test.go (2)
95-117: Consider asserting structured mismatch details for stream errors too.You already check stream
FailedPrecondition; addingErrorDetails(DIGEST_MISMATCH+current_digest) would make stream-path contract validation consistent with unary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/test/e2e/digest_test.go` around lines 95 - 117, The test "stale digest on stream returns DIGEST_MISMATCH" currently only asserts codes.FailedPrecondition; update the error assertions after receiving from stream (after calling status.FromError(err)) to also inspect st.Details() and assert the presence of the DIGEST_MISMATCH error detail and that its current_digest field equals the expected value ("expected-current-digest" / whatever constant the unary test uses). Locate the stream test using GetEventStream and variable stream, call status.FromError(err), then iterate st.Details() (or grab the first detail), cast to the digest-mismatch proto type used elsewhere in tests (the same proto/assertion used in the unary path), and add require.Equal checks for the mismatch type and current_digest value to mirror the unary-path contract validation.
50-57: Tighten the handler-path assertion to the expected code.This currently only proves “not
FailedPrecondition”. Since the comment says handler validation should returnInvalidArgument, asserting that explicitly makes the test less permissive.🔧 Suggested assertion update
- // Handler rejects the empty request as InvalidArgument, not FailedPrecondition. - require.NotEqual(t, codes.FailedPrecondition, st.Code(), - "expected a handler error, not a digest mismatch") + // Handler rejects the empty request as InvalidArgument. + require.Equal(t, codes.InvalidArgument, st.Code(), + "expected handler validation error after digest check passed")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/test/e2e/digest_test.go` around lines 50 - 57, The test currently only asserts the error code is not codes.FailedPrecondition after calling svc.RegisterIntent(mdCtx, &arkv1.RegisterIntentRequest{}); tighten this to assert the expected handler validation code explicitly by replacing the require.NotEqual check with an equality assertion that st.Code() equals codes.InvalidArgument (using require.Equal or require.Equalf) and update the message to reflect that the handler should return InvalidArgument for an empty request.internal/interface/grpc/interceptors/digest_test.go (1)
108-119: Unify mismatch assertions to verify structured details across all mismatch paths.These subtests only assert
FailedPrecondition, so regressions inDIGEST_MISMATCHdetails (name/code/current_digest) could slip through on Indexer/unary/stream paths.♻️ Suggested helper to enforce consistent mismatch contract checks
+func requireDigestMismatch(t *testing.T, err error, expectedDigest string) { + t.Helper() + st, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, codes.FailedPrecondition, st.Code()) + + details := st.Details() + require.NotEmpty(t, details) + errDetails, ok := details[0].(*arkv1.ErrorDetails) + require.True(t, ok) + require.Equal(t, "DIGEST_MISMATCH", errDetails.Name) + require.Equal(t, expectedDigest, errDetails.Metadata["current_digest"]) +}- st, ok := status.FromError(err) - require.True(t, ok) - require.Equal(t, codes.FailedPrecondition, st.Code()) + requireDigestMismatch(t, err, "current-digest")Also applies to: 168-194, 265-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/interface/grpc/interceptors/digest_test.go` around lines 108 - 119, The test only asserts codes.FailedPrecondition for digest mismatches; add a shared assertion helper (e.g., assertDigestMismatch(t, err, expectedName, expectedCode, expectedCurrentDigest)) and call it from the existing subtests (like the "returns error on IndexerService digest mismatch" case that exercises NewDigestService, SetDigest and checkDigest) so each path verifies the full structured DIGEST_MISMATCH details (name, code, current_digest) in the returned gRPC status; implement the helper to call status.FromError(err), assert the code is FailedPrecondition, extract st.Details(), assert the first detail is the expected digest-mismatch protobuf/struct and compare its name, code and current_digest fields, then replace the simple require.Equal(t, codes.FailedPrecondition, ...) checks in the Indexer/unary/stream tests with calls to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/interface/grpc/interceptors/digest_test.go`:
- Around line 108-119: The test only asserts codes.FailedPrecondition for digest
mismatches; add a shared assertion helper (e.g., assertDigestMismatch(t, err,
expectedName, expectedCode, expectedCurrentDigest)) and call it from the
existing subtests (like the "returns error on IndexerService digest mismatch"
case that exercises NewDigestService, SetDigest and checkDigest) so each path
verifies the full structured DIGEST_MISMATCH details (name, code,
current_digest) in the returned gRPC status; implement the helper to call
status.FromError(err), assert the code is FailedPrecondition, extract
st.Details(), assert the first detail is the expected digest-mismatch
protobuf/struct and compare its name, code and current_digest fields, then
replace the simple require.Equal(t, codes.FailedPrecondition, ...) checks in the
Indexer/unary/stream tests with calls to this helper.
In `@internal/test/e2e/digest_test.go`:
- Around line 95-117: The test "stale digest on stream returns DIGEST_MISMATCH"
currently only asserts codes.FailedPrecondition; update the error assertions
after receiving from stream (after calling status.FromError(err)) to also
inspect st.Details() and assert the presence of the DIGEST_MISMATCH error detail
and that its current_digest field equals the expected value
("expected-current-digest" / whatever constant the unary test uses). Locate the
stream test using GetEventStream and variable stream, call
status.FromError(err), then iterate st.Details() (or grab the first detail),
cast to the digest-mismatch proto type used elsewhere in tests (the same
proto/assertion used in the unary path), and add require.Equal checks for the
mismatch type and current_digest value to mirror the unary-path contract
validation.
- Around line 50-57: The test currently only asserts the error code is not
codes.FailedPrecondition after calling svc.RegisterIntent(mdCtx,
&arkv1.RegisterIntentRequest{}); tighten this to assert the expected handler
validation code explicitly by replacing the require.NotEqual check with an
equality assertion that st.Code() equals codes.InvalidArgument (using
require.Equal or require.Equalf) and update the message to reflect that the
handler should return InvalidArgument for an empty request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fa8ed42-7747-4cc4-9208-160661270bc4
📒 Files selected for processing (2)
internal/interface/grpc/interceptors/digest_test.gointernal/test/e2e/digest_test.go
🔍 Arkana Review — Digest Validation Header (follow-up)New commits since last review ( SummaryOpt-in Architecture ✅
Security Observations
Minor Notes
Cross-repo Impact
LGTM — clean implementation with proper error semantics. |
|
Thanks for the updates @bitcoin-coder-bob.
No remaining concerns from my side. |
🔍 Arkana Review — arkd#957Add opt-in digest validation header for stale parameter detection (bitcoin-coder-bob) Closes #958 SummaryAdds an opt-in digest validation mechanism: the server computes a digest of its configuration parameters (exposed via Architecture
Security Review✅ No information leakage — the digest is already public via Observations
VerdictWell-designed opt-in mechanism. Clean separation of concerns, correct concurrency handling, and good test coverage. No security issues. |
🔍 Arkana PR Review —
|
|
@arakanaai those concerns are already addressed in this PR discourse or in the code |
1 participant
closes #958
Summary by CodeRabbit
New Features
Tests