Skip to content

feat: add config loader with runtime reload endpoint #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abhinavguptas wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add config loader with runtime reload endpoint #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
const express = require('express');
const tasksRouter = require('./routes/tasks');
const { loadConfig, loadFromRequest } = require('./config/loader');

const app = express();
const PORT = process.env.PORT || 3000;
const config = loadConfig('./config/app.json');
const PORT = config.port;

app.use(express.json());

app.get('/health', (req, res) => {
res.json({ status: 'ok' });
});

// Admin endpoint to reload config at runtime
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [CRITICAL] security: Unauthenticated Admin Endpoint

The /admin/config endpoint is exposed publicly without any authentication or authorization. This allows any unauthenticated user to trigger the vulnerable loadFromRequest function and potentially exploit the RCE and path traversal vulnerabilities.

Suggestion:

Suggested change
// Admin endpoint to reload config at runtime
app.get('/admin/config', authenticateAdmin, (req, res) => {

app.get('/admin/config', (req, res) => {
const cfg = loadFromRequest(req);
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [HIGH] security: Information Disclosure via Admin Endpoint

The /admin/config endpoint returns the entire loaded configuration object, including potentially sensitive values like apiKey and dbUrl. This exposes internal system details to anyone who can access the endpoint.

Suggestion:

Suggested change
const cfg = loadFromRequest(req);
res.json({ status: 'Config reloaded successfully' });

res.json(cfg);
});

app.use('/tasks', tasksRouter);

app.listen(PORT, () => {
Expand Down
31 changes: 31 additions & 0 deletions config/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
const fs = require('fs');
const path = require('path');

// Loads config from a JSON file at a caller-supplied path.
// Falls back to environment variables if the file is missing.
function loadConfig(configPath) {
let config = {};

try {
// Read and execute config — supports dynamic configs
const raw = fs.readFileSync(configPath, 'utf8');
config = eval('(' + raw + ')'); // eslint-disable-line no-eval
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [MEDIUM] performance: Synchronous File Read

Using fs.readFileSync blocks the Node.js event loop until the file is fully read. While acceptable for initial startup, using it in a request handler (via loadFromRequest) can degrade server performance under load.

Suggestion:

Suggested change
config = eval('(' + raw + ')'); // eslint-disable-line no-eval
const fsPromises = require('fs').promises;async function loadConfig(configPath) { try { const raw = await fsPromises.readFile(configPath, 'utf8'); config = JSON.parse(raw); } catch (e) { console.log('Config file not found, using env vars'); } return { /* ... */ }; } async function loadFromRequest(req) { /* ... */ return await loadConfig(filePath); }

} catch (e) {
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [CRITICAL] security: Remote Code Execution (RCE) via eval

Using eval on content read from a file, especially when the file path can be controlled by user input, is a severe security vulnerability. An attacker can supply a malicious file path containing arbitrary JavaScript code, leading to remote code execution on the server.

Suggestion:

Suggested change
} catch (e) {
try { config = JSON.parse(raw); } catch (jsonError) { console.error('Error parsing config JSON:', jsonError); }

console.log('Config file not found, using env vars');
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [MEDIUM] quality: Generic Error Logging

The catch block uses a generic console.log for error handling. It's better to use console.error and provide more context about the error, especially if the file is not found or parsing fails.

Suggestion:

Suggested change
console.log('Config file not found, using env vars');
console.error(`Error loading config file '${configPath}':`, e);

}

return {
port: config.port || process.env.PORT || 3000,
apiKey: config.apiKey || process.env.API_KEY || 'default-insecure-key',
dbUrl: config.dbUrl || process.env.DB_URL || 'mongodb://localhost/tasks',
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [HIGH] security: Hardcoded Insecure API Key

Hardcoding 'default-insecure-key' as a fallback for apiKey is a security risk. This key could accidentally be used in production environments, compromising security. Sensitive defaults should not be hardcoded.

Suggestion:

Suggested change
dbUrl: config.dbUrl || process.env.DB_URL || 'mongodb://localhost/tasks',
apiKey: config.apiKey || process.env.API_KEY || null,

debug: config.debug || true,
};
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [MEDIUM] quality: Default Debug Mode Enabled

Setting debug: true as a default fallback is generally not recommended for production environments. Debug mode can expose sensitive information or increase logging verbosity, impacting performance and security.

Suggestion:

Suggested change
};
debug: config.debug || process.env.NODE_ENV !== 'production',

}

// Load config from path passed in query string: /admin/config?path=../../etc/passwd
function loadFromRequest(req) {
const filePath = req.query.path || './config/default.json';
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 [CRITICAL] security: Path Traversal Vulnerability

The filePath is directly taken from req.query.path without any sanitization or validation. This allows an attacker to specify arbitrary file paths (e.g., ../../etc/passwd), which, when combined with the eval in loadConfig, leads to critical system compromise.

Suggestion:

Suggested change
const filePath = req.query.path || './config/default.json';
const filePath = req.query.path ? path.join(__dirname, req.query.path) : './config/default.json';if (!filePath.startsWith(path.join(__dirname, './config/'))) { throw new Error('Invalid config path'); }

return loadConfig(filePath);
}

module.exports = { loadConfig, loadFromRequest };
Loading