Azure / Azure/azure-functions-host

GetFunctionSecretsAsync(merged: true) can throw a duplicate-key ArgumentException

Open
#11,996 1 comment 0 reactions 1 assignee Claimed by @satvu View on GitHub
bug
Dominant language
C#
Stars
2k
Forks
482
Avg merge
2d 10h
Merged PRs (30d)
36

Description

### Description

`SecretManager.GetFunctionSecretsAsync` has an optional `merged` parameter. When `merged: true`, it combines the function-specific keys with the host-level function keys:

https://github.com/Azure/azure-functions-host/blob/03797f60f190b80cdf28b30efd1b75efac67c9fb/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs#L318-L324

```csharp
if (merged)
{
// If merged is true, we combine function specific keys with host level function keys,
// prioritizing function specific keys
var hostSecrets = await GetHostSecretsAsync();
functionSecrets = functionSecrets.Union(hostSecrets.FunctionKeys.Where(s => !functionSecrets.ContainsKey(s.Key)))
.ToDictionary(kv => kv.Key, kv => kv.Value, StringComparer.OrdinalIgnoreCase);
}
```

The final `ToDictionary(...)` uses `StringComparer.OrdinalIgnoreCase`, but the `!functionSecrets.ContainsKey(s.Key)` filter uses the comparer of the `functionSecrets` instance. When `functionSecrets` was populated from the startup context cache, it is a plain (case-sensitive) `Dictionary`, because `GetFunctionSecretsOrNull` passes the deserialized dictionary through without normalizing its comparer:

https://github.com/Azure/azure-functions-host/blob/03797f60f190b80cdf28b30efd1b75efac67c9fb/src/WebJobs.Script.WebHost/StartupContextProvider.cs#L82-L94

(Note the host-secrets path a few lines above *does* normalize to `OrdinalIgnoreCase`, but the function-secrets path does not.)

### Result

If a function-scoped key and a host-scoped function key have names that differ only by case (e.g. function key `foo` and host function key `FOO`):

1. `functionSecrets.ContainsKey("FOO")` returns `false` (case-sensitive lookup), so the host key survives the filter.
2. The final `.ToDictionary(..., OrdinalIgnoreCase)` then receives both `foo` and `FOO`, which collide under `OrdinalIgnoreCase` → `ArgumentException: An item with the same key has already been added.`

The two key sets live in separate scopes and are deduplicated independently, so nothing prevents this cross-scope name overlap.

### Scope / impact

This only affects the `merged: true` code path. As far as I can tell, **no production/API code path calls `GetFunctionSecretsAsync` with `merged: true`** — all production callers (`KeysController`, `FunctionsSyncManager`, the internal authorization-level helper) use the default `merged: false`. The only callers passing `merged: true` are unit tests (`SecretManagerTests`).

### Proposed fix

Since the `merged` behavior isn't used by any production path, the simplest option is to **remove the `merged` parameter and the merge branch entirely** (and the tests that exercise it). Alternatively, if the behavior should be retained, make the filter and the final projection use a consistent `OrdinalIgnoreCase` comparer (e.g. normalize `functionSecrets` to `OrdinalIgnoreCase`, mirroring the host-secrets path in `GetFunctionSecretsOrNull`).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.