bug: remediate audit control failures on money movement (amount validation, atomic transfer, CSRF, audit log) - #286
Open
achalc wants to merge 1 commit into
Open
Conversation
…ation, atomic transfer, CSRF, audit log) Adds positive/bounded BigDecimal validation, @transactional on deposit/withdraw/transfer, re-enables CSRF with tokens in all views, and a structured audit log for every financial event. Includes docs/audit-control-report.md with the control test results and residual findings. Co-Authored-By: Achal Channarasappa <achal.channarasappa@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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Control test of 7 controls against the code, then remediation of the four that can be fixed safely in one PR. Results,
file:lineevidence and residual findings are indocs/audit-control-report.md.Four control failures fixed:
deposit/withdraw/transferAmountaccepted anyBigDecimal, so a negative transfer debited the recipient. All three now callvalidateAmountfirst:from.username.equals(toUsername)), the existing overdraft checks retained, andmin/max/stepon the amount inputs.BankController.depositgained the error path the other two endpoints already had.@Transactionalondeposit,withdraw,transferAmount. Previously the transfer's four repository writes auto-committed individually, so a failure after the debit destroyed money..csrf(csrf -> csrf.disable())(Spring Security'sCsrfFilteris on by default), added_csrfhidden inputs to every POST form, and made logout a POST form matched byAntPathRequestMatcher("/logout", "POST")instead of a GET link.AuditLoggeremits one structured line per financial event on both the success and failure path:Not fixed here, written up as residual findings R-1…R-6 with owner and reason: maker-checker/daily limits, the role model for SoD (needs a schema change), committed DB credentials (needs rotation + external secret store), CODEOWNERS/branch protection, the Jenkinsfile checking out a different upstream repo, and
ddl-auto=update.Verification
15 tests,
BUILD SUCCESS, against a real MySQL (jdbc:mysql://localhost:3306/bankappdb, root/Test@123,mysql_native_password):AccountServiceValidationTest— negative/zero/over-limit amounts and overdraft on deposit, withdraw and transfer all reject with no repository write, and failures are audit-logged.TransferAtomicityIntegrationTest— injects a failure on the credit leg via@SpyBean TransactionRepositoryand asserts both balances and both ledger rows roll back; plus the happy path.BankControllerCsrfTest— POST/depositand/transferare 403 without a token, redirect with one.docker run -d --name bankapp-mysql -e MYSQL_ROOT_PASSWORD=Test@123 -e MYSQL_DATABASE=bankappdb \ -p 3306:3306 mysql:8.0 --default-authentication-plugin=mysql_native_password ./mvnw clean testNo schema-strategy change, no new secrets.
mvnwis marked executable (it was committed without the bit).Link to Devin session: https://app.devin.ai/sessions/40c0f70ecdf84cbda9b95eee5e32a754
Requested by: @achalc
Devin Review