Improve security of endpoints

Skip deleting branch if main

Author: Filip-L

Cargo.lock

@@ -0,0 +1,36 @@
+use actix_web::{
+    error::{ErrorInternalServerError, ErrorUnauthorized},
+    Error,
+};
+use fplus_lib::{config::get_env_var_or_default, external_services::github::github_async_new};
+
+pub async fn check_if_pull_request_opened_by_bot(
+    owner: &str,
+    repo: &str,
+    pr_number: &u64,
+) -> Result<(), Error> {
+    let gh_bot = get_env_var_or_default("BOT_USER");
+    let gh = github_async_new(owner.to_string(), repo.to_string())
+        .await
+        .map_err(|e| ErrorInternalServerError(format!("Failed to get GitHub client: {e}")))?;
+    let pr = gh
+        .get_pull_request_by_number(*pr_number)
+        .await
+        .map_err(|e| ErrorInternalServerError(format!("Failed to get pull request: {e}")))?;
+
+    let login = pr
+        .user
+        .as_ref()
+        .map(|user| user.login.as_str())
+        .ok_or_else(|| {
+            ErrorInternalServerError("User handle not found in pull request".to_string())
+        })?;
+
+    if login.is_empty() {
+        return Err(ErrorInternalServerError("User handle is empty".to_string()));
+    }
+    if login != gh_bot {
+        return Err(ErrorUnauthorized(format!("Unauthorized user: {login}")));
+    }
+    Ok(())
+}

fplus-http-server/src/auth/gh_handle_auth.rs

@@ -0,0 +1 @@
+pub mod gh_handle_auth;

fplus-http-server/src/auth/mod.rs

@@ -1,6 +1,7 @@
 use fplus_lib::core::allocator::update_installation_ids_logic;
 mod middleware;
 use middleware::verifier_auth::VerifierAuth;
+mod auth;
 pub(crate) mod router;
 
 use actix_web::{
@@ -107,7 +108,6 @@ async fn main() -> std::io::Result<()> {
             .service(router::application::validate_application_proposal)
             .service(router::application::validate_application_approval)
             .service(router::application::validate_application_merge)
-            .service(router::application::delete_branch)
             .service(router::application::cache_renewal)
             .service(router::application::update_from_issue)
             .service(router::application::submit_kyc)
@@ -116,7 +116,6 @@ async fn main() -> std::io::Result<()> {
             .service(router::verifier::verifiers)
             .service(router::allocator::allocators)
             .service(router::allocator::allocator)
-            .service(router::allocator::delete)
             .service(router::allocator::create_allocator_from_json)
             .service(router::allocator::update_allocator_force)
             .service(router::allocator::check_if_repository_application_is_installed)

fplus-http-server/src/main.rs

@@ -1,5 +1,4 @@
 use actix_web::{
-    delete,
     error::{ErrorInternalServerError, ErrorNotFound},
     get, post, web, HttpResponse, Responder,
 };
@@ -70,24 +69,6 @@ pub async fn allocator(path: web::Path<(String, String)>) -> actix_web::Result<i
     }
 }
 
-/**
- * Delete an allocator
- *
- * # Arguments
- * @param path: web::Path<(String, String)> - The owner and repo of the allocator
- *
- * # Returns
- * @return HttpResponse - The result of the operation
- */
-#[delete("/allocator/{owner}/{repo}")]
-pub async fn delete(path: web::Path<(String, String)>) -> actix_web::Result<impl Responder> {
-    let (owner, repo) = path.into_inner();
-    allocators_db::delete_allocator(&owner, &repo)
-        .await
-        .map_err(ErrorInternalServerError)?;
-    Ok(HttpResponse::Ok().finish())
-}
-
 /**
  * Force updating allocator files from template.
  * It receives a list of changed files and allocators to update.

fplus-http-server/src/router/allocator.rs

@@ -7,15 +7,17 @@ use fplus_lib::core::{
     application::file::{
         DecreaseClientAllowanceVerifier, StorageProviderChangeVerifier, VerifierInput,
     },
-    ApplicationQueryParams, BranchDeleteInfo, CompleteGovernanceReviewInfo,
-    CompleteNewApplicationApprovalInfo, CompleteNewApplicationProposalInfo, CreateApplicationInfo,
-    DcReachedInfo, DecreaseAllowanceApprovalInfo, DecreaseAllowanceProposalInfo,
+    ApplicationQueryParams, CompleteGovernanceReviewInfo, CompleteNewApplicationApprovalInfo,
+    CompleteNewApplicationProposalInfo, CreateApplicationInfo, DcReachedInfo,
+    DecreaseAllowanceApprovalInfo, DecreaseAllowanceProposalInfo,
     GetApplicationsByClientContractAddressQueryParams, GithubQueryParams, LDNApplication,
     MoreInfoNeeded, NotifyRefillInfo, StorageProvidersChangeApprovalInfo,
     StorageProvidersChangeProposalInfo, SubmitKYCInfo, TriggerSSAInfo, ValidationPullRequestData,
     VerifierActionsQueryParams,
 };
 
+use crate::auth::gh_handle_auth::check_if_pull_request_opened_by_bot;
+
 #[post("/application")]
 pub async fn create(info: web::Json<CreateApplicationInfo>) -> actix_web::Result<impl Responder> {
     let app = LDNApplication::new_from_issue(info.into_inner())
@@ -543,8 +545,8 @@ pub async fn validate_application_merge(
         owner,
         repo,
     } = info.into_inner();
-
     if let Ok(pr_number) = pr_number.trim_matches('"').parse::<u64>() {
+        check_if_pull_request_opened_by_bot(&owner, &repo, &pr_number).await?;
         let result = LDNApplication::validate_merge_application(pr_number, owner, repo)
             .await
             .map_err(ErrorInternalServerError)?;
@@ -554,15 +556,6 @@ pub async fn validate_application_merge(
     }
 }
 
-#[post("/application/branch/delete")]
-pub async fn delete_branch(data: web::Json<BranchDeleteInfo>) -> actix_web::Result<impl Responder> {
-    let info = data.into_inner();
-    let result = LDNApplication::delete_branch(info.owner, info.repo, info.branch_name)
-        .await
-        .map_err(ErrorInternalServerError)?;
-    Ok(HttpResponse::Ok().json(result))
-}
-
 #[post("application/cache/renewal")]
 pub async fn cache_renewal(
     info: web::Json<GithubQueryParams>,
@@ -597,13 +590,14 @@ pub async fn check_for_changes(
 ) -> actix_web::Result<impl Responder> {
     let ValidationPullRequestData {
         pr_number,
-        user_handle,
+        user_handle: _,
         owner,
         repo,
     } = info.into_inner();
 
     if let Ok(pr_number) = pr_number.trim_matches('"').parse::<u64>() {
-        let result = LDNApplication::check_for_changes(pr_number, &user_handle, owner, repo)
+        check_if_pull_request_opened_by_bot(&owner, &repo, &pr_number).await?;
+        let result = LDNApplication::check_for_changes(pr_number, owner, repo)
             .await
             .map_err(ErrorInternalServerError)?;
         Ok(HttpResponse::Ok().json(result))

fplus-http-server/src/router/application.rs

@@ -42,6 +42,7 @@ tfidf-summarizer = "2.0.0"
 ndarray = "0.16.1"
 strsim = "0.10"
 url = "2.5.4"
+snafu = "0.7.5"
 
 [dev-dependencies]
 actix-rt = "2.9.0"

fplus-lib/Cargo.toml

@@ -20,7 +20,7 @@ pub fn default_env_vars() -> &'static HashMap<&'static str, &'static str> {
         m.insert("ALLOCATOR_GOVERNANCE_REPO", "Allocator-Governance-Staging");
         m.insert("ALLOCATOR_TEMPLATE_OWNER", "fidlabs");
         m.insert("ALLOCATOR_TEMPLATE_REPO", "allocator-template");
-        m.insert("BOT_USER", "filplus-allocators-staging-bot[bot]");
+        m.insert("BOT_USER", "filplus-dr-bot-ghapp[bot]");
         m.insert(
             "BACKEND_URL",
             "https://fp-core.dp04sa0tdc6pk.us-east-1.cs.amazonlightsail.com",

fplus-lib/src/config.rs

@@ -81,13 +81,6 @@ pub struct TriggerSSAInfo {
     pub early_refill_comment: Option<String>,
 }
 
-#[derive(Deserialize)]
-pub struct BranchDeleteInfo {
-    pub owner: String,
-    pub repo: String,
-    pub branch_name: String,
-}
-
 #[derive(Deserialize)]
 pub struct NewPrNumberAndFileSha {
     pub pr_number: u64,
@@ -1952,11 +1945,13 @@ impl LDNApplication {
 
         let gh = github_async_new(self.github.owner.clone(), self.github.repo.clone()).await?;
 
-        gh.merge_pull_request(pr_number).await.map_err(|e| {
-            LDNError::Load(format!(
-                "Failed to merge pull request {pr_number}. Reason: {e}"
-            ))
-        })?;
+        gh.merge_pull_request_and_delete_branch(&pr_number)
+            .await
+            .map_err(|e| {
+                LDNError::Load(format!(
+                    "Failed to merge pull request {pr_number} or delete branch. Reason: {e}"
+                ))
+            })?;
 
         database::applications::merge_application_by_pr_number(
             self.github.owner.clone(),
@@ -2249,29 +2244,31 @@ impl LDNApplication {
             }
             log::info!("- Application is in a valid state!");
 
-            Self::merge_application(pr_number, owner, repo).await?;
+            Self::merge_application_and_delete_branch(pr_number, owner, repo).await?;
             return Ok(true);
         } else if application.lifecycle.get_state() == AppState::Declined {
-            Self::merge_application(pr_number, owner, repo).await?;
+            Self::merge_application_and_delete_branch(pr_number, owner, repo).await?;
             return Ok(true);
         }
 
         log::warn!("- Application is not in a valid state");
         Ok(false)
     }
 
-    pub async fn merge_application(
+    pub async fn merge_application_and_delete_branch(
         pr_number: u64,
         owner: String,
         repo: String,
     ) -> Result<bool, LDNError> {
         let gh = github_async_new(owner.to_string(), repo.to_string()).await?;
 
-        gh.merge_pull_request(pr_number).await.map_err(|e| {
-            LDNError::Load(format!(
-                "Failed to merge pull request {pr_number}. Reason: {e}"
-            ))
-        })?;
+        gh.merge_pull_request_and_delete_branch(&pr_number)
+            .await
+            .map_err(|e| {
+                LDNError::Load(format!(
+                    "Failed to merge pull request {pr_number} or delete branch. Reason: {e}"
+                ))
+            })?;
 
         database::applications::merge_application_by_pr_number(owner, repo, pr_number)
             .await
@@ -2370,11 +2367,6 @@ impl LDNApplication {
             log::info!("- Application is in a valid state!");
             return Ok(true);
         }
-        // let bot_user = get_env_var_or_default("BOT_USER");
-        // if author != bot_user {
-        //     log::warn!("- Author is not the bot user");
-        //     return Ok(false);
-        // }
 
         log::info!("- Application is in a valid state");
         Ok(true)
@@ -2745,18 +2737,11 @@ impl LDNApplication {
 
     pub async fn check_for_changes(
         pr_number: u64,
-        author: &str,
         owner: String,
         repo: String,
     ) -> Result<bool, LDNError> {
         log::info!("Starting check_for_changes:");
 
-        let bot_user = get_env_var_or_default("BOT_USER");
-        if author != bot_user {
-            log::warn!("- Author is not the bot user");
-            return Err(LDNError::New("PR File edited by user".to_string()));
-        }
-
         let gh: GithubWrapper = github_async_new(owner.clone(), repo.clone()).await?;
         let result = Self::get_pr_files_and_app(owner.clone(), repo.clone(), pr_number).await;
 
@@ -3491,23 +3476,6 @@ impl LDNApplication {
         Ok(())
     }
 
-    pub async fn delete_branch(
-        owner: String,
-        repo: String,
-        branch_name: String,
-    ) -> Result<bool, LDNError> {
-        let gh = github_async_new(owner, repo).await?;
-        let request = gh
-            .build_remove_ref_request(branch_name.clone())
-            .map_err(|e| LDNError::New(format!("build_remove_ref_request function failed: {e}")))?;
-
-        gh.remove_branch(request)
-            .await
-            .map_err(|e| LDNError::New(format!("Error deleting branch {branch_name} /// {e}")))?;
-
-        Ok(true)
-    }
-
     async fn add_comment_to_issue(
         issue_number: String,
         owner: String,
@@ -4268,11 +4236,13 @@ _The initial issue can be edited in order to solve the request of the verifier.
         if !application_file.allocation.0.is_empty() {
             let gh = github_async_new(owner.to_string(), repo.to_string()).await?;
 
-            gh.merge_pull_request(pr_number).await.map_err(|e| {
-                LDNError::Load(format!(
-                    "Failed to merge pull request {pr_number}. Reason: {e}"
-                ))
-            })?;
+            gh.merge_pull_request_and_delete_branch(&pr_number)
+                .await
+                .map_err(|e| {
+                    LDNError::Load(format!(
+                        "Failed to merge pull request {pr_number} or delete branch. Reason: {e}"
+                    ))
+                })?;
 
             database::applications::merge_application_by_pr_number(
                 owner.to_string(),
@@ -4712,7 +4682,8 @@ _The initial issue can be edited in order to solve the request of the verifier.
             self.remove_first_pending_allocation(&application_file)
                 .await?;
         } else {
-            self.remove_pending_refill(&app_model.pr_number).await?;
+            self.remove_pending_refill(&(app_model.pr_number as u64))
+                .await?;
         }
 
         let comment = format!(
@@ -4784,18 +4755,21 @@ _The initial issue can be edited in order to solve the request of the verifier.
         Ok(())
     }
 
-    async fn remove_pending_refill(&self, pr_number: &i64) -> Result<(), LDNError> {
-        Self::delete_branch(
-            self.github.owner.clone(),
-            self.github.repo.clone(),
-            self.branch_name.clone(),
-        )
-        .await?;
+    async fn remove_pending_refill(&self, pr_number: &u64) -> Result<(), LDNError> {
+        self.github
+            .delete_branch_safe(pr_number)
+            .await
+            .map_err(|e| {
+                LDNError::New(format!(
+                    "Failed to delete branch for PR number: {pr_number} Reason: {e:?}"
+                ))
+            })?;
+
         database::applications::delete_application(
             self.application_id.clone(),
             self.github.owner.clone(),
             self.github.repo.clone(),
-            *pr_number as u64,
+            *pr_number,
         )
         .await
         .map_err(|e| {
@@ -5044,22 +5018,6 @@ impl LDNPullRequest {
         Ok(())
     }
 
-    pub async fn delete_branch(
-        application_id: String,
-        owner: String,
-        repo: String,
-    ) -> Result<(), LDNError> {
-        let gh = github_async_new(owner.clone(), repo.clone()).await?;
-        let branch_name = LDNPullRequest::application_branch_name(&application_id);
-        let request = gh
-            .build_remove_ref_request(branch_name.clone())
-            .map_err(|e| LDNError::New(format!("build_remove_ref_request function failed: {e}")))?;
-        gh.remove_branch(request)
-            .await
-            .map_err(|e| LDNError::New(format!("Error deleting branch {branch_name} /// {e}")))?;
-        Ok(())
-    }
-
     pub(super) fn application_branch_name(application_id: &str) -> String {
         format!("Application/{application_id}")
     }