Support an extensible avoid_unsafe_apis lint in analyzer

[mehmetf]

See http://go/flutter-lints-dd for overall context. Also see #40429 for a separate attempt to add a generic Unsafe annotation which is how this proposal sprung to life.

tl;dr

We would like to define a new lint avoid_unsafe_apis where the definition of unsafe APIs is domain specific and can be passed in as input to the analyzer.

Proposal

avoid_unsafe_apis lint would read some configuration that would contain a list of symbols and the rationale for considering them banned. We first define a class to represent Unsafe APIs and the rationale.

class Unsafe {
  // The fully qualified name of the symbol
  final String symbol;
  // The reason why this symbol is considered unsafe
  final String reason;
  
  const Unsafe(this.symbol, this.reason);
}

Then the application can define a Dart file that contains the list of APIs it considers unsafe:

import ‘dart:analyzer’ show Unsafe;

const unsafeFlutterApis = [
  Unsafe('package:mypackage/MyClass#myMethod', 'Unsafe method. Consider using other method'),
  Unsafe('package:mypackage/MyClass#someValue', 'Unsafe value. Consider using other value.'),
]

In analysis_options.yaml file, annotate the avoid_unsafe_apis lint with a list of files that contain these banned apis:

• avoid_unsafe_apis
  • ./unsafe_apis.dart
  • some/other/path/unsafe_apis.dart

Analyzer reads this configuration and finds the union of all unsafe APIs listed in these files. The lint itself simply walks through various expression types and flags if the statement matches to any of the unsafe symbols.

Currently the following constructs can be marked as unsafe: Top-level variables, functions, getters, setters, constructors, static and instance members of classes, enums, and enum constants. We do not want to be able to mark entire Classes or Libraries as unsafe.

FAQ

Why Dart instead of json or yaml?

We can eventually make it easier and less error prone to refer to Dart symbols in a Dart file. You can imagine an evolution of Unsafe class as follows:

class Unsafe {
  final Symbol symbol;
  final String reason;
  final bool ignorable;
  
  const Unsafe(this.symbol, this.reason, {this.ignorable = true});
}

and the corresponding unsafe_apis.dart file:

import ‘dart:analyzer’ show Unsafe;
import ‘package:my_package/my_package.dart’;

const unsafeFlutterApis = [
  Unsafe(#MyClass.myMethod, ‘Unsafe method. Consider using other method’, ignorable: false),
  Unsafe(#MyClass.someValue, ‘Unsafe value. Consider using other value.’),
]

This way, we can ensure the correctness of the symbols without having to write another set of tests to map string representations to actual symbols.

I would love it if we could get to this in v1.

Why support multiple files?

For very large projects, a single file might get too unwieldy. Developers would prefer to split these perhaps by package. Internally, we would like each unsafe_apis.dart to live in its own package directory rather than in the application. Analyzer will be provided with the full path of these files.

Why not mark Classes and Libraries as unsafe?

Large containers of code 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 to be used safely. We would want to avoid creation of classes like UnsafeFile and SafeFile and instead rely on library maintainers marking the smallest unit of compilation that deserves the unsafe annotation.

We also don't want to mark large containers as unsafe because the only mechanism to allow an unsafe API is via //ignore comment. This forces the developer to give a valid reason for every instance of unsafe API usage (including links to a security review). If we were to mark entire libraries as unsafe, you will end up seeing unintended effects such as:

//ignore(unsafe_apis): Just using absolutePath which really is safe.
final filePath = File('foo').absolutePath;

See #40429 for further discussion.

[mehmetf]

@davidmorgan @pq @bwilkerson FYI

[bwilkerson]

I generally like this proposal, but I do have a few comments / suggestions.

We should consider making this a hint (enabled by default) rather than a lint (disabled by default). The reason I say that is because the hint wouldn't do anything unless the user has also specified a list of files containing descriptions of unsafe APIs, so providing that list is already a way of opting in, and it seems better to not require the user to opt in multiple times. Not a big issue internally because we'll have a small number of people that control the analysis options files, but it would improve the usability externally.

As for the list of files, relative file paths would work fine internally because everyone has exactly the same layout on disk, but externally that isn't the case. I'd like to propose that we use URIs. This would make this feature easier to use externally and would be consistent with the way the include directive works. We might even want to consider publishing our internal list(s) of unsafe APIs externally as another part of the pedantic package.

If we do use a hint, then we'll need a different place to add the list of unsafe APIs. I'd like to propose that we use something like the following:

analysis:
  security:
    unsafe_apis:
      - package:thisPackage/unsafe_apis.dart
      - package:some.other.package/unsafe_apis.dart

Of course, relative paths would still work as a valid URI, just like in import statements, but this provides more flexability. I added the extra security key so that we could provide additional security related information in the future if we discover a need for it.

The class Unsafe should not be defined in the analyzer package. I didn't think of this implication in our earlier conversations, but we don't want every package that uses this feature to have a dependency on the analyzer package; it's a very big package that pulls in lots of other dependencies. I think we should put this class in the meta package.

Using symbols for the APIs that are unsafe is definitely better than using strings, but it might be better to use an arbitrary expression. The only compile-time checking done for symbols is to ensure that they are syntactically valid. In particular, there's no association made between #Foo.bar and any member of any class. If there are multiple classes named Foo that define a member named bar, then analyzer wouldn't have any way to know which one was meant. If we were to use an arbitrary expression instead, then analysis would confirm that the element had been imported and that there is no ambiguity about which element is being referenced. It would also mean that renaming operations would be able to find the reference to the element without any special support. It would look something like:

import ‘package:my_package/my_package.dart’;

var unsafeFlutterApis = [
  Unsafe(MyClass.myMethod, ‘Unsafe method. Consider using other method’, ignorable: false), // method
  Unsafe(MyClass.someValue, ‘Unsafe value. Consider using other value.’), // getter
  Unsafe(MyClass.someValue = throw 0, ‘Setting this is not safe.’), // setter
]

This is safe because the code will never be executed. The one case I'm not thrilled with is setters, but I think we can live with it.

Note that I also changed the example from const to var. This file won't be loaded into any runtime, so there's no advantage to making the instances constant (because the instances won't actually ever exist). For the same reason, there's no real reason to define fields. The class probably wants to look like the following:

class Unsafe {
  Unsafe(Object reference, String reason, {bool ignorable = true});
}

@matanlurey Curious as to whether this would (or could be made to) address your needs. I'd prefer to have a single solution if possible.

[matanlurey]

I don't have time to go into a lot of detail with OOO coming up, but in summary, this might work for our needs as written, but I am not confident:

1. We would need to annotate APIs in the SDK itself (dart:html), would that work?

2. We need to be able to control separate whitelists, so having a single unsafe_api error code would not be fine grained enough in order to grandfather existing uses in or control policy exceptions.

The best person to discuss this with would be @srawlins who is currently OOO.

[bwilkerson]

1. We would need to annotate APIs in the SDK itself (dart:html), would that work?

I believe that this would work for APIs in the SDK by doing something like the following:

import 'dart:html';

var unsafeApis = [
  Unsafe(Element.appendHtml, "Don't append raw HTML."),
];

The analyzer would then flag any use of Element.appendHtml.

2. We need to be able to control separate whitelists, so having a single unsafe_api error code would not be fine grained enough in order to grandfather existing uses in or control policy exceptions.

That's a bit trickier, but I can think of a couple of possibilities. I'll talk to Sam when he's back to work out the details.

[pq]

Thoughtful write-up!

We've talked about opening lints up to configuration (proposed for example in #57673). If we supported that, could this be simplified to look something like this?

linter:
  rules:
    unsafe_apis:
      api:
        - element: 'package:mypackage/MyClass#myMethod'
        - description: 'Unsafe method. Consider using other method'
      api:
        - element: 'package:mypackage/MyClass#myVar'
        - description: 'Setting {0} is not safe.' 

or, taking it further:

linter:
  rules:
    unsafe_apis:
        element: 
             - kind: method_invocation
             - name:  'package:mypackage/MyClass#myMethod'
             - description: 'Invoking method {0} is unsafe. Use OtherClass#foo instead.'

[bwilkerson]

If we supported that, could this be simplified to look something like this?

Yes. One downside is that the information is now buried in strings, so refactorings, "find references", etc. won't work with them (at least not without additional work).

Another is that all of these unsafe APIs would need to be specified in a single file, and there is a goal of allowing them to be split up. I don't know how important that goal will be in practice.

[mehmetf]

Re: @matanlurey's comment on whitelisting... I was imagining all grandfathered usage would need to be annotated in code with an //ignore comment. There are certain use cases which does not fit this lint. For instance, we cannot use it to ban all of dart:io from Flutter apps (see "Why not ban Class or Library" in the FAQ). Such a lint would need to be defined as standalone and likely be optional. Given that this lint is meant to represent a security concern for a specific API, I would not want to protect entire packages from it to avoid further propagation of the API. What do you think?

+1 to all of @bwilkerson's comments. I would very much likely avoid a stringified representation of these APIs for the reasons Brian mentions. I agree that the multiple configuration files use case is weak. It primarily existed because if we do have real symbols/references in this file, a single unsafe_apis.dart file would end up having 100s of dependencies in a large centralized repository like google3. However, this file can be generated in google3 from smaller unsafe_apis.dart files breaking the dependency chain.

[bwilkerson]

There is a potentially interesting external case for allowing multiple URLs to be specified: if individual package providers wanted to provide a file specific to the APIs exposed by their package. I don't know how likely that is to happen, but I'd want any design to not preclude future support for multiple URLs (assuming that we support a single URL initially).

[davidmorgan]

I don't think generating smaller files in google3 is sufficient to solve the dependency problem; I think we'll need to at least allow (not require) stringified config.

Otherwise, we force you to take a dependency on a library in order to ensure you don't use it. This is certain to bite us at some point. (Your code wouldn't depend on the library, but your build would, in order to run analysis; that means any change to the library you're not supposed to use will force your build to rerun / trigger all your tests on the continuous build.)

[mehmetf]

We force you to take a dependency on a library in order to ensure you don't use it.

This lint should only be used for cases where you are banning a subset of APIs exposed by a library that is already being used by the app. In google3, we would have a centralized unsafe_apis.dart only for dart core libraries. Every other API ban will happen within the library-specific unsafe_apis.dart which will be owned by the library maintainer. This file will only be considered if the app depends on the library.

This lint should not be used for cases where you are banning an entire library like dart:io or package:this_is_only_meant_for_server. The circumstances for such a wide ban is different enough (1) that a different mechanics is warranted (2).

1: See FAQ in the proposal.
2: Perhaps a separate optional lint? or a build dependency ban with whitelisting?

[lrhn]

For declaring unsafeness in a library, I'd very much recommend using an annotation, not a normal declaration.

The normal declaration introduces a name that can be accessed by arbitrary code and retained at run-time, which seems unnecessary for something that is only intended for the analyzer.
I also notice that the declarations are not constant, and neither are the instances of Unsafe, so the analyzer would need to do extra work to figure out what the value is. The declarations should at least be constant, and probably also private. Then it's just better to use annotations.

For:

import 'dart:html';

var unsafeApis = [
  Unsafe(Element.appendHtml, "Don't append raw HTML."),
];

I would write

@Unsafe(#Element.appendHtml, "Don't append raw HTML")
import 'dart:html';

and for

import ‘package:my_package/my_package.dart’;

var unsafeFlutterApis = [
  Unsafe(MyClass.myMethod, ‘Unsafe method. Consider using other method’, ignorable: false), // method
  Unsafe(MyClass.someValue, ‘Unsafe value. Consider using other value.’), // getter
  Unsafe(MyClass.someValue = throw 0, ‘Setting this is not safe.’), // setter
];

I would do

@Unsafe(#MyClass.myMethod, ‘Unsafe method. Consider using other method’, ignorable: false), // method
@Unsafe(#MyClass.someValue, ‘Unsafe value. Consider using other value.’), // getter
@Unsafe(#MyClass.someValue=, ‘Setting this is not safe.’), // setter
library something.something;

(I really want to allow a library; declaration with no name so you have an easy place to hang annotations).

In both cases you cannot refer to instance methods like MyClass.myMethod. That's a compile-time error because the method is not static. You cannot refer to getters or setters like that, the argument to an annotation is evaluated as a constant expression, it's not just untouched syntax.

(I'd also love to have ##MyClass.myMethod which is a symbol which really does refer to the instance method, and where it become a compile-time error if the thing isn't there, that would allow refactoring to work).

We should consider some way of marking annotations as "not retained at run-time" (like the Java retention policy annotation).

[bwilkerson]

For declaring unsafeness in a library, I'd very much recommend using an annotation, not a normal declaration.

The normal declaration introduces a name that can be accessed by arbitrary code and retained at run-time, which seems unnecessary for something that is only intended for the analyzer.

True. But I think the danger that arbitrary code would attempt to access these objects is fairly low. Still, I suppose it might be considered a security hole to have code that could be used at runtime to determine which APIs are not safe to use. We could get around the latter by making the top-level variable final, or even by putting the constructor invocations in a private top-level function.

I also notice that the declarations are not constant, and neither are the instances of Unsafe, so the analyzer would need to do extra work to figure out what the value is.

Not true. The analyzer wouldn't need to evaluate anything in order to get the information it needs. All of the information is available through static analysis without any extra work. Using annotations wouldn't be any better from an implementation point of view.

In both cases you cannot refer to instance methods like MyClass.myMethod. That's a compile-time error because the method is not static.

You are, of course, right. I missed that. In which case I suppose we have no choice by to use symbols.

You cannot refer to getters or setters like that, the argument to an annotation is evaluated as a constant expression, it's not just untouched syntax.

True. Not that it matters because that syntax won't work for instance members, but I was suggesting using it as an argument to a non-const constructor, in which case it wouldn't be a constant expression.

(I'd also love to have ##MyClass.myMethod which is a symbol which really does refer to the instance method, and where it become a compile-time error if the thing isn't there, that would allow refactoring to work).

We don't necessarily need new syntax for that purpose. We could define a lint that would catch the use of symbols that do not correspond to existing declarations. We could also special case these "unsafe api" files to require that all symbols reference existing declarations and even support them in our rename refactoring.

[lrhn]

My main worry about using program-level declarations for metadata is not security, just style. It is metadata, so it should be represented as metadata, which in Dart means annotations. At least unless there is a very compelling reason to do something else, which there does not seem to be here.

It's cool that the analyzer can handle non-constant expressions without extra effort, but if any other source-tool needs access to the information, it would still be better if it was stored as constant annotations like other metadata.

It does make it constant expressions, which comes with some restrictions, but probably not something which is insurmountable.

As for ##source.reference.symbols, it's definitely not needed, but it is something I'd want for other reasons, if nothing else then having compile-time errors when my symbol stops referring to something without having to use the analyzer. (But then, I'd probably want ##ClassName.methodName to evaluate to the same symbol as #methodName, so it won't be useful as a symbolic path).