Potentially unsound trimming suppression in DefaultAntiforgeryStateProvider

[halter73]

aspnetcore/src/Components/Shared/src/DefaultAntiforgeryStateProvider.cs

Lines 14 to 18 in f510d86

	[UnconditionalSuppressMessage(
	"Trimming",
	"IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code",
	Justification = $"{nameof(DefaultAntiforgeryStateProvider)} uses the {nameof(PersistentComponentState)} APIs to deserialize the token, which are already annotated.")]
	public DefaultAntiforgeryStateProvider(PersistentComponentState state)

The justification for the trimmer warning suppression does not make sense to me. Yes, PersistentComponentState's PersistAsJson and TryTakeFromJson are "already annotated." That's why it correctly raises a warning when the DefaultAntiforgeryStateProvider constructor calls these methods.

This might be okay if the static JsonSerializerOptionsProvider.Options used by PersistentComponentState had a TypeInfoResolver backed by a JsonSerializerContext statically generated using [JsonSerializable(typeof(AntiforgeryRequestToken))]. Otherwise, don't we risk trimming the AntiforgeryRequestToken constructor which has no callers other indirectly via the JsonSerializer calls in PersistentComponentState?

Or better yet, we should probably update PersistentComponentState to use the JsonOptions in DI, and register its statically generated JsonSerializerContext with the TypeInfoResolverChain similar to what we do for MapIdentityApi and ProblemDetails.

@javiercn @eerhardt

[javiercn]

Or better yet, we should probably update PersistentComponentState to use the JsonOptions in DI, and register its statically generated JsonSerializerContext with the TypeInfoResolverChain similar to what we do for MapIdentityApi and ProblemDetails.

The context has problems because it doesn't allow serializing types that we don't know, and we don't expose serialization settings to customers in any place we serialize data, as we want to be consistent, and we don't want serialization settings to impact our own serialization requirements.

I believe we already have annotations in place to prevent trimming in this case.

[eerhardt]

We should update the Justification to explain this better. The reason this works is because of 2 things:

1. PersistAsJson/TryTakeFromJson methods annotate the TValue with [DynamicallyAccessedMembers((JsonSerialized)], which preserves the constructors and properties:

aspnetcore/src/Components/Components/src/PersistentComponentState.cs

Lines 61 to 88 in 00d0038

	public void PersistAsJson<[DynamicallyAccessedMembers(JsonSerialized)] TValue>(string key, TValue instance)
	{
	ArgumentNullException.ThrowIfNull(key);
	if (!PersistingState)
	{
	throw new InvalidOperationException("Persisting state is only allowed during an OnPersisting callback.");
	}
	if (_currentState.ContainsKey(key))
	{
	throw new ArgumentException($"There is already a persisted object under the same key '{key}'");
	}
	_currentState.Add(key, JsonSerializer.SerializeToUtf8Bytes(instance, JsonSerializerOptionsProvider.Options));
	}
	/// <summary>
	/// Tries to retrieve the persisted state as JSON with the given <paramref name="key"/> and deserializes it into an
	/// instance of type <typeparamref name="TValue"/>.
	/// When the key is present, the state is successfully returned via <paramref name="instance"/>
	/// and removed from the <see cref="PersistentComponentState"/>.
	/// </summary>
	/// <param name="key">The key used to persist the instance.</param>
	/// <param name="instance">The persisted instance.</param>
	/// <returns><c>true</c> if the state was found; <c>false</c> otherwise.</returns>
	[RequiresUnreferencedCode("JSON serialization and deserialization might require types that cannot be statically analyzed.")]
	public bool TryTakeFromJson<[DynamicallyAccessedMembers(JsonSerialized)] TValue>(string key, [MaybeNullWhen(false)] out TValue? instance)

2. We only call the APIs with the Type AntiforgeryRequestToken, which only has primitive properties at the top-level:

aspnetcore/src/Components/Web/src/Forms/AntiforgeryRequestToken.cs

Lines 16 to 33 in 00d0038

	public AntiforgeryRequestToken(string value, string formFieldName)
	{
	ArgumentNullException.ThrowIfNull(value);
	ArgumentNullException.ThrowIfNull(formFieldName);
	Value = value;
	FormFieldName = formFieldName;
	}
	/// <summary>
	/// Gets the antiforgery token value.
	/// </summary>
	public string Value { get; }
	/// <summary>
	/// Gets the form field name for the antiforgery token.
	/// </summary>
	public string FormFieldName { get; }

If this Type had a complex object as a property, this wouldn't work.

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.