refactor: improve logging in src/proxy/processors, remove log-related stubs/spies in tests

cypress/e2e/login.cy.js

@@ -54,6 +54,6 @@ describe('Login page', () => {
 
     cy.get('.MuiSnackbarContent-message')
       .should('be.visible')
-      .and('contain', 'You entered an invalid username or password...');
+      .and('contain', 'You entered an invalid username or password.');
   });
 });

src/proxy/processors/push-action/checkAuthorEmails.ts

@@ -55,8 +55,6 @@ const exec = async (req: any, action: Action): Promise<Action> => {
   const illegalEmails = uniqueAuthorEmails.filter((email) => !isEmailAllowed(email));
 
   if (illegalEmails.length > 0) {
-    console.log(`The following commit author e-mails are illegal: ${illegalEmails}`);
-
     step.error = true;
     step.log(`The following commit author e-mails are illegal: ${illegalEmails}`);
     step.setError(
@@ -67,7 +65,7 @@ const exec = async (req: any, action: Action): Promise<Action> => {
     return action;
   }
 
-  console.log(`The following commit author e-mails are legal: ${uniqueAuthorEmails}`);
+  step.log(`The following commit author e-mails are legal: ${uniqueAuthorEmails}`);
   action.addStep(step);
   return action;
 };

src/proxy/processors/push-action/checkCommitMessages.ts

@@ -17,19 +17,19 @@
 import { Action, Step } from '../../actions';
 import { getCommitConfig } from '../../../config';
 
-const isMessageAllowed = (commitMessage: string): boolean => {
+const isMessageAllowed = (commitMessage: string, step: Step): boolean => {
   try {
     const commitConfig = getCommitConfig();
 
     // Commit message is empty, i.e. '', null or undefined
     if (!commitMessage) {
-      console.log('No commit message included...');
+      step.log('No commit message included.');
       return false;
     }
 
     // Validation for configured block pattern(s) check...
     if (typeof commitMessage !== 'string') {
-      console.log('A non-string value has been captured for the commit message...');
+      step.log('A non-string value has been captured for the commit message.');
       return false;
     }
 
@@ -52,11 +52,11 @@ const isMessageAllowed = (commitMessage: string): boolean => {
 
     // Commit message matches configured block pattern(s)
     if (literalMatches.length || patternMatches.length) {
-      console.log('Commit message is blocked via configured literals/patterns...');
+      step.log('Commit message is blocked via configured literals/patterns.');
       return false;
     }
   } catch (error) {
-    console.log('Invalid regex pattern...');
+    step.log('Invalid regex pattern.');
     return false;
   }
 
@@ -69,11 +69,11 @@ const exec = async (req: any, action: Action): Promise<Action> => {
 
   const uniqueCommitMessages = [...new Set(action.commitData?.map((commit) => commit.message))];
 
-  const illegalMessages = uniqueCommitMessages.filter((message) => !isMessageAllowed(message));
+  const illegalMessages = uniqueCommitMessages.filter(
+    (message) => !isMessageAllowed(message, step),
+  );
 
   if (illegalMessages.length > 0) {
-    console.log(`The following commit messages are illegal: ${illegalMessages}`);
-
     step.error = true;
     step.log(`The following commit messages are illegal: ${illegalMessages}`);
     step.setError(
@@ -84,7 +84,7 @@ const exec = async (req: any, action: Action): Promise<Action> => {
     return action;
   }
 
-  console.log(`The following commit messages are legal: ${uniqueCommitMessages}`);
+  step.log(`The following commit messages are legal: ${uniqueCommitMessages}`);
   action.addStep(step);
   return action;
 };

src/proxy/processors/push-action/checkUserPushPermission.ts

@@ -47,7 +47,6 @@ const validateUser = async (userEmail: string, action: Action, step: Step): Prom
   const list = await getUsers({ email: userEmail });
 
   if (list.length > 1) {
-    console.error(`Multiple users found with email address ${userEmail}, ending`);
     step.error = true;
     step.log(
       `Multiple Users have email <${userEmail}> so we cannot uniquely identify the user, ending`,
@@ -59,15 +58,14 @@ const validateUser = async (userEmail: string, action: Action, step: Step): Prom
     action.addStep(step);
     return action;
   } else if (list.length == 0) {
-    console.error(`No user with email address ${userEmail} found`);
+    step.log(`No user with email address ${userEmail} found`);
   } else {
     isUserAllowed = await isUserPushAllowed(action.url, list[0].username);
   }
 
-  console.log(`User ${userEmail} permission on Repo ${action.url} : ${isUserAllowed}`);
+  step.log(`User ${userEmail} permission on Repo ${action.url}: ${isUserAllowed}`);
 
   if (!isUserAllowed) {
-    console.log('User not allowed to Push');
     step.error = true;
     step.log(`User ${userEmail} is not allowed to push on repo ${action.url}, ending`);
     step.setError(

src/proxy/processors/push-action/gitleaks.ts

@@ -106,8 +106,7 @@ const getPluginConfig = async (): Promise<ConfigOptions> => {
     if (userConfigPath.length > 0 && (await fileIsReadable(userConfigPath))) {
       configPath = userConfigPath;
     } else {
-      console.error('could not read file at the config path provided, will not be fed to gitleaks');
-      throw new Error("could not check user's config path");
+      throw new Error(`Unable to read file at the provided config path: ${userConfigPath}`);
     }
   }
 
@@ -132,22 +131,22 @@ const exec = async (req: any, action: Action): Promise<Action> => {
   try {
     config = await getPluginConfig();
   } catch (e) {
-    console.error('failed to get gitleaks config, please fix the error:', e);
+    step.setError(`Failed to get gitleaks config: ${e}`);
     action.error = true;
-    step.setError('failed setup gitleaks, please contact an administrator\n');
+    step.setError('Failed to setup gitleaks, please contact an administrator.');
     action.addStep(step);
     return action;
   }
 
   if (!config.enabled) {
-    console.log('gitleaks is disabled, skipping');
+    step.log('Gitleaks is disabled, skipping.');
     action.addStep(step);
     return action;
   }
 
   const { commitFrom, commitTo } = action;
   const workingDir = `${action.proxyGitPath}/${action.repoName}`;
-  console.log(`Scanning range with gitleaks: ${commitFrom}:${commitTo}`, workingDir);
+  step.log(`Scanning range with gitleaks: ${commitFrom}:${commitTo} in ${workingDir}`);
 
   try {
     const gitRootCommit = await runCommand(workingDir, 'git', [
@@ -180,19 +179,19 @@ const exec = async (req: any, action: Action): Promise<Action> => {
       // any failure
       step.error = true;
       if (gitleaks.exitCode !== EXIT_CODE) {
-        step.setError('failed to run gitleaks, please contact an administrator\n');
+        step.setError('Failed to run gitleaks, please contact an administrator.');
       } else {
         // exit code matched our gitleaks findings exit code
         // newline prefix to avoid tab indent at the start
         step.setError('\n' + gitleaks.stdout + gitleaks.stderr);
       }
     } else {
-      console.log('succeeded');
-      console.log(gitleaks.stderr);
+      step.log('Succeeded.');
+      step.log(`Gitleaks output: ${gitleaks.stderr}`);
     }
   } catch (e) {
     action.error = true;
-    step.setError('failed to spawn gitleaks, please contact an administrator\n');
+    step.setError('Failed to spawn gitleaks, please contact an administrator.');
     action.addStep(step);
     return action;
   }

src/proxy/processors/push-action/parsePush.ts

@@ -63,7 +63,7 @@ async function exec(req: any, action: Action): Promise<Action> {
         'Your push has been blocked. Please make sure you are pushing to a single branch.',
       );
     } else {
-      console.log(`refUpdates: ${JSON.stringify(refUpdates, null, 2)}`);
+      console.log(`parsePush refUpdates: ${JSON.stringify(refUpdates, null, 2)}`);
     }
 
     const [commitParts] = refUpdates[0].split('\0');
@@ -109,7 +109,9 @@ async function exec(req: any, action: Action): Promise<Action> {
       }
 
       const { committer, committerEmail } = action.commitData[action.commitData.length - 1];
-      console.log(`Push Request received from user ${committer} with email ${committerEmail}`);
+      // Note: This is not always the pusher's email, it's the last committer's email.
+      // See https://github.com/finos/git-proxy/issues/1400
+      step.log(`Push request received from user ${committer} with email ${committerEmail}`);
       action.user = committer;
       action.userEmail = committerEmail;
     }

src/proxy/processors/push-action/preReceive.ts

@@ -79,14 +79,12 @@ const exec = async (
       step.log('Push requires manual approval.');
       action.addStep(step);
     } else {
-      step.error = true;
       step.log(`Unexpected hook status: ${status}`);
       step.setError(stdoutTrimmed || 'Unknown pre-receive hook error.');
       action.addStep(step);
     }
     return action;
   } catch (error: any) {
-    step.error = true;
     step.log('Push failed, pre-receive hook returned an error.');
     step.setError(`Hook execution error: ${stderrTrimmed || error.message}`);
     action.addStep(step);

src/proxy/processors/push-action/scanDiff.ts

@@ -49,19 +49,23 @@ type Match = {
   content: string;
 };
 
-const getDiffViolations = (diff: string, organization: string): Match[] | string | null => {
+const getDiffViolations = (
+  diff: string,
+  organization: string,
+  step: Step,
+): Match[] | string | null => {
   // Commit diff is empty, i.e. '', null or undefined
   if (!diff) {
-    console.log('No commit diff found, but this may be legitimate (empty diff)');
+    step.log('No commit diff found, but this may be legitimate (empty diff).');
     // Empty diff is not necessarily a violation - could be legitimate
     // (e.g., cherry-pick with no changes, reverts, etc.)
     return null;
   }
 
-  // Validation for configured block pattern(s) check...
+  // Validation for configured block pattern(s) check
   if (typeof diff !== 'string') {
-    console.log('A non-string value has been captured for the commit diff...');
-    return 'A non-string value has been captured for the commit diff...';
+    step.log('A non-string value has been captured for the commit diff.');
+    return 'A non-string value has been captured for the commit diff.';
   }
 
   const parsedDiff = parseDiff(diff);
@@ -70,7 +74,7 @@ const getDiffViolations = (diff: string, organization: string): Match[] | string
   const res = collectMatches(parsedDiff, combinedMatches);
   // Diff matches configured block pattern(s)
   if (res.length > 0) {
-    console.log('Diff is blocked via configured literals/patterns/providers...');
+    step.log('Diff is blocked via configured literals/patterns/providers.');
     // combining matches with file and line number
     return res;
   }
@@ -174,12 +178,12 @@ const exec = async (req: any, action: Action): Promise<Action> => {
   const step = new Step('scanDiff');
 
   const { steps, commitFrom, commitTo } = action;
-  console.log(`Scanning diff: ${commitFrom}:${commitTo}`);
+  step.log(`Scanning diff: ${commitFrom}:${commitTo}`);
 
   const diff = steps.find((s) => s.stepName === 'diff')?.content;
 
-  console.log(diff);
-  const diffViolations = getDiffViolations(diff, action.project);
+  step.log(diff);
+  const diffViolations = getDiffViolations(diff, action.project, step);
 
   if (diffViolations) {
     const formattedMatches = Array.isArray(diffViolations)
@@ -191,14 +195,9 @@ const exec = async (req: any, action: Action): Promise<Action> => {
     errorMsg.push(formattedMatches);
     errorMsg.push('\n');
 
-    console.log(`The following diff is illegal: ${commitFrom}:${commitTo}`);
-
     step.error = true;
     step.log(`The following diff is illegal: ${commitFrom}:${commitTo}`);
     step.setError(errorMsg.join('\n'));
-
-    action.addStep(step);
-    return action;
   }
 
   action.addStep(step);

src/service/routes/repo.ts

@@ -70,7 +70,7 @@ const repo = (proxy: any) => {
       res.send({ message: 'created' });
     } else {
       res.status(401).send({
-        message: 'You are not authorised to perform this action...',
+        message: 'You are not authorised to perform this action.',
       });
     }
   });
@@ -90,7 +90,7 @@ const repo = (proxy: any) => {
       res.send({ message: 'created' });
     } else {
       res.status(401).send({
-        message: 'You are not authorised to perform this action...',
+        message: 'You are not authorised to perform this action.',
       });
     }
   });
@@ -112,7 +112,7 @@ const repo = (proxy: any) => {
         res.send({ message: 'created' });
       } else {
         res.status(401).send({
-          message: 'You are not authorised to perform this action...',
+          message: 'You are not authorised to perform this action.',
         });
       }
     },
@@ -135,7 +135,7 @@ const repo = (proxy: any) => {
         res.send({ message: 'created' });
       } else {
         res.status(401).send({
-          message: 'You are not authorised to perform this action...',
+          message: 'You are not authorised to perform this action.',
         });
       }
     },
@@ -160,7 +160,7 @@ const repo = (proxy: any) => {
       res.send({ message: 'deleted' });
     } else {
       res.status(401).send({
-        message: 'You are not authorised to perform this action...',
+        message: 'You are not authorised to perform this action.',
       });
     }
   });
@@ -217,7 +217,7 @@ const repo = (proxy: any) => {
       }
     } else {
       res.status(401).send({
-        message: 'You are not authorised to perform this action...',
+        message: 'You are not authorised to perform this action.',
       });
     }
   });

src/ui/services/repo.ts

@@ -41,7 +41,7 @@ const canAddUser = async (repoId: string, user: string, action: string) => {
 class DupUserValidationError extends Error {
   constructor(message: string) {
     super(message);
-    this.name = 'The user already has this role...';
+    this.name = 'The user already has this role.';
   }
 }
 

src/ui/views/Login/Login.tsx

@@ -101,10 +101,10 @@ const Login: React.FC = () => {
         } else if (error.response?.status === 403) {
           setMessage(processAuthError(error, false));
         } else {
-          setMessage('You entered an invalid username or password...');
+          setMessage('You entered an invalid username or password.');
         }
       } else {
-        setMessage('You entered an invalid username or password...');
+        setMessage('You entered an invalid username or password.');
       }
     } finally {
       setIsLoading(false);