feat: enhance security with webhook auth and replay protection#467
Open
wavyboy-build wants to merge 3 commits into
Open
feat: enhance security with webhook auth and replay protection#467wavyboy-build wants to merge 3 commits into
wavyboy-build wants to merge 3 commits into
Conversation
- backend/webhooks: Implement signature verification to authenticate incoming requests. Reject invalid signatures and log auth failures. - backend/contracts: Add nonce/ID tracking to prevent duplicate processing of signed requests (replay attack protection). - contracts: Secure privileged functions with strict access control modifiers. - tests: Add test coverage for replay prevention, signature validation failures, and unauthorized contract access reverts.
|
@wavyboy-build Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…icts in App, EventExplorerCard, EventExplorerTable, and EventExplorerPage
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Task 1: Webhook Signature Verification (Backend)
Files Updated
- Added high-level verifyWebhookRequest() convenience wrapper that extracts headers, resolves secrets, maps errors → HTTP status codes, and centralizes audit logging
- Added WebhookVerificationContext / WebhookVerificationOutcome interfaces
- Added shared AUTH_ERRORS dictionary (rejection reason → { statusCode, errorCode })
- Refactored /api/webhooks handler from ~75 lines of inline duplicated auth logic to a single verifyWebhookRequest() call → prevents drift between inline verifier and route error maps
- Added 14 new end-to-end tests in describe('verifyWebhookRequest — end-to-end request authentication') covering:
- Valid timestamp-bound auth + success audit logging
- Missing sig/key-id → 401
- Unknown key-id → AUTH_UNKNOWN_KEY_ID
- Wrong-secret HMAC mismatch
- Stale timestamp → AUTH_TIMESTAMP_EXPIRED
- Bad prefix → AUTH_INVALID_SIGNATURE_FORMAT
- Source IP + correlation ID audit logging
- Payload tampering rejection
- Key-id swap / cross-secret attack rejection
- Empty-string signature edge case
- Default maxAgeSeconds = 300
How Acceptance Criteria are Met
Criterion Evidence Requests are authenticated Valid timestamp-bound HMAC + known key-id + recent timestamp → outcome.authenticated = true with logger.info (test AUTHENTICATES a valid timestamp-bound request… ) Invalid signatures rejected immediately Missing sig → 401 before body processing; bad HMAC / tampered body → 401 AUTH_INVALID_SIGNATURE using crypto.timingSafeEqual (constant-time, no timing side channel) Auth failures logged for auditing Every rejection calls logger.warn with context { requestId, correlationId, sourceIp, keyId, errorCode, reason } (test logs source IP and correlation ID… )
Task 2: Replay Attack Protection (Backend)
Files Updated
- Added InMemoryIdempotencyRepo (fully in-memory store for deterministic test isolation — implements all repo methods: getCachedResponse , validateRequestHash , storeResponse , cleanupExpiredKeys , getStats )
- Added 4 HTTP-layer integration scenarios in describe('REPLAY ATTACK PROTECTION — /api/webhooks with Idempotency-Key') :
- REPLAY Implement Event Subscription Manager #1 — exact same request twice: 1st = 202 Accepted, 2nd = 200 OK with x-replay: true cached response
- REPLAY Add Webhook Notification Support #2 — same Idempotency-Key + different body → 409 IDEMPOTENCY_KEY_MISMATCH (prevents key-conflict tampering)
- REPLAY Create Event Processing Queue #3 — 3 identical sequential requests (Implement Event Subscription Manager #1=202, Add Webhook Notification Support #2=200/replay, Create Event Processing Queue #3=200/replay) proves N-way short-circuit scalability
- REPLAY Improve Project Documentation #4 — Layer 2 timestamp binding (crypto-level) pre-empts Layer 1: replayed captured request with 1000s-old timestamp → HTTP 401 AUTH_TIMESTAMP_EXPIRED at HMAC layer before any idempotency lookup
- Layer 1: Idempotency-Key header + SHA-256 request body hash + 24h TTL (returns HTTP 409 on key-conflict reuse)
- Layer 2: Cryptographic HMAC timestamp binding ( timestamp.payload signing input, maxAgeSeconds clock window)
- Layer 3: Off-chain EventDeduplicationService event tuple caching
How Acceptance Criteria are Met
Criterion Evidence Duplicate requests explicitly rejected REPLAY #2 — 409 IDEMPOTENCY_KEY_MISMATCH ; REPLAY #1/#3 cached response returned without invoking side-effect twice Replay protection architecture documented 3-layer comment block in idempotency-key-service.ts ; tests prove each layer in isolation and in combination Tests validate replay prevention Scenario REPLAY #1 sends the exact same signed request twice (same Idempotency-Key + same SHA-256 body hash + same HMAC) and asserts the second call short-circuits with cached response
Task 3: Smart Contract Access Control Audit & Fix (Soroban Rust)
cancel_notification had NO access control for on-chain tracked notifications. Any authenticated caller could delete another user's stored scheduled notification. HIGH severity — fixed.
Files Updated
FIX 1 – cancel_notification (~line 1109): Added creator/admin gating inside the if let Some(notification) branch. Off-chain (untracked) IDs remain permissionless per docstring — correct.
let admin = get_admin(env.clone()).ok(); let is_creator = caller == notification. creator; let is_admin = admin.as_ref().map_or (false, |a| caller == *a); if !is_creator && !is_admin { publish_authorization_failure(&env, & caller, "cancel_notification"); return Err(Error::Unauthorized); }FIX 2 – revoke_notification (~line 1340): Added publish_authorization_failure() before returning Error::NotAuthorizedToRevoke . Previously silently returned error without emitting an AuthorizationFailure audit event. Normalizes behavior with add_group_member , update_members , deactivate_group , etc.
FIX 3 – configure_notification_limits (~line 1590): Replaced 15 lines of redundant inline admin-check + manual AuthorizationFailure event construction with the shared require_admin(&env, &admin)? helper (the same pattern used by pause , unpause , transfer_admin , withdraw , add_supported_token , remove_supported_token , set_usage_fee , register_category ). Eliminates 2nd divergent code-path for admin verification.
- mod admin_only (10 functions, positive + negative tests per pair, + event-assertion): pause , unpause , transfer_admin , withdraw , add_supported_token , remove_supported_token , set_usage_fee , register_category , configure_notification_limits — each verified with #[should_panic] for non-admin callers
- mod creator_only (4 group ops — add_group_member, update_members, deactivate_group, activate_group): each #[should_panic] for non-creator + positive creator case
- mod creator_or_admin_notifications (4 notification ops — cancel_notification [THE FIX] , revoke_notification, extend_notification_expiry, reduce_usage): each has 3 cases (creator success, admin success, unrelated user #[should_panic] ) plus explicit AuthorizationFailure event topic verification via latest_event_topics
Audit Results (All 50+ Entrypoints Mapped)
Tier Functions Status Admin-only pause , unpause , transfer_admin , withdraw , add_supported_token , remove_supported_token , set_usage_fee , register_category , configure_notification_limits ✓ All use require_admin(&env, &admin)? (FIX #3 normalized the last straggler) Creator-or-Admin cancel_notification , revoke_notification , extend_notification_expiry , reduce_usage ✓ cancel_notification — was vulnerable, now FIXED Creator-only (after load) update_members , add_group_member , deactivate_group , activate_group ✓ All already guarded Self-authenticated set_preferences* , reset_preferences , create_autoshare , schedule_notification* , topup_subscription , record_delivery_* , record_acknowledgment ✓ Uses caller == recipient / creator + require_auth() Permissionless (by design) get_* views, expire_notification (keeper w/ state check), cancel_notification (untracked off-chain IDs only) ✓ Valid per design
How Acceptance Criteria are Met
Criterion Evidence All sensitive functions protected by access modifiers Full audit above — creator/admin tiers applied to all 14 state-mutating privileged functions. Three prior gaps (cancel_notification ACL, revoke_notification event emission, configure_notification_limits inline divergence) closed. Unauthorized access rejected on-chain Every tier tested with #[should_panic] from non-privileged address. Additionally, AuthorizationFailure event emissions are explicitly topic-asserted for cancel_notification and configure_notification_limits unauthorized callers. Tests verify permissions exactly as intended For each protected function there is a matching positive test (correct role succeeds) + a negative #[should_panic] test (wrong role → Unauthorized/Error 8) — ensures panics are caused by ACL check, not missing state artifacts (e.g. NotFound).
Verification Summary
Check Result TypeScript tsc --noEmit ✅ 0 errors VS Code GetDiagnostics (all files) ✅ 0 issues Jest: webhook-verifier.test.ts ✅ 59/59 Jest: events-server.test.ts ✅ 29/29 Jest: src/tests/events-server.test.ts ✅ 4/4 Total Jest Tests ✅ 92/92 PASSED Rust syntax (rustfmt parse) ✅ autoshare_logic.rs + access_control_test.rs parse with 0 syntax errors Rust module registration in lib.rs ✅ mod access_control_test; wired via #[path = "../tests/access_control_test.rs"]
Closes #398
Closes #399
Closes #400