Introduce @Unsafe annotation

[mehmetf]

Introduce @Unsafe to annotate methods or values that typically require a security audit to use.

See go/flutter-lints-dd for rationale.

[pq]

What do you mean by value? Can top-level members and all class members be tagged Unsafe? What about library declarations?

[pq]

Is it an error to annotate a private member?

[bwilkerson]

I agree, the comment could use some work if this annotation is added here, but see my other review comment. For me, the important parts are defining where the annotation can validly be used, what the annotation means, and what behavior the analyzer should implement related to the annotation.

[mehmetf]

This can be used to annotate API. If the unsafe code can appear in client code, we should be able to mark it. Some examples:

• Classes
• Methods
• Public Members
• Top-level Public Members
• Enums
• Enum values (I suppose this is the same as public member?)

Private members should not be marked IMO. If there's a public API that is using the private method, the library maintainer should be responsible for marking that @Unsafe. Also, if there are other protections on the member, there's no need to mark it unsafe. For instance @visibleForTesting. If your code is using such a method that is not meant to be used, then you are already ignoring lints.

Is this a library declaration?

library foo;

and is library the same thing as a pub package? (We don't use library declarations in google3 so I am not familiar with them). I am not sure what would be the effect of marking this unsafe. If you feel that's useful, then sure.

[bwilkerson]

Given that, I'd write the description something like the following:

/// Used to annotate a public member to indicate that any use of the annotated
/// API should require a security review. Such members include top-level
/// variables, functions, getters, setter, classes, constructors, static and
/// instance members of classes, enums, and enum constants.
///
/// If the annotation is applied to an enum then it is equivalent to applying
/// the annotation to all of the constants in that enum. Applying the annotation
/// to a class does *not* apply the annotation to subclasses, but does apply the
/// annotation to all public members of the class.
///
/// Tools, such as the analyzer, can provide feedback if
///
/// * the annotation is associated with a declaration that is not part of the
///   public interface of a library (such as a local variable or a declaration
///   that is private) or a directive,
/// * the declaration is annotated with `@visibleForTesting`, or
/// * the declaration is referenced by any member that is not also marked as
///   being unsafe.

Are the semantics above for classes and enums correct? I was just guessing about those.

Are there annotations other than @visibleForTesting that should disallow the use of this annotation?

Do we also need to be able to use the annotation on mixins and extensions (and their members)? How about typedefs (that seems unlikely to me, but I'm not as well versed in security as you probably are).

If your code is using such a method that is not meant to be used, then you are already ignoring lints.

Possibly a nit, but we produce hints for violations of annotations, not lints. The distinction is that hints are enabled by default in the analyzer whereas lints are disabled by default. Probably not important to the point you were making, but it does impact how we implement checking, so you might want to know the distinction for later work.

Is this a library declaration?

Yes, that's a library declaration. In Dart, a library is a compilation unit that does not contain a part-of directive. A package is a collection of libraries and any parts associated with those libraries. Internally a package is a directory with a build file, though just to be confusing we refer to that directory as a "library".

[mehmetf]

I am unclear whether you can mark a whole library as unsafe. @Unsafe means do NOT use this unless you have a very clear reason to and that reason is ratified by a security review. I cannot think of an entire library that falls under that category. I would prefer library maintainers to be more specific and mark only APIs that are of concern as unsafe.

Ack on lint vs hint. I do think this should be enabled by default. However, I am not sure how we treat hints in google3 @davidmorgan .

Do we also need to be able to use the annotation on mixins and extensions (and their members)?

Yes. A mixin or an extension can easily contain unsafe code.

Typedefs?

I would think not since typedefs by themselves do not contain any logic. I was trying to figure out whether there's a case where a library author might accidentally hide an unsafe class behind a typedef. In that case though, they should simply mark the method(s) that use that Typedef as unsafe.

[bwilkerson]

I would prefer library maintainers to be more specific and mark only APIs that are of concern as unsafe.

I also think that's the right choice.

However, I am not sure how we treat hints in google3 ...

David can correct me if I'm wrong, but I'm fairly sure they cause the build to fail (though there might be a whitelist that allows some to be ignored for that purpose).

I was trying to figure out whether there's a case where a library author might accidentally hide an unsafe class behind a typedef.

The only way I can think of to hide an unsafe class would be by using dynamic. I can't think of any security hole that we'd leave open by not marking typedefs.

Then again, I initially wondered why an enum constant would be marked unsafe (after all, writing E.c doesn't execute any code). I'm guessing that it's to prevent the use of a subset of the constants in places where other constants from the same enum would be perfectly safe.

The question of marking libraries also touches on the semantics of putting @Unsafe on a class. I assume we want that to be equivalent to putting @Unsafe on every public member of the class. Following the logic against annotating a library, I'd have to ask whether it even makes sense to annotate a whole class, or whether we should also prefer to have the specific members marked.

[mehmetf]

Good point about classes vs libraries. You are absolutely right that containers are hard to mark as unsafe. As they get larger (class => library), it takes a lot of discipline to stick to the encapsulation. For instance, a File object might offer write method which can be considered unsafe but also absolutePath getter which might not be. If someone argues that File itself is unsafe, all of a sudden absolutePath needs to find a new place which would not good. We would want to avoid creation of classes like UnsafeFile and SafeFile.

OTOH, it would be easy to move an unsafe declaration to more specific elements. The opposite direction would be harder. We could allow unsafe classes and libraries with the explanation that they should be sparingly used.

I am not sure of the right way to bootstrap annotations. If we want to be conservative and only allow it on constructs we have a use case for, I am happy to drop classes from the list of supported constructs as we don't have a use case for it. We have use cases for method declarations (both class and top level), public members (both class and top level) and enum values.

I'm guessing that it's to prevent the use of a subset of the constants in places where other constants from the same enum would be perfectly safe.

Correct.

Case in point:

enum JavascriptMode {
  @Unsafe('Allowing javascript execution is unsafe. Consider using [disallow].')
  allow,
  disallow,
}

[bwilkerson]

I am not sure of the right way to bootstrap annotations.

I generally prefer to limit the places to which they can be applied to the places we know we need them. If someone tries to use it somewhere new we can always allow that case later, but removing support is a breaking change. Also, the fewer places they're allowed the less code we write and maintain to support and test the feature. But it's always a judgement call, and I'm happy to trust your judgement in this case.

[eernstg]

I like @bwilkerson's proposals. However, we could still make the adjustment about where the metadata is supported, cf. my comment on CHANGELOG.md:

Suggested change

	/// Used to annotate an unsafe method or value in a class.
	/// Annotate an unsafe top level or class member declaration, or enum entry.

If a String valued constructor argument is standardized in some way to specify different kinds of lack of safety, it should be documented here as well.

[pq]

=> instance with a given [reason].

[lrhn]

I'd say :

Creates an unsafe annotation with the given [reason].".

Generally document constructors as "Creates a ... where ..."

[pq]

Perhaps an example?

[pq]

See below w/ a question about what can be annotated @Unsafe.

[eernstg]

Wouldn't that be @unsafe or @Unsafe() in practice?

Why not include a possible qualification, e.g., a constructor argument of type String, such that it could be indicated that one API typically requires a security audit (e.g., @Unsafe('security-audit')), another one could be unsafe because it runs an algorithm that uses a lot of resources (time and/or memory, e.g., @Unsafe('memory')), etc.

I can see that the declaration in 'meta.dart' already supports a String valued constructor argument giving a reason, and the notion of having more than one kind of lack-of-safety would then simply be a matter of standardization with respect to that string (@Unsafe("security-audit: ...<reason>...")).

About the supported targets: We could say

Suggested change

	* Introduce `@Unsafe` to annotate methods or values that typically require
	* Introduce `@Unsafe` to annotate top level declarations, class member
	declarations, or enum entries that typically require

This would allow all the constructs that @mehmetf mentions in the comment on line 281. It would rule out "smaller" things (type parameters, formal parameters, local functions and variables including for loop iteration variables), and library level entities (imports/exports, part and part of directives, and library directives).

You could make the case that an optional formal parameter should be allowed to carry this metadata as well: An @unsafe optional parameter would be one where only the default value is safe. But it's probably not sufficiently useful to carry its own weight.

[bwilkerson]

Where do you want to be able to use this annotation? My guess is that you'd want to use it in dart:html or some other SDK library. If that's the case, then I believe you'll need to define the annotation in the SDK. There used to be, and probably still is, a restriction that the SDK wasn't allowed to refer to anything outside the SDK.

[bwilkerson]

I agree, the comment could use some work if this annotation is added here, but see my other review comment. For me, the important parts are defining where the annotation can validly be used, what the annotation means, and what behavior the analyzer should implement related to the annotation.

[mehmetf]

@bwilkerson The current use cases are all for non-SDK packages. However, you are quite right that this type of generic annotation can be used in the SDK as well.

I suppose this question is more for Dart SDK developers. Do we ever intend to mark SDK constructs as unsafe? Can we @ mention someone who can answer this?

[bwilkerson]

Sounds like a question for the language team: @leafpetersen @lrhn @munificent @eernstg

[kevmoo]

CC @mit-mit RE core-platform thoughts.

Here's my vote for a short design doc and a bit of discussion before we land this...

[mehmetf]

The rationale behind this annotation is captured at go/flutter-lints-dd. Updated description of the PR.

[lrhn]

I'd prefer not to mark SDK members as "unsafe".

Whether something is "unsafe" is domain specific.

If you are writing back-end code, you don't want all of dart:io to be unsafe. If you are running run-time-supplied plugins on a deployed platform, you probably do consider all of dart:io unsafe (and not in a way that's fixable by reviewing). The concept of "security audit" doesn't apply to eveybody either.

Unless there is an unambiguous and universal definition of what is unsafe, it is not something that belongs in the platform libraries. What you could do is to allow a way to declaratively make third-party API unsafe, say:

@Unsafe.includes(["dart:io", #Crypto, #Zone.root])
library;

where this marks the entire library dart:io as unsafe, as well as the Crypto class and the Zone.root getter.

Then you can specify, for each domain, which code is considered unsafe for you.

There shouldn't be anything public in the Dart platform libraries which is inherently not intended for use. If there is, it'll be deprecated instead.

[lrhn]

I'd say :

Creates an unsafe annotation with the given [reason].".

Generally document constructors as "Creates a ... where ..."

[eernstg]

Added a couple of drive-by comments.

[davidmorgan]

Re: google3, Brian is correct, hints cause the build to fail by default. We can choose to ignore hints everywhere (today: TODO) or selectively (today: deprecation hints). So, in practice there's little difference between hints and lints. When something can be a hint, I prefer that, since there is a higher bar for something becoming a hint and so less work for me to evaluate/enforce :)

FWIW I think Lasse has a good point--requiring the annotation directly on the API seems too restrictive. It'd be good to have a way for it to apply at a distance. This seems like something new: we'll likely need a way to have such a configuration written once for e.g. all of Flutter in google3 and discovered by the analyzer.

[mehmetf]

Good points @lrhn re: how far we can take this "annotation". One of the use cases I highlighted in the doc go/flutter-lints-dd is indeed dart:io. I did not propose that we implement that using @Unsafe but rather as its own lint. I am going to move your proposal to the design doc, we can further discuss there.

[mehmetf]

Just wanted to address:

The concept of "security audit" doesn't apply to eveybody either.

My original proposal was to define this as a lint as opposed to a hint. I agree that we should not just start warning everyone but I also believe the concept of security audit applies to many large companies, not just Google.

[mehmetf]

Closing this PR as there's a better proposal being discussed.