Skip to content

feat: WMP (Wallet Messaging Protocol) integration - #163

Draft
leifj wants to merge 67 commits into
mainfrom
feat/wmp-integration
Draft

feat: WMP (Wallet Messaging Protocol) integration#163
leifj wants to merge 67 commits into
mainfrom
feat/wmp-integration

Conversation

@leifj

@leifj leifj commented May 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds Wallet Messaging Protocol (WMP) support as an alternative transport to WebSocket, enabling HTTP+SSE-based communication for mobile and web wallet clients.

Key Changes

Architecture & Transport

  • Extract SessionTransport interface from Session for transport-agnostic engine
  • Implement wsTransport (gorilla/websocket) and sseTransport (HTTP+SSE with ring buffer, event replay)
  • Add WMPAdapter wrapping the engine for JSON-RPC 2.0 over HTTP with SSE server-push

WMP Protocol

  • JSON-RPC 2.0 request/response via POST /api/v2/wallet/rpc
  • SSE notification stream via GET /api/v2/wallet/events
  • Session resumption with cryptographic token rotation (§4.5.2)
  • Capability negotiation per WMP spec
  • Nested sign/match sub-flows (wmp.flow.start / wmp.flow.complete)

Configuration

  • CORS: allow Wmp-Session-Id header
  • Wire WMP adapter routes in EngineProvider

Documentation

  • WMP migration analysis with design decisions (Q1–Q5)

Testing

  • HTTP+SSE endpoint and transport unit tests
  • Conformance tested with OIDF Conformance Suite on both WebSocket and WMP transports
  • Transport does not affect conformance results — failures are identical on both transports
  • See sirosid-tests triage report for details

Related

Copilot AI review requested due to automatic review settings May 15, 2026 09:52
@leifj
leifj marked this pull request as draft May 15, 2026 09:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Wallet Messaging Protocol (WMP) support as an alternative transport to the existing v2 WebSocket engine. Introduces a transport-agnostic SessionTransport interface and new HTTP+SSE / WMP JSON-RPC adapters mounted at /api/v2/wallet/{rpc,events}. Also fixes well-known URI construction for issuer metadata across three places (RFC 8615) and allows the Wmp-Session-Id header through CORS.

Changes:

  • New WMP adapter (wmphandler.go, wmphttp.go) bridging engine flows to JSON-RPC over HTTP+SSE, including session create/resume and capability negotiation.
  • Engine refactored to use a SessionTransport interface; new HTTP+SSE handler (httpsse.go) added alongside the WS handler.
  • RFC 8615 well-known URI construction in pkg/issuermetadata/resolver.go, internal/metadata/issuer.go, internal/engine/oid4vci.go; CORS updated; capability list updated.

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 20 comments.

Show a summary per file
File Description
pkg/issuermetadata/resolver.go RFC 8615 well-known URI construction (path-aware).
pkg/config/config.go Allow Wmp-Session-Id CORS header.
internal/server/providers.go Wire /api/v2/wallet/{rpc,events} to new WMP adapter.
internal/metadata/issuer.go RFC 8615 well-known URI construction.
internal/engine/wmphttp.go HTTP/SSE entrypoints for WMP RPC and event stream.
internal/engine/wmphandler.go Core WMP adapter: sessions, peers, flow bridging, sub-flows.
internal/engine/wmphandler_test.go Tests for WMP adapter behavior.
internal/engine/transport.go New SessionTransport interface; WS and SSE implementations.
internal/engine/session.go Session refactor to use transport abstraction.
internal/engine/oid4vci.go RFC 8615 well-known URI construction for OAuth AS metadata.
internal/engine/match_test.go Update test to new session struct fields.
internal/engine/httpsse.go HTTP+SSE engine handlers (handshake, RPC, events).
internal/engine/httpsse_test.go Tests for HTTP+SSE handlers and SSE transport.
internal/api/version.go Advertise wmp engine capability.
go.mod / go.sum Add go-wmp dependency.
docs/wmp-migration-analysis.md Design/migration analysis document.
configs/config.yaml Add Wmp-Session-Id allowed CORS header.

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

Comment thread internal/engine/wmphttp.go Outdated
Comment thread internal/engine/wmphttp.go Outdated
Comment thread internal/engine/wmphandler.go Outdated
Comment thread internal/engine/wmphandler.go
Comment thread internal/engine/wmphandler.go Outdated
Comment thread internal/engine/wmphandler_test.go Outdated
Comment thread internal/engine/transport.go Outdated
Comment thread internal/engine/transport.go Outdated
Comment thread internal/api/version.go Outdated
Comment thread internal/engine/wmphandler.go Outdated
@leifj
leifj force-pushed the feat/wmp-integration branch 2 times, most recently from a34e047 to 3f7108e Compare May 15, 2026 10:22
@sonarqubecloud

sonarqubecloud Bot commented Jun 9, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
7.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Leif Johansson and others added 16 commits July 3, 2026 23:35
Detailed analysis of migrating the WebSocket engine to go-wmp with
HTTP+SSE (POST /wmp/rpc + GET /wmp/events) as the primary transport,
replacing the current gorilla/websocket-based protocol.

Covers: wire protocol mapping, session model, flow handler bridging,
sign/match conventions, error code mapping, 4-phase migration plan,
and 8-week recommended sequence.
Use fetch-event-source instead of native EventSource to get custom
headers (Authorization, X-Tenant-ID) on SSE connections, custom
reconnection strategy, and Page Visibility API integration.
X-Tenant-ID header for LB routing + Session.Metadata for persistence.
Middleware validates header matches stored tenant on every request.
Validate HTTP+SSE-only on web first, then iOS/Android. Transport
choice is per-deployment, not runtime fallback.
HTTP+SSE reduces token loss severity from 'flow destroyed' to 'flow
paused' — session survives server-side, SSE reconnects with replay.
Combined with native bridge storage for defense in depth.

Q5: keep sign/match as flow progress/action, no WMP spec change.
- Extract SessionTransport interface from Session (SendJSON, ReadMessage, Close)
- Implement wsTransport wrapping gorilla/websocket.Conn
- Implement sseTransport with POST-fed incoming queue, SSE event stream,
  monotonic event IDs, ring buffer (200 cap), Last-Event-ID replay
- Refactor Session to use SessionTransport instead of *websocket.Conn
- Add Manager.HandleRPC (POST /api/v2/wallet/rpc) with Bearer JWT auth
- Add Manager.HandleEvents (GET /api/v2/wallet/events) with Bearer JWT auth
- Both HTTP+SSE endpoints enforce same security: JWT validation, session
  ownership check, tenant binding — identical to WebSocket handshake
- Register HTTP+SSE routes alongside WebSocket in EngineProvider
- Update match_test.go to use newWSTransport wrapper
- 9 HandleRPC tests: handshake, auth, ownership, tenant, method checks
- 4 HandleEvents tests: auth, session lookup, ownership enforcement
- 4 SSE transport tests: buffer, eviction, push/read, replay+stream
- 5 extractBearerToken tests: valid/invalid/missing Authorization header
- Fix userID[:8] panic for short user IDs in handleHTTPHandshake
- Add WMP adapter: session lifecycle, message delivery, flow management
- Add HTTP endpoints for WMP JSON-RPC over REST
- Register 'wmp' capability in engine version info
- Add go-wmp dependency with local replace directive
- 19 unit tests for WMP handler
- Add spec→engine action name translation in FlowAction handler:
  accept_offer→consent, provide_tx_code→provide_pin,
  authorize→authorization_complete, select_credentials→consent,
  cancel→decline. Engine-native names still accepted for backwards compat.
- Add capability negotiation in session.create: server advertises
  flows and sign capabilities, returns intersection when client
  sends capabilities_offered.
- Add session resumption with cryptographic token rotation (§4.5.2)
- Generate resumption tokens on session.create, validate+rotate on resume
- Route session.resume in HandleRPC, create new transport for resumed sessions
- Clean up resumption tokens on session close
- Replace flow.progress sign/match requests with nested sub-flows
- Server sends wmp.flow.start (flow_type=sign/match) instead of flow.progress
- Track child flows, route wmp.flow.complete to engine signCh/matchCh
- Wire handler reference on wmpSessionTransport for child flow tracking
- Route /api/v2/wallet/rpc and /api/v2/wallet/events through WMPAdapter
- Add Wmp-Session-Id to CORS allowed headers
- Replace direct manager.HandleRPC/HandleEvents with adapter methods
@leifj
leifj force-pushed the feat/wmp-integration branch from ef29c7d to afc0424 Compare July 3, 2026 21:38
@leifj

leifj commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on main (post-AS merge). Key resolution points:

  • session.go: Merged transport abstraction (SessionTransport interface) with main's anonymous session guard (UserID != "" check) and pre-computed sessionID/logLabel variables. Adapted pingLoop to type-assert *wsTransport since ping/pong is WebSocket-specific.
  • match_test.go: Updated test helper to use newWSTransport(conn) and added stopPing channel.
  • go.mod: Dropped local go-wmp replace directive (uses published module), kept main's updated virtualwebauthn and g119612 versions.
  • Skipped now-redundant commit that removed the local replace (already resolved in rebase).

Build verified clean.

leifj added 4 commits July 4, 2026 00:10
Implements Wallet Instance Attestation (WIA) as Phase 1 of the WUA
implementation plan. The wallet backend can now issue WIA JWTs after
validating a client-signed WIA-PoP.

New endpoints:
  POST /wallet-provider/wia/challenge → single-use nonce
  POST /wallet-provider/wia/generate  → WIA JWT after PoP validation

WIA JWT claims (per CS-04 / EC TS03):
  - typ: oauth-client-attestation+jwt
  - cnf: {jwk, jkt} — binds to instance DPoP key
  - wallet_name, wallet_version, wallet_link
  - attestation_source: backend_attested (Tier 3 baseline)
  - client_status: {} (placeholder for Token Status List)
  - No iss (identity from x5c chain per EC TS03 §2.2.1)
  - x5c chain in header

WIA-PoP validation:
  - typ: oauth-client-attestation-pop+jwt
  - Signature verified against jwk header
  - Nonce must match issued challenge (single-use)
  - exp, iss required

Uses Go 1.26 ecdsa.ParseUncompressedPublicKey for EC key parsing.
Includes 5 unit tests covering success path, single-use enforcement,
nonce mismatch, and expired challenge.
- Remove default struct tags from WIAConfig; set defaults in defaultConfig()
  so YAML values are never overridden by envconfig tags
- Gate WIA service wiring on cfg.WalletProvider.WIA.Enabled flag
- Gate WIA route registration on the same Enabled flag
- Remove unused ErrWIACNFMissing sentinel error
- Add best-effort pruning of expired challenges on insert to prevent
  unbounded memory growth from abandoned challenges
- Apply config.JWTLeeway to WIA-PoP time-claim validation for clock-skew
  tolerance consistent with the rest of the codebase
- Enforce max 10-minute expiry on WIA-PoP (update comment to match)
- Remove unused encoding/pem import and suppression line from tests
- Add handler-level tests (WIA disabled → 503, invalid body → 400,
  nonexistent challenge → 400)
PKCS#11 signing (pkg/signing):
- CryptoSignerES256: JWT signing via any crypto.Signer (HSM, file, PKCS#11)
- PKCS11Signer: PKCS#11-backed crypto.Signer (build tag: pkcs11)
- KeyLoader: dispatches between file-based and PKCS#11 key loading
- WalletProviderService and WIAService now use crypto.Signer abstraction
- Backward compatible: file-based keys still work without build tag

Native platform attestation (internal/service/native_attestation.go):
- Apple App Attest verification (DCAppAttestService)
- Google Play Integrity verification
- Attestation evidence passed through WIA generate endpoint
- Falls back to backend_attested if verification fails
- Produces platform_attested attestation_source on success

Attestation lifecycle config (pkg/config):
- attestation.lifetime_seconds: global attestation lifetime (default: 3600)
- attestation.status_list_mode: always/never/auto (default: never)
- attestation.status_list_url: Token Status List endpoint URL
- attestation.native_attestation.*: Apple/Google platform config

SonarCloud:
- sonar-project.properties: suppress go:S5659 false positive
  (jwt.WithValidMethods already restricts to ES256/384/512)
… verification

- Apple App Attest: CBOR parsing with fxamacker/cbor/v2, x5c chain validation
  against Apple root CA, nonce verification via leaf cert extension, rpIdHash
  and app ID validation, development mode leniency
- Google Play Integrity: JWE decryption (A256KW/A256GCM) with go-jose/v4,
  JWS signature verification (ES256), verdict parsing and validation including
  nonce, package name, timestamp freshness, and device integrity checks
- Add 15 unit tests covering error paths, struct helpers, and full E2E
  Play Integrity flow with real JWE/JWS construction
leifj added 3 commits July 14, 2026 11:01
- sonarcloud.yml: adopt main's || true pattern for resilient coverage
- sonar-project.properties: merge expanded exclusion list with oid4vp.go from main
- go.mod/go.sum: bump kin-openapi to v0.142.0, retain cbor/v2 dependency
Address Copilot review: don't ignore the error from
computeJWKThumbprint() when setting up the attestation provider.
- Fix JWT secret in test config to meet minimum 32-byte requirement
- Use store.WalletInstances() instead of nil in test WIA service setup
- Fix stale comment about StatusListIndexOffset (no such config exists)
@leifj
leifj changed the base branch from main to feat/wia-service July 14, 2026 09:23
leifj added 4 commits July 14, 2026 11:23
Resolves conflicts in session.go (keep transport abstraction + add
notifications field from wia-service) and go.sum (regenerated).

The WMP FlowStart handler already unmarshals FlowStartMessage from
params.Params JSON — the new ClientAttestation field is automatically
available to WMP clients without any WMP-specific code changes.
…lient-held

The wallet instance key NEVER resides on the backend. It lives in the
client's encrypted private data blob (protected by passkey-PRF-derived
encryption key, inaccessible by the backend).

Architecture correction:
- Remove PoPSigner, ServerSideAttestation, NewECDSAPoPSigner
  (backend cannot sign PoPs — it doesn't have the instance key)
- Replace PreSuppliedAttestation with TransportSuppliedAttestation
  (simple forwarder: client supplies both WIA + PoP, backend forwards)
- Add ClientAttestationPoP to FlowStartMessage (client signs PoP locally
  with aud = issuer AS URL before starting the flow)
- SetHeaders no longer takes audience param (PoP already has correct aud)

The client (frontend/SDK) is responsible for:
1. Obtaining WIA from /wallet-provider/wia/generate
2. Signing the PoP JWT with its instance key (aud = AS URL from offer)
3. Passing both at flow_start time

The backend just forwards them as HTTP headers. Zero crypto on the backend
for attestation — all signing is client-side (WSCA-compatible by design).
- Implement CapabilityList() — returns negotiated capabilities for session
- Implement CredentialNotification() — routes OID4VCI §10 events from
  WMP clients to the engine's notification forwarding logic (same path
  as WebSocket). Requires go-wmp v0.1.1-notification for types.
- Add security mode validation — reject 'mls' in session.create (§2.1)
- Add /.well-known/wmp-configuration discovery endpoint
- Add notifications store to WMP session creation (prevents nil panic)
- Bump go-wmp to v0.1.1-0.20260707 (CredentialNotificationParams)

Also merges feat/wia-service attestation architecture correction:
  instance key is client-held, backend only forwards WIA+PoP as headers.
leifj and others added 4 commits August 3, 2026 19:21
go.mod/go.sum: kept this branch's fxamacker/cbor and go-wmp deps, took
main's newer kin-openapi version, then ran go mod tidy.

admin_invite_handlers_test.go / admin_oidc_gate_test.go: both branches
added these independently (this branch merged an intermediate state of
feat/wia-service before the R2PS admin proxy was split out into its
own PR, carrying a since-superseded 4-arg NewAdminHandlers signature).
Took main's canonical version of both files and updated the
NewAdminHandlers call sites to this branch's 4-arg signature
(store, logger, auditor, r2psClient), passing nil for the latter two
as done elsewhere in these files.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two Critical findings from Copilot review, both confirmed still real
against current code:

- handleSessionResume validated only the resumption token, never that
  the bearer-authenticated caller (userID/tenantID from HandleWMPRPC)
  matches the session's owner. A leaked/guessed resumption token plus
  any valid bearer token (of any user) was sufficient to take over
  another user's session. Threaded userID/tenantID through
  HandleRPC -> handleSessionResume and reject on mismatch.

- The peer.Serve goroutine started by handleSessionCreate/
  handleSessionResume ran unconditional cleanup (CloseSession) when
  Serve returned - including when a resume itself closed the old
  transport to install a new one. The old goroutine's cleanup would
  then delete the *newly resumed* session out from under the client,
  undoing the resume. Added closeSessionIfCurrent, which only tears
  down a session if the wmpSession pointer in the peers map is still
  the same instance the goroutine was started for.

Also fixes two related transport.go issues from the same review:

- A second concurrent SSE connection for the same session silently
  displaced the first (overwriting sseW/sseFl/sseCtx), orphaning the
  first handler's goroutine forever blocked on <-r.Context().Done().
  Now rejected with 409 while a connection is active, checked and
  registered atomically under sseMu together with event replay.

- SendJSON held sseMu only to read the writer pointer, not around the
  actual Fprintf+Flush, so concurrent SendJSON calls (from different
  engine goroutines) could interleave their output and corrupt the SSE
  frame stream. sseMu is now held for the whole write.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…w findings

Implements real SSE event replay for the WMPAdapter/wmp.ChannelTransport
path (internal/engine/wmphttp.go, wmphandler.go): a per-session
wmpEventBuffer persists across wmp.session.resume (unlike the
per-connection ChannelTransport, which is replaced on resume and
discards whatever it hadn't delivered yet). Reconnecting clients now
get real Last-Event-ID replay with durable, session-scoped event IDs
instead of a per-connection counter that reset to 0 on every
reconnect. SessionResumeResult.MissedMessages now reports the buffer's
actual pending count instead of a hardcoded 0. The same buffer also
rejects a second concurrent GET /wmp/events for a session (previously
unguarded, unlike the already-fixed transport.go/sseTransport
implementation) rather than letting two connections race to read the
same channel and silently split the notification stream.

Also addresses the remaining Copilot review findings on this PR:
- Bound the nested sign/match sub-flow's wmp.flow.start Call() with a
  timeout instead of context.Background(), so an unresponsive client
  can't stall the engine goroutine indefinitely.
- Give FlowAction's sign/match/action channel sends a brief bounded
  wait instead of an instant reject, so a momentarily-full channel
  doesn't force clients to recover only via the server-side timeout.
- Reject non-bearer auth types in wmp.session.create (a client
  declaring type "dpop" was silently treated as bearer) and collapse
  the redundant "no auth object" / "empty token" error messages.
- HandleRPC dispatch failures now return a JSON-RPC error envelope
  instead of a plain-text 500, so JSON-RPC clients can still parse the
  response.
- Centralized the "trim a bare trailing slash before RFC 8615
  well-known URI construction" logic (previously duplicated slightly
  differently in pkg/issuermetadata/resolver.go and missing entirely
  in internal/metadata/issuer.go) into oidc.NormalizeIssuerURL, and
  fixed a discarded url.Parse/WellKnownURL error along the way.
- Fixed a stale doc comment (internal/api/version.go claimed /wmp/rpc
  + /wmp/events; actual routes are /api/v2/wallet/rpc + .../events)
  and removed a var _ = io.ReadAll import-suppression hack for an
  import that isn't actually used in the file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
session.go's WS session-create path did userID[:8] unconditionally,
unlike the WMP path's userID[:min(8, len(userID))] guard, and would
panic on a userID shorter than 8 characters.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@leifj
leifj changed the base branch from feat/wia-service to main August 4, 2026 06:21
@leifj leifj closed this Aug 4, 2026
@leifj leifj reopened this Aug 4, 2026
Retargeting this PR to main (from the stale feat/wia-service base)
surfaced ~519 uncovered "new" lines across 17 files that SonarCloud
had never analyzed before, spanning both the WMP feature itself and
unrelated pre-existing code (WIA service, admin handlers) that just
happened to diverge from main during this branch's long lifetime.

Adds real unit tests for previously-0%-or-low-coverage functions:

- internal/engine: wmphandler.go (cleanupExpired, SessionClose,
  FlowCancel, CapabilityList, CredentialNotification, FlowAction,
  FlowStart, FlowComplete, wmpSessionTransport.SendJSON/ReadMessage,
  handleSessionCreate, replayActiveFlowProgress, wmpResponseBytes),
  wmphttp.go (HandleWMPRPC, HandleWMPEvents, HandleWMPConfiguration,
  mustMarshalJSON), httpsse.go (HandleEvents), transport.go
  (wsTransport.ReadMessage/Close)
- internal/service: wia.go (wscdTypeFromAttestation and several
  partially-covered functions), wallet_provider.go
  (NewWalletProviderService, loadKeys, GenerateKeyAttestation)
- internal/api: wia_handlers.go (WIAChallenge, WIAGenerate — both now
  100%), admin_handlers.go (RegisterRoutes and CRUD error branches),
  admin_instance_handlers.go, admin_user_handlers.go (GetUserDetail,
  now 100%)
- internal/storage/memory: wallet_instance.go (GetAllByTenant,
  GetByUser, Delete, GetByID)
- internal/metadata: issuer_test.go is new — this package had zero
  test coverage; covers DiscoverIssuer end-to-end including the
  oidc.NormalizeIssuerURL call added earlier in this PR
- pkg/signing: crypto_signer.go (NewCryptoSignerES256's non-ECDSA
  rejection branch, Sign's underlying-signer-failure branch,
  parseASN1Signature directly)
- pkg/middleware: bodysize_test.go is new — BodySizeLimitMiddleware
  had no test file at all

No production code was changed; no bugs were found during this pass
(a few observations about intentional-but-non-obvious behavior were
noted inline as comments, not treated as defects).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 4, 2026 07:59
@sonarqubecloud

sonarqubecloud Bot commented Aug 4, 2026

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 81 out of 82 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

internal/api/admin_instance_handlers.go:66

  • Tenant scoping is not enforced when updating instance status. UpdateStatus is executed by instance ID only, so an admin for tenant A could suspend/revoke an instance belonging to tenant B if they know the instance ID. Fetch and verify the instance's TenantID matches :id (or update via a tenant+id filter) before applying the status change.
func (h *AdminHandlers) UpdateWalletInstanceStatus(c *gin.Context) {
	instanceID := c.Param("instance_id")

	var req updateInstanceStatusRequest
	if err := c.ShouldBindJSON(&req); err != nil {
		c.JSON(http.StatusBadRequest, gin.H{"error": "invalid request: status must be active, suspended, or revoked"})
		return
	}

	status := domain.InstanceStatus(req.Status)

	if err := h.store.WalletInstances().UpdateStatus(c.Request.Context(), instanceID, status, req.Reason); err != nil {
		if errors.Is(err, storage.ErrNotFound) {

internal/api/admin_instance_handlers.go:98

  • Tenant scoping is not enforced on delete: Delete is called by instance ID only, allowing cross-tenant deletion if an admin can supply another tenant's instance ID. Verify the instance belongs to the :id tenant before deleting.
func (h *AdminHandlers) DeleteWalletInstance(c *gin.Context) {
	instanceID := c.Param("instance_id")

	if err := h.store.WalletInstances().Delete(c.Request.Context(), instanceID); err != nil {
		if errors.Is(err, storage.ErrNotFound) {
			c.JSON(http.StatusNotFound, gin.H{"error": "wallet instance not found"})
			return
		}
		h.logger.Error("failed to delete wallet instance", zap.Error(err))
		c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to delete wallet instance"})
		return
	}

	h.emitAudit(set.EventWIDeactivated, instanceID, map[string]any{"action": "deleted"})
	c.Status(http.StatusNoContent)

internal/server/providers.go:49

  • NewAuthProvider now starts background workers immediately via services.Start(), but the owning BackendProvider.Close() does not call p.auth.Close()/services.Stop(). Since cmd/server/main.go only closes the BackendProvider resource, this leaves WIA/challenge cleanup (and any future workers) running until process exit and can also leak resources in tests that construct a BackendProvider. Ensure BackendProvider.Close() stops the auth provider's services (and ideally call p.auth.Close() before closing the store).
    internal/service/wallet_provider.go:32
  • The comment claims multi-instance uniqueness is achieved via a startup-time-derived offset, but statusIndexCounter is never initialized to a non-zero offset. As implemented it starts at 0 in every process, so multiple pods using the same status list URI will reuse the same indices and can collide. Either implement the offset/range allocation logic or adjust the comment to avoid stating a guarantee that the code does not provide.

Comment thread internal/api/admin_instance_handlers.go
leifj added 2 commits August 5, 2026 15:59
Addresses all 12 new CodeQL alerts (8 critical, 4 high) flagged on this PR.

SSRF-class (go/request-forgery, critical) in oid4vci.go, oid4vp.go (x4),
vctm.go, resolver.go, jwtutil.go: every flagged call site already routes
through an HTTP client built via cfg.HTTPClient.NewHTTPClient(), whose
DialContext blocks private/loopback/link-local IPs by default. CodeQL's
taint tracking can't see that dial-level guard, so these are false
positives; added narrowly-scoped `codeql[go/request-forgery]` suppression
comments explaining why each is safe. Also fixed resolver.go's existing
suppression, which used the legacy `lgtm[...]` syntax that GitHub's
current code scanning doesn't recognize (hence still open as alert #40).

NoSQL-injection-class (go/sql-injection, high) in challenges.go (x2),
mongodb.go, tenant.go: in every flagged bson.M{...} filter, the
user-influenced value is used only as a map VALUE under a hardcoded
string KEY. The official mongo-driver only interprets "$"-prefixed KEYS
as operators, so a plain string value can never be reinterpreted as one
regardless of its contents - not exploitable with this driver. Added
narrowly-scoped `codeql[go/sql-injection]` suppressions with reasoning,
matching the pattern already established on sibling PRs #250/#196.

Verified go build/vet/test all pass after these changes.
GetWalletInstance, UpdateWalletInstanceStatus, and DeleteWalletInstance
looked up/mutated wallet instances by instance ID alone, ignoring the
:id (tenant) path parameter. An admin with access to one tenant could
read, suspend/revoke, or delete a wallet instance belonging to a
different tenant simply by guessing or knowing its instance ID.

Add a checkInstanceTenant helper that fetches the instance and verifies
its TenantID matches the path parameter before allowing the
UpdateStatus/Delete calls to proceed (GetWalletInstance checks inline
since it already has the instance in hand); mismatches and missing
instances both return 404, matching pre-existing not-found behavior and
avoiding tenant enumeration.

Also fixes a test gap exposed by this change: TestDeleteWalletInstance_
StoreError deleted an instance that was never seeded into the store, so
it now needs seedInstance() to reach the injected store error past the
new tenant check. Adds regression tests for all three handlers
confirming cross-tenant access is rejected with 404.

Addresses unresolved Copilot review comment on PR #163
(internal/api/admin_instance_handlers.go).
Copilot AI review requested due to automatic review settings August 5, 2026 14:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 86 out of 87 changed files in this pull request and generated no new comments.

Suppressed comments (7)

internal/engine/wmphttp.go:43

  • io.LimitReader will silently truncate bodies larger than maxWMPRPCBodyBytes, which can turn an oversized request into a confusing parse failure (or partially processed JSON-RPC). Use http.MaxBytesReader so oversized payloads reliably fail with 413 instead of being truncated.
    internal/engine/httpsse.go:41
  • HandleRPC reads the request body with io.LimitReader, which silently truncates bodies beyond the limit. For request bodies, prefer http.MaxBytesReader so oversized payloads reliably fail (413) rather than being parsed as truncated JSON.
	// Read request body (bounded).
	body, err := io.ReadAll(io.LimitReader(r.Body, MaxHTTPResponseBodyBytes))
	if err != nil {
		http.Error(w, "failed to read body", http.StatusBadRequest)
		return

internal/engine/wmphttp.go:186

  • HandleWMPConfiguration advertises endpoints under /wmp/*, but the router wires WMP under /api/v2/wallet/rpc and /api/v2/wallet/events. Clients using this discovery response will call the wrong paths.
    pkg/audit/audit.go:68
  • NewFromFile is documented as loading a PEM-encoded EC private key, but the algorithm selection defaults to EdDSA for any non-ECDSA key type. That will mis-handle RSA keys and hides configuration errors. If only ECDSA is supported here, fail fast with a clear error instead of guessing an algorithm.
    internal/server/providers.go:49
  • NewAuthProvider now starts background workers via services.Start(), and a Close() method was added to stop them, but BackendProvider.Close() (the one actually called from cmd/server/main.go resource cleanup) does not call p.auth.Close(). In backend mode this means WIA/cleanup workers can keep running past shutdown and may leak goroutines/resources during tests or graceful shutdown.
    internal/engine/httpsse.go:16
  • The comment says "Authorization: ******" which looks like an accidental redaction. This is a public developer-facing doc comment; it should describe the real scheme (Bearer token) consistently with the rest of the code.
// HandleRPC handles POST requests for JSON-RPC style messages over HTTP.
// Authentication is via Authorization: Bearer <jwt> header — same token
// validation as the WebSocket handshake.
func (m *Manager) HandleRPC(w http.ResponseWriter, r *http.Request) {

internal/service/services.go:55

  • The MongoDB WIA challenge store fallback check (store.(interface{ Database() *mongo.Database })) will never succeed with the current backend wrappers: internal/backend.mongoBackend embeds a *mongodb.Store but does not expose Database(). As a result, even in MongoDB deployments the service will silently fall back to the in-memory challenge store, breaking cross-pod single-use/expiry guarantees.

leifj added a commit that referenced this pull request Sep 10, 2026
…ation PoP per request (#318)

Closes #317. Backend half of sirosfoundation/wallet-frontend#282.

The WIA cnf key must be the DPoP key (EC TS03 §2.2.1.1), and the attestation
PoP must not be replayed across PAR, token and nonce retries. Both failed
because the engine generated and held the DPoP key itself and asked for the
WIA + PoP once per flow.

New sign action `sign_client_auth`: for every outbound request that needs
client authentication the engine asks the client for exactly what that
request needs - a DPoP proof over htm/htu (with ath and the current DPoP
nonce), and/or a WIA with a freshly signed PoP for the given audience. The
client answers with dpop_key_id (its identifier for the key that is both
WIA cnf and DPoP key), so the backend never sees a DPoP private key.

Mode is decided per flow at the first authenticated request: a client that
answers the probe with dpop_key_id runs client-held for the rest of the
flow; an empty reply or a timeout falls back to the previous behaviour
(engine-held key, one request_attestation, replayed). A flow_start that
pre-resolved its attestation, or a renewal presenting dpop_jwk, is legacy
without a probe. Renewals carry dpop_key_id on flow_complete/flow_start
instead of the private JWK relay, and the §10 notification signs through
the same abstraction after the flow completed.

Works unchanged over WMP: the adapter in #163 unmarshals sign results into
SignResponseMessage; the new request params need go-wmp SignSubFlowParams
fields (separate PR) and #163's SignRequestMessage mapping to copy them.

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants