Perform more security cleanup by hughbe · Pull Request #801 · dotnet/winforms

hughbe · 2019-04-16T15:37:43Z

No description provided.

zsd4yr · 2019-04-16T16:16:48Z

@hughbe you're going to have to break this down more into multiple commits; also the deleted code is concerning

hughbe · 2019-04-16T17:22:55Z

@zsd4yr hopefully this helps!

codecov · 2019-04-16T18:18:31Z

Codecov Report

Merging #801 into master will increase coverage by 0.19622%.
The diff coverage is 25.37313%.

@@                Coverage Diff                @@
##            master        #801         +/-   ##
=================================================
+ Coverage   28.642%   28.83822%   +0.19622%     
=================================================
  Files         1075        1078          +3     
  Lines       293506      293919        +413     
  Branches     38399       38408          +9     
=================================================
+ Hits         84066       84761        +695     
+ Misses      205407      205124        -283     
- Partials      4033        4034          +1

Flag	Coverage Δ
#Debug	`28.83822% <25.37313%> (+0.19622%)`	⬆️
#production	`18.75241% <23.31288%> (+0.07146%)`	⬆️
#test	`98.68151% <100%> (+0.00849%)`	⬆️

zsd4yr · 2019-04-19T16:40:11Z

@zsd4yr hopefully this helps!

That is helpful. Would you mind doing me another favor and breaking these down the first commit even more into:

changes to using statements
comment changes; and
changes inside methods

zsd4yr · 2019-04-19T16:40:54Z

Mostly because I really only want to PR number 3 😉

danmoseley · 2019-04-21T06:05:06Z

src/Common/tests/InternalUtilitiesForTests/src/PlatformDetection.Windows.cs

I believe registry operations throw SecurityException for regular ACL issues (not just CAS) instead of UnauthorizedAccessException - if so this line should be reverted.

danmoseley · 2019-04-21T06:08:09Z

src/System.Windows.Forms/src/System/Windows/Forms/Application.cs

What happened to UnsafeNativeMethods.ThemingScope.CreateActivationContext(assemblyFileName, nativeResourceID); ?

danmoseley · 2019-04-21T06:11:10Z

src/System.Windows.Forms/src/System/Windows/Forms/RichTextBox.cs

Previously it passed null for the final parameter, now 0. I guess one marshals to the other?

danmoseley

Looks OK with couple comments.

@hughe thanks for doing this. Breaking out into commits helped. However, there are still unrelated changes, even within commits. What would really help reviewers is smaller commits and smaller PR's. Eg., pure refactoring to eg., remove or move using statements, fix comments and brackets and braces could be all in 4 commits and one PR, the work to coalesce the "Internal" properties that weren't needed anymore really should be its own PR, the remaining CAS cleanup another PR. This would make it much quicker to review because we need less "state" in our head and can pay more attention where it's needed. As a rule of thumb, changes that shouldn't change IL (formatting, comments) ought to be reall fast to review in their own PR.

Also, this repo isn't yet tested as well as it ideally would be. Separating out commits and PR's (especially given most PR's get squashed) might help us later if we need to bisect and reverse an error.

It's great to get all these changes from you, I'm just thinking about how to get them in most easily and reliably. cc @merriemcgaw

danmoseley · 2019-04-21T06:20:20Z

Looks like there's some rebasing required. 😸

RussKie · 2019-04-21T06:44:13Z

especially given most PR's get squashed

Perhaps this practice as blanket rule should be reconsidered, precisely for the above mentioned reasons.

It is author's responsibility to present work in clear and easy to understand manner, separating functional and non functional changes.
A PR should (must) contain only a single functional change, and may contain several NF changes related to the F change.

hughbe · 2019-04-21T15:34:53Z

I don't expect this PR to build... waiting on #801 to be merged :)

zsd4yr · 2019-04-22T15:28:20Z

I don't expect this PR to build... waiting on #801 to be merged :)

adding [WIP] since this depends on another PR

weltkante · 2019-04-22T18:30:33Z

@hughbe this is #801, you probably meant you are waiting for something else?

hughbe · 2019-04-22T18:33:26Z

Oops! #843

zsd4yr · 2019-04-24T17:26:20Z

Oops! #843

Why is this change blocked by that one?

hughbe · 2019-04-24T17:52:09Z

Would cause more merge conflicts.

danmoseley · 2019-04-27T21:49:55Z

Perhaps this practice as blanket rule should be reconsidered, precisely for the above mentioned reasons. It is author's responsibility to present work in clear and easy to understand manner, separating functional and non functional changes.
A PR should (must) contain only a single functional change, and may contain several NF changes related to the F change.

In CoreFX, we avoid this by asking that significant NF changes are in a separate PR.

danmoseley · 2019-04-27T21:50:23Z

@hughbe happy to take another look when you've done battle with the merge,

RussKie · 2019-04-28T12:47:17Z

In CoreFX, we avoid this by asking that significant NF changes are in a separate PR.

Totally in agreement 👍
This typically works well in controlled environments (e.g. in a workplace/where contributors are incentivised to follow up on submissions) and doesn't quite work in the "wild" (afterhours hobby OSS - it may be weeks/months before reviews/feedback/merge occur).

Over the years in different projects I've experienced various styles.
Some years ago in a busy framework repo we've only accepted PRs containing a single commit - F or NF changes. When the team shrunk and amount of changes subsided - a PR with a single F + NF commits became a norm.
In https://github.com/gitextensions/gitextensions we accept PRs with a single F + additional NFs (if needed).

In the end of the day - it is all about the reviewer experience - they must be able to clearly understand what/how/why changed. And it is the author responsibility to present the work in such manner.

danmoseley · 2019-04-28T17:02:43Z

src/System.Windows.Forms/src/System/Windows/Forms/Application.cs

@@ -51,10 +51,10 @@ public sealed class Application {
        static string productName;
        static string productVersion;


Looks like we should make a pass (separately) to add visibility specifiers everywhere.

src/System.Windows.Forms/src/System/Windows/Forms/Application.cs

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

danmoseley · 2019-04-28T17:11:07Z

@hughbe this seems fine to merge - especially before we get more conflicts - can you remove the WIP ?

danmoseley · 2019-04-28T17:11:37Z

If you address the feedback please use new commits as usual 😸

hughbe requested a review from a team as a code owner April 16, 2019 15:37

hughbe force-pushed the more-security-cleanup branch from 3c70ad2 to 8d793a5 Compare April 16, 2019 17:20

hughbe force-pushed the more-security-cleanup branch from 8d793a5 to 599c2c5 Compare April 16, 2019 21:35

danmoseley mentioned this pull request Apr 21, 2019

Remove remaining CAS/transparency goo #841

Merged

danmoseley reviewed Apr 21, 2019

View reviewed changes

danmoseley approved these changes Apr 21, 2019

View reviewed changes

hughbe force-pushed the more-security-cleanup branch from 599c2c5 to 66f6688 Compare April 21, 2019 15:30

hughbe mentioned this pull request Apr 21, 2019

Extract security cleanups #843

Merged

zsd4yr changed the title ~~Perform more security cleanup~~ [WIP] Perform more security cleanup Apr 22, 2019

hughbe added 3 commits April 28, 2019 08:55

Remove *Internal methods that are no longer relevant

db6a23c

Remove dead security code

73ff032

Remove ResourceExposure

94a1fbb

hughbe force-pushed the more-security-cleanup branch from 6e1e4e6 to 94a1fbb Compare April 28, 2019 07:56

danmoseley reviewed Apr 28, 2019

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/Application.cs Show resolved Hide resolved

danmoseley reviewed Apr 28, 2019

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs Show resolved Hide resolved

danmoseley approved these changes Apr 28, 2019

View reviewed changes

PR feedback

27ae2f7

hughbe changed the title ~~[WIP] Perform more security cleanup~~ Perform more security cleanup Apr 28, 2019

danmoseley merged commit be88f03 into dotnet:master Apr 28, 2019

hughbe deleted the more-security-cleanup branch April 28, 2019 19:37

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

		@@ -51,10 +51,10 @@ public sealed class Application {
		static string productName;
		static string productVersion;

Conversation

hughbe commented Apr 16, 2019

Uh oh!

zsd4yr commented Apr 16, 2019

Uh oh!

hughbe commented Apr 16, 2019

Uh oh!

codecov bot commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zsd4yr commented Apr 19, 2019

Uh oh!

zsd4yr commented Apr 19, 2019

Uh oh!

danmoseley Apr 21, 2019

Choose a reason for hiding this comment

Uh oh!

danmoseley Apr 21, 2019

Choose a reason for hiding this comment

Uh oh!

danmoseley Apr 21, 2019

Choose a reason for hiding this comment

Uh oh!

danmoseley left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Apr 21, 2019

Uh oh!

RussKie commented Apr 21, 2019

Uh oh!

hughbe commented Apr 21, 2019

Uh oh!

zsd4yr commented Apr 22, 2019

Uh oh!

weltkante commented Apr 22, 2019

Uh oh!

hughbe commented Apr 22, 2019

Uh oh!

zsd4yr commented Apr 24, 2019

Uh oh!

hughbe commented Apr 24, 2019

Uh oh!

danmoseley commented Apr 27, 2019

Uh oh!

danmoseley commented Apr 27, 2019

Uh oh!

RussKie commented Apr 28, 2019

Uh oh!

danmoseley Apr 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Apr 28, 2019

Uh oh!

danmoseley commented Apr 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Apr 16, 2019 •

edited

Loading

danmoseley left a comment •

edited

Loading