Address PR review comments: security hardening, code quality, and doc accuracy#308
Conversation
…e docs Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses prior review feedback across the Spring Boot server, Docker/CI configuration, and the Pygame client by hardening security defaults, improving code quality, and correcting documentation.
Changes:
- Harden deployment defaults (require explicit Postgres password, tighten CORS defaults, strengthen JWT default-secret guard).
- Improve API correctness and code quality (HTTP status mapping in auth endpoints, spec-compliant
@PreDestroy, client config field renames, deterministic DeerMeat energy value, fix client entrypoint script). - Reduce CI duplication and update docs to reflect implemented security features and current client UI.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/screen/serverBackedWorldScreen.py |
Updates WebSocket config attribute names to camelCase. |
src/run_client.sh |
Fixes client entrypoint to roam.py. |
src/entity/deerMeat.py |
Replaces random energy restore with a fixed authoritative constant. |
src/config/config.py |
Renames WebSocket config fields to camelCase for consistency. |
src/CLIENT_README.md |
Removes “stats” screen from documented feature list. |
server/src/main/java/com/preponderous/roam/service/AutoSaveService.java |
Makes shutdown hook @PreDestroy spec-compliant and keeps a testable save method. |
server/src/main/java/com/preponderous/roam/security/JwtUtils.java |
Expands default-secret guard to treat postgresql as production-like. |
server/src/main/java/com/preponderous/roam/controller/AuthController.java |
Switches from rethrowing RuntimeException to ResponseStatusException for more accurate HTTP codes. |
docs/SECURITY_SUMMARY.md |
Moves implemented security/persistence items out of “Future Enhancements”. |
compose.yml |
Removes weak default DB password; requires explicit POSTGRES_PASSWORD. |
Dockerfile |
Removes server’s own origin from default ALLOWED_ORIGINS. |
.github/workflows/server-tests.yml |
Adjusts triggers to avoid duplicate runs (push + PR). |
.github/workflows/python-syntax-check.yml |
Adjusts triggers to avoid duplicate runs (push + PR). |
Comments suppressed due to low confidence (1)
server/src/main/java/com/preponderous/roam/controller/AuthController.java:59
- Same issue as login/register: catching all
RuntimeExceptionand responding with 401 will mask unexpected server errors (e.g., repository failures when loading refresh tokens) as "unauthorized". Consider mapping only expected refresh-token validation failures to 401 and letting other exceptions propagate as 5xx via the global exception handler.
@PostMapping("/refresh")
public ResponseEntity<AuthResponse> refreshToken(@Valid @RequestBody RefreshTokenRequest request) {
try {
AuthResponse response = authService.refreshToken(request);
return ResponseEntity.ok(response);
} catch (RuntimeException e) {
throw new ResponseStatusException(HttpStatus.UNAUTHORIZED, "Invalid or expired refresh token");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Enforce secure configuration: do not allow default secret in non-development profiles | ||
| if (jwtSecret.equals(DEFAULT_SECRET)) { | ||
| if (activeProfiles != null && activeProfiles.contains("production")) { | ||
| boolean isProductionLike = activeProfiles != null && | ||
| (activeProfiles.contains("production") || activeProfiles.contains("postgresql")); | ||
| if (isProductionLike) { | ||
| throw new IllegalStateException( | ||
| "Insecure configuration: default JWT secret is being used while 'production' profile is active. " + | ||
| "Please configure a strong 'jwt.secret' value for production."); | ||
| "Insecure configuration: default JWT secret is being used in a production-like profile (" + | ||
| activeProfiles + "). Please configure a strong 'jwt.secret' value."); | ||
| } | ||
| logger.warn("WARNING: Using default JWT secret. This is insecure! Set JWT_SECRET environment variable in production."); | ||
| } |
There was a problem hiding this comment.
The default-secret guard is likely not triggering because jwtSecret is compared only against DEFAULT_SECRET, but jwt.secret is already set in application.yml with a different default fallback string. As a result, deployments that rely on the YAML default may silently use an insecure secret without warning or failure. Consider aligning the YAML default with DEFAULT_SECRET, removing the YAML default so @Value's fallback is used, or extending the check to treat the YAML fallback value as insecure as well.
| @@ -34,8 +35,7 @@ public ResponseEntity<AuthResponse> register(@Valid @RequestBody RegisterRequest | |||
| AuthResponse response = authService.register(request); | |||
| return new ResponseEntity<>(response, HttpStatus.CREATED); | |||
| } catch (RuntimeException e) { | |||
| // Prevent leaking internal error details | |||
| throw new RuntimeException("Registration failed. Please check your input and try again."); | |||
| throw new ResponseStatusException(HttpStatus.CONFLICT, "Registration failed. Please check your input and try again."); | |||
| } | |||
There was a problem hiding this comment.
Catching RuntimeException here will also convert unexpected server-side failures (e.g., DB outages, encoder/auth manager failures) into a 409 Conflict, which can mask real 5xx errors and make troubleshooting harder. It also no longer matches the existing AuthControllerIntegrationTest expectations (currently asserts 5xx for duplicate username). Consider catching and mapping only the specific expected conflict cases (e.g., a dedicated exception type for duplicates) and letting other exceptions propagate to the global 500 handler; update the integration tests to assert the new 409 behavior.
| // Generic error message for security | ||
| throw new RuntimeException("Invalid username or password"); | ||
| throw new ResponseStatusException(HttpStatus.UNAUTHORIZED, "Invalid username or password"); | ||
| } |
There was a problem hiding this comment.
Catching RuntimeException broadly and always returning 401 can incorrectly classify non-auth failures (e.g., persistence errors when fetching the user, infrastructure issues during authentication) as "unauthorized". Prefer catching the specific authentication/validation exceptions you expect (e.g., BadCredentialsException / token-validation exceptions) and allowing unexpected exceptions to surface as 5xx via the global handler; update the existing integration tests that currently expect 5xx for invalid credentials to assert 401 once the exception mapping is narrowed.
|
@copilot address comments |
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Addressed all three review comments in commit
|
src/run_client.sh, config camelCase, DeerMeat energy, CI triggers, docs, CORS, compose.yml, JwtUtils postgresql check, AutoSaveService @PreDestroyJwtUtils.java/application.yml: align YAML fallback withDEFAULT_SECRETconstant so the guard actually triggersUserAlreadyExistsExceptionfor specific conflict handlingUserAlreadyExistsExceptionhandler toGlobalExceptionHandler(409 Conflict)AuthServiceto throwUserAlreadyExistsExceptionfor duplicate usersAuthController: remove broadRuntimeExceptioncatches fromregister()andlogin(); let specific exceptions propagate to global handlerAuthControllerIntegrationTest: fix expected HTTP status codes (5xx → 409/401)🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.