flutter_tools: validate plugin identifiers before generating registrant code - #190462

Hi @herdiyana256! Thanks for the contribution. Is there an issue associated with the problem this PR is solving?

Hi bkonyi,

No public issue for this one on purpose -- it's a supply-chain code-injection bug (a malicious pub.dev package's pluginClass/dartFileName gets compiled and executed inside every app that depends on it), so I didn't want to file a public issue spelling out the exploit before there's a fix in place.

Happy to route it through Flutter's security reporting channel if you'd prefer that over discussing further in the open PR -- let me know.

Update: filed the formal report through Google's official vulnerability disclosure channel.

Google IssueTracker: https://issuetracker.google.com/issues/542989102

Good catch, thanks. You're right, _fromLegacyYaml builds AndroidPlugin/IOSPlugin through their plain constructors, not .fromYaml, so none of the validation in this PR was reached for the legacy format.

Fixed in d98ace8 by calling validatePluginIdentifier directly in _fromLegacyYaml for pluginClass, androidPackage, and iosPrefix (that last one wasn't in your example but it also gets interpolated raw as {{prefix}}{{class}} in the iOS template, so same issue). Added three tests in plugins_test.dart covering all three fields through the actual findPlugins pipeline with a legacy-format pubspec, plus the hyphen fix from your other comment.

Heads up: I've rebased and significantly narrowed this PR.

The core fix here (restricting pluginClass/dartPluginClass/package, plus the WebPlugin and dartFileName cases, in every platform's fromYaml) has since landed on master via #189156, so I've dropped all of it to avoid duplicating merged validation.

What's left is the one vector that fix doesn't reach: the legacy plugin format @bkonyi flagged. Plugin._fromLegacyYaml builds AndroidPlugin/IOSPlugin through their plain constructors, bypassing the fromYaml validation, so a legacy-format pubspec with an injected androidPackage / pluginClass / iosPrefix still lands verbatim in the generated registrant on current master. This PR closes that gap: a small validatePluginIdentifier helper reused for all three fields in _fromLegacyYaml, plus three regression tests through the full findPlugins pipeline.

@chingjun the read-fields-via-helper suggestion targets the fromYaml reads, which are now merged and out of scope here; this PR only touches the legacy path and mirrors the existing check. Happy to follow up on that ergonomics separately.

Thanks for pointing out #189156, I thought this looks familiar :)

In that case, can you follow the pattern established in #189156? i.e. check that in the validate() function. Having two different ways to achieve the same thing is confusing to future maintainers.

The existing mechanism with Plugin.fromYaml() will actually show multiple errors at once if the validate returns an invalid result (even though the existing validate() function does not take advantage of that, that should be improved), but throwing a tool exit when that is used will only surface one error at a time.

Done, moved the check into _validateLegacyYaml in 419defa so it follows the pattern from #189156: the legacy identifiers are validated alongside the existing type checks and every invalid field is reported at once through Plugin.fromYaml's error list, instead of exiting on the first one. Nothing throws from _fromLegacyYaml anymore, so both plugin formats now fail the same way.

The only other production change is making the existing predicate non-private (_isValidPluginIdentifier -> isValidPluginIdentifier), so plugins.dart can call it the same way it already calls isValidPluginDartFileName. iosPrefix is skipped when empty, since an empty prefix is the default for a legacy plugin that does not set one.

Tests moved next to the existing injection tests in the plugin-parsing group and now go through Plugin.fromYaml directly, matching #189156: one per field (pluginClass, androidPackage, iosPrefix), one asserting all three errors are surfaced together, and one confirming a valid legacy declaration still parses. plugins_test.dart (84), plugin_parsing_test.dart (20), dart_plugin_test.dart (25), android_plugin_test.dart (7), flutter_manifest_test.dart (70) and project_test.dart (116) all pass, and analyzer/formatter are clean.

@bkonyi sorry for dismissing your approval again with the force-push, PTAL when you get a chance.

autosubmit label was removed for flutter/flutter/190462, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

• Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a member of flutter-hackers before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

autosubmit label was removed for flutter/flutter/190462, because - The status or check suite ci.yaml validation has failed. Please fix the issues identified (or deflake) before re-applying this label.

autosubmit label was removed for flutter/flutter/190462, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

Both remaining red checks look unrelated to this change, so I don't think there is anything to fix here.

Dashboard Checks is red on two Mac tool_integration_tests subshards, neither of which touches plugin parsing:

• Mac tool_integration_tests_7 (build 8673724692222606833): debug_adapter/test_adapter_test.dart: widget tests can run in debug mode hit the 15 minute test timeout. The exact same test timed out the same way on an unrelated commit in build 8673722276765844945, and the same shard failed on several other unrelated shas the same day on the neighbouring integration tests can run ... cases (builds 8673734273458043745, 8673742889578939073).
• Mac tool_integration_tests_4 (build 8673724692440436817): gen_l10n_test.dart: generated l10n classes produce expected localized strings failed with Exception: Received app.stop event while waiting for app.started event, i.e. flutter-tester never came up. The same shard failed on unrelated shas the same day on swift_package_manager_create_plugin_test.dart and lldb_init_test.dart, and once on this same gen_l10n_test case with a missing dartaotruntime in the search path.

Across the last 60 try builds of each, those two shards are at 8/60 and 10/60 failures on a wide spread of commits.

Google testing ("Internal smoke tests failed") went red on at least six other PRs in the same window: #190935, #190667, #190929, #190014, #189500 and #185430, all of which have since merged.

For what it's worth, the diff only adds string checks inside Plugin._validateLegacyYaml, so it has no path to the l10n generator or the DAP adapter. Every tool_tests_general shard (Linux, Mac, Windows), which is where plugins_test.dart runs, is green on a57d2e0, as are the other 22 tool integration shards.

@bkonyi could you re-apply autosubmit when you get a chance? I can't set the label myself, and I'd rather not push a no-op commit since that dismisses both approvals again.

Correction to my previous comment, and an update.

The argument I made about Google testing there was weak and I want to withdraw it. I said the check had gone red on six other PRs that have since merged. That is true only in the sense that it went red at some point in each of them; every one of those six ended up green on its final head before it landed. So it showed nothing, and I should have checked that before posting it.

The actual evidence is better than what I offered, and it points at the master sync rather than at this PR:

a57d2e0 is purely the merge commit, so the diff under test is byte for byte identical in both runs. The same content passed at 19:39 and failed at 23:05. The only variable is the 87 commits that came in with the sync, which include four Dart SDK rolls and #190480's move of examples/api to packages/flutter/examples/api. Whatever broke has since cleared: #190951 and #190960 both got a green Google testing after that window.

I have synced master again, so this is now on 9c298d7 and everything is re-running. Both approvals carried over, since a merge from base does not dismiss them the way my earlier force-pushes did.

The Dashboard Checks analysis in my previous comment still stands as written: Mac tool_integration_tests_4 and _7 failed on gen_l10n_test.dart and debug_adapter/test_adapter_test.dart, neither reachable from a change to Plugin._validateLegacyYaml, and both shards fail at roughly 8/60 and 10/60 across unrelated commits. The re-run should give those a fresh roll of the dice too.

@bkonyi the CICD label came off automatically with the new commit, so Dashboard Checks is sitting behind Awaiting CICD label. Could you re-add it, and re-apply autosubmit if things come back green?

On your nit: I deliberately kept this push a pure merge from base so the approvals would survive, which meant not touching the comment in the same commit. Happy to drop it as a follow-up, or now if you would rather have it gone before this lands and do not mind re-approving.

Google testing came back green on 9c298d7 (successful in 22m), which confirms the diagnosis above: the same diff that failed on a57d2e0 passes once master is re-synced, so nothing in this PR was ever the cause.

@bkonyi three things need a maintainer click before this can land, none of which I can do:

1. Approve the 3 pending workflow runs (Mac Verify Binaries, Tree Analyze, No PRs on main), which are held because this is a fork PR.
2. Add the CICD label. It came off automatically with the new commit, and Dashboard Checks has not started at all yet, it is still sitting on Awaiting CICD label.
3. Re-apply autosubmit once the presubmit comes back green.

No rush on my end, whenever you get a moment.