Open Tanya-Solyanik opened 1 month ago
FYI @pchaurasia14 , @singhashish-wpf , @Kuldeep-MS
@miloush Look, new APIs!
Thanks @h3xds1nz, would have missed this.
The API changes pertaining to WPF should be posted separately in the WPF repo.
In general I don't have anything against adding new methods, although I don't understand why they wouldn't have the index
parameter. If people are happy to do breaking changes to the IDataObject
interface, it should include methods taking index
for both old and new methods.
Is it suggested that the shared implementation would support WPF primitive types as much as the System.Drawing ones? Because the link to intrinsically handled types does not list any WPF types.
I am not thrilled about obsoleting methods that don't have any equivalent (people can put whole controls in the DataObject now and it works even when not serializable, and you can test for reference equality), but hiding them completely is nefarious.
Windows.ClipboardDragDrop.EnableUnsafeBinaryFormatterSerialization
namespace System.Windows.Forms;
and
namespace System.Windows;
public static partial class Clipboard
{
+ public static void SetDataAsJson<T>(string format, T data) { }
+ [Obsolete("`Clipboard.GetData(string)` method is obsolete. Use `Clipboard.TryGetData<T>` instead.", false, DiagnosticId = "WFDEV005", UrlFormat = "https://aka.ms/winforms-warnings/{0}")
public static object? GetData(string format) { }
+ public static bool TryGetData<T>(string format, out T data) { }
+ public static bool TryGetData<T>(string format, Func<Reflection.Metadata.TypeName, Type> resolver, out T data) { }
}
public partial class DataObject : IDataObject, Runtime.InteropServices.ComTypes.IDataObject
+ ITypedDataObject
{
+ public void SetDataAsJson<T>(T data) { }
+ public void SetDataAsJson<T>(string format, T data) { }
+ public void SetDataAsJson<T>(string format, bool autoConvert, T data) { }
+ [Obsolete("`DataObject.GetData` methods are obsolete. Use the corresponding `DataObject.TryGetData<T>` instead.", false, DiagnosticId = "WFDEV005", UrlFormat = "https://aka.ms/winforms-warnings/{0}")]
public virtual object? GetData(string format, bool autoConvert) { }
+ [Obsolete("`DataObject.GetData` methods are obsolete. Use the corresponding `DataObject.TryGetData<T>` instead.", false, DiagnosticId = "WFDEV005", UrlFormat = "https://aka.ms/winforms-warnings/{0}")]
public virtual object? GetData(string format) { }
+ [Obsolete("`DataObject.GetData` methods are obsolete. Use the corresponding `DataObject.TryGetData<T>` instead.", false, DiagnosticId = "WFDEV005", UrlFormat = "https://aka.ms/winforms-warnings/{0}")]
public virtual object? GetData(Type format) { }
+ public bool TryGetData<T>(out T data) { }
+ public bool TryGetData<T>(string format, out T data) { }
+ public bool TryGetData<T>(string format, bool autoConvert, out T data) { }
+ public bool TryGetData<T>(string format, Func<Reflection.Metadata.TypeName, Type> resolver, bool autoConvert, out T data) { }
+ protected virtual bool TryGetDataCore<T>(string format, Func<Reflection.Metadata.TypeName, Type> resolver, bool autoConvert, out T data) { }
}
+public interface ITypedDataObject
+{
+ bool TryGetData<T>(out T data);
+ bool TryGetData<T>(string format, out T data);
+ bool TryGetData<T>(string format, bool autoConvert, out T data);
+ bool TryGetData<T>(string format, Func<Reflection.Metadata.TypeName, Type> resolver, bool autoConvert, out T data);
+}
+public sealed class DataObjectExtensions
{
+ public static bool TryGetData<T>(this IDataObject dataObject, out T data);
+ public static bool TryGetData<T>(this IDataObject dataObject, string format, out T data);
+ public static bool TryGetData<T>(this IDataObject dataObject, string format, bool autoConvert, out T data);
+ public static bool TryGetData<T>(this IDataObject dataObject, string format, Func<Reflection.Metadata.TypeName, Type> resolver, bool autoConvert, out T data);
}
namespace System.Windows.Forms;
public class partial class Control
{
+ public DragDropEffects DoDragDropAsJson<T>(T data, DragDropEffects allowedEffects, Bitmap? dragImage, Point cursorOffset, bool useDefaultDragImage)
+ public DragDropEffects DoDragDropAsJson<T>(T data, DragDropEffects allowedEffects)
}
namespace System.Windows;
public static class DragDrop
{
+ public static DragDropEffects DoDragDropAsJson<T>(DependencyObject dragSource, T data, DragDropEffects allowedEffects)
}
// VisualBasic wrapper for WinForms Clipboard.
namespace Microsoft.VisualBasic.MyServices
public partial class ClipboardProxy
{
+ public void SetDataAsJson<T>(T data) { }
+ public void SetDataAsJson<T>(string format, T data) { }
+ [Obsolete("`ClipboardProxy.GetData(As String)` method is obsolete. Use `ClipboardProxy.TryGetData(Of T)` instead.", false, DiagnosticId = "WFDEV005", UrlFormat = "https://aka.ms/winforms-warnings/{0}")
public object GetData(string format) { }
+ public bool TryGetData<T>(string format, out T data) { }
+ public bool TryGetData<T>(string format, System.Func<System.Reflection.Metadata.TypeName, System.Type> resolver, out T data) { }
}
For the index parameter, we have an API proposal in WPF affecting data objects: https://github.com/dotnet/wpf/issues/7744
It might be useful to do this as a part of unifying the codebase.
public partial class DataObject : IDataObject, Runtime.InteropServices.ComTypes.IDataObject ITypedDataObject { public void SetDataAsJson
(T data) { }
Do we need overloads of these "Json" APIs that take a JsonTypeInfo
, so the Json source generator can be used (to support trimming and native AOT)?
Do we need overloads of these "Json" APIs that take a JsonTypeInfo, so the Json source generator can be used (to support trimming and native AOT)?
That's the first bullet in the meeting summary. The answer was "no, not at this time"
Not to be too daring, but if it used XML, we wouldn't need the extra System.Text.Json dependency, especially if this is planned to be used in .NET Framework for interopability.
In general, the fact that it uses JSON seems to be a rather implementation detail of safety, especially if we don't allow reading and writing the JSON directly. Do I understand the idea correctly that the SetDataAsJson would still store the value as binary if it is one of the types on a safelist? If so, calling it AsJson seems a bit misleading.
My 2 Cents here. Just out of curiosity, why are the methods called SomethingAsJson
, when they do not have any JSON-specific info in their signature? Isn't JSON here something like an implementation detail, like binary formatting is for the existing implementation?
SetDataAsJson
feels more like a convenience method that should be provided as an extension method instead of being part of the public interface of the Clipboard
class. But I guess that's not an option, since the class is static?
@MichaeIDietrich one option to do that would be to have a separate JsonClipboard
or SafeClipboard
or similar that could be an in-place replacement and only deal with the safe types. People have to opt in using the new API anyway, and you get freedom to design the API the way you want.
Background and motivation
OLE clipboard supports standard exchange formats and additionally allows users to register custom formats for data exchange. Although standard exchange formats do not use binary format as they are defined by Windows, custom format serialization of objects in .NET has used the BinaryFormatter (in both WinForms and WPF). WinForms Clipboard falls back to the BinaryFormatter when consuming custom types stored as custom clipboard formats. We propose a set of Clipboard and DataObject APIs that make it easier to follow best practices when deserializing untrusted data. They restrict unbounded BinaryFormatter deserialization to known types and provide an alternative serialization method, JSON, for user types.
API Proposal
We propose the same API changes in WinForms and WPF assemblies. The new API surface will be backed by a shared implementation. The proposed APIs will support our updated Clipboard and Drag/Drop guidance which is:
BinaryFormatter
with the proposed APIs, instead they are JSON-serialized. This is the recommended way for new applications to use the Clipboard or Drag/Drop.byte[]
orstring
. Adjustment will need to be made on the consuming side to handle receiving a JSON formatted type.Clipboard
Managed implementation of OLE's IDataObject definition and is being used in all OLE operations for WinForms/WPF.
DataObject
IDataObject
Control
DragDrop
ClipboardProxy
New configuration switch:
ClipboardDragDrop.EnableUnsafeBinaryFormatterSerialization
- controls whether BinaryFormatter is enabled as a fallback for Clipboard and Drag/drop scenarios. By default, it is false.API Usage
Clipboard API Examples
Before introduction of new APIs
public void SetGetClipboardData() { WeatherForecastPOCO weatherForecast = new() { Date = DateTime.Parse("2019-08-01"), TemperatureCelsius = 25, Summary = "Hot", };
pragma warning disable WFDEV005 // Type or member is obsolete
pragma warning restore WFDEV005
}
After introduction of new APIs
BinaryFormatter
.BinaryFormatter
deserialization by providing a set of allowed types.// Consumer side if (Clipboard.TryGetData("font", FontResolver, out Font data)) { // Do things with data }
private static Type FontResolver(TypeName typeName) { (string name, Type type)[] allowedTypes = [ (typeof(FontStyle).FullName!, typeof(FontStyle)), (typeof(FontFamily).FullName!, typeof(FontFamily)), (typeof(GraphicsUnit).FullName!, typeof(GraphicsUnit)), ];
}
Alternative Designs
Func<TypeName, Type>
with a resolver interface that can be reused in ResX and ActiveX scenarios and potentially in other Runtime scenarios, move the IResolver interface intoSystem.Runtime.Serialization
namespace. That would be a replacement for the resolverFunc<TypeName, Type>
.Should we make the
GetData
methods in the managedIDataObject
interface obsolete? These methods are implemented by the user, we don’t know if they are vulnerable or not. But they propagate a bad pattern by returning an unconstrained type (System.Object). There might be too many false positives if we obsolete them. The recommended way is for users to derive from the managed DataObject, not to implement the interface from scratch, and that scenario is covered.Should we make the
SetData
overloads obsolete? No, this scenario is not vulnerable, it propagates a bad pattern only when serializing more complex types. We will address this with an analyzer.Why take
T
forSetDataAsJson
/DoDragDropAsJson
APIs instead ofobject
? We need to save the original type of the data that is being passed in so that we can rehydrate the type when the user asks for it, meaning that we will need to rely on Object.GetType to get the type of the passed in data in SetDataAsJson / DoDragDropAsJson APIs. This is not trim friendly because it might use reflection.Should an optional parameter to
SetData
andDoDragDrop
APIs to indicate serializing with Json is desired instead of introducing a new API signature? This is an option, but it is again not trim friendly as we would need to rely on Object.GetType.Should we make the managed
IDataObject
interface obsolete? The shape of the managed interface matches that of the OLE interface, and the whole ecosystem depends on it, so any replacement would be very similar. We assume that most use cases override our DataObject instead of implementing the IDataObject from scratch.Should the consumption side APIs be
T GetData<T>(..)
orbool TryGetData<T>(… out T data)
. No strong preference, an API that returns a Boolean seems to be more convenient for the common use patterns observed in GitHUB.Naming for the configuration switch should indicate that it’s applicable to WPF as well when we share code with WPF. After the code is merged in the WPF repo,
System.Windows.Forms.Clipboard
andSystem.Windows.Clipboard
will share the implementation. We want the configuration switch name to indicate that it's applicable to both. We could use the common namespace name portion:System.Windows.ClipboardDragDrop.EnableUnsafeBinaryFormatterSerialization
OrSystem.ClipboardDragDrop.EnableUnsafeBinaryFormatterSerialization
Or no namespace nameClipboardDragDrop.EnableUnsafeBinaryFormatterSerialization
Risks
• Users would have to implement the assembly loader and type resolver that works with TryGetData() to support types other than "T". If that resolver calls Type.GetType() they might lose control over assembly loading. Proposed APIs that do not accept the type resolver parameter, don't have this issue. When doing type matching, we rely on
NrbfDecoder
and type name matching API to be safe (threat model). Sample user-provided resolver code:• When deserializing objects that have been JSON serialized, this carries the same risks as any JSON data going through System.Text.Json, so it is possible to misuse
SetDataAsJson
to do bad things during clipboard and drag/drop operation (System.Text.Json threat model). As with any data, users need to trust the JSON data they are trying to grab.Risk mitigation
We are adding a new configuration switch that would block the fallback into
BinaryFormatter
use in Clipboard and DragDrop scenarios, the proposed APIs allow users to use JSON format instead. We will encourage users to use only primitive exchange types or POCOc.Will this feature affect UI controls?
No