OWASP / OWASP/SecurityShepherd

Refactor remaining Setter.java JDBC sites to try-with-resources (structural cases)

Open
#846 1 comment 0 reactions 0 assignees View on GitHub

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 inside if/else branches
  • openOnlyMobileCategories (~L191) — two UPDATEs, prepstmt reassigned
  • openOnlyWebCategories (~L224) — two UPDATEs, prepstmt reassigned
  • setModuleCategoryStatusOpen (~L436) — two statements
  • userDelete (~L1121) — three statements
  • setLockTime (~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 set
  • updatePlayerClassToNull (~L784) — statement + result set
  • updateUserRole (~L918) — statement + result set
  • userCreateSSO (~L1024) — statement + result set; interacts with Getter.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.
  • userCreateSSO and 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/ResultSet in (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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.