crate_owner_invitation: Reject invalid names in PUT /api/v1/me/crate_owner_invitations/{crate_id}

This reject requests with invalid names such as those containing null
bytes before the database lookup. This prevents PostgreSQL errors from
surfacing as 500 responses and returns a 404 Not Found instead.

Author: eth3lbert

src/controllers/crate_owner_invitation.rs

@@ -7,14 +7,17 @@ use crate::models::{Crate, CrateOwnerInvitation, PublicUser};
 use crate::schema::{crate_owner_invitations, crates, users};
 use crate::server::ServerContext;
 use crate::util::RequestUtils;
-use crate::util::errors::{AppResult, BoxedAppError, bad_request, custom, forbidden, internal};
+use crate::util::errors::{
+    AppResult, BoxedAppError, bad_request, crate_not_found, custom, forbidden, internal,
+};
 use crate::util::no_store;
 use crate::views::{EncodableCrateOwnerInvitation, EncodablePublicUser, InvitationResponse};
 use axum::Json;
 use axum::extract::{FromRequestParts, Path, Query};
 use axum_extra::TypedHeader;
 use axum_extra::headers::CacheControl;
 use chrono::Utc;
+use crates_io_validation::validate_crate_name;
 use diesel::pg::Pg;
 use diesel::prelude::*;
 use diesel::sql_types::Bool;
@@ -350,7 +353,7 @@ pub struct InvitationData {
 )]
 pub async fn handle_crate_owner_invitation(
     ctx: ServerContext,
-    Path(path): Path<CrateRefPath>,
+    path: CrateRefPath,
     parts: Parts,
     Json(crate_invite): Json<OwnerInvitation>,
 ) -> AppResult<Json<HandleResponse>> {
@@ -448,6 +451,30 @@ impl From<AcceptError> for BoxedAppError {
     }
 }
 
+impl<S: Send + Sync> FromRequestParts<S> for CrateRefPath {
+    type Rejection = BoxedAppError;
+
+    async fn from_request_parts(parts: &mut Parts, state: &S) -> Result<Self, Self::Rejection> {
+        let Path(path) = Path::<Self>::from_request_parts(parts, state)
+            .await
+            .map_err(|err| custom(err.status(), err.body_text()))?;
+
+        // If the name is not a valid crate name it cannot exist in the
+        // database, so we skip the lookup and return a regular "not found"
+        // response. This also avoids passing invalid input (e.g. names
+        // containing null bytes) to the database layer, where PostgreSQL would
+        // reject the query with a confusing `invalid byte sequence for encoding
+        // "UTF8": 0x00` error and cause a 500 response.
+        if let CrateRef::Name(ref crate_name) = path.crate_id
+            && validate_crate_name("crate", crate_name).is_err()
+        {
+            return Err(crate_not_found(crate_name));
+        }
+
+        Ok(path)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::CrateRef;

src/tests/owners.rs

@@ -716,6 +716,70 @@ async fn test_invitation_crate_id_mismatch() {
     assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"`crate_id` mismatch"}]}"#);
 }
 
+#[tokio::test(flavor = "multi_thread")]
+async fn test_invitation_with_null_byte() {
+    let (app, _anon, _owner) = TestApp::full().with_user().await;
+    let invited_user = app.db_new_user("user_bar").await;
+
+    struct Data<T> {
+        crate_id: T,
+        accepted: bool,
+    }
+    fn make_body<T: Serialize>(Data { crate_id, accepted }: Data<T>) -> serde_json::Value {
+        json!({
+            "crate_owner_invite": {
+                "invited_by_username": "",
+                "crate_name": "foobar",
+                "crate_id": crate_id,
+                "created_at": "",
+                "accepted": accepted
+            }
+        })
+    }
+
+    // "foo%00bar" in the path, "foobar" in the body
+
+    let url = "/api/v1/me/crate_owner_invitations/foo%00bar";
+    let body = make_body(Data {
+        crate_id: "foobar",
+        accepted: true,
+    });
+    let response = invited_user.put::<()>(url, body.to_string()).await;
+    assert_snapshot!(response.status(), @"404 Not Found");
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"crate `foo\u0000bar` does not exist"}]}"#);
+
+    let url = "/api/v1/me/crate_owner_invitations/foo%00bar";
+    let body = make_body(Data {
+        crate_id: "foobar",
+        accepted: false,
+    });
+    let response = invited_user.put::<()>(url, body.to_string()).await;
+    assert_snapshot!(response.status(), @"404 Not Found");
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"crate `foo\u0000bar` does not exist"}]}"#);
+
+    // "foobar" in the path, "foo%00bar" in the body
+    // this will be treated as a mismatch and return 400 instead of 404 because we only validate the
+    // name in the path.
+
+    let url = "/api/v1/me/crate_owner_invitations/foobar";
+    let body = make_body(Data {
+        crate_id: "foo%00bar",
+        accepted: true,
+    });
+    let response = invited_user.put::<()>(url, body.to_string()).await;
+    assert_snapshot!(response.status(), @"400 Bad Request");
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"`crate_id` mismatch"}]}"#);
+
+    let url = "/api/v1/me/crate_owner_invitations/foobar";
+    let body = make_body(Data {
+        crate_id: "foo%00bar",
+        accepted: false,
+    });
+    let response = invited_user.put::<()>(url, body.to_string()).await;
+    assert_snapshot!(response.status(), @"400 Bad Request");
+    assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"`crate_id` mismatch"}]}"#);
+}
+
 /// Hacky way to simulate the expiration of an ownership invitation. Instead of letting a month
 /// pass, the creation date of the invite is moved back a month.
 pub async fn expire_invitation(app: &TestApp, crate_id: i32) {