diff --git a/app/config/dartlang-pub.yaml b/app/config/dartlang-pub.yaml index d3ab17bd02..7f836955f5 100644 --- a/app/config/dartlang-pub.yaml +++ b/app/config/dartlang-pub.yaml @@ -75,6 +75,10 @@ rateLimits: burst: 3 hourly: 6 daily: 12 + weekly: 20 # 20 versions/week + # monthly: 32 # ~ 8 versions/week + # quarterly: 50 # ~ 4 versions/week + # yearly: 100 # ~ 2 versions/week - operation: package-published scope: user burst: 200 diff --git a/app/lib/package/backend.dart b/app/lib/package/backend.dart index 3f8f4a2007..a22b043f7e 100644 --- a/app/lib/package/backend.dart +++ b/app/lib/package/backend.dart @@ -1085,11 +1085,17 @@ class PackageBackend { 'Package "${newVersion.package}" has no admin email to notify.'); } - // check rate limits before the transaction + final existingVersions = await db + .query(ancestorKey: newVersion.packageKey!) + .run() + .toList(); + + // check long term (over a day) rate limits with version created timestamps await verifyPackageUploadRateLimit( agent: agent, package: newVersion.package, isNew: isNew, + timestamps: existingVersions.map((v) => v.created!).toList(), ); final email = createPackageUploadedEmail( @@ -1102,11 +1108,6 @@ class PackageBackend { final outgoingEmail = emailBackend.prepareEntity(email); Package? package; - final existingVersions = await db - .query(ancestorKey: newVersion.packageKey!) - .run() - .toList(); - // Add the new package to the repository by storing the tarball and // inserting metadata to datastore (which happens atomically). final pv = await withRetryTransaction(db, (tx) async { diff --git a/app/lib/service/rate_limit/rate_limit.dart b/app/lib/service/rate_limit/rate_limit.dart index 33f8ff137f..bcc67bcbf4 100644 --- a/app/lib/service/rate_limit/rate_limit.dart +++ b/app/lib/service/rate_limit/rate_limit.dart @@ -16,17 +16,17 @@ import '../../shared/redis_cache.dart'; final _logger = Logger('rate_limit'); -/// Verifies if the current package upload has a rate limit and throws -/// if the limit has been exceeded. +/// Verifies if the current package upload has an applicable rate limit +/// and throws if the limit has been exceeded. Future verifyPackageUploadRateLimit({ required AuthenticatedAgent agent, required String package, required bool isNew, + required List timestamps, }) async { - final packagePublishedOp = AuditLogRecordKind.packagePublished; - await _verifyRateLimit( - rateLimit: _getRateLimit(packagePublishedOp, RateLimitScope.user), + rateLimit: + _getRateLimit(AuditLogRecordKind.packagePublished, RateLimitScope.user), agentId: agent.agentId, ); @@ -42,8 +42,10 @@ Future verifyPackageUploadRateLimit({ // regular package-specific limits await _verifyRateLimit( - rateLimit: _getRateLimit(packagePublishedOp, RateLimitScope.package), + rateLimit: _getRateLimit( + AuditLogRecordKind.packagePublished, RateLimitScope.package), package: package, + timestamps: timestamps, ); } @@ -72,6 +74,7 @@ Future _verifyRateLimit({ required RateLimit? rateLimit, String? package, String? agentId, + List? timestamps, }) async { assert(agentId != null || package != null); if (agentId == KnownAgents.pubSupport) { @@ -81,9 +84,7 @@ Future _verifyRateLimit({ if (rateLimit == null) { return; } - if (rateLimit.burst == null && - rateLimit.hourly == null && - rateLimit.daily == null) { + if (rateLimit.isEmpty) { return; } @@ -123,20 +124,34 @@ Future _verifyRateLimit({ final now = clock.now().toUtc(); final windowStart = now.subtract(window); - final relevantEntries = auditEntriesFromLastDay! - .where((e) => e.kind == rateLimit.operation) - .where((e) => e.created!.isAfter(windowStart)) - .where((e) => package == null || _containsPackage(e.packages, package)) - .where((e) => - agentId == null || - e.agent == agentId || - _containsUserId(e.users, agentId)) - .toList(); - - if (relevantEntries.length >= maxCount) { - final firstTimestamp = relevantEntries + + late List relevantTimestamps; + if (timestamps != null) { + relevantTimestamps = + timestamps.where((t) => t.isAfter(windowStart)).toList(); + } else { + // Sanity check: most rate limit operations only support rate limit within a day. + // The ones which support longer window must provide their list of timestamps. + if (window.inDays > 1) { + throw ArgumentError( + 'Rate limit "${rateLimit.operation}" does not support long-term rules.'); + } + relevantTimestamps = auditEntriesFromLastDay! + .where((e) => e.kind == rateLimit.operation) + .where((e) => e.created!.isAfter(windowStart)) + .where( + (e) => package == null || _containsPackage(e.packages, package)) + .where((e) => + agentId == null || + e.agent == agentId || + _containsUserId(e.users, agentId)) .map((e) => e.created!) - .reduce((a, b) => a.isBefore(b) ? a : b); + .toList(); + } + + if (relevantTimestamps.length >= maxCount) { + final firstTimestamp = + relevantTimestamps.reduce((a, b) => a.isBefore(b) ? a : b); await entry.set(firstTimestamp.add(window), window); throw RateLimitException( operation: operation, @@ -165,6 +180,32 @@ Future _verifyRateLimit({ maxCount: rateLimit.daily, windowAsText: 'last day', ); + + // note: long-term limits are only checked if timestamps are provided + await check( + operation: rateLimit.operation, + window: Duration(days: 7), + maxCount: rateLimit.weekly, + windowAsText: 'last week', + ); + await check( + operation: rateLimit.operation, + window: Duration(days: 30), + maxCount: rateLimit.monthly, + windowAsText: 'last month', + ); + await check( + operation: rateLimit.operation, + window: Duration(days: 91), + maxCount: rateLimit.quarterly, + windowAsText: 'last 3 months', + ); + await check( + operation: rateLimit.operation, + window: Duration(days: 365), + maxCount: rateLimit.yearly, + windowAsText: 'last year', + ); sw.stop(); _logger.info('[rate-limit-verified] Rate limit verified in ${sw.elapsed}'); } diff --git a/app/lib/shared/configuration.dart b/app/lib/shared/configuration.dart index af3a13411c..d813414773 100644 --- a/app/lib/shared/configuration.dart +++ b/app/lib/shared/configuration.dart @@ -543,16 +543,40 @@ class RateLimit { /// Maximum number of operations in 24 hours. final int? daily; + /// Maximum number of operations in 7 days. + final int? weekly; + + /// Maximum number of operations in 30 days. + final int? monthly; + + /// Maximum number of operations in 91 days. + final int? quarterly; + + /// Maximum number of operations in 365 days. + final int? yearly; + RateLimit({ required this.operation, required this.scope, this.burst, this.hourly, this.daily, + this.weekly, + this.monthly, + this.quarterly, + this.yearly, }); factory RateLimit.fromJson(Map json) => _$RateLimitFromJson(json); Map toJson() => _$RateLimitToJson(this); + + late final isEmpty = burst == null && + hourly == null && + daily == null && + weekly == null && + monthly == null && + quarterly == null && + yearly == null; } diff --git a/app/lib/shared/configuration.g.dart b/app/lib/shared/configuration.g.dart index 86144f90f9..3a0b2b9fa6 100644 --- a/app/lib/shared/configuration.g.dart +++ b/app/lib/shared/configuration.g.dart @@ -251,7 +251,17 @@ RateLimit _$RateLimitFromJson(Map json) => $checkedCreate( ($checkedConvert) { $checkKeys( json, - allowedKeys: const ['operation', 'scope', 'burst', 'hourly', 'daily'], + allowedKeys: const [ + 'operation', + 'scope', + 'burst', + 'hourly', + 'daily', + 'weekly', + 'monthly', + 'quarterly', + 'yearly' + ], ); final val = RateLimit( operation: $checkedConvert('operation', (v) => v as String), @@ -260,6 +270,10 @@ RateLimit _$RateLimitFromJson(Map json) => $checkedCreate( burst: $checkedConvert('burst', (v) => (v as num?)?.toInt()), hourly: $checkedConvert('hourly', (v) => (v as num?)?.toInt()), daily: $checkedConvert('daily', (v) => (v as num?)?.toInt()), + weekly: $checkedConvert('weekly', (v) => (v as num?)?.toInt()), + monthly: $checkedConvert('monthly', (v) => (v as num?)?.toInt()), + quarterly: $checkedConvert('quarterly', (v) => (v as num?)?.toInt()), + yearly: $checkedConvert('yearly', (v) => (v as num?)?.toInt()), ); return val; }, @@ -280,6 +294,10 @@ Map _$RateLimitToJson(RateLimit instance) { writeNotNull('burst', instance.burst); writeNotNull('hourly', instance.hourly); writeNotNull('daily', instance.daily); + writeNotNull('weekly', instance.weekly); + writeNotNull('monthly', instance.monthly); + writeNotNull('quarterly', instance.quarterly); + writeNotNull('yearly', instance.yearly); return val; } diff --git a/app/test/package/upload_rate_limit_test.dart b/app/test/package/upload_rate_limit_test.dart index a43f3ae2c7..c59e48cb8a 100644 --- a/app/test/package/upload_rate_limit_test.dart +++ b/app/test/package/upload_rate_limit_test.dart @@ -20,7 +20,7 @@ import '../shared/test_services.dart'; import 'backend_test_utils.dart'; void main() { - final refTime = clock.now().subtract(Duration(days: 2)); + final refTime = clock.now().subtract(Duration(days: 2 * 365)); group('Upload rate limit', () { Future upload({ @@ -63,6 +63,29 @@ environment: }) as R; } + Future _expectLimit({ + required int count, + required Duration shortGap, + required Duration longGap, + required String expectedMessage, + }) async { + for (var i = 0; i < count; i++) { + await upload(version: '1.0.$i', time: shortGap * i); + } + final rs = upload(version: '1.0.$count', time: shortGap * count); + await expectLater( + rs, + throwsA( + isA().having( + (e) => e.message, + 'message', + contains(expectedMessage), + ), + ), + ); + await upload(version: '1.0.$count', time: longGap); + } + testWithProfile( 'new versions 1 per minute', testProfile: TestProfile(packages: [], defaultUser: adminAtPubDevEmail), @@ -75,21 +98,12 @@ environment: burst: 1, ), ], - () async { - await upload(version: '1.0.0', time: Duration.zero); - final rs = upload(version: '1.0.1', time: Duration(seconds: 16)); - await expectLater( - rs, - throwsA( - isA().having( - (e) => e.message, - 'message', - contains('(1 in the last few minutes)'), - ), - ), - ); - await upload(version: '1.0.1', time: Duration(minutes: 3)); - }, + () => _expectLimit( + count: 1, + shortGap: Duration(seconds: 16), + longGap: Duration(minutes: 3), + expectedMessage: '(1 in the last few minutes)', + ), ); }, ); @@ -106,26 +120,12 @@ environment: hourly: 12, ), ], - () async { - for (var i = 0; i < 12; i++) { - await upload(version: '1.0.$i', time: Duration(minutes: i * 2)); - } - - final rs = - upload(version: '1.0.12', time: Duration(minutes: 12 * 2)); - await expectLater( - rs, - throwsA( - isA().having( - (e) => e.message, - 'message', - contains('(12 in the last hour)'), - ), - ), - ); - await upload( - version: '1.0.12', time: Duration(hours: 1, minutes: 1)); - }, + () => _expectLimit( + count: 12, + shortGap: Duration(minutes: 2), + longGap: Duration(minutes: 61), + expectedMessage: '(12 in the last hour)', + ), ); }, ); @@ -142,26 +142,100 @@ environment: daily: 24, ), ], - () async { - for (var i = 0; i < 24; i++) { - await upload(version: '1.0.$i', time: Duration(minutes: i * 10)); - } + () => _expectLimit( + count: 24, + shortGap: Duration(minutes: 10), + longGap: Duration(days: 1, minutes: 1), + expectedMessage: '(24 in the last day)', + ), + ); + }, + ); - final rs = - upload(version: '1.0.24', time: Duration(minutes: 24 * 10)); - await expectLater( - rs, - throwsA( - isA().having( - (e) => e.message, - 'message', - contains('(24 in the last day)'), - ), - ), - ); - await upload( - version: '1.0.24', time: Duration(days: 1, minutes: 1)); - }, + testWithProfile( + 'new versions 20 per week', + testProfile: TestProfile(packages: [], defaultUser: adminAtPubDevEmail), + fn: () async { + await _withRateLimits( + [ + RateLimit( + operation: AuditLogRecordKind.packagePublished, + scope: RateLimitScope.package, + weekly: 20, + ), + ], + () => _expectLimit( + count: 20, + shortGap: Duration(hours: 5), + longGap: Duration(days: 7, minutes: 1), + expectedMessage: '(20 in the last week)', + ), + ); + }, + ); + + testWithProfile( + 'new versions 20 per month', + testProfile: TestProfile(packages: [], defaultUser: adminAtPubDevEmail), + fn: () async { + await _withRateLimits( + [ + RateLimit( + operation: AuditLogRecordKind.packagePublished, + scope: RateLimitScope.package, + monthly: 20, + ), + ], + () => _expectLimit( + count: 20, + shortGap: Duration(days: 1), + longGap: Duration(days: 30, minutes: 1), + expectedMessage: '(20 in the last month)', + ), + ); + }, + ); + + testWithProfile( + 'new versions 20 per quarter', + testProfile: TestProfile(packages: [], defaultUser: adminAtPubDevEmail), + fn: () async { + await _withRateLimits( + [ + RateLimit( + operation: AuditLogRecordKind.packagePublished, + scope: RateLimitScope.package, + quarterly: 20, + ), + ], + () => _expectLimit( + count: 20, + shortGap: Duration(days: 3), + longGap: Duration(days: 91, minutes: 1), + expectedMessage: '(20 in the last 3 months)', + ), + ); + }, + ); + + testWithProfile( + 'new versions 20 per year', + testProfile: TestProfile(packages: [], defaultUser: adminAtPubDevEmail), + fn: () async { + await _withRateLimits( + [ + RateLimit( + operation: AuditLogRecordKind.packagePublished, + scope: RateLimitScope.package, + yearly: 20, + ), + ], + () => _expectLimit( + count: 20, + shortGap: Duration(days: 3), + longGap: Duration(days: 365, minutes: 1), + expectedMessage: '(20 in the last year)', + ), ); }, ); diff --git a/app/test/shared/configuration_test.dart b/app/test/shared/configuration_test.dart index b5ce6048e4..8bbfebd305 100644 --- a/app/test/shared/configuration_test.dart +++ b/app/test/shared/configuration_test.dart @@ -5,6 +5,7 @@ import 'dart:convert'; import 'dart:io'; +import 'package:pub_dev/audit/models.dart'; import 'package:pub_dev/shared/configuration.dart'; import 'package:test/test.dart'; import 'package:yaml/yaml.dart' as yaml; @@ -73,7 +74,21 @@ void main() { // no duplicate rules expect(rateLimits.map((e) => '${e.operation}/${e.scope}').toSet().length, rateLimits.length); - // some rules for prod config + // only package-uploaded rules have long-term rules + for (final limit in rateLimits) { + if (limit.weekly != null || + limit.monthly != null || + limit.quarterly != null || + limit.yearly != null) { + if (limit.operation == AuditLogRecordKind.packagePublished) { + // uploads are handling custom timestamps + } else { + fail( + 'Rate limit operation "${limit.operation}" does not support long-term rules.'); + } + } + } + // some rules are present in the prod config if (config.isProduction) { expect(rateLimits, hasLength(greaterThan(10))); }