diff --git a/config/nginx.conf.erb b/config/nginx.conf.erb index e326382787b..f5d8158704b 100644 --- a/config/nginx.conf.erb +++ b/config/nginx.conf.erb @@ -176,8 +176,6 @@ http { client_body_timeout 30; client_max_body_size 50m; - limit_req_zone $remote_addr zone=publish:10m rate=1r/m; - upstream app_server { server localhost:8888 fail_timeout=0; } @@ -261,12 +259,5 @@ http { proxy_pass http://app_server; } <% end %> - - location ~ ^/api/v./crates/new$ { - proxy_pass http://app_server; - - limit_req zone=publish burst=30 nodelay; - limit_req_status 429; - } } } diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 1e42e659786..8325ba1bf14 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -11,6 +11,7 @@ use crate::models::{ NewVersion, Rights, VersionAction, }; +use crate::rate_limiter::LimitedAction; use crate::render; use crate::schema::*; use crate::util::errors::{cargo_err, AppResult}; @@ -72,6 +73,16 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult { )) })?; + // Check the rate limits on the endpoint. Publishing a new crate uses a different (stricter) + // limiting pool than publishing a new version of an existing crate to prevent mass squatting. + let limited_action = if Crate::by_name(&new_crate.name).execute(&*conn)? == 0 { + LimitedAction::PublishNew + } else { + LimitedAction::PublishExisting + }; + app.rate_limiter + .check_rate_limit(user.id, limited_action, &conn)?; + // Create a transaction on the database, if there are no errors, // commit the transactions to record a new or updated crate. conn.transaction(|| { @@ -107,7 +118,7 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult { }; let license_file = new_crate.license_file.as_deref(); - let krate = persist.create_or_update(&conn, user.id, Some(&app.rate_limiter))?; + let krate = persist.create_or_update(&conn, user.id)?; let owners = krate.owners(&conn)?; if user.rights(req.app(), &owners)? < Rights::Publish { diff --git a/src/downloads_counter.rs b/src/downloads_counter.rs index 9ff203ad9bb..e4168069cdf 100644 --- a/src/downloads_counter.rs +++ b/src/downloads_counter.rs @@ -438,7 +438,7 @@ mod tests { name: "foo", ..NewCrate::default() } - .create_or_update(conn, user.id, None) + .create_or_update(conn, user.id) .expect("failed to create crate"); Self { diff --git a/src/models/krate.rs b/src/models/krate.rs index 9466ba805d1..fff4868459c 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -15,7 +15,6 @@ use crate::models::{ use crate::util::errors::{cargo_err, AppResult}; use crate::models::helpers::with_count::*; -use crate::rate_limiter::{LimitedAction, RateLimiter}; use crate::schema::*; #[derive(Debug, Queryable, Identifiable, Associations, Clone, Copy)] @@ -93,12 +92,7 @@ pub struct NewCrate<'a> { } impl<'a> NewCrate<'a> { - pub fn create_or_update( - self, - conn: &PgConnection, - uploader: i32, - rate_limit: Option<&RateLimiter>, - ) -> AppResult { + pub fn create_or_update(self, conn: &PgConnection, uploader: i32) -> AppResult { use diesel::update; self.validate()?; @@ -108,9 +102,6 @@ impl<'a> NewCrate<'a> { // To avoid race conditions, we try to insert // first so we know whether to add an owner if let Some(krate) = self.save_new_crate(conn, uploader)? { - if let Some(rate_limit) = rate_limit { - rate_limit.check_rate_limit(uploader, LimitedAction::PublishNew, conn)?; - } return Ok(krate); } diff --git a/src/rate_limiter.rs b/src/rate_limiter.rs index 31d1b3d9b33..70353fa0969 100644 --- a/src/rate_limiter.rs +++ b/src/rate_limiter.rs @@ -9,6 +9,7 @@ use std::time::Duration; crate::pg_enum! { pub enum LimitedAction { PublishNew = 0, + PublishExisting = 1, } } @@ -17,6 +18,7 @@ impl LimitedAction { pub fn default_rate_seconds(&self) -> u64 { match self { LimitedAction::PublishNew => 60 * 10, + LimitedAction::PublishExisting => 60, } } @@ -24,6 +26,7 @@ impl LimitedAction { pub fn default_burst(&self) -> i32 { match self { LimitedAction::PublishNew => 5, + LimitedAction::PublishExisting => 30, } } @@ -32,6 +35,9 @@ impl LimitedAction { LimitedAction::PublishNew => { "You have published too many new crates in a short period of time." } + LimitedAction::PublishExisting => { + "You have published too many versions of existing crates in a short period of time." + } } } @@ -39,6 +45,7 @@ impl LimitedAction { pub fn env_var_key(&self) -> &'static str { match self { LimitedAction::PublishNew => "PUBLISH_NEW", + LimitedAction::PublishExisting => "PUBLISH_EXISTING", } } } @@ -350,6 +357,49 @@ mod tests { Ok(()) } + #[test] + fn two_actions_dont_interfere_with_each_other() -> QueryResult<()> { + let conn = pg_connection(); + let now = now(); + let user_id = new_user(&conn, "user")?; + + let mut config = HashMap::new(); + config.insert( + LimitedAction::PublishNew, + RateLimiterConfig { + rate: Duration::from_secs(1), + burst: 10, + }, + ); + config.insert( + LimitedAction::PublishExisting, + RateLimiterConfig { + rate: Duration::from_secs(1), + burst: 20, + }, + ); + let rate = RateLimiter::new(config); + + let refill_time = now + chrono::Duration::seconds(4); + assert_eq!( + 10, + rate.take_token(user_id, LimitedAction::PublishNew, refill_time, &conn)? + .tokens, + ); + assert_eq!( + 9, + rate.take_token(user_id, LimitedAction::PublishNew, refill_time, &conn)? + .tokens, + ); + assert_eq!( + 20, + rate.take_token(user_id, LimitedAction::PublishExisting, refill_time, &conn)? + .tokens, + ); + + Ok(()) + } + #[test] fn override_is_used_instead_of_global_burst_if_present() -> QueryResult<()> { let conn = pg_connection(); @@ -375,6 +425,47 @@ mod tests { Ok(()) } + #[test] + fn override_is_different_for_each_action() -> QueryResult<()> { + let conn = pg_connection(); + let now = now(); + let user_id = new_user(&conn, "user")?; + + let mut config = HashMap::new(); + for action in &[LimitedAction::PublishNew, LimitedAction::PublishExisting] { + config.insert( + *action, + RateLimiterConfig { + rate: Duration::from_secs(1), + burst: 10, + }, + ); + } + let rate = RateLimiter::new(config); + + diesel::insert_into(publish_rate_overrides::table) + .values(( + publish_rate_overrides::user_id.eq(user_id), + publish_rate_overrides::action.eq(LimitedAction::PublishNew), + publish_rate_overrides::burst.eq(20), + )) + .execute(&conn)?; + + let refill_time = now + chrono::Duration::seconds(4); + assert_eq!( + 20, + rate.take_token(user_id, LimitedAction::PublishNew, refill_time, &conn)? + .tokens, + ); + assert_eq!( + 10, + rate.take_token(user_id, LimitedAction::PublishExisting, refill_time, &conn)? + .tokens, + ); + + Ok(()) + } + #[test] fn overrides_can_expire() -> QueryResult<()> { let conn = pg_connection(); diff --git a/src/tasks/update_downloads.rs b/src/tasks/update_downloads.rs index 925ce3c5502..252a7071125 100644 --- a/src/tasks/update_downloads.rs +++ b/src/tasks/update_downloads.rs @@ -98,7 +98,7 @@ mod test { name: "foo", ..Default::default() } - .create_or_update(conn, user_id, None) + .create_or_update(conn, user_id) .unwrap(); let version = NewVersion::new( krate.id, diff --git a/src/tests/builders/krate.rs b/src/tests/builders/krate.rs index 5669567587c..f1323fa8e02 100644 --- a/src/tests/builders/krate.rs +++ b/src/tests/builders/krate.rs @@ -114,9 +114,7 @@ impl<'a> CrateBuilder<'a> { pub fn build(mut self, connection: &PgConnection) -> AppResult { use diesel::{insert_into, select, update}; - let mut krate = self - .krate - .create_or_update(connection, self.owner_id, None)?; + let mut krate = self.krate.create_or_update(connection, self.owner_id)?; // Since we are using `NewCrate`, we can't set all the // crate properties in a single DB call. diff --git a/src/tests/http-data/krate_publish_publish_existing_crates_rate_limit b/src/tests/http-data/krate_publish_publish_existing_crates_rate_limit new file mode 100644 index 00000000000..20c7b357b17 --- /dev/null +++ b/src/tests/http-data/krate_publish_publish_existing_crates_rate_limit @@ -0,0 +1,204 @@ +[ + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/rate_limited/rate_limited-1.0.0.crate", + "method": "PUT", + "headers": [ + [ + "content-length", + "35" + ], + [ + "content-type", + "application/x-tar" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "authorization", + "AWS AKIAICL5IWUZYWWKA7JA:3P5wvArAHvV7o8atB0gUc0RTCCc=" + ], + [ + "accept", + "*/*" + ], + [ + "date", + "Fri, 15 Sep 2017 07:53:06 -0700" + ], + [ + "host", + "alexcrichton-test.s3.amazonaws.com" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [ + [ + "x-amz-id-2", + "9ZLFTsEwh2iNu+BlWzaTZ85mFA7pxZgsGhQCj3Qi67LqT/iB5eiCOJQPYww2BEkoivJbr0mWruo=" + ], + [ + "date", + "Fri, 15 Sep 2017 14:53:07 GMT" + ], + [ + "content-length", + "0" + ], + [ + "x-amz-request-id", + "FD643F2FC49A7DF3" + ], + [ + "Server", + "AmazonS3" + ], + [ + "ETag", + "\"f9016ad360cebb4fe2e6e96e5949f022\"" + ] + ], + "body": "" + } + }, + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/rate_limited/rate_limited-1.0.1.crate", + "method": "PUT", + "headers": [ + [ + "content-length", + "35" + ], + [ + "content-type", + "application/x-tar" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "authorization", + "AWS AKIAICL5IWUZYWWKA7JA:3P5wvArAHvV7o8atB0gUc0RTCCc=" + ], + [ + "accept", + "*/*" + ], + [ + "date", + "Fri, 15 Sep 2017 07:53:06 -0700" + ], + [ + "host", + "alexcrichton-test.s3.amazonaws.com" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [ + [ + "x-amz-id-2", + "9ZLFTsEwh2iNu+BlWzaTZ85mFA7pxZgsGhQCj3Qi67LqT/iB5eiCOJQPYww2BEkoivJbr0mWruo=" + ], + [ + "date", + "Fri, 15 Sep 2017 14:53:07 GMT" + ], + [ + "content-length", + "0" + ], + [ + "x-amz-request-id", + "FD643F2FC49A7DF3" + ], + [ + "Server", + "AmazonS3" + ], + [ + "ETag", + "\"f9016ad360cebb4fe2e6e96e5949f022\"" + ] + ], + "body": "" + } + }, + { + "request": { + "uri": + "http://alexcrichton-test.s3.amazonaws.com/crates/rate_limited/rate_limited-1.0.3.crate", + "method": "PUT", + "headers": [ + [ + "content-length", + "35" + ], + [ + "content-type", + "application/x-tar" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "authorization", + "AWS AKIAICL5IWUZYWWKA7JA:3P5wvArAHvV7o8atB0gUc0RTCCc=" + ], + [ + "accept", + "*/*" + ], + [ + "date", + "Fri, 15 Sep 2017 07:53:06 -0700" + ], + [ + "host", + "alexcrichton-test.s3.amazonaws.com" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [ + [ + "x-amz-id-2", + "9ZLFTsEwh2iNu+BlWzaTZ85mFA7pxZgsGhQCj3Qi67LqT/iB5eiCOJQPYww2BEkoivJbr0mWruo=" + ], + [ + "date", + "Fri, 15 Sep 2017 14:53:07 GMT" + ], + [ + "content-length", + "0" + ], + [ + "x-amz-request-id", + "FD643F2FC49A7DF3" + ], + [ + "Server", + "AmazonS3" + ], + [ + "ETag", + "\"f9016ad360cebb4fe2e6e96e5949f022\"" + ] + ], + "body": "" + } + } +] diff --git a/src/tests/http-data/krate_publish_publish_existing_crates_rate_limit_doesnt_affect_new_crates b/src/tests/http-data/krate_publish_publish_existing_crates_rate_limit_doesnt_affect_new_crates new file mode 100644 index 00000000000..81a309d6b33 --- /dev/null +++ b/src/tests/http-data/krate_publish_publish_existing_crates_rate_limit_doesnt_affect_new_crates @@ -0,0 +1,203 @@ +[ + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/rate_limited1/rate_limited1-1.0.0.crate", + "method": "PUT", + "headers": [ + [ + "content-length", + "35" + ], + [ + "content-type", + "application/x-tar" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "authorization", + "AWS AKIAICL5IWUZYWWKA7JA:3P5wvArAHvV7o8atB0gUc0RTCCc=" + ], + [ + "accept", + "*/*" + ], + [ + "date", + "Fri, 15 Sep 2017 07:53:06 -0700" + ], + [ + "host", + "alexcrichton-test.s3.amazonaws.com" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [ + [ + "x-amz-id-2", + "9ZLFTsEwh2iNu+BlWzaTZ85mFA7pxZgsGhQCj3Qi67LqT/iB5eiCOJQPYww2BEkoivJbr0mWruo=" + ], + [ + "date", + "Fri, 15 Sep 2017 14:53:07 GMT" + ], + [ + "content-length", + "0" + ], + [ + "x-amz-request-id", + "FD643F2FC49A7DF3" + ], + [ + "Server", + "AmazonS3" + ], + [ + "ETag", + "\"f9016ad360cebb4fe2e6e96e5949f022\"" + ] + ], + "body": "" + } + }, + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/rate_limited1/rate_limited1-1.0.1.crate", + "method": "PUT", + "headers": [ + [ + "content-length", + "35" + ], + [ + "content-type", + "application/x-tar" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "authorization", + "AWS AKIAICL5IWUZYWWKA7JA:3P5wvArAHvV7o8atB0gUc0RTCCc=" + ], + [ + "accept", + "*/*" + ], + [ + "date", + "Fri, 15 Sep 2017 07:53:06 -0700" + ], + [ + "host", + "alexcrichton-test.s3.amazonaws.com" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [ + [ + "x-amz-id-2", + "9ZLFTsEwh2iNu+BlWzaTZ85mFA7pxZgsGhQCj3Qi67LqT/iB5eiCOJQPYww2BEkoivJbr0mWruo=" + ], + [ + "date", + "Fri, 15 Sep 2017 14:53:07 GMT" + ], + [ + "content-length", + "0" + ], + [ + "x-amz-request-id", + "FD643F2FC49A7DF3" + ], + [ + "Server", + "AmazonS3" + ], + [ + "ETag", + "\"f9016ad360cebb4fe2e6e96e5949f022\"" + ] + ], + "body": "" + } + }, + { + "request": { + "uri": "http://alexcrichton-test.s3.amazonaws.com/crates/rate_limited2/rate_limited2-1.0.0.crate", + "method": "PUT", + "headers": [ + [ + "content-length", + "35" + ], + [ + "content-type", + "application/x-tar" + ], + [ + "accept-encoding", + "gzip" + ], + [ + "authorization", + "AWS AKIAICL5IWUZYWWKA7JA:3P5wvArAHvV7o8atB0gUc0RTCCc=" + ], + [ + "accept", + "*/*" + ], + [ + "date", + "Fri, 15 Sep 2017 07:53:06 -0700" + ], + [ + "host", + "alexcrichton-test.s3.amazonaws.com" + ] + ], + "body": "H4sIAAAAAAAA/+3AAQEAAACCIP+vbkhQwKsBLq+17wAEAAA=" + }, + "response": { + "status": 200, + "headers": [ + [ + "x-amz-id-2", + "9ZLFTsEwh2iNu+BlWzaTZ85mFA7pxZgsGhQCj3Qi67LqT/iB5eiCOJQPYww2BEkoivJbr0mWruo=" + ], + [ + "date", + "Fri, 15 Sep 2017 14:53:07 GMT" + ], + [ + "content-length", + "0" + ], + [ + "x-amz-request-id", + "FD643F2FC49A7DF3" + ], + [ + "Server", + "AmazonS3" + ], + [ + "ETag", + "\"f9016ad360cebb4fe2e6e96e5949f022\"" + ] + ], + "body": "" + } + } +] diff --git a/src/tests/http-data/krate_publish_publish_new_crate_rate_limited b/src/tests/http-data/krate_publish_publish_new_crates_rate_limit similarity index 100% rename from src/tests/http-data/krate_publish_publish_new_crate_rate_limited rename to src/tests/http-data/krate_publish_publish_new_crates_rate_limit diff --git a/src/tests/http-data/krate_publish_publish_rate_limit_doesnt_affect_existing_crates b/src/tests/http-data/krate_publish_publish_new_crates_rate_limit_doesnt_affect_existing_crates similarity index 100% rename from src/tests/http-data/krate_publish_publish_rate_limit_doesnt_affect_existing_crates rename to src/tests/http-data/krate_publish_publish_new_crates_rate_limit_doesnt_affect_existing_crates diff --git a/src/tests/krate/publish.rs b/src/tests/krate/publish.rs index 54f72402974..2d7dc20bd5d 100644 --- a/src/tests/krate/publish.rs +++ b/src/tests/krate/publish.rs @@ -913,7 +913,7 @@ fn new_krate_tarball_with_hard_links() { } #[test] -fn publish_new_crate_rate_limited() { +fn publish_new_crates_rate_limit() { let (app, anon, _, token) = TestApp::full() .with_rate_limit(LimitedAction::PublishNew, Duration::from_millis(500), 1) .with_token(); @@ -942,7 +942,7 @@ fn publish_new_crate_rate_limited() { } #[test] -fn publish_rate_limit_doesnt_affect_existing_crates() { +fn publish_new_crates_rate_limit_doesnt_affect_existing_crates() { let (app, _, _, token) = TestApp::full() .with_rate_limit(LimitedAction::PublishNew, Duration::from_millis(500), 1) .with_token(); @@ -955,3 +955,70 @@ fn publish_rate_limit_doesnt_affect_existing_crates() { token.enqueue_publish(new_version).good(); app.run_pending_background_jobs(); } + +#[test] +fn publish_existing_crates_rate_limit() { + let (app, anon, _, token) = TestApp::full() + .with_rate_limit( + LimitedAction::PublishExisting, + Duration::from_millis(500), + 1, + ) + .with_token(); + + // Upload a new crate (uses a different rate limit not tested here) + let crate_to_publish = PublishBuilder::new("rate_limited").version("1.0.0"); + token.enqueue_publish(crate_to_publish).good(); + + // Upload a new version of the crate + let crate_to_publish = PublishBuilder::new("rate_limited").version("1.0.1"); + token.enqueue_publish(crate_to_publish).good(); + + // Uploading a second version of the crate is limited + let crate_to_publish = PublishBuilder::new("rate_limited").version("1.0.2"); + let response = token.enqueue_publish(crate_to_publish); + assert_eq!(response.status(), StatusCode::TOO_MANY_REQUESTS); + app.run_pending_background_jobs(); + + let json = anon.show_crate("rate_limited"); + assert_eq!(json.krate.max_version, "1.0.1"); + + // Wait for the limit to be up + thread::sleep(Duration::from_millis(500)); + + let crate_to_publish = PublishBuilder::new("rate_limited").version("1.0.3"); + token.enqueue_publish(crate_to_publish).good(); + + let json = anon.show_crate("rate_limited"); + assert_eq!(json.krate.max_version, "1.0.3"); +} + +#[test] +fn publish_existing_crates_rate_limit_doesnt_affect_new_crates() { + let (app, _, _, token) = TestApp::full() + .with_rate_limit( + LimitedAction::PublishExisting, + Duration::from_millis(500), + 1, + ) + .with_token(); + + // Upload a new crate (uses a different rate limit not tested here) + let crate_to_publish = PublishBuilder::new("rate_limited1").version("1.0.0"); + token.enqueue_publish(crate_to_publish).good(); + + // Upload a new version of the crate + let crate_to_publish = PublishBuilder::new("rate_limited1").version("1.0.1"); + token.enqueue_publish(crate_to_publish).good(); + + // Uploading a second version of the crate is limited + let crate_to_publish = PublishBuilder::new("rate_limited1").version("1.0.2"); + let response = token.enqueue_publish(crate_to_publish); + assert_eq!(response.status(), StatusCode::TOO_MANY_REQUESTS); + app.run_pending_background_jobs(); + + // Uploading another crate should still work, as the rate limit is separate + let crate_to_publish = PublishBuilder::new("rate_limited2"); + token.enqueue_publish(crate_to_publish).good(); + app.run_pending_background_jobs(); +}