Code refactor: tagPattern verification and ref checks. (#8697)

Author: isoos

Diff for: app/lib/package/backend.dart

@@ -527,6 +527,7 @@ class PackageBackend {
         final repository = githubConfig.repository?.trim() ?? '';
         githubConfig.repository = repository.isEmpty ? null : repository;
         final tagPattern = githubConfig.tagPattern?.trim() ?? '';
+        verifyTagPattern(tagPattern: tagPattern);
         githubConfig.tagPattern = tagPattern.isEmpty ? null : tagPattern;
         final environment = githubConfig.environment?.trim() ?? '';
         githubConfig.environment = environment.isEmpty ? null : environment;
@@ -544,15 +545,6 @@ class PackageBackend {
               'The `repository` field has invalid characters.');
         }
 
-        final tagPatternParts = tagPattern.split('{{version}}');
-        InvalidInputException.check(tagPatternParts.length == 2,
-            'The `tagPattern` field must contain a single `{{version}}` part.');
-        InvalidInputException.check(
-            tagPatternParts
-                .where((e) => e.isNotEmpty)
-                .every(_validGitHubVersionPattern.hasMatch),
-            'The `tagPattern` field has invalid characters.');
-
         InvalidInputException.check(
             !githubConfig.requireEnvironment || environment.isNotEmpty,
             'The `environment` field must not be empty when enabled.');
@@ -1339,11 +1331,6 @@ class PackageBackend {
     if (repository == null || repository.isEmpty) {
       throw AssertionError('Missing or empty repository.');
     }
-    final tagPattern = githubConfig.tagPattern ?? '';
-    if (!tagPattern.contains('{{version}}')) {
-      throw AssertionError(
-          'Configured tag pattern does not include `{{version}}`');
-    }
     final requireEnvironment = githubConfig.requireEnvironment;
     final environment = githubConfig.environment;
     if (requireEnvironment && (environment == null || environment.isEmpty)) {
@@ -1375,25 +1362,11 @@ class PackageBackend {
       throw AuthorizationException.githubActionIssue(
           'publishing is only allowed from "tag" refType, this token has "${agent.payload.refType}" refType');
     }
-    final expectedRefStart = 'refs/tags/';
-    if (!agent.payload.ref.startsWith(expectedRefStart)) {
-      throw AuthorizationException.githubActionIssue(
-          'publishing is only allowed from "refs/tags/*" ref, this token has "${agent.payload.ref}" ref');
-    }
-    final expectedTagValue = tagPattern.replaceFirst('{{version}}', newVersion);
-    if (agent.payload.ref != 'refs/tags/$expectedTagValue') {
-      // At this point we have concluded that the agent has push rights to the repository,
-      // however, the tag pattern they have used is not the one we expect.
-      //
-      // By revealing the expected tag pattern, we are serving the users with better
-      // error message, while not exposing much information to an assumed attacker.
-      // With the current access level, an attacker would have access to past tags, and
-      // figuring out the tag pattern from those should be straightforward anyway.
-      throw AuthorizationException.githubActionIssue(
-          'publishing is configured to only be allowed from actions with specific ref pattern, '
-          'this token has "${agent.payload.ref}" ref for which publishing is not allowed. '
-          'Expected tag "$expectedTagValue". Check that the version in the tag matches the version in "pubspec.yaml"');
-    }
+    verifyTagPatternWithRef(
+      tagPattern: githubConfig.tagPattern ?? '',
+      ref: agent.payload.ref,
+      newVersion: newVersion,
+    );
 
     // When environment is configured, it must match the action's environment.
     if (requireEnvironment && environment != agent.payload.environment) {
@@ -1759,6 +1732,54 @@ Future<void> purgePackageCache(String package) async {
   ]);
 }
 
+/// Verifies the [tagPattern] before storing it on the automated publishing
+/// settings object.
+@visibleForTesting
+void verifyTagPattern({required String tagPattern}) {
+  final tagPatternParts = tagPattern.split('{{version}}');
+  InvalidInputException.check(tagPatternParts.length == 2,
+      'The `tagPattern` field must contain a single `{{version}}` part.');
+  InvalidInputException.check(
+      tagPatternParts
+          .where((e) => e.isNotEmpty)
+          .every(_validGitHubVersionPattern.hasMatch),
+      'The `tagPattern` field has invalid characters.');
+}
+
+/// Verifies the user-settings [tagPattern] with the authentication-provided
+/// [ref] value, and throws if the ref is not allowed or not recognized as
+/// valid pattern.
+@visibleForTesting
+void verifyTagPatternWithRef({
+  required String tagPattern,
+  required String ref,
+  required String newVersion,
+}) {
+  if (!tagPattern.contains('{{version}}')) {
+    throw AssertionError(
+        'Configured tag pattern does not include `{{version}}`');
+  }
+  final expectedRefStart = 'refs/tags/';
+  if (!ref.startsWith(expectedRefStart)) {
+    throw AuthorizationException.githubActionIssue(
+        'publishing is only allowed from "refs/tags/*" ref, this token has "$ref" ref');
+  }
+  final expectedTagValue = tagPattern.replaceFirst('{{version}}', newVersion);
+  if (ref != 'refs/tags/$expectedTagValue') {
+    // At this point we have concluded that the agent has push rights to the repository,
+    // however, the tag pattern they have used is not the one we expect.
+    //
+    // By revealing the expected tag pattern, we are serving the users with better
+    // error message, while not exposing much information to an assumed attacker.
+    // With the current access level, an attacker would have access to past tags, and
+    // figuring out the tag pattern from those should be straightforward anyway.
+    throw AuthorizationException.githubActionIssue(
+        'publishing is configured to only be allowed from actions with specific ref pattern, '
+        'this token has "$ref" ref for which publishing is not allowed. '
+        'Expected tag "$expectedTagValue". Check that the version in the tag matches the version in "pubspec.yaml"');
+  }
+}
+
 /// The status of an invite after being created or updated.
 class InviteStatus {
   final String? urlNonce;

Diff for: app/test/package/backend_test.dart

@@ -470,4 +470,67 @@ void main() {
       });
     });
   });
+
+  group('GitHub tagPattern', () {
+    test('verifyTagPattern: valid inputs', () {
+      final values = [
+        '{{version}}',
+        'v{{version}}',
+        '{{version}}v',
+        'package-{{version}}',
+        'package-v{{version}}',
+        'package-v{{version}}-postfix',
+      ];
+      for (final value in values) {
+        verifyTagPattern(tagPattern: value);
+      }
+    });
+
+    test('verifyTagPattern: invalid inputs', () {
+      final values = [
+        '', // empty pattern is not allowed
+        '{{version}}{{version}}', // two {{version}} is not allowed
+        '%-{{version}}', // % is not allowed
+        'abc/def-{{version}}', // / is not allowed
+        '{{version}}-abc/def', // / is not allowed
+      ];
+      for (final value in values) {
+        expect(
+          () => verifyTagPattern(tagPattern: value),
+          throwsA(isA<InvalidInputException>()),
+        );
+      }
+    });
+
+    test('verifyTagPatternWithRef: valid inputs', () {
+      final values = [
+        ('{{version}}', 'refs/tags/1.0.0'),
+        ('pkg-v{{version}}', 'refs/tags/pkg-v1.0.0'),
+      ];
+      for (final value in values) {
+        verifyTagPatternWithRef(
+          tagPattern: value.$1,
+          ref: value.$2,
+          newVersion: '1.0.0',
+        );
+      }
+    });
+
+    test('verifyTagPatternWithRef: invalid inputs', () {
+      final values = [
+        ('v{{version}}', 'refs/tags/1.0.0'), // does not match `v` prefix
+        ('v{{version}}', 'refs/x/v1.0.0'), // missing refs/tags
+      ];
+      for (final value in values) {
+        expect(
+          () => verifyTagPatternWithRef(
+            tagPattern: value.$1,
+            ref: value.$2,
+            newVersion: '1.0.0',
+          ),
+          throwsA(isA<AuthorizationException>()),
+        );
+      }
+    });
+  });
 }