Skip to content

Commit 7dc8f9f

Browse files
thcedthced
committed
refactor: Address Sonar findings in CORS preflight handler
Reduce cognitive complexity of the predicate corsPreflightHandler overload by extracting requireHeader / isPreflightAllowed / parseMethodOrNull / requestedHeadersAllowed / buildPreflightSuccess helpers. Introduce an ALLOW constant for the repeated header name. Mark the swallowed parse exception with an unnamed pattern. Hoist throwing argument expressions out of assertThatThrownBy lambdas so each lambda has a single risky call.
1 parent c958b3b commit 7dc8f9f

2 files changed

Lines changed: 96 additions & 56 deletions

File tree

‎src/main/java/com/retailsvc/http/Handlers.java‎

Lines changed: 86 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
public final class Handlers {
3434

3535
private static final Logger LOG = LoggerFactory.getLogger(Handlers.class);
36+
private static final String ALLOW = "Allow";
3637

3738
private Handlers() {}
3839

@@ -127,59 +128,96 @@ public static RequestHandler corsPreflightHandler(
127128
.map(h -> h.toLowerCase(Locale.ROOT))
128129
.collect(Collectors.toUnmodifiableSet());
129130
String maxAgeHeader = maxAge == null ? null : Long.toString(maxAge.getSeconds());
131+
boolean emitAllowHeaders = !allowedHeaders.isEmpty();
130132

131133
return req -> {
132134
if (req.method() != OPTIONS) {
133-
return Response.status(HTTP_BAD_METHOD).withHeader("Allow", "OPTIONS");
135+
return Response.status(HTTP_BAD_METHOD).withHeader(ALLOW, "OPTIONS");
134136
}
135-
String origin = req.header("Origin").orElse(null);
136-
if (origin == null) {
137-
throw new BadRequestException("CORS preflight is missing the Origin header");
138-
}
139-
String requestMethod = req.header("Access-Control-Request-Method").orElse(null);
140-
if (requestMethod == null) {
141-
throw new BadRequestException(
142-
"CORS preflight is missing the Access-Control-Request-Method header");
143-
}
144-
if (!originAllowed.test(origin)) {
145-
return Response.status(HTTP_FORBIDDEN);
146-
}
147-
HttpMethod parsedMethod;
148-
try {
149-
parsedMethod = HttpMethod.parse(requestMethod);
150-
} catch (IllegalArgumentException e) {
151-
return Response.status(HTTP_FORBIDDEN);
152-
}
153-
if (!allowedMethods.contains(parsedMethod)) {
137+
String origin = requireHeader(req, "Origin");
138+
String requestMethod = requireHeader(req, "Access-Control-Request-Method");
139+
if (!isPreflightAllowed(
140+
req, origin, requestMethod, originAllowed, allowedMethods, headerAllowlistLower)) {
154141
return Response.status(HTTP_FORBIDDEN);
155142
}
156-
String requestedHeaders = req.header("Access-Control-Request-Headers").orElse("");
157-
for (String raw : requestedHeaders.split(",")) {
158-
String h = raw.trim().toLowerCase(Locale.ROOT);
159-
if (h.isEmpty()) {
160-
continue;
161-
}
162-
if (!headerAllowlistLower.contains(h)) {
163-
return Response.status(HTTP_FORBIDDEN);
164-
}
165-
}
143+
return buildPreflightSuccess(
144+
origin,
145+
allowMethodsHeader,
146+
allowHeadersHeader,
147+
emitAllowHeaders,
148+
allowCredentials,
149+
maxAgeHeader);
150+
};
151+
}
166152

167-
Response resp =
168-
Response.status(HTTP_NO_CONTENT)
169-
.withHeader("Access-Control-Allow-Origin", origin)
170-
.withHeader("Access-Control-Allow-Methods", allowMethodsHeader)
171-
.withHeader("Vary", "Origin");
172-
if (!allowedHeaders.isEmpty()) {
173-
resp = resp.withHeader("Access-Control-Allow-Headers", allowHeadersHeader);
174-
}
175-
if (allowCredentials) {
176-
resp = resp.withHeader("Access-Control-Allow-Credentials", "true");
153+
private static String requireHeader(Request req, String name) {
154+
return req.header(name)
155+
.orElseThrow(
156+
() -> new BadRequestException("CORS preflight is missing the " + name + " header"));
157+
}
158+
159+
private static boolean isPreflightAllowed(
160+
Request req,
161+
String origin,
162+
String requestMethod,
163+
Predicate<String> originAllowed,
164+
List<HttpMethod> allowedMethods,
165+
Set<String> headerAllowlistLower) {
166+
if (!originAllowed.test(origin)) {
167+
return false;
168+
}
169+
HttpMethod parsed = parseMethodOrNull(requestMethod);
170+
if (parsed == null || !allowedMethods.contains(parsed)) {
171+
return false;
172+
}
173+
return requestedHeadersAllowed(req, headerAllowlistLower);
174+
}
175+
176+
private static HttpMethod parseMethodOrNull(String s) {
177+
try {
178+
return HttpMethod.parse(s);
179+
} catch (IllegalArgumentException _) {
180+
// Unknown method token — treated as disallowed by the caller.
181+
return null;
182+
}
183+
}
184+
185+
private static boolean requestedHeadersAllowed(Request req, Set<String> allowedLower) {
186+
String requested = req.header("Access-Control-Request-Headers").orElse("");
187+
for (String raw : requested.split(",")) {
188+
String h = raw.trim().toLowerCase(Locale.ROOT);
189+
if (h.isEmpty()) {
190+
continue;
177191
}
178-
if (maxAgeHeader != null) {
179-
resp = resp.withHeader("Access-Control-Max-Age", maxAgeHeader);
192+
if (!allowedLower.contains(h)) {
193+
return false;
180194
}
181-
return resp;
182-
};
195+
}
196+
return true;
197+
}
198+
199+
private static Response buildPreflightSuccess(
200+
String origin,
201+
String allowMethodsHeader,
202+
String allowHeadersHeader,
203+
boolean emitAllowHeaders,
204+
boolean allowCredentials,
205+
String maxAgeHeader) {
206+
Response resp =
207+
Response.status(HTTP_NO_CONTENT)
208+
.withHeader("Access-Control-Allow-Origin", origin)
209+
.withHeader("Access-Control-Allow-Methods", allowMethodsHeader)
210+
.withHeader("Vary", "Origin");
211+
if (emitAllowHeaders) {
212+
resp = resp.withHeader("Access-Control-Allow-Headers", allowHeadersHeader);
213+
}
214+
if (allowCredentials) {
215+
resp = resp.withHeader("Access-Control-Allow-Credentials", "true");
216+
}
217+
if (maxAgeHeader != null) {
218+
resp = resp.withHeader("Access-Control-Max-Age", maxAgeHeader);
219+
}
220+
return resp;
183221
}
184222

185223
public static ExceptionHandler defaultExceptionHandler() {
@@ -199,7 +237,7 @@ public static ExceptionHandler defaultExceptionHandler() {
199237
case MethodNotAllowedException mna ->
200238
Response.status(HTTP_BAD_METHOD)
201239
.withHeader(
202-
"Allow",
240+
ALLOW,
203241
mna.allowed().stream().map(Enum::name).collect(Collectors.joining(", ")));
204242
default -> {
205243
LOG.error("Unhandled exception in handler", t);
@@ -213,7 +251,7 @@ public static RequestHandler aliveHandler() {
213251
return req ->
214252
switch (req.method()) {
215253
case GET, HEAD -> Response.empty();
216-
default -> Response.status(HTTP_BAD_METHOD).withHeader("Allow", "GET, HEAD");
254+
default -> Response.status(HTTP_BAD_METHOD).withHeader(ALLOW, "GET, HEAD");
217255
};
218256
}
219257

@@ -238,7 +276,7 @@ public static RequestHandler healthHandler(Supplier<HealthOutcome> probe) {
238276
Objects.requireNonNull(probe, "probe");
239277
return req -> {
240278
if (req.method() != GET && req.method() != HEAD) {
241-
return Response.status(HTTP_BAD_METHOD).withHeader("Allow", "GET, HEAD");
279+
return Response.status(HTTP_BAD_METHOD).withHeader(ALLOW, "GET, HEAD");
242280
}
243281
boolean up;
244282
List<Dependency> dependencies;
@@ -297,7 +335,7 @@ private static RequestHandler resourceHandler(ResourceSource source) {
297335
Response.status(HTTP_OK)
298336
.withContentType(contentType)
299337
.withHeader("Content-Length", String.valueOf(length));
300-
default -> Response.status(HTTP_BAD_METHOD).withHeader("Allow", "GET, HEAD");
338+
default -> Response.status(HTTP_BAD_METHOD).withHeader(ALLOW, "GET, HEAD");
301339
};
302340
}
303341
}

‎src/test/java/com/retailsvc/http/CorsPreflightHandlerTest.java‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -114,18 +114,19 @@ void corsPreflightHandlerRejectsNonOptionsWith405AndAllowOptions() {
114114
@Test
115115
void corsPreflightHandlerRejectsMissingOriginWith400() {
116116
RequestHandler handler = Handlers.corsPreflightHandler(ORIGINS, METHODS, HEADERS, false, null);
117+
Request noOrigin = preflight(null, "POST", "content-type");
117118

118-
assertThatThrownBy(() -> handler.handle(preflight(null, "POST", "content-type")))
119+
assertThatThrownBy(() -> handler.handle(noOrigin))
119120
.isInstanceOf(BadRequestException.class)
120121
.hasMessageContaining("Origin");
121122
}
122123

123124
@Test
124125
void corsPreflightHandlerRejectsMissingRequestMethodWith400() {
125126
RequestHandler handler = Handlers.corsPreflightHandler(ORIGINS, METHODS, HEADERS, false, null);
127+
Request noRequestMethod = preflight("https://app.example.com", null, "content-type");
126128

127-
assertThatThrownBy(
128-
() -> handler.handle(preflight("https://app.example.com", null, "content-type")))
129+
assertThatThrownBy(() -> handler.handle(noRequestMethod))
129130
.isInstanceOf(BadRequestException.class)
130131
.hasMessageContaining("Access-Control-Request-Method");
131132
}
@@ -240,8 +241,9 @@ void corsPreflightHandlerRejectsNullMethods() {
240241

241242
@Test
242243
void corsPreflightHandlerRejectsEmptyMethods() {
243-
assertThatThrownBy(
244-
() -> Handlers.corsPreflightHandler(ORIGINS, List.of(), HEADERS, false, null))
244+
List<HttpMethod> empty = List.of();
245+
246+
assertThatThrownBy(() -> Handlers.corsPreflightHandler(ORIGINS, empty, HEADERS, false, null))
245247
.isInstanceOf(IllegalArgumentException.class)
246248
.hasMessageContaining("allowedMethods");
247249
}
@@ -255,10 +257,10 @@ void corsPreflightHandlerRejectsNullHeaders() {
255257

256258
@Test
257259
void corsPreflightHandlerRejectsNegativeMaxAge() {
260+
Duration negative = Duration.ofSeconds(-1);
261+
258262
assertThatThrownBy(
259-
() ->
260-
Handlers.corsPreflightHandler(
261-
ORIGINS, METHODS, HEADERS, false, Duration.ofSeconds(-1)))
263+
() -> Handlers.corsPreflightHandler(ORIGINS, METHODS, HEADERS, false, negative))
262264
.isInstanceOf(IllegalArgumentException.class)
263265
.hasMessageContaining("maxAge");
264266
}

0 commit comments

Comments
 (0)