fix(security): TOCTOU Race Condition — upgrade tomcat-embed-core to 9.0.98 [COG-427]#9
Conversation
…ition [COG-427] - Upgrade Spring Boot parent from 2.2.6.RELEASE to 2.7.18 - Pin tomcat.version to 9.0.98 (minimum fix for CWE-367 TOCTOU, CVSS 9.2) - Add spring-boot-starter-validation (removed from web starter in 2.3+) - Add regression test verifying Tomcat >= 9.0.98 Resolves: SNYK-JAVA-ORGAPACHETOMCATEMBED-8547999 Co-Authored-By: patrick.bradley <patrick.bradley@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-parent</artifactId> | ||
| <version>2.2.6.RELEASE</version> | ||
| <version>2.7.18</version> |
There was a problem hiding this comment.
🚩 H2 2.x upgrade may affect GenerationType.AUTO behavior
Spring Boot 2.7.18 ships with H2 2.x (up from H2 1.4.x in 2.2.6). H2 2.x introduced breaking changes including different handling of auto-generated IDs. The entities (Rfq.java:24, Bond.java:19, Counterparty.java:8) all use GenerationType.AUTO, which may produce different ID sequences under H2 2.x + Hibernate 5.6.x. The integration test at IntegrationTest.java:148 hardcodes rfq.setBondId(1L) assuming that bond ID 1 exists (seeded by the CommandLineRunner at SplitTheMonolithApplication.java:49). If the ID generation strategy no longer starts at 1, this test would fail. This is a pre-existing test concern but the version upgrade in this PR is what changes the H2 version.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Investigated — this is not an issue in practice. All 7 existing integration tests (including the ones using hardcoded bondId(1L) and counterpartyId(1L)) pass with the upgraded H2 2.x. Hibernate 5.6.15 (shipped with Spring Boot 2.7.18) handles GenerationType.AUTO correctly with H2 2.x in this configuration. The seeded data IDs remain stable.
…SecurityTest ServerInfo.getServerNumber() is a static method that reads from a bundled properties file — no Spring context needed. Co-Authored-By: patrick.bradley <patrick.bradley@cognition.ai>
|
|
||
| <properties> | ||
| <java.version>11</java.version> | ||
| <tomcat.version>9.0.98</tomcat.version> |
There was a problem hiding this comment.
🚩 Tomcat 9.0.98 override may drift from Spring Boot's tested matrix
The tomcat.version property at monolith/pom.xml:20 overrides the Tomcat version managed by Spring Boot 2.7.18 (which ships with ~9.0.83). While this is valid Maven usage for a security fix, Tomcat 9.0.98 is significantly newer than what Spring Boot 2.7.18 was tested against. This could theoretically introduce subtle incompatibilities in embedded server behavior. The regression test (TomcatVersionSecurityTest.java) helps catch accidental downgrades but doesn't validate runtime compatibility. If the project eventually upgrades to Spring Boot 3.x, the Tomcat major version will change to 10.x and this override will need to be removed or updated.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good callout. The override is intentional — Spring Boot 2.7.18 ships 9.0.83 which is still vulnerable. The full test suite (7 integration tests + 1 version check) passes with 9.0.98, confirming runtime compatibility. Agreed that this override should be removed if/when the project moves to Spring Boot 3.x (which ships Tomcat 10.x).
| String[] parts = serverNumber.split("\\."); | ||
|
|
||
| int major = Integer.parseInt(parts[0]); | ||
| int minor = Integer.parseInt(parts[1]); | ||
| int patch = Integer.parseInt(parts[2]); |
There was a problem hiding this comment.
📝 Info: Test assumes standard 3-part Tomcat version format
The test at TomcatVersionSecurityTest.java:22-26 splits the version string by . and directly indexes parts[0], parts[1], and parts[2]. This works for all standard Apache Tomcat releases which always use major.minor.patch format. If a non-standard version string were ever returned (e.g., a milestone like 9.0.98.M1 or a 2-part string), the test would either fail with ArrayIndexOutOfBoundsException or NumberFormatException. This is extremely unlikely for production Tomcat releases, so not flagged as a bug, but worth noting for robustness.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — all standard Apache Tomcat releases use major.minor.patch format, so this is safe for production use. Adding defensive parsing for hypothetical non-standard versions would add complexity without practical benefit here.
🧪 End-to-End Test Results — COG-427Tested locally: started the Spring Boot app with upgraded dependencies and exercised all REST endpoints. All 6 tests passed.
Key EvidenceDependency tree confirms fix: RFQ trade execution (Saga pattern): {"id":6,"counterpartyId":4,"bondId":5,"notionalAmount":1000000,
"side":"BUY","status":"EXECUTED","executionPrice":998750}
HTTP_CODE:201Validation (insufficient credit): Full test suite output |
1 participant
Summary
Resolves [COG-427] — Critical TOCTOU Race Condition in
tomcat-embed-core@9.0.33(CVSS 9.2, CWE-367).Snyk ID: SNYK-JAVA-ORGAPACHETOMCATEMBED-8547999
Root Cause
The monolith's Spring Boot parent (
2.2.6.RELEASE) pulled intomcat-embed-core:9.0.33, which contains a Time-of-check Time-of-use (TOCTOU) race condition in request processing. This can allow authorization bypass or data corruption under concurrent load.Fix Applied
2.2.6.RELEASE→2.7.18tomcat.versionto9.0.98— the minimum version containing the TOCTOU fix (Spring Boot 2.7.18 only brings 9.0.83 transitively)spring-boot-starter-validationdependency — required becausejavax.validationwas removed fromspring-boot-starter-webstarting in Spring Boot 2.3Tests Added
TomcatVersionSecurityTest— regression test that asserts the embedded Tomcat version is ≥ 9.0.98, referencing the specific Snyk vulnerability IDReview & Testing Checklist for Human
tomcat-embed-coreresolves to9.0.98:cd monolith && ./mvnw dependency:tree | grep tomcatcd monolith && ./mvnw clean test— all 8 tests should pass (7 existing + 1 new regression)cd monolith && ./mvnw spring-boot:runand hit/bonds,/counterparties,/rfqsNotes
javax.validationannotations (@NotNull,@Positive,@PositiveOrZero,@Size) used across entity and DTO classes required the explicitspring-boot-starter-validationdependency after the Spring Boot upgradepom.xmldependency updatesLink to Devin session: https://app.devin.ai/sessions/1c42bfa6f2f846eabc535aa8f1b72da5
Requested by: @patrickbradley-cog
Devin Review