diff --git a/src/db/spanner/models.rs b/src/db/spanner/models.rs index 4029a147..69b23654 100644 --- a/src/db/spanner/models.rs +++ b/src/db/spanner/models.rs @@ -739,8 +739,8 @@ impl SpannerDb { "INSERT INTO user_collections (fxa_uid, fxa_kid, collection_id, modified) VALUES (@fxa_uid, @fxa_kid, @collection_id, @modified)", )? - .params(params.clone()) - .param_types(types.clone()) + .params(params) + .param_types(types) .execute(&self.conn)?; // Return timestamp, because sometimes there's a delay between writing and // reading the database. @@ -928,25 +928,25 @@ impl SpannerDb { let mut sqltypes = HashMap::new(); if let Some(older) = older { - query = format!("{} AND modified < @older", query).to_string(); + query = format!("{} AND modified < @older", query); sqlparams.insert("older".to_string(), as_value(older.as_rfc3339()?)); sqltypes.insert("older".to_string(), as_type(TypeCode::TIMESTAMP)); } if let Some(newer) = newer { - query = format!("{} AND modified > @newer", query).to_string(); + query = format!("{} AND modified > @newer", query); sqlparams.insert("newer".to_string(), as_value(newer.as_rfc3339()?)); sqltypes.insert("newer".to_string(), as_type(TypeCode::TIMESTAMP)); } if !ids.is_empty() { - query = format!("{} AND bso_id IN UNNEST(@ids)", query).to_string(); + query = format!("{} AND bso_id IN UNNEST(@ids)", query); sqlparams.insert("ids".to_owned(), as_list_value(ids.into_iter())); } query = match sort { - Sorting::Index => format!("{} ORDER BY sortindex DESC", query).to_string(), - Sorting::Newest => format!("{} ORDER BY modified DESC", query).to_string(), - Sorting::Oldest => format!("{} ORDER BY modified ASC", query).to_string(), + Sorting::Index => format!("{} ORDER BY sortindex DESC", query), + Sorting::Newest => format!("{} ORDER BY modified DESC", query), + Sorting::Oldest => format!("{} ORDER BY modified ASC", query), _ => query, }; @@ -967,7 +967,7 @@ impl SpannerDb { if offset != 0 { // XXX: copy over this optimization: // https://github.com/mozilla-services/server-syncstorage/blob/a0f8117/syncstorage/storage/sql/__init__.py#L404 - query = format!("{} OFFSET {}", query, offset).to_string(); + query = format!("{} OFFSET {}", query, offset); } self.sql(&query)? diff --git a/src/db/spanner/pool.rs b/src/db/spanner/pool.rs index f329ba2e..256877c3 100644 --- a/src/db/spanner/pool.rs +++ b/src/db/spanner/pool.rs @@ -82,7 +82,7 @@ impl SpannerDbPool { .build(), ), coll_cache: Default::default(), - metrics: metrics.clone(), + metrics, }) } diff --git a/src/settings.rs b/src/settings.rs index b9cf5360..a165e983 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -112,7 +112,7 @@ impl Settings { Ok(s) => { // Adjust the max values if required. if s.uses_spanner() { - let mut ms = s.clone(); + let mut ms = s; ms.limits.max_total_bytes = min(ms.limits.max_total_bytes, MAX_SPANNER_LOAD_SIZE as u32); return Ok(ms); diff --git a/src/web/error.rs b/src/web/error.rs index a3686e88..2735bab5 100644 --- a/src/web/error.rs +++ b/src/web/error.rs @@ -107,6 +107,7 @@ impl From> for HawkError { impl From> for ValidationError { fn from(inner: Context) -> Self { + debug!("Validation Error: {:?}", inner.get_context()); let status = match inner.get_context() { ValidationErrorKind::FromDetails(ref _description, ref location, Some(ref name)) if *location == RequestErrorLocation::Header => diff --git a/src/web/extractors.rs b/src/web/extractors.rs index 70ba9861..ae66fc80 100644 --- a/src/web/extractors.rs +++ b/src/web/extractors.rs @@ -8,7 +8,7 @@ use actix_web::{ dev::{ConnectionInfo, Extensions, Payload}, error::ErrorInternalServerError, http::{ - header::{qitem, Accept, ContentType, Header, HeaderMap}, + header::{qitem, Accept, ContentType, Header, HeaderMap, HeaderValue}, Uri, }, web::{Json, Query}, @@ -367,8 +367,29 @@ impl FromRequest for BsoBody { let max_payload_size = state.limits.max_record_payload_bytes as usize; + // The `meta/global` BSO is special in that it's used as a command to + // reset the timestamp. A possible client bug is sending an empty + // body. Handle it, but issue an info so we can track the event. + if req.path().ends_with("/meta/global") + && req + .headers() + .get("content-length") + .unwrap_or(&HeaderValue::from(0)) + == HeaderValue::from(0) + { + info!("⚠️ Got an empty meta BSO"); + return Box::new(future::ok(BsoBody { + id: None, + sortindex: None, + payload: None, + ttl: None, + _ignored_modified: None, + })); + } + let fut = >::from_request(&req, payload) .map_err(|e| { + warn!("⚠️ Could not parse BSO Body: {:?}", e); let err: ApiError = ValidationErrorKind::FromDetails( e.to_string(), RequestErrorLocation::Body, @@ -421,6 +442,7 @@ impl BsoParam { let elements: Vec<&str> = uri.path().split('/').collect(); let elem = elements.get(3); if elem.is_none() || elem != Some(&"storage") || elements.len() != 6 { + warn!("⚠️ Unexpected BSO URI: {:?}", uri.path()); return Err(ValidationErrorKind::FromDetails( "Invalid BSO".to_owned(), RequestErrorLocation::Path, @@ -429,7 +451,7 @@ impl BsoParam { } if let Some(v) = elements.get(5) { let sv = String::from_str(v).map_err(|e| { - debug!("⚠️ BsoParam Error element:{:?} error:{:?}", v, e); + warn!("⚠️ BsoParam Error element:{:?} error:{:?}", v, e); ValidationErrorKind::FromDetails( "Invalid BSO".to_owned(), RequestErrorLocation::Path, @@ -438,6 +460,7 @@ impl BsoParam { })?; Ok(Self { bso: sv }) } else { + warn!("⚠️ Missing BSO: {:?}", uri.path()); Err(ValidationErrorKind::FromDetails( "Missing BSO".to_owned(), RequestErrorLocation::Path, @@ -674,7 +697,7 @@ impl FromRequest for CollectionPostRequest { BsoBodies, )>::from_request(&req, payload) .and_then(move |(user_id, db, collection, query, mut bsos)| { - let collection = collection.collection.clone(); + let collection = collection.collection; if collection == "crypto" { // Verify the client didn't mess up the crypto if we have a payload for bso in &bsos.valid { @@ -746,9 +769,7 @@ impl FromRequest for BsoRequest { let user_id = HawkIdentifier::from_request(req, payload)?; let db = >::from_request(req, payload)?; let query = BsoQueryParams::from_request(req, payload)?; - let collection = CollectionParam::from_request(req, payload)? - .collection - .clone(); + let collection = CollectionParam::from_request(req, payload)?.collection; let bso = BsoParam::from_request(req, payload)?; Ok(BsoRequest { @@ -756,7 +777,7 @@ impl FromRequest for BsoRequest { db, user_id, query, - bso: bso.bso.clone(), + bso: bso.bso, metrics: metrics::Metrics::from(req), }) } @@ -792,7 +813,7 @@ impl FromRequest for BsoPutRequest { BsoBody, )>::from_request(req, payload) .and_then(|(user_id, db, collection, query, bso, body)| { - let collection = collection.collection.clone(); + let collection = collection.collection; if collection == "crypto" { // Verify the client didn't mess up the crypto if we have a payload if let Some(ref data) = body.payload { @@ -813,7 +834,7 @@ impl FromRequest for BsoPutRequest { db, user_id, query, - bso: bso.bso.clone(), + bso: bso.bso, body, metrics, }) @@ -897,7 +918,7 @@ impl HawkIdentifier { let elements: Vec<&str> = uri.path().split('/').collect(); if let Some(v) = elements.get(2) { u64::from_str(v).map_err(|e| { - debug!("⚠️ HawkIdentifier Error {:?} {:?}", v, e); + info!("⚠️ HawkIdentifier Error invalid UID {:?} {:?}", v, e); ValidationErrorKind::FromDetails( "Invalid UID".to_owned(), RequestErrorLocation::Path, @@ -906,6 +927,7 @@ impl HawkIdentifier { .into() }) } else { + info!("⚠️ HawkIdentifier Error missing UID {:?}", uri); Err(ValidationErrorKind::FromDetails( "Missing UID".to_owned(), RequestErrorLocation::Path, @@ -947,7 +969,9 @@ impl HawkIdentifier { uri: &Uri, ) -> Result { let payload = HawkPayload::extrude(header, method, secrets, connection_info, uri)?; - if payload.user_id != Self::uid_from_path(&uri)? { + let puid = Self::uid_from_path(&uri)?; + if payload.user_id != puid { + info!("⚠️ Hawk UID not in URI: {:?} {:?}", payload.user_id, uri); Err(ValidationErrorKind::FromDetails( "conflicts with payload".to_owned(), RequestErrorLocation::Path, @@ -1143,7 +1167,7 @@ impl FromRequest for BatchRequestOpt { })?, None => continue, }; - let count = value.parse::<(u32)>().map_err(|_| { + let count = value.parse::().map_err(|_| { let err: ApiError = ValidationErrorKind::FromDetails( format!("Invalid integer value: {}", value), RequestErrorLocation::Header, diff --git a/src/web/middleware.rs b/src/web/middleware.rs index 22f105e7..29f9d054 100644 --- a/src/web/middleware.rs +++ b/src/web/middleware.rs @@ -402,7 +402,7 @@ where } }; let bso = BsoParam::extrude(&sreq.uri(), &mut sreq.extensions_mut()).ok(); - let bso_opt = bso.clone().map(|b| b.bso); + let bso_opt = bso.map(|b| b.bso); let mut service = Rc::clone(&self.service); Box::new(