Add vulnerable-by-design account statement export feature - #8
Bonckheere1 wants to merge 1 commit into
Conversation
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.
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
|
|
||
| console.log(`[statement-export] Executing: ${command}`); | ||
|
|
||
| exec(command, (err) => { |
There was a problem hiding this comment.
Potential for OS command injection via child_process call - critical severity
It is generally not recommended to call out to the operating system to execute commands. When the application is executing file-system-based commands, user input should never be used in constructing commands or command arguments.
Show fix
Remediation: If you can, avoid the exec() call. If that is not possible, consider hard coding both the command and arguments to be used. Alternatively, install Aikido Runtime for NodeJS to prevent command injection completely.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| return res.render("statement", { | ||
| userId, | ||
| statements, | ||
| searchTerm: search || "", | ||
| environmentalScripts | ||
| }); |
There was a problem hiding this comment.
Server-Side Template Injection via untrusted input in express.render() - high severity
Direct use of user-controlled inputs as arguments to the express.render() function can result in server-side template injection when the template engine evaluates untrusted data. An attacker may craft malicious payloads to read local files or, depending on the template engine and its configuration, escalate the issue to remote code execution by abusing template logic and expression handling.
Show fix
Remediation: Validate and sanitize all inputs before rendering, never pass raw user objects to templates, restrict template capabilities, and enforce strict variable whitelisting or safe rendering modes.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| const { | ||
| fileName | ||
| } = req.params; | ||
| const filePath = path.join(EXPORT_DIR, fileName); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - high severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| } = req.params; | ||
| const filePath = path.join(EXPORT_DIR, fileName); | ||
|
|
||
| fs.readFile(filePath, "utf8", (err, data) => { |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - high severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Summary by Aikido
🚀 New Features
📚 Documentation
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 12 critical 6 high |
🟢 Metrics 37 complexity · 0 duplication
Metric Results Complexity 37 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
| const { | ||
| fileName | ||
| } = req.params; | ||
| const filePath = path.join(EXPORT_DIR, fileName); |
There was a problem hiding this comment.
🟡 Medium - Statement files saved with .txt cannot be downloaded or removed through the new routes
The exporter writes every generated statement to ${fileName}.txt, but both the download and delete handlers reconstruct the path from the bare fileName route parameter instead of the on-disk filename. As a result, the "Download" button errors for every statement created by this feature, and "Delete" only removes the MongoDB row while silently leaving the actual file behind. This breaks the feature's core contract and will accumulate orphaned files on disk over time.
Show fix
Use one canonical filename format across create/read/delete operations. Either store the exact generated path or filename (including extension) in MongoDB and reuse it for download/delete, or append the same .txt suffix in both handlers before accessing the filesystem.
More info - Reply on this comment to give feedback on the issue.
2 participants
Summary
Adds a new Statements feature to NodeGoat: a nav-bar-linked page where a logged-in user can generate, search, download, and delete exported account statements. It's a real, working feature (not a diff-only snippet) whose vulnerable sinks live a few function calls away from the route handlers, so exercising them requires following the call chain:
app/routes/statement.js(controller) →app/data/statement-dao.js(Mongo query) /app/utils/statement-export.js(shell export) → filesystem / MongoDBVulnerabilities introduced
app/data/statement-dao.js):getAllForUserbuilds a MongoDB$whereclause via string concatenation of thesearchquery param.app/utils/statement-export.js):exportStatementToFileshells out viachild_process.exec()with the user-suppliedfileName/notesconcatenated straight into the command string.app/routes/statement.js):downloadStatement/deleteStatementjoin:fileNamefrom the URL onto the export directory with no sanitization.app/routes/statement.js): every handler trusts:userIdfrom the URL instead ofreq.session.userId, with no ownership check — any logged-in user can view, export against, download, or delete another user's statements.deleteStatementis a state-changing action exposed via plainGETwith no CSRF token.Each sink has an inline comment with an attack example and a commented-out fix, matching the existing NodeGoat teaching style (see
reports-dao.js/allocations-dao.jsfor precedent).Verification
node --checkpassed on all new/modified files.$whereclause is built with unsanitizedsearchinput.notes("; touch /tmp/CMD_INJECTION_PROOF; echo ") actually executed a shell command outside the intendedecho.../fileNameresolves outsideEXPORT_DIR.Test plan
npm install && npm run docker-mongo(ordocker-compose up), thennpm start"; touch /tmp/pwned; echo "as the export request, confirm the command executes server-sideuserIdin the/statement/:userIdURL to another account's id and confirm you can view/export/delete their statementssearch=') || ('1'=='1on the statement search box and confirm it returns statements belonging to other usersGenerated by Claude Code