Merge pull request #3579 from github/mbg/start-proxy/token-check-fixes

Fix warning for PAT-like token with username

.vscode/tests.code-snippets

@@ -0,0 +1,30 @@
+{
+  // Place your codeql-action workspace snippets here. Each snippet is defined under a snippet name and has a scope, prefix, body and
+  // description. Add comma separated ids of the languages where the snippet is applicable in the scope field. If scope
+  // is left empty or omitted, the snippet gets applied to all languages. The prefix is what is
+  // used to trigger the snippet and the body will be expanded and inserted. Possible variables are:
+  // $1, $2 for tab stops, $0 for the final cursor position, and ${1:label}, ${2:another} for placeholders.
+  // Placeholders with the same ids are connected.
+  // Example:
+  // "Print to console": {
+  // 	"scope": "javascript,typescript",
+  // 	"prefix": "log",
+  // 	"body": [
+  // 		"console.log('$1');",
+  // 		"$2"
+  // 	],
+  // 	"description": "Log output to console"
+  // }
+  "Test Macro": {
+    "scope": "javascript, typescript",
+    "prefix": "testMacro",
+    "body": [
+      "const ${1:nameMacro} = test.macro({",
+      "  exec: async (t: ExecutionContext<unknown>) => {},",
+      "",
+      "  title: (providedTitle = \"\") => `${2:common title} - \\${providedTitle}`,",
+      "});",
+    ],
+    "description": "An Ava test macro",
+  },
+}

lib/start-proxy-action.js

@@ -122242,7 +122242,10 @@ function getCredentials(logger, registrySecrets, registriesCredentials, language
         );
       }
     }
-    if ((!hasUsername(authConfig) || !isDefined2(authConfig.username)) && isUsernamePassword(authConfig) && isDefined2(authConfig.password) && isPAT(authConfig.password) || isToken(authConfig) && isDefined2(authConfig.token) && isPAT(authConfig.token)) {
+    const noUsername = !hasUsername(authConfig) || !isDefined2(authConfig.username);
+    const passwordIsPAT = isUsernamePassword(authConfig) && isDefined2(authConfig.password) && isPAT(authConfig.password);
+    const tokenIsPAT = isToken(authConfig) && isDefined2(authConfig.token) && isPAT(authConfig.token);
+    if (noUsername && (passwordIsPAT || tokenIsPAT)) {
       logger.warning(
         `A ${e.type} private registry is configured for ${e.host || e.url} using a GitHub Personal Access Token (PAT), but no username was provided. This may not work correctly. When configuring a private registry using a PAT, select "Username and password" and enter the username of the user who generated the PAT.`
       );

src/start-proxy.test.ts

@@ -14,9 +14,9 @@ import * as startProxyExports from "./start-proxy";
 import { parseLanguage } from "./start-proxy";
 import * as statusReport from "./status-report";
 import {
+  assertNotLogged,
   checkExpectedLogMessages,
   createFeatures,
-  getRecordingLogger,
   makeTestToken,
   RecordingLogger,
   setupTests,
@@ -439,41 +439,155 @@ test("getCredentials accepts OIDC configurations", (t) => {
   t.assert(credentials.some((c) => startProxyExports.isJFrogConfig(c)));
 });
 
-test("getCredentials logs a warning when a PAT is used without a username", async (t) => {
-  const loggedMessages = [];
-  const logger = getRecordingLogger(loggedMessages);
-  const likelyWrongCredentials = toEncodedJSON([
+const getCredentialsMacro = test.macro({
+  exec: async (
+    t: ExecutionContext<unknown>,
+    credentials: startProxyExports.RawCredential[],
+    checkAccepted: (
+      t: ExecutionContext<unknown>,
+      logger: RecordingLogger,
+      results: startProxyExports.Credential[],
+    ) => void,
+  ) => {
+    const logger = new RecordingLogger();
+    const credentialsString = toEncodedJSON(credentials);
+
+    const results = startProxyExports.getCredentials(
+      logger,
+      undefined,
+      credentialsString,
+      undefined,
+    );
+
+    checkAccepted(t, logger, results);
+  },
+
+  title: (providedTitle = "") => `getCredentials - ${providedTitle}`,
+});
+
+test(
+  "warns for PAT-like password without a username",
+  getCredentialsMacro,
+  [
     {
       type: "git_server",
       host: "https://github.com/",
       password: `ghp_${makeTestToken()}`,
     },
-  ]);
+  ],
+  (t, logger, results) => {
+    // The configurations should be accepted, despite the likely problem.
+    t.assert(results);
+    t.is(results.length, 1);
+    t.is(results[0].type, "git_server");
+    t.is(results[0].host, "https://github.com/");
 
-  const results = startProxyExports.getCredentials(
-    logger,
-    undefined,
-    likelyWrongCredentials,
-    undefined,
-  );
+    if (startProxyExports.isUsernamePassword(results[0])) {
+      t.assert(results[0].password?.startsWith("ghp_"));
+    } else {
+      t.fail("Expected a `UsernamePassword`-based credential.");
+    }
 
-  // The configuration should be accepted, despite the likely problem.
-  t.assert(results);
-  t.is(results.length, 1);
-  t.is(results[0].type, "git_server");
-  t.is(results[0].host, "https://github.com/");
+    // A warning should have been logged.
+    checkExpectedLogMessages(t, logger.messages, [
+      "using a GitHub Personal Access Token (PAT), but no username was provided",
+    ]);
+  },
+);
 
-  if (startProxyExports.isUsernamePassword(results[0])) {
-    t.assert(results[0].password?.startsWith("ghp_"));
-  } else {
-    t.fail("Expected a `UsernamePassword`-based credential.");
-  }
+test(
+  "no warning for PAT-like password with a username",
+  getCredentialsMacro,
+  [
+    {
+      type: "git_server",
+      host: "https://github.com/",
+      username: "someone",
+      password: `ghp_${makeTestToken()}`,
+    },
+  ],
+  (t, logger, results) => {
+    // The configurations should be accepted, despite the likely problem.
+    t.assert(results);
+    t.is(results.length, 1);
+    t.is(results[0].type, "git_server");
+    t.is(results[0].host, "https://github.com/");
 
-  // A warning should have been logged.
-  checkExpectedLogMessages(t, loggedMessages, [
-    "using a GitHub Personal Access Token (PAT), but no username was provided",
-  ]);
-});
+    if (startProxyExports.isUsernamePassword(results[0])) {
+      t.assert(results[0].password?.startsWith("ghp_"));
+    } else {
+      t.fail("Expected a `UsernamePassword`-based credential.");
+    }
+
+    assertNotLogged(
+      t,
+      logger,
+      "using a GitHub Personal Access Token (PAT), but no username was provided",
+    );
+  },
+);
+
+test(
+  "warns for PAT-like token without a username",
+  getCredentialsMacro,
+  [
+    {
+      type: "git_server",
+      host: "https://github.com/",
+      token: `ghp_${makeTestToken()}`,
+    },
+  ],
+  (t, logger, results) => {
+    // The configurations should be accepted, despite the likely problem.
+    t.assert(results);
+    t.is(results.length, 1);
+    t.is(results[0].type, "git_server");
+    t.is(results[0].host, "https://github.com/");
+
+    if (startProxyExports.isToken(results[0])) {
+      t.assert(results[0].token?.startsWith("ghp_"));
+    } else {
+      t.fail("Expected a `Token`-based credential.");
+    }
+
+    // A warning should have been logged.
+    checkExpectedLogMessages(t, logger.messages, [
+      "using a GitHub Personal Access Token (PAT), but no username was provided",
+    ]);
+  },
+);
+
+test(
+  "no warning for PAT-like token with a username",
+  getCredentialsMacro,
+  [
+    {
+      type: "git_server",
+      host: "https://github.com/",
+      username: "someone",
+      token: `ghp_${makeTestToken()}`,
+    },
+  ],
+  (t, logger, results) => {
+    // The configurations should be accepted, despite the likely problem.
+    t.assert(results);
+    t.is(results.length, 1);
+    t.is(results[0].type, "git_server");
+    t.is(results[0].host, "https://github.com/");
+
+    if (startProxyExports.isToken(results[0])) {
+      t.assert(results[0].token?.startsWith("ghp_"));
+    } else {
+      t.fail("Expected a `Token`-based credential.");
+    }
+
+    assertNotLogged(
+      t,
+      logger,
+      "using a GitHub Personal Access Token (PAT), but no username was provided",
+    );
+  },
+);
 
 test("getCredentials returns all credentials for Actions when using LANGUAGE_TO_REGISTRY_TYPE", async (t) => {
   const credentialsInput = toEncodedJSON(mixedCredentials);

src/start-proxy.ts

@@ -447,15 +447,18 @@ export function getCredentials(
     }
 
     // If the password or token looks like a GitHub PAT, warn if no username is configured.
-    if (
-      ((!hasUsername(authConfig) || !isDefined(authConfig.username)) &&
-        isUsernamePassword(authConfig) &&
-        isDefined(authConfig.password) &&
-        isPAT(authConfig.password)) ||
-      (isToken(authConfig) &&
-        isDefined(authConfig.token) &&
-        isPAT(authConfig.token))
-    ) {
+    const noUsername =
+      !hasUsername(authConfig) || !isDefined(authConfig.username);
+    const passwordIsPAT =
+      isUsernamePassword(authConfig) &&
+      isDefined(authConfig.password) &&
+      isPAT(authConfig.password);
+    const tokenIsPAT =
+      isToken(authConfig) &&
+      isDefined(authConfig.token) &&
+      isPAT(authConfig.token);
+
+    if (noUsername && (passwordIsPAT || tokenIsPAT)) {
       logger.warning(
         `A ${e.type} private registry is configured for ${e.host || e.url} using a GitHub Personal Access Token (PAT), but no username was provided. ` +
           `This may not work correctly. When configuring a private registry using a PAT, select "Username and password" and enter the username of the user ` +

src/testing-utils.ts

@@ -185,8 +185,8 @@ export interface LoggedMessage {
 
 export class RecordingLogger implements Logger {
   messages: LoggedMessage[] = [];
-  groups: string[] = [];
-  unfinishedGroups: Set<string> = new Set();
+  readonly groups: string[] = [];
+  readonly unfinishedGroups: Set<string> = new Set();
   private currentGroup: string | undefined = undefined;
 
   constructor(private readonly logToConsole: boolean = true) {}
@@ -200,6 +200,19 @@ export class RecordingLogger implements Logger {
     }
   }
 
+  /**
+   * Checks whether the logged messages contain `messageOrRegExp`.
+   *
+   * If `messageOrRegExp` is a string, this function returns true as long as
+   * `messageOrRegExp` appears as part of one of the `messages`.
+   *
+   * If `messageOrRegExp` is a regular expression, this function returns true as long as
+   * one of the `messages` matches `messageOrRegExp`.
+   */
+  hasMessage(messageOrRegExp: string | RegExp): boolean {
+    return hasLoggedMessage(this.messages, messageOrRegExp);
+  }
+
   isDebug() {
     return true;
   }
@@ -238,41 +251,37 @@ export function getRecordingLogger(
   messages: LoggedMessage[],
   { logToConsole }: { logToConsole?: boolean } = { logToConsole: true },
 ): Logger {
-  return {
-    debug: (message: string) => {
-      messages.push({ type: "debug", message });
-      if (logToConsole) {
-        // eslint-disable-next-line no-console
-        console.debug(message);
-      }
-    },
-    info: (message: string) => {
-      messages.push({ type: "info", message });
-      if (logToConsole) {
-        // eslint-disable-next-line no-console
-        console.info(message);
-      }
-    },
-    warning: (message: string | Error) => {
-      messages.push({ type: "warning", message });
-      if (logToConsole) {
-        // eslint-disable-next-line no-console
-        console.warn(message);
-      }
-    },
-    error: (message: string | Error) => {
-      messages.push({ type: "error", message });
-      if (logToConsole) {
-        // eslint-disable-next-line no-console
-        console.error(message);
-      }
-    },
-    isDebug: () => true,
-    startGroup: () => undefined,
-    endGroup: () => undefined,
-  };
+  const logger = new RecordingLogger(logToConsole);
+  logger.messages = messages;
+  return logger;
 }
 
+/**
+ * Checks whether `messages` contains `messageOrRegExp`.
+ *
+ * If `messageOrRegExp` is a string, this function returns true as long as
+ * `messageOrRegExp` appears as part of one of the `messages`.
+ *
+ * If `messageOrRegExp` is a regular expression, this function returns true as long as
+ * one of the `messages` matches `messageOrRegExp`.
+ */
+function hasLoggedMessage(
+  messages: LoggedMessage[],
+  messageOrRegExp: string | RegExp,
+): boolean {
+  const check = (val: string) =>
+    typeof messageOrRegExp === "string"
+      ? val.includes(messageOrRegExp)
+      : messageOrRegExp.test(val);
+
+  return messages.some(
+    (msg) => typeof msg.message === "string" && check(msg.message),
+  );
+}
+
+/**
+ * Checks that `messages` contains all of `expectedMessages`.
+ */
 export function checkExpectedLogMessages(
   t: ExecutionContext<any>,
   messages: LoggedMessage[],
@@ -281,13 +290,7 @@ export function checkExpectedLogMessages(
   const missingMessages: string[] = [];
 
   for (const expectedMessage of expectedMessages) {
-    if (
-      !messages.some(
-        (msg) =>
-          typeof msg.message === "string" &&
-          msg.message.includes(expectedMessage),
-      )
-    ) {
+    if (!hasLoggedMessage(messages, expectedMessage)) {
       missingMessages.push(expectedMessage);
     }
   }
@@ -304,6 +307,20 @@ export function checkExpectedLogMessages(
   }
 }
 
+/**
+ * Asserts that `message` should not have been logged to `logger`.
+ */
+export function assertNotLogged(
+  t: ExecutionContext<any>,
+  logger: RecordingLogger,
+  message: string | RegExp,
+) {
+  t.false(
+    logger.hasMessage(message),
+    `'${message}' should not have been logged, but was.`,
+  );
+}
+
 /**
  * Initialises a recording logger and calls `body` with it.
  *