Removing or annotating Serializable attribute on winforms classes #1425

Tanya-Solyanik · 2019-07-19T03:43:50Z

Fixes #889
Fixes #890
Fixes #1251

Proposed changes

Removed SerializableAttribute from types that don't have known binary serialization scenarios and annotated types that are staying serializable.
Lists of types are in #889

Customer Impact

This change reduces security risk associated with binary serialization

Regression?

no

Risk

high - might have to add attributes again if legitimate serialization scenarios exist

Test methodology

Manual verification and additional testing of resx serialization

Microsoft Reviewers: Open in CodeFlow

RussKie

Looks good 👍
Few minor comments

src/System.Windows.Forms.Design/src/System/ComponentModel/Design/ExceptionCollection.cs

...esign/src/System/ComponentModel/Design/Serialization/CodeDomComponentSerializationService.cs

src/System.Windows.Forms/src/System/Windows/Forms/ListViewItem.cs

src/System.Windows.Forms/src/System/Resources/ResXFileRef.cs

src/System.Windows.Forms/src/System/Windows/Forms/Padding.cs

src/System.Windows.Forms.Design/src/System/ComponentModel/Design/ExceptionCollection.cs

zsd4yr

Pending more tests, LGTM

codecov · 2019-07-23T20:08:50Z

Codecov Report

Merging #1425 into master will increase coverage by 0.11197%.
The diff coverage is 70.65217%.

@@                 Coverage Diff                 @@
##              master       #1425         +/-   ##
===================================================
+ Coverage   25.64901%   25.76099%   +0.11198%     
===================================================
  Files            786         786                 
  Lines         268256      268169         -87     
  Branches       37978       37968         -10     
===================================================
+ Hits           68805       69083        +278     
+ Misses        194557      194164        -393     
- Partials        4894        4922         +28

Flag	Coverage Δ
#Debug	`25.76099% <70.65217%> (+0.11197%)`	⬆️
#production	`25.76099% <70.65217%> (+0.11197%)`	⬆️
#test	`100% <ø> (ø)`	⬆️

src/System.Windows.Forms/tests/UnitTests/SerializableTypesTests.cs

zsd4yr · 2019-07-23T20:18:26Z

LGTM but you should investigate this test failure. Looks like something in the debug skew for System.ComponentModel.Design.DesignerHost.AddToContainerPostProcess

ericstj · 2019-07-23T20:20:14Z

src/Common/tests/InternalUtilitiesForTests/src/BinarySerialization.cs

+{
+    public static class BinarySerialization
+    {
+        public static void EnsureSerializableAttribute(Assembly assemblyUnderTest, Dictionary<string, bool> serializableTypes)


Rather than test by checking for attribute, just ensure you have a test-case that actually serializes every type. Could be useful for a negative case.

This isn't too bad after further consideration, just make sure you have actual test cases.

I simplified these tests to focus on blocking addition of serializable types.

ericstj · 2019-07-23T20:21:27Z

src/Common/tests/InternalUtilitiesForTests/src/BinarySerialization.cs

+{
+    public static class BinarySerialization
+    {
+        public static void EnsureSerializableAttribute(Assembly assemblyUnderTest, Dictionary<string, bool> serializableTypes)


This isn't too bad after further consideration, just make sure you have actual test cases.

ericstj · 2019-07-23T20:27:41Z

src/System.Windows.Forms.Design/src/System/ComponentModel/Design/ExceptionCollection.cs

    public sealed class ExceptionCollection : Exception
    {
        private readonly ArrayList _exceptions;
+        private const string SerializationKey = "exceptions";


Rather than introducing a constant here, consider reverting the field name to exceptions then using nameof(exceptions) in the constructor / GetObjectData. Also consider adding // Do not rename (binary serialization). It'll be helpful if you express the serialization constraint consistently across the repo so that its easier for code-reviewers to catch the cases where folks may introduce new fields, or rename, and break serialization.

The change is not as uniform as I had hoped because usually we have custom serialization code and do not serialize private fields, but rather pre-process data that we want to serialize or serialize public properties, Or serialize public property under a different name - for example Tag property is serialized as UserData.
I applied your suggestion only in cases where we are serializing private properties.

I see. Well one benefit of always having custom implementations of these methods is that its much harder to break compatibility accidentally.

yes, the compatibility goal is achieved, but the source is not "intuitive" - it's impossible to deduce type members from the serialized data. But this is not the goal of this work item.

RussKie

Few little nits.
One question whether it is worth adding tests to check there are serialisable types other than those that we expect.

src/System.Windows.Forms.Design.Editors/tests/UnitTests/SerializableAttributeTest.cs

src/System.Windows.Forms.Design/src/System/ComponentModel/Design/ExceptionCollection.cs

src/System.Windows.Forms.Design/tests/UnitTests/SerializableAttributeTests.cs

src/System.Windows.Forms/src/System/Windows/Forms/Cursor.cs

src/System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/TreeViewTest.Designer.cs

src/System.Windows.Forms/tests/UnitTests/SerializableTypesTests.cs

RussKie · 2019-07-24T08:07:12Z

Btw happy to have it merged as-is to take it into P8, we can do further tweaks as a follow up.

src/System.Windows.Forms/tests/UnitTests/SerializableTypesTests.cs

...ystem.Windows.Forms.Design/tests/UnitTests/System/ComponentModel/Design/DesignerHostTests.cs

Tanya-Solyanik · 2019-07-24T23:02:37Z

Btw happy to have it merged as-is to take it into P8, we can do further tweaks as a follow up.

@RussKie - thanks! I would like to remove this attribute from 3 more types, need to double check that I am not missing some scenarios . And then will send to Olina for testing. Thus I made this [WIP] again

Tanya-Solyanik · 2019-08-01T07:01:14Z

For all types that have moved from different assemblies than desktop you should add TypeForwardedFrom attribute to the type. I believe this applies to System.Windows.Forms.Design & System.Windows.Forms.Design.Editors. This impacts serialization from core, consumed in desktop.

@ericstj - we don't have any serializable types in S.W.F.Editors. In System.Windows.Forms.Design we have a single serializable type (CodeDomSerializationStore) This type is used in Copy/Paste scenarios in VisualStudio. We are likely to use BinarySerialization on this type in the CoreDesigner as well, but we don't anticipate scenarios where we exchange types between the Core and .Net. Primarily because even if we are able to deserialize property store and type information of a control from one FX on another FX, we will not be able to discover the "destination" framework assembly. This type will be used to copy/paste controls between 2 Core containers only.

Tanya-Solyanik · 2019-08-01T07:09:41Z

The only issue that concerns me is that currently the tests deserialize .NET Framework blobs on Core but never the other way around, means the tests never run on .NET Framework. Tanya will create a follow-up issue for that.

@ViktorHofer - thank you for the review, here is the issue #1549

…y serialization scenarios and annotates fields of types that still participate inbinary serialization

RussKie · 2019-09-03T14:06:57Z

docs - dotnet/docs#14175

RussKie · 2019-09-16T12:31:24Z

@Tanya-Solyanik reviewing #1237 I spotted that we still have serialisation-related decorations and logic in ResXFileRef:

Should OptionalField attribute and OnDeserializing and OnDeserialized methods be gone?

Tanya-Solyanik · 2019-09-16T22:00:57Z

yes, thank you, good point!

removed here: #1921

Tanya-Solyanik requested a review from a team as a code owner July 19, 2019 03:43

RussKie reviewed Jul 19, 2019

View reviewed changes

RussKie changed the title ~~[WIP] Removeing or annotating Serializable attribute on winforms classse~~ [WIP] Removing or annotating Serializable attribute on winforms classse Jul 19, 2019

Tanya-Solyanik force-pushed the dev/tanyaso/serializable branch 2 times, most recently from b0b8739 to 1e4647f Compare July 20, 2019 00:18

zsd4yr changed the title ~~[WIP] Removing or annotating Serializable attribute on winforms classse~~ [WIP] Removing or annotating Serializable attribute on winforms classes Jul 22, 2019

zsd4yr reviewed Jul 22, 2019

View reviewed changes

src/System.Windows.Forms.Design/src/System/ComponentModel/Design/ExceptionCollection.cs Outdated Show resolved Hide resolved

zsd4yr approved these changes Jul 22, 2019

View reviewed changes

Tanya-Solyanik force-pushed the dev/tanyaso/serializable branch from 1e4647f to 3882bbf Compare July 22, 2019 19:17

Tanya-Solyanik requested a review from zsd4yr July 22, 2019 19:19

Tanya-Solyanik force-pushed the dev/tanyaso/serializable branch 3 times, most recently from 46f1f71 to c2ef8e4 Compare July 23, 2019 20:07

Tanya-Solyanik changed the title ~~[WIP] Removing or annotating Serializable attribute on winforms classes~~ Removing or annotating Serializable attribute on winforms classes Jul 23, 2019

zsd4yr reviewed Jul 23, 2019

View reviewed changes

src/System.Windows.Forms/tests/UnitTests/SerializableTypesTests.cs Outdated Show resolved Hide resolved

zsd4yr approved these changes Jul 23, 2019

View reviewed changes

ericstj reviewed Jul 23, 2019

View reviewed changes

Tanya-Solyanik force-pushed the dev/tanyaso/serializable branch from c2ef8e4 to e365d35 Compare July 24, 2019 06:15

RussKie approved these changes Jul 24, 2019

View reviewed changes

Tanya-Solyanik commented Jul 24, 2019

View reviewed changes

src/System.Windows.Forms/tests/UnitTests/SerializableTypesTests.cs Show resolved Hide resolved

Tanya-Solyanik force-pushed the dev/tanyaso/serializable branch from e365d35 to 52881e1 Compare July 24, 2019 19:39

Tanya-Solyanik commented Jul 24, 2019

View reviewed changes

...ystem.Windows.Forms.Design/tests/UnitTests/System/ComponentModel/Design/DesignerHostTests.cs Outdated Show resolved Hide resolved

Tanya-Solyanik assigned ViktorHofer Jul 24, 2019

Tanya-Solyanik changed the title ~~Removing or annotating Serializable attribute on winforms classes~~ [WIP] Removing or annotating Serializable attribute on winforms classes Jul 24, 2019

Tanya-Solyanik force-pushed the dev/tanyaso/serializable branch from 52881e1 to 2ad76d1 Compare July 24, 2019 23:24

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Jul 29, 2019

This comment has been minimized.

Sign in to view

RussKie mentioned this pull request Jul 31, 2019

Porting DataMemberFieldConverter into core #1545

Merged

Tanya-Solyanik added the tell-mode label Jul 31, 2019

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jul 31, 2019

ghost added this to the 3.0.0-Preview9 milestone Jul 31, 2019

Tanya-Solyanik force-pushed the dev/tanyaso/serializable branch from 95800f6 to 2872a34 Compare August 1, 2019 08:19

merriemcgaw added tell-mode and removed tell-mode labels Aug 1, 2019

Tanya-Solyanik changed the title ~~[WIP] Removing or annotating Serializable attribute on winforms classes~~ Removing or annotating Serializable attribute on winforms classes Aug 2, 2019

Tanya-Solyanik force-pushed the dev/tanyaso/serializable branch 2 times, most recently from 67396e1 to 5b03e70 Compare August 2, 2019 18:30

RussKie added the 📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. label Aug 2, 2019

Tanya-Solyanik added 3 commits August 2, 2019 14:37

Assert.Same->Assert.Equal to enable runnign tests in the VisualStudio.

32348ca

removed SerializationAttribute from types that don't have known binar…

958c116

…y serialization scenarios and annotates fields of types that still participate inbinary serialization

remove serializable attribute from the CodeDomSerializationStore

0fb5f8b

Tanya-Solyanik force-pushed the dev/tanyaso/serializable branch from 5b03e70 to 0fb5f8b Compare August 2, 2019 22:05

Tanya-Solyanik added 2 commits August 2, 2019 15:34

removed unuse resource file

dc70669

change NotSupportedException to PlatformNotSupportedException

4373998

Tanya-Solyanik merged commit 44b5076 into master Aug 3, 2019

Tanya-Solyanik deleted the dev/tanyaso/serializable branch August 9, 2019 16:18

kpreisser mentioned this pull request Aug 25, 2019

Implement the Windows Task Dialog #1133

Merged

3 tasks

This was referenced Sep 9, 2019

CodeDomSerializationStore in not marked as Serializable #1831

Closed

Do we need attributes of the style '[OptionalField(VersionAdded = 2)]' ??? #1237

Closed

ghost locked as resolved and limited conversation to collaborators Feb 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing or annotating Serializable attribute on winforms classes #1425

Removing or annotating Serializable attribute on winforms classes #1425

Tanya-Solyanik commented Jul 19, 2019 •

edited by RussKie

Loading

RussKie left a comment

zsd4yr left a comment

codecov bot commented Jul 23, 2019 •

edited

Loading

zsd4yr commented Jul 23, 2019

ericstj Jul 23, 2019

ericstj Jul 23, 2019

Tanya-Solyanik Jul 24, 2019 •

edited

Loading

ericstj Jul 23, 2019

ericstj Jul 23, 2019

Tanya-Solyanik Jul 24, 2019 •

edited

Loading

ericstj Jul 24, 2019

Tanya-Solyanik Jul 24, 2019

RussKie left a comment •

edited by zsd4yr

Loading

RussKie commented Jul 24, 2019

Tanya-Solyanik commented Jul 24, 2019 •

edited

Loading

This comment has been minimized.

Tanya-Solyanik commented Aug 1, 2019

Tanya-Solyanik commented Aug 1, 2019

RussKie commented Sep 3, 2019

RussKie commented Sep 16, 2019 •

edited

Loading

Tanya-Solyanik commented Sep 16, 2019 •

edited

Loading

Removing or annotating Serializable attribute on winforms classes #1425

Removing or annotating Serializable attribute on winforms classes #1425

Conversation

Tanya-Solyanik commented Jul 19, 2019 • edited by RussKie Loading

Proposed changes

Customer Impact

Regression?

Risk

Test methodology

Microsoft Reviewers: Open in CodeFlow

RussKie left a comment

Choose a reason for hiding this comment

zsd4yr left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 23, 2019 • edited Loading

Codecov Report

zsd4yr commented Jul 23, 2019

ericstj Jul 23, 2019

Choose a reason for hiding this comment

ericstj Jul 23, 2019

Choose a reason for hiding this comment

Tanya-Solyanik Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

ericstj Jul 23, 2019

Choose a reason for hiding this comment

ericstj Jul 23, 2019

Choose a reason for hiding this comment

Tanya-Solyanik Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

ericstj Jul 24, 2019

Choose a reason for hiding this comment

Tanya-Solyanik Jul 24, 2019

Choose a reason for hiding this comment

RussKie left a comment • edited by zsd4yr Loading

Choose a reason for hiding this comment

RussKie commented Jul 24, 2019

Tanya-Solyanik commented Jul 24, 2019 • edited Loading

This comment has been minimized.

Tanya-Solyanik commented Aug 1, 2019

Tanya-Solyanik commented Aug 1, 2019

RussKie commented Sep 3, 2019

RussKie commented Sep 16, 2019 • edited Loading

Tanya-Solyanik commented Sep 16, 2019 • edited Loading

Tanya-Solyanik commented Jul 19, 2019 •

edited by RussKie

Loading

codecov bot commented Jul 23, 2019 •

edited

Loading

Tanya-Solyanik Jul 24, 2019 •

edited

Loading

Tanya-Solyanik Jul 24, 2019 •

edited

Loading

RussKie left a comment •

edited by zsd4yr

Loading

Tanya-Solyanik commented Jul 24, 2019 •

edited

Loading

RussKie commented Sep 16, 2019 •

edited

Loading

Tanya-Solyanik commented Sep 16, 2019 •

edited

Loading