From 628b97c6dc5fa488897b88bbcb05aba9a2a05efb Mon Sep 17 00:00:00 2001
From: Matthew Bell <github@mdbell.me>
Date: Mon, 19 Aug 2024 16:47:44 -0400
Subject: [PATCH] ise ip instead of rolling my own ip address validation

---
 client/pages/config/authentication.vue    | 59 +++++++++++++++---
 package-lock.json                         |  9 +--
 package.json                              |  1 +
 server/Auth.js                            | 15 +++--
 server/objects/settings/ServerSettings.js |  2 +-
 server/utils/ForwardStrategy.js           | 74 +++--------------------
 6 files changed, 76 insertions(+), 84 deletions(-)

diff --git a/client/pages/config/authentication.vue b/client/pages/config/authentication.vue
index d301d6a68..a7e8cbafb 100644
--- a/client/pages/config/authentication.vue
+++ b/client/pages/config/authentication.vue
@@ -128,6 +128,10 @@
 </template>
 
 <script>
+import { isPrivate } from 'ip'
+import { isV4Format, isV6Format } from 'ip'
+
+
 export default {
   async asyncData({ store, redirect, app }) {
     if (!store.getters['user/getIsAdminOrUp']) {
@@ -300,11 +304,43 @@ export default {
       return isValid
     },
 
-    validateForwardAuth(){
-      // Checks for input in the format of:
-      // 255.255.255.255[/32] (IPv4 address with optional CIDR notation)
-      const pattern = new RegExp('^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\/([0-9]|[1-2][0-9]|3[0-2]))?$')
-      return pattern.test(this.newAuthSettings.authForwardAuthPattern)
+    validateForwardAuth(input){
+      let cidr = '';
+      let address = input;
+      let message = undefined;
+      console.log(input)
+      // Check to see if a cidr is included
+      if(input.includes('/')){
+        [address, cidr] = input.split('/');
+      }
+      if(isV4Format(address)) {
+        if(cidr && (cidr < 0 || cidr > 32)){
+          message = `'${cidr}' is an invalid CIDR range for ipv4, a valid range is between 0 and 32`;
+        }
+        else if (!cidr) {
+          // default to 32 if no cidr is provided
+          cidr = 32;
+        }
+      } else if (isV6Format(address)) {
+        if(cidr && (cidr < 0 || cidr > 128)){
+          message = `'${cidr}' is an invalid CIDR range for ipv6, a valid range is between 0 and 128`;
+        }else if (!cidr) {
+          // default to 128 if no cidr is provided
+          cidr = 128;
+        }
+      } else {
+        message = 'Address is not a valid ipv4 or ipv6 address';
+      }
+
+      if(!message && !isPrivate(address)){
+        message = `Address '${address}' is not a private address. For security reasons, only private addresses are allowed.`;
+      }
+      
+      return {
+        message,
+        address,
+        cidr
+        };
     },
 
     async saveSettings() {
@@ -317,9 +353,16 @@ export default {
         return
       }
 
-      if(this.newAuthSettings.authForwardAuthEnabled && !this.validateForwardAuth()){
-        this.$toast.error('Invalid forward auth pattern')
-        return
+      if(this.newAuthSettings.authForwardAuthEnabled){
+        const validationResults = this.validateForwardAuth(this.newAuthSettings.authForwardAuthPattern);
+        console.log(validationResults);
+        // if there is a message, then there was an error
+        if(validationResults.message){
+          this.$toast.error(validationResults.message);
+          return;
+        }
+        // ensure the address is in IP/CIDR format
+        this.newAuthSettings.authForwardAuthPattern = `${validationResults.address}/${validationResults.cidr}`;
       }
 
       if (!this.showCustomLoginMessage || !this.newAuthSettings.authLoginCustomMessage?.trim()) {
diff --git a/package-lock.json b/package-lock.json
index 168401a47..2e378ec93 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -15,6 +15,7 @@
         "express-session": "^1.17.3",
         "graceful-fs": "^4.2.10",
         "htmlparser2": "^8.0.1",
+        "ip": "^2.0.1",
         "lru-cache": "^10.0.3",
         "nodemailer": "^6.9.13",
         "openid-client": "^5.6.1",
@@ -2411,10 +2412,10 @@
       "integrity": "sha512-k/vGaX4/Yla3WzyMCvTQOXYeIHvqOKtnqBduzTHpzpQZzAskKMhZ2K+EnBiSM9zGSoIFeMpXKxa4dYeZIQqewQ=="
     },
     "node_modules/ip": {
-      "version": "2.0.0",
-      "resolved": "https://registry.npmjs.org/ip/-/ip-2.0.0.tgz",
-      "integrity": "sha512-WKa+XuLG1A1R0UWhl2+1XQSi+fZWMsYKffMZTTYsiZaUD8k2yDAj5atimTUD2TZkyCkNEeYE5NhFZmupOGtjYQ==",
-      "optional": true
+      "version": "2.0.1",
+      "resolved": "https://registry.npmjs.org/ip/-/ip-2.0.1.tgz",
+      "integrity": "sha512-lJUL9imLTNi1ZfXT+DU6rBBdbiKGBuay9B6xGSPVjUeQwaH1RIGqef8RZkUtHioLmSNpPR5M4HVKJGm1j8FWVQ==",
+      "license": "MIT"
     },
     "node_modules/ipaddr.js": {
       "version": "1.9.1",
diff --git a/package.json b/package.json
index c06428899..2817d7526 100644
--- a/package.json
+++ b/package.json
@@ -41,6 +41,7 @@
     "express-session": "^1.17.3",
     "graceful-fs": "^4.2.10",
     "htmlparser2": "^8.0.1",
+    "ip": "^2.0.1",
     "lru-cache": "^10.0.3",
     "nodemailer": "^6.9.13",
     "openid-client": "^5.6.1",
diff --git a/server/Auth.js b/server/Auth.js
index 4ec7dcffb..7ab176e3f 100644
--- a/server/Auth.js
+++ b/server/Auth.js
@@ -10,7 +10,8 @@ const ExtractJwt = require('passport-jwt').ExtractJwt
 const OpenIDClient = require('openid-client')
 const Database = require('./Database')
 const Logger = require('./Logger')
-const { ipInRange, ForwardStrategy } = require('./utils/ForwardStrategy')
+const ForwardStrategy = require('./utils/ForwardStrategy')
+const ip = require('ip')
 
 /**
  * @class Class for handling all the authentication related functionality.
@@ -77,18 +78,22 @@ class Auth {
     )
   }
 
-  async forwardAuthCheck(_req, user, ip, done) {
+  async forwardAuthCheck(_req, user, ipAddress, done) {
     const ipPattern = Database.serverSettings.authForwardAuthPattern
     if (!Database.serverSettings.authForwardAuthEnabled) {
       // silently deny access via forward strategy
       return done(null, null)
     }
     if (!ipPattern) {
-      Logger.warn(`[Auth] Forward strategy: Unauthorized access from ${ip}`)
+      Logger.warn(`[Auth] Forward strategy: Unauthorized access from ${ipAddress} User: ${user} Reason: No IP pattern`)
       return done(null, null)
     }
-    if (!ipInRange(ip, ipPattern)) {
-      Logger.warn(`[Auth] Forward strategy: Unauthorized access from ${ip}`)
+    if (!ip.isPrivate(ipAddress)) {
+      Logger.warn(`[Auth] Forward strategy: Unauthorized access from ${ipAddress} User: ${user} Reason: IP not private`)
+      return done(null, null)
+    }
+    if (!ip.cidrSubnet(ipPattern).contains(ipAddress)) {
+      Logger.warn(`[Auth] Forward strategy: Unauthorized access from ${ipAddress} User: ${user} Reason: IP not in range`)
       return done(null, null)
     }
 
diff --git a/server/objects/settings/ServerSettings.js b/server/objects/settings/ServerSettings.js
index 232162620..02f558aa8 100644
--- a/server/objects/settings/ServerSettings.js
+++ b/server/objects/settings/ServerSettings.js
@@ -80,7 +80,7 @@ class ServerSettings {
     this.authOpenIDAdvancedPermsClaim = ''
 
     // Forward Auth
-    this.authForwardAuthPattern = '127.0.0.1/0' // default to exact localhost
+    this.authForwardAuthPattern = '127.0.0.1/32' // default to exact localhost
     this.authForwardAuthPath = ''
     this.authForwardAuthEnabled = false
 
diff --git a/server/utils/ForwardStrategy.js b/server/utils/ForwardStrategy.js
index 50fcd4943..cd6a896a7 100644
--- a/server/utils/ForwardStrategy.js
+++ b/server/utils/ForwardStrategy.js
@@ -1,66 +1,6 @@
 const passport = require('passport')
 const requestIp = require('../libs/requestIp')
 
-function ipToBinary(ip) {
-  if (ip === '::1') {
-    ip = '127.0.0.1'
-  }
-  return ip
-    .split('.')
-    .map(Number)
-    .map((num) => num.toString(2).padStart(8, '0'))
-    .join('')
-}
-
-function binaryToIp(binary) {
-  return binary
-    .match(/.{8}/g)
-    .map((bin) => parseInt(bin, 2))
-    .join('.')
-}
-
-function cidrToMask(cidr) {
-  const mask = Array(32).fill('0')
-  for (let i = 0; i < cidr; i++) {
-    mask[i] = '1'
-  }
-  return mask
-    .join('')
-    .match(/.{8}/g)
-    .map((bin) => parseInt(bin, 2))
-    .join('.')
-}
-
-/**
- * Checks if an IP address falls within a given CIDR range.
- * If no range is provided, it assumes the CIDR + range are in the "0.0.0.0/0" format.
- *
- * @param {string} ip - The IP address to check. Can be either IPv4 or IPv6.
- * @param {string} cidr - The CIDR notation specifying the network. If `range` is not provided, this value represents the network IP and CIDR in "IP/CIDR" format.
- * @param {string} [range] - The IP address representing the range or network. Optional. If not provided, the function assumes "0.0.0.0/0" as the default range.
- * @returns {boolean} `true` if the IP address is within the given CIDR range; otherwise, `false`.
- */
-function ipInRange(ip, cidr, range) {
-  // if no range is provided, assume the cidr + range are in the "0.0.0.0/0" format
-  if (!range) {
-    let [rangeIp, rangeCidr] = cidr.split('/')
-    if (!rangeCidr) {
-      rangeCidr = '0' // if no cidr is provided, assume 0 (exact match)
-    }
-    return ipInRange(ip, rangeCidr, rangeIp)
-  }
-  const mask = cidrToMask(cidr)
-  const ipBin = ipToBinary(ip)
-  const maskBin = ipToBinary(mask)
-  const rangeBin = ipToBinary(range)
-
-  // Apply the mask to the IP and range
-  const ipNetwork = ipBin.substring(0, maskBin.length)
-  const rangeNetwork = rangeBin.substring(0, maskBin.length)
-
-  return ipNetwork === rangeNetwork
-}
-
 class ForwardStrategy extends passport.Strategy {
   /**
    * Creates a new ForwardStrategy instance.
@@ -88,12 +28,17 @@ class ForwardStrategy extends passport.Strategy {
    */
   authenticate(req) {
     const username = req.headers[this._header]
-    const ip = requestIp.getClientIp(req)
+    const ipAddress = requestIp.getClientIp(req)
+
     if (!username) {
       return this.fail('No username found')
     }
 
-    this._verify(req, username, ip, (err, user) => {
+    if (!ipAddress) {
+      return this.fail('No IP address found')
+    }
+
+    this._verify(req, username, ipAddress, (err, user) => {
       if (err) {
         return this.error(err)
       }
@@ -105,7 +50,4 @@ class ForwardStrategy extends passport.Strategy {
   }
 }
 
-module.exports = {
-  ForwardStrategy,
-  ipInRange
-}
+module.exports = ForwardStrategy