diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 6082113..01ebc78 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -32,5 +32,17 @@ jobs: - name: Install dependencies run: npm ci + # Release-time guard (issue #111): publishing x.y.z requires a matching "# x.y.z ..." + # release-notes heading in CHANGELOG.md, so the changelog cannot silently fall out of + # date. Keeping the check here (instead of on every PR) avoids blocking non-user-facing + # PRs while still enforcing the CONTRIBUTING.md convention where it matters. + - name: Check CHANGELOG.md covers this release + run: | + VERSION="$(node -p "require('./package.json').version")" + if ! grep -Eq "^# ${VERSION//./\\.}( |$)" CHANGELOG.md; then + echo "::error file=CHANGELOG.md::No release-notes heading for version ${VERSION}. Move the '# Unreleased' notes into a '# ${VERSION} Release notes (YYYY-MM-DD)' section before publishing." + exit 1 + fi + - name: Publish run: npm publish --provenance --access public diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fe380f..69501e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,14 @@ point (one `resolver.resolve()` call site instead of three). No behavior change — the pipeline is locked by new characterization tests across v1/v2 mounts, namespaces, and extra headers. Closes #110. +- Non-2xx Vault responses are now rejected with a typed `VaultHttpError` (part of the `VaultError` + hierarchy, exported from the errors module), so callers can `instanceof`-check HTTP failures. + Backward compatible: the message (`" - "`) and the `statusCode`/`error` + properties are unchanged, and the thrown value is still an `instanceof Error`. +- Auth backends now fail fast with `InvalidArgumentsError` when a required config field is missing: + `role_id` for AppRole, `role` for IAM and Kubernetes (previously only Token auth validated its + input, and a missing/empty config surfaced later as an unhelpful login failure or `TypeError`). + Valid configurations behave exactly as before. # 2.0.4 Release notes (2026-07-02) diff --git a/src/VaultApiClient.js b/src/VaultApiClient.js index 2674a3c..fbeff69 100644 --- a/src/VaultApiClient.js +++ b/src/VaultApiClient.js @@ -1,5 +1,7 @@ 'use strict'; +const errors = require('./errors'); + /** * Joins URL segments with single slashes while preserving the leading * protocol (e.g. "https://"). Replaces the former `url-join` dependency. @@ -111,10 +113,7 @@ class VaultApiClient { } if (!response.ok) { - const error = new Error(`${response.status} - ${text}`); - error.statusCode = response.status; - error.error = body; - throw error; + throw new errors.VaultHttpError(response.status, text, body); } // Do not log the response body: Vault responses carry secret diff --git a/src/VaultNodeConfig.js b/src/VaultNodeConfig.js index 73421f6..b19bf85 100644 --- a/src/VaultNodeConfig.js +++ b/src/VaultNodeConfig.js @@ -148,3 +148,6 @@ class VaultNodeConfig { } module.exports = VaultNodeConfig; +// The prototype-pollution guard in deepMerge is a deliberate security control; +// the helper is exported so tests can exercise it directly. +module.exports.deepMerge = deepMerge; diff --git a/src/auth/VaultAppRoleAuth.js b/src/auth/VaultAppRoleAuth.js index 3bcb428..c6911c6 100644 --- a/src/auth/VaultAppRoleAuth.js +++ b/src/auth/VaultAppRoleAuth.js @@ -1,6 +1,7 @@ 'use strict'; const VaultBaseAuth = require('./VaultBaseAuth'); +const errors = require('../errors'); class VaultAppRoleAuth extends VaultBaseAuth { @@ -17,6 +18,10 @@ class VaultAppRoleAuth extends VaultBaseAuth { constructor(apiClient, logger, config, mount) { super(apiClient, logger, mount || 'approle'); + if (!config || !config.role_id) { + throw new errors.InvalidArgumentsError('"role_id" should be provided for VaultAppRoleAuth'); + } + this.__roleId = config.role_id; this.__secretId = config.secret_id; } diff --git a/src/auth/VaultBaseAuth.js b/src/auth/VaultBaseAuth.js index 010448a..9fa62f1 100644 --- a/src/auth/VaultBaseAuth.js +++ b/src/auth/VaultBaseAuth.js @@ -1,5 +1,9 @@ 'use strict'; +// `long-timeout` is unmaintained (last release 2016) but tiny (~50 LOC), dependency-free and +// stable: it only works around setTimeout's 32-bit signed-int limit (~24.8 days), which real +// Vault token TTLs can exceed. Audited and deliberately kept as-is (see issue #111); revisit +// only if a maintained alternative or a Node.js core fix appears. const lt = require('long-timeout'); const AuthToken = require('./AuthToken'); const errors = require('../errors'); diff --git a/src/auth/VaultIAMAuth.js b/src/auth/VaultIAMAuth.js index 73b8340..5170ed5 100644 --- a/src/auth/VaultIAMAuth.js +++ b/src/auth/VaultIAMAuth.js @@ -65,6 +65,10 @@ class VaultIAMAuth extends VaultBaseAuth { constructor(api, logger, config, mount) { super(api, logger, mount || 'aws'); + if (!config || !config.role) { + throw new errors.InvalidArgumentsError('"role" should be provided for VaultIAMAuth'); + } + this.__role = config.role; this.__iam_server_id_header_value = config.iam_server_id_header_value; this.__region = config.region; diff --git a/src/auth/VaultKubernetesAuth.js b/src/auth/VaultKubernetesAuth.js index 9ec8a7c..ddc10c6 100644 --- a/src/auth/VaultKubernetesAuth.js +++ b/src/auth/VaultKubernetesAuth.js @@ -1,5 +1,6 @@ const fs = require('fs'); const VaultBaseAuth = require('./VaultBaseAuth'); +const errors = require('../errors'); class VaultKubernetesAuth extends VaultBaseAuth { /** @@ -15,6 +16,10 @@ class VaultKubernetesAuth extends VaultBaseAuth { constructor(apiClient, logger, config, mount) { super(apiClient, logger, mount || 'kubernetes'); + if (!config || !config.role) { + throw new errors.InvalidArgumentsError('"role" should be provided for VaultKubernetesAuth'); + } + this.__role = config.role; this.__tokenPath = '/var/run/secrets/kubernetes.io/serviceaccount/token'; if (config.tokenPath !== undefined) { diff --git a/src/auth/VaultTokenAuth.js b/src/auth/VaultTokenAuth.js index e60167a..adff9c7 100644 --- a/src/auth/VaultTokenAuth.js +++ b/src/auth/VaultTokenAuth.js @@ -15,7 +15,7 @@ class VaultTokenAuth extends VaultBaseAuth { constructor(apiClient, logger, config, mount) { super(apiClient, logger, mount || 'token'); - if (!config.token) { + if (!config || !config.token) { throw new errors.InvalidArgumentsError('Auth token should be provided for VaultTokenAuth'); } diff --git a/src/errors.js b/src/errors.js index c441fe5..29c009d 100644 --- a/src/errors.js +++ b/src/errors.js @@ -19,10 +19,33 @@ class InvalidAWSCredentialsError extends InvalidArgumentsError {} class AuthTokenExpiredError extends VaultError {} class UnsupportedOperationError extends VaultError {} +/** + * A non-2xx HTTP response from the Vault server. + * + * Preserves the legacy plain-`Error` shape previously thrown by + * {@link VaultApiClient#makeRequest} — the message stays `" - "` and the + * `statusCode`/`error` properties keep their meaning — while letting callers + * `instanceof`-check HTTP failures against the `VaultError` hierarchy. + */ +class VaultHttpError extends VaultError { + /** + * @param {number} statusCode - HTTP status code of the response. + * @param {string} text - raw response body text, used in the message. + * @param {*} [body] - parsed JSON body when the response was valid JSON, the raw text + * otherwise, or `undefined` for an empty body. Exposed as `error.error`. + */ + constructor(statusCode, text, body) { + super(`${statusCode} - ${text}`); + this.statusCode = statusCode; + this.error = body; + } +} + module.exports = { VaultError, InvalidArgumentsError, InvalidAWSCredentialsError, AuthTokenExpiredError, UnsupportedOperationError, + VaultHttpError, }; diff --git a/test/Lease.test.mjs b/test/Lease.test.mjs index 30f4d3a..c9eeaa1 100644 --- a/test/Lease.test.mjs +++ b/test/Lease.test.mjs @@ -55,6 +55,25 @@ describe('Lease', function () { }); }); + describe('#getMetadata()', function () { + const metadata = { created_time: '2026-07-02T00:00:00Z', version: 3 }; + + it('returns the KV v2 version metadata when present', function () { + const lease = new Lease('r', 'l', 10, false, { a: 1 }, metadata); + expect(lease.getMetadata()).to.equal(metadata); + }); + + it('is mapped from response.metadata by fromResponse', function () { + const lease = Lease.fromResponse({ ...response, metadata }); + expect(lease.getMetadata()).to.deep.equal(metadata); + }); + + it('returns undefined for KV v1 / non-KV leases', function () { + expect(Lease.fromResponse(response).getMetadata()).to.equal(undefined); + expect(new Lease('r', 'l', 10, false, {}).getMetadata()).to.equal(undefined); + }); + }); + describe('#isRenewable()', function () { it('reflects the renewable flag', function () { expect(new Lease('r', 'l', 10, true, {}).isRenewable()).to.equal(true); diff --git a/test/VaultApiClient.test.mjs b/test/VaultApiClient.test.mjs index 99b2b36..7c4c555 100644 --- a/test/VaultApiClient.test.mjs +++ b/test/VaultApiClient.test.mjs @@ -4,6 +4,7 @@ import sinon from 'sinon'; import { expect, use } from 'chai'; import sinonChai from 'sinon-chai'; import VaultApiClient from '../src/VaultApiClient.js'; +import errors from '../src/errors.js'; use(sinonChai); @@ -125,6 +126,61 @@ describe('VaultApiClient', function () { ); }); + it('rejects a non-2xx JSON response with a VaultHttpError carrying the parsed body on err.error', function () { + responder = (req, res) => { + res.statusCode = 403; + res.setHeader('Content-Type', 'application/json'); + res.end('{"errors":["permission denied"]}'); + }; + const api = new VaultApiClient({ url: baseUrl }, logger); + return api.makeRequest('GET', '/secret/foo').then( + () => { throw new Error('expected rejection'); }, + (err) => { + expect(err).to.be.instanceOf(errors.VaultHttpError); + expect(err).to.be.instanceOf(errors.VaultError); + expect(err).to.be.instanceOf(Error); + // Legacy plain-Error shape is preserved: same message, same properties. + expect(err.message).to.equal('403 - {"errors":["permission denied"]}'); + expect(err.statusCode).to.equal(403); + expect(err.error).to.deep.equal({ errors: ['permission denied'] }); + } + ); + }); + + it('falls back to the raw text on err.error when the error body is not JSON', function () { + responder = (req, res) => { + res.statusCode = 500; + res.end('boom'); + }; + const api = new VaultApiClient({ url: baseUrl }, logger); + return api.makeRequest('GET', '/secret/foo').then( + () => { throw new Error('expected rejection'); }, + (err) => { + expect(err).to.be.instanceOf(errors.VaultHttpError); + expect(err.message).to.equal('500 - boom'); + expect(err.statusCode).to.equal(500); + expect(err.error).to.equal('boom'); + } + ); + }); + + it('leaves err.error undefined when the error body is empty', function () { + responder = (req, res) => { + res.statusCode = 500; + res.end(); + }; + const api = new VaultApiClient({ url: baseUrl }, logger); + return api.makeRequest('GET', '/secret/foo').then( + () => { throw new Error('expected rejection'); }, + (err) => { + expect(err).to.be.instanceOf(errors.VaultHttpError); + expect(err.message).to.equal('500 - '); + expect(err.statusCode).to.equal(500); + expect(err.error).to.equal(undefined); + } + ); + }); + it('merges config.requestOptions.headers, with per-request headers taking precedence', function () { const api = new VaultApiClient({ url: baseUrl, diff --git a/test/VaultNodeConfig.test.mjs b/test/VaultNodeConfig.test.mjs index 3561562..bfd2bef 100644 --- a/test/VaultNodeConfig.test.mjs +++ b/test/VaultNodeConfig.test.mjs @@ -106,6 +106,56 @@ describe('VaultNodeConfig', function () { }); }); + describe('deepMerge()', function () { + const { deepMerge } = VaultNodeConfig; + + it('is exported for direct use in tests', function () { + expect(deepMerge).to.be.a('function'); + }); + + it('skips an own __proto__ key so a malicious source cannot pollute prototypes', function () { + // JSON.parse creates an *own* "__proto__" data property (an object literal would + // set the prototype instead), which is exactly how polluted input arrives. + const source = JSON.parse('{"__proto__": {"polluted": "yes"}, "safe": 1}'); + const target = {}; + deepMerge(target, source); + + expect(target.safe).to.equal(1); + expect(Object.hasOwn(target, '__proto__')).to.equal(false); + expect(Object.getPrototypeOf(target)).to.equal(Object.prototype); + expect({}.polluted, 'Object.prototype must not be polluted').to.equal(undefined); + }); + + it('skips constructor and prototype keys', function () { + const source = JSON.parse('{"constructor": {"prototype": {"polluted": "yes"}}, "prototype": {"polluted": "yes"}, "safe": 1}'); + const target = {}; + deepMerge(target, source); + + expect(target.safe).to.equal(1); + expect(Object.hasOwn(target, 'constructor')).to.equal(false); + expect(Object.hasOwn(target, 'prototype')).to.equal(false); + expect(target.constructor).to.equal(Object); + expect({}.polluted, 'Object.prototype must not be polluted').to.equal(undefined); + }); + + it('merges nested objects in place, skips undefined source values and returns the target', function () { + const target = { nested: { keep: 1, replace: 1 }, scalar: 'a' }; + const result = deepMerge(target, { nested: { replace: 2, add: 3 }, scalar: undefined }); + + expect(result).to.equal(target); + expect(target).to.deep.equal({ nested: { keep: 1, replace: 2, add: 3 }, scalar: 'a' }); + }); + + it('deep-clones object values assigned over non-object targets', function () { + const source = { obj: { a: 1 } }; + const target = { obj: 'scalar' }; + deepMerge(target, source); + + expect(target.obj).to.deep.equal({ a: 1 }); + expect(target.obj, 'must not share the source reference').to.not.equal(source.obj); + }); + }); + describe('#populate()', function () { function instance(substitutionMap, dataByPath) { process.env.NODE_CONFIG_DIR = CONFIG_BASE; diff --git a/test/auth.appRole.test.mjs b/test/auth.appRole.test.mjs index 89d0c44..8df4969 100644 --- a/test/auth.appRole.test.mjs +++ b/test/auth.appRole.test.mjs @@ -4,6 +4,7 @@ import { expect, use } from 'chai'; import sinonChai from 'sinon-chai'; import VaultApiClient from '../src/VaultApiClient.js'; import VaultAppRoleAuth from '../src/auth/VaultAppRoleAuth.js'; +import errors from '../src/errors.js'; use(sinonChai); @@ -113,6 +114,18 @@ describe('AppRole auth backend', function () { }); }); + describe('config validation', function () { + it('throws InvalidArgumentsError when role_id is missing', () => { + expect(() => new VaultAppRoleAuth(getApiStub(), logger, { secret_id: 'secret456' })) + .to.throw(errors.InvalidArgumentsError, '"role_id" should be provided for VaultAppRoleAuth'); + }); + + it('throws InvalidArgumentsError when the config is missing entirely', () => { + expect(() => new VaultAppRoleAuth(getApiStub(), logger, undefined)) + .to.throw(errors.InvalidArgumentsError, '"role_id" should be provided for VaultAppRoleAuth'); + }); + }); + describe('does not leak the client_token to the logger (regression #104)', function () { const CLIENT_TOKEN = 's.RAWAPPROLETOKENSHOULDNEVERBELOGGED'; const ACCESSOR = 'approle-accessor-1234'; diff --git a/test/auth.base.test.mjs b/test/auth.base.test.mjs index 791542c..a86b39f 100644 --- a/test/auth.base.test.mjs +++ b/test/auth.base.test.mjs @@ -251,6 +251,8 @@ describe('VaultBaseAuth', function () { const auth = new TestAuth(api, 'mount', { authStub: sinon.stub().resolves(renewable) }); auth._log = _.assign({}, logger, { error: errorSpy }); + let renewalsAfterFirstFailure; + return auth.getAuthToken() .then(() => { clock.tick(50000); @@ -259,6 +261,15 @@ describe('VaultBaseAuth', function () { .then(() => { expect(api.makeRequest).to.have.been.calledWith('POST', '/auth/token/renew-self'); expect(errorSpy).to.have.been.called; + // The timer must actually re-arm after the failure ... + expect(auth.__refreshTimeout).to.not.equal(null); + renewalsAfterFirstFailure = api.makeRequest.callCount; + clock.tick(50000); + return flush(); + }) + .then(() => { + // ... so advancing the clock again triggers another renewal attempt. + expect(api.makeRequest.callCount).to.be.greaterThan(renewalsAfterFirstFailure); }); }); }); diff --git a/test/auth.iam.test.mjs b/test/auth.iam.test.mjs index a838029..85e7008 100644 --- a/test/auth.iam.test.mjs +++ b/test/auth.iam.test.mjs @@ -295,6 +295,18 @@ describe('Unit AWS auth backend :: IAM', function () { }); }); + describe('config validation', function () { + it('Should throw InvalidArgumentsError if the role is missing', () => { + expect(() => new VaultIAMAuth(getApiStub(), logger, {}, 'aws')) + .to.throw(errors.InvalidArgumentsError, '"role" should be provided for VaultIAMAuth'); + }); + + it('Should throw InvalidArgumentsError if the config is missing entirely', () => { + expect(() => new VaultIAMAuth(getApiStub(), logger, undefined, 'aws')) + .to.throw(errors.InvalidArgumentsError, '"role" should be provided for VaultIAMAuth'); + }); + }); + describe('base64 encoding', function () { it('encodes via Buffer.from (no deprecated new Buffer) and round-trips', function () { const auth = new VaultIAMAuth(getApiStub(), logger, { role: 'MyRole' }, 'aws'); diff --git a/test/auth.kubernetes.test.mjs b/test/auth.kubernetes.test.mjs index 3a9390e..7fa4f28 100644 --- a/test/auth.kubernetes.test.mjs +++ b/test/auth.kubernetes.test.mjs @@ -6,6 +6,7 @@ import sinonChai from 'sinon-chai'; import VaultApiClient from '../src/VaultApiClient.js'; import VaultKubernetesAuth from '../src/auth/VaultKubernetesAuth.js'; import AuthToken from '../src/auth/AuthToken.js'; +import errors from '../src/errors.js'; use(sinonChai); @@ -64,6 +65,24 @@ describe('VaultKubernetesAuth', function () { }); }); + it('throws InvalidArgumentsError when the role is missing', function () { + expect(() => new VaultKubernetesAuth(apiStub(), logger, {})) + .to.throw(errors.InvalidArgumentsError, '"role" should be provided for VaultKubernetesAuth'); + expect(() => new VaultKubernetesAuth(apiStub(), logger, undefined)) + .to.throw(errors.InvalidArgumentsError, '"role" should be provided for VaultKubernetesAuth'); + }); + + it('propagates the fs error when the service-account token file is missing or unreadable, without hitting Vault', function () { + const fsError = new Error("ENOENT: no such file or directory, open '/var/run/secrets/kubernetes.io/serviceaccount/token'"); + fsError.code = 'ENOENT'; + readFileSync = sinon.stub(fs, 'readFileSync').throws(fsError); + const api = apiStub(); + const auth = new VaultKubernetesAuth(api, logger, { role: 'r' }); + + expect(() => auth._authenticate()).to.throw(fsError); + expect(api.makeRequest).to.not.have.been.called; + }); + it('never logs the JWT or the Vault client token', function () { readFileSync = sinon.stub(fs, 'readFileSync').returns(Buffer.from('super-secret-jwt')); const debug = sinon.spy(); diff --git a/test/auth.token.test.mjs b/test/auth.token.test.mjs index 924d264..32fba2c 100644 --- a/test/auth.token.test.mjs +++ b/test/auth.token.test.mjs @@ -21,6 +21,11 @@ describe('VaultTokenAuth', function () { .to.throw(errors.InvalidArgumentsError, 'Auth token should be provided'); }); + it('throws when the config is missing entirely', function () { + expect(() => new VaultTokenAuth(apiStub(), logger, undefined)) + .to.throw(errors.InvalidArgumentsError, 'Auth token should be provided'); + }); + it('defaults the mount to "token"', function () { const auth = new VaultTokenAuth(apiStub(), logger, { token: 't' }); expect(auth._mount).to.equal('token'); diff --git a/test/errors.test.mjs b/test/errors.test.mjs index 9efe5e3..5ac3d51 100644 --- a/test/errors.test.mjs +++ b/test/errors.test.mjs @@ -9,6 +9,7 @@ describe('errors', function () { 'InvalidAWSCredentialsError', 'AuthTokenExpiredError', 'UnsupportedOperationError', + 'VaultHttpError', ]); }); @@ -63,5 +64,32 @@ describe('errors', function () { expect(err.name).to.equal('UnsupportedOperationError'); expect(err.message).to.equal('not supported'); }); + + it('VaultHttpError extends VaultError', function () { + const err = new errors.VaultHttpError(503, 'sealed'); + expect(err).to.be.instanceOf(errors.VaultError); + expect(err).to.be.instanceOf(Error); + expect(err.name).to.equal('VaultHttpError'); + }); + }); + + describe('VaultHttpError', function () { + it('keeps the legacy " - " message shape', function () { + const err = new errors.VaultHttpError(403, '{"errors":["permission denied"]}'); + expect(err.message).to.equal('403 - {"errors":["permission denied"]}'); + }); + + it('exposes the status code as `statusCode` and the parsed body as `error`', function () { + const body = { errors: ['permission denied'] }; + const err = new errors.VaultHttpError(403, '{"errors":["permission denied"]}', body); + expect(err.statusCode).to.equal(403); + expect(err.error).to.equal(body); + }); + + it('leaves `error` undefined when the response body is empty', function () { + const err = new errors.VaultHttpError(500, ''); + expect(err.message).to.equal('500 - '); + expect(err.error).to.equal(undefined); + }); }); });