security/phase1_foundation/SEC-P1-01 #5
Conversation
add comprehensive security documentation including architecture, data flow, and incident response plans
A beginning set of tests to drive the phase1 requirements for TDD use. Git: NA
Modified security configuration to have roles considered for API user access. Made modifications to code to get the first set of phase1 tdd tests to run. Also had to make some changes to the tests since I am using the Oauth package to how to mock out the users and roles for the tests. Git: NA
Created TokenBucketService that will be the go-between for token buckets that are going to be used for rate limiting. Started with a prototype and then converted into bucket4j. Git: NA
Added preliminary tests to prove out a design. Added some expected classes for rate limiting to allow for compile but not impl yet. Updated to latest spring boot web version. Git: NA
…o_security_standards' into security/phase1_foundation # Conflicts: # .idea/.gitignore # docs/security/SECURITY_IMPLEMENTATION_PLAN.md
Feature adds HSTS transport security. Closes: SEC-P1-01
Feature adds HSTS transport security. Closes: SEC-P1-01
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| .anyRequest().authenticated()) | ||
| .oauth2ResourceServer(oauth2 -> oauth2.jwt( | ||
| Customizer.withDefaults())); | ||
| .requestMatchers("/actuator/health", "/actuator/health/**", "/actuator/info", "/actuator/**").hasRole("ADMIN") |
There was a problem hiding this comment.
Bug: Actuator Health: Authentication Breaks Expected Access
The actuator endpoints now require ADMIN role, but the comment still says "Permits unauthenticated access to basic Actuator endpoints for health/info". This creates a critical security misconfiguration where health checks requiring authentication will break monitoring systems and load balancers that expect unauthenticated access to /actuator/health.
| } | ||
| resp.setHeader(SecurityConfig.STRICT_TRANSPORT_SECURITY_HEADER, value.toString()); | ||
| } else if (resp.getHeader(SecurityConfig.STRICT_TRANSPORT_SECURITY_HEADER) != null) { | ||
| resp.setHeader(SecurityConfig.STRICT_TRANSPORT_SECURITY_HEADER, ""); |
There was a problem hiding this comment.
Bug: Header Null: An Unnecessary Risk
Setting a header to empty string and then to null is redundant and incorrect. The setHeader method with null value may throw NullPointerException or have undefined behavior depending on the servlet container. To remove a header, only one call is needed, and passing null is not the correct approach.
|
|
||
| public RateLimitResult tryConsume(String key) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Bug: Rate Limiter Null Reference Causes App Crash
The tryConsume method returns null, which will cause NullPointerException when the rate limit filter attempts to call methods like isAllowed() on the result. This stub implementation is incomplete and will crash the application if rate limiting is enabled.
|
|
||
| public RateLimitFilter(RateLimitService service) { | ||
|
|
||
| } |
There was a problem hiding this comment.
Bug: Uninitialized service field causes null pointer.
The constructor accepts a RateLimitService parameter but never assigns it to the rateLimitService field. This leaves the field uninitialized as null, which will cause NullPointerException when doFilterInternal attempts to use it.
| username: postgres | ||
| password: postgres | ||
| username: ${DB_USERNAME:postgres} | ||
| password: ${DB_PASSWORD:} |
There was a problem hiding this comment.
Bug: Database Password Default: Security Vulnerability
The database password defaults to an empty string when DB_PASSWORD environment variable is not set. This allows the application to connect to PostgreSQL with no password, which is a security vulnerability. The application should fail to start if the password is not provided rather than defaulting to an insecure empty value.
| resp.setHeader(SecurityConfig.STRICT_TRANSPORT_SECURITY_HEADER, ""); | ||
| resp.setHeader(SecurityConfig.STRICT_TRANSPORT_SECURITY_HEADER, null); | ||
| } | ||
| }, org.springframework.security.web.access.intercept.AuthorizationFilter.class); |
There was a problem hiding this comment.
Bug: HSTS Header Security Gap
The HSTS filter calls chain.doFilter() before adding the header, but the filter is added after AuthorizationFilter. If authorization fails and returns a 403/401 response, the filter chain may not complete normally, potentially preventing the HSTS header from being added to error responses. The header should be set before calling chain.doFilter() or the filter should be positioned differently.
Add HSTS header.
Added/refactored basic security standards.
Note
Adds configurable HSTS and stricter RBAC, introduces partial-update validation, scaffolds rate limiting with Bucket4j and extensive tests, and updates security docs/config.
Strict-Transport-SecurityviaSecurityConfigwithHstsProperties; adds prod/dev/insecure request tests."/actuator/**"toROLE_ADMIN, protects API routes, permits public/docs; adds@PreAuthorizeon controllers; conditional OAuth2 resource server.@AtLeastOneFieldNotNulland new*UpdateRequestDTOs for partial updates.bucket4j-coredependency; introduces prototype services/filter (TokenBucketService,RateLimitService,RateLimitFilter) with comprehensive unit/integration tests.application.ymlusing env vars.docs/security/SECURITY_HARDENING_PLAN.mdandSECURITY_TRACKING.mdfor phased hardening and tracking.Written by Cursor Bugbot for commit 039972a. This will update automatically on new commits. Configure here.