Avoid ArgumentException when Problem/ValidationProblem extensions conflict with defaults

[UditDewan]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Make caller-supplied ProblemDetails extensions override framework defaults instead of throwing.

Description

ControllerBase.Problem and ControllerBase.ValidationProblem merge the caller-supplied extensions dictionary into the created ProblemDetails with IDictionary.Add. When ProblemDetailsFactory has already populated a key with the same name (for example traceId, which DefaultProblemDetailsFactory always sets), the call throws ArgumentException at runtime. Callers have no way to know which keys the factory pre-populates, so passing common keys like traceId or timestamp currently crashes the request.

This change merges the entries with the dictionary indexer instead, so an explicitly passed extension overrides the default value rather than throwing. Existing behavior for non-conflicting keys is unchanged. Added regression tests covering both methods.

Fixes #66407

[dotnet-policy-service]

Thanks for your PR, @UditDewan. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[UditDewan]

@dotnet-policy-service agree

[Copilot]

Pull request overview

This PR fixes a runtime ArgumentException that can occur when callers pass extensions to ControllerBase.Problem / ControllerBase.ValidationProblem using keys already populated by ProblemDetailsFactory (notably traceId). Instead of throwing on duplicate keys, caller-supplied values now override the pre-populated defaults.

Changes:

• Update ControllerBase.Problem to merge extensions via the dictionary indexer (overwrite) rather than IDictionary.Add (throw on duplicate).
• Update ControllerBase.ValidationProblem to use the same overwrite merge behavior.
• Add regression tests verifying that passing a conflicting traceId extension does not throw and results in the caller’s value being used.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Mvc/Mvc.Core/src/ControllerBase.cs	Switch extension merge behavior to overwrite duplicates (avoids ArgumentException on keys like traceId).
src/Mvc/Mvc.Core/test/ControllerBaseTest.cs	Add tests confirming caller-supplied traceId overrides factory-provided values for both Problem and ValidationProblem.

[DeagleGross]

Thanks for the fix! Minimal API has similar behavior in Results

aspnetcore/src/Http/Http.Results/src/Results.cs

Lines 818 to 826 in 81a85fc

	private static void CopyExtensions(IEnumerable<KeyValuePair<string, object?>>? extensions, HttpValidationProblemDetails problemDetails)
	{
	if (extensions is not null)
	{
	foreach (var extension in extensions)
	{
	problemDetails.Extensions.Add(extension);
	}
	}

and Typed Results

aspnetcore/src/Http/Http.Results/src/TypedResults.cs

Lines 872 to 880 in 81a85fc

	private static void CopyExtensions(IEnumerable<KeyValuePair<string, object?>>? extensions, ProblemDetails problemDetails)
	{
	if (extensions is not null)
	{
	foreach (var extension in extensions)
	{
	problemDetails.Extensions.Add(extension);
	}
	}

Can you please change code there as well and cover with tests (ResultsTests.cs and TypedResultsTests.cs)? Thanks!

[Youssef1313]

I'm not honestly sure if this is a change we want to do. What's the use case of user needing to write a value with the same key as ASP.NET Core?

Giving that ability to users might mean that they eliminate assumptions on well-known keys that we add and what values do we expect to be there.

[UditDewan]

The main issue is the failure mode, not necessarily the ability to override framework values.

Rn, passing a key like traceId doesn't fail consistently. It only throws an ArgumentException when the factory happens to populate that key, which depends on things like Activity.Current or TraceIdentifier. There also doesn't seem to be clear documentation on which keys are reserved.

Users can already replace well-known keys through CustomizeProblemDetails, since that runs after DefaultProblemDetailsWriter sets traceId. The framework also merges these values using the indexer in DefaultProblemDetailsFactory and DefaultProblemDetailsWriter, so “last writer wins” already seems to be the existing behavior.

This PR just makes the explicit extensions parameter follow the same behavior instead of crashing. Though , if y'all would prefer framework-provided values to remain authoritative, using TryAdd would also fix the crash. I can to switch to that approach if you guys prefer

[halter73]

Approving. This should generally be deterministic, and fields like traceId will usually be added automatically, but extension names have never been documented as reserved.

The exception might expose an unnecessary user-supplied value during development, but I don't think justifies failing the request after the user code has already returned. Overwriting is a more useful and consistent behavior.