isInsecureConnectionAllowed documentation needs improvement

[mraleph]

Here is what we have now:

/// Whether insecure connections to [host] are allowed.
///
/// [host] must be a [String] or [InternetAddress].
///
/// If any of the domain policies match [host], the matching policy will make
/// the decision. If multiple policies apply, the top matching policy makes the
/// decision. If none of the domain policies match, the embedder default is
/// used.
///
/// Loopback addresses are always allowed.

It needs to be expanded to document how network policies are specified (because there is nothing in the dart:io documentation which explains it).

I had to read SDK source to figure out that there are two environment defines (dart.library.io.domain_network_policies and dart.library.io.may_insecurely_connect_to_all_domains plus an _EmbedderConfig). _EmbedderConfig probably does not need public documentation, but environment defines most certainly do.

/cc @lrhn @mehmetf

[mehmetf]

That was intentional. The environment defines were created solely for testing and are not meant to be public API. I agree it is not ideal. Let me know if there's a different way of testing these.

_EmbedderConfig is the only intended way for users to (indirectly) configure the network policy. Embedders are supposed to have their own documentation on how to do it. https://flutter.dev/docs/release/breaking-changes/network-policy-ios-android

[mraleph]

That was intentional. The environment defines were created solely for testing and are not meant to be public API.

That's the reason I started looking for documentation. If the library (in this case gRPC) wants to add a test that it correctly respects isInsecureConnectionAllowed it needs to be able to set the policies somehow, which means library author should be able to find the documentation on how to configure things (if only for tests).

@lrhn Lasse, do you have any suggestions on how to document that? Is that okay just to include description of dart.library.io.domain_network_policies|may_insecurely_connect_to_all_domains into the public doc comment with a remark that they are intended for testing purposes only and actual configuration is embedder specific?

[lrhn]

I'd prefer not to expose that in public API docs.

I'd perhaps put it as non-DartDoc comments in the file which declares the embedder config class, because then the embedder has a chance to see it.

(I'd also be completely fine with not exposing any "for testing only" hooks at all. More than fine! If the embedder needs to test something, they can build a special binary for it. It's not something normal users should ever need to have access to.)

[mraleph]

(I'd also be completely fine with not exposing any "for testing only" hooks at all. More than fine! If the embedder needs to test something, they can build a special binary for it. It's not something normal users should ever need to have access to.)

Note this has nothing to do with embedders testing. Dart packages need to have a way to test that they respect isInsecureConnectionAllowed correctly in an embedder agnostic way e.g. by running pub test which uses default (dart binary) embedder. An example of such library is gRPC. You can't require gRPC to build a custom dart binary just to verify that isInsecureConnectionAllowed is checked in the right places.

Of course you could require that any library that uses isInsecureConnectionAllowed builds an additional layer of abstraction over it for testing purposes - but that also sounds strange to me given that policy is configurable.

[lrhn]

ACK, so users of the feature do need to know about the testing part, because they're the ones using it.
No great suggestions then. And it's in dart:io, so there is just the one library.

In that case, I'd probably document it in the isInsecureConnectionAllowed docs, at the very end with a comment like

For testing only, packages which depend on checking whether insecure connections are allowed,
setting the compile-time environment keys dart.library.io.domain_network_policies or
dart.library.io.may_insecurely_connect_to_all_domains can be used to override the platform
behavior. The format is ....

(whatever the format is, I'm guessing true of the latter and a more complicated format for the former.

[mehmetf]

FWIW, I would provide a mockable abstraction over isInsecureConnectionAllowed for testing purposes at a random Dart package level rather than futzing with env variables.

EDIT: I just saw your edit @mraleph :) I don't think this is strange. It is a standard way of testing endpoints you do not own. The configurability of network policy is hidden on purpose and is exposed to embedders only. In fact, we never wanted this policy to be configurable at runtime. I just didn't have another way of properly testing it. The intention was to translate the platform configuration at build time into a private configuration format that Dart understands.

[Mereep]

Just want to add on this:

I switched for testing purposes to the "beta" channel, which completely skrew my App because of throwing me a StateException when trying to connect to my local machines' API. Switching back to "stable" returned to a working state again. I guess this will be a breaking change for a huge user base. Trying to figure out whats happening suddenly, I searched through the SDK and eventually came out here after finding isInsecureConnectionAllowed somewhere deep inside the woods.

IMO this whole thing will unnecessarily complicate initial development cycles (since it does not even respect a tunneling connection to the local machine as "secure").

Even if Android and iOS did change this policy on their own, why Flutter has to do the same? Who you gonna "protect" with that? It breaks and on top it is not consistent between platforms.

Please make this at least consistent. Users do not want to fiddle within multiple platform dependent configuration files to configure something which has to work under all platforms identically as partially described here: https://flutter.dev/docs/release/breaking-changes/network-policy-ios-android

I really don't understand a policy which pushes a breaking change like that at all. This is completely against "cross platform development".

To sum up:

• Please make this work consistent between all platforms (why it does not affect web, desktop, ...). Aren't we talking about cross platform development?
• Please do make this uniformly configurable (i.e., I don't want my iOS app be able to "insecurely" access non-https while my android app crashes at the same time my desktop or webapp doesn't care at all.)
• Please, at the very least check if the connection is accessing the local machine (tunnel set up etc.). If I do such a thing, you may assume I know what I am doing.

tbh, I would revert this change completely.

[mraleph]

@Mereep If there is something wrong with breaking change description (https://flutter.dev/docs/release/breaking-changes/network-policy-ios-android) then you should file a Flutter issue. Dart's default behaviour did not actually change, so the change is not breaking for Dart itself.

There is a protocol for breaking changes (there are actually two - one for Dart and one for Flutter) and it was followed in this particular case.

[Mereep]

@Mereep If there is something wrong with breaking change description (https://flutter.dev/docs/release/breaking-changes/network-policy-ios-android) then you should file a Flutter issue. Dart's default behaviour did not actually change, so the change is not breaking for Dart itself.

There is a protocol for breaking changes (there are actually two - one for Dart and one for Flutter) and it was followed in this particular case.

yes, you're right. I'm sorry. That seems the wrong place for that issue. I just came out here when trying to dig into the problem and honestly just was a bit ragy for the next thing which broke just silently w/o any deprecation period and dropped it straight here. I think I will open a discussion on flutter on that matter, since I think my arguments still have their point.