Skip to content

security: harden server bindings, auth gates, and error handling #39

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

Merged
atomantic merged 2 commits into from
Mar 6, 2026
Merged

security: harden server bindings, auth gates, and error handling #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e517c2
security: harden server bindings, auth gates, and error handling
atomantic Mar 6, 2026
f400c04
chore: bump version to 0.8.5
atomantic Mar 6, 2026
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
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "sparsetree",
"version": "0.8.4",
"version": "0.8.5",
"private": true,
"description": "",
"main": "index.js",
Expand Down
4 changes: 4 additions & 0 deletions server/src/db/sqlite.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,13 @@ function initDb(): Database.Database {
}

// Create database connection
const isNew = !fs.existsSync(DB_PATH);
db = new Database(DB_PATH, {
verbose: process.env.SQLITE_VERBOSE ? console.log : undefined,
});
if (isNew) {
fs.chmodSync(DB_PATH, 0o600);

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

fs.chmodSync(DB_PATH, 0o600) can throw (e.g., permission issues) and will crash server startup. Consider wrapping in try/catch and logging a warning, and/or applying permissions in a best-effort way rather than failing hard.

Suggested change
fs.chmodSync(DB_PATH, 0o600);
try {
fs.chmodSync(DB_PATH, 0o600);
} catch (error) {
console.warn(
`Warning: Failed to set permissions on SQLite database file at "${DB_PATH}".`,
error,
);
}

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +31

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

DB permissions are only tightened when the DB file is newly created. If a user already has an existing DB with overly-permissive permissions, this change won’t address it. Consider applying the chmod best-effort on every startup (or at least when the file exists but mode is too open), while handling platforms/filesystems where chmod is unsupported.

Suggested change
if (isNew) {
fs.chmodSync(DB_PATH, 0o600);
// Best-effort: ensure database file permissions are restrictive (owner read/write only)
try {
if (fs.existsSync(DB_PATH)) {
const stats = fs.statSync(DB_PATH);
const mode = stats.mode & 0o777;
// If group/other have any permissions, or owner is missing read/write, tighten to 0o600
if ((mode & 0o077) !== 0 || (mode & 0o600) !== 0o600) {
fs.chmodSync(DB_PATH, 0o600);
}
}
} catch {
// Ignore errors (e.g., unsupported filesystem/platform or insufficient privileges)

Copilot uses AI. Check for mistakes.
}

// Performance optimizations
db.pragma('journal_mode = WAL'); // Write-Ahead Logging for better concurrency
Expand Down
20 changes: 17 additions & 3 deletions server/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ import { logger } from './lib/logger.js';

const CORS_ORIGIN = process.env.CORS_ORIGIN || 'http://localhost:6373';
const corsOrigin = CORS_ORIGIN.includes(',')
? CORS_ORIGIN.split(',').map(o => o.trim())
? CORS_ORIGIN.split(',').map(o => {
const trimmed = o.trim();
new URL(trimmed); // throws on invalid origin

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

new URL(trimmed) will throw during module initialization if CORS_ORIGIN contains an invalid entry (including a trailing comma producing an empty string). Right now that will crash startup without a clear message about which env var/value was bad. Consider catching the error and re-throwing with an explicit message that includes the offending origin string and that it came from CORS_ORIGIN.

Suggested change
new URL(trimmed); // throws on invalid origin
if (!trimmed) {
throw new Error(
`Invalid CORS_ORIGIN configuration: found empty origin entry in CORS_ORIGIN='${CORS_ORIGIN}'. ` +
'Remove empty entries (for example, trailing commas).'
);
}
try {
// Validate origin; URL constructor will throw on invalid values
new URL(trimmed);
} catch (err) {
const message = (err as Error).message || String(err);
throw new Error(
`Invalid CORS_ORIGIN configuration: origin '${trimmed}' from CORS_ORIGIN='${CORS_ORIGIN}' is not a valid URL: ${message}`
);
}

Copilot uses AI. Check for mistakes.
return trimmed;
})
: CORS_ORIGIN;
Comment on lines 36 to 43

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

CORS_ORIGIN validation/normalization is only applied when the env var contains a comma. For a single origin, whitespace and invalid URLs won’t be caught, and leading/trailing spaces will cause origin mismatches. Consider always splitting (even for a single value), trimming, validating with URL parsing, and using the parsed origin so values like http://host:port/path don’t silently misconfigure CORS.

Copilot uses AI. Check for mistakes.

const app = express();
Expand Down Expand Up @@ -114,8 +118,18 @@ if (existsSync(clientDist)) {
// Error handling
app.use(errorHandler);

httpServer.listen(PORT, '0.0.0.0', () => {
logger.start('server', `Running on http://localhost:${PORT}`);
const HOST = process.env.HOST || 'localhost';

const shutdown = () => {
logger.warn('server', 'Shutting down gracefully...');
httpServer.close(() => process.exit(0));
setTimeout(() => process.exit(1), 5000);
};
Comment on lines +123 to +127

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

The shutdown handler calls httpServer.close(), which waits for all open connections to finish. With SSE endpoints in this server, the close callback may never run, leading to the forced process.exit(1) after 5s even during a normal shutdown. Consider tracking sockets and destroying them on shutdown (or using httpServer.closeAllConnections()/closeIdleConnections where available) so shutdown is predictable and can exit cleanly.

Copilot uses AI. Check for mistakes.
process.on('SIGTERM', shutdown);
process.on('SIGINT', shutdown);

httpServer.listen(PORT, HOST, () => {
logger.start('server', `Running on http://${HOST}:${PORT}`);

// Auto-connect to browser if enabled and browser is running
browserService.autoConnectIfEnabled();
Expand Down
2 changes: 1 addition & 1 deletion server/src/middleware/errorHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export const errorHandler = (
res: Response,
_next: NextFunction
) => {
logger.error('server', `Unhandled: ${err.stack || err.message}`);
logger.error('server', `Unhandled: ${process.env.NODE_ENV !== 'production' ? (err.stack || err.message) : err.message}`);
res.status(500).json({
success: false,
error: err.message || 'Internal server error'
Expand Down
2 changes: 1 addition & 1 deletion server/src/routes/ancestry-update.routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ router.get('/:dbId/events', async (req: Request, res: Response) => {
}
}

const isTestMode = testMode === 'true';
const isTestMode = testMode === 'true' && process.env.NODE_ENV !== 'production';

initSSE(res);

Expand Down
2 changes: 2 additions & 0 deletions server/src/routes/browser.routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ router.get('/photos/:personId/exists', async (req: Request, res: Response) => {
});

// Get FamilySearch authentication token from browser session
// Security note: This endpoint returns an auth token in the JSON response.
// Acceptable because SparseTree is a local-only tool and FS tokens are short-lived.
router.get('/token', async (_req: Request, res: Response) => {
Comment on lines +214 to 215

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

These comments state returning an auth token is acceptable because the tool is "local-only", but the server can be exposed by setting HOST (and CORS can be configured to allow non-local origins). Consider updating the comment to reflect that this endpoint is only safe when the server is bound to localhost/not exposed, or alternatively gate the endpoint (e.g., require a local-only check / auth token / non-production guard) to prevent accidental exposure.

Suggested change
// Acceptable because SparseTree is a local-only tool and FS tokens are short-lived.
router.get('/token', async (_req: Request, res: Response) => {
// It is intended to be used only from localhost and must not be exposed externally.
router.get('/token', async (req: Request, res: Response) => {
// Enforce localhost-only access to prevent accidental exposure when server is not bound
// exclusively to a loopback interface.
const remoteAddr = req.ip || req.socket.remoteAddress || '';
const isLocalhost =
remoteAddr === '127.0.0.1' ||
remoteAddr === '::1' ||
remoteAddr.startsWith('::ffff:127.0.0.1');
if (!isLocalhost) {
res.status(403).json({ success: false, error: 'Token endpoint is only accessible from localhost' });
return;
}

Copilot uses AI. Check for mistakes.
if (!browserService.isConnected()) {
res.status(400).json({ success: false, error: 'Browser not connected' });
Expand Down
3 changes: 2 additions & 1 deletion server/src/routes/genealogy-provider.routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Router, Request, Response } from 'express';
import crypto from 'crypto';
import type { GenealogyProviderConfig, PlatformType } from '@fsf/shared';
import { genealogyProviderService } from '../services/genealogy-provider.service.js';
import { pickFields } from '../utils/validation.js';
Expand Down Expand Up @@ -60,7 +61,7 @@ router.post('/', (req: Request, res: Response) => {

// Generate ID if not provided
if (!config.id) {
config.id = config.platform + '-' + Date.now();
config.id = config.platform + '-' + crypto.randomUUID();
}
Comment on lines 62 to 65

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

This route now generates provider IDs with crypto.randomUUID(), but genealogyProviderService.saveProvider() still falls back to config.platform + '-' + Date.now() when config.id is missing. That means callers other than this route (or future refactors) may still create predictable IDs. Consider updating the service-level fallback to use the same UUID strategy so ID generation is consistent and not dependent on the route layer.

Copilot uses AI. Check for mistakes.

// Set defaults if not provided
Expand Down
9 changes: 9 additions & 0 deletions server/src/routes/test-runner.routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@ import { logger } from '../lib/logger.js';

export const testRunnerRouter = Router();

// Gate all test-runner endpoints behind non-production environment
testRunnerRouter.use((_req, res, next) => {
if (process.env.NODE_ENV === 'production') {
res.status(403).json({ success: false, error: 'Test runner is disabled in production' });
return;
}
next();
});

// GET /api/test-runner/status - Get current test run status
testRunnerRouter.get('/status', (_req, res) => {
res.json({ success: true, data: testRunnerService.getStatus() });
Expand Down
Loading