Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .github/workflows/pr_notify.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
name: PR Notifier

on:
pull_request:
types: [opened, reopened, closed]

jobs:
notify:
runs-on: ubuntu-latest
steps:
- name: Notify Discord
env:
DISCORD_WEBHOOK_URL: ${{ secrets.DISCORD_WEBHOOK_URL }}
run: |
curl -H "Content-Type: application/json" -d '{"content": "πŸ”” Pull Request [${{ github.event.pull_request.title }}](${{ github.event.pull_request.html_url }}) by ${{ github.event.pull_request.user.login }} - ${{ github.event.action }}"}' $DISCORD_WEBHOOK_URL
- name: Notify Slack
env:
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}
run: |
curl -H "Content-Type: application/json" -d '{"text": ":bell: Pull Request <${{ github.event.pull_request.html_url }}|${{ github.event.pull_request.title }}> by ${{ github.event.pull_request.user.login }} - ${{ github.event.action }}"}' $SLACK_WEBHOOK_URL
6 changes: 1 addition & 5 deletions app/data/allocations-dao.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ const AllocationsDAO = function(db){
const searchCriteria = () => {

if (threshold) {
/*
// Fix for A1 - 2 NoSQL Injection - escape the threshold parameter properly
// Fix this NoSQL Injection which doesn't sanitze the input parameter 'threshold' and allows attackers
// to inject arbitrary javascript code into the NoSQL query:
Expand All @@ -73,10 +72,7 @@ const AllocationsDAO = function(db){
return {$where: `this.userId == ${parsedUserId} && this.stocks > ${parsedThreshold}`};
}
throw `The user supplied threshold: ${parsedThreshold} was not valid.`;
*/
return {
$where: `this.userId == ${parsedUserId} && this.stocks > '${threshold}'`
};

}
return {
userId: parsedUserId
Expand Down
2 changes: 1 addition & 1 deletion app/data/user-dao.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ function UserDAO(db) {
};

usersCol.findOne({
userName: userName
userName: String(userName) // Ensure userName is treated as a string
}, validateUserDoc);
};

Expand Down
6 changes: 3 additions & 3 deletions app/routes/contributions.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ function ContributionsHandler(db) {

/*jslint evil: true */
// Insecure use of eval() to parse inputs
const preTax = eval(req.body.preTax);
const afterTax = eval(req.body.afterTax);
const roth = eval(req.body.roth);
const preTax = parseInt(req.body.preTax);
const afterTax = parseInt(req.body.afterTax);
const roth = parseInt(req.body.roth);

/*
//Fix for A1 -1 SSJS Injection attacks - uses alternate method to eval
Expand Down
52 changes: 32 additions & 20 deletions app/routes/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const MemosHandler = require("./memos");
const ResearchHandler = require("./research");
const tutorialRouter = require("./tutorial");
const ErrorHandler = require("./error").errorHandler;
const rateLimit = require("express-rate-limit");

const index = (app, db) => {

Expand All @@ -26,54 +27,65 @@ const index = (app, db) => {
//Middleware to check if user has admin rights
const isAdmin = sessionHandler.isAdminUserMiddleware;

// Rate limiter middleware
const limiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100 // limit each IP to 100 requests per windowMs
});

// The main page of the app
app.get("/", sessionHandler.displayWelcomePage);
app.get("/", limiter, sessionHandler.displayWelcomePage);

// Login form
app.get("/login", sessionHandler.displayLoginPage);
app.post("/login", sessionHandler.handleLoginRequest);
app.get("/login", limiter, sessionHandler.displayLoginPage);
app.post("/login", limiter, sessionHandler.handleLoginRequest);

// Signup form
app.get("/signup", sessionHandler.displaySignupPage);
app.post("/signup", sessionHandler.handleSignup);
app.get("/signup", limiter, sessionHandler.displaySignupPage);
app.post("/signup", limiter, sessionHandler.handleSignup);

// Logout page
app.get("/logout", sessionHandler.displayLogoutPage);
app.get("/logout", limiter, sessionHandler.displayLogoutPage);

// The main page of the app
app.get("/dashboard", isLoggedIn, sessionHandler.displayWelcomePage);
app.get("/dashboard", limiter, isLoggedIn, sessionHandler.displayWelcomePage);

// Profile page
app.get("/profile", isLoggedIn, profileHandler.displayProfile);
app.post("/profile", isLoggedIn, profileHandler.handleProfileUpdate);
app.get("/profile", limiter, isLoggedIn, profileHandler.displayProfile);
app.post("/profile", limiter, isLoggedIn, profileHandler.handleProfileUpdate);

// Contributions Page
app.get("/contributions", isLoggedIn, contributionsHandler.displayContributions);
app.post("/contributions", isLoggedIn, contributionsHandler.handleContributionsUpdate);
app.get("/contributions", limiter, isLoggedIn, contributionsHandler.displayContributions);
app.post("/contributions", limiter, isLoggedIn, contributionsHandler.handleContributionsUpdate);

// Benefits Page
app.get("/benefits", isLoggedIn, benefitsHandler.displayBenefits);
app.post("/benefits", isLoggedIn, benefitsHandler.updateBenefits);
app.get("/benefits", limiter, isLoggedIn, benefitsHandler.displayBenefits);
app.post("/benefits", limiter, isLoggedIn, benefitsHandler.updateBenefits);
/* Fix for A7 - checks user role to implement Function Level Access Control
app.get("/benefits", isLoggedIn, isAdmin, benefitsHandler.displayBenefits);
app.post("/benefits", isLoggedIn, isAdmin, benefitsHandler.updateBenefits);
*/

// Allocations Page
app.get("/allocations/:userId", isLoggedIn, allocationsHandler.displayAllocations);
app.get("/allocations/:userId", limiter, isLoggedIn, allocationsHandler.displayAllocations);

// Memos Page
app.get("/memos", isLoggedIn, memosHandler.displayMemos);
app.post("/memos", isLoggedIn, memosHandler.addMemos);
app.get("/memos", limiter, isLoggedIn, memosHandler.displayMemos);
app.post("/memos", limiter, isLoggedIn, memosHandler.addMemos);

// Handle redirect for learning resources link
app.get("/learn", isLoggedIn, (req, res) => {
// Insecure way to handle redirects by taking redirect url from query string
return res.redirect(req.query.url);
app.get("/learn", limiter, isLoggedIn, (req, res) => {
const allowedUrls = ["https://trusted.com", "https://another-trusted.com"];
const redirectUrl = req.query.url;
if (allowedUrls.includes(redirectUrl)) {
return res.redirect(redirectUrl);
} else {
return res.status(400).send("Invalid redirect URL");
}
});

// Research Page
app.get("/research", isLoggedIn, researchHandler.displayResearch);
app.get("/research", limiter, isLoggedIn, researchHandler.displayResearch);

// Mount tutorial router
app.use("/tutorial", tutorialRouter);
Expand Down
2 changes: 1 addition & 1 deletion app/routes/profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ function ProfileHandler(db) {
// --
// The Fix: Instead of using greedy quantifiers the same regex will work if we omit the second quantifier +
// const regexPattern = /([0-9]+)\#/;
const regexPattern = /([0-9]+)+\#/;
const regexPattern = /([0-9]+)#/;
// Allow only numbers with a suffix of the letter #, for example: 'XXXXXX#'
const testComplyWithRequirements = regexPattern.test(bankRouting);
// if the regex test fails we do not allow saving
Expand Down
10 changes: 9 additions & 1 deletion app/routes/research.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,15 @@ function ResearchHandler(db) {
this.displayResearch = (req, res) => {

if (req.query.symbol) {
const url = req.query.url + req.query.symbol;
const allowedDomains = ["https://api.stockinfo.com", "https://api.finance.com"];
const baseUrl = req.query.url;
const isAllowed = allowedDomains.some(domain => baseUrl.startsWith(domain));

if (!isAllowed) {
return res.status(400).send("Invalid URL");
}

const url = baseUrl + req.query.symbol;
return needle.get(url, (error, newResponse, body) => {
if (!error && newResponse.statusCode === 200) {
res.writeHead(200, {
Expand Down
2 changes: 1 addition & 1 deletion app/routes/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ function SessionHandler(db) {
const USER_RE = /^.{1,20}$/;
const FNAME_RE = /^.{1,100}$/;
const LNAME_RE = /^.{1,100}$/;
const EMAIL_RE = /^[\S]+@[\S]+\.[\S]+$/;
const EMAIL_RE = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; // Updated regex to prevent ReDoS
const PASS_RE = /^.{1,20}$/;
/*
//Fix for A2-2 - Broken Authentication - requires stronger password
Expand Down
22 changes: 4 additions & 18 deletions server.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const express = require("express");
const favicon = require("serve-favicon");
const bodyParser = require("body-parser");
const session = require("express-session");
// const csrf = require('csurf');
const csrf = require('csurf');
const consolidate = require("consolidate"); // Templating library adapter for Express
const swig = require("swig");
// const helmet = require("helmet");
Expand Down Expand Up @@ -82,26 +82,13 @@ MongoClient.connect(db, (err, db) => {
secret: cookieSecret,
// Both mandatory in Express v4
saveUninitialized: true,
resave: true
/*
// Fix for A5 - Security MisConfig
// Use generic cookie name
key: "sessionId",
*/

/*
// Fix for A3 - XSS
// TODO: Add "maxAge"
resave: true,
cookie: {
httpOnly: true
// Remember to start an HTTPS server to get this working
// secure: true
httpOnly: true,
secure: true // Remember to start an HTTPS server to get this working
}
*/

}));

/*
// Fix for A8 - CSRF
// Enable Express csrf protection
app.use(csrf());
Expand All @@ -110,7 +97,6 @@ MongoClient.connect(db, (err, db) => {
res.locals.csrftoken = req.csrfToken();
next();
});
*/

// Register templating engine
app.engine(".html", consolidate.swig);
Expand Down
Loading