Skip to content

Commit 54184a2

Browse files
devin-ai-integration[bot]stephencornwell
devin-ai-integration[bot]
and
stephencornwell
committed
Remediate Snyk SCA and Code findings
Upgrade vulnerable dependencies, hash passwords, add CSRF/rate limiting, remove hardcoded secrets, fix NoSQL injection, open redirect, command injection, zip-slip and prototype pollution paths. Co-Authored-By: Stephen Cornwell <stephen@cognition.ai>
1 parent d240896 commit 54184a2

16 files changed

Lines changed: 332 additions & 278 deletions

‎Dockerfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# FROM node:6-stretch
2-
FROM node:18.13.0
2+
FROM node:20
33

44
RUN mkdir /usr/src/goof
55
RUN mkdir /tmp/extracted_files

‎README.md‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,18 @@ mongod &
1616

1717
git clone https://github.com/snyk-labs/nodejs-goof
1818
npm install
19-
npm start
19+
ADMIN_USERNAME=admin@snyk.io ADMIN_PASSWORD=<choose-a-password> npm start
2020
```
2121
This will run Goof locally, using a local mongo on the default port and listening on port 3001 (http://localhost:3001)
2222

23-
Note: You *have* to use an old version of MongoDB version due to some of these old libraries' database server APIs. MongoDB 3 is known to work ok.
23+
`ADMIN_USERNAME` is required to seed the admin account; if `ADMIN_PASSWORD` is unset a random one is generated and printed at startup. Set `SESSION_SECRET` for a stable session-signing key, and `TLS_KEY`/`TLS_CERT` to serve over HTTPS. MySQL credentials are read from `MYSQL_HOST`, `MYSQL_PORT`, `MYSQL_USER`, `MYSQL_PASSWORD` and `MYSQL_DATABASE`.
24+
25+
Note: MongoDB 4.0 or newer is required by the current mongoose version.
2426

2527
You can also run the MongoDB server individually via Docker, such as:
2628

2729
```sh
28-
docker run --rm -p 27017:27017 mongo:3
30+
docker run --rm -p 27017:27017 mongo:6
2931
```
3032

3133
## Running with docker-compose
@@ -85,7 +87,7 @@ The form is completely functional. The way it works is, it receives the profile
8587
You'd think that what's the worst that can happen because we use a validation to confirm the expected input, however the validation doesn't take into account a new field that can be added to the object, such as `layout`, which when passed to a template language, could lead to Local File Inclusion (Path Traversal) vulnerabilities. Here is a proof-of-concept showing it:
8688

8789
```sh
88-
curl -X 'POST' --cookie c.txt --cookie-jar c.txt -H 'Content-Type: application/json' --data-binary '{"username": "admin@snyk.io", "password": "SuperSecretPassword"}' 'http://localhost:3001/login'
90+
curl -X 'POST' --cookie c.txt --cookie-jar c.txt -H 'Content-Type: application/json' --data-binary '{"username": "admin@snyk.io", "password": "$ADMIN_PASSWORD"}' 'http://localhost:3001/login'
8991
```
9092

9193
```sh
@@ -118,7 +120,7 @@ echo '{"username":"admin@snyk.io", "password":"WrongPassword"}' | http --json $G
118120

119121
And another request, as denoted with the following JSON request to sign-in as the admin user works as expected:
120122
```sh
121-
echo '{"username":"admin@snyk.io", "password":"SuperSecretPassword"}' | http --json $GOOF_HOST/login -v
123+
echo '{"username":"admin@snyk.io", "password":"$ADMIN_PASSWORD"}' | http --json $GOOF_HOST/login -v
122124
```
123125

124126
However, what if the password wasn't a string? what if it was an object? Why would an object be harmful or even considered an issue?

‎app.js‎

Lines changed: 52 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -6,53 +6,74 @@
66
require('./mongoose-db');
77
require('./typeorm-db')
88

9-
var st = require('st');
109
var crypto = require('crypto');
1110
var express = require('express');
1211
var http = require('http');
12+
var https = require('https');
13+
var fs = require('fs');
1314
var path = require('path');
14-
var ejsEngine = require('ejs-locals');
15-
var bodyParser = require('body-parser');
15+
var expressLayouts = require('express-ejs-layouts');
1616
var session = require('express-session')
1717
var methodOverride = require('method-override');
1818
var logger = require('morgan');
1919
var errorHandler = require('errorhandler');
20-
var optional = require('optional');
21-
var marked = require('marked');
20+
var lusca = require('lusca');
21+
var { marked } = require('marked');
22+
var sanitizeHtml = require('sanitize-html');
2223
var fileUpload = require('express-fileupload');
2324
var dust = require('dustjs-linkedin');
24-
var dustHelpers = require('dustjs-helpers');
25-
var cons = require('consolidate');
2625
const hbs = require('hbs')
2726

2827
var app = express();
2928
var routes = require('./routes');
3029
var routesUsers = require('./routes/users.js')
3130

31+
var isProduction = app.get('env') === 'production';
32+
var sessionSecret = process.env.SESSION_SECRET || crypto.randomBytes(32).toString('hex');
33+
34+
function dustEngine(filePath, options, callback) {
35+
fs.readFile(filePath, 'utf8', function (err, source) {
36+
if (err) return callback(err);
37+
dust.renderSource(source, options, callback);
38+
});
39+
}
40+
3241
// all environments
42+
app.disable('x-powered-by');
43+
app.set('trust proxy', 1);
3344
app.set('port', process.env.PORT || 3001);
34-
app.engine('ejs', ejsEngine);
35-
app.engine('dust', cons.dust);
45+
app.engine('ejs', require('ejs').__express);
46+
app.engine('dust', dustEngine);
3647
app.engine('hbs', hbs.__express);
37-
cons.dust.helpers = dustHelpers;
3848
app.set('views', path.join(__dirname, 'views'));
3949
app.set('view engine', 'ejs');
50+
app.set('layout', 'layout');
51+
app.use(expressLayouts);
4052
app.use(logger('dev'));
4153
app.use(methodOverride());
4254
app.use(session({
43-
secret: 'keyboard cat',
55+
secret: sessionSecret,
4456
name: 'connect.sid',
45-
cookie: { path: '/' }
57+
resave: false,
58+
saveUninitialized: false,
59+
cookie: { path: '/', httpOnly: true, sameSite: 'lax', secure: isProduction }
4660
}))
47-
app.use(bodyParser.json());
48-
app.use(bodyParser.urlencoded({ extended: false }));
49-
app.use(fileUpload());
61+
app.use(express.json());
62+
app.use(express.urlencoded({ extended: false }));
63+
app.use(fileUpload({ limits: { fileSize: 5 * 1024 * 1024 } }));
64+
65+
// JSON APIs (authenticated via request body, not cookies)
66+
app.get('/chat', routes.chat.get);
67+
app.put('/chat', routes.chat.add);
68+
app.delete('/chat', routes.chat.delete);
69+
app.use('/users', routesUsers)
5070

5171
// Routes
72+
app.use(lusca.csrf());
5273
app.use(routes.current_user);
5374
app.get('/', routes.index);
5475
app.get('/login', routes.login);
55-
app.post('/login', routes.loginHandler);
76+
app.post('/login', routes.loginLimiter, routes.loginHandler);
5677
app.get('/admin', routes.isLoggedIn, routes.admin);
5778
app.get('/account_details', routes.isLoggedIn, routes.get_account_details);
5879
app.post('/account_details', routes.isLoggedIn, routes.save_account_details);
@@ -61,28 +82,32 @@ app.post('/create', routes.create);
6182
app.get('/destroy/:id', routes.destroy);
6283
app.get('/edit/:id', routes.edit);
6384
app.post('/update/:id', routes.update);
64-
app.post('/import', routes.import);
85+
app.post('/import', routes.importLimiter, routes.import);
6586
app.get('/about_new', routes.about_new);
66-
app.get('/chat', routes.chat.get);
67-
app.put('/chat', routes.chat.add);
68-
app.delete('/chat', routes.chat.delete);
69-
app.use('/users', routesUsers)
7087

7188
// Static
72-
app.use(st({ path: './public', url: '/public' }));
89+
app.use('/public', express.static(path.join(__dirname, 'public')));
7390

7491
// Add the option to output (sanitized!) markdown
75-
marked.setOptions({ sanitize: true });
76-
app.locals.marked = marked;
92+
app.locals.marked = function (src) {
93+
return sanitizeHtml(marked.parse(String(src), { async: false }));
94+
};
7795

7896
// development only
7997
if (app.get('env') == 'development') {
8098
app.use(errorHandler());
8199
}
82100

83-
var token = 'SECRET_TOKEN_f8ed84e8f41e4146403dd4a6bbcea5e418d23a9';
84-
console.log('token: ' + token);
101+
var server;
102+
if (process.env.TLS_KEY && process.env.TLS_CERT) {
103+
server = https.createServer({
104+
key: fs.readFileSync(process.env.TLS_KEY),
105+
cert: fs.readFileSync(process.env.TLS_CERT),
106+
}, app);
107+
} else {
108+
server = http.createServer(app);
109+
}
85110

86-
http.createServer(app).listen(app.get('port'), function () {
111+
server.listen(app.get('port'), function () {
87112
console.log('Express server listening on port ' + app.get('port'));
88113
});

‎docker-compose.yml‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ services:
55
container_name: goof
66
environment:
77
- DOCKER=1
8+
- ADMIN_USERNAME=admin@snyk.io
9+
- MYSQL_HOST=goof-mysql
10+
- MYSQL_PASSWORD=root
811
ports:
912
- "3001:3001"
1013
- "9229:9229"
@@ -14,7 +17,7 @@ services:
1417
- goof-mongo
1518
goof-mongo:
1619
container_name: goof-mongo
17-
image: mongo:3
20+
image: mongo:6
1821
ports:
1922
- "27017:27017"
2023
good-mysql:

‎mongoose-db.js‎

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
var mongoose = require('mongoose');
22
var cfenv = require("cfenv");
3+
var crypto = require('crypto');
34
var Schema = mongoose.Schema;
45

56
var Todo = new Schema({
@@ -11,14 +12,35 @@ mongoose.model('Todo', Todo);
1112

1213
var User = new Schema({
1314
username: String,
14-
password: String,
15+
passwordHash: String,
16+
passwordSalt: String,
1517
});
1618

19+
function scrypt(password, salt) {
20+
return new Promise(function (resolve, reject) {
21+
crypto.scrypt(password, salt, 64, function (err, key) {
22+
if (err) return reject(err);
23+
resolve(key);
24+
});
25+
});
26+
}
27+
28+
User.methods.setPassword = async function (password) {
29+
this.passwordSalt = crypto.randomBytes(16).toString('hex');
30+
this.passwordHash = (await scrypt(password, this.passwordSalt)).toString('hex');
31+
};
32+
33+
User.methods.verifyPassword = async function (password) {
34+
if (!this.passwordHash || !this.passwordSalt) return false;
35+
var expected = Buffer.from(this.passwordHash, 'hex');
36+
var actual = await scrypt(password, this.passwordSalt);
37+
return expected.length === actual.length && crypto.timingSafeEqual(expected, actual);
38+
};
39+
1740
mongoose.model('User', User);
1841

1942
// CloudFoundry env vars
2043
var mongoCFUri = cfenv.getAppEnv().getServiceURL('goof-mongo');
21-
console.log(JSON.stringify(cfenv.getAppEnv()));
2244

2345
// Default Mongo URI is local
2446
const DOCKER = process.env.DOCKER
@@ -42,17 +64,30 @@ if (mongoCFUri) {
4264

4365
console.log("Using Mongo URI " + mongoUri);
4466

45-
mongoose.connect(mongoUri);
46-
4767
User = mongoose.model('User');
48-
User.find({ username: 'admin@snyk.io' }).exec(function (err, users) {
49-
console.log(users);
50-
if (users.length === 0) {
51-
console.log('no admin');
52-
new User({ username: 'admin@snyk.io', password: 'SuperSecretPassword' }).save(function (err, user, count) {
53-
if (err) {
54-
console.log('error saving admin user');
55-
}
56-
});
68+
69+
async function seedAdmin() {
70+
var adminUsername = process.env.ADMIN_USERNAME;
71+
if (!adminUsername) {
72+
console.log('ADMIN_USERNAME not set; skipping admin user seed');
73+
return;
74+
}
75+
var existing = await User.findOne({ username: adminUsername }).exec();
76+
if (existing) return;
77+
console.log('no admin');
78+
var password = process.env.ADMIN_PASSWORD;
79+
if (!password) {
80+
password = crypto.randomBytes(12).toString('base64url');
81+
console.log('ADMIN_PASSWORD not set; generated admin password: ' + password);
5782
}
58-
});
83+
var admin = new User({ username: adminUsername });
84+
await admin.setPassword(password);
85+
await admin.save();
86+
}
87+
88+
mongoose.connect(mongoUri)
89+
.then(seedAdmin)
90+
.catch(function (err) {
91+
console.log('error connecting to MongoDB or seeding admin user');
92+
console.error(err);
93+
});

‎package.json‎

Lines changed: 41 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -7,51 +7,55 @@
77
"type": "git",
88
"url": "https://github.com/Snyk/snyk-todo-list-demo-app/"
99
},
10+
"engines": {
11+
"node": ">=20"
12+
},
1013
"scripts": {
11-
"dev": "NODE_OPTIONS=--openssl-legacy-provider nodemon ./app.js",
12-
"start": "NODE_OPTIONS=--openssl-legacy-provider node app.js",
14+
"dev": "nodemon ./app.js",
15+
"start": "node app.js",
1316
"build": "browserify -r jquery > public/js/bundle.js",
1417
"cleanup": "mongo express-todo --eval 'db.todos.remove({});'",
1518
"test": "snyk test"
1619
},
1720
"dependencies": {
18-
"adm-zip": "0.4.7",
19-
"body-parser": "1.9.0",
20-
"cfenv": "^1.0.4",
21-
"consolidate": "0.14.5",
22-
"dustjs-helpers": "1.5.0",
23-
"dustjs-linkedin": "2.5.0",
24-
"ejs": "1.0.0",
25-
"ejs-locals": "1.0.2",
26-
"errorhandler": "1.2.0",
27-
"express": "4.12.4",
28-
"express-fileupload": "0.0.5",
29-
"express-session": "^1.17.2",
30-
"file-type": "^8.1.0",
31-
"hbs": "^4.0.4",
32-
"humanize-ms": "1.0.1",
33-
"jquery": "^2.2.4",
34-
"lodash": "4.17.4",
35-
"marked": "0.3.5",
36-
"method-override": "latest",
37-
"moment": "2.15.1",
38-
"mongodb": "^3.5.9",
39-
"mongoose": "4.2.4",
40-
"morgan": "latest",
41-
"ms": "^0.7.1",
21+
"adm-zip": "^0.6.1",
22+
"cfenv": "^1.2.7",
23+
"dustjs-linkedin": "^3.0.1",
24+
"ejs": "^3.1.10",
25+
"errorhandler": "^1.5.2",
26+
"express": "^4.21.2",
27+
"express-ejs-layouts": "^2.5.1",
28+
"express-fileupload": "^1.5.2",
29+
"express-rate-limit": "^8.0.0",
30+
"express-session": "^1.18.1",
31+
"file-type": "^21.3.1",
32+
"hbs": "^4.3.0",
33+
"humanize-ms": "^2.0.0",
34+
"jquery": "^3.7.1",
35+
"lodash": "^4.18.1",
36+
"lusca": "^1.7.0",
37+
"marked": "^15.0.12",
38+
"method-override": "^3.0.0",
39+
"moment": "^2.31.0",
40+
"mongoose": "^8.19.0",
41+
"morgan": "^1.12.1",
42+
"ms": "^2.1.3",
4243
"mysql": "^2.18.1",
43-
"npmconf": "0.0.24",
44-
"optional": "^0.1.3",
45-
"st": "0.2.4",
46-
"stream-buffers": "^3.0.1",
47-
"tap": "^11.1.3",
48-
"typeorm": "^0.2.24",
49-
"validator": "^13.5.2"
44+
"sanitize-html": "^2.17.7",
45+
"typeorm": "^0.3.31",
46+
"validator": "^13.15.0"
5047
},
5148
"devDependencies": {
52-
"browserify": "^13.1.1",
53-
"nodemon": "^2.0.7",
54-
"snyk": "^1.244.0"
49+
"browserify": "^17.0.1",
50+
"nodemon": "^3.1.10",
51+
"snyk": "^1.1300.0"
5552
},
56-
"license": "Apache-2.0"
53+
"license": "Apache-2.0",
54+
"overrides": {
55+
"minimatch": "^3.1.3",
56+
"brace-expansion": "^1.1.18",
57+
"cli": {
58+
"glob": "^9.3.5"
59+
}
60+
}
5761
}

0 commit comments

Comments
 (0)