From 85a6a39e8b834e7ed001b18133861b8858fa1456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20Paw=C5=82owski?= Date: Tue, 10 Dec 2024 14:14:17 +0100 Subject: [PATCH] SNOW-1846395 improve url data sanitization (#980) --- lib/constants/sf_params.js | 10 +- lib/errors.js | 2 +- lib/http/node.js | 9 +- lib/http/request_util.js | 215 +++++++++++++++++++++++++++++++---- lib/logger/logging_util.js | 61 +++++++++- lib/request_util_test.js | 149 ++++++++++++++++++++++++ test/integration/testUtil.js | 6 +- test/unit/snowflake_test.js | 7 +- 8 files changed, 417 insertions(+), 42 deletions(-) create mode 100644 lib/request_util_test.js diff --git a/lib/constants/sf_params.js b/lib/constants/sf_params.js index cd8cdf400..3f2b9747f 100644 --- a/lib/constants/sf_params.js +++ b/lib/constants/sf_params.js @@ -1,2 +1,8 @@ -exports.paramsNames = {}; -exports.paramsNames.SF_REQUEST_GUID = 'request_guid'; +exports.paramsNames = Object.freeze({ + SF_REQUEST_GUID: 'request_guid', + SF_REQUEST_ID: 'requestId', + SF_TOKEN: 'token', + SF_WAREHOUSE_NAME: 'warehouse', + SF_DB_NAME: 'databaseName', + SF_SCHEMA_NAME: 'schemaName', +}); diff --git a/lib/errors.js b/lib/errors.js index 4875c781d..9a31d81fd 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -342,7 +342,7 @@ exports.createOperationFailedError = function ( return createError(errorTypes.OperationFailedError, { code: errorCode, - data: { ...data, queryId: null }, + data: data, message: message, sqlState: sqlState }); diff --git a/lib/http/node.js b/lib/http/node.js index ab6b8cc63..f911e3cf8 100644 --- a/lib/http/node.js +++ b/lib/http/node.js @@ -10,6 +10,7 @@ const HttpsProxyAgent = require('../agent/https_proxy_agent'); const HttpAgent = require('http').Agent; const GlobalConfig = require('../../lib/global_config'); const Logger = require('../logger'); +const RequestUtil = require('../http/request_util'); /** * Returns the delay time calculated by exponential backoff with @@ -107,19 +108,19 @@ function isBypassProxy(proxy, destination, agentId) { * @inheritDoc */ NodeHttpClient.prototype.getAgent = function (parsedUrl, proxy, mock) { - Logger.getInstance().trace('Agent[url: %s] - getting an agent instance.', parsedUrl.href); + Logger.getInstance().trace('Agent[url: %s] - getting an agent instance.', RequestUtil.describeURL(parsedUrl.href)); if (!proxy && GlobalConfig.isEnvProxyActive()) { const isHttps = parsedUrl.protocol === 'https:'; proxy = ProxyUtil.getProxyFromEnv(isHttps); if (proxy) { - Logger.getInstance().debug('Agent[url: %s] - proxy info loaded from the environment variable. Proxy host: %s', parsedUrl.href, proxy.host); + Logger.getInstance().debug('Agent[url: %s] - proxy info loaded from the environment variable. Proxy host: %s', RequestUtil.describeURL(parsedUrl.href), proxy.host); } } return getProxyAgent(proxy, parsedUrl, parsedUrl.href, mock); }; function getProxyAgent(proxyOptions, parsedUrl, destination, mock) { - Logger.getInstance().trace('Agent[url: %s] - getting a proxy agent instance.', parsedUrl.href); + Logger.getInstance().trace('Agent[url: %s] - getting a proxy agent instance.', RequestUtil.describeURL(parsedUrl.href)); const agentOptions = { protocol: parsedUrl.protocol, hostname: parsedUrl.hostname, @@ -129,7 +130,7 @@ function getProxyAgent(proxyOptions, parsedUrl, destination, mock) { if (mock) { const mockAgent = mock.agentClass(agentOptions); if (mockAgent.protocol === parsedUrl.protocol) { - Logger.getInstance().debug('Agent[url: %s] - the mock agent will be used.', parsedUrl.href); + Logger.getInstance().debug('Agent[url: %s] - the mock agent will be used.', RequestUtil.describeURL(parsedUrl.href)); return mockAgent; } } diff --git a/lib/http/request_util.js b/lib/http/request_util.js index 0a778d5e5..38f69222e 100644 --- a/lib/http/request_util.js +++ b/lib/http/request_util.js @@ -1,37 +1,202 @@ +const LoggingUtil = require('../logger/logging_util'); const sfParams = require('../constants/sf_params'); +// Initial whitelist for attributes - they will be described with values +const DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITH_VALUES = [ + 'baseUrl', + 'path', + 'method', + sfParams.paramsNames.SF_REQUEST_ID, + sfParams.paramsNames.SF_REQUEST_GUID, + sfParams.paramsNames.SF_WAREHOUSE_NAME, + sfParams.paramsNames.SF_DB_NAME, + sfParams.paramsNames.SF_SCHEMA_NAME, +]; + +// Initial blacklist for attributes - described as present/not present only +const DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITHOUT_VALUES = [ + sfParams.paramsNames.SF_TOKEN +]; + +// Helper function to resolve attributes arrays given defaults and overrides. +function resolveAttributeList(defaultAttrs, overrideAttrs) { + return overrideAttrs || defaultAttrs; +} + /** - * Should work with not yet parsed options as well (before calling prepareRequestOptions method). + * Describes a request based on its options. + * Should work with not-yet-parsed options as well (before calling prepareRequestOptions method). * - * @param requestOptions - object representing the request data with top-level keys - * @returns {string} + * @param {Object} requestOptions - Object representing the request data with top-level keys. + * @param {Object} [options] - Options for describing attributes. + * @param {Array} [options.overrideAttributesDescribedWithValues] + * @param {Array} [options.overrideAttributesDescribedWithoutValues] + * @returns {string} A string representation of the request data. */ -exports.describeRequestFromOptions = function (requestOptions) { - return exports.describeRequestData({ method: requestOptions.method, url: requestOptions.url, guid: requestOptions.params?.[sfParams.paramsNames.SF_REQUEST_GUID] }); -}; +function describeRequestFromOptions( + requestOptions, + { + overrideAttributesDescribedWithValues, + overrideAttributesDescribedWithoutValues + } = {} +) { + const describingAttributesWithValues = resolveAttributeList( + DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITH_VALUES, + overrideAttributesDescribedWithValues + ); + + const describingAttributesWithoutValues = resolveAttributeList( + DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITHOUT_VALUES, + overrideAttributesDescribedWithoutValues + ); + + const { method, url, params } = requestOptions || {}; + + return describeRequestData( + { method, url, params }, + describingAttributesWithValues, + describingAttributesWithoutValues + ); +} /** - * Creates string that represents response data. - * Should allow to identify request, which was the source for this response. - * @param response - axios response object - * @returns {string} + * Creates a string that represents request data from a response. + * Helps to identify the request that was the source of the response. + * + * @param {Object} response - Axios response object. + * @param {Object} [options] - Options for describing attributes. + * @param {Array} [options.overrideAttributesDescribedWithValues] + * @param {Array} [options.overrideAttributesDescribedWithoutValues] + * @returns {string} A string representation of the request data. */ -exports.describeRequestFromResponse = function (response) { - let methodUppercase = undefined; - let url = undefined; - let guid = undefined; - const responseConfig = response.config; +function describeRequestFromResponse( + response, + { + overrideAttributesDescribedWithValues, + overrideAttributesDescribedWithoutValues + } = {} +) { + let method; + let url; + let params; + const responseConfig = response?.config; + + const describingAttributesWithValues = resolveAttributeList( + DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITH_VALUES, + overrideAttributesDescribedWithValues + ); + + const describingAttributesWithoutValues = resolveAttributeList( + DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITHOUT_VALUES, + overrideAttributesDescribedWithoutValues + ); + if (responseConfig) { - // Uppercase is used to allow finding logs corresponding to the same request - response pair, - // by matching identical options' descriptions. - methodUppercase = responseConfig.method.toUpperCase(); + method = responseConfig.method; url = responseConfig.url; - guid = responseConfig.params?.[sfParams.paramsNames.SF_REQUEST_GUID]; + params = responseConfig.params; + } + + return describeRequestData( + { method, url, params }, + describingAttributesWithValues, + describingAttributesWithoutValues + ); +} + +/** + * Constructs a string representation of request data. + * + * @param {Object} requestData - Object containing the method, url, and parameters. + * @param {string} requestData.method - HTTP method. + * @param {string} requestData.url - Request URL. + * @param {Object} [requestData.params] - Additional query parameters. + * @param {Array} attributesWithValues - Attributes to describe with values. + * @param {Array} attributesWithoutValues - Attributes to describe without values. + * @returns {string} A string describing the request data. + */ +function describeRequestData( + { method, url, params } = {}, + attributesWithValues, + attributesWithoutValues +) { + const requestObject = { + // Ensure consistent casing for methods to match request-response pairs in logs. + method: method?.toUpperCase(), + ...constructURLData(url, params), + }; + + return LoggingUtil.describeAttributes( + requestObject, + attributesWithValues, + attributesWithoutValues + ); +} + +/** + * Constructs an object representing URL data including the base URL, path, and query parameters. + * + * @param {string} url - The full URL. + * @param {Object} [params] - Additional query parameters. + * @returns {Object} Contains baseUrl, path, and merged query parameters. + */ +function constructURLData(url, params = {}) { + if (!url) { + return { baseUrl: undefined, path: undefined, queryParams: {} }; } - return exports.describeRequestData({ method: methodUppercase, url: url, guid: guid }); -}; -exports.describeRequestData = function ({ method, url, guid } = {}) { - const guidDescription = guid ? `, guid: ${guid}` : ''; - return `[method: ${method}, url: ${url}` + guidDescription + ']'; -}; \ No newline at end of file + const urlObj = new URL(url); + const queryParams = { ...params }; + + urlObj.searchParams.forEach((value, key) => { + queryParams[key] = value; + }); + + const baseUrl = `${urlObj.protocol}//${urlObj.hostname}${urlObj.port ? `:${urlObj.port}` : ''}`; + + return { + baseUrl: baseUrl, + path: urlObj.pathname, + ...queryParams, + }; +} + +/** + * @param {string} url - The URL to describe. + * @param {Object} [options] - Options for describing attributes. + * @param {Array} [options.overrideAttributesDescribedWithValues] + * @param {Array} [options.overrideAttributesDescribedWithoutValues] + * @returns {string} A string describing the URL. + */ +function describeURL( + url, + { + overrideAttributesDescribedWithValues, + overrideAttributesDescribedWithoutValues + } = {} +) { + const describingAttributesWithValues = resolveAttributeList( + DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITH_VALUES, + overrideAttributesDescribedWithValues + ); + + const describingAttributesWithoutValues = resolveAttributeList( + DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITHOUT_VALUES, + overrideAttributesDescribedWithoutValues + ); + + const urlData = constructURLData(url); + + return LoggingUtil.describeAttributes( + urlData, + describingAttributesWithValues, + describingAttributesWithoutValues + ); +} + +exports.DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITH_VALUES = DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITH_VALUES; +exports.DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITHOUT_VALUES = DEFAULT_ATTRIBUTES_DESCRIBING_REQUEST_WITHOUT_VALUES; + +exports.describeRequestFromOptions = describeRequestFromOptions; +exports.describeRequestFromResponse = describeRequestFromResponse; +exports.describeURL = describeURL; diff --git a/lib/logger/logging_util.js b/lib/logger/logging_util.js index a8ca1e26e..b6fcbc3a3 100644 --- a/lib/logger/logging_util.js +++ b/lib/logger/logging_util.js @@ -1,8 +1,61 @@ const Util = require('../util'); -exports.describePresence = function (valueToDescribe) { - const VAR_PROVIDED_TEXT = 'provided'; - const VAR_NOT_PROVIDED_TEXT = 'not provided'; +const PROVIDED_TEXT = 'provided'; +const NOT_PROVIDED_TEXT = 'not provided'; - return Util.isNotEmptyAsString(valueToDescribe) ? VAR_PROVIDED_TEXT : VAR_NOT_PROVIDED_TEXT; +/** + * Describes the presence of a given value. If the value is not empty (as a string), + * returns the corresponding text (by default: 'provided' or 'not provided'). + * + * @param {*} valueToDescribe - The value to check for presence. + * @param {Object} [options] - Optional overrides for the "provided" and "not provided" text. + * @param {string} [options.overrideProvidedText] + * @param {string} [options.overrideNotProvidedText] + * @returns {string} A string indicating the presence of `valueToDescribe`. + */ +exports.describePresence = function (valueToDescribe, { overrideProvidedText, overrideNotProvidedText } = {}) { + const providedText = overrideProvidedText || PROVIDED_TEXT; + const notProvidedText = overrideNotProvidedText || NOT_PROVIDED_TEXT; + return Util.isNotEmptyAsString(valueToDescribe) ? providedText : notProvidedText; +}; + +/** + * @param {Object} sourceObject - The object holding attribute values. + * @param {Array} attributesWithValues - Attributes to show with their values. + * @param {Array} attributesWithoutValues - Attributes to show as present/not present. + * @returns {string} Comma-separated string describing the attributes. + */ +exports.attributesToString = function ( + sourceObject = {}, + attributesWithValues = [], + attributesWithoutValues = [] +) { + const withValues = attributesWithValues + .filter(attr => sourceObject[attr] !== undefined) + .map(attr => `${attr}=${String(sourceObject[attr])}`); + + const withoutValues = attributesWithoutValues + .filter(attr => sourceObject[attr] !== undefined) + .map(attr => `${attr} is ${exports.describePresence(sourceObject[attr])}`); + + return [...withValues, ...withoutValues].join(', '); +}; + +/** + * @param {Object} sourceObject - The object holding attribute values. + * @param {Array} attributesWithValues - Attributes to show with their values. + * @param {Array} attributesWithoutValues - Attributes to show as present/not present. + * @returns {string} A bracketed string of described attributes. + */ +exports.describeAttributes = function ( + sourceObject, + attributesWithValues, + attributesWithoutValues +) { + const attributesDescription = exports.attributesToString( + sourceObject, + attributesWithValues, + attributesWithoutValues + ); + return `[${attributesDescription}]`; }; diff --git a/lib/request_util_test.js b/lib/request_util_test.js new file mode 100644 index 000000000..06f272936 --- /dev/null +++ b/lib/request_util_test.js @@ -0,0 +1,149 @@ +const assert = require('assert'); +const RequestUtil = require('../lib/http/request_util'); +const sfParams = require('../lib/constants/sf_params'); + +describe('RequestUtil', function () { + describe('describeRequestFromOptions', function () { + it('should describe request with default attributes when given basic request options', function () { + const requestOptions = { + method: 'get', + url: 'https://example.com:8080/api/data?foo=bar', + params: { + requestId: '12345', + // eslint-disable-next-line camelcase + request_guid: 'abcde', + warehouse: 'test_warehouse', + databaseName: 'test_db', + schemaName: 'test_schema', + } + }; + + const result = RequestUtil.describeRequestFromOptions(requestOptions); + + // Check if defaults appear correctly + assert.ok(result.includes('method=GET'), 'Should include method=GET'); + assert.ok(result.includes('baseUrl=https://example.com:8080'), 'Should include baseUrl'); + assert.ok(result.includes('path=/api/data'), 'Should include path'); + assert.ok(result.includes(`${sfParams.paramsNames.SF_REQUEST_ID}=12345`), 'Should include requestId'); + assert.ok(result.includes(`${sfParams.paramsNames.SF_REQUEST_GUID}=abcde`), 'Should include request_guid'); + assert.ok(result.includes(`${sfParams.paramsNames.SF_WAREHOUSE_NAME}=test_warehouse`), 'Should include warehouse'); + assert.ok(result.includes(`${sfParams.paramsNames.SF_DB_NAME}=test_db`), 'Should include databaseName'); + assert.ok(result.includes(`${sfParams.paramsNames.SF_SCHEMA_NAME}=test_schema`), 'Should include schemaName'); + + // token not present, should not appear + assert.ok(!result.includes('token='), 'Should not include token='); + assert.ok(!result.includes('token is'), 'Should not include token presence'); + }); + + it('should handle when no requestOptions are provided', function () { + const result = RequestUtil.describeRequestFromOptions(undefined); + // Should be empty brackets since nothing can be described + assert.strictEqual(result, '[]', 'Should return empty brackets'); + }); + + it('should use overridden attributes lists if provided', function () { + const requestOptions = { + method: 'post', + url: 'https://example.org/resource?test=1', + params: { + token: 'secret_token_value' + } + }; + + const overrideAttributesDescribedWithValues = ['baseUrl']; + const overrideAttributesDescribedWithoutValues = ['token']; + + const result = RequestUtil.describeRequestFromOptions(requestOptions, { + overrideAttributesDescribedWithValues, + overrideAttributesDescribedWithoutValues + }); + + assert.ok(result.includes('baseUrl=https://example.org'), 'Should include only baseUrl with value'); + assert.ok(result.includes('token is provided'), 'Should indicate token is provided'); + // Should not have path or other defaults + assert.ok(!result.includes('path='), 'Should not include path'); + }); + }); + + describe('describeRequestFromResponse', function () { + it('should describe the request from a response config', function () { + const response = { + config: { + method: 'post', + url: 'https://api.example.com/action?query=value', + params: { + // eslint-disable-next-line camelcase + request_guid: 'xyz123', + warehouse: 'wh1' + } + } + }; + + const result = RequestUtil.describeRequestFromResponse(response); + assert.ok(result.includes('method=POST'), 'Should have POST method'); + assert.ok(result.includes('baseUrl=https://api.example.com'), 'Should have correct baseUrl'); + assert.ok(result.includes('path=/action'), 'Should have correct path'); + assert.ok(result.includes(`${sfParams.paramsNames.SF_REQUEST_GUID}=xyz123`), 'Should have request_guid'); + assert.ok(result.includes(`${sfParams.paramsNames.SF_WAREHOUSE_NAME}=wh1`), 'Should have warehouse'); + }); + + it('should handle a response without config', function () { + const response = {}; + const result = RequestUtil.describeRequestFromResponse(response); + // No config means no attributes described + assert.strictEqual(result, '[]', 'Should return empty brackets'); + }); + + it('should allow overriding described attributes from a response', function () { + const response = { + config: { + method: 'get', + url: 'https://myapp.com/test?user=john', + params: { + token: 'abcd' + } + } + }; + + const result = RequestUtil.describeRequestFromResponse(response, { + overrideAttributesDescribedWithValues: ['baseUrl'], + overrideAttributesDescribedWithoutValues: ['token'] + }); + + assert.ok(result.includes('baseUrl=https://myapp.com'), 'Should include overridden baseUrl'); + assert.ok(result.includes('token is provided'), 'Should indicate token is provided'); + assert.ok(!result.includes('path='), 'Should not include path since overridden'); + }); + }); + + describe('describeURL', function () { + it('should describe a URL using default attributes', function () { + const url = 'https://example.net:3000/endpoint/sub?flag=true'; + const result = RequestUtil.describeURL(url); + + assert.ok(result.includes('baseUrl=https://example.net:3000'), 'Should include baseUrl'); + assert.ok(result.includes('path=/endpoint/sub'), 'Should include path'); + + // Other defaults would not show because not set + assert.ok(!result.includes('requestId='), 'Should not have requestId'); + assert.ok(!result.includes('token='), 'Should not have token'); + }); + + it('should describe a URL with overridden attributes', function () { + const url = 'https://something.com/path'; + const result = RequestUtil.describeURL(url, { + overrideAttributesDescribedWithValues: ['baseUrl', 'path'], + overrideAttributesDescribedWithoutValues: ['token'] + }); + + assert.ok(result.includes('baseUrl=https://something.com'), 'Should have baseUrl'); + assert.ok(result.includes('path=/path'), 'Should have path'); + assert.ok(!result.includes('token is'), 'Should not indicate token presence'); + }); + + it('should handle empty or invalid URLs', function () { + const result = RequestUtil.describeURL(''); + assert.strictEqual(result, '[]', 'Should return empty brackets for invalid URL'); + }); + }); +}); diff --git a/test/integration/testUtil.js b/test/integration/testUtil.js index dd836cc40..1f082d28c 100644 --- a/test/integration/testUtil.js +++ b/test/integration/testUtil.js @@ -13,7 +13,7 @@ const path = require('path'); const os = require('os'); module.exports.createConnection = function (validConnectionOptionsOverride = {}, coreInstance) { - coreInstance = coreInstance ? coreInstance : snowflake; + coreInstance = coreInstance || snowflake; return coreInstance.createConnection({ ...connOptions.valid, @@ -22,7 +22,7 @@ module.exports.createConnection = function (validConnectionOptionsOverride = {}, }; module.exports.createProxyConnection = function (validConnectionOptionsOverride, coreInstance) { - coreInstance = coreInstance ? coreInstance : snowflake; + coreInstance = coreInstance || snowflake; return coreInstance.createConnection({ ...connOptions.connectionWithProxy, @@ -31,7 +31,7 @@ module.exports.createProxyConnection = function (validConnectionOptionsOverride, }; module.exports.createConnectionPool = function (validConnectionOptionsOverride, coreInstance) { - coreInstance = coreInstance ? coreInstance : snowflake; + coreInstance = coreInstance || snowflake; return coreInstance.createPool({ ...connOptions.valid, diff --git a/test/unit/snowflake_test.js b/test/unit/snowflake_test.js index 0dc44ba64..c14896e4e 100644 --- a/test/unit/snowflake_test.js +++ b/test/unit/snowflake_test.js @@ -815,9 +815,10 @@ describe('connection.execute() statement failure', function () { assert.strictEqual(statement.getColumns(), undefined); assert.strictEqual(statement.getNumRows(), undefined); assert.strictEqual(statement.getSessionState(), undefined); - - assert.strictEqual(statement.getStatementId(), null); - assert.strictEqual(statement.getQueryId(), null); + assert.ok(Util.exists(statement.getStatementId())); + assert.ok(Util.isString(statement.getStatementId())); + assert.ok(Util.exists(statement.getQueryId())); + assert.ok(Util.isString(statement.getQueryId())); callback(); }