feat: Fixes #15 - Validate hostnames and adds capability for unit tests in dev

Author: zsteinkamp

Dockerfile

@@ -1,3 +1,4 @@
+# syntax=docker/dockerfile:1
 ARG NGINX_VERSION=1.24.0
 
 FROM node:18 AS builder
@@ -14,3 +15,25 @@ COPY --from=builder /app/dist/acme.js /usr/lib/nginx/njs_modules/acme.js
 COPY ./examples/nginx.conf /etc/nginx/nginx.conf
 RUN mkdir /etc/nginx/njs-acme
 RUN chown nginx: /etc/nginx/njs-acme
+
+# install the latest njs > 0.8.0 (not yet bundled with nginx-1.25.1)
+RUN --mount=type=cache,target=/var/cache/apt <<EOF
+    set -eux
+    export DEBIAN_FRONTEND=noninteractive
+    apt -qq update
+    apt install -qq  --yes --no-install-recommends --no-install-suggests \
+        curl gnupg2 ca-certificates lsb-release debian-archive-keyring
+    update-ca-certificates
+    curl https://nginx.org/keys/nginx_signing.key | gpg --dearmor \
+        | tee /usr/share/keyrings/nginx-archive-keyring.gpg >/dev/null
+    gpg --dry-run --quiet --no-keyring --import --import-options import-show \
+        /usr/share/keyrings/nginx-archive-keyring.gpg
+    echo "deb [signed-by=/usr/share/keyrings/nginx-archive-keyring.gpg] \
+        http://nginx.org/packages/mainline/debian `lsb_release -cs` nginx" \
+    | tee /etc/apt/sources.list.d/nginx.list
+    apt update -qq
+    apt install -qq  --yes --no-install-recommends --no-install-suggests \
+        nginx-module-njs
+    apt remove --purge --auto-remove --yes
+    rm -rf /var/lib/apt/lists/* /etc/apt/sources.list.d/nginx.list
+EOF

README.md

@@ -280,7 +280,8 @@ Look at `clientAutoMode` in [`src/index.ts`](./src/index.ts) to see how you can
 |                       Path              | Description |
 | ----                                    | ------------|
 | [src](src)                              | Contains your source code that will be compiled to the `dist/` directory. |
-| [integration-tests](integration-tests)  | Contains your source code of tests. |
+| [integration-tests](integration-tests)  | Integration tests. |
+| [unit-tests](unit-tests)                | Unit tests for code in `src/`. |
 
 ## Contributing
 

examples/nginx.conf

@@ -29,7 +29,7 @@ http {
 
   server {
     listen 0.0.0.0:8000; # testing with 8000 should be 80 in prod, pebble usees httpPort in dev/pebble/config.json
-    listen 443 ssl http2;
+    listen 443 ssl;
     server_name proxy.nginx.com;
 
     # The full set of configuration variables. These can also be defined in

integration-tests/acme-auto.test.ts

@@ -1,5 +1,4 @@
 import { strict as assert } from 'assert'
-import { test } from 'mocha'
 import './hooks'
 
 interface CertificateData {

src/tsconfig.json

@@ -29,7 +29,6 @@
     "resolveJsonModule": true,
     "rootDir": "../",
     "typeRoots": [],
-    "types": ["njs-types"]
   },
   "include": [".", "../package.json"],
   "exclude": ["../node_modules"],

src/utils.ts

@@ -3,8 +3,8 @@ import * as pkijs from 'pkijs'
 import * as asn1js from 'asn1js'
 import fs from 'fs'
 import querystring from 'querystring'
-import { ClientExternalAccountBindingOptions } from './client.js'
-import { Logger } from './logger.js'
+import { ClientExternalAccountBindingOptions } from './client'
+import { Logger } from './logger'
 
 const log = new Logger('utils')
 
@@ -833,7 +833,17 @@ export function acmeAltNames(r: NginxHTTPRequest): string[] {
 export function acmeServerNames(r: NginxHTTPRequest): string[] {
   const nameStr = getVariable(r, 'njs_acme_server_names') // no default == mandatory
   // split string value on comma and/or whitespace and lowercase each element
-  return nameStr.split(/[,\s]+/).map((n) => n.toLowerCase())
+  const names = nameStr.split(/[,\s]+/)
+  const invalidNames = names.filter((name) => !isValidHostname(name))
+
+  if (invalidNames.length > 0) {
+    const errMsg =
+      'Invalid hostname(s) in `njs_acme_server_names` detected: ' +
+      invalidNames.join(', ')
+    log.error(errMsg)
+    throw new Error(errMsg)
+  }
+  return names.map((n) => n.toLowerCase())
 }
 
 /**
@@ -926,3 +936,15 @@ export function joinPaths(...args: string[]): string {
   // join args with a slash remove duplicate slashes
   return args.join('/').replace(/\/+/g, '/')
 }
+
+export function isValidHostname(hostname: string): boolean {
+  return (
+    !!hostname &&
+    hostname.length < 256 &&
+    !!hostname.match(
+      // hostnames are dot-separated groups of letters, numbers, hyphens (but
+      // not beginning or ending with hyphens), and may end with a period
+      /^[a-z\d]([-a-z\d]{0,61}[a-z\d])?(\.[a-z\d]([-a-z\d]{0,61}[a-z\d])?)*\.?$/i
+    )
+  )
+}

unit-tests/.mocharc.js

@@ -9,6 +9,7 @@ module.exports = {
     'babel-register-ts',
     'source-map-support/register',
   ],
+  file: 'unit-tests/setupGlobals.ts',
   spec: [
     'unit-tests/**/*.test.ts',
   ],

unit-tests/setupGlobals.ts

@@ -0,0 +1,24 @@
+import { webcrypto } from 'crypto'
+
+/**
+ * Fake implementation of the logging functions of NGXObject
+ */
+class FakeNGX {
+  readonly logs: { level: number; message: NjsStringOrBuffer }[]
+  readonly INFO: number
+  readonly WARN: number
+  readonly ERR: number
+  constructor() {
+    this.logs = []
+    this.INFO = 1
+    this.WARN = 2
+    this.ERR = 3
+  }
+  log(level: number, message: NjsStringOrBuffer) {
+    this.logs.push({ level, message })
+  }
+}
+
+const globalAny: Record<string, unknown> = global
+globalAny.ngx = new FakeNGX()
+globalAny.crypto = webcrypto

unit-tests/tsconfig.json

@@ -1,7 +1,7 @@
 {
   "extends": "../tsconfig.json",
   "compilerOptions": {
-    "types": ["njs-types", "node"]
+    "types": ["node"]
   },
   "files": [
     "../node_modules/njs-types/ngx_http_js_module.d.ts",

unit-tests/utils.test.ts

@@ -0,0 +1,76 @@
+import assert from 'assert'
+import { describe, it } from 'mocha'
+import { acmeServerNames, isValidHostname } from '../src/utils'
+
+describe('utils', () => {
+  describe('isValidHostname', () => {
+    it('returns true for valid names', () => {
+      assert(isValidHostname('nginx.com'))
+      assert(isValidHostname('5guys.nginx.com'))
+      assert(isValidHostname('5guys.nginx.com.'))
+      assert(isValidHostname('5-guys.'))
+      assert(isValidHostname('5'))
+      assert(isValidHostname('a-z.123'))
+      assert(isValidHostname('a.'.repeat(100)))
+      assert(isValidHostname('a'.repeat(61) + '.com'))
+    })
+    it('returns false for invalid names', () => {
+      assert(!isValidHostname('.com'))
+      assert(!isValidHostname('.foobarbaz'))
+      assert(!isValidHostname('*.nginx.com'))
+      assert(!isValidHostname('-5guys.'))
+      assert(!isValidHostname('5guys-'))
+      assert(!isValidHostname('domäin.'))
+      assert(!isValidHostname('  '))
+      assert(!isValidHostname(''))
+      assert(!isValidHostname('a'.repeat(65) + '.com'))
+      assert(!isValidHostname('*'))
+      assert(
+        !isValidHostname(
+          // too long - 256 chars
+          '1234567890abcdef'.repeat(16)
+        )
+      )
+    })
+  })
+  describe('acmeServerNames', () => {
+    it('returns an array given valid input', () => {
+      const r = {
+        variables: {
+          njs_acme_server_names: null,
+        },
+      } as unknown as NginxHTTPRequest
+      const testCases = {
+        'nginx.com': 1,
+        'foo.bar.baz': 1,
+        'foo.bar.baz foo.baz': 2,
+        'foo. bar. baz.': 3,
+        'foo.,bar.': 2,
+        'foo.\tbar.': 2,
+        'foo.        bar.': 2,
+      }
+
+      for (const [names, expected] of Object.entries(testCases)) {
+        r.variables.njs_acme_server_names = names
+        const result = acmeServerNames(r)
+        assert(result.length === expected)
+      }
+    })
+    it('throws given invalid input', () => {
+      const r = {
+        variables: {
+          njs_acme_server_names: null,
+        },
+      } as unknown as NginxHTTPRequest
+      for (const name of [
+        'nginx-.com',
+        '*.bar.baz',
+        'foo.bar.baz *.baz',
+        '-foo. bar. baz.',
+      ]) {
+        r.variables.njs_acme_server_names = name
+        assert.throws(() => acmeServerNames(r))
+      }
+    })
+  })
+})