Skip to content
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

A way to restrict the allowed hosts for dependencies. #4328

Open
4 tasks
matanlurey opened this issue Jul 17, 2024 · 5 comments
Open
4 tasks

A way to restrict the allowed hosts for dependencies. #4328

matanlurey opened this issue Jul 17, 2024 · 5 comments
Labels
type-enhancement A request for a change that isn't a bug

Comments

@matanlurey
Copy link
Contributor

matanlurey commented Jul 17, 2024

Context: flutter/engine prohibits dependencies from pub, requiring all packages to come from gclient sync (i.e. our Chromium version of git submodules), so we go out of our way to make sure that, given a repo xyz, package xyx/packages/foo doesn't start depending on say, package:archive without adding it to xyz/DEPS.

In flutter/engine#53539 (comment), @zanderso asked:

For my understanding, is it possible for a pubspec that uses the workspace to add a dependency on a package that isn't in the workspace? Like can a workspace-using pubspec add a new dependency and pull a package from pub that isn't declared in the workspace?

It looks like the answer was surprisingly yes:

# Declare all packages that are part of the workspace.
workspace:
  - tools/engine_tool

dependencies:
  args: any

dependency_overrides:
  args:
    path: ./third_party/dart/third_party/pkg/args

And then later, in tools/engine_tool/pubspec.yaml:

name: engine_tool

# This package is managed as part of the engine workspace.
resolution: workspace

dependencies:
  archive: any
  args: any

I expected this to fail, or warn, or do something. Here is the result of dart pub get:

Resolving dependencies in `/Users/matanl/Developer/engine/src/flutter`... 
Downloading packages... 
! args 2.5.1-wip from path ../../third_party/dart/third_party/pkg/args (overridden)
! async 2.12.0-wip from path ../../third_party/dart/third_party/pkg/async (overridden)
! async_helper 0.0.0 from path ../../third_party/dart/pkg/async_helper (overridden)
! collection 1.19.0 from path ../../third_party/dart/third_party/pkg/collection (overridden)
! engine_build_configs 0.0.0 from path ../pkg/engine_build_configs (overridden)
! engine_repo_tools 0.0.0 from path ../pkg/engine_repo_tools (overridden)
! expect 0.0.0 from path ../../third_party/dart/pkg/expect (overridden)
! file 7.0.1-dev from path ../../third_party/dart/third_party/pkg/file/packages/file (overridden)
! litetest 0.0.0 from path ../../testing/litetest (overridden)
! logging 1.3.0-wip from path ../../third_party/dart/third_party/pkg/logging (overridden)
! meta 1.16.0-dev from path ../../third_party/dart/pkg/meta (overridden)
! path 1.9.1-wip from path ../../third_party/dart/third_party/pkg/path (overridden)
! platform 3.1.0 from path ../../third_party/pkg/platform (overridden)
! process 4.2.1 from path ../../third_party/pkg/process (overridden)
! process_fakes 0.0.0 from path ../pkg/process_fakes (overridden)
! process_runner 4.1.3 from path ../../third_party/pkg/process_runner (overridden)
! smith 0.0.0 from path ../../third_party/dart/pkg/smith (overridden)
! source_span 1.10.1-wip from path ../../third_party/dart/third_party/pkg/source_span (overridden)
! string_scanner 1.3.0 from path ../../third_party/dart/third_party/pkg/string_scanner (overridden)
! term_glyph 1.2.2-wip from path ../../third_party/dart/third_party/pkg/term_glyph (overridden)
! yaml 3.1.3-wip from path ../../third_party/dart/third_party/pkg/yaml (overridden)
Got dependencies in `/Users/matanl/Developer/engine/src/flutter`!

Note that archive is omitted, and not mentioned. However, if I look at the root .dart_tool/package_config.json:

{
  "configVersion": 2,
  "packages": [
    {
      "name": "archive",
      "rootUri": "file:///Users/matanl/.pub-cache/hosted/pub.dev/archive-3.6.1",
      "packageUri": "lib/",
      "languageVersion": "3.0"
    },
    {
      "name": "args",
      "rootUri": "../third_party/dart/third_party/pkg/args",
      "packageUri": "lib/",
      "languageVersion": "3.3"
    }
}

... and I can indeed import and use package:archive in engine_tool. This is not a blocker for us as we have that custom script that checks that packages aren't resolved to pub, but it was surprising and silent behavior - I'd love to see some mechanism to get "protection" or at least notice that this is happening.


Possible suggestions for improvement:

  • Clarify how the tool is intended to work to make sure the above is expected
  • Ensure package:archive comes up in dart pub get and similar, perhaps with a (not from workspace) moniker?
  • Allow the root pubspec to have something like workspace_options: { "dependency_sources": "workspace_only" }
  • Add a lint, pub_workspace_single_source_of_dependencies that can be opted-in to

/cc @kevmoo @jtmcdole

@jonasfj
Copy link
Member

jonasfj commented Jul 17, 2024

Dependencies from workspace-only is likely a niche use case.

When doing things like this you are effectively saying that you don't want to use the version resolution logic that pub provides.
Instead you're just using pub to generate a package_config.json and get a development experience that is somewhat similar to normal app developers who gets packages from pub.dev.


We could certainly add things to pubspec.yaml that would make setups like this nicer. Like an allowed_sources as a list of git-repos, pub-servers and paths that are allowed as package sources in the resolution of a pubspec (only when the pubspec is a root, like how dev-dependencies work).

The allowed_sources idea above is just a quick of the top of my head. I think modt people won't need it. And I think that advanced users could solve by just writing a test case that reads pubspec.lock to check where dependencies comes from.

I think a feature like allowed_sources will be relevant when/if we add support for SLSA in pub. Then you might want to specify that you only want dependencies thet satisfy certain constraints like:

  • An allowed list of publishers
  • Requirement for SLSA level 2, 3 or ?,
  • Enforcement that no version may be affected by a security vulnerability.
  • An allow list of hosted repositories
  • Packages must be published using automated publishing... Etc..

I think that once we have SLSA support on pub.dev we should spend time designing a set of policies to be enforced on dependencies. I guess workspace feature is a good mechanism for enforcing such policies across a mono-repository.

TL;DR: Some sort of policy enforcement on dependencies would be cool, but maybe we shouldn't design it until we have SLSA. In the meantime people can enforce policies with a test case.


One could also imagine something like hook/preresolve.dart, a script that is called before pub get resolve. That could then trigger gclient sync. But that might make pub get so slow that you can't run it from an IDE whenever you modify a relevant file.

@matanlurey
Copy link
Contributor Author

To be clear, the main surprise was that it silently works and doesn't even show up in the pub get list.

@jtmcdole
Copy link

I think we just want a failure at pub get with package {package referenced} not defined at workspace pubspec.yaml.

@jakemac53
Copy link
Contributor

Dependencies from workspace-only is likely a niche use case.

I do actually think that for larger companies this might not be a niche use case. It is probably not all that uncommon to want to have some higher level control over exactly what dependencies are allowed, where they come from, and exactly what version a project is on.

@sigurdm sigurdm added the type-enhancement A request for a change that isn't a bug label Aug 1, 2024
@sigurdm
Copy link
Contributor

sigurdm commented Aug 1, 2024

I like the idea of being able to restrict dependencies to a number of select hosts.
I guess that should only take effect in the root pubspec...

allowed_host_urls:
  - https://pub.dev
  - https://my-pub-server.com

and for the flutter engine you would provide an empty list:

allowed_host_urls: [] # don't allow any hosted packages

Maybe jonas is right and we should also allow for specifying allowed publishers, salsa levels etc. But I think these are somewhat orthogonal, and could be added separately later...

@sigurdm sigurdm changed the title Pub Workspaces: Adding a non-workspace dependency in a package pubspec silently succeeds A way to restrict the allowed hosts for dependencies. Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants