Skip to content

.NET Core samples should not include CAS elements #837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
danmoseley opened this issue Aug 16, 2018 · 11 comments
Open

.NET Core samples should not include CAS elements #837

danmoseley opened this issue Aug 16, 2018 · 11 comments
Assignees
Labels
area-System.Security Issues related to security practices for .NET developers. doc-bug Problem with the content; needs to be fixed dotnet-api/prod Pri2 Indicates issues/PRs that are medium priority
Milestone

Comments

@danmoseley
Copy link
Member

In .NET Core, CAS types (like FileIOPermission, etc) and Security Transparency types are stubbed only. CAS is not supported. Therefore the samples in .NET Core topics should not include use of them.

For example, in the AppDomain.UnhandledException topic for .NET Core 2.1 it has this sample:

using System;
using System.Security.Permissions;

public class Example 
{
   [SecurityPermission(SecurityAction.Demand, Flags=SecurityPermissionFlag.ControlAppDomain)]
   public static void Main()
   {
...

Here [SecurityPermission(SecurityAction.Demand, Flags=SecurityPermissionFlag.ControlAppDomain)] should be removed as it does nothing and implies that CAS is relevant.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@mairaw
Copy link
Contributor

mairaw commented Aug 16, 2018

Thanks @danmosemsft. I've added this issue to our security review project. We're hoping to get started on this soon.

@BillWagner BillWagner transferred this issue from dotnet/docs Nov 20, 2018
@dotnet-bot dotnet-bot added untriaged New issue has not been triaged by the area owner Source - Docs.ms and removed rerun-labels labels Nov 20, 2018
@danmoseley
Copy link
Member Author

Here's another example, which reminded me.
https://docs.microsoft.com/en-us/dotnet/api/system.console.bufferheight?view=netcore-3.0

Security
UIPermission 
for modifying safe top-level windows and subwindows. Associated enumeration: SafeTopLevelWindows

A bit confusing if you heard that CAS is dead.

@rpetrusha rpetrusha self-assigned this May 10, 2019
@rpetrusha rpetrusha removed the untriaged New issue has not been triaged by the area owner label May 10, 2019
@rpetrusha rpetrusha added this to the June 2019 milestone May 10, 2019
@rpetrusha
Copy link

This issue has languished, @danmosemsft, but I'll start addressing it in the next sprint.

@danmoseley
Copy link
Member Author

Thanks, @rpetrusha

@rpetrusha rpetrusha modified the milestones: June 2019, July 2019 Jul 1, 2019
@rpetrusha rpetrusha modified the milestones: July 2019, August 2019 Jul 26, 2019
@rpetrusha rpetrusha modified the milestones: August 2019, October 2019 Sep 9, 2019
@rpetrusha rpetrusha removed their assignment Oct 11, 2019
@rpetrusha rpetrusha modified the milestones: October 2019, Backlog Oct 11, 2019
@tdykstra tdykstra added the area-System.Security Issues related to security practices for .NET developers. label Mar 9, 2020
@tdykstra
Copy link
Contributor

@danmosemsft How can I get a list of the CAS types and Security Transparency types?

@tdykstra tdykstra added doc-bug Problem with the content; needs to be fixed waiting-on-feedback Indicates PRs that are waiting for feedback from SMEs before they can be merged labels Jun 11, 2020
@danmoseley
Copy link
Member Author

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Security.Permissions/ref/System.Security.Permissions.cs seems like a pretty good list

@GrabYourPitchforks does that map pretty well to what should not be in samples anymore?

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Jun 12, 2020

That list, plus pretty much everything else in the System.Security.Permissions namespace, regardless of which reference assembly defines it.

Conveniently, Dan just provided a nice list of every API I want to slap [Obsolete] on. ;)

@tdykstra tdykstra removed the waiting-on-feedback Indicates PRs that are waiting for feedback from SMEs before they can be merged label Jun 12, 2020
@tdykstra
Copy link
Contributor

The only instances I've found where the CAS types are used for Core snippets are in API ref. There, the same snippet is shown regardless of whether Core or Framework is selected. To remove CAS from the snippet removes it from the Framework 4.x version too. Some options that come to mind:

  1. Duplicate each snippet, remove CAS types from one copy, and include both copies in the doc with an appropriate heading for each.
  2. Remove CAS types from each snippet, i.e., let the CAS types be missing even when Framework 4.x is selected.
  3. Add a code comment at the top of each snippet noting that CAS doesn't apply to Core.

Given that > 1,000 snippet files are affected, all of these options will involve a significant time investment, # 3 seems least problematic, and # 1 seems questionable whether the benefit would outweigh the cost.

@danmoseley
Copy link
Member Author

We haven't supported CAS in .NET Framework for years either. So I think generally (2) would make most sense.

I see the size of the task. If I search samples\snippets for ^\s*\[\w+Permission[^\]*\] I see 819 hits in .cs files. That ignores .vb, and attributes that cross lines. And any imperative use of these things. And of course any use of them in the docs for themselves needs to be preserved.

The good news is that it doesn't have to be perfect or complete. My suggestion is to look for something like the regex above (possbily improved to straddle lines) and just search and replace it away. Then do the same (using angle brackets) for VB. Then bulk revert anything the seems inappropriate. Then compile and either fix or just revert anything that breaks. I'd imagine you could spend a few hours and make a major dent, if you reverted anything that broke.

It does seem like a bunch of work, but after all these years, we should not be encouraging people to continue using these broken things. Thoughts?

@tdykstra
Copy link
Contributor

That sounds like a good plan to me, I'll get started on it next week.

@tdykstra
Copy link
Contributor

Discussed this issue offline with @GrabYourPitchforks and @danmosemsft. Levi will identify specific categories of changes required, and for each category I'll make a PR that addresses all affected snippets.

@PRMerger10 PRMerger10 added the Pri2 Indicates issues/PRs that are medium priority label Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Security Issues related to security practices for .NET developers. doc-bug Problem with the content; needs to be fixed dotnet-api/prod Pri2 Indicates issues/PRs that are medium priority
Projects
None yet
Development

No branches or pull requests

8 participants