Small code quality improvements (#2553)

* Small code quality improvements

- Fix misleading JSDoc comment in cache.ts
- Mitigate timing-based username enumeration in Basic auth
- Extract duplicated TOTP configuration into private method
- Replace manual peer counter with clients.length in Prometheus metrics
- Simplify isValidPasswordHash return expression

* reset session.ts

this is currently worked on in the dev-oauth branch

* reset password.ts

no need to change

* specify unit for cache function

* remove type assertion

---------

Co-authored-by: Anghios <[email protected]>
Co-authored-by: Bernd Storath <[email protected]>

src/server/database/repositories/user/service.ts

@@ -58,6 +58,17 @@ export class UserService {
     this.#statements = createPreparedStatement(db);
   }
 
+  #createTotp(user: { username: string; totpKey: string }) {
+    return new TOTP({
+      issuer: 'wg-easy',
+      label: user.username,
+      algorithm: 'SHA1',
+      digits: 6,
+      period: 30,
+      secret: user.totpKey,
+    });
+  }
+
   async getAll() {
     return this.#statements.findAll.execute();
   }
@@ -156,22 +167,13 @@ export class UserService {
         if (!code) {
           return { success: false, error: 'TOTP_REQUIRED' };
         } else {
-          if (!txUser.totpKey) {
+          const totpKey = txUser.totpKey;
+          if (!totpKey) {
             return { success: false, error: 'UNEXPECTED_ERROR' };
           }
 
-          const totp = new TOTP({
+          const totp = this.#createTotp({ username: txUser.username, totpKey });
-            issuer: 'wg-easy',
+          if (totp.validate({ token: code, window: 1 }) === null) {
-            label: txUser.username,
-            algorithm: 'SHA1',
-            digits: 6,
-            period: 30,
-            secret: txUser.totpKey,
-          });
-
-          const valid = totp.validate({ token: code, window: 1 });
-
-          if (valid === null) {
             return { success: false, error: 'INVALID_TOTP_CODE' };
           }
         }
@@ -195,22 +197,13 @@ export class UserService {
         throw new Error('User not found');
       }
 
-      if (!txUser.totpKey) {
+      const totpKey = txUser.totpKey;
+      if (!totpKey) {
         throw new Error('TOTP key is not set');
       }
 
-      const totp = new TOTP({
+      const totp = this.#createTotp({ username: txUser.username, totpKey });
-        issuer: 'wg-easy',
+      if (totp.validate({ token: code, window: 1 }) === null) {
-        label: txUser.username,
-        algorithm: 'SHA1',
-        digits: 6,
-        period: 30,
-        secret: txUser.totpKey,
-      });
-
-      const valid = totp.validate({ token: code, window: 1 });
-
-      if (valid === null) {
         throw new Error('Invalid TOTP code');
       }
 

src/server/routes/metrics/prometheus.get.ts

@@ -6,14 +6,12 @@ export default defineMetricsHandler('prometheus', async ({ event }) => {
 async function getPrometheusResponse() {
   const wgInterface = await Database.interfaces.get();
   const clients = await WireGuard.getAllClients();
-  let wireguardPeerCount = 0;
   let wireguardEnabledPeersCount = 0;
   let wireguardConnectedPeersCount = 0;
   const wireguardSentBytes = [];
   const wireguardReceivedBytes = [];
   const wireguardLatestHandshakeSeconds = [];
   for (const client of clients) {
-    wireguardPeerCount++;
     if (client.enabled === true) {
       wireguardEnabledPeersCount++;
     }
@@ -41,7 +39,7 @@ async function getPrometheusResponse() {
   const returnText = [
     '# HELP wireguard_configured_peers',
     '# TYPE wireguard_configured_peers gauge',
-    `wireguard_configured_peers{${id}} ${wireguardPeerCount}`,
+    `wireguard_configured_peers{${id}} ${clients.length}`,
     '',
     '# HELP wireguard_enabled_peers',
     '# TYPE wireguard_enabled_peers gauge',

src/server/utils/cache.ts

@@ -6,7 +6,7 @@ type Opts = {
 };
 
 /**
- * Cache function for 1 hour
+ * Cache the result of a function for the given expiry time in milliseconds
  */
 export function cacheFunction<T>(fn: () => T, { expiry }: Opts): () => T {
   let cache: { value: T; expiry: number } | null = null;