[ismisepaul]

Note: This PR supersedes draft #800 (same work; head branch is now OWASP/SecurityShepherd:dev#536 instead of a fork). You can close #800 in favor of this PR.

Summary

Fixes #536 — automated attack tools (e.g. ZAP spider/fuzzer) exhaust all MySQL connections, causing multi-hour outages.

• HikariCP connection pooling for MySQL/MariaDB with configurable pool size, connection timeouts, idle timeouts, and max lifetime — connections are bounded and reused instead of created per-request
• Singleton MongoClient pattern for MongoDB, leveraging the driver's built-in connection pooling
• DatabaseLifecycleListener for clean pool initialization on startup and shutdown on context destroy — no more leaked connections requiring manual Tomcat restarts
• Documentation for database configuration, capacity planning, and troubleshooting
• Tests for ConnectionPool, MongoDatabase singleton, and lifecycle listener

Changes

File	Change
ConnectionPool.java	New — HikariCP pool manager with core + per-challenge pools
Database.java	Refactored to delegate all connections through ConnectionPool
MongoDatabase.java	Refactored to singleton pattern with double-checked locking
DatabaseLifecycleListener.java	New — ServletContextListener for pool lifecycle
web.xml	Registered lifecycle listener
pom.xml	Added HikariCP dependency
docs/database-configuration.md	New — pool config reference, sizing guide, troubleshooting
docs/development-workflow.md	New — dev cycle guide
docs/testing.md	New — test running guide
CONTRIBUTING.md	Links to new docs
*.properties.example	New — example config files for MySQL and MongoDB
*Test.java	New/updated tests for pool and singleton patterns

Default Pool Configuration

Setting	Default	Purpose
pool.maximumPoolSize	10	Max connections per pool
pool.minimumIdle	2	Min idle connections maintained
pool.connectionTimeout	30s	Max wait for a connection before failing
pool.idleTimeout	10min	Idle connection cleanup
pool.maxLifetime	30min	Max connection lifetime

All values are configurable via database.properties.

Test plan

• Unit tests pass (mvn test with Docker db containers running)
• Application starts cleanly with pool initialization logged
• Application shuts down cleanly with pool shutdown logged
• Under load (e.g. ZAP fuzzer), connections are bounded and reused — no connection exhaustion
• Challenge connections work correctly with per-schema pools
• MongoDB operations work correctly with singleton client
• Custom pool sizes in database.properties are respected

Discussion (from #800 comments)

The following was posted on PR #800 so reviewers do not need to open that draft for context.

Challenge pool sizing

After reviewing the challenge connection architecture:

Per-challenge pools are the right design — Each challenge uses different DB credentials scoped to its own schema (26 challenge properties files, each with unique username/password). This is intentional: it prevents a SQL injection in one challenge from accessing another challenge's data. A single shared pool with setCatalog() would require a single DB user with access to all schemas, defeating that isolation.

Challenge pools should be smaller than core — The connection exhaustion problem (#536) is caused by fuzzers/ZAP hammering core app endpoints (auth, scoreboard, module lookups via Getter/Setter), not challenge endpoints. Challenges are low-traffic by nature. Keeping pooling for challenges still helps (avoids TCP handshake + auth per request), but with minIdle=0 there is zero cost when a challenge is not in use, and connections spin up on demand when a student starts working.

Defaults in ConnectionPool.java for challenge vs core:

Setting	Core pool	Challenge pool
maxPoolSize	10	3
minIdle	2	0
idleTimeout	10 min	2 min

With 26 challenges, worst case is 26 × 3 = 78 connections if every challenge is under active use simultaneously; in practice with minIdle=0 and a 2-minute idle timeout, idle challenge pools hold zero connections.

Connection leaks and follow-up PRs

Pooling exposes pre-existing connection leaks in Getter.java (~32 methods) and Setter.java (~24 methods). Without try-with-resources, connections borrowed from the pool may not be returned — the core pool (max 10) can exhaust within minutes under normal use.

Required follow-up: convert Database.get*Connection() usage to try-with-resources and remove manual Database.closeConnection(); add tests alongside fixes. Priority: Getter.java (every request) > Setter.java (admin / user ops) > challenge servlets (isolated pools, lower risk).

Status: Getter.java fixes are in #817 (stacked on this PR). Setter.java and servlets are tracked separately (e.g. #815).

[ismisepaul]

Integration test failures: connection pool exhaustion

The test failures are caused by connection leaks that exhaust HikariCP's pool (max 20 connections, 5s timeout). Three categories of leak:

1. TestProperties.executeSql() — never closes its connection

Every test class calling this in @BeforeAll leaks a connection:

// src/test/java/testUtils/TestProperties.java:50
Connection databaseConnection = Database.getDatabaseConnection(null, true);
// ... used but never closed

2. Three Getter methods return ResultSet without closing the Connection

• getClassInfo(ApplicationRoot) (line 614)
• getPlayersByClass(ApplicationRoot, classId) (line 1759)
• getAdmins(ApplicationRoot) (line 2285)

Each caller leaks a connection since there's no way to close it through the returned ResultSet.

3. All ~39 Setter methods close connections inside try but not finally

Any exception between acquiring and closing leaks the connection:

try {
    Connection conn = Database.getCoreConnection(ApplicationRoot);
    // ... if anything throws here, conn leaks
    Database.closeConnection(conn);  // only reached on success
} catch (SQLException e) { ... }

Fix

• TestProperties.executeSql(): wrap in try-with-resources
• Getter ResultSet methods: refactor to process results internally and close the connection, or pass the connection from a try-with-resources block in the caller
• Setter methods: convert all to try (Connection conn = ...) { }

These leaks were invisible before pooling since each connection was just opened/GC'd independently. With HikariCP, unreturned connections stay checked out permanently, so ~10-15 leaked connections exhaust the pool and every subsequent test times out.

[ismisepaul]

Status update on the deferred follow-ups (re: next steps)

Picking up the three deferred items from the next-steps comment:

✅ Item 1 — nested try-with-resources for Getter/Setter statements/result-sets — DONE

Originally planned for a branch off dev post-merge; it's now complete and already merged into dev#536:

• Setter.java — Refactor remaining Setter.java JDBC sites to try-with-resources (structural cases) #846 / PR fix: try-with-resources for remaining Setter.java JDBC sites (#846) #852 (also closed the SSO-path slice Refactor Getter.authUserSSO to use try-with-resources for JDBC resources #840 via fix: try-with-resources for authUserSSO (#840) + Setter JDBC statements #845). Setter.java now has 0 unmanaged JDBC sites.
• Getter.java — Refactor remaining Getter.java JDBC sites to try-with-resources #855 / PR refactor: try-with-resources for remaining Getter.java JDBC sites (#855) #856. All 44 sites converted. The only remaining bare declarations are intentional: the three CachedRowSet-returning methods tracked under item 2 / Convert ResultSet-returning Getter methods to bounded collections (DAO refactor follow-up) #839, and getJsonScore's conditionally-assigned statement (which is closed via a TWR resource).

Both validated green via the workflow_dispatch Build and Test runs (incl. integration-tests + lint-java/Spotless), and the latest dev#536 PR run is green with both merged in. This should silence the recurring CodeQL "Potential database resource leak" flags for Getter.java/Setter.java.

⏸️ Item 2 — ResultSet-returning Getter methods → bounded collections

Still deferred as designed — tracked in #839, part of the DAO refactor (#815). The 3 methods (getClassInfo(String), getPlayersByClass, getAdmins) are intentionally untouched.

⏸️ Item 3 — challenge servlet connection-leak conversions

Still deferred as designed — ~35 files under servlets/module/challenge/ and .../lesson/, to be batched by challenge family post-merge.

---

Net: the work this PR's next-steps comment flagged as blocking nothing is progressing; item 1 is fully landed ahead of schedule. Items 2 and 3 remain open follow-ups and (per the original note) do not block this PR.

[SeanDuggan]

Substantial update. Everything here looks good from my perspective.

Note:
tests/load/README.md makes references to load-test.sh (e.g. ## load-test.sh, ./load-test.sh, ./load-test.sh --skip-build) but the actual file appear to be load-test.py