Repository navigation
[flutter_tools] Validate plugin class/package identifiers to prevent GeneratedPluginRegistrant injection - #189156
Conversation
Plugin pluginClass/dartPluginClass and the Android package are interpolated verbatim into the generated GeneratedPluginRegistrant source files (the mustache renderer is invoked with htmlEscapeValues defaulting to false). The per-platform validate() checks only verified these fields were strings, not that they were valid identifiers, so a (possibly transitive) dependency could declare a pluginClass/package containing arbitrary source that lands in the consuming app's GeneratedPluginRegistrant and is compiled into the app. Restrict pluginClass, dartPluginClass and the Android package to identifier characters in each platform's validate(), rejecting the plugin specification otherwise. Adds a regression test.
There was a problem hiding this comment.
Code Review
This pull request introduces validation for plugin identifiers across multiple platforms to prevent arbitrary native code injection, along with a corresponding unit test. The feedback recommends updating the documentation comments for the newly added private members to use triple slashes (///) instead of double slashes (//), in accordance with the Flutter Repository Style Guide.
| // Matches a valid native plugin class or dot-separated package identifier. | ||
| // | ||
| // Plugin `class`/`package` values are interpolated verbatim into the generated | ||
| // GeneratedPluginRegistrant source files (Java/Kotlin, Swift, Objective-C, | ||
| // C++). Restricting them to identifier characters prevents a (possibly | ||
| // transitive) dependency from injecting arbitrary native code into the | ||
| // consuming app's build via its pubspec plugin declaration. | ||
| final RegExp _pluginIdentifierPattern = RegExp( | ||
| r'^[a-zA-Z_$][a-zA-Z0-9_$]*(\.[a-zA-Z_$][a-zA-Z0-9_$]*)*$', | ||
| ); | ||
|
|
||
| // Returns false only when [value] is a String that is not a valid identifier. | ||
| // Absent or non-String values are left to the existing schema type checks. | ||
| bool _isValidPluginIdentifier(Object? value) => | ||
| value is! String || _pluginIdentifierPattern.hasMatch(value); |
There was a problem hiding this comment.
According to the Flutter Repository Style Guide, public-quality documentation should use /// even on private members. Please update the comments for _pluginIdentifierPattern and _isValidPluginIdentifier to use triple slashes (///).
| // Matches a valid native plugin class or dot-separated package identifier. | |
| // | |
| // Plugin `class`/`package` values are interpolated verbatim into the generated | |
| // GeneratedPluginRegistrant source files (Java/Kotlin, Swift, Objective-C, | |
| // C++). Restricting them to identifier characters prevents a (possibly | |
| // transitive) dependency from injecting arbitrary native code into the | |
| // consuming app's build via its pubspec plugin declaration. | |
| final RegExp _pluginIdentifierPattern = RegExp( | |
| r'^[a-zA-Z_$][a-zA-Z0-9_$]*(\.[a-zA-Z_$][a-zA-Z0-9_$]*)*$', | |
| ); | |
| // Returns false only when [value] is a String that is not a valid identifier. | |
| // Absent or non-String values are left to the existing schema type checks. | |
| bool _isValidPluginIdentifier(Object? value) => | |
| value is! String || _pluginIdentifierPattern.hasMatch(value); | |
| /// Matches a valid native plugin class or dot-separated package identifier. | |
| /// | |
| /// Plugin class/package values are interpolated verbatim into the generated | |
| /// GeneratedPluginRegistrant source files (Java/Kotlin, Swift, Objective-C, | |
| /// C++). Restricting them to identifier characters prevents a (possibly | |
| /// transitive) dependency from injecting arbitrary native code into the | |
| /// consuming app's build via its pubspec plugin declaration. | |
| final RegExp _pluginIdentifierPattern = RegExp( | |
| r'^[a-zA-Z_$][a-zA-Z0-9_$]*(\.[a-zA-Z_$][a-zA-Z0-9_$]*)*$', | |
| ); | |
| /// Returns false only when [value] is a String that is not a valid identifier. | |
| /// Absent or non-String values are left to the existing schema type checks. | |
| bool _isValidPluginIdentifier(Object? value) => | |
| value is! String || _pluginIdentifierPattern.hasMatch(value); |
References
- Use
///for public-quality documentation, even on private members. (link)
Extend the plugin-identifier validation to the remaining registrant interpolation sites: the web GeneratedPluginRegistrant (pluginClass and fileName in WebPlugin.fromYaml) and the Dart plugin registrant import (dartFileName), which are otherwise written verbatim into generated Dart source. fileName/dartFileName are restricted to a safe relative .dart path. Adds regression tests.
| /// Returns false only when [value] is a String that is not a valid identifier. | ||
| /// Absent or non-String values are left to the existing schema type checks. | ||
| bool _isValidPluginIdentifier(Object? value) => | ||
| value is! String || _pluginIdentifierPattern.hasMatch(value); |
There was a problem hiding this comment.
It is weird that _isValidPluginIdentifier(123) returns true.
I think changing this function to bool _isValidPluginIdentifier(String value), and handle the typing issue at the call site would make the intention clearer.
Same for isValidPluginDartFileName below
| if (yaml['fileName'] is! String) { | ||
| throwToolExit('The plugin `$name` is missing the required field `fileName` in pubspec.yaml'); | ||
| } | ||
| if (!_isValidPluginIdentifier(yaml['pluginClass'])) { |
There was a problem hiding this comment.
Use kPluginClass instead of hardcoded 'pluginClass'
| if (!_isValidPluginIdentifier(yaml['pluginClass'])) { | ||
| throwToolExit('The plugin `$name` has an invalid `pluginClass` in its web plugin declaration.'); | ||
| } | ||
| if (!isValidPluginDartFileName(yaml['fileName'])) { |
There was a problem hiding this comment.
While we're at this, maybe create a new constant for fileName since it is used in more than a few places now.
| yaml[kDartPluginClass] is String || | ||
| yaml[kFfiPlugin] == true || | ||
| yaml[kDefaultPackage] is String; | ||
| return ((yaml['package'] is String && yaml[kPluginClass] is String) || |
There was a problem hiding this comment.
After changing _isValidPluginIdentifier to accept only a String type, this part can be changed to something like below to take advantage of automatic type casting.
final Object? package = yaml['package'];
final Object? pluginClass = yaml[kPluginClass];
...
return (
package is String && _isValidPluginIdentifier(package) &&
pluginClass is String && _isValidPluginIdentifier(pluginClass)) || ...| ); | ||
| }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Add a test case for kDartPluginClass containing injection.
- Narrow _isValidPluginIdentifier and isValidPluginDartFileName to take a non-null String and match only. Callers now promote the value through the existing schema type checks, so a non-String no longer implicitly passes. - Use the kPluginClass constant and a new kFileName constant in WebPlugin.fromYaml instead of hardcoded key strings. - Add a test that a dartPluginClass containing injection characters is rejected.
|
Thanks for the review! Addressed in the latest push:
On the |
| final Object? package = yaml['package']; | ||
| final Object? pluginClass = yaml[kPluginClass]; | ||
| final Object? dartPluginClass = yaml[kDartPluginClass]; | ||
| return ((package is String && pluginClass is String) || |
There was a problem hiding this comment.
This boolean expression is getting too long and is not readable. It's basically doing two things at once, let's split those things.
What do you think if we do something like:
static bool validate(YamlMap yaml) {
final Object? package = yaml['package'];
final Object? pluginClass = yaml[kPluginClass];
final Object? dartPluginClass = yaml[kDartPluginClass];
final hasPluginDeclaration =
(package is String && pluginClass is String) ||
dartPluginClass is String ||
yaml[kFfiPlugin] == true ||
yaml[kDefaultPackage] is String;
if (!hasPluginDeclaration) {
return false;
}
// Validate values provided.
if (package is String && !_isValidPluginIdentifier(package)) {
return false;
}
if (pluginClass is String && !_isValidPluginIdentifier(pluginClass)) {
return false;
}
if (dartPluginClass is String && !_isValidPluginIdentifier(dartPluginClass)) {
return false;
}
return true;
}Same for all the validate() functions below.
There was a problem hiding this comment.
Split each platform's validate() into a hasPluginDeclaration check followed by per-identifier validation, as suggested — applied to Android, iOS, macOS, Windows and Linux. WebPlugin.fromYaml already used early returns.
Every identifier that is present is still validated regardless of which declaration form matched, so an unsafe pluginClass alongside ffiPlugin/default_package remains rejected.
One tweak vs. the snippet: final bool hasPluginDeclaration for specify_nonobvious_local_variable_types.
… identifier checks The combined boolean expression did two things at once: deciding whether the platform block is a plugin declaration at all, and validating the identifiers it carries. Split them so each step reads on its own, and keep validating every identifier that is present rather than only the ones on the matched declaration branch.
chingjun
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix!
|
autosubmit label was removed for flutter/flutter/189156, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
… comment The repo style check rejects "simply" in documentation strings, which failed the Linux analyze presubmit on this branch.
|
Pushed 8722786 to clear the The failure was a repo style check, not a test: The new commit drops that one word from the On the remaining red checks, in case it saves someone the triage: I do not think they are from this PR.
@chingjun @bkonyi could I trouble one of you for a re-approval when you have a moment? Happy to rebase if the tree-side Android failures need a fresher base first. |
|
autosubmit label was removed for flutter/flutter/189156, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
…acyYaml flutter#189156 restricts pluginClass/dartPluginClass/package to identifiers in each platform's fromYaml, but the legacy plugin format is parsed by Plugin._fromLegacyYaml, which builds AndroidPlugin/IOSPlugin through their plain constructors and never reaches that validation. A legacy-format pubspec whose androidPackage, pluginClass or iosPrefix contains injection characters therefore still lands verbatim in the generated GeneratedPluginRegistrant and is compiled into any app that (transitively) depends on the plugin. Reuse the existing identifier check via a small validatePluginIdentifier helper and call it for pluginClass, androidPackage and iosPrefix in _fromLegacyYaml. Adds three regression tests driving the full findPlugins pipeline with a legacy-format pubspec for each field.
flutter#189156 restricts pluginClass/dartPluginClass/package to identifiers in each platform's validate(), but the legacy plugin format is parsed by Plugin._fromLegacyYaml, which builds AndroidPlugin/IOSPlugin through their plain constructors and never reaches that validation. A legacy-format pubspec whose androidPackage, pluginClass or iosPrefix contains injection characters therefore still lands verbatim in the generated GeneratedPluginRegistrant and is compiled into any app that (transitively) depends on the plugin. Follow the pattern flutter#189156 established: check the identifiers in _validateLegacyYaml alongside the existing type checks, so every invalid field is reported at once through Plugin.fromYaml's error list rather than exiting on the first one. This only requires making the existing identifier predicate visible outside platform_plugins.dart, matching isValidPluginDartFileName. Adds five tests covering each field's injection payload, the combined multiple-error output and an unchanged valid legacy declaration.
… prevent GeneratedPluginRegistrant injection (#191294) This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request. ### Issue Link: #189156 ### Impact Description: Plugin `pluginClass`, `dartPluginClass`, and Android `package` fields are interpolated verbatim into generated `GeneratedPluginRegistrant` native source files (Swift, Objective-C, Java, Kotlin, C++). Previously, validation only checked that these fields were strings, allowing transitive dependencies with malicious or malformed plugin identifiers to inject arbitrary code into `GeneratedPluginRegistrant` during `flutter pub get` or `flutter build`. ### Changelog Description: [flutter/189156] Validate plugin class and package identifiers to prevent arbitrary code injection into GeneratedPluginRegistrant. ### Workaround: Inspect transitive dependency `pubspec.yaml` plugin configurations manually. ### Risk: ### Test Coverage: ### Validation Steps: 1. Test a plugin declaration containing non-identifier characters in `pluginClass` or `package`. 2. Confirm that the tool rejects the invalid plugin specification. 3. Run `packages/flutter_tools/test/general.shard/plugins_test.dart`.
Plugin fields are interpolated verbatim into the generated registrants, so restrict them to identifier characters and plain file names. Replace the assert, which is stripped in the snapshot, with a tool exit. flutter/flutter#189156
Flutter 3.47.1 validates plugin identifiers to prevent code injection into the generated registrants, but not the `tizen` block that flutter-tizen reads. Apply the same rules here and replace the assert, which is stripped in the snapshot, with a tool exit. flutter/flutter#189156
Description
Plugin
pluginClass/dartPluginClassand the Androidpackageare interpolated verbatim into the generatedGeneratedPluginRegistrantsource files.flutter_plugins.dartrenders these templates via_renderTemplateToFile→templateRenderer.renderString(template, context), and the renderer'shtmlEscapeValuesdefaults tofalse, so the values are emitted with no escaping into Java/Kotlin, Swift, Objective-C and C++ source (e.g.new {{package}}.{{class}}(),{{prefix}}{{class}}.register(...),#import <{{name}}/{{class}}.h>).The per-platform
validate()methods inplatform_plugins.dartonly checked that these fields were strings, not that they were valid identifiers. As a result a plugin declaration whosepluginClass/packagecontains arbitrary source (spaces,;,{},(), newlines, …) passes validation and that source lands in the consuming app'sGeneratedPluginRegistrantand is compiled into the app.Because plugins are collected over
computeTransitiveDependencies(...)with no opt-in from the consuming app, a transitive dependency can use this to have arbitrary native code compiled into an app that merely depends on it (via a plainflutter pub get/build/run). This is the same "a package must not escape its declared boundary at build time" boundary enforced for asset paths in #187661 and for pub-cache extraction in CVE-2026-27704.Reproduced end-to-end: a dependency declaring
resulted, after
flutter pub get, in the injected statements appearing verbatim inmacos/Flutter/GeneratedPluginRegistrant.swiftandios/Runner/GeneratedPluginRegistrant.m.Fix
Restrict
pluginClass,dartPluginClassand the Androidpackageto identifier characters (dot-separated identifiers) in each platform'svalidate(), rejecting the plugin specification otherwise. Legitimate class/package names are unaffected; a value that is not a plain identifier now fails withInvalid plugin specification <name>.Tests
pluginClasscontaining injection characters is rejected.test/general.shard/plugins_test.dartpasses (77/77) — no legitimate plugin specification regresses.Pre-launch Checklist