From a833ed32168c30d6878c0a82bda9771b10f97dae Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Thu, 3 Apr 2025 14:31:02 +0200 Subject: [PATCH] Code refactor: tagPattern verification and ref checks. --- app/lib/package/backend.dart | 87 ++++++++++++++++++------------ app/test/package/backend_test.dart | 63 ++++++++++++++++++++++ 2 files changed, 117 insertions(+), 33 deletions(-) diff --git a/app/lib/package/backend.dart b/app/lib/package/backend.dart index 27631a5b5d..4818d11b13 100644 --- a/app/lib/package/backend.dart +++ b/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 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 --git a/app/test/package/backend_test.dart b/app/test/package/backend_test.dart index 29153fee58..96f1373625 100644 --- a/app/test/package/backend_test.dart +++ b/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()), + ); + } + }); + + 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()), + ); + } + }); + }); }