diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 211c766..43f8faf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,6 +20,16 @@ jobs: cache: maven - name: Run tests run: mvn -B -ntp test + - name: Build backend image for scan + run: docker build -t transaction-api:${{ github.sha }} -f Dockerfile . + - name: Scan backend image + uses: aquasecurity/trivy-action@v0.36.0 + with: + image-ref: transaction-api:${{ github.sha }} + vuln-type: os,library + severity: CRITICAL + ignore-unfixed: true + exit-code: "1" deploy-dev: needs: test diff --git a/DEV.md b/DEV.md index 3d5b375..584ea6a 100644 --- a/DEV.md +++ b/DEV.md @@ -119,6 +119,8 @@ Required: Optional: - `APP_SECURITY_ADMIN_EMAILS` (comma-separated admin allowlist) - `APP_SECURITY_JWT_DYNAMO_MAX_STALE=PT72H` +- `APP_RATE_LIMIT_TRUST_FORWARDED_HEADERS=false` +- `APP_RATE_LIMIT_MAX_BUCKETS=10000` - `APP_SESSION_TIMEOUT=PT2H` - `APP_SESSION_COOKIE_SECURE=true` - `APP_SESSION_COOKIE_SAME_SITE=lax` diff --git a/pom.xml b/pom.xml index f70311f..e594b0b 100644 --- a/pom.xml +++ b/pom.xml @@ -10,7 +10,7 @@ Spring Boot service for recording financial transactions 21 - 3.5.0 + 3.5.15 2.41.17 diff --git a/src/main/java/com/transactionapi/controller/AuthController.java b/src/main/java/com/transactionapi/controller/AuthController.java index 469d75b..5ecbaab 100644 --- a/src/main/java/com/transactionapi/controller/AuthController.java +++ b/src/main/java/com/transactionapi/controller/AuthController.java @@ -104,7 +104,7 @@ public UserProfileResponse login( SecurityContextHolder.setContext(context); securityContextRepository.saveContext(context, request, response); - return UserProfileResponse.from(user); + return UserProfileResponse.from(user, userIdResolver.isAdmin(jwtAuthentication)); } @PostMapping("/logout") diff --git a/src/main/java/com/transactionapi/controller/UserController.java b/src/main/java/com/transactionapi/controller/UserController.java index b50a2a9..410280a 100644 --- a/src/main/java/com/transactionapi/controller/UserController.java +++ b/src/main/java/com/transactionapi/controller/UserController.java @@ -2,6 +2,7 @@ import com.transactionapi.constants.ApiPaths; import com.transactionapi.dto.UserProfileResponse; +import com.transactionapi.model.User; import com.transactionapi.security.UserIdResolver; import com.transactionapi.service.UserService; import org.springframework.security.core.Authentication; @@ -26,13 +27,15 @@ public UserController(UserIdResolver userIdResolver, UserService userService) { public UserProfileResponse getProfile(Authentication authentication) { String userId = userIdResolver.requireUserId(authentication); String email = userIdResolver.resolveEmail(authentication); - return UserProfileResponse.from(userService.getOrCreateUser(userId, email)); + User user = userService.getOrCreateUser(userId, email); + return UserProfileResponse.from(user, userIdResolver.isAdmin(authentication)); } @PostMapping("/legal-agreement") public UserProfileResponse acceptLegalAgreement(Authentication authentication) { String userId = userIdResolver.requireUserId(authentication); String email = userIdResolver.resolveEmail(authentication); - return UserProfileResponse.from(userService.acceptLegalAgreement(userId, email)); + User user = userService.acceptLegalAgreement(userId, email); + return UserProfileResponse.from(user, userIdResolver.isAdmin(authentication)); } } diff --git a/src/main/java/com/transactionapi/dto/UserProfileResponse.java b/src/main/java/com/transactionapi/dto/UserProfileResponse.java index 4dc849f..b9c7779 100644 --- a/src/main/java/com/transactionapi/dto/UserProfileResponse.java +++ b/src/main/java/com/transactionapi/dto/UserProfileResponse.java @@ -16,6 +16,7 @@ public record UserProfileResponse( UUID id, String authId, String email, + boolean admin, boolean premium, Instant createdAt, Instant updatedAt, @@ -32,6 +33,10 @@ public record UserProfileResponse( Instant privacyPolicyAcceptedAt ) { public static UserProfileResponse from(User user) { + return from(user, false); + } + + public static UserProfileResponse from(User user, boolean admin) { ThemeMode themeMode = user.getThemeMode() != null ? user.getThemeMode() : ThemeMode.LIGHT; PnlDisplayMode pnlDisplayMode = user.getPnlDisplayMode() != null ? user.getPnlDisplayMode() : PnlDisplayMode.PNL; TradeSortField defaultTradeSortBy = user.getDefaultTradeSortBy() != null @@ -44,6 +49,7 @@ public static UserProfileResponse from(User user) { user.getId(), user.getAuthId(), user.getEmail(), + admin, user.isPremium(), user.getCreatedAt(), user.getUpdatedAt(), diff --git a/src/main/java/com/transactionapi/security/RateLimiterService.java b/src/main/java/com/transactionapi/security/RateLimiterService.java index d53feab..20a52e4 100644 --- a/src/main/java/com/transactionapi/security/RateLimiterService.java +++ b/src/main/java/com/transactionapi/security/RateLimiterService.java @@ -12,16 +12,19 @@ public class RateLimiterService { private final int maxRequests; private final int maxPublicShareRequests; private final long windowMillis; + private final int maxBuckets; private final Map> buckets = new ConcurrentHashMap<>(); public RateLimiterService( @Value("${app.rate-limit.per-minute:100}") int maxRequests, @Value("${app.rate-limit.public-share-per-minute:20}") int maxPublicShareRequests, - @Value("${app.rate-limit.window-ms:60000}") long windowMillis + @Value("${app.rate-limit.window-ms:60000}") long windowMillis, + @Value("${app.rate-limit.max-buckets:10000}") int maxBuckets ) { this.maxRequests = maxRequests; this.maxPublicShareRequests = maxPublicShareRequests; this.windowMillis = windowMillis; + this.maxBuckets = Math.max(1, maxBuckets); } public boolean allow(String userId) { @@ -36,7 +39,14 @@ private boolean allowWithLimit(String key, int limit) { long now = System.currentTimeMillis(); long cutoff = now - windowMillis; - ConcurrentLinkedDeque deque = buckets.computeIfAbsent(key, k -> new ConcurrentLinkedDeque<>()); + ConcurrentLinkedDeque deque = buckets.get(key); + if (deque == null) { + cleanupExpiredBuckets(cutoff); + if (buckets.size() >= maxBuckets) { + return false; + } + deque = buckets.computeIfAbsent(key, k -> new ConcurrentLinkedDeque<>()); + } while (!deque.isEmpty() && deque.peekFirst() < cutoff) { deque.pollFirst(); @@ -49,4 +59,15 @@ private boolean allowWithLimit(String key, int limit) { deque.addLast(now); return true; } + + private void cleanupExpiredBuckets(long cutoff) { + buckets.forEach((bucketKey, deque) -> { + while (!deque.isEmpty() && deque.peekFirst() < cutoff) { + deque.pollFirst(); + } + if (deque.isEmpty()) { + buckets.remove(bucketKey, deque); + } + }); + } } diff --git a/src/main/java/com/transactionapi/security/RateLimitingFilter.java b/src/main/java/com/transactionapi/security/RateLimitingFilter.java index 0208d00..09606ad 100644 --- a/src/main/java/com/transactionapi/security/RateLimitingFilter.java +++ b/src/main/java/com/transactionapi/security/RateLimitingFilter.java @@ -6,6 +6,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; +import org.springframework.beans.factory.annotation.Value; import org.springframework.http.HttpStatus; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; @@ -18,6 +19,9 @@ public class RateLimitingFilter extends OncePerRequestFilter { private final RateLimiterService rateLimiterService; private final UserIdResolver userIdResolver; + @Value("${app.rate-limit.trust-forwarded-headers:false}") + private boolean trustForwardedHeaders; + public RateLimitingFilter(RateLimiterService rateLimiterService, UserIdResolver userIdResolver) { this.rateLimiterService = rateLimiterService; this.userIdResolver = userIdResolver; @@ -70,6 +74,10 @@ protected void doFilterInternal( } private String getClientIp(HttpServletRequest request) { + if (!trustForwardedHeaders) { + return request.getRemoteAddr() != null ? request.getRemoteAddr() : "unknown"; + } + String ip = request.getHeader("X-Forwarded-For"); if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { ip = request.getHeader("X-Real-IP"); diff --git a/src/main/java/com/transactionapi/security/UserIdResolver.java b/src/main/java/com/transactionapi/security/UserIdResolver.java index 2ae4a70..e35f2ff 100644 --- a/src/main/java/com/transactionapi/security/UserIdResolver.java +++ b/src/main/java/com/transactionapi/security/UserIdResolver.java @@ -80,6 +80,14 @@ public void requireAdmin(Authentication authentication) { } } + public boolean isAdmin(Authentication authentication) { + if (adminEmailSet.isEmpty()) { + return false; + } + String email = resolveEmail(authentication); + return StringUtils.hasText(email) && adminEmailSet.contains(email.toLowerCase()); + } + private void enforceAllowedEmails(Authentication authentication) { if (allowedEmailSet.isEmpty()) { return; diff --git a/src/main/java/com/transactionapi/service/ShareLinkService.java b/src/main/java/com/transactionapi/service/ShareLinkService.java index 295262b..a0e8cd1 100644 --- a/src/main/java/com/transactionapi/service/ShareLinkService.java +++ b/src/main/java/com/transactionapi/service/ShareLinkService.java @@ -68,7 +68,7 @@ public Optional getShareLink(String code, String requestingUserId) { if (!link.isRequiresAuth()) { return true; } - return requestingUserId != null && !requestingUserId.isBlank(); + return link.getUserId().equals(requestingUserId); }) .map(link -> { link.setAccessCount(link.getAccessCount() + 1); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 1ffbb23..08c71b6 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -38,3 +38,5 @@ app.security.jwt.dynamo.max-stale=PT72H app.security.allow-header-auth=false app.security.header-name=X-User-Id app.security.dev-user-id= +app.rate-limit.trust-forwarded-headers=${APP_RATE_LIMIT_TRUST_FORWARDED_HEADERS:false} +app.rate-limit.max-buckets=${APP_RATE_LIMIT_MAX_BUCKETS:10000} diff --git a/src/test/java/com/transactionapi/controller/AuthControllerTest.java b/src/test/java/com/transactionapi/controller/AuthControllerTest.java index a3df7e1..fcea0eb 100644 --- a/src/test/java/com/transactionapi/controller/AuthControllerTest.java +++ b/src/test/java/com/transactionapi/controller/AuthControllerTest.java @@ -29,7 +29,7 @@ import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; -@SpringBootTest +@SpringBootTest(properties = "app.security.admin-emails=admin@example.com") @AutoConfigureMockMvc @ActiveProfiles("test") class AuthControllerTest { @@ -66,6 +66,7 @@ void loginCreatesSessionCookieAndAuthenticatesSubsequentRequests() throws Except .andExpect(status().isOk()) .andExpect(jsonPath("$.authId").value("google-sub-1")) .andExpect(jsonPath("$.email").value("user@example.com")) + .andExpect(jsonPath("$.admin").value(false)) .andExpect(jsonPath("$.termsAcceptedAt").doesNotExist()) .andExpect(jsonPath("$.privacyPolicyAcceptedAt").doesNotExist()) .andReturn(); @@ -75,7 +76,42 @@ void loginCreatesSessionCookieAndAuthenticatesSubsequentRequests() throws Except mockMvc.perform(get(ApiPaths.USER_ME).session(session)) .andExpect(status().isOk()) .andExpect(jsonPath("$.authId").value("google-sub-1")) - .andExpect(jsonPath("$.email").value("user@example.com")); + .andExpect(jsonPath("$.email").value("user@example.com")) + .andExpect(jsonPath("$.admin").value(false)); + } + + @Test + void configuredAdminGetsAdminProfileOnLoginProfileAndLegalAcceptance() throws Exception { + Jwt jwt = Jwt.withTokenValue("google-id-token") + .header("alg", "RS256") + .subject("google-sub-admin") + .claim("email", "admin@example.com") + .claim("name", "Admin User") + .build(); + when(jwtDecoder.decode("google-id-token")).thenReturn(jwt); + + MvcResult loginResult = loginWithCsrf("google-id-token") + .andExpect(status().isOk()) + .andExpect(jsonPath("$.authId").value("google-sub-admin")) + .andExpect(jsonPath("$.email").value("admin@example.com")) + .andExpect(jsonPath("$.admin").value(true)) + .andReturn(); + + MockHttpSession session = (MockHttpSession) loginResult.getRequest().getSession(false); + + mockMvc.perform(get(ApiPaths.USER_ME).session(session)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.admin").value(true)); + + mockMvc.perform( + post(ApiPaths.USER_LEGAL_AGREEMENT) + .session(session) + .with(csrf()) + ) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.admin").value(true)) + .andExpect(jsonPath("$.termsAcceptedAt").isNotEmpty()) + .andExpect(jsonPath("$.privacyPolicyAcceptedAt").isNotEmpty()); } @Test diff --git a/src/test/java/com/transactionapi/controller/ShareLinkControllerTest.java b/src/test/java/com/transactionapi/controller/ShareLinkControllerTest.java index 522751e..471ea2b 100644 --- a/src/test/java/com/transactionapi/controller/ShareLinkControllerTest.java +++ b/src/test/java/com/transactionapi/controller/ShareLinkControllerTest.java @@ -167,6 +167,16 @@ void getShareLink_authRequired_authenticated_shouldSucceed() throws Exception { .andExpect(jsonPath("$.requiresAuth").value(true)); } + @Test + @WithMockUser + void getShareLink_authRequired_differentAuthenticatedUser_shouldReturn404() throws Exception { + when(shareLinkService.getShareLink(eq("xyz98765"), eq(USER_ID))) + .thenReturn(Optional.empty()); + + mockMvc.perform(get(ApiPaths.SHARES + "/xyz98765")) + .andExpect(status().isNotFound()); + } + @Test void getShareLink_notFound_shouldReturn404() throws Exception { when(shareLinkService.getShareLink(eq("notfound"), isNull())) diff --git a/src/test/java/com/transactionapi/security/RateLimiterServiceTest.java b/src/test/java/com/transactionapi/security/RateLimiterServiceTest.java index 5e152c0..70b506e 100644 --- a/src/test/java/com/transactionapi/security/RateLimiterServiceTest.java +++ b/src/test/java/com/transactionapi/security/RateLimiterServiceTest.java @@ -8,10 +8,27 @@ class RateLimiterServiceTest { @Test void blocksAfterLimit() { - RateLimiterService limiter = new RateLimiterService(2, 2, 60_000); + RateLimiterService limiter = new RateLimiterService(2, 2, 60_000, 100); assertThat(limiter.allow("user-1")).isTrue(); assertThat(limiter.allow("user-1")).isTrue(); assertThat(limiter.allow("user-1")).isFalse(); } + + @Test + void blocksNewKeysAfterBucketLimit() { + RateLimiterService limiter = new RateLimiterService(2, 2, 60_000, 1); + + assertThat(limiter.allow("user-1")).isTrue(); + assertThat(limiter.allow("user-2")).isFalse(); + } + + @Test + void evictsExpiredBucketsBeforeApplyingBucketLimit() throws Exception { + RateLimiterService limiter = new RateLimiterService(2, 2, 1, 1); + + assertThat(limiter.allow("user-1")).isTrue(); + Thread.sleep(5); + assertThat(limiter.allow("user-2")).isTrue(); + } } diff --git a/src/test/java/com/transactionapi/security/RateLimitingFilterTest.java b/src/test/java/com/transactionapi/security/RateLimitingFilterTest.java new file mode 100644 index 0000000..d09bcd8 --- /dev/null +++ b/src/test/java/com/transactionapi/security/RateLimitingFilterTest.java @@ -0,0 +1,75 @@ +package com.transactionapi.security; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.transactionapi.constants.ApiPaths; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockFilterChain; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.http.HttpStatus; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.test.util.ReflectionTestUtils; + +class RateLimitingFilterTest { + + private final RateLimiterService rateLimiterService = org.mockito.Mockito.mock(RateLimiterService.class); + private final UserIdResolver userIdResolver = org.mockito.Mockito.mock(UserIdResolver.class); + + @BeforeEach + void clearSecurityContext() { + SecurityContextHolder.clearContext(); + } + + @Test + void unauthenticatedPublicShareUsesRemoteAddressByDefault() throws Exception { + RateLimitingFilter filter = new RateLimitingFilter(rateLimiterService, userIdResolver); + MockHttpServletRequest request = publicShareRequest(); + request.setRemoteAddr("203.0.113.10"); + request.addHeader("X-Forwarded-For", "198.51.100.7"); + when(rateLimiterService.allowPublicShare("203.0.113.10")).thenReturn(true); + + filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain()); + + verify(rateLimiterService).allowPublicShare("203.0.113.10"); + } + + @Test + void unauthenticatedPublicShareRejectsSpoofedForwardedHeaderWhenRemoteBucketIsLimited() throws Exception { + RateLimitingFilter filter = new RateLimitingFilter(rateLimiterService, userIdResolver); + MockHttpServletRequest request = publicShareRequest(); + request.setRemoteAddr("203.0.113.10"); + request.addHeader("X-Forwarded-For", "198.51.100.7"); + MockHttpServletResponse response = new MockHttpServletResponse(); + when(rateLimiterService.allowPublicShare("203.0.113.10")).thenReturn(false); + + filter.doFilter(request, response, new MockFilterChain()); + + assertThat(response.getStatus()).isEqualTo(HttpStatus.TOO_MANY_REQUESTS.value()); + assertThat(response.getContentAsString()).contains("Too many requests"); + verify(rateLimiterService).allowPublicShare("203.0.113.10"); + } + + @Test + void unauthenticatedPublicShareUsesForwardedHeaderOnlyWhenTrusted() throws Exception { + RateLimitingFilter filter = new RateLimitingFilter(rateLimiterService, userIdResolver); + ReflectionTestUtils.setField(filter, "trustForwardedHeaders", true); + MockHttpServletRequest request = publicShareRequest(); + request.setRemoteAddr("203.0.113.10"); + request.addHeader("X-Forwarded-For", "198.51.100.7, 10.0.0.1"); + when(rateLimiterService.allowPublicShare("198.51.100.7")).thenReturn(true); + + filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain()); + + verify(rateLimiterService).allowPublicShare("198.51.100.7"); + } + + private MockHttpServletRequest publicShareRequest() { + MockHttpServletRequest request = new MockHttpServletRequest("GET", ApiPaths.SHARES + "/abc12345"); + request.setServletPath(ApiPaths.SHARES + "/abc12345"); + return request; + } +} diff --git a/src/test/java/com/transactionapi/security/UserIdResolverTest.java b/src/test/java/com/transactionapi/security/UserIdResolverTest.java index f0054b4..846f9ca 100644 --- a/src/test/java/com/transactionapi/security/UserIdResolverTest.java +++ b/src/test/java/com/transactionapi/security/UserIdResolverTest.java @@ -85,6 +85,14 @@ void allowsConfiguredAdmin() { userIdResolver.requireAdmin(auth); } + @Test + void reportsAdminStatusWithoutThrowing() { + configureEmails("", "admin@example.com"); + + assertThat(userIdResolver.isAdmin(buildAuth("sub-6", "admin@example.com"))).isTrue(); + assertThat(userIdResolver.isAdmin(buildAuth("sub-7", "user@example.com"))).isFalse(); + } + private void configureAllowlist(String allowed) { configureEmails(allowed, ""); } diff --git a/src/test/java/com/transactionapi/service/ShareLinkServiceTest.java b/src/test/java/com/transactionapi/service/ShareLinkServiceTest.java index b57e3fc..719a966 100644 --- a/src/test/java/com/transactionapi/service/ShareLinkServiceTest.java +++ b/src/test/java/com/transactionapi/service/ShareLinkServiceTest.java @@ -136,7 +136,7 @@ void getShareLink_publicShare_shouldReturnForAnyone() { } @Test - void getShareLink_authRequired_shouldReturnForAuthenticatedUser() { + void getShareLink_authRequired_shouldReturnForOwner() { when(shareLinkRepository.findByCode("xyz98765")).thenReturn(Optional.of(authRequiredShare)); when(shareLinkRepository.save(any(ShareLink.class))).thenAnswer(invocation -> invocation.getArgument(0)); @@ -147,6 +147,16 @@ void getShareLink_authRequired_shouldReturnForAuthenticatedUser() { assertThat(result.get().getAccessCount()).isEqualTo(1); } + @Test + void getShareLink_authRequired_shouldNotReturnForDifferentAuthenticatedUser() { + when(shareLinkRepository.findByCode("xyz98765")).thenReturn(Optional.of(authRequiredShare)); + + Optional result = shareLinkService.getShareLink("xyz98765", "user2"); + + assertThat(result).isEmpty(); + verify(shareLinkRepository, never()).save(any(ShareLink.class)); + } + @Test void getShareLink_authRequired_shouldNotReturnForUnauthenticated() { when(shareLinkRepository.findByCode("xyz98765")).thenReturn(Optional.of(authRequiredShare));