From b89bfd8e375cf08494cc7b30d97b0747df9f63d4 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Mon, 10 Jul 2023 12:32:56 -0700 Subject: [PATCH] re-enable "Linux packages_autoroller" (#130088) Fixes https://github.com/flutter/flutter/issues/129744 This change: 1. re-enables the Linux packages_autoroller 2. ensures we redact the token from appearing in any logs (in local testing I realized some failure logs might still expose the token) What actually fixed authentication however was creating and uploading a new GitHub personal access token, not this change. It's currently failing post-submit because being marked `bringup` it is running in the try pool, which does not have permissions to access the cloud KMS. However, I ran a LED build in the prod pool that succeeded: https://ci.chromium.org/raw/build/logs.chromium.org/flutter/led/fujino_google.com/3a8f128c352fca53a9a29f1e7eab6c3ed24f3bb2a5feb196ea1a69127540e8a6/+/build.proto?server=chromium-swarm.appspot.com --- .ci.yaml | 2 -- .../core/bin/packages_autoroller.dart | 5 +-- dev/conductor/core/lib/src/stdio.dart | 25 +++++++++++++++ dev/conductor/core/test/common.dart | 1 + dev/conductor/core/test/next_test.dart | 30 ++---------------- .../core/test/packages_autoroller_test.dart | 31 +++++++++++++++++++ 6 files changed, 63 insertions(+), 31 deletions(-) diff --git a/.ci.yaml b/.ci.yaml index 0a51da5a3a..78b22e1e4d 100644 --- a/.ci.yaml +++ b/.ci.yaml @@ -243,8 +243,6 @@ targets: - name: Linux packages_autoroller presubmit: false - # TODO(fujino): https://github.com/flutter/flutter/issues/129744 - bringup: true recipe: pub_autoroller/pub_autoroller timeout: 30 enabled_branches: diff --git a/dev/conductor/core/bin/packages_autoroller.dart b/dev/conductor/core/bin/packages_autoroller.dart index 6cb35b2cf8..a890927f0a 100644 --- a/dev/conductor/core/bin/packages_autoroller.dart +++ b/dev/conductor/core/bin/packages_autoroller.dart @@ -80,7 +80,7 @@ ${parser.usage} } final FrameworkRepository framework = FrameworkRepository( - _localCheckouts, + _localCheckouts(token), mirrorRemote: Remote.mirror(mirrorUrl), upstreamRemote: Remote.upstream(upstreamUrl), ); @@ -106,7 +106,7 @@ String _parseOrgName(String remoteUrl) { return match.group(1)!; } -Checkouts get _localCheckouts { +Checkouts _localCheckouts(String token) { const FileSystem fileSystem = LocalFileSystem(); const ProcessManager processManager = LocalProcessManager(); const Platform platform = LocalPlatform(); @@ -114,6 +114,7 @@ Checkouts get _localCheckouts { stdout: io.stdout, stderr: io.stderr, stdin: io.stdin, + filter: (String message) => message.replaceAll(token, '[GitHub TOKEN]'), ); return Checkouts( fileSystem: fileSystem, diff --git a/dev/conductor/core/lib/src/stdio.dart b/dev/conductor/core/lib/src/stdio.dart index 8777b91f40..df8f81d056 100644 --- a/dev/conductor/core/lib/src/stdio.dart +++ b/dev/conductor/core/lib/src/stdio.dart @@ -70,6 +70,7 @@ class VerboseStdio extends Stdio { required this.stdout, required this.stderr, required this.stdin, + this.filter, }); factory VerboseStdio.local() => VerboseStdio( @@ -82,26 +83,50 @@ class VerboseStdio extends Stdio { final io.Stdout stderr; final io.Stdin stdin; + /// If provided, all messages will be passed through this function before being logged. + final String Function(String)? filter; + @override void printError(String message) { + if (filter != null) { + message = filter!(message); + } super.printError(message); stderr.writeln(message); } + @override + void printWarning(String message) { + if (filter != null) { + message = filter!(message); + } + super.printWarning(message); + stderr.writeln(message); + } + @override void printStatus(String message) { + if (filter != null) { + message = filter!(message); + } super.printStatus(message); stdout.writeln(message); } @override void printTrace(String message) { + if (filter != null) { + message = filter!(message); + } super.printTrace(message); stdout.writeln(message); } @override void write(String message) { + if (filter != null) { + message = filter!(message); + } super.write(message); stdout.write(message); } diff --git a/dev/conductor/core/test/common.dart b/dev/conductor/core/test/common.dart index 9cf66ffdfe..f1a2f39999 100644 --- a/dev/conductor/core/test/common.dart +++ b/dev/conductor/core/test/common.dart @@ -6,6 +6,7 @@ import 'package:args/args.dart'; import 'package:conductor_core/src/stdio.dart'; import 'package:test/test.dart'; +export 'package:test/fake.dart'; export 'package:test/test.dart' hide isInstanceOf; export '../../../../packages/flutter_tools/test/src/fake_process_manager.dart'; diff --git a/dev/conductor/core/test/next_test.dart b/dev/conductor/core/test/next_test.dart index f124aa2417..dee3a2e567 100644 --- a/dev/conductor/core/test/next_test.dart +++ b/dev/conductor/core/test/next_test.dart @@ -1227,34 +1227,10 @@ void main() { } /// A [Stdio] that will throw an exception if any of its methods are called. -class _UnimplementedStdio implements Stdio { - const _UnimplementedStdio(); +class _UnimplementedStdio extends Fake implements Stdio { + _UnimplementedStdio(); - static const _UnimplementedStdio _instance = _UnimplementedStdio(); - static _UnimplementedStdio get instance => _instance; - - Never _throw() => throw Exception('Unimplemented!'); - - @override - List get logs => _throw(); - - @override - void printError(String message) => _throw(); - - @override - void printWarning(String message) => _throw(); - - @override - void printStatus(String message) => _throw(); - - @override - void printTrace(String message) => _throw(); - - @override - void write(String message) => _throw(); - - @override - String readLineSync() => _throw(); + static final _UnimplementedStdio instance = _UnimplementedStdio(); } class _TestRepository extends Repository { diff --git a/dev/conductor/core/test/packages_autoroller_test.dart b/dev/conductor/core/test/packages_autoroller_test.dart index 50e21a1aee..9ce7a282a6 100644 --- a/dev/conductor/core/test/packages_autoroller_test.dart +++ b/dev/conductor/core/test/packages_autoroller_test.dart @@ -512,4 +512,35 @@ void main() { expect(processManager, hasNoRemainingExpectations); }); }); + + test('VerboseStdio logger can filter out confidential pattern', () async { + const String token = 'secret'; + const String replacement = 'replacement'; + final VerboseStdio stdio = VerboseStdio( + stdin: _NoOpStdin(), + stderr: _NoOpStdout(), + stdout: _NoOpStdout(), + filter: (String msg) => msg.replaceAll(token, replacement), + ); + stdio.printStatus('Hello'); + expect(stdio.logs.last, '[status] Hello'); + + stdio.printStatus('Using $token'); + expect(stdio.logs.last, '[status] Using $replacement'); + + stdio.printWarning('Using $token'); + expect(stdio.logs.last, '[warning] Using $replacement'); + + stdio.printError('Using $token'); + expect(stdio.logs.last, '[error] Using $replacement'); + + stdio.printTrace('Using $token'); + expect(stdio.logs.last, '[trace] Using $replacement'); + }); +} + +class _NoOpStdin extends Fake implements io.Stdin {} +class _NoOpStdout extends Fake implements io.Stdout { + @override + void writeln([Object? object]) {} }