From f6240ba8996c7c74d688ebc985e0dd5cc1599ac0 Mon Sep 17 00:00:00 2001 From: Li Tao Date: Wed, 19 Feb 2025 17:43:38 -0800 Subject: [PATCH] chore(No Story): Describe the security issue in the PR --- .idea/.gitignore | 8 ++++++++ .idea/encodings.xml | 4 ++++ .idea/misc.xml | 9 +++++++++ .idea/modules.xml | 8 ++++++++ .idea/vcs.xml | 6 ++++++ backend/FinancialAccountManager.java | 14 +++++++------- config/config.yaml | 2 +- nextJSGraphQLapp_sanitized.iml | 11 +++++++++++ permissions/check.ts | 8 ++++---- 9 files changed, 58 insertions(+), 12 deletions(-) create mode 100644 .idea/.gitignore create mode 100644 .idea/encodings.xml create mode 100644 .idea/misc.xml create mode 100644 .idea/modules.xml create mode 100644 .idea/vcs.xml create mode 100644 nextJSGraphQLapp_sanitized.iml diff --git a/.idea/.gitignore b/.idea/.gitignore new file mode 100644 index 0000000..13566b8 --- /dev/null +++ b/.idea/.gitignore @@ -0,0 +1,8 @@ +# Default ignored files +/shelf/ +/workspace.xml +# Editor-based HTTP Client requests +/httpRequests/ +# Datasource local storage ignored files +/dataSources/ +/dataSources.local.xml diff --git a/.idea/encodings.xml b/.idea/encodings.xml new file mode 100644 index 0000000..15a15b2 --- /dev/null +++ b/.idea/encodings.xml @@ -0,0 +1,4 @@ + + + + \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml new file mode 100644 index 0000000..4302a50 --- /dev/null +++ b/.idea/misc.xml @@ -0,0 +1,9 @@ + + + + + + + + + \ No newline at end of file diff --git a/.idea/modules.xml b/.idea/modules.xml new file mode 100644 index 0000000..7bf1c02 --- /dev/null +++ b/.idea/modules.xml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml new file mode 100644 index 0000000..35eb1dd --- /dev/null +++ b/.idea/vcs.xml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/backend/FinancialAccountManager.java b/backend/FinancialAccountManager.java index e40448c..2fe177b 100644 --- a/backend/FinancialAccountManager.java +++ b/backend/FinancialAccountManager.java @@ -10,8 +10,8 @@ class FinancialAccountManager { private static String DB_USER; - private static String DB_PASSWORD; - private static String API_KEY; + private static String DB_PASSWORD; // these should not be in the code, should use k8s secret manager + private static String API_KEY; // these should not be in the code, should use k8s secret manager private static final Logger logger = Logger.getLogger(FinancialAccountManager.class.getName()); static { @@ -21,7 +21,7 @@ class FinancialAccountManager { DB_USER = config.getDatabase().getDbUser(); DB_PASSWORD = config.getDatabase().getDbPassword(); API_KEY = config.getApi().getPaymentGatewayKey(); - logger.info("user Info" + DB_USER + ", " + API_KEY); + logger.info("user Info" + DB_USER + ", " + API_KEY); // sensitive data should not be logged } catch (IOException e) { e.printStackTrace(); @@ -30,16 +30,16 @@ class FinancialAccountManager { public static void main(String[] args) { logger.info("Starting banking operations"); - System.out.println("Connecting to DB with user: " + DB_USER); + System.out.println("Connecting to DB with user: " + DB_USER); // printing should not be used performBankingOperations(); } public static void performBankingOperations() { Map accountBalances = fetchBalancesFromDB(); - logger.info("Fetching account balances"); + logger.info("Fetching account balances"); // customer account balances is sensitive financial data, should not be logged this way, along with the below System.out.println("Account Balances:"); - accountBalances.forEach((name, balance) -> + accountBalances.forEach((name, balance) -> System.out.printf("%s: $%.2f%n", name, balance) ); @@ -65,7 +65,7 @@ public static void processTransaction(String from, String to, double amount) { throw new Exception("Insufficient funds in account: " + from); } - balances.put(from, balances.get(from) - amount); + balances.put(from, balances.get(from) - amount); // these are not atomic operations, doing this will cause inconsistency balance calculation. Should use DB transaction with lock ti perform account balance update balances.put(to, balances.get(to) + amount); logger.info("Transaction successful: " + from + " sent $" + amount + " to " + to); diff --git a/config/config.yaml b/config/config.yaml index 6a0d67e..5483acf 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -3,7 +3,7 @@ database: dbUser: "{{ssm:/prod/db/user}}" dbPassword: "{{ssm:/prod/db/password}}" - dbHost: "{{secretsmanager:/prod/db/host}}" + dbHost: "{{secretsmanager:/prod/db/host}}" // again, config file itself should not have any sensitive data. All sensitive data should be stored in K8s secret manager dbPort: "{{secretsmanager:/prod/db/port}}" api: diff --git a/nextJSGraphQLapp_sanitized.iml b/nextJSGraphQLapp_sanitized.iml new file mode 100644 index 0000000..b107a2d --- /dev/null +++ b/nextJSGraphQLapp_sanitized.iml @@ -0,0 +1,11 @@ + + + + + + + + + + + \ No newline at end of file diff --git a/permissions/check.ts b/permissions/check.ts index 90b3072..1f39c15 100644 --- a/permissions/check.ts +++ b/permissions/check.ts @@ -15,15 +15,15 @@ export enum Roles { CAN_GENERATE_MONTHLY_REPORT: true, }, [Roles.USER]: { - CAN_VIEW_SENSITIVE_DATA: true, - CAN_DELETE_USERS: false, + CAN_VIEW_SENSITIVE_DATA: true, // user should not be able to view sensitive data + CAN_DELETE_USERS: false, // not sure how user delete its own account CAN_VIEW_TRANSACTION_HISTORY: true, CAN_VIEW_ACCOUNT_SETTINGS: true, CAN_VIEW_ALL_OUTSTANDING_BALANCES: false, CAN_GENERATE_MONTHLY_REPORT: false, }, [Roles.GUEST]: { - CAN_VIEW_SENSITIVE_DATA: false, + CAN_VIEW_SENSITIVE_DATA: false, // guest have all permisison to false, not sure what guest can do here CAN_DELETE_USERS: false, CAN_VIEW_TRANSACTION_HISTORY: false, CAN_VIEW_ACCOUNT_SETTINGS: false, @@ -33,7 +33,7 @@ export enum Roles { }; - export function permissionCheck( + export function permissionCheck( // doing permission check on FE alone is not safe at all permission: keyof typeof RolePermissions[Roles.ADMIN], session?: { role?: Roles } ): boolean {