Include client key in a header, not the URL

Since this value is used both for identification and authentication, it
shouldn't be in the URL where it might be logged or otherwise
discovered.

docs/src/sync-protocol.md

@@ -69,10 +69,11 @@ The transactions above are realized for an HTTP server at `<origin>` using the H
 The `origin` *should* be an HTTPS endpoint on general principle, but nothing in the functonality or security of the protocol depends on connection encryption.
 
 The replica identifies itself to the server using a `clientKey` in the form of a UUID.
+This value is passed with every request in the `X-Client-Id` header, in its dashed-hex format.
 
 ### AddVersion
 
-The request is a `POST` to `<origin>/client/<clientId>/add-version/<parentVersionId>`.
+The request is a `POST` to `<origin>/client/add-version/<parentVersionId>`.
 The request body contains the history segment, optionally encoded using any encoding supported by actix-web.
 The content-type must be `application/vnd.taskchampion.history-segment`.
 
@@ -86,7 +87,7 @@ Other error responses (4xx or 5xx) may be returned and should be treated appropr
 
 ### GetChildVersion
 
-The request is a `GET` to `<origin>/client/<clientId>/get-child-version/<parentVersionId>`.
+The request is a `GET` to `<origin>/client/get-child-version/<parentVersionId>`.
 The response is 404 NOT FOUND if no such version exists.
 Otherwise, the response is a 200 OK.
 The version's history segment is returned in the response body, with content-type `application/vnd.taskchampion.history-segment`.

sync-server/src/api/add_version.rs

@@ -1,8 +1,8 @@
 use crate::api::{
-    failure_to_ise, ServerState, HISTORY_SEGMENT_CONTENT_TYPE, PARENT_VERSION_ID_HEADER,
-    VERSION_ID_HEADER,
+    client_key_header, failure_to_ise, ServerState, HISTORY_SEGMENT_CONTENT_TYPE,
+    PARENT_VERSION_ID_HEADER, VERSION_ID_HEADER,
 };
-use crate::server::{add_version, AddVersionResult, ClientKey, VersionId, NO_VERSION_ID};
+use crate::server::{add_version, AddVersionResult, VersionId, NO_VERSION_ID};
 use actix_web::{error, post, web, HttpMessage, HttpRequest, HttpResponse, Result};
 use futures::StreamExt;
 
@@ -19,11 +19,11 @@ const MAX_SIZE: usize = 100 * 1024 * 1024;
 /// parent version ID in the `X-Parent-Version-Id` header.
 ///
 /// Returns other 4xx or 5xx responses on other errors.
-#[post("/client/{client_key}/add-version/{parent_version_id}")]
+#[post("/client/add-version/{parent_version_id}")]
 pub(crate) async fn service(
     req: HttpRequest,
     server_state: web::Data<ServerState>,
-    web::Path((client_key, parent_version_id)): web::Path<(ClientKey, VersionId)>,
+    web::Path((parent_version_id,)): web::Path<(VersionId,)>,
     mut payload: web::Payload,
 ) -> Result<HttpResponse> {
     // check content-type
@@ -31,6 +31,8 @@ pub(crate) async fn service(
         return Err(error::ErrorBadRequest("Bad content-type"));
     }
 
+    let client_key = client_key_header(&req)?;
+
     // read the body in its entirety
     let mut body = web::BytesMut::new();
     while let Some(chunk) = payload.next().await {
@@ -97,13 +99,14 @@ mod test {
         let server_state = ServerState::new(server_box);
         let mut app = test::init_service(App::new().service(app_scope(server_state))).await;
 
-        let uri = format!("/client/{}/add-version/{}", client_key, parent_version_id);
+        let uri = format!("/client/add-version/{}", parent_version_id);
         let req = test::TestRequest::post()
             .uri(&uri)
             .header(
                 "Content-Type",
                 "application/vnd.taskchampion.history-segment",
             )
+            .header("X-Client-Key", client_key.to_string())
             .set_payload(b"abcd".to_vec())
             .to_request();
         let resp = test::call_service(&mut app, req).await;
@@ -133,13 +136,14 @@ mod test {
         let server_state = ServerState::new(server_box);
         let mut app = test::init_service(App::new().service(app_scope(server_state))).await;
 
-        let uri = format!("/client/{}/add-version/{}", client_key, parent_version_id);
+        let uri = format!("/client/add-version/{}", parent_version_id);
         let req = test::TestRequest::post()
             .uri(&uri)
             .header(
                 "Content-Type",
                 "application/vnd.taskchampion.history-segment",
             )
+            .header("X-Client-Key", client_key.to_string())
             .set_payload(b"abcd".to_vec())
             .to_request();
         let resp = test::call_service(&mut app, req).await;
@@ -159,10 +163,11 @@ mod test {
         let server_state = ServerState::new(server_box);
         let mut app = test::init_service(App::new().service(app_scope(server_state))).await;
 
-        let uri = format!("/client/{}/add-version/{}", client_key, parent_version_id);
+        let uri = format!("/client/add-version/{}", parent_version_id);
         let req = test::TestRequest::post()
             .uri(&uri)
             .header("Content-Type", "not/correct")
+            .header("X-Client-Key", client_key.to_string())
             .set_payload(b"abcd".to_vec())
             .to_request();
         let resp = test::call_service(&mut app, req).await;
@@ -177,13 +182,14 @@ mod test {
         let server_state = ServerState::new(server_box);
         let mut app = test::init_service(App::new().service(app_scope(server_state))).await;
 
-        let uri = format!("/client/{}/add-version/{}", client_key, parent_version_id);
+        let uri = format!("/client/add-version/{}", parent_version_id);
         let req = test::TestRequest::post()
             .uri(&uri)
             .header(
                 "Content-Type",
                 "application/vnd.taskchampion.history-segment",
             )
+            .header("X-Client-Key", client_key.to_string())
             .to_request();
         let resp = test::call_service(&mut app, req).await;
         assert_eq!(resp.status(), StatusCode::BAD_REQUEST);

sync-server/src/api/get_child_version.rs

@@ -1,9 +1,9 @@
 use crate::api::{
-    failure_to_ise, ServerState, HISTORY_SEGMENT_CONTENT_TYPE, PARENT_VERSION_ID_HEADER,
-    VERSION_ID_HEADER,
+    client_key_header, failure_to_ise, ServerState, HISTORY_SEGMENT_CONTENT_TYPE,
+    PARENT_VERSION_ID_HEADER, VERSION_ID_HEADER,
 };
-use crate::server::{get_child_version, ClientKey, VersionId};
-use actix_web::{error, get, web, HttpResponse, Result};
+use crate::server::{get_child_version, VersionId};
+use actix_web::{error, get, web, HttpRequest, HttpResponse, Result};
 
 /// Get a child version.
 ///
@@ -13,13 +13,16 @@ use actix_web::{error, get, web, HttpResponse, Result};
 ///
 /// If no such child exists, returns a 404 with no content.
 /// Returns other 4xx or 5xx responses on other errors.
-#[get("/client/{client_key}/get-child-version/{parent_version_id}")]
+#[get("/client/get-child-version/{parent_version_id}")]
 pub(crate) async fn service(
+    req: HttpRequest,
     server_state: web::Data<ServerState>,
-    web::Path((client_key, parent_version_id)): web::Path<(ClientKey, VersionId)>,
+    web::Path((parent_version_id,)): web::Path<(VersionId,)>,
 ) -> Result<HttpResponse> {
     let mut txn = server_state.txn().map_err(failure_to_ise)?;
 
+    let client_key = client_key_header(&req)?;
+
     txn.get_client(client_key)
         .map_err(failure_to_ise)?
         .ok_or_else(|| error::ErrorNotFound("no such client"))?;
@@ -65,11 +68,11 @@ mod test {
         let server_state = ServerState::new(server_box);
         let mut app = test::init_service(App::new().service(app_scope(server_state))).await;
 
-        let uri = format!(
-            "/client/{}/get-child-version/{}",
-            client_key, parent_version_id
-        );
-        let req = test::TestRequest::get().uri(&uri).to_request();
+        let uri = format!("/client/get-child-version/{}", parent_version_id);
+        let req = test::TestRequest::get()
+            .uri(&uri)
+            .header("X-Client-Key", client_key.to_string())
+            .to_request();
         let mut resp = test::call_service(&mut app, req).await;
         assert_eq!(resp.status(), StatusCode::OK);
         assert_eq!(
@@ -98,11 +101,11 @@ mod test {
         let server_state = ServerState::new(server_box);
         let mut app = test::init_service(App::new().service(app_scope(server_state))).await;
 
-        let uri = format!(
-            "/client/{}/get-child-version/{}",
-            client_key, parent_version_id
-        );
-        let req = test::TestRequest::get().uri(&uri).to_request();
+        let uri = format!("/client/get-child-version/{}", parent_version_id);
+        let req = test::TestRequest::get()
+            .uri(&uri)
+            .header("X-Client-Key", client_key.to_string())
+            .to_request();
         let resp = test::call_service(&mut app, req).await;
         assert_eq!(resp.status(), StatusCode::NOT_FOUND);
         assert_eq!(resp.headers().get("X-Version-Id"), None);
@@ -123,11 +126,11 @@ mod test {
         let server_state = ServerState::new(server_box);
         let mut app = test::init_service(App::new().service(app_scope(server_state))).await;
 
-        let uri = format!(
-            "/client/{}/get-child-version/{}",
-            client_key, parent_version_id
-        );
-        let req = test::TestRequest::get().uri(&uri).to_request();
+        let uri = format!("/client/get-child-version/{}", parent_version_id);
+        let req = test::TestRequest::get()
+            .uri(&uri)
+            .header("X-Client-Key", client_key.to_string())
+            .to_request();
         let resp = test::call_service(&mut app, req).await;
         assert_eq!(resp.status(), StatusCode::NOT_FOUND);
         assert_eq!(resp.headers().get("X-Version-Id"), None);

sync-server/src/api/mod.rs

@@ -1,5 +1,6 @@
+use crate::server::ClientKey;
 use crate::storage::Storage;
-use actix_web::{error, http::StatusCode, web, Scope};
+use actix_web::{error, http::StatusCode, web, HttpRequest, Result, Scope};
 use std::sync::Arc;
 
 mod add_version;
@@ -9,10 +10,13 @@ mod get_child_version;
 pub(crate) const HISTORY_SEGMENT_CONTENT_TYPE: &str =
     "application/vnd.taskchampion.history-segment";
 
-/// The header names for version ID
+/// The header name for version ID
 pub(crate) const VERSION_ID_HEADER: &str = "X-Version-Id";
 
-/// The header names for parent version ID
+/// The header name for client key
+pub(crate) const CLIENT_KEY_HEADER: &str = "X-Client-Key";
+
+/// The header name for parent version ID
 pub(crate) const PARENT_VERSION_ID_HEADER: &str = "X-Parent-Version-Id";
 
 /// The type containing a reference to the Storage object in the Actix state.
@@ -28,3 +32,17 @@ pub(crate) fn api_scope() -> Scope {
 fn failure_to_ise(err: failure::Error) -> impl actix_web::ResponseError {
     error::InternalError::new(err, StatusCode::INTERNAL_SERVER_ERROR)
 }
+
+/// Get the client key
+fn client_key_header(req: &HttpRequest) -> Result<ClientKey> {
+    fn badrequest() -> error::Error {
+        error::ErrorBadRequest("bad x-client-id")
+    }
+    if let Some(client_key_hdr) = req.headers().get(CLIENT_KEY_HEADER) {
+        let client_key = client_key_hdr.to_str().map_err(|_| badrequest())?;
+        let client_key = ClientKey::parse_str(client_key).map_err(|_| badrequest())?;
+        Ok(client_key)
+    } else {
+        Err(badrequest())
+    }
+}

taskchampion/src/server/remote/mod.rs

@@ -56,10 +56,7 @@ impl Server for RemoteServer {
         parent_version_id: VersionId,
         history_segment: HistorySegment,
     ) -> Fallible<AddVersionResult> {
-        let url = format!(
-            "{}/client/{}/add-version/{}",
-            self.origin, self.client_key, parent_version_id
-        );
+        let url = format!("{}/client/add-version/{}", self.origin, parent_version_id);
         let history_cleartext = HistoryCleartext {
             parent_version_id,
             history_segment,
@@ -74,6 +71,7 @@ impl Server for RemoteServer {
                 "Content-Type",
                 "application/vnd.taskchampion.history-segment",
             )
+            .set("X-Client-Key", &self.client_key.to_string())
             .send_bytes(history_ciphertext.as_ref());
         if resp.ok() {
             let version_id = get_uuid_header(&resp, "X-Version-Id")?;
@@ -88,14 +86,15 @@ impl Server for RemoteServer {
 
     fn get_child_version(&mut self, parent_version_id: VersionId) -> Fallible<GetVersionResult> {
         let url = format!(
-            "{}/client/{}/get-child-version/{}",
-            self.origin, self.client_key, parent_version_id
+            "{}/client/get-child-version/{}",
+            self.origin, parent_version_id
         );
         let resp = self
             .agent
             .get(&url)
             .timeout_connect(10_000)
             .timeout_read(60_000)
+            .set("X-Client-Key", &self.client_key.to_string())
             .call();
 
         if resp.ok() {