Bring Microsoft.AspNetCore.DataProtection.SystemWeb compatibility mode into the FrameworkServices project #672
Open
twsouthwick wants to merge 18 commits intomainfrom
Open
Bring Microsoft.AspNetCore.DataProtection.SystemWeb compatibility mode into the FrameworkServices project #672twsouthwick wants to merge 18 commits intomainfrom
twsouthwick wants to merge 18 commits intomainfrom
Conversation
…e into the FrameworkServices project
There was a problem hiding this comment.
Pull request overview
This PR moves the ASP.NET Framework machineKey ↔ ASP.NET Core DataProtection compatibility implementation into the Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices project so consumers no longer need the legacy Microsoft.AspNetCore.DataProtection.SystemWeb package or explicit web.config machineKey wiring.
Changes:
- Adds an in-box
CompatibilityDataProtectorimplementation and runtime machineKey override wiring. - Introduces a
MachineKeySectionreflection shim and anIApplicationDiscriminatorimplementation for SystemWeb hosting. - Removes the old
Microsoft.AspNetCore.DataProtection.SystemWebpackage references and sampleweb.configmachineKey configuration.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs | Switches to runtime machineKey override + registers a SystemWeb-based application discriminator. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices.csproj | Removes dependency on the old SystemWeb DataProtection package. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/MachineKeyExtensions.cs | Adds reflection-based accessors for internal MachineKeySection config state. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/CompatibilityDataProtector.cs | Adds the compatibility DataProtector implementation used by System.Web MachineKey. |
| samples/MachineKey/MachineKeyFramework/Web.config | Removes explicit <machineKey ... dataProtectorType=... /> configuration from the sample. |
| samples/MachineKey/MachineKeyFramework/MachineKeyFramework.csproj | Removes the old SystemWeb DataProtection package reference from the sample. |
| samples/MachineKey/MachineKeyFramework/Global.asax.cs | Updates sample startup to call AddSystemWebDependencyInjection() before DataProtection setup. |
| Directory.Packages.props | Removes centrally-pinned version for Microsoft.AspNetCore.DataProtection.SystemWeb. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (7)
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs:55
- TrySetMachineKeyOverride currently throws whenever is present, even if it is already configured to the CompatibilityDataProtector. Since the previous integration required setting this in web.config, this is a breaking change for existing users. Consider allowing the expected/compatible value(s) (including the older assembly-qualified name) and treating it as already configured instead of throwing.
if (ConfigurationManager.GetSection("system.web/machineKey") is MachineKeySection section)
{
if (!string.IsNullOrEmpty(section.DataProtectorType))
{
throw new InvalidOperationException("Could not setup DataProtection for use with MachineKey due to existing configuration of system.web/machineKey");
}
if (section.CompatibilityMode != MachineKeyCompatibilityMode.Framework45)
{
throw new InvalidOperationException($"Could not setup DataProtection for use with MachineKey due to invalid CompatibilityMode ({section.CompatibilityMode})");
}
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs:66
- When constructing the new MachineKeySection, only ApplicationName, Decryption, and DecryptionKey are copied from the existing config. This risks losing explicitly-configured validation settings (e.g., Validation/ValidationKey) and changing MachineKey behavior. Copy the full set of relevant machineKey settings from the existing section (at least validation-related properties) before setting DataProtectorType/CompatibilityMode.
var existing = MachineKeySection.GetApplicationConfig();
var updated = new MachineKeySection()
{
ApplicationName = existing.ApplicationName,
CompatibilityMode = MachineKeyCompatibilityMode.Framework45,
DataProtectorType = typeof(CompatibilityDataProtector).AssemblyQualifiedName,
Decryption = existing.Decryption,
DecryptionKey = existing.DecryptionKey,
};
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs:44
- _isSet is a non-thread-safe guard and is set to true before configuration validation/mutation completes. If an exception is thrown during setup (or two threads race), later calls will skip setup even though it may not have been applied successfully. Consider using a lock/Interlocked pattern and only marking completion after the override is successfully applied.
private static bool _isSet;
private static void TrySetMachineKeyOverride()
{
if (_isSet)
{
return;
}
_isSet = true;
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/MachineKeyExtensions.cs:16
- These reflection lookups can return null if the underlying System.Web implementation changes (or in different framework versions), leading to NullReferenceException later. Add null checks after GetField/GetMethod and throw a clear InvalidOperationException/PlatformNotSupportedException explaining that machineKey override isn't supported for this runtime if the members can't be found.
private static FieldInfo _configField = typeof(MachineKeySection).GetField("s_config", BindingFlags.NonPublic | BindingFlags.Static);
private static MethodInfo _ensureConfig = typeof(MachineKeySection).GetMethod("EnsureConfig", BindingFlags.NonPublic | BindingFlags.Static);
private static MethodInfo _getApplicationConfig = typeof(MachineKeySection).GetMethod("GetApplicationConfig", BindingFlags.NonPublic | BindingFlags.Static);
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/MachineKeyExtensions.cs:28
- This uses the new
extension(Type ...) { ... }syntax to add static members to MachineKeySection. If this syntax requires a preview compiler, it may not compile with the repo'sLangVersion=latestsetting and could also make the code harder to maintain. Consider rewriting this as a normal static helper/extension-methods class so it compiles on non-preview compilers.
extension(MachineKeySection section)
{
internal static MachineKeySection Value
{
get => (MachineKeySection)_configField.GetValue(null);
set => _configField.SetValue(null, value);
}
internal static void EnsureConfig() => _ensureConfig.Invoke(null, []);
internal static MachineKeySection GetApplicationConfig() => (MachineKeySection)_getApplicationConfig.Invoke(null, []);
}
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/CompatibilityDataProtector.cs:15
- There is an extra space in the declaration
public class CompatibilityDataProtector, which looks unintentional and inconsistent with surrounding style.
[EditorBrowsable(EditorBrowsableState.Never)]
public class CompatibilityDataProtector : DataProtector
{
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs:30
- The new MachineKey override + application discriminator behavior is security-sensitive and has several branches (existing machineKey config, invalid compatibilityMode, etc.), but there don’t appear to be tests covering SystemWebDataProtectionExtensions.AddDataProtection / TrySetMachineKeyOverride. Please add unit/integration tests (likely under Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices.Tests) to validate the intended behaviors and prevent regressions.
public static IDataProtectionBuilder AddDataProtection(this HttpApplicationHostBuilder builder)
{
if (builder is null)
{
throw new ArgumentNullException(nameof(builder));
}
TrySetMachineKeyOverride();
builder.Services.TryAddSingleton<IApplicationDiscriminator, SystemWebApplicationDiscriminator>();
return builder.Services.AddDataProtection();
- Files reviewed: 8/8 changed files
- Comments generated: 3
There was a problem hiding this comment.
Pull request overview
This PR moves the ASP.NET Framework machineKey ↔ ASP.NET Core Data Protection compatibility implementation into the FrameworkServices project so framework-hosted apps can use HttpApplicationHostBuilder.AddDataProtection() without requiring a web.config machineKey override or an external Microsoft.AspNetCore.DataProtection.SystemWeb package.
Changes:
- Adds an in-repo
CompatibilityDataProtectorand runtimemachineKeyoverride logic (reflection-based) to enable System.Web compatibility mode. - Registers a custom
IApplicationDiscriminatorto align Data Protection application naming withmachineKey applicationNamewhen explicitly set. - Removes the
Microsoft.AspNetCore.DataProtection.SystemWeb2.3.0 package usage and updates the MachineKey sample to no longer requireweb.configconfiguration.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs | Configures machineKey override at runtime and wires Data Protection discriminator integration. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices.csproj | Drops the legacy DataProtection.SystemWeb package reference. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/MachineKeyExtensions.cs | Adds reflection helpers to mutate MachineKeySection internals. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/CompatibilityDataProtector.cs | Introduces the System.Web-compatible DataProtector implementation backed by ASP.NET Core Data Protection. |
| samples/MachineKey/MachineKeyFramework/Web.config | Removes the explicit machineKey dataProtectorType=... configuration from the sample. |
| samples/MachineKey/MachineKeyFramework/MachineKeyFramework.csproj | Removes the legacy DataProtection.SystemWeb package reference. |
| samples/MachineKey/MachineKeyFramework/Global.asax.cs | Ensures System.Web DI is configured before Data Protection usage. |
| Directory.Packages.props | Removes the centralized package version entry for Microsoft.AspNetCore.DataProtection.SystemWeb. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (4)
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/MachineKeyExtensions.cs:17
extension(MachineKeySection section)uses the new extension-block syntax. The repo is configured with<LangVersion>latest</LangVersion>(notpreview) and doesn’t enable preview features, so this syntax is likely to fail compilation on supported toolsets. Consider rewriting this as regular extension methods / a normal static helper (e.g.,MachineKeySectionHelpers.GetApplicationConfig()/SetConfigValue(...)) rather than relying on extension blocks, or explicitly enabling the required language version for the repo.
extension(MachineKeySection section)
{
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs:49
- This throws whenever
<machineKey dataProtectorType="..." />is configured in web.config (line 46). Several existing samples in the repo still havedataProtectorType="Microsoft.AspNetCore.DataProtection.SystemWeb.CompatibilityDataProtector, Microsoft.AspNetCore.DataProtection.SystemWeb", so they will now fail at runtime when callingAddDataProtection. Either update the remaining sample web.config files to remove/adjust that setting, or allow the configuredDataProtectorTypewhen it matches the new compatibility protector type.
if (ConfigurationManager.GetSection("system.web/machineKey") is MachineKeySection section)
{
if (!string.IsNullOrEmpty(section.DataProtectorType))
{
throw new InvalidOperationException("Could not setup DataProtection for use with MachineKey due to existing configuration of system.web/machineKey");
}
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs:65
- When constructing the new
MachineKeySection, only a subset of the existing configuration is copied over (ApplicationName, Decryption, DecryptionKey). Any other machineKey settings present in config will be reset to defaults whenMachineKeySection.Valueis replaced. Consider copying all relevant properties fromexisting(or mutating only the specific fields you need) to avoid unintended behavior changes for apps that set additional machineKey options.
var existing = MachineKeySection.GetApplicationConfig();
var updated = new MachineKeySection()
{
ApplicationName = existing.ApplicationName,
CompatibilityMode = MachineKeyCompatibilityMode.Framework45,
DataProtectorType = typeof(CompatibilityDataProtector).AssemblyQualifiedName,
Decryption = existing.Decryption,
DecryptionKey = existing.DecryptionKey,
};
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs:100
WebConfigurationManager.GetWebApplicationSection("system.web/machineKey")can return null (or a non-MachineKeySection), but the code casts directly and then dereferencesElementInformation, which would throw. Consider usingas MachineKeySectionand falling back toenv.ApplicationNamewhen the section can’t be read.
var machineKeySection = (MachineKeySection)WebConfigurationManager.GetWebApplicationSection("system.web/machineKey");
if (machineKeySection.ElementInformation.Properties["applicationName"].ValueOrigin != PropertyValueOrigin.Default)
{
return machineKeySection.ApplicationName;
- Files reviewed: 8/8 changed files
- Comments generated: 4
…achineKeyExtensions.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ompatibilityDataProtector.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/systemweb-adapters/sessions/6aeed256-6eb7-4a3d-95de-27d7bf77c723 Co-authored-by: twsouthwick <583206+twsouthwick@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/systemweb-adapters/sessions/6aeed256-6eb7-4a3d-95de-27d7bf77c723 Co-authored-by: twsouthwick <583206+twsouthwick@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/systemweb-adapters/sessions/6aeed256-6eb7-4a3d-95de-27d7bf77c723 Co-authored-by: twsouthwick <583206+twsouthwick@users.noreply.github.com>
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
Bring Microsoft.AspNetCore.DataProtection.SystemWeb compatibility mode into the FrameworkServices project
Harden machineKey override one-time initialization path
Apr 7, 2026
twsouthwick
commented
Apr 8, 2026
CZEMacLeod
reviewed
Apr 8, 2026
Contributor
|
Just as a comment - Despite using reflection to do it, I love that this does not require modifying |
There was a problem hiding this comment.
Pull request overview
This PR moves the ASP.NET MachineKey ↔ IDataProtectionProvider compatibility wiring into the FrameworkServices project so MachineKey integration can be enabled programmatically (instead of requiring web.config + the Microsoft.AspNetCore.DataProtection.SystemWeb package).
Changes:
- Add runtime initialization via an
IHostedServicethat reflects intoMachineKeySectionto enableFramework45compatibility mode and setDataProtectorType. - Introduce an in-repo
CompatibilityDataProtectorimplementation (same namespace as the legacy package). - Remove
Microsoft.AspNetCore.DataProtection.SystemWebpackage usage and update the MachineKey sample to no longer require theweb.configmachineKey override.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs | Adds hosted startup initialization + custom application discriminator for System.Web MachineKey integration. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices.csproj | Drops the Microsoft.AspNetCore.DataProtection.SystemWeb package reference. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/CompatibilityDataProtector.cs | Adds the compatibility DataProtector implementation in-repo. |
| samples/MachineKey/MachineKeyFramework/Web.config | Removes the machineKey override that referenced the legacy package assembly. |
| samples/MachineKey/MachineKeyFramework/MachineKeyFramework.csproj | Removes the legacy package reference from the sample. |
| samples/MachineKey/MachineKeyFramework/Global.asax.cs | Ensures System.Web DI is configured in the sample before DataProtection setup. |
| Directory.Packages.props | Removes central package version for Microsoft.AspNetCore.DataProtection.SystemWeb. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs:103
- In
Initialize, a newMachineKeySectioninstance is created and assigned to the internals_configfield, but only a subset of the existing settings are copied (ApplicationName,Decryption,DecryptionKey). This will drop any other MachineKey settings (e.g.,validation/validationKeyand any other configured attributes), potentially breaking auth cookies / viewstate validation for apps that have explicit machineKey configuration.
Instead of replacing the whole section with a partially-populated instance, update the existing section in-place (set CompatibilityMode and DataProtectorType) or ensure all relevant properties are copied over from existing.
var existing = GetApplicationConfig();
var updated = new MachineKeySection()
{
ApplicationName = existing.ApplicationName,
CompatibilityMode = MachineKeyCompatibilityMode.Framework45,
DataProtectorType = typeof(CompatibilityDataProtector).AssemblyQualifiedName,
Decryption = existing.Decryption,
DecryptionKey = existing.DecryptionKey,
};
Value = updated;
src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebDataProtectionExtensions.cs:46
AddDataProtectionrelies onCompatibilityDataProtectorbeing able to resolve anIDataProtectionProviderviaHttpRuntime.WebObjectActivator, but this method doesn't ensure that System.Web DI has been configured (e.g., viaAddSystemWebDependencyInjection/AddWebObjectActivator) beforeMachineKeySetupruns. If a caller invokesAddDataProtectionwithout first configuring DI, startup will fail whenMachineKeySetupcallsMachineKey.Protect.
Consider either (1) calling builder.AddWebObjectActivator() inside AddDataProtection, or (2) performing an explicit validation and throwing an actionable exception explaining the required setup/order.
// We use this to auto-start it ASAP to ensure the dataprotector is set up before anyone else tries to do anything
builder.Services.AddHostedService<MachineKeySetup>();
builder.Services.TryAddSingleton<IApplicationDiscriminator, SystemWebApplicationDiscriminator>();
return builder.Services.AddDataProtection();
- Files reviewed: 7/7 changed files
- Comments generated: 2
…ystemWebDataProtectionExtensions.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Thanks for the quick turnaround from the deprecation announcement. :) As current users of the data protection functionality, it looks like this'll work for us. (We're calling |
| } | ||
|
|
||
| // We take care of flowing purposes ourselves. | ||
| protected override bool PrependHashedPurposeToPlaintext { get; } |
There was a problem hiding this comment.
I don't see anything setting this property. Could then be { get => false; } so as not to generate an instance field.
Member
Author
There was a problem hiding this comment.
This is something that existed in the previous version (the 2.3 shim library) so that people could (if they wanted) override the CompatibilityDataProtector and set it themselves manually. Not sure how much we need that behavior, or if we want to rethink a design that fits in with the more IHostApplicationBuilder pattern
Member
Author
There was a problem hiding this comment.
oh I guess your point is still valid if we keep it - I misunderstood initially. Sure I'll change it so it doesn't create a field. Deciding if we want to keep the same pattern is also worth having
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.