diff --git a/.github/workflows/pr_notify.yml b/.github/workflows/pr_notify.yml new file mode 100644 index 0000000000..2b34036d06 --- /dev/null +++ b/.github/workflows/pr_notify.yml @@ -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 diff --git a/app/data/allocations-dao.js b/app/data/allocations-dao.js index 24d4718c4a..8262e2e57a 100644 --- a/app/data/allocations-dao.js +++ b/app/data/allocations-dao.js @@ -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: @@ -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 diff --git a/app/data/user-dao.js b/app/data/user-dao.js index a674363efa..49102ab2e7 100644 --- a/app/data/user-dao.js +++ b/app/data/user-dao.js @@ -89,7 +89,7 @@ function UserDAO(db) { }; usersCol.findOne({ - userName: userName + userName: String(userName) // Ensure userName is treated as a string }, validateUserDoc); }; diff --git a/app/routes/contributions.js b/app/routes/contributions.js index 7f68170b94..5238c45458 100644 --- a/app/routes/contributions.js +++ b/app/routes/contributions.js @@ -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 diff --git a/app/routes/index.js b/app/routes/index.js index a9e55426bf..b4d16a8030 100644 --- a/app/routes/index.js +++ b/app/routes/index.js @@ -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) => { @@ -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); diff --git a/app/routes/profile.js b/app/routes/profile.js index 0b5b34f2dd..cd89718db7 100644 --- a/app/routes/profile.js +++ b/app/routes/profile.js @@ -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 diff --git a/app/routes/research.js b/app/routes/research.js index c3ae59df6e..5140ef6a37 100644 --- a/app/routes/research.js +++ b/app/routes/research.js @@ -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, { diff --git a/app/routes/session.js b/app/routes/session.js index 3810fb980a..61249891d3 100644 --- a/app/routes/session.js +++ b/app/routes/session.js @@ -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 diff --git a/server.js b/server.js index d6bb500a2d..6132b44243 100644 --- a/server.js +++ b/server.js @@ -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"); @@ -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()); @@ -110,7 +97,6 @@ MongoClient.connect(db, (err, db) => { res.locals.csrftoken = req.csrfToken(); next(); }); - */ // Register templating engine app.engine(".html", consolidate.swig);