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
12 changes: 1 addition & 11 deletions app/data/allocations-dao.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,23 +60,13 @@ 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:
// 1. 0';while(true){}'
// 2. 1'; return 1 == '1
// Also implement fix in allocations.html for UX.
const parsedThreshold = parseInt(threshold, 10);

if (parsedThreshold >= 0 && parsedThreshold <= 99) {
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 Expand Up @@ -111,4 +101,4 @@ const AllocationsDAO = function(db){

};

module.exports.AllocationsDAO = AllocationsDAO;
module.exports.AllocationsDAO = AllocationsDAO;
9 changes: 8 additions & 1 deletion app/data/user-dao.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,15 @@ function UserDAO(db) {
}
};

// Validate and sanitize userName input
if (typeof userName !== 'string' || userName.trim() === '') {
const invalidUserNameError = new Error("Invalid userName");
invalidUserNameError.invalidUserName = true;
return callback(invalidUserNameError, null);
}

usersCol.findOne({
userName: userName
userName: userName.trim()
}, 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
20 changes: 16 additions & 4 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,16 +27,22 @@ const index = (app, db) => {
//Middleware to check if user has admin rights
const isAdmin = sessionHandler.isAdminUserMiddleware;

// Rate limiting 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);

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

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

// Logout page
app.get("/logout", sessionHandler.displayLogoutPage);
Expand Down Expand Up @@ -68,8 +75,13 @@ const index = (app, db) => {

// 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);
const allowedDomains = ['example.com', 'anotherexample.com'];
const url = new URL(req.query.url, `http://${req.headers.host}`);
if (allowedDomains.includes(url.hostname)) {
return res.redirect(req.query.url);
} else {
return res.status(400).send('Invalid redirect URL');
}
});

// Research Page
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://trustedsource.com', 'https://anothertrustedsource.com'];
const userUrl = req.query.url;
const isValidDomain = allowedDomains.some(domain => userUrl.startsWith(domain));

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

const url = userUrl + 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 avoid ReDoS
const PASS_RE = /^.{1,20}$/;
/*
//Fix for A2-2 - Broken Authentication - requires stronger password
Expand Down
20 changes: 3 additions & 17 deletions server.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,35 +82,21 @@ 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 // Ensure cookies are sent only over HTTPS
}
*/

}));

/*
// Fix for A8 - CSRF
// Enable Express csrf protection
app.use(csrf());
// Make csrf token available in templates
app.use((req, res, next) => {
res.locals.csrftoken = req.csrfToken();
next();
});
*/

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