added comments to firewall rules and updated tests

src/server/utils/firewall.ts

@@ -21,6 +21,18 @@ type ParsedEntry = {
   proto?: 'tcp' | 'udp' | 'both';
 };
 
+/**
+ * Sanitize a client identifier for use in an iptables comment.
+ * Strips all characters except ASCII alphanumeric, space, underscore, hyphen, and dot.
+ * Combines with client ID for a safe, identifiable comment.
+ * Truncates to 256 bytes (iptables comment module limit).
+ */
+function sanitizeComment(clientId: number, clientName: string): string {
+  const safe = clientName.replace(/[^a-zA-Z0-9 _.-]/g, '');
+  const comment = `client ${clientId}: ${safe}`;
+  return comment.slice(0, 256);
+}
+
 /**
  * Parse a firewall entry string into its components.
  * Supports formats:
@@ -109,22 +121,28 @@ function parseFirewallEntry(entry: string): ParsedEntry {
 function generateRuleArgs(
   clientIp: string,
   entry: ParsedEntry,
+  comment?: string,
   action: 'A' | 'D' = 'A'
 ): string[] {
   const rules: string[] = [];
+  const commentArg = comment ? ` -m comment --comment "${comment}"` : '';
   const baseArgs = `-${action} ${CHAIN_NAME} -s ${clientIp} -d ${entry.ip}`;
 
   if (entry.port) {
     // Port-specific rules
     if (entry.proto === 'tcp' || entry.proto === 'both') {
-      rules.push(`${baseArgs} -p tcp --dport ${entry.port} -j ACCEPT`);
+      rules.push(
+        `${baseArgs} -p tcp --dport ${entry.port}${commentArg} -j ACCEPT`
+      );
     }
     if (entry.proto === 'udp' || entry.proto === 'both') {
-      rules.push(`${baseArgs} -p udp --dport ${entry.port} -j ACCEPT`);
+      rules.push(
+        `${baseArgs} -p udp --dport ${entry.port}${commentArg} -j ACCEPT`
+      );
     }
   } else {
     // No port - allow all traffic to destination
-    rules.push(`${baseArgs} -j ACCEPT`);
+    rules.push(`${baseArgs}${commentArg} -j ACCEPT`);
   }
 
   return rules;
@@ -181,19 +199,21 @@ export const firewall = {
       `Applying firewall rules for client ${client.name} (${client.id}): ${effectiveIps.join(', ')}`
     );
 
+    const comment = sanitizeComment(client.id, client.name);
+
     for (const ipEntry of effectiveIps) {
       const parsed = parseFirewallEntry(ipEntry);
       const destIsIpv6 = isIPv6(parsed.ip.split('/')[0]); // Handle CIDR by checking base IP
 
       if (destIsIpv6) {
         if (enableIpv6) {
-          const rules = generateRuleArgs(client.ipv6Address, parsed);
+          const rules = generateRuleArgs(client.ipv6Address, parsed, comment);
           for (const rule of rules) {
             await exec(`ip6tables ${rule}`);
           }
         }
       } else {
-        const rules = generateRuleArgs(client.ipv4Address, parsed);
+        const rules = generateRuleArgs(client.ipv4Address, parsed, comment);
         for (const rule of rules) {
           await exec(`iptables ${rule}`);
         }
@@ -324,7 +344,8 @@ export const firewall = {
   },
 };
 
-export const testExports = {
+export const firewallTestExports = {
   parseFirewallEntry,
   generateRuleArgs,
+  sanitizeComment,
 };

src/server/utils/types.ts

@@ -259,4 +259,4 @@ export function assertUnreachable(_: never): never {
   throw new Error("Didn't expect to get here");
 }
 
-export const testExports = { FirewallIpEntrySchema };
+export const typesTestExports = { FirewallIpEntrySchema };

src/test/unit/firewall.spec.ts

@@ -1,103 +1,107 @@
 import { describe, expect, test } from 'vitest';
-import { testExports } from '../../server/utils/firewall';
-import { testExports as typesTextExports } from '../../server/utils/types';
+import { firewallTestExports } from '../../server/utils/firewall';
+import { typesTestExports } from '../../server/utils/types';
 
 describe('firewall', () => {
   describe('isValidFirewallEntry', () => {
     test('invalid ips', () => {
-      expect(() => typesTextExports.FirewallIpEntrySchema.parse('')).toThrow();
+      expect(() => typesTestExports.FirewallIpEntrySchema.parse('')).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('255.255.255.256')
+        typesTestExports.FirewallIpEntrySchema.parse('255.255.255.256')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('1.1.1.256')
+        typesTestExports.FirewallIpEntrySchema.parse('1.1.1.256')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('1.1.1.1.1')
+        typesTestExports.FirewallIpEntrySchema.parse('1.1.1.1.1')
       ).toThrow();
 
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('[]:443/udp')
+        typesTestExports.FirewallIpEntrySchema.parse('[]:443/udp')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('[]:443')
+        typesTestExports.FirewallIpEntrySchema.parse('[]:443')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('[::1]/32')
+        typesTestExports.FirewallIpEntrySchema.parse('[::1]/32')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('[1.1.1.1]/32')
+        typesTestExports.FirewallIpEntrySchema.parse('[1.1.1.1]/32')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('[::g]/32')
+        typesTestExports.FirewallIpEntrySchema.parse('[::g]/32')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('2001:dbx::1')
+        typesTestExports.FirewallIpEntrySchema.parse('2001:dbx::1')
       ).toThrow();
     });
     test('invalid port, protocol or cidr', () => {
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('1.1.1.1:80/tcpp')
+        typesTestExports.FirewallIpEntrySchema.parse('1.1.1.1:80/tcpp')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('1.1.1.1:65536')
+        typesTestExports.FirewallIpEntrySchema.parse('1.1.1.1:65536')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('1.1.1.1:0')
+        typesTestExports.FirewallIpEntrySchema.parse('1.1.1.1:0')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('1.1.1.1/33')
+        typesTestExports.FirewallIpEntrySchema.parse('1.1.1.1/33')
       ).toThrow();
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('1.1.1.1/32:0')
+        typesTestExports.FirewallIpEntrySchema.parse('1.1.1.1/32:0')
       ).toThrow();
     });
     test('protocol without port', () => {
       expect(() =>
-        typesTextExports.FirewallIpEntrySchema.parse('1.1.1.1/tcp')
+        typesTestExports.FirewallIpEntrySchema.parse('1.1.1.1/tcp')
       ).toThrow();
     });
     test('valid entries', () => {
-      expect(typesTextExports.FirewallIpEntrySchema.parse('1.1.1.1')).toBe(
+      expect(typesTestExports.FirewallIpEntrySchema.parse('1.1.1.1')).toBe(
         '1.1.1.1'
       );
-      expect(typesTextExports.FirewallIpEntrySchema.parse('::/0')).toBe('::/0');
-      expect(typesTextExports.FirewallIpEntrySchema.parse('::0/0')).toBe(
+      expect(typesTestExports.FirewallIpEntrySchema.parse('::/0')).toBe('::/0');
+      expect(typesTestExports.FirewallIpEntrySchema.parse('::0/0')).toBe(
         '::0/0'
       );
-      expect(typesTextExports.FirewallIpEntrySchema.parse('2001:db8::1')).toBe(
+      expect(typesTestExports.FirewallIpEntrySchema.parse('2001:db8::1')).toBe(
         '2001:db8::1'
       );
-      expect(typesTextExports.FirewallIpEntrySchema.parse('::1')).toBe('::1');
+      expect(typesTestExports.FirewallIpEntrySchema.parse('::1')).toBe('::1');
       expect(
-        typesTextExports.FirewallIpEntrySchema.parse('2001:db8::1/32')
+        typesTestExports.FirewallIpEntrySchema.parse('2001:db8::1/32')
       ).toBe('2001:db8::1/32');
-      expect(typesTextExports.FirewallIpEntrySchema.parse('[::1]')).toBe(
+      expect(typesTestExports.FirewallIpEntrySchema.parse('[::1]')).toBe(
         '[::1]'
       );
-      expect(testExports.parseFirewallEntry('[::1]')).toEqual({
+      expect(firewallTestExports.parseFirewallEntry('[::1]')).toEqual({
         ip: '::1',
       });
     });
   });
   describe('parseFirewallEntry', () => {
     test('IPv4 with port and protocol', () => {
-      expect(() => testExports.parseFirewallEntry('1.1.1.1/tcp')).toThrow();
-      expect(() => testExports.parseFirewallEntry('1.1.1.1/udp')).toThrow();
-      expect(testExports.parseFirewallEntry('1.1.1.1')).toEqual({
+      expect(() =>
+        firewallTestExports.parseFirewallEntry('1.1.1.1/tcp')
+      ).toThrow();
+      expect(() =>
+        firewallTestExports.parseFirewallEntry('1.1.1.1/udp')
+      ).toThrow();
+      expect(firewallTestExports.parseFirewallEntry('1.1.1.1')).toEqual({
         ip: '1.1.1.1',
       });
-      expect(testExports.parseFirewallEntry('1.1.1.1:80')).toEqual({
+      expect(firewallTestExports.parseFirewallEntry('1.1.1.1:80')).toEqual({
         ip: '1.1.1.1',
         port: 80,
         proto: 'both',
       });
-      expect(testExports.parseFirewallEntry('1.1.1.1:80/tcp')).toEqual({
+      expect(firewallTestExports.parseFirewallEntry('1.1.1.1:80/tcp')).toEqual({
         ip: '1.1.1.1',
         port: 80,
         proto: 'tcp',
       });
-      expect(testExports.parseFirewallEntry('1.1.1.1:80/udp')).toEqual({
+      expect(firewallTestExports.parseFirewallEntry('1.1.1.1:80/udp')).toEqual({
         ip: '1.1.1.1',
         port: 80,
         proto: 'udp',
@@ -106,34 +110,102 @@ describe('firewall', () => {
 
     test('IPv6 with port and protocol', () => {
       expect(() =>
-        testExports.parseFirewallEntry('[2001:db8::1]/tcp')
+        firewallTestExports.parseFirewallEntry('[2001:db8::1]/tcp')
       ).toThrow();
       expect(() =>
-        testExports.parseFirewallEntry('[2001:db8::1]/udp')
+        firewallTestExports.parseFirewallEntry('[2001:db8::1]/udp')
       ).toThrow();
-      expect(testExports.parseFirewallEntry('[2001:db8::1]')).toEqual({
+      expect(firewallTestExports.parseFirewallEntry('[2001:db8::1]')).toEqual({
         ip: '2001:db8::1',
       });
-      expect(testExports.parseFirewallEntry('[2001:db8::1]:443')).toEqual({
+      expect(
+        firewallTestExports.parseFirewallEntry('[2001:db8::1]:443')
+      ).toEqual({
         ip: '2001:db8::1',
         port: 443,
         proto: 'both',
       });
-      expect(testExports.parseFirewallEntry('[2001:db8::1]:443/tcp')).toEqual({
+      expect(
+        firewallTestExports.parseFirewallEntry('[2001:db8::1]:443/tcp')
+      ).toEqual({
         ip: '2001:db8::1',
         port: 443,
         proto: 'tcp',
       });
-      expect(testExports.parseFirewallEntry('[2001:db8::1]:443/udp')).toEqual({
+      expect(
+        firewallTestExports.parseFirewallEntry('[2001:db8::1]:443/udp')
+      ).toEqual({
         ip: '2001:db8::1',
         port: 443,
         proto: 'udp',
       });
 
-      expect(testExports.parseFirewallEntry('::0/0')).toEqual({
+      expect(firewallTestExports.parseFirewallEntry('::0/0')).toEqual({
         ip: '::0/0',
       });
-      expect(() => testExports.parseFirewallEntry('::0/0/tcp')).toThrow();
+      expect(() =>
+        firewallTestExports.parseFirewallEntry('::0/0/tcp')
+      ).toThrow();
+    });
+  });
+  describe('sanitizeComment', () => {
+    test('basic ASCII name', () => {
+      expect(firewallTestExports.sanitizeComment(1, 'My Laptop')).toBe(
+        'client 1: My Laptop'
+      );
+    });
+    test('strips non-ASCII and shell metacharacters', () => {
+      expect(firewallTestExports.sanitizeComment(42, 'café')).toBe(
+        'client 42: caf'
+      );
+      expect(firewallTestExports.sanitizeComment(5, 'a"; rm -rf /')).toBe(
+        'client 5: a rm -rf '
+      );
+      expect(firewallTestExports.sanitizeComment(7, 'test$(cmd)`id`')).toBe(
+        'client 7: testcmdid'
+      );
+    });
+    test('preserves allowed punctuation', () => {
+      expect(firewallTestExports.sanitizeComment(3, 'phone-2.lan_home')).toBe(
+        'client 3: phone-2.lan_home'
+      );
+    });
+    test('truncates to 256 bytes', () => {
+      const longName = 'a'.repeat(300);
+      const result = firewallTestExports.sanitizeComment(1, longName);
+      expect(result.length).toBeLessThanOrEqual(256);
+      expect(result).toBe('client 1: ' + 'a'.repeat(246));
+    });
+  });
+  describe('generateRuleArgs', () => {
+    test('includes comment when provided', () => {
+      const rules = firewallTestExports.generateRuleArgs(
+        '10.8.0.2',
+        { ip: '10.0.0.1' },
+        'client 1: test'
+      );
+      expect(rules).toEqual([
+        '-A WG_CLIENTS -s 10.8.0.2 -d 10.0.0.1 -m comment --comment "client 1: test" -j ACCEPT',
+      ]);
+    });
+    test('omits comment when not provided', () => {
+      const rules = firewallTestExports.generateRuleArgs('10.8.0.2', {
+        ip: '10.0.0.1',
+      });
+      expect(rules).toEqual([
+        '-A WG_CLIENTS -s 10.8.0.2 -d 10.0.0.1 -j ACCEPT',
+      ]);
+    });
+    test('comment with port generates two rules for both proto', () => {
+      const rules = firewallTestExports.generateRuleArgs(
+        '10.8.0.2',
+        { ip: '10.0.0.1', port: 443, proto: 'both' },
+        'client 2: phone'
+      );
+      expect(rules).toEqual([
+        '-A WG_CLIENTS -s 10.8.0.2 -d 10.0.0.1 -p tcp --dport 443 -m comment --comment "client 2: phone" -j ACCEPT',
+        '-A WG_CLIENTS -s 10.8.0.2 -d 10.0.0.1 -p udp --dport 443 -m comment --comment "client 2: phone" -j ACCEPT',
+      ]);
     });
   });
 });