MOSIP-29075 : Remove secrets from API testrig logs when debug is disabled. #1859
Conversation
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
|
Warning Rate limit exceeded@SradhaMohanty5899 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds masked-logging utilities and a RestAssured logging filter; integrates masked logging across RestClient; adds expiry/timestamp and request-ID extraction helpers in AdminTestUtil; expands GlobalConstants with many constants; and introduces API_LOGGER config in log4j.properties. Changes
Sequence DiagramsequenceDiagram
autonumber
actor Client
participant RestClient
participant Filter as RestAssuredPrettyLogger
participant Mask as LogMaskingUtil
participant API_LOGGER as API_LOGGER
participant Remote as RemoteAPI
Client->>RestClient: call HTTP method
RestClient->>Filter: attach getMaskingFilter()
Filter->>Mask: mask request headers/body/cookies/params
Mask-->>Filter: masked request
Filter->>API_LOGGER: log masked request
Filter->>Remote: execute HTTP request
Remote-->>Filter: response
Filter->>Mask: mask response headers/body
Mask-->>Filter: masked response
Filter->>API_LOGGER: log masked response
Filter-->>RestClient: return response
RestClient-->>Client: return response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java (1)
778-783: Inconsistent masking: sensitive data may leak in non-debug mode.The else branch on line 781 still uses
.log().all()which will log unmasked sensitive data. This defeats the purpose of the masking filter added in the debug branch.} else { postResponse = given().config(config).relaxedHTTPSValidation().headers(headers).body(body) .contentType(contentHeader) .cookie(GlobalConstants.XSRF_TOKEN, cookieMap.get(GlobalConstants.XSRF_TOKEN)) - .cookie(key, cookieMap.get(key)).accept(acceptHeader).when().post(url).then().log().all() + .cookie(key, cookieMap.get(key)).accept(acceptHeader).when().post(url).then() .extract().response(); }
🧹 Nitpick comments (4)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (1)
7324-7349: Expiry-date helper works but can be simplified and made more robustThe loop in
addCustomExpiryDatecorrectly replaces all$EXPIRYDATE:amount$-style tokens usingChronoUnit, andgetUtcTimestampproduces UTC timestamps in the existing format.Two optional improvements:
- Use
DateTimeFormatteronZonedDateTimeinstead of converting back toDate/SimpleDateFormatto stay fully injava.time.- Optionally accept a
Clock(or pass inZonedDateTime now) for easier deterministic testing of time-based behaviour.These are non-blocking and can be deferred.
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (1)
15-85: Consider making the filter instance a singleton.
getMaskingFilter()creates a newFilterinstance on every call. Since the filter is stateless, consider caching a single instance for better performance.+ private static final Filter MASKING_FILTER = createMaskingFilter(); + public static Filter getMaskingFilter() { + return MASKING_FILTER; + } + + private static Filter createMaskingFilter() { return new Filter() { // ... existing implementation }; }apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (2)
12-15: Consider case-insensitive key matching and additional sensitive keys.The current implementation uses case-sensitive matching which won't catch variations like "Password", "TOKEN", or "CLIENT_SECRET". Also consider adding commonly used sensitive field names.
- private static final List<String> SENSITIVE_KEYS = Arrays.asList( - "clientSecret", "client_secret", "password", "pwd", - "token", "access_token", "refresh_token", "refreshToken" - ); + private static final List<String> SENSITIVE_KEYS = Arrays.asList( + "clientSecret", "client_secret", "password", "pwd", + "token", "access_token", "refresh_token", "refreshToken", + "secret", "apiKey", "api_key", "credential", "auth_token", + "private_key", "privateKey", "authorization" + );For case-insensitive matching, modify the check in
mask():- if (SENSITIVE_KEYS.contains(key)) { + if (SENSITIVE_KEYS.stream().anyMatch(k -> k.equalsIgnoreCase(key))) {
50-57: Masking reveals last 3 characters which may be too revealing for short secrets.For short tokens or passwords (e.g., 6-8 characters), showing the last 3 characters reveals a significant portion. Consider showing fewer characters or using a fixed mask.
private static String maskValue(String value) { - if (value.length() <= 4) { + if (value.length() <= 6) { return "****"; } - int unmasked = 3; // last 3 digits shown + int unmasked = Math.min(2, value.length() / 4); // show at most 2 chars or 25% String stars = "*".repeat(value.length() - unmasked); return stars + value.substring(value.length() - unmasked); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apitest-commons/src/main/java/io/mosip/testrig/apirig/testrunner/BaseTestCase.java(1 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java(6 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/GlobalConstants.java(1 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/GlobalMethods.java(1 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java(1 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java(1 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java(89 hunks)apitest-commons/src/main/resources/log4j.properties(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (1)
LogMaskingUtil(10-68)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java (4)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (1)
RestAssuredPrettyLogger(11-87)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (1)
LogMaskingUtil(10-68)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/GlobalConstants.java (1)
GlobalConstants(3-284)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/ConfigManager.java (1)
ConfigManager(15-281)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/GlobalConstants.java (1)
GlobalConstants(3-284)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-maven-authentication-demo-service / maven-build
- GitHub Check: build-maven-apitest-commons / maven-build
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (10)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/GlobalMethods.java (4)
76-83: Formatting improvement adds clarity.The inserted blank line between
getRunContext()andmain()improves code readability through better method separation.
269-294: Sensitive data masking implementation is solid.The
maskOutSensitiveInfo()method appropriately masks sensitive fields across JSON payloads with case-insensitive matching, debug-mode bypass, and specialized handling for biometric data.
308-328: Request masking is properly integrated.The
reportRequest()methods correctly applymaskOutSensitiveInfo()before logging request bodies, maintaining consistency with the PR's security objectives.
330-356: Unable to verify the review comment due to repository access failure. Manual verification required to confirm:
- Whether response bodies are currently masked before logging
- If PR objectives explicitly require response masking
- Whether
RestAssuredPrettyLoggeror other downstream filters handle masking- The exact locations where masking should be applied
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (2)
43-50: Time/Chrono imports correctly support new expiry utilitiesThe added
java.time/ChronoUnitimports line up withgetUtcTimestampusage; no issues from a correctness perspective.Also applies to: 67-73
1251-1253: _reqsid flow integration looks consistent with existing _sid handlingWiring
_reqsidtowriteAutoGeneratedIdFromRequesthere mirrors the existing_sid/response-based flow and happens after all keyword/OTP preprocessing, which is appropriate.apitest-commons/src/main/java/io/mosip/testrig/apirig/testrunner/BaseTestCase.java (1)
362-368: LGTM!The programmatic Log4j configuration for
API_LOGGERis correctly implemented. The minimal conversion pattern (%m%n) is appropriate for the masking filter's pre-formatted output.apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java (2)
289-290: Masking filter applied unconditionally outside debug check.Line 290 applies the masking filter outside the
IsDebugEnabled()check, meaning the filter runs even when debug logging is disabled. This is inconsistent with other methods where the filter is only applied in debug mode. Consider whether this is intentional.
314-315: Same inconsistency: masking filter applied outside debug check.Similar to line 290, the filter is applied before entering the debug block at line 315.
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/GlobalConstants.java (1)
179-205: LGTM!The new constants are well-organized and follow existing naming conventions. They properly support the logging, transaction tracking, and masking infrastructure introduced in this PR.
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java
Outdated
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java
Outdated
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java
Outdated
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java
Outdated
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java
Outdated
Show resolved
Hide resolved
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java (1)
781-781: Inconsistent logging:.log().all()bypasses masking in non-debug branch.Line 781 uses
.log().all()in the else (non-debug) branch, which logs the response without masking. This is inconsistent with other methods in this file where the else branch omits explicit logging.Apply this diff to maintain consistency:
postResponse = given().config(config).relaxedHTTPSValidation().headers(headers).body(body) .contentType(contentHeader) .cookie(GlobalConstants.XSRF_TOKEN, cookieValue.get(GlobalConstants.XSRF_TOKEN)) - .cookie(key, cookieValue.get(key)).accept(acceptHeader).when().post(url).then().log().all() + .cookie(key, cookieValue.get(key)).accept(acceptHeader).when().post(url).then() .extract().response();
♻️ Duplicate comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (1)
51-63: Critical: Cookies with session/auth tokens are not being masked.Similar to the headers issue,
LogMaskingUtil.maskSensitiveData()expects JSON input. Cookie values are plain strings, so sensitive cookies (includingAUTHORIZATION, session tokens, etc.) are logged without masking.This is a critical security issue that undermines the PR's objective.
Apply this diff to implement proper cookie masking:
// Cookies apiLogger.info("Cookies:\t"); if (req.getCookies() == null || req.getCookies().asList().isEmpty()) { apiLogger.info("\t<none>"); } else { req.getCookies().asList().forEach(cookie -> { - // cookie.getName() -> cookie name - // cookie.getValue() -> cookie value (String) - String maskedValue = LogMaskingUtil.maskSensitiveData(cookie.getValue()); + String value = cookie.getValue(); + String cookieName = cookie.getName().toLowerCase(); + // Mask auth/session cookies + if (cookieName.contains("authorization") || + cookieName.contains("auth") || + cookieName.contains("session") || + cookieName.contains("token")) { + value = (value != null && value.length() > 4) + ? "****" + value.substring(value.length() - 4) + : "****"; + } - apiLogger.info("\t" + cookie.getName() + "=" + maskedValue); + apiLogger.info("\t" + cookie.getName() + "=" + value); }); }
🧹 Nitpick comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (1)
1248-1250: Request-based auto-generated ID extraction looks correct; minor robustness tweaks possibleThe
_reqsidhook andwriteAutoGeneratedIdFromRequestimplementation correctly:
- Parse the request JSON,
- Safely handle missing
"request"objects, and- Store extracted IDs while masking logged payloads via
LogMaskingUtil.Two small robustness improvements to consider:
- Trim field names after
idKeyName.split(",")to tolerate"field1, field2"style inputs.- In the catch block, log the exception with the throwable (e.g.,
logger.error("...", e)) so stack traces are available when debugging template issues.These don’t affect current behavior but would make failures easier to diagnose.
Also applies to: 3252-3277
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java(7 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java(1 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java(89 hunks)apitest-commons/src/main/resources/log4j.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apitest-commons/src/main/resources/log4j.properties
🧰 Additional context used
🧬 Code graph analysis (2)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (1)
LogMaskingUtil(10-68)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java (4)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (1)
RestAssuredPrettyLogger(11-107)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (1)
LogMaskingUtil(10-68)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/GlobalConstants.java (1)
GlobalConstants(3-284)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/ConfigManager.java (1)
ConfigManager(15-281)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-maven-apitest-commons / maven-build
- GitHub Check: build-maven-authentication-demo-service / maven-build
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (3)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (2)
3350-3353: Good: response body now masked in error logsThe changes in
writeAutoGeneratedId(Response, ...)to wrapresponse.asString()withLogMaskingUtil.maskSensitiveData(...)ensure that error logs no longer contain raw response bodies, aligning this helper with the PR’s log-masking goals.
7167-7195: Masked logging incheckResponseUTCTimealigns with log-masking objectivesSwitching the initial log in
checkResponseUTCTimeto:logger.info(LogMaskingUtil.maskSensitiveData(response.asString()));ensures this diagnostic helper no longer prints raw responses, while leaving the parsing logic intact. This is consistent with the rest of the masking changes in the PR.
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestClient.java (1)
55-55: LGTM: Masking filter and safe logging correctly applied.The changes consistently apply
RestAssuredPrettyLogger.getMaskingFilter()to request specifications and useLogMaskingUtil.safeLogInfo()for response body logging. The integration pattern is correct throughout the file.Note: The effectiveness of this masking depends on fixing the issues identified in RestAssuredPrettyLogger.java regarding header and cookie masking.
Also applies to: 82-86, 290-301, 1982-1983
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java
Outdated
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java
Outdated
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java
Show resolved
Hide resolved
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java
Show resolved
Hide resolved
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (4)
46-49: Critical: Request headers containing tokens are logged without masking.
LogMaskingUtil.maskSensitiveData()expects JSON input and returns the raw string unchanged when JSON parsing fails (see lines 14-17 in LogMaskingUtil). Since header values are plain strings (e.g.,"Bearer eyJ...","Basic dXNlcjpw..."), they are logged without any masking. This exposes authentication tokens, API keys, and credentials in logs, directly contradicting the PR's security objective.Apply this diff to implement proper header value masking:
// Headers apiLogger.info("Headers:\t"); req.getHeaders().asList() - .forEach(h -> apiLogger.info("\t" + h.getName() + "=" + LogMaskingUtil.maskSensitiveData(h.getValue()))); + .forEach(h -> { + String value = h.getValue(); + String headerNameLower = h.getName().toLowerCase(); + // Mask sensitive header values + if (headerNameLower.contains("authorization") || + headerNameLower.contains("token") || + headerNameLower.contains("secret") || + headerNameLower.contains("api-key") || + headerNameLower.contains("cookie")) { + value = (value != null && value.length() > 4) + ? "****" + value.substring(value.length() - 4) + : "****"; + } + apiLogger.info("\t" + h.getName() + "=" + value); + });
51-59: Critical: Cookies containing session/auth tokens are logged without masking.Cookie values are plain strings, not JSON.
LogMaskingUtil.maskSensitiveData()will fail to parse them as JSON and return the original unmasked values. This exposes session tokens, authentication cookies (e.g.,Authorization,JSESSIONID), and other sensitive cookie data in logs.Apply this diff to implement proper cookie value masking:
// Cookies - String cookiesLog = (req.getCookies() == null || req.getCookies().asList().isEmpty()) - ? "<none>" - : req.getCookies().asList().stream() - .map(c -> c.getName() + "=" + LogMaskingUtil.maskSensitiveData(c.getValue())) - .reduce((a, b) -> a + ", " + b) - .orElse("<none>"); + String cookiesLog; + if (req.getCookies() == null || req.getCookies().asList().isEmpty()) { + cookiesLog = "<none>"; + } else { + cookiesLog = req.getCookies().asList().stream() + .map(c -> { + String value = c.getValue(); + String cookieNameLower = c.getName().toLowerCase(); + // Mask sensitive cookies + if (cookieNameLower.contains("auth") || + cookieNameLower.contains("session") || + cookieNameLower.contains("token") || + cookieNameLower.equals("authorization")) { + value = (value != null && value.length() > 4) + ? "****" + value.substring(value.length() - 4) + : "****"; + } + return c.getName() + "=" + value; + }) + .reduce((a, b) -> a + ", " + b) + .orElse("<none>"); + } apiLogger.info("Cookies:\t" + cookiesLog);
62-76: Major: Multipart fields with sensitive data are not properly masked.The current implementation has two issues:
- File content logged via
String.valueOf()produces unhelpful output likeFile@hashcode- Plain string multipart field values won't be masked since
LogMaskingUtil.maskSensitiveData()expects JSONThis exposes sensitive form fields (e.g., password, secret, token) and provides poor debugging information for file uploads.
Apply this diff to properly handle multipart logging:
// Multiparts (one line) if (req.getMultiPartParams() == null || req.getMultiPartParams().isEmpty()) { apiLogger.info("Multiparts:\t<none>"); } else { StringBuilder mpLog = new StringBuilder(); req.getMultiPartParams().forEach(mp -> { String key = mp.getControlName(); // field name Object value = mp.getContent(); // value (file or string) - String maskedValue = LogMaskingUtil.maskSensitiveData(String.valueOf(value)); + String displayValue; + if (value instanceof java.io.File) { + displayValue = "<file: " + ((java.io.File) value).getName() + ">"; + } else { + String keyLower = key.toLowerCase(); + displayValue = String.valueOf(value); + // Mask sensitive field names + if (keyLower.contains("password") || keyLower.contains("secret") || + keyLower.contains("token") || keyLower.contains("key")) { + displayValue = "****"; + } + } if (mpLog.length() > 0) { mpLog.append(", "); } - mpLog.append(key).append("=").append(maskedValue); + mpLog.append(key).append("=").append(displayValue); }); apiLogger.info("Multiparts:\t" + mpLog.toString()); }
90-93: Critical: Response headers with sensitive tokens are logged without masking.Response headers suffer from the same issue as request headers.
LogMaskingUtil.maskSensitiveData()cannot mask plain string header values. This exposesSet-Cookie,Authorization, and other sensitive response headers containing session tokens and credentials.Apply this diff to implement proper response header masking:
// Response headers response.getHeaders().forEach( - h -> apiLogger.info(h.getName() + ": " + LogMaskingUtil.maskSensitiveData(h.getValue())) + h -> { + String value = h.getValue(); + String headerNameLower = h.getName().toLowerCase(); + // Mask sensitive response headers + if (headerNameLower.contains("authorization") || + headerNameLower.contains("token") || + headerNameLower.contains("secret") || + headerNameLower.contains("set-cookie") || + headerNameLower.contains("api-key") || + headerNameLower.contains("cookie")) { + value = (value != null && value.length() > 4) + ? "****" + value.substring(value.length() - 4) + : "****"; + } + apiLogger.info(h.getName() + ": " + value); + } );
🧹 Nitpick comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (1)
86-97: Add null safety for response handling.While unlikely, the response from
ctx.next()could theoretically be null orresponse.asString()could throw an exception. Adding null checks would improve robustness.Apply this diff to add defensive null checks:
// Execute the actual request Response response = ctx.next(req, res); + + if (response == null) { + apiLogger.error("Response is null"); + return null; + } apiLogger.info(response.getStatusLine()); // Response headers response.getHeaders().forEach( h -> apiLogger.info(h.getName() + ": " + LogMaskingUtil.maskSensitiveData(h.getValue())) ); // Response body - String maskedResponse = LogMaskingUtil.maskSensitiveData(response.asString()); - apiLogger.info("\n" + maskedResponse); + try { + String maskedResponse = LogMaskingUtil.maskSensitiveData(response.asString()); + apiLogger.info("\n" + maskedResponse); + } catch (Exception e) { + apiLogger.error("Failed to log response body: " + e.getMessage()); + } return response;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-maven-authentication-demo-service / maven-build
- GitHub Check: build-maven-apitest-commons / maven-build
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (1)
3-3: Verify Log4j version and consider upgrading to Log4j 2.x if not required.The code imports
org.apache.log4j.Logger(Log4j 1.x), which reached end-of-life in 2015 and has known security vulnerabilities. Check the project's dependency configuration to confirm whether this is an intentional requirement or if the project should migrate to Log4j 2.x (org.apache.logging.log4j.Logger).
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (1)
20-30: Root-level JSON arrays and nested arrays are not masked, leaking secrets in array bodies.
maskSensitiveData(String json)assumes a JSON object root and returns the raw input on parse failure, so payloads like["token1", "token2"]or[{"password":"p"}, {"token":"t"}]will be logged unmasked.mask(JSONObject)also only walks a single-levelJSONArrayofJSONObjectitems and skips nested arrays.Given this PR’s goal, array payloads should be treated the same as objects.
Consider extending the logic to handle both object and array roots and to recurse through arrays:
- public static String maskSensitiveData(String json) { - if (json == null) return null; - - try { - JSONObject obj = new JSONObject(json); - mask(obj); - return obj.toString(4); // pretty print - } catch (Exception e) { - return json; // In case the body is not JSON, just return raw - } - } + public static String maskSensitiveData(String json) { + if (json == null) { + return null; + } + + String trimmed = json.trim(); + try { + if (trimmed.startsWith("{")) { + JSONObject obj = new JSONObject(trimmed); + mask(obj); + return obj.toString(4); + } else if (trimmed.startsWith("[")) { + JSONArray arr = new JSONArray(trimmed); + maskArray(arr); + return arr.toString(4); + } + // Not a JSON object/array, return as-is + return json; + } catch (Exception e) { + // In case the body is not valid JSON, just return raw + return json; + } + } @@ - } else if (value instanceof JSONArray) { - JSONArray arr = (JSONArray) value; - for (int i = 0; i < arr.length(); i++) { - if (arr.get(i) instanceof JSONObject) { - mask(arr.getJSONObject(i)); - } - } - } else { + } else if (value instanceof JSONArray) { + maskArray((JSONArray) value); + } else { @@ - } + } + + private static void maskArray(JSONArray arr) { + for (int i = 0; i < arr.length(); i++) { + Object element = arr.get(i); + if (element instanceof JSONObject) { + mask((JSONObject) element); + } else if (element instanceof JSONArray) { + maskArray((JSONArray) element); + } + } + }This ensures both request and response bodies that are arrays (including nested arrays) get the same masking guarantees as objects.
Also applies to: 36-44
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (1)
43-49: Expiry-date token handling is sound; refine error reporting and trimming inaddCustomExpiryDateThe
$EXPIRYDATE:/$EXPIRYDATE_YEARS:support wired throughinputJsonKeyWordHandelerand implemented viaaddCustomExpiryDate+getUtcTimestamplooks functionally correct and UTC-safe.Two small issues in
addCustomExpiryDate:
- The NumberFormatException branch message is truncated and doesn’t show the offending numeric segment:
throw new IllegalArgumentException("Invalid number for token '" + token);- Parsing
Long.parseLong(json.substring(start, end))without trimming means templates like$EXPIRYDATE: 5$will fail even though a human would consider them valid.A minimal, clearer, and still payload-safe improvement:
- long amount; - try { - amount = Long.parseLong(json.substring(start, end)); - } catch (NumberFormatException e) { - throw new IllegalArgumentException("Invalid number for token '" + token); - } + final String rawAmount = json.substring(start, end).trim(); + final long amount; + try { + amount = Long.parseLong(rawAmount); + } catch (NumberFormatException e) { + throw new IllegalArgumentException( + "Invalid numeric amount for token '" + token + "': '" + rawAmount + "'", e); + }
- Keeps error messages self-contained and readable.
- Accepts minor whitespace in templates.
- Continues to avoid embedding the full JSON payload, consistent with earlier review feedback.
Also applies to: 3668-3673, 7321-7351
🧹 Nitpick comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (1)
76-90: Avoid!=for Strings and harden Set-Cookie regex masking.The header-masking loop uses reference comparison and an unescaped regex key:
if (headerName.equalsIgnoreCase(sensitiveKey) && sensitiveKey != "set-cookie") { return maskValue(value); } ... if ("set-cookie".equalsIgnoreCase(headerName)) { maskedValue = maskedValue.replaceAll("(?i)" + sensitiveKey + "=[^;]*", sensitiveKey + "=********"); }Issues:
sensitiveKey != "set-cookie"relies on String interning and is a brittle Java anti-pattern.- Directly concatenating
sensitiveKeyinto a regex will break if future keys contain regex metacharacters.Suggested fix:
- // If header name itself is sensitive -> mask fully - if (headerName.equalsIgnoreCase(sensitiveKey) && sensitiveKey != "set-cookie") { - return maskValue(value); - } - - // If Set-Cookie header -> mask sensitive keys inside cookie value - if ("set-cookie".equalsIgnoreCase(headerName)) { - maskedValue = maskedValue.replaceAll("(?i)" + sensitiveKey + "=[^;]*", sensitiveKey + "=********"); - } + // If header name itself is sensitive (except Set-Cookie) -> mask fully + if (!"set-cookie".equalsIgnoreCase(headerName) + && headerName.equalsIgnoreCase(sensitiveKey)) { + return maskValue(value); + } + + // If Set-Cookie header -> mask sensitive keys inside cookie value + if ("set-cookie".equalsIgnoreCase(headerName)) { + String quotedKey = Pattern.quote(sensitiveKey); + maskedValue = maskedValue.replaceAll("(?i)" + quotedKey + "=[^;]*", + sensitiveKey + "=********"); + }This keeps the current behavior while avoiding subtle String/regex pitfalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java(7 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java(1 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (1)
LogMaskingUtil(11-92)
🔇 Additional comments (3)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (3)
1248-1250: Request-side auto-generated ID extraction is null-safe and masking-awareThe new
_reqsidhandling pluswriteAutoGeneratedIdFromRequestcorrectly:
- Guards against missing
"request"object (optJSONObject+ null check).- Caches per-field values via
getAutogenIdKeyName/writeToCache.- Uses
LogMaskingUtil.maskSensitiveDataboth for individual field values and entire request context in error logs.This aligns with the PR’s masking goals and reuses existing ID-caching patterns cleanly.
Also applies to: 3252-3277
3350-3354: Masked response-body logging on ID extraction failuresSwitching the error logs in
writeAutoGeneratedId(Response, ...)to:LogMaskingUtil.maskSensitiveData(response.asString())prevents raw response payloads (tokens, secrets, PII) from being dumped into logs while still giving enough context for troubleshooting. This is exactly in line with the masking objectives.
7167-7195: Response-time logging now masked before UTC check
checkResponseUTCTimenow logs the response viaLogMaskingUtil.maskSensitiveData(response.asString()), which avoids raw body leakage while preserving the existing UTC validation logic. This is a safe, targeted change aligned with the masking requirements.
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java
Outdated
Show resolved
Hide resolved
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (1)
22-32: JSON arrays (root-level and under sensitive keys) can leak secrets; extend masking logicRight now
maskSensitiveData(String json)assumes a JSON object; if the body is a valid JSON array (e.g.["secret1","secret2"]),new JSONObject(json)will throw and the catch block returns the raw string, so nothing is masked. Similarly, inmask(JSONObject jsonObject), when a value is aJSONArray, you recurse only intoJSONObjectelements and never mask primitive elements, even if the key itself is inSENSITIVE_KEYS(e.g.{"token": ["secret1","secret2"]}).This leaves several realistic cases (root-level arrays and arrays under sensitive keys) unmasked and defeats the purpose of this utility.
You can address both points by:
- Detecting whether the input starts with
{or[and parsing asJSONObjectorJSONArrayaccordingly.- Introducing a shared
maskArray(JSONArray arr, boolean maskPrimitiveValues)helper that recurses into nested objects/arrays and masks primitive elements when the parent key (or the root) is considered sensitive.For example:
@@ - public static String maskSensitiveData(String json) { - if (json == null) return null; - - try { - JSONObject obj = new JSONObject(json); - mask(obj); - return obj.toString(4); // pretty print - } catch (Exception e) { - return json; // In case the body is not JSON, just return raw - } - } + public static String maskSensitiveData(String json) { + if (json == null) { + return null; + } + + try { + String trimmed = json.trim(); + if (trimmed.startsWith("{")) { + JSONObject obj = new JSONObject(trimmed); + mask(obj); + return obj.toString(4); // pretty print + } else if (trimmed.startsWith("[")) { + JSONArray arr = new JSONArray(trimmed); + // Root-level arrays are treated as fully sensitive + maskArray(arr, true); + return arr.toString(4); // pretty print + } + // Not JSON, just return raw + return json; + } catch (Exception e) { + // In case the body is not JSON, just return raw + return json; + } + } + + private static void maskArray(JSONArray arr, boolean maskPrimitiveValues) { + for (int i = 0; i < arr.length(); i++) { + Object element = arr.get(i); + if (element instanceof JSONObject) { + mask((JSONObject) element); + } else if (element instanceof JSONArray) { + maskArray((JSONArray) element, maskPrimitiveValues); + } else if (maskPrimitiveValues) { + arr.put(i, maskValue(String.valueOf(element))); + } + } + } @@ - } else if (value instanceof JSONArray) { - JSONArray arr = (JSONArray) value; - for (int i = 0; i < arr.length(); i++) { - if (arr.get(i) instanceof JSONObject) { - mask(arr.getJSONObject(i)); - } - } + } else if (value instanceof JSONArray) { + JSONArray arr = (JSONArray) value; + // If the key is sensitive, also mask primitive array elements + boolean maskPrimitives = SENSITIVE_KEYS.contains(key); + maskArray(arr, maskPrimitives); } else { if (SENSITIVE_KEYS.contains(key)) { jsonObject.put(key, maskValue(value.toString())); } }This way:
- Root-level arrays are masked (including primitive strings).
- Arrays under sensitive keys (like
token,password) will have both nested objects and primitive elements masked.- Arrays under non-sensitive keys retain non-sensitive primitive values.
Also applies to: 34-52
🧹 Nitpick comments (2)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (2)
64-71: Clarify expectations forsafeLogInfoinput format
safeLogInfopasses the wholemessagethroughmaskSensitiveData(message), which only masks whenmessageis valid JSON (object/array). If callers ever pass mixed log lines such as"Request failed: " + jsonBody, the parser will treat it as non‑JSON and return the original string, leaking secrets.Either:
- Document that
safeLogInfomust be called with raw JSON only (and keep composition outside), or- Add a defensive fallback (e.g., detect and extract JSON segment, or apply header/param-style masking to the whole message).
Given this is a “safe” helper, tightening or documenting the contract would avoid surprises.
15-20: Consider using aSetforSENSITIVE_KEYSfor faster lookups
SENSITIVE_KEYSis aList, and lookups useSENSITIVE_KEYS.contains(key)in tight loops over JSON keys and headers. The list is small, so this is not a blocker, but converting to an unmodifiableSetwould give O(1) membership checks and slightly cleaner intent:-import java.util.List; +import java.util.List; +import java.util.Set; @@ - private static final List<String> SENSITIVE_KEYS = Arrays.asList( + private static final Set<String> SENSITIVE_KEYS = Set.of((Not essential, but worth considering while this utility is new.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java(1 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/RestAssuredPrettyLogger.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-maven-authentication-demo-service / maven-build
- GitHub Check: build-maven-apitest-commons / maven-build
- GitHub Check: Analyze (java-kotlin)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java
Outdated
Show resolved
Hide resolved
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (2)
20-30: Root-level JSON arrays remain unmasked despite previous resolution.The past review comment on lines 20-30 flagged that root-level JSON arrays are not masked and was marked as resolved, but the suggested fix is not present in the current code. If the input JSON is an array (e.g.,
["secret1", "secret2"]), parsing asJSONObjecton line 24 will throw an exception and return the raw, unmasked content on line 28.The suggested fix from the previous review should be applied:
public static String maskSensitiveData(String json) { if (json == null) return null; try { - JSONObject obj = new JSONObject(json); - mask(obj); - return obj.toString(4); // pretty print + // Try parsing as object first + if (json.trim().startsWith("{")) { + JSONObject obj = new JSONObject(json); + mask(obj); + return obj.toString(4); + } else if (json.trim().startsWith("[")) { + JSONArray arr = new JSONArray(json); + maskArray(arr); + return arr.toString(4); + } + return json; } catch (Exception e) { return json; // In case the body is not JSON, just return raw } } + +private static void maskArray(JSONArray arr) { + for (int i = 0; i < arr.length(); i++) { + if (arr.get(i) instanceof JSONObject) { + mask(arr.getJSONObject(i)); + } else if (arr.get(i) instanceof JSONArray) { + maskArray(arr.getJSONArray(i)); + } + } +}
38-44: Primitive string values in arrays are not masked.The array handling logic only masks
JSONObjectelements within arrays (lines 41-43) but ignores primitive values. If a sensitive key contains an array of strings (e.g.,{"tokens": ["secret1", "secret2"]}), the string elements will remain unmasked.Consider extending the masking logic to handle primitive string values:
} else if (value instanceof JSONArray) { JSONArray arr = (JSONArray) value; for (int i = 0; i < arr.length(); i++) { - if (arr.get(i) instanceof JSONObject) { + Object element = arr.get(i); + if (element instanceof JSONObject) { mask(arr.getJSONObject(i)); + } else if (element instanceof JSONArray) { + // Recursively handle nested arrays + mask(new JSONObject().put("temp", element)); + } else if (SENSITIVE_KEYS.contains(key) && element instanceof String) { + arr.put(i, maskValue(element.toString())); } } }
🧹 Nitpick comments (3)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (1)
12-12: Consider adding a private constructor and JavaDoc.As a utility class with only static methods, consider:
- Adding a private constructor to prevent instantiation
- Adding JavaDoc comments to document the masking behavior and usage
+/** + * Utility class for masking sensitive information in logs. + * Masks fields like passwords, tokens, and client secrets in JSON bodies, + * HTTP headers, and request parameters. + */ public class LogMaskingUtil { + + // Private constructor to prevent instantiation + private LogMaskingUtil() { + throw new UnsupportedOperationException("Utility class"); + }apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (2)
43-49: Expiry-date helper and tokens look correct; optional cleanup to stay purely onjava.timeThe new
$EXPIRYDATE$/$EXPIRYDATE_10YEARS$handling andgetUtcTimestamphelper correctly generate UTC timestamps in the same string format as existing timestamp utilities, so behavior is consistent.If you want to simplify later, you could avoid the
Date/SimpleDateFormatbridge and formatZonedDateTimedirectly via aDateTimeFormatter, but that’s purely cosmetic here.Also applies to: 3781-3786, 7436-7443
3365-3391:writeAutoGeneratedIdFromRequestcorrectly handles missingrequest; trim field names for robustnessThis helper now safely uses
optJSONObject("request"), guards the null case, and routes both success/error logs throughLogMaskingUtil, which aligns well with the PR’s masking goals.To make it a bit more tolerant of configuration typos, you could trim each field name after splitting on commas, so
"id1, id2"still works:- String[] fieldNames = idKeyName.split(","); - for (String filedName : fieldNames) { + String[] fieldNames = idKeyName.split(","); + for (String filedName : fieldNames) { + filedName = filedName.trim(); + if (filedName.isEmpty()) { + continue; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java(7 hunks)apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-maven-apitest-commons / maven-build
- GitHub Check: build-maven-authentication-demo-service / maven-build
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (5)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/LogMaskingUtil.java (2)
71-90: Verify the fallback masking strategy for unmatched headers.On line 89, if the header name doesn't match any sensitive key and isn't
set-cookie, the method falls back tomaskSensitiveData(value), which attempts to parsevalueas JSON. Ifvalueis a plain string (e.g., a bearer token or API key), it will be returned unmasked since it's not valid JSON.Please confirm whether this is the intended behavior. If unrecognized headers should remain unmasked (allow-list approach), the current logic is correct. However, if the goal is to mask all potentially sensitive headers defensively (deny-list approach), consider updating the fallback logic:
- return maskedValue.equals(value) ? maskSensitiveData(value) : maskedValue; + // If no masking was applied and value looks sensitive, mask it + return maskedValue.equals(value) && value.length() > 10 ? maskValue(value) : maskedValue;Alternatively, you can verify the intended behavior by checking how headers are used in
RestAssuredPrettyLoggerandRestClientto ensure the masking policy aligns with security requirements.
92-100: Clean implementation of parameter masking.The method correctly handles null/empty maps and leverages the header masking logic to mask parameter values. The stream-based formatting is clear and concise.
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (3)
3452-3467: Masked logging inwriteAutoGeneratedId(Response…)is aligned with log‑masking objectivesReplacing raw
response.asString()withLogMaskingUtil.maskSensitiveData(response.asString())in the error paths ensures whole-response logs are redacted without changing functional behavior.Looks good and consistent with the rest of the PR.
7282-7310:checkResponseUTCTimenow logs masked responses; behavior unchangedThe added call to
LogMaskingUtil.maskSensitiveData(response.asString())protects the response payload in this diagnostic log while preserving the existing UTC‑validation logic.No further changes needed here.
7436-7443:getUtcTimestampimplementation is correct and thread‑safeThe helper computes
now + amountToAddin UTC and formats it with a freshSimpleDateFormat, so there are no shared mutable date-format instances and no timezone surprises.All good as implemented.
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java
Show resolved
Hide resolved
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (1)
1265-1334: FixuserDefinedCookiebranch to removeCOOKIE_NAMEbefore re-serializing the bodyIn
postWithBodyAndCookieForAutoGeneratedIdFromRequest, theuserDefinedCookiebranch still does:
inputJson = request.toString();before removingGlobalConstants.COOKIE_NAME, so the outgoing JSON body can retaincookieName, unlike other helpers that strip bothCOOKIEandCOOKIE_NAME.This is the same issue already raised in a prior review and should be aligned with the fixed pattern above in
postWithBodyAndCookie.Suggested fix for this method:
- } else if (role.equals("userDefinedCookie")) { - JSONObject request = new JSONObject(inputJson); - if (request.has(GlobalConstants.COOKIE)) { - token = request.get(GlobalConstants.COOKIE).toString(); - request.remove(GlobalConstants.COOKIE); - inputJson = request.toString(); - if (request.has(GlobalConstants.COOKIE_NAME)) { - cookieName = request.get(GlobalConstants.COOKIE_NAME).toString(); - request.remove(GlobalConstants.COOKIE_NAME); - } - } - } else { + } else if (role.equals("userDefinedCookie")) { + JSONObject request = new JSONObject(inputJson); + if (request.has(GlobalConstants.COOKIE)) { + token = request.get(GlobalConstants.COOKIE).toString(); + request.remove(GlobalConstants.COOKIE); + if (request.has(GlobalConstants.COOKIE_NAME)) { + cookieName = request.get(GlobalConstants.COOKIE_NAME).toString(); + request.remove(GlobalConstants.COOKIE_NAME); + } + inputJson = request.toString(); + } + } else {Also consider applying the same ordering fix to the earlier
postWithBodyAndCookieForAutoGeneratedIdmethod, which has the same pattern.
🧹 Nitpick comments (3)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (3)
3736-3741: Expiry placeholders andgetUtcTimestampare consistent; verify token naming and duration choicesThe new handling:
- Replaces
$EXPIRYDATE$withgetUtcTimestamp(365, ChronoUnit.DAYS)and$EXPIRYDATE_10YEARS$withgetUtcTimestamp(10, ChronoUnit.YEARS), andgetUtcTimestampproduces a UTC ISO‑8601-style string (yyyy-MM-dd'T'HH:mm:ss.SSS'Z'), matching the format of existing timestamp helpers.Behavior looks correct, but two minor points to confirm:
- The PR description mentions
$EXPIRYDATE_YEARS$while the code uses$EXPIRYDATE_10YEARS$; ensure test templates and docs use the same token names.- For
$EXPIRYDATE$, you hard-code 365 days rather than 1 calendar year; if leap-year accuracy matters even in tests, you may preferChronoUnit.YEARSwith amount1.If these semantics match your test data, no further changes are needed.
Also applies to: 7390-7397
4755-4759: KEYWORD_DATA translation path mirrors FILTERS logic and looks soundThe updated
isDataRequiredbranch:
- Rebuilds the first
KEYWORD_DATAobject withvalueToConvertreplaced bytranslatedValue.- Wraps it back into a singleton array and reassigns
GlobalConstants.KEYWORD_DATAonlangjson.This mirrors the existing
FILTERShandling and keeps the structure[ { ...translated fields... } ]. Only caveat is thattoString().replace(...)will also replace incidental substring matches, but that’s consistent with prior code.
7390-7397:getUtcTimestampimplementation is correct; consider reusing java.time formatting
getUtcTimestampcorrectly:
- Starts from
ZonedDateTime.now(ZoneOffset.UTC),- Adds
amountToAddin the providedChronoUnit, and- Formats the result as a UTC
'Z'timestamp.Functionality is fine; if you ever revisit this, you could simplify by using
DateTimeFormatteron theZonedDateTimedirectly and avoid bouncing throughDate/SimpleDateFormat, but that’s purely a style/maintainability tweak.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-maven-apitest-commons / maven-build
- GitHub Check: build-maven-authentication-demo-service / maven-build
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (2)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (2)
3320-3346: Request-ID extraction from request body is now safe and masked; behavior looks correctThe new
writeAutoGeneratedIdFromRequesthelper:
- Guards against a missing
"request"block (requestObj != null ? ... : reqJson.toString()), avoiding the NPE risk called out in a prior review.- Supports multiple comma-separated field names and writes each to the existing cache using
getAutogenIdKeyName, consistent with the response-based variant.- Logs both successes and failures through
LogMaskingUtil.maskSensitiveData, so even when logging the wholerequest/root JSON, secrets are masked.The corresponding changes in
writeAutoGeneratedId(Response, ...)to mask the logged response body also align with the PR’s log-masking goal. No functional issues spotted here.Also applies to: 3417-3422
7236-7264: Masking full response body incheckResponseUTCTimealigns with logging goals
checkResponseUTCTimenow logs:logger.info(LogMaskingUtil.maskSensitiveData(response.asString()));instead of the raw body, which reduces accidental leakage of tokens/secrets in this diagnostic method. Time comparison logic is unchanged. No further issues here.
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (1)
1265-1334:userDefinedCookiebranch still leavesCOOKIE_NAMEin request JSON bodyIn
postWithBodyAndCookieForAutoGeneratedIdFromRequest,inputJsonis set torequest.toString()before removingCOOKIE_NAME, so the outgoing JSON can still contain that field. This is the same issue previously flagged for this method and can cause schema/validation mismatches.Consider aligning with the pattern used in
postWithBodyAndCookieby moving the serialization after both removals:} else if (role.equals("userDefinedCookie")) { JSONObject request = new JSONObject(inputJson); if (request.has(GlobalConstants.COOKIE)) { token = request.get(GlobalConstants.COOKIE).toString(); request.remove(GlobalConstants.COOKIE); - inputJson = request.toString(); if (request.has(GlobalConstants.COOKIE_NAME)) { cookieName = request.get(GlobalConstants.COOKIE_NAME).toString(); request.remove(GlobalConstants.COOKIE_NAME); } + inputJson = request.toString(); } } else {You may also want to mirror this fix in the existing
postWithBodyAndCookieForAutoGeneratedIdmethod above, which currently has the same sequencing.
🧹 Nitpick comments (3)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (3)
3367-3393: Request‑ID extraction helper is correct and masks logs; consider trimming field namesThe new
writeAutoGeneratedIdFromRequestcorrectly:
- Reads from an optional nested
"request"object.- Caches each requested field under the computed key.
- Uses
LogMaskingUtil.maskSensitiveDatafor both success and error logs, avoiding raw payload leakage.Functionally this looks good. Optionally, to be robust against
"idKeyName"strings like"id1, id2", you couldtrim()each split field name and skip empty entries before lookup.
3783-3788: Expiry tokens wired correctly; verify intended semantics for 1‑year vs 365‑day token
$EXPIRYDATE$is now resolved asnow + 365 DAYSand$EXPIRYDATE_10YEARS$asnow + 10 YEARSviagetUtcTimestamp, which is functionally sound. If callers conceptually treat$EXPIRYDATE$as “+1 year” rather than “+365 days”, you may want to switch that call toChronoUnit.YEARSfor consistency with the 10‑year variant; otherwise the current behavior is fine.
7438-7445:getUtcTimestampimplementation is correct; could be simplified withjava.timeformattingThe helper correctly computes
now(UTC) + amountToAddin the givenChronoUnitand formats it as an ISO‑like UTC string compatible with existing timestamp fields. If you ever want to reducejava.util.Dateusage, you could instead format theZonedDateTimedirectly with a cachedDateTimeFormatter, but that’s an optional cleanup and not required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-maven-apitest-commons / maven-build
- GitHub Check: build-maven-authentication-demo-service / maven-build
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (3)
apitest-commons/src/main/java/io/mosip/testrig/apirig/utils/AdminTestUtil.java (3)
42-49: Time-related imports align with new utilitiesAdditional
java.timeimports are consistent with usages ingetCurrentUTCTime/getUtcTimestamp; no issues from an API or dependency standpoint.
3465-3469: Masked error logging inwriteAutoGeneratedId(Response,…)Wrapping
response.asString()withLogMaskingUtil.maskSensitiveDatain the error paths ensures full response bodies aren’t logged in clear text when IDs can’t be found, which aligns with the log‑masking objectives.
7284-7312: UTC response‑time check now logs masked payloadsChanging
checkResponseUTCTimeto logLogMaskingUtil.maskSensitiveData(response.asString())instead of the raw body prevents unmasked responses from appearing in info‑level logs while preserving the existing UTC validation logic.
SradhaMohanty5899
changed the title
2 participants
Masking Sensitive Information (Client-secret key, Password, pwd, token, refreshtoken) etc. From Log.
Summary by CodeRabbit
New Features
Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.