Skip to content

Introduce @Unsafe annotation #40429

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Introduce @Unsafe annotation #40429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/meta/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 1.1.9

* Introduce `@Unsafe` to annotate methods or values that typically require
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@eernstg eernstg Feb 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

a security audit to use.

## 1.1.8

* Introduce `@nonVirtual` to annotate instance members that should not be
Expand Down
18 changes: 18 additions & 0 deletions pkg/meta/lib/meta.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,24 @@ class Required {
const Required([this.reason = '']);
}


/// Used to annotate an unsafe method or value in a class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it an error to annotate a private member?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@eernstg eernstg Feb 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// This annotation is expected to be used in sensitive libraries where API
/// usage typically requires a security review.
class Unsafe {

/// Human readable reason of why this construct is unsafe.
///
/// This is required because unsafe annotations are not self-explanatory.
/// Explanation should give alternative APIs to use if applicable. It should
/// also link to documentation with more details as to why the API is unsafe.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps an example?

final String reason;

/// Initialize a newly created instance with a given reason.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> instance with a given [reason].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say :

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

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

const Unsafe(this.reason);
}

class _AlwaysThrows {
const _AlwaysThrows();
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/meta/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: meta
version: 1.1.8
version: 1.1.9
author: Dart Team <[email protected]>
homepage: https://github.com/dart-lang/sdk/tree/master/pkg/meta
description: >
Expand Down