Skip to content

Omit empty authorization_servers and scopes_supported from protected … - #1999

Open
shlee1223 wants to merge 1 commit into
modelcontextprotocol:mainfrom
shlee1223:prm-omit-empty-arrays
Open

shlee1223 wants to merge 1 commit into
modelcontextprotocol:mainfrom
shlee1223:prm-omit-empty-arrays

Conversation

@shlee1223

Copy link
Copy Markdown

Omit empty authorization_servers and scopes_supported from the protected resource metadata response.

Motivation and Context

Both lists default to [], so a server without scopes returns "scopes_supported": []. RFC 9728 §3.2: "Parameters with zero values MUST be omitted from the response."

How Has This Been Tested?

Added ProtectedResourceMetadataSerializationTests; the omission test fails without the change. The OAuth tests in ModelContextProtocol.AspNetCore.Tests pass. Not tested in a real application.

Breaking Changes

None. The public properties keep their type and default.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Serialized through internal [JsonInclude] properties (the pattern in ExperimentalInternalPropertyTests). bearer_methods_supported is unchanged, since RFC 9728 allows [] there.

@PederHP PederHP left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The RFC reading is right: RFC 9728 §3.2 requires omitting zero-value parameters, and §2 explicitly allows bearer_methods_supported: [], so keeping that one is correct. With the SDK's own options, which is how McpAuthenticationHandler serves the document, the change does what it says.

The points below are about the mechanism. Moving the wire names off the public properties onto internal [JsonInclude] shadow properties has some side effects outside the SDK's own serializer context (details inline):

  • A consumer's own JsonSerializerContext that includes ProtectedResourceMetadata can't see the internal members. The source generator reports SYSLIB1038 and drops authorization_servers and scopes_supported entirely. Today they serialize normally.
  • With options that don't set WhenWritingNull (e.g. JsonSerializerOptions.Web), the output becomes "scopes_supported": null rather than being omitted.
  • The public properties lose [JsonPropertyName] and gain [JsonIgnore], which reflection-based tooling (OpenAPI generators, resolver modifiers) will notice. So "Breaking Changes: None" may be a bit optimistic.
  • In this repo, the *Core pattern is documented in ExperimentalInternalPropertyTests as a temporary measure for experimental properties, removed on stabilization. Here it would be permanent on a stable type.

One alternative to consider: keep the public properties as the serialized members and skip empty collections with a contract modifier (JsonPropertyInfo.ShouldSerialize). It could apply either to the SDK's default options or only where the PRM response is written. Consumers using their own serializers would then keep today's behavior rather than lose fields. It would also cover the other optional array parameters in one place. Against that, the current approach has a smaller footprint, so the choice is between the two.

🤖 Generated with Claude Code
(including inline comments below)

/// OPTIONAL.
/// </remarks>
[JsonPropertyName("authorization_servers")]
[JsonIgnore]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since the public property is now [JsonIgnore], the only serialized member is internal. Anyone who adds [JsonSerializable(typeof(ProtectedResourceMetadata))] to their own context gets SYSLIB1038, which breaks builds with TreatWarningsAsErrors. They also get metadata with no authorization_servers or scopes_supported in either direction, and a served PRM missing authorization_servers can't be used for discovery. That trade-off needs weighing for a stable type, against keeping the public properties as the serialized members.

// RFC 9728 §3.2: parameters with zero values MUST be omitted from the response.
[JsonInclude]
[JsonPropertyName("authorization_servers")]
internal IList<string>? AuthorizationServersCore

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this approach stays, two hardening points to consider:

  • The omission depends on the caller's options using WhenWritingNull. With JsonSerializerOptions.Web (reflection), this now writes "authorization_servers": null, which is neither [] nor omitted. Adding [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] to the *Core properties would make the omission independent of the ambient options.
  • AuthorizationServers.Count throws if someone assigned null! to the public property, which today is just omitted. AuthorizationServers is { Count: > 0 } servers ? servers : null avoids that. The same applies to ScopesSupportedCore.

// RFC 9728 §3.2: parameters with zero values MUST be omitted from the response.
[JsonInclude]
[JsonPropertyName("scopes_supported")]
internal IList<string>? ScopesSupportedCore

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

§3.2 applies to every parameter, so resource_signing_alg_values_supported, authorization_details_types_supported and dpop_signing_alg_values_supported would also be emitted as [] if set to an empty list. They default to null, so this is much less likely than the two fixed here. Whether to cover them in this PR or a follow-up is open. With the current pattern each would need its own shadow property, which is one reason a single modifier may be easier to maintain.

}

[Fact]
public void MissingAuthorizationServersAndScopes_DeserializeAsEmpty()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One behavior change that seems worth pinning with a test: {"authorization_servers": null} used to deserialize to a null list, which would have thrown in ClientOAuthProvider at availableAuthorizationServers.Count. With the setter's ?? [] it now becomes empty, which is an improvement. A test with explicit nulls, plus one serializing with options that lack WhenWritingNull, would cover the paths the current tests don't reach.

This branch has not been deployed

No deployments
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