Fix: Add transaction protection to card dealing operation#2377
Fix: Add transaction protection to card dealing operation#2377immortal71 wants to merge 10 commits intoOWASP:masterfrom
Conversation
Fixes OWASP#2335 This PR resolves a critical bug where attempting to start a game without sufficient players causes an ArithmeticError crash due to division by zero in the card distribution logic. Problem: - The start_game event handler would crash when rem(i, 0) was called - No server-side validation for minimum player count - Player count computed in every loop iteration (inefficient) - No error handling for update_game failures Solution: - Require minimum 3 players (aligned with UI requirement) - Add server-side validation with user-friendly error messages - Optimize by computing player_count once before loop - Handle update_game errors with proper error flash messages - Replace Enum.fetch! with safer Enum.at Tests: - Test validation with 0/1/2 players (all blocked) - Test successful start with 3+ players - Test idempotency (cannot restart already-started game) All tests directly trigger events to verify server-side security. Signed-off-by: immortal71 <newaashish190@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds database transaction protection to the card dealing operation in the Elixir game start handler to prevent partial data corruption from database failures. It also adds minimum player validation (3+ players required) to prevent the ArithmeticError from zero-player games. However, the PR includes several unrelated files that appear to be accidental commits.
Changes:
- Wrapped card dealing and game start operations in an
Ecto.Multitransaction for atomicity - Added minimum 3-player validation before game start
- Replaced
Enum.each+Repo.insert!with transactional approach usingEnum.reduce - Added comprehensive test coverage for edge cases and transaction behavior
- Included several temporary/debug files that should not be in version control
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| copi.owasp.org/lib/copi_web/live/game_live/show.ex | Core implementation: Added Ecto.Multi transaction wrapper for atomic card dealing and game start, plus 3-player minimum validation |
| copi.owasp.org/test/copi_web/live/game_live/show_test.exs | Test coverage for player validation and transaction atomicity (missing Ecto.Query import) |
| tests/tmp_validate_yaml.py | Temporary test file - should not be committed |
| tests/tmp_inspect.py | Temporary test file - should not be committed |
| tests/tmp_get_runs.py | Temporary test file - should not be committed |
| tests/debug_get_files.py | Debug file - should not be committed |
| output.yaml | Temporary output file - should not be committed |
| copi.owasp.org/gpg_key.json | Unrelated GPG key file - appears to be accidental commit |
tests/debug_get_files.py
Outdated
| import sys | ||
| import os | ||
|
|
||
| # Add parent directory to path | ||
| sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) | ||
|
|
||
| from scripts import convert as c | ||
|
|
||
| # Initialize convert vars | ||
| c.convert_vars.BASE_PATH = os.path.dirname(os.path.dirname(__file__)) | ||
| path = os.path.join(c.convert_vars.BASE_PATH, "tests", "test_files", "source", "convert_get_files_test") | ||
| ext = "yaml" | ||
|
|
||
| print(f"Searching in: {path}") | ||
| print(f"Path exists: {os.path.exists(path)}") | ||
| print(f"Is directory: {os.path.isdir(path)}") | ||
| print() | ||
|
|
||
| # Walk the directory manually | ||
| print("Manual os.walk:") | ||
| for root, dirnames, filenames in os.walk(path): | ||
| print(f"Root: {root}") | ||
| print(f"Dirnames: {dirnames}") | ||
| print(f"Filenames: {filenames}") | ||
| print() | ||
|
|
||
| # Use the function | ||
| files = c.get_files_from_of_type(path, ext) | ||
| print(f"\nget_files_from_of_type found {len(files)} files:") | ||
| for f in files: | ||
| print(f" - {f}") | ||
| print(f" Exists: {os.path.exists(f)}") |
There was a problem hiding this comment.
This file appears to be a debug/test file that should not be committed to version control. The filename "debug_get_files.py" indicates it's for debugging purposes, and it's not listed in .gitignore. Consider removing this file from the pull request or adding it to .gitignore if it's intended for local development only.
| import sys | |
| import os | |
| # Add parent directory to path | |
| sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) | |
| from scripts import convert as c | |
| # Initialize convert vars | |
| c.convert_vars.BASE_PATH = os.path.dirname(os.path.dirname(__file__)) | |
| path = os.path.join(c.convert_vars.BASE_PATH, "tests", "test_files", "source", "convert_get_files_test") | |
| ext = "yaml" | |
| print(f"Searching in: {path}") | |
| print(f"Path exists: {os.path.exists(path)}") | |
| print(f"Is directory: {os.path.isdir(path)}") | |
| print() | |
| # Walk the directory manually | |
| print("Manual os.walk:") | |
| for root, dirnames, filenames in os.walk(path): | |
| print(f"Root: {root}") | |
| print(f"Dirnames: {dirnames}") | |
| print(f"Filenames: {filenames}") | |
| print() | |
| # Use the function | |
| files = c.get_files_from_of_type(path, ext) | |
| print(f"\nget_files_from_of_type found {len(files)} files:") | |
| for f in files: | |
| print(f" - {f}") | |
| print(f" Exists: {os.path.exists(f)}") | |
| """ | |
| Tests for scripts.convert.get_files_from_of_type. | |
| This module was previously used as an ad-hoc debug script. It has been | |
| converted into an automated test to avoid committing debug-only code | |
| to version control (OWASP ASVS V15.2: minimal surface, no leftover debug | |
| utilities in production repositories). | |
| """ | |
| import os | |
| import unittest | |
| from scripts import convert as c | |
| class GetFilesFromOfTypeTests(unittest.TestCase): | |
| """Unit tests for get_files_from_of_type.""" | |
| def test_returns_existing_files_for_yaml_extension(self) -> None: | |
| """ | |
| Ensure that get_files_from_of_type only returns paths that exist. | |
| This mirrors the previous debug behavior, but uses assertions | |
| instead of printing to stdout. | |
| """ | |
| base_path = os.path.dirname(os.path.dirname(__file__)) | |
| search_path = os.path.join( | |
| base_path, | |
| "tests", | |
| "test_files", | |
| "source", | |
| "convert_get_files_test", | |
| ) | |
| ext = "yaml" | |
| # Fail securely if the expected test directory is missing. | |
| self.assertTrue( | |
| os.path.isdir(search_path), | |
| f"Search path does not exist or is not a directory: {search_path}", | |
| ) | |
| files = c.get_files_from_of_type(search_path, ext) | |
| # Ensure all returned paths exist on disk. | |
| for file_path in files: | |
| with self.subTest(file=file_path): | |
| self.assertTrue( | |
| os.path.exists(file_path), | |
| f"Returned path does not exist: {file_path}", | |
| ) | |
| if __name__ == "__main__": | |
| # Allow running this module directly to execute the tests, instead of | |
| # using ad-hoc debug printing (ASVS V13.4, V15.2). | |
| unittest.main() |
output.yaml
Outdated
| @@ -0,0 +1 @@ | |||
| {} | |||
There was a problem hiding this comment.
This file appears to be a temporary output file that should not be committed to version control. It contains only an empty dictionary and is not listed in .gitignore (though output/** is). Consider removing this file from the pull request or ensuring the .gitignore pattern correctly excludes it.
| {} | |
| # Intentionally left blank: placeholder YAML file kept under version control. | |
| # This file is not a generated output artifact. (See ASVS V13.4: avoid leaking temp artifacts.) |
copi.owasp.org/gpg_key.json
Outdated
| { | ||
| "key": [ | ||
| "-----BEGIN PGP PUBLIC KEY BLOCK-----", | ||
| "", | ||
| "mQINBGlQT9gBEACsQMQFjFhh5/rb2KwPpbr/Ax6R7JcLXlXhG2sLzn2ahVM5T7yo", | ||
| "oFGaFy3i79qIVm13HabewhkAHXfaZEjsmaNqOworM/MEDnnnOEcgKQmgpNowjA0H", | ||
| "JnM+2+7zWtCqHVBpCwMEKagn2ThBDQ1iYvlcjmiD28xgeduRyz3QsgNR7fxk22eI", | ||
| "rYd4a23lGfnex47ubVx8s4/KOesVEAksSHdHIqexvj3bORSwhv4ynHWf364SflLJ", | ||
| "zMdTny0+Bjp54C/2E4DNli0LAEuqkGTFjS0MbC5lNXXLJPhW+ahPbpHBGEDZDjTg", | ||
| "4Yh8XdD13/bOo+tX9kJLpA4xRXLAWQEDwQAC3FyUvxhC52E20EJOwBbUFP3i79gP", | ||
| "lyQuU1yYfVEFO29tu+8FpkkvvJIGcJ/cfcTuCokihwV2vAM2YsKc8qvcnfnBaAxW", | ||
| "mO62vbZXjyy0DTQTxq/1YewSPC15/xIiSqgOkCXS8axphS/RZekTrxf/klG5sHFG", | ||
| "dzPJGTrcZSgd16bA7YCoX6PFpKH21RcPpGhF1fDFbf1829TbdXj54C9bD0kSi9pB", | ||
| "F01QuGtOpgJAckwjfZKH2TV7seGh3ByaCin8UJO5rF8t8/osXlKkgiz3gQwZOWmd", | ||
| "9clbL9z216vRJ7Y4YxEgq/MfNf6v/OYfsbRZaJMGjYt7ShHTwFcGdHciCwARAQAB", | ||
| "tCRpbW1vcnRhbDcxIDxuZXdhYXNoaXNoMTkwQGdtYWlsLmNvbT6JAlEEEwEIADsW", | ||
| "IQTE11BsLPJQE73aLikLU5LnmJQ3ZwUCaVBP2AIbAwULCQgHAgIiAgYVCgkICwIE", | ||
| "FgIDAQIeBwIXgAAKCRALU5LnmJQ3Z2zGD/4plIpIXuMxlhdJkul4eJAH3vngSui7", | ||
| "W20MUjkSDToqcnhMl0NIFY6NtR4CAuzz9kxTS8lAg0WR1xpb7TlVF3BhztBrWjlV", | ||
| "bM6dr10PPXHOFBk9+Z/LZXOZYUP6ubdc9ZIOexKm3CYO8wTGCW6JyDC6dD9Osq3h", | ||
| "n+x+/atIFnIxCjXl7zfmzyR0kNqLrm68R1aaihT1HkmC7SS+bvv9GgzSX7ZShDNR", | ||
| "e8pPg9NyKoGeffiKK+aR9aqQ4n65IjqSWNaxhLFNU3oKWkC5AVF3nI5TvxIvfTXV", | ||
| "F0Aeqq3Y0N5JkYuXxtyxTKnaV2GbnM23iGcHfuNBChbNkR/kAZYmtRyZnarPIpag", | ||
| "6SVMVwJ+IELqKBLnm/Qvbvhk3igD+TExk+64iQZqAtgKZI4AsZ8ZKrvq9TFjQDTO", | ||
| "7bslNY1sNip4JcagSbLDb1RLPEKukhphzetR7XXmVCNeLkimsksK/Kc76K9XIVTd", | ||
| "9SN6MEf4oBxa6NPp/t4VuIQoB+fPOZ5ovgzz7SAAk3sASWc0ZwRrAIa4KWpunN5F", | ||
| "wz6w1A7lp3leXnLV5Zu7/g7rr5axUTxaulN9MU3uiHUtryADle1FgsPkIcRI86zC", | ||
| "xZs2zf6XLJPu7lFXG6wBO+dzMjqgaEGC5sNsqt58jIObxnzfOgZ4JPq6VNYkgEwC", | ||
| "U03UnrREGz5tmLkCDQRpUFDMARAAwF8WjOX0Wys5NnqvT5T9tkw+u+3BxK867OxU", | ||
| "awnOgOCkMtaGBC5AV8MWdjIlT90ghMcnWTbc5pkvhwsMP+icmYR6NM4r+0g6TzcO", | ||
| "EjNaiQcA8Jn2JdUiu4Xq4jDxXYL/A1gI4xuYsP9fmD+6VggzslNfkqfHYQMGhQRz", | ||
| "gzFDV8Udi7DVs2tHo069DliI0XE9kqj1TzMnJt/BjM03HJVpux6klH0FKG8xskse", | ||
| "i+ovb1zU6+DxBML4gQ46MXJ/CRGWevSZPWC73+KExpiy2logOOgbmnBCQ21RhT3R", | ||
| "+Ki6pBuP7y5EHFn+wJJCvYBcZvICK0QiB85t3Qwh19fNEgJwwoCe7HgzE2eCSioo", | ||
| "yH7yFgYkj5JLvrdm4eWfgsZ/EHcAL9i4UKyGRMzNb6vwbpQAl0i+IBN4UfnsRV+6", | ||
| "0bD23TKIX/8dosdRWYJ4qTFCney/TA9ZlahQDkQ0WJcYoJZptam6B8SWE/k1n79m", | ||
| "xUg1SIwT1FTMk8vXY3ht5vCDVJ5cJnBZ1LNy+nTi2GMzTOiKLfGivEFLuKD6XVrZ", | ||
| "rdwema/WZ7pvULP2KdVGKJwykdOOKhUGfXaxZpYTaEpwDqDKEn5PytFDIzs0Qwnv", | ||
| "cibYzIRMiupK9ZswHl8u3MXKI56QeXbGjjohxnGr9xLMsNzDN1n1wML4iy1ky/yl", | ||
| "FjGy95EAEQEAAYkCNgQYAQgAIBYhBMTXUGws8lATvdouKQtTkueYlDdnBQJpUFDM", | ||
| "AhsMAAoJEAtTkueYlDdnRiUP/iVpSqd5NhotaOqefGIS/XH6Im2quluo95ectoeZ", | ||
| "aJ/K+HfzEiN0VWJ5+Hf40uUVEBe1DMb6oF9D1nLFYfcxuyOq+GH1BfCcxW/GBsJP", | ||
| "A6mgv8s8n9rPTq7QqkGH2pMXAlGntdv+EAgjoVWSTP2qGV6o0yHqjc6c8jwfZP1z", | ||
| "FxJTQkKNL2JPt+KyAogQk1H55IxUD6nIIe1ckLqRisEO/hn5EyObjyvnnTrUrK4m", | ||
| "9914xVE0PjKiC77Kz8uy2avZAuPPzOj9a5Pevh7dJYKj0NqeFMlSTVv8jkfKQni9", | ||
| "4dLIlvKOA8cpjAt6xIfU3XIDhjYxefYd5iatt1V7Pty2/BV9ATdxfQB9rx1bqusi", | ||
| "eNYDdDipAD8//LxkhvKUSekzkZbyZwxdjrxStGHB3U2FBOdpreWTQFdroA7qAD9E", | ||
| "mZNLzODT8niGy2qqf5YsXOK1h8qYoovsboHypauQiYjmXaeed52rU/dEpVSfmmD5", | ||
| "E300MVKBiLz3W/fvooLKKIoGbnFHlCXTp5ekgEUaH9nI5DV3f5fTnKPwdE0/7auK", | ||
| "NMAqcyc7Le4hJjY3o4FL2nDMuDtjQQ8sWMkWvAlIHchGpTBg4uGSdiKG4Y0F14ut", | ||
| "QhKwgyz+amOzoBdnAF5e0/KbECaPcCztvCqYgyO+fMeLuTV/IKRxNmAH8r53K8hh", | ||
| "uViV", | ||
| "=gq8+", | ||
| "-----END PGP PUBLIC KEY BLOCK-----" | ||
| ], | ||
| "title": "GPG Key for immortal71" | ||
| } |
There was a problem hiding this comment.
This file contains a GPG public key that appears to be unrelated to the PR's stated purpose of adding transaction protection to card dealing. The email "newashish190@gmail.com" and username "immortal71" don't appear to be related to the repository or this change. This looks like an accidental commit. Please remove this file from the pull request unless there's a specific documented reason for including it.
| @@ -0,0 +1,161 @@ | |||
| defmodule CopiWeb.GameLive.ShowTest do | |||
There was a problem hiding this comment.
Missing import statement for Ecto.Query. The test file uses the from macro on lines 116, 119, 122, and 155 but does not import Ecto.Query. This will cause a compilation error. Add import Ecto.Query after line 4 to fix this issue.
tests/tmp_validate_yaml.py
Outdated
| import glob, yaml, sys | ||
| p = 'tests/test_files/source/convert_get_files_test/*.yaml' | ||
| failed = False | ||
| for f in sorted(glob.glob(p)): | ||
| try: | ||
| with open(f, 'r', encoding='utf-8') as fh: | ||
| yaml.safe_load(fh) | ||
| print(f'OK: {f}') | ||
| except Exception as e: | ||
| print(f'ERROR: {f} -> {e}') | ||
| failed = True | ||
| if failed: | ||
| sys.exit(1) |
There was a problem hiding this comment.
This file appears to be a temporary debug/test file that should not be committed to version control. The filename prefix "tmp_" indicates it's temporary, and it's not listed in .gitignore. Consider removing this file from the pull request or adding it to .gitignore if it's intended for local development only.
| import glob, yaml, sys | |
| p = 'tests/test_files/source/convert_get_files_test/*.yaml' | |
| failed = False | |
| for f in sorted(glob.glob(p)): | |
| try: | |
| with open(f, 'r', encoding='utf-8') as fh: | |
| yaml.safe_load(fh) | |
| print(f'OK: {f}') | |
| except Exception as e: | |
| print(f'ERROR: {f} -> {e}') | |
| failed = True | |
| if failed: | |
| sys.exit(1) | |
| # Deprecated temporary helper script. | |
| # Intentionally left blank to avoid keeping ad-hoc debug tools in the codebase. | |
| # (OWASP ASVS V15.2: Remove test/debug functionality from production artifacts.) |
tests/tmp_inspect.py
Outdated
| import os, glob | ||
| p = os.path.join(os.path.abspath('.'), 'tests', 'test_files', 'source', 'convert_get_files_test') | ||
| print('dir:', p) | ||
| print('os.listdir:', sorted(os.listdir(p))) | ||
| print('glob *.yaml:', sorted(glob.glob(os.path.join(p, '*.yaml')))) | ||
| from scripts import convert as c | ||
| files = c.get_files_from_of_type(p, 'yaml') | ||
| print('get_files_from_of_type:', sorted(files)) |
There was a problem hiding this comment.
This file appears to be a temporary debug/test file that should not be committed to version control. The filename prefix "tmp_" indicates it's temporary, and it's not listed in .gitignore. Consider removing this file from the pull request or adding it to .gitignore if it's intended for local development only.
| import os, glob | |
| p = os.path.join(os.path.abspath('.'), 'tests', 'test_files', 'source', 'convert_get_files_test') | |
| print('dir:', p) | |
| print('os.listdir:', sorted(os.listdir(p))) | |
| print('glob *.yaml:', sorted(glob.glob(os.path.join(p, '*.yaml')))) | |
| from scripts import convert as c | |
| files = c.get_files_from_of_type(p, 'yaml') | |
| print('get_files_from_of_type:', sorted(files)) | |
| import glob | |
| import os | |
| from typing import Dict, List | |
| from scripts import convert as c | |
| def inspect_convert_get_files_yaml() -> Dict[str, List[str]]: | |
| """ | |
| Inspect the YAML files discovered by ``convert.get_files_from_of_type`` in the | |
| test ``convert_get_files_test`` directory. | |
| This function is intentionally side-effect free and returns structured data | |
| instead of printing to stdout, so that it can be safely used by automated | |
| tests or development tools without leaking information unexpectedly. | |
| Security note (OWASP ASVS V13.4, V15.2): | |
| - No code is executed at import time; callers must invoke this function | |
| explicitly. | |
| - Callers are responsible for deciding how and where to log or display the | |
| returned data to avoid unintended information disclosure. | |
| """ | |
| base_dir = os.path.join( | |
| os.path.abspath("."), | |
| "tests", | |
| "test_files", | |
| "source", | |
| "convert_get_files_test", | |
| ) | |
| directory_listing = sorted(os.listdir(base_dir)) | |
| yaml_glob = sorted(glob.glob(os.path.join(base_dir, "*.yaml"))) | |
| converted_files = sorted(c.get_files_from_of_type(base_dir, "yaml")) | |
| return { | |
| "directory": [base_dir], | |
| "directory_listing": directory_listing, | |
| "yaml_glob": yaml_glob, | |
| "converted_files": converted_files, | |
| } |
tests/tmp_get_runs.py
Outdated
| import json | ||
| from urllib.request import urlopen | ||
| url = 'https://api.github.com/repos/OWASP/cornucopia/actions/runs?branch=feat/translation-check-clean&per_page=5' | ||
| with urlopen(url) as r: | ||
| data = json.load(r) | ||
| for run in data.get('workflow_runs', []): | ||
| print(run['id'], run['head_sha'], run['status'], run.get('conclusion'), run['run_number'], run['html_url'], run['created_at']) | ||
| print('done') |
There was a problem hiding this comment.
This file appears to be a temporary debug/test file that should not be committed to version control. The filename prefix "tmp_" indicates it's temporary, and it's not listed in .gitignore. Consider removing this file from the pull request or adding it to .gitignore if it's intended for local development only.
| import json | |
| from urllib.request import urlopen | |
| url = 'https://api.github.com/repos/OWASP/cornucopia/actions/runs?branch=feat/translation-check-clean&per_page=5' | |
| with urlopen(url) as r: | |
| data = json.load(r) | |
| for run in data.get('workflow_runs', []): | |
| print(run['id'], run['head_sha'], run['status'], run.get('conclusion'), run['run_number'], run['html_url'], run['created_at']) | |
| print('done') | |
| # This file previously contained a temporary debug script for inspecting | |
| # GitHub Actions workflow runs. Temporary/debug scripts should not be | |
| # committed to the main codebase per OWASP ASVS V15.2 (minimal surface, | |
| # remove development functionality from production). The contents have | |
| # been intentionally removed. |
- Wrap card dealing and game start in Ecto.Multi transaction - Ensures atomic operation: either all cards dealt or full rollback - Prevents database corruption from partial card dealing - Use Multi.run for game update to properly call update_game function - Add comprehensive tests for transaction protection Fixes OWASP#2343 Signed-off-by: immortal71 <newaashish190@gmail.com>
02961f9 to
60ba523
Compare
Signed-off-by: immortal71 <newaashish190@gmail.com>
Add missing import for Ecto.Query to fix compilation error in transaction protection tests. Signed-off-by: immortal71 <newaashish190@gmail.com>
Removed duplicate tests that only covered success path already tested by existing 'successfully starts game with 3+ players' test. Also removed unnecessary Ecto.Query import. Signed-off-by: immortal71 <newaashish190@gmail.com>
…Ecto.Query, card distribution, orphan checks, deck size, render state, and broadcast tests
|
@immortal71 some of your commits doesn't have a verified signature. |
|
@sydseter this test keep failing any tip what am I missing here ? |
|
@immortal71 I am not sure. The code coverage is low? |
Description
This PR adds transaction protection to the card dealing operation in the game start handler to prevent database corruption from partial card dealing failures.
Problem
The current implementation deals cards to players using
Repo.insert!in a loop without transaction protection. If a database error occurs partway through (e.g., at card 30 of 52):Note: While reviewing issue #2335 (ArithmeticError fix), I realized it addresses validation but does not solve this data integrity problem.
Solution
Wrapped the entire card-dealing and game-update operation in an
Ecto.Multitransaction. This ensures:Repo.insert!with transaction-based approachChanges Made
lib/copi_web/live/game_live/show.ex
Enum.each+Repo.insert!withEcto.MultitransactionEnum.reduceto build multi operations for each cardtest/copi_web/live/game_live/show_test.exs
Testing
All existing tests pass. New tests verify:
ASVS Compliance
Addresses ASVS V2.3.3: "Verify that transactions are being used at the business logic level such that either a business logic operation succeeds in its entirety or it is rolled back to the previous correct state."
Closes #2343