From 698a94ff6517c896761010eed35922aeaffbb0a2 Mon Sep 17 00:00:00 2001 From: csmig <33138761+csmig@users.noreply.github.com> Date: Tue, 14 Nov 2023 16:13:03 -0500 Subject: [PATCH] fix: escape special characters in filenames (#1147) * fix: escape special characters in zip filenames * Applied filename substitution to individual asset exports, tweaked client regex when saving filename to prevent it from cutting off the suggested filename after a semicolon (semicolon is used in the filename character substitutions) * tests for filename reserved chars substitution * removed label property from test asset creation --------- Co-authored-by: csmig Co-authored-by: cd-rite <61710958+cd-rite@users.noreply.github.com> --- api/source/controllers/Asset.js | 17 +- api/source/controllers/Collection.js | 6 +- api/source/utils/escape.js | 72 +++-- client/src/js/SM/Exports.js | 2 +- client/src/js/review.js | 6 +- test/api/postman_collection.json | 425 +++++++++++++++++++++++++++ 6 files changed, 499 insertions(+), 29 deletions(-) diff --git a/api/source/controllers/Asset.js b/api/source/controllers/Asset.js index e9285a8f6..7b6513e2e 100644 --- a/api/source/controllers/Asset.js +++ b/api/source/controllers/Asset.js @@ -2,6 +2,7 @@ const writer = require('../utils/writer'); const config = require('../utils/config') +const escape = require('../utils/escape') const AssetService = require(`../service/${config.database.type}/AssetService`); const CollectionService = require(`../service/${config.database.type}/CollectionService`); const dbUtils = require(`../service/${config.database.type}/utils`) @@ -9,6 +10,7 @@ const {XMLBuilder} = require("fast-xml-parser") const SmError = require('../utils/error') const {escapeForXml} = require('../utils/escape') + module.exports.createAsset = async function createAsset (req, res, next) { try { const elevate = req.query.elevate @@ -284,7 +286,8 @@ module.exports.getChecklistByAssetStig = async function getChecklistByAssetStig } else if (format === 'cklb') { response.cklb.title = `${response.assetName}-${benchmarkId}-${response.revisionStrResolved}` - writer.writeInlineFile(res, JSON.stringify(response.cklb), `${response.assetName}-${benchmarkId}-${response.revisionStrResolved}.cklb`, 'application/json') // revisionStrResolved provides specific rev string, if "latest" was asked for. + let filename = escape.escapeFilename(`${response.assetName}-${benchmarkId}-${response.revisionStrResolved}.cklb`) + writer.writeInlineFile(res, JSON.stringify(response.cklb), filename, 'application/json') // revisionStrResolved provides specific rev string, if "latest" was asked for. } else if (format === 'ckl') { const builder = new XMLBuilder({ @@ -300,7 +303,8 @@ module.exports.getChecklistByAssetStig = async function getChecklistByAssetStig }) let xml = `\n\n\n` xml += builder.build(response.xmlJs) - writer.writeInlineFile(res, xml, `${response.assetName}-${benchmarkId}-${response.revisionStrResolved}.ckl`, 'application/xml') // revisionStrResolved provides specific rev string, if "latest" was asked for. + let filename = escape.escapeFilename(`${response.assetName}-${benchmarkId}-${response.revisionStrResolved}.ckl`) + writer.writeInlineFile(res, xml, filename, 'application/xml') // revisionStrResolved provides specific rev string, if "latest" was asked for. } else if (format === 'xccdf') { const builder = new XMLBuilder({ @@ -318,7 +322,8 @@ module.exports.getChecklistByAssetStig = async function getChecklistByAssetStig }) let xml = `\n\n\n` xml += builder.build(response.xmlJs) - writer.writeInlineFile(res, xml, `${response.assetName}-${benchmarkId}-${response.revisionStrResolved}-xccdf.xml`, 'application/xml') // revisionStrResolved provides specific rev string, if "latest" was asked for. + let filename = escape.escapeFilename(`${response.assetName}-${benchmarkId}-${response.revisionStrResolved}-xccdf.xml`) + writer.writeInlineFile(res, xml, filename, 'application/xml') // revisionStrResolved provides specific rev string, if "latest" was asked for. } } else { @@ -358,7 +363,8 @@ module.exports.getChecklistByAsset = async function getChecklistByAssetStig (req const response = await AssetService.getChecklistByAsset(assetId, stigs, format, false, req.userObject ) if (format === 'cklb') { - writer.writeInlineFile(res, JSON.stringify(response.cklb), `${response.assetName}.cklb`, 'application/json') + let filename = escape.escapeFilename(`${response.assetName}.cklb`) + writer.writeInlineFile(res, JSON.stringify(response.cklb), filename, 'application/json') } else if (format === 'ckl') { const builder = new XMLBuilder({ @@ -374,7 +380,8 @@ module.exports.getChecklistByAsset = async function getChecklistByAssetStig (req }) let xml = `\n\n` xml += builder.build(response.xmlJs) - writer.writeInlineFile(res, xml, `${response.assetName}.ckl`, 'application/xml') + let filename = escape.escapeFilename(`${response.assetName}.ckl`) + writer.writeInlineFile(res, xml, filename, 'application/xml') } } catch (err) { diff --git a/api/source/controllers/Collection.js b/api/source/controllers/Collection.js index cf9b6d21c..6d6ad4880 100644 --- a/api/source/controllers/Collection.js +++ b/api/source/controllers/Collection.js @@ -2,6 +2,7 @@ const writer = require('../utils/writer') const config = require('../utils/config') +const escape = require('../utils/escape') const CollectionService = require(`../service/${config.database.type}/CollectionService`) const AssetService = require(`../service/${config.database.type}/AssetService`) const STIGService = require(`../service/${config.database.type}/STIGService`) @@ -708,7 +709,9 @@ async function postArchiveByCollection ({format = 'ckl-mono', req, res, parsedRe attrValueProcessor: escapeForXml }) const zip = Archiver('zip', {zlib: {level: 9}}) - res.attachment(`${parsedRequest.collection.name}-${format.startsWith('ckl-') ? 'CKL' : format.startsWith('cklb-') ? 'CKLB' : 'XCCDF'}.zip`) + const attachmentName = escape.escapeFilename(`${parsedRequest.collection.name}-${format.startsWith('ckl-') ? + 'CKL' : format.startsWith('cklb-') ? 'CKLB' : 'XCCDF'}.zip`) + res.attachment(attachmentName) zip.pipe(res) const manifest = { started: new Date().toISOString(), @@ -755,6 +758,7 @@ async function postArchiveByCollection ({format = 'ckl-mono', req, res, parsedRe filename += `-${arg.stigs[0].benchmarkId}-${response.revisionStrResolved}` } filename += `${format === 'xccdf' ? '-xccdf.xml' : format.startsWith('ckl-') ? '.ckl' : '.cklb'}` + filename = escape.escapeFilename(filename) zip.append(data, {name: filename}) manifest.members.push(filename) manifest.memberCount += 1 diff --git a/api/source/utils/escape.js b/api/source/utils/escape.js index 7bb7a0d0c..6a6a5265b 100644 --- a/api/source/utils/escape.js +++ b/api/source/utils/escape.js @@ -1,31 +1,65 @@ /** - * Regex matches characters that need to be escaped in XML. - * @type {RegExp} + * Escapes XML reserved characters with named entity references. + * @param {string} value - The string to escape. + * @returns {string} The escaped string. */ -const regexEscapeXml = /["&'<>]/g +module.exports.escapeForXml = function (name, value) { + /** + * Regex matches characters that need to be escaped in XML. + * @type {RegExp} + */ + const regexEscapeXml = /["&'<>]/g -/** - * Map of characters to their corresponding named XML entities. - * @type {Object.} - */ -const escapeMapXml = { - '"': '"', - '&': '&', - '\'': ''', - '<': '<', - '>': '>' -}; + /** + * Map of characters to their corresponding named XML entities. + * @type {Object.} + */ + const escapeMapXml = { + '"': '"', + '&': '&', + '\'': ''', + '<': '<', + '>': '>' + } + return value.toString().replace(regexEscapeXml, function ($0) { + return escapeMapXml[$0] + }) +} /** - * Escapes XML reserved characters with named entity references. + * Escapes filesystem reserved characters with named entity references. * @param {string} value - The string to escape. * @returns {string} The escaped string. */ -module.exports.escapeForXml = function(name, value) { - return value.toString().replace(regexEscapeXml, function($0) { - return escapeMapXml[$0] - }); +module.exports.escapeFilename = function (value) { + /** + * Regexes match characters that need to be escaped in filenames. + * @type {RegExp} + */ + const osReserved = /[\/\\:\*"\?<>\|]/g + const controlChars = /[\x00-\x1f]/g + + /** + * Map of characters to their corresponding named HTML entities. + * @type {Object.} + */ + const osReserveReplace = { + '/': '/', + '\\': '\', + ':': ':', + '*': '*', + '"': '"', + '?': '?', + '<': '<', + '>': '>', + '|': '|', + } + + return value.toString() + .replace(osReserved, (match) => osReserveReplace[match]) + .replace(controlChars, (match) => `&#x${match.charCodeAt(0).toString().padStart(2,'0')};`) + .substring(0, 255) } diff --git a/client/src/js/SM/Exports.js b/client/src/js/SM/Exports.js index f0957012e..4f85a5266 100644 --- a/client/src/js/SM/Exports.js +++ b/client/src/js/SM/Exports.js @@ -750,7 +750,7 @@ SM.Exports.exportArchiveStreaming = async function ({collectionId, checklists, f } initProgress("Downloading checklists", "Initializing...") updateStatusText(`When the stream has finished you will be prompted to save the data to disk. The final size of the archive is unknown during streaming.`, true) - const filename = contentDisposition.match(/filename\*?=['"]?(?:UTF-\d['"]*)?([^;\r\n"']*)['"]?;?/)[1] + const filename = contentDisposition.match(/filename\*?=['"]?(?:UTF-\d['"]*)?([^\r\n"']*)['"]?;?/)[1] const reader = response.body.getReader() let receivedLength = 0; // received that many bytes at the moment let chunks = []; // array of received binary chunks (comprises the body) diff --git a/client/src/js/review.js b/client/src/js/review.js index bb655c02b..886b93f7f 100644 --- a/client/src/js/review.js +++ b/client/src/js/review.js @@ -219,7 +219,7 @@ async function addReview( params ) { xhr.onload = function () { if (this.status >= 200 && this.status < 300) { var contentDispo = this.getResponseHeader('Content-Disposition') - var fileName = contentDispo.match(/filename\*?=['"]?(?:UTF-\d['"]*)?([^;\r\n"']*)['"]?;?/)[1] + var fileName = contentDispo.match(/filename\*?=['"]?(?:UTF-\d['"]*)?([^\r\n"']*)['"]?;?/)[1] resolve({ blob: xhr.response, filename: fileName @@ -267,7 +267,7 @@ async function addReview( params ) { xhr.onload = function () { if (this.status >= 200 && this.status < 300) { var contentDispo = this.getResponseHeader('Content-Disposition') - var fileName = contentDispo.match(/filename\*?=['"]?(?:UTF-\d['"]*)?([^;\r\n"']*)['"]?;?/)[1] + var fileName = contentDispo.match(/filename\*?=['"]?(?:UTF-\d['"]*)?([^\r\n"']*)['"]?;?/)[1] resolve({ blob: xhr.response, filename: fileName @@ -314,7 +314,7 @@ async function addReview( params ) { }) const contentDispo = response.headers.get("content-disposition") if (contentDispo) { - const filename = contentDispo.match(/filename\*?=['"]?(?:UTF-\d['"]*)?([^;\r\n"']*)['"]?;?/)[1] + const filename = contentDispo.match(/filename\*?=['"]?(?:UTF-\d['"]*)?([^\r\n"']*)['"]?;?/)[1] console.log(filename) const blob = await response.blob() saveAs(blob, filename) diff --git a/test/api/postman_collection.json b/test/api/postman_collection.json index 77d7baa81..6d49b430e 100644 --- a/test/api/postman_collection.json +++ b/test/api/postman_collection.json @@ -62600,6 +62600,431 @@ "response": [] } ] + }, + { + "name": "valid filename from Asset with reserved chars", + "item": [ + { + "name": "Create an Asset in collection to be deleted Copy", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "let user = pm.environment.get(\"user\");\r", + "console.log(\"user: \" + user);\r", + "\r", + "if (pm.request.url.getQueryString().match(/elevate=true/)) {\r", + " user = \"elevated\";\r", + " console.log(\"setting user to 'elevated'\");\r", + "}\r", + "\r", + "// if (user == \"lvl1\" || user == \"lvl2\" || user == \"collectioncreator\" || user == \"globular\") { //placeholder for \"users\" that should fail\r", + "// pm.test(\"Status should be is 403 for all users except stigmanAdmin(elevated), lvl3 and lvl4\", function () {\r", + "// pm.response.to.have.status(403);\r", + "// });\r", + "// return;\r", + "// }\r", + "// else {\r", + " pm.test(\"Status code is 201\", function () {\r", + " pm.response.to.have.status(201);\r", + " });\r", + "// }\r", + "if (pm.response.code !== 201) {\r", + " return;\r", + "}\r", + "\r", + "let respJson = pm.response.json();\r", + "let assetInCollectionWithReservedChars = respJson.assetId\r", + "pm.environment.set(\"assetInCollectionWithReservedChars\", assetInCollectionWithReservedChars);\r", + "\r", + "console.log(\"created asset with id: \" + JSON.stringify(assetInCollectionWithReservedChars));\r", + "\r", + "\r", + "// pm.test(\"Response matches request\", function () {\r", + "// pm.expect(assetGetToPost(respJson))\r", + "// .to.eql(JSON.parse(pm.request.body.raw))\r", + "// })\r", + "\r", + "// function assetGetToPost(assetGet) {\r", + "// // extract the transformed and unposted properties\r", + "// const {assetId, collection, stigs, mac, fqdn, ...assetPost} = assetGet\r", + " \r", + "// // add transformed properties to the derived post \r", + "// assetPost.collectionId = collection.collectionId\r", + "// assetPost.stigs = stigsGetToPost(stigs)\r", + "\r", + "// // the derived post object\r", + "// return assetPost\r", + "// }\r", + "\r", + "// function stigsGetToPost(stigsGetArray) {\r", + "// const stigsPostArray = []\r", + "// for (const stig of stigsGetArray) {\r", + "// stigsPostArray.push(stig.benchmarkId)\r", + "// }\r", + "// return stigsPostArray\r", + "// }\r", + "\r", + "" + ], + "type": "text/javascript" + } + }, + { + "listen": "prerequest", + "script": { + "exec": [ + "" + ], + "type": "text/javascript" + } + } + ], + "request": { + "auth": { + "type": "bearer", + "bearer": [ + { + "key": "token", + "value": "{{token.stigmanadmin}}", + "type": "string" + } + ] + }, + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\n \"name\": \"TxxxxxEST_\\\\slash:colon..x2\",\n \"collectionId\": \"{{scrapCollection}}\",\n \"description\": \"test desc\",\n \"ip\": \"1.1.1.1\",\n \"noncomputing\": true,\n \"metadata\": {\n \"pocName\": \"poc2Put\",\n \"pocEmail\": \"pocEmailPut@email.com\",\n \"pocPhone\": \"12342\",\n \"reqRar\": \"true\"\n },\n \"stigs\": [\n \"{{testBenchmark}}\",\n \"Windows_10_STIG_TEST\"\n ]\n}", + "options": { + "raw": { + "language": "json" + } + } + }, + "url": { + "raw": "{{baseUrl}}/assets?projection=stigs", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "assets" + ], + "query": [ + { + "key": "elevate", + "value": "{{elevated}}", + "description": "Elevate the user context for this request if user is permitted (canAdmin)", + "disabled": true + }, + { + "key": "projection", + "value": "stigs", + "description": "Additional properties to include in the response.\n" + } + ] + } + }, + "response": [] + }, + { + "name": "Return the ckl for Asset with reserved chars", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "let user = pm.environment.get(\"user\");\r", + "console.log(\"user: \" + user);\r", + "\r", + "\r", + "\r", + "if (user == \"bizarroLvl1\" ) {\r", + " pm.test(\"Status should be is 403 for user collectioncreator, bizarroLvl1\", function () {\r", + " pm.response.to.have.status(403);\r", + " });\r", + " return;\r", + "}\r", + "else if (user == \"collectioncreator\" ) {\r", + " pm.test(\"Status should be is 204 for user collectioncreator\", function () {\r", + " pm.response.to.have.status(204);\r", + " });\r", + " return;\r", + "}\r", + "\r", + "\r", + "pm.test(\"Status code is 200\", function () {\r", + " pm.response.to.have.status(200);\r", + "});\r", + "\r", + "if (pm.response.code !== 200) {\r", + " return;\r", + "}\r", + "\r", + "\r", + "pm.test(\"Content-Disposition is set with expected filename\", function () {\r", + " pm.response.to.have.header(\"Content-Disposition\", \"inline; filename=\\\"TxxxxxEST_\slash:colon..x2-VPN_SRG_TEST-V1R1.ckl\\\"\");\r", + "});\r", + "" + ], + "type": "text/javascript" + } + } + ], + "request": { + "auth": { + "type": "bearer", + "bearer": [ + { + "key": "token", + "value": "{{token.stigmanadmin}}", + "type": "string" + } + ] + }, + "method": "GET", + "header": [], + "url": { + "raw": "{{baseUrl}}/assets/:assetId/checklists/:benchmarkId/:revisionStr?format=ckl", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "assets", + ":assetId", + "checklists", + ":benchmarkId", + ":revisionStr" + ], + "query": [ + { + "key": "format", + "value": "ckl", + "description": "The format of the response. Default if missing is 'json'" + } + ], + "variable": [ + { + "key": "assetId", + "value": "{{assetInCollectionWithReservedChars}}", + "description": "(Required) A path parameter that indentifies an Asset" + }, + { + "key": "benchmarkId", + "value": "{{testBenchmark}}", + "description": "(Required) A path parameter that indentifies a STIG" + }, + { + "key": "revisionStr", + "value": "{{testRev}}", + "description": "(Required) A path parameter that indentifies a STIG revision [ V{version_num}R{release_num} | 'latest' ]" + } + ] + } + }, + "response": [] + }, + { + "name": "Return the cklB for Asset with reserved chars", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "let user = pm.environment.get(\"user\");\r", + "console.log(\"user: \" + user);\r", + "\r", + "\r", + "if (user == \"bizarroLvl1\" ) {\r", + " pm.test(\"Status should be is 403 for user collectioncreator, bizarroLvl1\", function () {\r", + " pm.response.to.have.status(403);\r", + " });\r", + " return;\r", + "}\r", + "else if (user == \"collectioncreator\" ) {\r", + " pm.test(\"Status should be is 204 for user collectioncreator\", function () {\r", + " pm.response.to.have.status(204);\r", + " });\r", + " return;\r", + "}\r", + "\r", + "\r", + "pm.test(\"Status code is 200\", function () {\r", + " pm.response.to.have.status(200);\r", + "});\r", + "\r", + "if (pm.response.code !== 200) {\r", + " return;\r", + "}\r", + "\r", + "\r", + "pm.test(\"Content-Disposition is set with expected filename\", function () {\r", + " pm.response.to.have.header(\"Content-Disposition\", \"inline; filename=\\\"TxxxxxEST_\slash:colon..x2-VPN_SRG_TEST-V1R1.cklb\\\"\");\r", + "});\r", + "\r", + "" + ], + "type": "text/javascript" + } + } + ], + "request": { + "auth": { + "type": "bearer", + "bearer": [ + { + "key": "token", + "value": "{{token.stigmanadmin}}", + "type": "string" + } + ] + }, + "method": "GET", + "header": [], + "url": { + "raw": "{{baseUrl}}/assets/:assetId/checklists/:benchmarkId/:revisionStr?format=cklb", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "assets", + ":assetId", + "checklists", + ":benchmarkId", + ":revisionStr" + ], + "query": [ + { + "key": "format", + "value": "cklb", + "description": "The format of the response. Default if missing is 'json'" + } + ], + "variable": [ + { + "key": "assetId", + "value": "{{assetInCollectionWithReservedChars}}", + "description": "(Required) A path parameter that indentifies an Asset" + }, + { + "key": "benchmarkId", + "value": "{{testBenchmark}}", + "description": "(Required) A path parameter that indentifies a STIG" + }, + { + "key": "revisionStr", + "value": "{{testRev}}", + "description": "(Required) A path parameter that indentifies a STIG revision [ V{version_num}R{release_num} | 'latest' ]" + } + ] + } + }, + "response": [] + }, + { + "name": "Return the cklB for Asset with reserved chars Copy", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "let user = pm.environment.get(\"user\");\r", + "console.log(\"user: \" + user);\r", + "\r", + "\r", + "if (user == \"bizarroLvl1\" ) {\r", + " pm.test(\"Status should be is 403 for user collectioncreator, bizarroLvl1\", function () {\r", + " pm.response.to.have.status(403);\r", + " });\r", + " return;\r", + "}\r", + "else if (user == \"collectioncreator\" ) {\r", + " pm.test(\"Status should be is 204 for user collectioncreator\", function () {\r", + " pm.response.to.have.status(204);\r", + " });\r", + " return;\r", + "}\r", + "\r", + "\r", + "pm.test(\"Status code is 200\", function () {\r", + " pm.response.to.have.status(200);\r", + "});\r", + "\r", + "if (pm.response.code !== 200) {\r", + " return;\r", + "}\r", + "\r", + "\r", + "pm.test(\"Content-Disposition is set with expected filename\", function () {\r", + " pm.response.to.have.header(\"Content-Disposition\", \"inline; filename=\\\"TxxxxxEST_\slash:colon..x2-VPN_SRG_TEST-V1R1-xccdf.xml\\\"\");\r", + "});\r", + "\r", + "" + ], + "type": "text/javascript" + } + } + ], + "request": { + "auth": { + "type": "bearer", + "bearer": [ + { + "key": "token", + "value": "{{token.stigmanadmin}}", + "type": "string" + } + ] + }, + "method": "GET", + "header": [], + "url": { + "raw": "{{baseUrl}}/assets/:assetId/checklists/:benchmarkId/:revisionStr?format=xccdf", + "host": [ + "{{baseUrl}}" + ], + "path": [ + "assets", + ":assetId", + "checklists", + ":benchmarkId", + ":revisionStr" + ], + "query": [ + { + "key": "format", + "value": "xccdf", + "description": "The format of the response. Default if missing is 'json'" + } + ], + "variable": [ + { + "key": "assetId", + "value": "{{assetInCollectionWithReservedChars}}", + "description": "(Required) A path parameter that indentifies an Asset" + }, + { + "key": "benchmarkId", + "value": "{{testBenchmark}}", + "description": "(Required) A path parameter that indentifies a STIG" + }, + { + "key": "revisionStr", + "value": "{{testRev}}", + "description": "(Required) A path parameter that indentifies a STIG revision [ V{version_num}R{release_num} | 'latest' ]" + } + ] + } + }, + "response": [] + } + ] } ], "description": "These tests should be self contained, provide their own authorization, and repopulate test data if required.",