[Audit-Medium] named server registration can cross-wire dispatchers w…

[pepone]

AI-generated audit finding — this issue was opened from an automated security/correctness audit. It has not been triaged by a human yet; verify the reasoning, reproducibility, and severity before acting on it.

Medium: named server registration can cross-wire dispatchers when the same optionsName is reused — CONFIRMED

Affected code:

• src/IceRpc.Extensions.DependencyInjection/ServerServiceCollectionExtensions.cs:98-99 — AddOptions<ServerOptions>(optionsName).Configure(o => o.ConnectionOptions.Dispatcher = dispatcher)
• src/IceRpc.Extensions.DependencyInjection/ServerServiceCollectionExtensions.cs:124-125 — direct mutation in the singleton factory: options.ConnectionOptions.Dispatcher = dispatcherBuilder.Build();
• src/IceRpc/Server.cs — Server stores the supplied ServerOptions reference without defensive copy; listeners and accepted protocol connections snapshot ConnectionOptions.Dispatcher from that shared object

Verification:

Confirmed. Two failure modes arise from the same root cause (shared mutable ServerOptions instance per optionsName):

1. AddIceRpcServer(optionsName, IDispatcher) (line 93-101) wires the dispatcher through IConfigureOptions<ServerOptions> at line 99. Two calls with the same name install two Configure callbacks on the same named options; the last one wins when the options are built.
2. AddIceRpcServer(optionsName, Action<IDispatcherBuilder>) (line 113-132) resolves the ServerOptions from IOptionsMonitor<ServerOptions>.Get(optionsName) — which returns the same cached instance on every call — and mutates options.ConnectionOptions.Dispatcher at line 125 directly. Two such registrations produce two Server singletons that share one ServerOptions / ConnectionOptions; both listeners end up routing into whichever dispatcher wrote last.

Because DI containers commonly return the last registration for a non-enumerable GetRequiredService<Server>(), the first AddIceRpcServer call's factory might never run at resolve time — but any code that enumerates (GetServices<Server>(), tests, IHostedService registration of both) runs both factories and leaves the options in the last-written state.

Impact:

• Requests dispatched to the wrong service graph on the wrong listener.
• Endpoint exposure can drift silently from the configured server address, since dispatchers may encode routing/authorization assumptions.
• The bug is invisible at registration time and only surfaces at runtime.

Recommendation:

• Clone ServerOptions (and its nested ConnectionOptions) before mutating Dispatcher inside the factory. Server could also defensively copy options in its constructor, but the DI package owns the named-options contract so the fix belongs here.
• Reject duplicate server registrations for the same optionsName, or document/enforce that names must be unique. A simple TryAdd-style guard would catch the common case.
• Add tests covering two server registrations with the same name and asserting that each receives its own dispatcher.

Status: Valid, Medium severity.

---

Source report: src-IceRpc.Extensions.DependencyInjection-audit-2026-04-14.md (finding named server registration can cross-wire dispatchers when the same optionsName is reused — **CONFIRMED**)

Severity (auditor-assigned): Medium

[pepone]

Before picking a fix, we should clarify the intended contract for optionsName in AddIceRpcServer(optionsName, …). The XML docs describe it as "the name of the options instance" but don't state uniqueness constraints, so there are two reasonable interpretations with different fixes:

Interpretation A — strict: one server per options name.
Duplicate-name registrations are a user error. The current behavior (silent cross-wiring via a shared mutable ServerOptions) is just a symptom of not validating upfront. Fix: a TryAdd-style guard that throws at AddIceRpcServer call time when the same optionsName is already registered.

Interpretation B — flexible: options name is a "configuration kind", shared across instances.
The name identifies a shared template (same transport, timeouts, buffer sizes, etc.) and individual servers each overlay their own dispatcher. This is useful with ServerAddress.Port = 0 (OS-assigned ports), where multiple servers of the same "kind" can coexist without bind conflicts — think test harnesses or multi-tenant dispatch isolation behind a shared config. Fix: clone ServerOptions / ConnectionOptions inside the factory before assigning the dispatcher so each server ends up with its own options instance, no shared mutable state.

The cross-wire bug shows up in both interpretations but demands a different remedy:

• Under A, a guard is enough — we never get to the mutation.
• Under B, the clone is required and a guard would block a legitimate pattern.

The existing docs and code don't pick a side. The overload's example in the XML docs ("a server application may need to host multiple Server instances, each with its own options") points toward A, but nothing in the API actively enforces unique names, and Path 2's factory design reads like an attempt at B (reading options from IOptionsMonitor, assigning the dispatcher per-factory).

Which is the intended contract?

[externl]

If we plan to mutate the options class that's passed in, I think it makes sense to do an options clone before doing any mutations on it. That's what we currently do for the authentication options now.

Between A and B I agree that we should pick one, but I'm not sure which one is best. I guess I would learn towards A.