diff --git a/cloud-run/ffai-runner/Dockerfile b/cloud-run/ffai-runner/Dockerfile index 2a14587..422b653 100644 --- a/cloud-run/ffai-runner/Dockerfile +++ b/cloud-run/ffai-runner/Dockerfile @@ -33,6 +33,7 @@ RUN dart pub get RUN mkdir -p /workspace COPY bin/ ./bin/ +COPY lib/ ./lib/ EXPOSE 8080 diff --git a/cloud-run/ffai-runner/bin/server.dart b/cloud-run/ffai-runner/bin/server.dart index c33a5fa..be4b67a 100644 --- a/cloud-run/ffai-runner/bin/server.dart +++ b/cloud-run/ffai-runner/bin/server.dart @@ -2,32 +2,12 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; -const maxClassesPerRequest = 20; -const maxCodeBytes = 500000; -const maxDependenciesPerRequest = 200; -// pub.dev's own ceiling on a package name. -const maxPackageNameLength = 64; - -const analysisPackageName = 'ccc_custom_code_analysis'; -const defaultSdkConstraint = '>=3.0.0 <4.0.0'; - -// Pub package names are lower-case identifiers. Matching the whole string is -// what keeps a caller from smuggling YAML structure in through a key. -final _packageNamePattern = RegExp(r'^[a-z_][a-z0-9_]*$'); - -// A pub version constraint, and nothing else. The allowed characters exclude -// `:`, `#`, `{`, `}`, `/` and whitespace beyond single spaces, so `git:`, -// `path:`, a nested `hosted:` mapping, and comment injection are all -// unrepresentable rather than merely discouraged. -final _constraintPattern = RegExp(r'^[A-Za-z0-9^~<>=*+._ -]+$'); - -// Which packages the Flutter SDK supplies is not decided here. A caller names -// the packages its project declares as `sdk: flutter`, and this runner emits -// them as `name: {sdk: flutter}` - a value it writes itself, never one a caller -// sent. There was an allowlist of SDK package names here, and it refused good -// deploys: the client classified a package correctly by its pubspec source, and -// the runner rejected it for not appearing on a list that had gone stale. -// `_packageNamePattern` is what actually keeps a key from carrying YAML. +import 'package:ccc_ffai_runner/request_validation.dart'; + +// The lowest flutterflow_cli the vendored FlutterFlow AI snapshot accepts. The +// image pins FLUTTERFLOW_CLI_VERSION to this (see Dockerfile); it must be kept +// in lockstep there, and it is what /healthz reports as the required minimum. +const minCliVersion = '0.0.41'; Future main() async { final port = int.tryParse(Platform.environment['PORT'] ?? '') ?? 8080; @@ -50,7 +30,13 @@ Future _handle(HttpRequest request) async { return; } - if (request.method != 'POST' || request.uri.path != '/deployCustomClasses') { + if (request.method == 'GET' && request.uri.path == '/healthz') { + await _writeJsonBody(request.response, await _healthz()); + return; + } + + if (request.method != 'POST' || + request.uri.path != '/deployCustomClasses') { await channel.result(HttpStatus.notFound, { 'success': false, 'error': 'Not found.', @@ -70,13 +56,18 @@ Future _handle(HttpRequest request) async { String commitMessage; bool dryRun; List classes; - _VerificationRequest? verification; + VerificationRequest? verification; try { payload = await _readJson(request); - apiKey = _stringField(payload, 'apiKey', maxLength: 10000); - projectId = _stringField(payload, 'projectId', maxLength: 200); - baseUrl = _stringField(payload, 'baseUrl', maxLength: 500, required: false); - commitMessage = _stringField( + apiKey = stringField(payload, 'apiKey', maxLength: 10000); + projectId = stringField(payload, 'projectId', maxLength: 200); + baseUrl = stringField( + payload, + 'baseUrl', + maxLength: 500, + required: false, + ); + commitMessage = stringField( payload, 'commitMessage', maxLength: 300, @@ -84,7 +75,7 @@ Future _handle(HttpRequest request) async { ); dryRun = payload['dryRun'] == true; classes = _normalizeClasses(payload['customClasses']); - verification = _normalizeVerification(payload['verification']); + verification = normalizeVerification(payload['verification']); } on FormatException catch (error) { await channel.result(HttpStatus.badRequest, { 'success': false, @@ -103,15 +94,17 @@ Future _handle(HttpRequest request) async { channel.phase('connected', 'Connected to the FlutterFlow deploy runner.'); final workspace = Directory( - Platform.environment['FFAI_WORKSPACE'] ?? '/workspace/custom_code_connect', + Platform.environment['FFAI_WORKSPACE'] ?? + '/workspace/custom_code_connect', ); final initResult = await _ensureWorkspace(workspace, apiKey, channel); if (initResult != null) { await channel.result(HttpStatus.badGateway, { 'success': false, - 'error': initResult.timedOut - ? 'Preparing the FlutterFlow AI workspace timed out.' - : 'FlutterFlow AI workspace initialization failed.', + 'error': + initResult.timedOut + ? 'Preparing the FlutterFlow AI workspace timed out.' + : 'FlutterFlow AI workspace initialization failed.', 'details': _trimOutput(initResult.output), 'exitCode': initResult.exitCode, // Workspace setup precedes any write to FlutterFlow. @@ -129,7 +122,8 @@ Future _handle(HttpRequest request) async { if (analysis != null && analysis.hasErrors) { await channel.result(HttpStatus.unprocessableEntity, { 'success': false, - 'error': 'The generated custom code does not compile, so nothing was ' + 'error': + 'The generated custom code does not compile, so nothing was ' 'deployed to FlutterFlow.', 'details': analysis.report, 'analyzerErrors': analysis.errors, @@ -175,6 +169,7 @@ Future _handle(HttpRequest request) async { workingDirectory: workspace.path, apiKey: apiKey, channel: channel, + timeout: _deployTimeout(classes.length), ); if (result.timedOut) { @@ -202,12 +197,15 @@ Future _handle(HttpRequest request) async { await channel.result(HttpStatus.ok, { 'success': true, 'message': 'Custom classes upserted through FlutterFlow AI DSL.', - 'deployed': classes - .map((entry) => { - 'artifactId': entry.artifactId, - 'className': entry.className, - }) - .toList(), + 'deployed': + classes + .map( + (entry) => { + 'artifactId': entry.artifactId, + 'className': entry.className, + }, + ) + .toList(), 'dryRun': dryRun, 'verified': analysis?.verifiedFiles ?? const [], 'verificationSkipped': analysis?.skippedReason, @@ -231,7 +229,9 @@ Future<_RunOutcome?> _ensureWorkspace( String apiKey, _ResponseChannel channel, ) async { - final packageConfig = File('${workspace.path}/.dart_tool/package_config.json'); + final packageConfig = File( + '${workspace.path}/.dart_tool/package_config.json', + ); if (packageConfig.existsSync()) { channel.phase('workspace_ready', 'Build environment is ready.'); return null; @@ -262,8 +262,22 @@ Future<_RunOutcome?> _ensureWorkspace( return null; } +/// Caps a single deploy invocation, scaling with how many classes it must +/// upload. A fixed cap would have to be sized to the largest bundle, either +/// killing a big one or over-waiting a small one; starting at 5 minutes and +/// growing by 15s per class keeps even the maximum 20-class request at 10 +/// minutes - comfortably under Cloud Run's 900s request timeout and inside the +/// browser's 840s wait, with headroom to spare. +Duration _deployTimeout(int classCount) { + return const Duration(minutes: 5) + Duration(seconds: 15 * classCount); +} + /// Runs the FlutterFlow CLI, forwarding its output line by line so the caller /// can report real progress instead of guessing at it. +/// +/// The [timeout] default is the workspace-init cap: `flutterflow ai init` +/// fetches the SDK snapshot once and is deliberately given a tighter bound +/// than a deploy, which pays for the per-class upload on top. Future<_RunOutcome> _runFlutterFlow( List args, { required String workingDirectory, @@ -275,10 +289,7 @@ Future<_RunOutcome> _runFlutterFlow( 'flutterflow', args, workingDirectory: workingDirectory, - environment: { - 'FF_API_KEY': apiKey, - 'FLUTTERFLOW_API_KEY': apiKey, - }, + environment: {'FF_API_KEY': apiKey, 'FLUTTERFLOW_API_KEY': apiKey}, ); final output = StringBuffer(); @@ -287,6 +298,15 @@ Future<_RunOutcome> _runFlutterFlow( timedOut = true; process.kill(ProcessSignal.sigkill); }); + // A cold start can be silent for a minute or more (SDK download, `flutter + // analyze`), which the caller cannot distinguish from a dead connection. Emit + // a heartbeat every 15s while the CLI runs so liveness stays observable; the + // channel drops it for callers that did not ask for a stream, so it costs a + // non-streaming request nothing. + final heartbeat = Timer.periodic( + const Duration(seconds: 15), + (_) => channel.heartbeat(), + ); void consume(String rawLine) { final line = _redact(rawLine, apiKey).trimRight(); @@ -315,6 +335,7 @@ Future<_RunOutcome> _runFlutterFlow( final exitCode = await process.exitCode; await Future.wait([stdoutDone, stderrDone]); timer.cancel(); + heartbeat.cancel(); return _RunOutcome( exitCode: exitCode, @@ -396,13 +417,22 @@ final class _ResponseChannel { _write({'event': 'log', 'message': message}); } + /// Marks the runner as alive without claiming progress. Existing clients + /// ignore unknown events, so this is invisible to them; it exists so a proxy + /// or the browser can tell a long, silent step from a dead connection. + void heartbeat() { + _write({'event': 'heartbeat'}); + } + void _write(Map event) { if (!_streaming || _closed) return; - _writes = _writes.then((_) { - _response.write('${jsonEncode(event)}\n'); - return _response.flush(); - // A disconnected client must not take down the deploy. - }).catchError((Object _) {}); + _writes = _writes + .then((_) { + _response.write('${jsonEncode(event)}\n'); + return _response.flush(); + // A disconnected client must not take down the deploy. + }) + .catchError((Object _) {}); } Future result(int statusCode, Map payload) async { @@ -434,194 +464,54 @@ void _writeCorsHeaders(HttpResponse response) { ..set('Vary', 'Origin'); } -Future> _readJson(HttpRequest request) async { - final raw = await utf8.decoder.bind(request).join(); - final decoded = jsonDecode(raw); - if (decoded is! Map) { - throw const FormatException('Request body must be a JSON object.'); - } - return decoded; -} - -String _stringField( - Map source, - String key, { - required int maxLength, - bool required = true, -}) { - final value = source[key]; - if (value == null || value == '') { - if (required) throw FormatException('Missing $key.'); - return ''; - } - if (value is! String) { - throw FormatException('$key must be a string.'); - } - final trimmed = value.trim(); - if (required && trimmed.isEmpty) { - throw FormatException('Missing $key.'); - } - if (utf8.encode(trimmed).length > maxLength) { - throw FormatException('$key is too long.'); - } - return trimmed; -} - -/// Reads the optional compile-check payload. Absent means the caller is an -/// older client that predates verification; its deploys keep working and are -/// reported as unverified rather than refused. -_VerificationRequest? _normalizeVerification(Object? value) { - if (value == null) return null; - if (value is! Map) { - throw const FormatException('verification must be an object.'); - } - - // Clients deployed before the manifest became structured sent pubspec.yaml - // text. Running it would reintroduce the caller-controlled-source problem, so - // it is never executed - but rejecting the request outright would break every - // deploy from a still-cached client during the rollout window. The deploy - // goes ahead with the check reported as unavailable, which is exactly what - // the deploy did before this feature existed, and the caller is told. - if (value.containsKey('pubspec') && !value.containsKey('dependencies')) { - return _VerificationRequest.unavailable( - 'The compile check was skipped: this client sent a package manifest in ' - 'the older format, which the runner no longer executes. Reload the app ' - 'to pick up the current version.', - ); - } - - final rawSources = value['sources']; - if (rawSources is! List) { - throw const FormatException('verification.sources must be an array.'); - } - if (rawSources.length > maxClassesPerRequest) { - throw const FormatException('Too many sources to verify in one request.'); - } - - final sources = rawSources.indexed.map((item) { - final raw = item.$2; - if (raw is! Map) { - throw FormatException('verification.sources[${item.$1}] must be an object.'); - } - final fileName = _stringField(raw, 'fileName', maxLength: 200); - // The name becomes a path inside the scratch package, so anything that - // could climb out of it is a request to write somewhere else. - if (!RegExp(r'^[a-z0-9_]+\.dart$').hasMatch(fileName)) { - throw FormatException('Invalid verification file name: $fileName.'); - } - return _VerificationSource( - fileName: fileName, - content: _stringField(raw, 'content', maxLength: maxCodeBytes), - ); - }).toList(); - - return _VerificationRequest( - sdkConstraint: _validateConstraint( - 'verification.sdkConstraint', - _stringField(value, 'sdkConstraint', maxLength: 200, required: false), - fallback: defaultSdkConstraint, - ), - dependencies: _normalizeDependencyMap(value['dependencies'], 'dependencies'), - overrides: _normalizeDependencyMap( - value['dependencyOverrides'], - 'dependencyOverrides', - ), - sdkPackages: _normalizeSdkPackages(value['sdkPackages'], 'sdkPackages'), - sdkOverrides: _normalizeSdkPackages(value['sdkOverrides'], 'sdkOverrides'), - sources: sources, - ); +/// Liveness and version probe, so a merged-but-never-deployed fix is +/// detectable in minutes rather than on the next failed deploy: the SHA the +/// image was built from (set by the deploy pipeline as RUNNER_GIT_SHA, absent +/// until then), the flutterflow_cli actually installed, and the minimum the +/// vendored snapshot requires. +Future> _healthz() async { + return { + 'sha': Platform.environment['RUNNER_GIT_SHA'] ?? '', + 'flutterflowCli': await _installedCliVersion(), + 'minFlutterflowCli': minCliVersion, + }; } -/// Reads a `{name: version-constraint}` map, rejecting anything that could -/// express more than a published package at a version. -/// -/// This is the whole reason the manifest is sent as data rather than as -/// pubspec.yaml text. The runner writes this manifest to disk and runs -/// `flutter pub get` against it on a publicly reachable route, so a -/// caller-supplied document could otherwise name a `git:` or `path:` source and -/// have the runner fetch from a host of the caller's choosing. Only bare -/// `name: version` entries can be expressed here, and the runner emits those -/// lines itself, so no source directive can survive into the manifest. -Map _normalizeDependencyMap(Object? value, String field) { - if (value == null) return const {}; - if (value is! Map) { - throw FormatException('verification.$field must be an object.'); - } - if (value.length > maxDependenciesPerRequest) { - throw FormatException('Too many entries in verification.$field.'); - } - - final result = {}; - for (final entry in value.entries) { - final name = '${entry.key}'; - if (!_isValidPackageName(name)) { - throw FormatException('Invalid package name in verification.$field: $name.'); - } - final constraint = '${entry.value}'.trim(); - if (!_constraintPattern.hasMatch(constraint)) { - throw FormatException( - 'Invalid version constraint for $name in verification.$field: $constraint.', - ); +Future _installedCliVersion() async { + try { + final process = await Process.start('dart', ['pub', 'global', 'list']); + final output = await process.stdout.transform(utf8.decoder).join(); + final exitCode = await process.exitCode; + // A version we could not read is better reported as absent than used to + // claim the deployed CLI matches the snapshot. + if (exitCode != 0) return ''; + final regex = RegExp(r'^flutterflow_cli\s+(\S+)'); + for (final line in const LineSplitter().convert(output)) { + final match = regex.firstMatch(line.trim()); + if (match != null) return match.group(1)!; } - result[name] = constraint; + return ''; + } on ProcessException { + return ''; } - return result; } -/// Reads the SDK-supplied package names a caller's project declares. -/// -/// Validated as package names rather than against a list of the packages the -/// Flutter SDK happens to ship: a name the SDK does not provide fails -/// `pub get` with an honest error, whereas a list refuses correct deploys the -/// moment it falls behind the SDK. The pattern is the load-bearing check - each -/// name becomes a key in the generated pubspec. -List _normalizeSdkPackages(Object? value, String field) { - if (value == null) return const []; - if (value is! List) { - throw FormatException('verification.$field must be an array.'); - } - // Bounded like the dependency maps: each entry expands into the generated - // pubspec, so an unbounded array is caller-controlled work before pub get - // ever sees it. - if (value.length > maxDependenciesPerRequest) { - throw FormatException('Too many entries in verification.$field.'); - } - - final result = {}; - for (final raw in value) { - final name = '$raw'; - if (!_isValidPackageName(name)) { - throw FormatException('Invalid SDK package name: $name.'); - } - result.add(name); - } - return result.toList(); +Future _writeJsonBody(HttpResponse response, Object body) async { + response.statusCode = HttpStatus.ok; + response.headers.contentType = ContentType.json; + response.write(jsonEncode(body)); + try { + await response.close(); + } catch (_) {} } -bool _isValidPackageName(String name) => - name.length <= maxPackageNameLength && _packageNamePattern.hasMatch(name); - -String _validateConstraint( - String field, - String value, { - required String fallback, -}) { - final constraint = value.trim(); - if (constraint.isEmpty) return fallback; - if (!_constraintPattern.hasMatch(constraint)) { - throw FormatException('Invalid $field: $constraint.'); +Future> _readJson(HttpRequest request) async { + final raw = await utf8.decoder.bind(request).join(); + final decoded = jsonDecode(raw); + if (decoded is! Map) { + throw const FormatException('Request body must be a JSON object.'); } - return constraint; -} - -/// Quotes a value for YAML when a plain scalar would be misread. -/// -/// `>=1.0.0 <2.0.0` is an ordinary pub range, but it opens with an indicator -/// character and YAML would reject it as a plain scalar - the same reason -/// pubspecSync's `formatConstraint` quotes on the client side. -String _yamlScalar(String value) { - if (RegExp(r'^[A-Za-z0-9][A-Za-z0-9._+-]*$').hasMatch(value)) return value; - return "'${value.replaceAll("'", "''")}'"; + return decoded; } List _normalizeClasses(Object? value) { @@ -632,7 +522,9 @@ List _normalizeClasses(Object? value) { throw const FormatException('At least one custom class is required.'); } if (value.length > maxClassesPerRequest) { - throw const FormatException('Too many custom classes in one deploy request.'); + throw const FormatException( + 'Too many custom classes in one deploy request.', + ); } return value.indexed.map((item) { @@ -642,14 +534,19 @@ List _normalizeClasses(Object? value) { throw FormatException('customClasses[$index] must be an object.'); } - final className = _stringField(raw, 'className', maxLength: 120); + final className = stringField(raw, 'className', maxLength: 120); if (!RegExp(r'^[A-Z][A-Za-z0-9_]*$').hasMatch(className)) { throw FormatException('Invalid custom class name: $className.'); } - final content = _stringField(raw, 'content', maxLength: maxCodeBytes); + final content = stringField(raw, 'content', maxLength: maxCodeBytes); return CustomClassEntry( - artifactId: _stringField(raw, 'artifactId', maxLength: 200, required: false), + artifactId: stringField( + raw, + 'artifactId', + maxLength: 200, + required: false, + ), className: className, content: content, ); @@ -667,7 +564,7 @@ List _normalizeClasses(Object? value) { /// caller states plainly what went unverified. Future<_AnalysisOutcome?> _verifyCustomCode( Directory workspace, - _VerificationRequest? verification, + VerificationRequest? verification, _ResponseChannel channel, ) async { if (verification == null) return null; @@ -685,11 +582,13 @@ Future<_AnalysisOutcome?> _verifyCustomCode( if (libDir.existsSync()) libDir.deleteSync(recursive: true); await libDir.create(recursive: true); - await File('${packageDir.path}/pubspec.yaml') - .writeAsString(verification.toPubspec()); + await File( + '${packageDir.path}/pubspec.yaml', + ).writeAsString(verification.toPubspec()); for (final source in verification.sources) { - await File('${libDir.path}/${source.fileName}') - .writeAsString(source.content); + await File( + '${libDir.path}/${source.fileName}', + ).writeAsString(source.content); } final pubGet = await _runProcess( @@ -697,6 +596,7 @@ Future<_AnalysisOutcome?> _verifyCustomCode( ['pub', 'get'], workingDirectory: packageDir.path, timeout: const Duration(minutes: 4), + channel: channel, ); if (pubGet.exitCode != 0) { return _AnalysisOutcome.skipped( @@ -710,6 +610,7 @@ Future<_AnalysisOutcome?> _verifyCustomCode( ['analyze', '--no-pub', '--no-fatal-infos', '--no-fatal-warnings'], workingDirectory: packageDir.path, timeout: const Duration(minutes: 4), + channel: channel, ); final errors = _analyzerErrors(analyze.output); @@ -760,8 +661,16 @@ Future<_RunOutcome> _runProcess( List args, { required String workingDirectory, required Duration timeout, + required _ResponseChannel channel, }) async { final buffer = StringBuffer(); + // Same reason as `_runFlutterFlow`: `flutter pub get` and `flutter analyze` + // can be silent for a long while, and a heartbeat keeps the request visibly + // alive to a streaming caller. + final heartbeat = Timer.periodic( + const Duration(seconds: 15), + (_) => channel.heartbeat(), + ); try { final process = await Process.start( executable, @@ -774,10 +683,13 @@ Future<_RunOutcome> _runProcess( process.stderr.transform(utf8.decoder).forEach(buffer.write), ]); - final exitCode = await process.exitCode.timeout(timeout, onTimeout: () { - process.kill(ProcessSignal.sigkill); - return -1; - }); + final exitCode = await process.exitCode.timeout( + timeout, + onTimeout: () { + process.kill(ProcessSignal.sigkill); + return -1; + }, + ); await drained; return _RunOutcome( exitCode: exitCode, @@ -790,6 +702,8 @@ Future<_RunOutcome> _runProcess( output: '$executable could not be started: ${error.message}', timedOut: false, ); + } finally { + heartbeat.cancel(); } } @@ -797,11 +711,13 @@ Future _writeDeployScript( Directory workspace, List classes, ) async { - final dslDir = Directory('${workspace.path}/dsl')..createSync(recursive: true); - final calls = classes.map((entry) { - final name = _dartSingleQuoted(entry.className); - final code = _dartRawString(entry.content); - return ''' + final dslDir = Directory('${workspace.path}/dsl') + ..createSync(recursive: true); + final calls = classes + .map((entry) { + final name = _dartSingleQuoted(entry.className); + final code = _dartRawString(entry.content); + return ''' if (findCustomClass(project, name: $name) == null) { addCustomClass( project, @@ -817,7 +733,8 @@ Future _writeDeployScript( } _ensureDartFileName(project, $name); '''; - }).join('\n'); + }) + .join('\n'); final script = ''' library; @@ -979,97 +896,6 @@ String _trimOutput(Object value) { return '${text.substring(0, 4000)}...'; } -final class _VerificationSource { - const _VerificationSource({required this.fileName, required this.content}); - - final String fileName; - final String content; -} - -final class _VerificationRequest { - const _VerificationRequest({ - required this.sdkConstraint, - required this.dependencies, - required this.overrides, - required this.sdkPackages, - required this.sdkOverrides, - required this.sources, - }) : unavailableReason = null; - - /// A request the runner will not compile, carrying why. - const _VerificationRequest.unavailable(this.unavailableReason) - : sdkConstraint = '', - dependencies = const {}, - overrides = const {}, - sdkPackages = const [], - sdkOverrides = const [], - sources = const <_VerificationSource>[]; - - /// Why this request cannot be compiled, or null when it can. - final String? unavailableReason; - - final String sdkConstraint; - final Map dependencies; - final Map overrides; - final List sdkPackages; - final List sdkOverrides; - final List<_VerificationSource> sources; - - /// Builds the pubspec the scratch package is compiled from. - /// - /// Emitted here rather than accepted from the caller: every line below is - /// either a fixed literal or a value already matched against - /// `_packageNamePattern` / `_constraintPattern`, so nothing a caller sends - /// can introduce a new YAML key, a nested mapping, or a comment. - String toPubspec() { - final lines = [ - 'name: $analysisPackageName', - 'description: Throwaway package used to compile generated custom code.', - 'publish_to: none', - 'version: 0.0.1', - '', - 'environment:', - ' sdk: ${_yamlScalar(sdkConstraint)}', - '', - 'dependencies:', - ' flutter:', - ' sdk: flutter', - ]; - - // `flutter` is always present above; any other SDK package the project - // declares is reproduced the same way. - for (final name in sdkPackages) { - if (name == 'flutter') continue; - lines - ..add(' $name:') - ..add(' sdk: flutter'); - } - - for (final name in dependencies.keys.toList()..sort()) { - lines.add(' $name: ${_yamlScalar(dependencies[name]!)}'); - } - - // SDK-sourced overrides belong in dependency_overrides, not in - // `dependencies` with sdkPackages: an override can share its name with a - // direct dependency, and emitting both under one map writes the key twice. - if (overrides.isNotEmpty || sdkOverrides.isNotEmpty) { - lines - ..add('') - ..add('dependency_overrides:'); - for (final name in overrides.keys.toList()..sort()) { - lines.add(' $name: ${_yamlScalar(overrides[name]!)}'); - } - for (final name in sdkOverrides.toList()..sort()) { - lines - ..add(' $name:') - ..add(' sdk: flutter'); - } - } - - return '${lines.join('\n')}\n'; - } -} - final class _AnalysisOutcome { const _AnalysisOutcome({ required this.errors, @@ -1078,10 +904,10 @@ final class _AnalysisOutcome { }) : skippedReason = null; const _AnalysisOutcome.skipped(String reason) - : errors = const [], - report = '', - verifiedFiles = const [], - skippedReason = reason; + : errors = const [], + report = '', + verifiedFiles = const [], + skippedReason = reason; final List errors; final String report; diff --git a/cloud-run/ffai-runner/lib/request_validation.dart b/cloud-run/ffai-runner/lib/request_validation.dart new file mode 100644 index 0000000..a69342a --- /dev/null +++ b/cloud-run/ffai-runner/lib/request_validation.dart @@ -0,0 +1,324 @@ +/// Request-manifest validation for the FlutterFlow AI deploy runner. +/// +/// The browser sends the verification manifest - the SDK packages, dependency +/// maps, version constraints and sources to compile - across a publicly +/// reachable route, and every field becomes a line the runner writes into a +/// pubspec it then runs `flutter pub get` against. Validation here is the +/// boundary that keeps a caller's data from turning into YAML structure, a +/// different package source, or a path outside the scratch package. The +/// functions are public so the contract tests in test/ can pin the exact +/// production behaviour, including the regression this helper used to cause: +/// sdkPackages is validated as package names, not against a stale allowlist, +/// so `flutter_web_plugins` and `flutter_localizations` are accepted. +/// +/// This file uses no server primitives - it only parses and rejects - so it +/// can be exercised in isolation. +library; + +import 'dart:convert'; + +const maxClassesPerRequest = 20; +const maxCodeBytes = 500000; +const maxDependenciesPerRequest = 200; +// pub.dev's own ceiling on a package name. +const maxPackageNameLength = 64; + +const analysisPackageName = 'ccc_custom_code_analysis'; +const defaultSdkConstraint = '>=3.0.0 <4.0.0'; + +// Pub package names are lower-case identifiers. Matching the whole string is +// what keeps a caller from smuggling YAML structure in through a key. +final _packageNamePattern = RegExp(r'^[a-z_][a-z0-9_]*$'); + +// A pub version constraint, and nothing else. The allowed characters exclude +// `:`, `#`, `{`, `}`, `/` and whitespace beyond single spaces, so `git:`, +// `path:`, a nested `hosted:` mapping, and comment injection are all +// unrepresentable rather than merely discouraged. +final _constraintPattern = RegExp(r'^[A-Za-z0-9^~<>=*+._ -]+$'); + +// Which packages the Flutter SDK supplies is not decided here. A caller names +// the packages its project declares as `sdk: flutter`, and this runner emits +// them as `name: {sdk: flutter}` - a value it writes itself, never one a caller +// sent. There was an allowlist of SDK package names here, and it refused good +// deploys: the client classified a package correctly by its pubspec source, and +// the runner rejected it for not appearing on a list that had gone stale. +// `_packageNamePattern` is what actually keeps a key from carrying YAML. + +/// Reads a `{name: version-constraint}` map, rejecting anything that could +/// express more than a published package at a version. +/// +/// This is the whole reason the manifest is sent as data rather than as +/// pubspec.yaml text. The runner writes this manifest to disk and runs +/// `flutter pub get` against it on a publicly reachable route, so a +/// caller-supplied document could otherwise name a `git:` or `path:` source and +/// have the runner fetch from a host of the caller's choosing. Only bare +/// `name: version` entries can be expressed here, and the runner emits those +/// lines itself, so no source directive can survive into the manifest. +Map normalizeDependencyMap(Object? value, String field) { + if (value == null) return const {}; + if (value is! Map) { + throw FormatException('verification.$field must be an object.'); + } + if (value.length > maxDependenciesPerRequest) { + throw FormatException('Too many entries in verification.$field.'); + } + + final result = {}; + for (final entry in value.entries) { + final name = '${entry.key}'; + if (!isValidPackageName(name)) { + throw FormatException( + 'Invalid package name in verification.$field: $name.', + ); + } + final constraint = '${entry.value}'.trim(); + if (!_constraintPattern.hasMatch(constraint)) { + throw FormatException( + 'Invalid version constraint for $name in verification.$field: $constraint.', + ); + } + result[name] = constraint; + } + return result; +} + +/// Reads the SDK-supplied package names a caller's project declares. +/// +/// Validated as package names rather than against a list of the packages the +/// Flutter SDK happens to ship: a name the SDK does not provide fails +/// `pub get` with an honest error, whereas a list refuses correct deploys the +/// moment it falls behind the SDK. The pattern is the load-bearing check - each +/// name becomes a key in the generated pubspec. +List normalizeSdkPackages(Object? value, String field) { + if (value == null) return const []; + if (value is! List) { + throw FormatException('verification.$field must be an array.'); + } + // Bounded like the dependency maps: each entry expands into the generated + // pubspec, so an unbounded array is caller-controlled work before pub get + // ever sees it. + if (value.length > maxDependenciesPerRequest) { + throw FormatException('Too many entries in verification.$field.'); + } + + final result = {}; + for (final raw in value) { + final name = '$raw'; + if (!isValidPackageName(name)) { + throw FormatException('Invalid SDK package name: $name.'); + } + result.add(name); + } + return result.toList(); +} + +bool isValidPackageName(String name) => + name.length <= maxPackageNameLength && _packageNamePattern.hasMatch(name); + +String validateConstraint( + String field, + String value, { + required String fallback, +}) { + final constraint = value.trim(); + if (constraint.isEmpty) return fallback; + if (!_constraintPattern.hasMatch(constraint)) { + throw FormatException('Invalid $field: $constraint.'); + } + return constraint; +} + +/// Reads a required or optional string field, trimming it and bounding its +/// encoded size. +String stringField( + Map source, + String key, { + required int maxLength, + bool required = true, +}) { + final value = source[key]; + if (value == null || value == '') { + if (required) throw FormatException('Missing $key.'); + return ''; + } + if (value is! String) { + throw FormatException('$key must be a string.'); + } + final trimmed = value.trim(); + if (required && trimmed.isEmpty) { + throw FormatException('Missing $key.'); + } + if (utf8.encode(trimmed).length > maxLength) { + throw FormatException('$key is too long.'); + } + return trimmed; +} + +/// Reads the optional compile-check payload. Absent means the caller is an +/// older client that predates verification; its deploys keep working and are +/// reported as unverified rather than refused. +VerificationRequest? normalizeVerification(Object? value) { + if (value == null) return null; + if (value is! Map) { + throw const FormatException('verification must be an object.'); + } + + // Clients deployed before the manifest became structured sent pubspec.yaml + // text. Running it would reintroduce the caller-controlled-source problem, so + // it is never executed - but rejecting the request outright would break every + // deploy from a still-cached client during the rollout window. The deploy + // goes ahead with the check reported as unavailable, which is exactly what + // the deploy did before this feature existed, and the caller is told. + if (value.containsKey('pubspec') && !value.containsKey('dependencies')) { + return VerificationRequest.unavailable( + 'The compile check was skipped: this client sent a package manifest in ' + 'the older format, which the runner no longer executes. Reload the app ' + 'to pick up the current version.', + ); + } + + final rawSources = value['sources']; + if (rawSources is! List) { + throw const FormatException('verification.sources must be an array.'); + } + if (rawSources.length > maxClassesPerRequest) { + throw const FormatException('Too many sources to verify in one request.'); + } + + final sources = + rawSources.indexed.map((item) { + final raw = item.$2; + if (raw is! Map) { + throw FormatException( + 'verification.sources[${item.$1}] must be an object.', + ); + } + final fileName = stringField(raw, 'fileName', maxLength: 200); + // The name becomes a path inside the scratch package, so anything that + // could climb out of it is a request to write somewhere else. + if (!RegExp(r'^[a-z0-9_]+\.dart$').hasMatch(fileName)) { + throw FormatException('Invalid verification file name: $fileName.'); + } + return VerificationSource( + fileName: fileName, + content: stringField(raw, 'content', maxLength: maxCodeBytes), + ); + }).toList(); + + return VerificationRequest( + sdkConstraint: validateConstraint( + 'verification.sdkConstraint', + stringField(value, 'sdkConstraint', maxLength: 200, required: false), + fallback: defaultSdkConstraint, + ), + dependencies: normalizeDependencyMap(value['dependencies'], 'dependencies'), + overrides: normalizeDependencyMap( + value['dependencyOverrides'], + 'dependencyOverrides', + ), + sdkPackages: normalizeSdkPackages(value['sdkPackages'], 'sdkPackages'), + sdkOverrides: normalizeSdkPackages(value['sdkOverrides'], 'sdkOverrides'), + sources: sources, + ); +} + +/// Quotes a value for YAML when a plain scalar would be misread. +/// +/// `>=1.0.0 <2.0.0` is an ordinary pub range, but it opens with an indicator +/// character and YAML would reject it as a plain scalar - the same reason +/// pubspecSync's `formatConstraint` quotes on the client side. +String _yamlScalar(String value) { + if (RegExp(r'^[A-Za-z0-9][A-Za-z0-9._+-]*$').hasMatch(value)) return value; + return "'${value.replaceAll("'", "''")}'"; +} + +final class VerificationSource { + const VerificationSource({required this.fileName, required this.content}); + + final String fileName; + final String content; +} + +final class VerificationRequest { + const VerificationRequest({ + required this.sdkConstraint, + required this.dependencies, + required this.overrides, + required this.sdkPackages, + required this.sdkOverrides, + required this.sources, + }) : unavailableReason = null; + + /// A request the runner will not compile, carrying why. + const VerificationRequest.unavailable(this.unavailableReason) + : sdkConstraint = '', + dependencies = const {}, + overrides = const {}, + sdkPackages = const [], + sdkOverrides = const [], + sources = const []; + + /// Why this request cannot be compiled, or null when it can. + final String? unavailableReason; + + final String sdkConstraint; + final Map dependencies; + final Map overrides; + final List sdkPackages; + final List sdkOverrides; + final List sources; + + /// Builds the pubspec the scratch package is compiled from. + /// + /// Emitted here rather than accepted from the caller: every line below is + /// either a fixed literal or a value already matched against + /// [_packageNamePattern] / [_constraintPattern], so nothing a caller sends + /// can introduce a new YAML key, a nested mapping, or a comment. + String toPubspec() { + final lines = [ + 'name: $analysisPackageName', + 'description: Throwaway package used to compile generated custom code.', + 'publish_to: none', + 'version: 0.0.1', + '', + 'environment:', + ' sdk: ${_yamlScalar(sdkConstraint)}', + '', + 'dependencies:', + ' flutter:', + ' sdk: flutter', + ]; + + // `flutter` is always present above; any other SDK package the project + // declares is reproduced the same way. + for (final name in sdkPackages) { + if (name == 'flutter') continue; + lines + ..add(' $name:') + ..add(' sdk: flutter'); + } + + for (final name in dependencies.keys.toList()..sort()) { + lines.add(' $name: ${_yamlScalar(dependencies[name]!)}'); + } + + // SDK-sourced overrides belong in dependency_overrides, not in + // `dependencies` with sdkPackages: an override can share its name with a + // direct dependency, and emitting both under one map writes the key twice. + if (overrides.isNotEmpty || sdkOverrides.isNotEmpty) { + lines + ..add('') + ..add('dependency_overrides:'); + for (final name in overrides.keys.toList()..sort()) { + lines.add(' $name: ${_yamlScalar(overrides[name]!)}'); + } + for (final name in sdkOverrides.toList()..sort()) { + lines + ..add(' $name:') + ..add(' sdk: flutter'); + } + } + + return '${lines.join('\n')}\n'; + } +} diff --git a/cloud-run/ffai-runner/pubspec.yaml b/cloud-run/ffai-runner/pubspec.yaml index a18d355..9f06f5a 100644 --- a/cloud-run/ffai-runner/pubspec.yaml +++ b/cloud-run/ffai-runner/pubspec.yaml @@ -5,3 +5,6 @@ publish_to: none environment: sdk: ">=3.7.0 <4.0.0" + +dev_dependencies: + test: ^1.25.0 diff --git a/cloud-run/ffai-runner/test/request_validation_test.dart b/cloud-run/ffai-runner/test/request_validation_test.dart new file mode 100644 index 0000000..db19e64 --- /dev/null +++ b/cloud-run/ffai-runner/test/request_validation_test.dart @@ -0,0 +1,140 @@ +import 'package:ccc_ffai_runner/request_validation.dart'; +import 'package:test/test.dart'; + +/// Contract tests for the runner's request-manifest validation. +/// +/// The production regression this guards: sdkPackages was once validated +/// against a hardcoded allowlist that had gone stale, so a correct deploy +/// declaring `flutter_web_plugins` (or `flutter_localizations`) was rejected +/// with "Unsupported SDK package" and every such deploy failed for months. The +/// list is gone - names are validated as package names, not membership - and +/// these tests pin that so the regression cannot quietly come back. +void main() { + group('normalizeSdkPackages', () { + test('accepts flutter_web_plugins and flutter_localizations', () { + final packages = normalizeSdkPackages([ + 'flutter', + 'flutter_web_plugins', + 'flutter_localizations', + ], 'sdkPackages'); + expect(packages, contains('flutter_web_plugins')); + expect(packages, contains('flutter_localizations')); + }); + + test('rejects a name carrying a dash', () { + expect( + () => normalizeSdkPackages(['flutter-web'], 'sdkPackages'), + throwsA(isA()), + ); + }); + + test('rejects a name carrying a colon', () { + expect( + () => normalizeSdkPackages(['foo:bar'], 'sdkPackages'), + throwsA(isA()), + ); + }); + + test('rejects YAML punctuation (colon, braces, spaces) in a name', () { + expect( + () => normalizeSdkPackages(['foo: {bar: baz}'], 'sdkPackages'), + throwsA(isA()), + ); + }); + + test('isValidPackageName reflects the same rule', () { + expect(isValidPackageName('flutter_web_plugins'), isTrue); + expect(isValidPackageName('flutter-localizations'), isFalse); + expect(isValidPackageName('flutter:localizations'), isFalse); + }); + }); + + group('normalizeDependencyMap', () { + test('accepts a bare name: constraint entry', () { + final map = normalizeDependencyMap({'http': '^1.0.0'}, 'dependencies'); + expect(map, {'http': '^1.0.0'}); + }); + + test('rejects an empty constraint', () { + expect( + () => normalizeDependencyMap({'http': ''}, 'dependencies'), + throwsA(isA()), + ); + }); + + test('rejects a constraint that could smuggle a git, path or comment', () { + for (final bad in [ + 'git: https://example.com/repo.git', + 'path: ../somewhere', + '^1.0.0 # a comment', + ]) { + expect( + () => normalizeDependencyMap({'http': bad}, 'dependencies'), + throwsA(isA()), + reason: 'expected rejection for constraint: $bad', + ); + } + }); + + test('rejects a non-object value and an oversized map', () { + expect( + () => normalizeDependencyMap('nope', 'dependencies'), + throwsA(isA()), + ); + }); + }); + + group('validateConstraint', () { + test('falls back to the default when the field is empty', () { + expect( + validateConstraint( + 'verification.sdkConstraint', + '', + fallback: '>=3.0.0 <4.0.0', + ), + '>=3.0.0 <4.0.0', + ); + }); + + test('rejects a constraint carrying a colon or slash', () { + expect( + () => validateConstraint( + 'verification.sdkConstraint', + '>=3.0.0 git: x', + fallback: '>=3.0.0 <4.0.0', + ), + throwsA(isA()), + ); + }); + }); + + group('normalizeVerification end-to-end', () { + test('accepts a full manifest declaring flutter_web_plugins', () { + final request = normalizeVerification({ + 'sdkPackages': [ + 'flutter', + 'flutter_web_plugins', + 'flutter_localizations', + ], + 'dependencies': {'http': '^1.0.0'}, + 'sources': [ + {'fileName': 'my_class.dart', 'content': 'class MyClass {}'}, + ], + }); + expect(request, isNotNull); + expect(request!.sdkPackages, contains('flutter_web_plugins')); + expect(request.sdkPackages, contains('flutter_localizations')); + expect(request.dependencies, {'http': '^1.0.0'}); + }); + + test('toPubspec emits the sdk package as a source the runner writes', () { + final request = normalizeVerification({ + 'sdkPackages': ['flutter_web_plugins'], + 'sources': [], + }); + final pubspec = request!.toPubspec(); + expect(pubspec, contains(' flutter_web_plugins:')); + expect(pubspec, contains(' sdk: flutter')); + }); + }); +} diff --git a/src/cloudRunRunner.test.js b/src/cloudRunRunner.test.js index dfdb100..4c0f5fb 100644 --- a/src/cloudRunRunner.test.js +++ b/src/cloudRunRunner.test.js @@ -6,10 +6,14 @@ const dockerfile = readFileSync( new URL("../cloud-run/ffai-runner/Dockerfile", import.meta.url), "utf8", ); -const runnerSource = readFileSync( - new URL("../cloud-run/ffai-runner/bin/server.dart", import.meta.url), - "utf8", -); +// Request validation lives in lib/request_validation.dart (server.dart imports +// it), so the contract assertions must read both the entrypoint and the module. +const runnerSource = [ + "../cloud-run/ffai-runner/bin/server.dart", + "../cloud-run/ffai-runner/lib/request_validation.dart", +] + .map((path) => readFileSync(new URL(path, import.meta.url), "utf8")) + .join("\n"); const deployScript = readFileSync( new URL("../scripts/deploy_cloud_run_ffai.sh", import.meta.url), "utf8", @@ -158,7 +162,7 @@ test("Cloud Run builds the analysis manifest itself rather than running caller-s ); assert.match( runnerSource, - /_VerificationRequest\.unavailable\(/, + /VerificationRequest\.unavailable\(/, "the legacy path must report itself unavailable", ); });