OWASP / OWASP/SecurityShepherd
Refactor remaining Setter.java JDBC sites to try-with-resources (structural cases)
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 1.5k
- Forks
- 515
- Avg merge
- 3h 46m
- Merged PRs (30d)
- 1
Description
Context
Follow-up to the dev#536 connection-pooling work (#816) and its try-with-resources cleanup PRs (#834, #838, #842, and the #840 + Setter-settings PR stacked on #816).
After those PRs, the mechanically simple Setter methods (bare Connection try-with-resources with a single inner PreparedStatement/CallableStatement) have been converted to combined try-with-resources. This issue tracks the remaining structurally complex sites in Setter.java that were intentionally deferred to keep that PR reviewable.
These are genuine CodeQL "potential database resource leak" candidates (statements/result-sets borrowed from a pooled Connection without being in nested try-with-resources). Severity is Low — HikariCP closes child statements/result-sets when the parent Connection returns to the pool, so the practical leak window is microseconds and the #536 outage cause is already fixed. The motivation is code consistency and stopping CodeQL from re-firing on every PR into dev.
Sites
Grouped by the kind of refactor required (not a uniform mechanical rewrap):
A. Multiple statements / reassigned statement variable
The statement var is reassigned to a second prepareStatement(...), dropping the first reference without closing it. Needs sequential or nested try-with-resources per statement.
openAllModules(~L159) — statement created insideif/elsebranchesopenOnlyMobileCategories(~L191) — twoUPDATEs,prepstmtreassignedopenOnlyWebCategories(~L224) — twoUPDATEs,prepstmtreassignedsetModuleCategoryStatusOpen(~L436) — two statementsuserDelete(~L1121) — three statementssetLockTime(~L1406),setEndTimeStatus(~L1430),setEndTime(~L1454) — settings setters with extra statements
B. Statement + ResultSet
Both the statement and the result set need nesting; some also reassign.
updatePlayerClass(~L754) — statement + result setupdatePlayerClassToNull(~L784) — statement + result setupdateUserRole(~L918) — statement + result setuserCreateSSO(~L1024) — statement + result set; interacts withGetter.authUserSSO(the #840 caller), so verify the SSO create flow
C. Challenge-connection sites (manual .close(), reassignment)
Use getChallengeConnection(...), reassign the statement, and call rs.close() / stmt.close() manually. Per-challenge pools (max=3, minIdle=0) so blast radius is small.
setCsrfChallengeFourCsrfToken(~L333)setCsrfChallengeSevenCsrfToken(~L383)
Suggested approach
- One PR (or a small series grouped A / B / C). Same nested try-with-resources pattern already used in #834 / #842.
- Where a statement is reassigned, give each query its own try-with-resources scope rather than forcing both into one resource list.
userCreateSSOand the CSRF challenge methods deserve targeted attention — confirm no behavior change in the SSO create flow and the CSRF token store/update flow.- Verify with the integration suite (
GetterIT/SetterIT) and, for the challenge sites, the relevant challenge ITs.
Acceptance criteria
- All listed methods have their
PreparedStatement/CallableStatement/ResultSetin (nested) try-with-resources. - CodeQL no longer flags these sites in
Setter.java. - No behavior change in module open/close, player class moves, user role changes, user delete, SSO user creation, or CSRF token storage.
-
GetterIT/SetterIT(and challenge ITs for the CSRF sites) pass.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in Setter.java at the listed methods and compare their structure with the try-with-resources conversions in PRs #834 and #842. Pay particular attention to userCreateSSO with Getter.authUserSSO and the two CSRF challenge methods, then run GetterIT, SetterIT, and the relevant challenge integration tests. Done means all listed statements and result sets are resource-managed and CodeQL no longer flags these sites without behavior changes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, databases
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100