From 07eec5e25993e8c4aedaa2572185bce7bd458a9f Mon Sep 17 00:00:00 2001 From: Graham Campbell Date: Fri, 12 Jun 2026 21:20:09 +0100 Subject: [PATCH] Harden JS file address property access --- lib/configuration/variables/sources/file.js | 20 ++++++++++--------- lib/utils/safe-object.js | 8 ++++++++ .../variables/sources/file.test.js | 14 +++++++++++++ .../fixture/file-property-access-errored.js | 19 ++++++++++++++++++ 4 files changed, 52 insertions(+), 9 deletions(-) create mode 100644 test/unit/lib/configuration/variables/sources/fixture/file-property-access-errored.js diff --git a/lib/configuration/variables/sources/file.js b/lib/configuration/variables/sources/file.js index f9ecc6fe1..1ba5d6824 100644 --- a/lib/configuration/variables/sources/file.js +++ b/lib/configuration/variables/sources/file.js @@ -4,12 +4,10 @@ const ensureString = require('type/string/ensure'); const isPlainFunction = require('type/plain-function/is'); const path = require('path'); const fsp = require('fs').promises; -const { isProxy } = require('util').types; const yaml = require('js-yaml'); const cloudformationSchema = require('../../../utils/serverless-utils/cloudformation-schema'); const ServerlessError = require('../../../serverless-error'); -const { hasOwn } = require('../../../utils/safe-object'); -const { isUnsafePropertyKey } = require('../../../utils/object-path'); +const { canFollowProperty } = require('../../../utils/safe-object'); const readFile = async (filePath, serviceDir) => { try { @@ -153,13 +151,17 @@ module.exports = { for (const propertyKey of propertyKeys) { if (typeof result === 'string') result = await resolveVariablesInString(result); if (result == null) return { value: null }; - if (!hasOwn(result, propertyKey)) { - // `hasOwnProperty` does not consult Proxy "has"/"get" traps, so virtual - // properties of Proxy-backed JS file results must be resolved through - // property access. Prototype internals stay blocked unless claimed as own. - if (!isProxy(result) || isUnsafePropertyKey(propertyKey)) return { value: null }; + try { + if (!canFollowProperty(result, propertyKey)) return { value: null }; + result = result[propertyKey]; + } catch (error) { + throw new ServerlessError( + `Cannot resolve "${address}" out of "${path.basename( + filePath + )}": Property access errored with: ${error && error.stack ? error.stack : error}`, + 'JS_FILE_PROPERTY_ACCESS_ERROR' + ); } - result = result[propertyKey]; if (result == null) return { value: null }; if (!isResolvedByFunction) { if (isPlainFunction(result)) { diff --git a/lib/utils/safe-object.js b/lib/utils/safe-object.js index 931f9de58..b9303d42a 100644 --- a/lib/utils/safe-object.js +++ b/lib/utils/safe-object.js @@ -1,11 +1,18 @@ 'use strict'; +const { isProxy } = require('util').types; const { isUnsafePropertyKey } = require('./object-path'); const hasOwnProperty = Object.prototype.hasOwnProperty; const hasOwn = (object, key) => object != null && hasOwnProperty.call(object, key); +// `hasOwnProperty` does not consult Proxy "has"/"get" traps, so virtual properties +// of Proxy-backed values can only be reached via property access. Such access must +// never traverse prototype internals, unless these are claimed as own properties. +const canFollowProperty = (value, key) => + hasOwn(value, key) || (isProxy(value) && !isUnsafePropertyKey(key)); + const dataDescriptor = (value) => Object.assign(Object.create(null), { value, @@ -54,6 +61,7 @@ const getOwnByPath = (source, path) => { }; module.exports = { + canFollowProperty, createRegistry, getOwnByPath, hasOwn, diff --git a/test/unit/lib/configuration/variables/sources/file.test.js b/test/unit/lib/configuration/variables/sources/file.test.js index f2837ecdf..bca392001 100644 --- a/test/unit/lib/configuration/variables/sources/file.test.js +++ b/test/unit/lib/configuration/variables/sources/file.test.js @@ -54,6 +54,8 @@ describe('test/unit/lib/configuration/variables/sources/file.test.js', () => { '${file(file-property-function-errored-non-error.js):property}', jsFilePropertyFunctionAccessUnresolvableProperty: '${file(file-property-function-access-unresolvable-property.js):property}', + jsFilePropertyProxyTrapErrored: '${file(file-property-access-errored.js):proxy.boom}', + jsFilePropertyGetterErrored: '${file(file-property-access-errored.js):getter.boom}', jsFilePropertyPromise: '${file(file-property-promise.js):property}', notFile: '${file(dir.yaml)}', noParams: '${file:}', @@ -207,6 +209,18 @@ describe('test/unit/lib/configuration/variables/sources/file.test.js', () => { 'VARIABLE_RESOLUTION_ERROR' )); + it('should report with an error a Proxy "get" trap that crashes during address resolution', () => { + const { error } = variablesMeta.get('jsFilePropertyProxyTrapErrored'); + expect(error.code).to.equal('VARIABLE_RESOLUTION_ERROR'); + expect(error.message).to.include('Property access errored with'); + }); + + it('should report with an error an own getter that crashes during address resolution', () => { + const { error } = variablesMeta.get('jsFilePropertyGetterErrored'); + expect(error.code).to.equal('VARIABLE_RESOLUTION_ERROR'); + expect(error.message).to.include('Property access errored with'); + }); + it('should report with an error non file paths', () => expect(variablesMeta.get('notFile').error.code).to.equal('VARIABLE_RESOLUTION_ERROR')); diff --git a/test/unit/lib/configuration/variables/sources/fixture/file-property-access-errored.js b/test/unit/lib/configuration/variables/sources/fixture/file-property-access-errored.js new file mode 100644 index 000000000..2aed0b113 --- /dev/null +++ b/test/unit/lib/configuration/variables/sources/fixture/file-property-access-errored.js @@ -0,0 +1,19 @@ +'use strict'; + +module.exports = { + proxy: new Proxy( + {}, + { + get(target, key) { + if (key === 'boom') throw new Error('Proxy get trap crashed'); + return target[key]; + }, + } + ), + getter: Object.defineProperty({}, 'boom', { + enumerable: true, + get() { + throw new Error('Getter crashed'); + }, + }), +};