Add pluggable serialization strategy and make BinaryFormatter opt-in#3745
Add pluggable serialization strategy and make BinaryFormatter opt-in#3745ValResnick wants to merge 6 commits into
Conversation
Introduce ISerializationStrategy and SerializationConfiguration. Default to a throwing strategy unless explicitly configured, and add NHibernate property serialization.strategy_class for strategy setup. Keep test bootstrap on BinaryFormatter for backward-compatible test behavior.
0eeff26 to
dd3cb68
Compare
|
I like the direction of introducing a pluggable serialization strategy. But please make the default behavior fail fast, so nhibernate-code has no direct BinaryFormatter dependency. The next step can be stricter: BinaryFormatter should be removed completely from the NHibernate core assembly. I suggest this split:
If projects still need legacy BinaryFormatter behavior, they should implement and register their own strategy in their .NET project and explicitly opt in there. That keeps NHibernate core clean and avoids endorsing BinaryFormatter in runtime code. |
|
I like this proposal a lot. It keeps the change simple, and since BinaryFormatter is effectively dead anyway, making it opt-in and moving it out of the core path feels like the right direction. I’ll update this PR accordingly. |
|
The direction looks right. Removing BinaryFormatter from the core assembly and moving it into the test project is a clean step, given that BinaryFormatter is deprecated and a known security risk. |
…Environment class.
| return settings; | ||
| } | ||
|
|
||
| private static void ConfigureSerializationStrategy(IDictionary<string, string> properties) |
There was a problem hiding this comment.
@ValResnick thx for your changes! Only a small review finding:
Is a process-global serialization strategy intended, or should strategy be tied to SessionFactory settings/context? I would prefer a process-global configuration, see NHibernate.Cfg.Environment.InitializeGlobalProperties
| /// </summary> | ||
| public static class SerializationConfiguration | ||
| { | ||
| private static ISerializationStrategy _strategy = new ThrowingSerializationStrategy(); |
There was a problem hiding this comment.
This is a breaking change for .net8 and lower. (Nh is build targeting: .Net Fx 4.6.1, 4.8; .net core 2.0; .net std 2.0, 2.1; .net 6.0, 8.0, 10.0)
For merging this in the next minor, it should be:
| private static ISerializationStrategy _strategy = new ThrowingSerializationStrategy(); | |
| private static ISerializationStrategy _strategy = | |
| #if NET9_0_OR_GREATER | |
| new ThrowingSerializationStrategy(); | |
| #else | |
| new BinaryFormatterSerializationStrategy(); | |
| #endif |
The BinaryFormatterSerializationStrategy class would have to be added in the lib, but encapsulated wholly in a #if !NET9_0_OR_GREATER.
That way, it could be in the next minor, while still being removed out of .net10 builds.
There was a problem hiding this comment.
It is actually still possible to use binary serialization with .net 10, as do the NH tests, through the System.Runtime.Serialization.Formatters NuGet package.
So, on principle, we should default to BinaryFormatterSerializationStrategy even for net10.
But to better align with the change in net10, we could maybe still do something similar to .net10: removing it when targeting .net10, document this a a "possible breaking change" in the release, explaining how to restore the feature.
There was a problem hiding this comment.
I believe NHibernate should drop BinaryFormatter support by default to prevent vulnerabilities. While removing it is a breaking change for projects relying on binary serialization for complex types, using ORM mappings this way is generally considered an anti-pattern.
If a project absolutely requires this functionality, it can easily be re-enabled by configuring an external dependency to the BinaryFormatter package. This approach aligns also in .NET 8 and .NET 9.
Introduce ISerializationStrategy and SerializationConfiguration.
Default to a throwing strategy unless explicitly configured, and add NHibernate property serialization.strategy_class for strategy setup.
Keep test bootstrap on BinaryFormatter for backward-compatible test behavior.
Refs #2603
Questions for reviewers
Suggestions for the best target location are very welcome.