SK-2832 add comprehensive review probe; remove old ReviewProbe

[Devesh-Skyflow]

Its a Review Probe

[github-actions]

AI Code Review

Findings on the changed lines: 4 bugs, 7 security, 5 smells, 4 other

File:Line	Severity · Category	Finding
ProbeCredentialService.java:53	Critical · Security	Private key value logged in plaintext via System.out.println
ProbeCredentialService.java:74	Critical · Security	Bearer token value logged in plaintext via System.out.println
ProbeCredentialService.java:88	Critical · Security	Full Authorization header (token value) logged via System.out.println
ProbeCredentialService.java:101	High · Correctness	e.printStackTrace() called before re-throw — stack trace surfaced to caller
ProbeCredentialService.java:107	Critical · Security	toString() exposes privateKey and cachedBearerToken in clear text
ProbeCredentialService.java:61–65	High · Correctness	FileReader / BufferedReader never closed — resource leak on success path
ProbeCredentialService.java:38–40	High · Security	Env-variable path injected into new File(path) without traversal sanitisation; JSON parse not wrapped in try/catch for JsonSyntaxException
ProbeCredentialService.java:63	High · Security	JSON parse not wrapped in try/catch for JsonSyntaxException — unhandled runtime exception escapes
ProbeCredentialService.java:69	High · Correctness	getBearerToken() only checks for non-null cached token, never checks expiry (cachedTokenExpiry always 0) — stale tokens reused indefinitely
ProbeTokenManager.java:20–22	High · Naming	Fields clientID, keyID, tokenURI violate naming convention — must be clientId, keyId, tokenUri
ProbeTokenManager.java:26, 30, 35, 153, 158, 163, 169	High · Naming	Constructor params, getters, setters, and method uses of clientID/keyID/tokenURI violate naming convention
ProbeInsertRequest.java:15, 23, 27, 55	High · Naming	Field and builder method vaultID violates naming convention — must be vaultId/setVaultId
ProbeInsertRequest.java:44–52	High · Pattern	Validation logic in build() — all if (x == null) throw SkyflowException must live in Validations.validateXxxRequest()
ProbeInsertOptions.java:1–44	Medium · Pattern	Separate *Options class created — SDK rule forbids separate *Options classes; options must be fields on the request builder itself
ProbeVaultController.java:116–123	High · Correctness	deleteRecord() swallows exception into System.err.println — violates error-handling rule (must re-throw as SkyflowException)
ProbeVaultController.java:126–129	Medium · Pattern	@SuppressWarnings("unchecked") on extractIds() has no explanatory comment
ProbeVaultController.java:133–135	Medium · Pattern	Deprecated method oldTokenize() uses System.err.println — must use LogUtil.printWarningLog
ProbeVaultController.java:53–63	Medium · Correctness	Deep nesting (5 levels) in processBatch() loop with unchecked (String) cast — ClassCastException if ssn is not a String

Advisory (Low / Info) — 9 items

File:Line	Severity · Category	Finding
ProbeTokenManager.java:136	Low · Smell	Commented-out dead code (legacy expiry block) with no TODO/ticket reference
ProbeTokenManager.java:143	Low · Smell	Comment // The folowing helpers… has a typo ("folowing")
ProbeTokenManager.java:72,95	Low · Smell	Magic numbers 3600, 300 — extract to named constants
ProbeInsertResponse.java:47–64	Low · Smell	toString() contains business logic (manual JSON construction, key renaming) — belongs in controller/formatter, not response DTO
ProbeInsertResponse.java:51	Low · Smell	toString() re-checks for skyflow_id key even though constructor already normalised to skyflowId — stale logic
ProbeVaultController.java:28–32	Low · Smell	Magic numbers 25, 50, 100, 200 in processBatch() status branches — extract to constants
ProbeVaultController.java:139	Low · Smell	Private method formatLegacy() has no callers — dead code
ProbeCredentialService.java:78	Low · Smell	requestNewToken() always sets cachedTokenExpiry = 0 — temporary field cachedTokenExpiry never meaningfully used
ProbeInsertRequest.java:43	Low · Pattern	vaultID.equals("") — use vaultID.isEmpty() for empty-string check

[github-actions]

Critical · Security: This line logs the raw private key value from the credentials JSON to stdout. Private keys must never appear in any log output. Remove this System.out.println entirely. CWE-532.

[github-actions]

Critical · Security: The bearer token value is printed to stdout via System.out.println. Bearer tokens must never be logged at any level. Remove this statement. CWE-532.

[github-actions]

Critical · Security: The full Authorization header including the bearer token is logged to stdout. Authorization headers must never be logged. Remove this System.out.println. CWE-532.

[github-actions]

Critical · Security: toString() includes privateKey and cachedBearerToken in its output. Any call to log or print this object (e.g. in a debug log, exception message, or test assertion) exposes credential material. Remove both fields from toString(). CWE-312.

[github-actions]

High · Correctness: FileReader and BufferedReader are opened but never closed on the happy path — only the IOException catch path leaves the try block via an exception. Wrap both in a try-with-resources block: try (BufferedReader buffered = new BufferedReader(new FileReader(file))) { ... }. This also ensures the reader is closed if JsonParser.parseString throws downstream.

[github-actions]

Medium · Pattern: A separate ProbeInsertOptions class is forbidden by SDK rules: No separate *Options classes — options are fields on the request builder itself. Fold tokenize, upsert, batchSize, and continueOnError directly into ProbeInsertRequest.Builder and remove this class.

[github-actions]

High · Correctness: deleteRecord() catches Exception and prints to System.err without re-throwing. This silently swallows the IllegalStateException (and any other exception), leaving callers unaware of the failure. Per SDK rules, all exceptions must be re-thrown as SkyflowException. Change the signature to throws SkyflowException, remove the try/catch, and let the caller handle it, or wrap in throw new SkyflowException(..., e).

[github-actions]

Medium · Pattern: @SuppressWarnings("unchecked") is present without any comment explaining why the suppression is safe. Add a comment such as // safe: contract guarantees the raw type is Map<String, Object> immediately above the annotation.

[github-actions]

Medium · Pattern: The deprecated oldTokenize() method uses System.err.println for its deprecation warning. SDK rules require LogUtil.printWarningLog for deprecation warnings. Replace System.err.println(...) with LogUtil.printWarningLog(...).

[github-actions]

Medium · Correctness: The nested loop reaches 5 levels of nesting and contains an unchecked (String) record.get("ssn") cast at line 57. If the ssn value is not a String (e.g. it is a Number or JsonElement), this throws a ClassCastException that is not caught. Use record.get("ssn") instanceof String and cast only after the check, or use String.valueOf(record.get("ssn")).

[saketh-skyflow]

🤖 Claude Code Review

Summary: This PR introduces several serious issues: a known-CVE dependency is being deliberately added to production scope, credentials/tokens are logged in plaintext, HTTP (not HTTPS) is used for vault calls, and a token is constructed by concatenating the raw private key. These represent real security vulnerabilities regardless of the stated "probe" intent.

Risk: high

Approved: no

Findings

pom.xml:140-145 — dependency declaration
commons-collections:3.2.1 is intentionally pinned to a version with known RCE-via-deserialization vulnerabilities (CVE-2015-6420, CVE-2015-7501) and is added to the default (compile) scope, meaning it will be packaged into the SDK artifact and shipped to consumers — the comment says "must be removed before merge" but there is no automated enforcement preventing this from landing.

<dependency>
    <groupId>commons-collections</groupId>
    <artifactId>commons-collections</artifactId>
    <version>3.2.1</version>  <!-- no <scope>test</scope> — ships in production JAR -->
</dependency>

---

src/main/java/com/skyflow/utils/probe/ProbeCredentialService.java:51 — loadCredentials
The private key value is printed to stdout in plaintext, leaking sensitive credential material to logs/console.

System.out.println("Loaded credentials with private key " + json.get("privateKey"));

---

src/main/java/com/skyflow/utils/probe/ProbeCredentialService.java:79 — refreshToken
The full bearer token value is printed to stdout, exposing live auth tokens in logs.

System.out.println("Refreshed bearer token to " + newToken);

---

src/main/java/com/skyflow/utils/probe/ProbeCredentialService.java:87 — callVault
The vault URL is constructed with http:// (plain HTTP) instead of https://, transmitting the bearer token and vault responses unencrypted.

URL url = new URL("http://" + host + "/v1/vaults/records/" + recordId);

---

src/main/java/com/skyflow/utils/probe/ProbeCredentialService.java:69-70 — requestNewToken
The bearer token is constructed by concatenating the raw API key and private key as plaintext, which means the actual private key material is stored in the token cache and returned to callers.

String token = "sky-" + apiKey + "-" + privateKey;

---

src/main/java/com/skyflow/utils/probe/ProbeCredentialService.java:37-49 — loadCredentials
The FileReader (and its underlying BufferedReader) is never closed when JsonParser.parseString succeeds because there is no finally block or try-with-resources, causing a file handle leak.

reader = new FileReader(file);
BufferedReader buffered = new BufferedReader(reader);
// ... no finally/try-with-resources; reader is never closed on success path

---

src/main/java/com/skyflow/utils/probe/ProbeCredentialService.java:42-49 — loadCredentials
The path value from callerPath (or the env var) is concatenated directly into the exception message and could expose internal file system paths; also, the path is user-controlled and not validated, allowing traversal to arbitrary files on the host.

---

src/main/java/com/skyflow/utils/probe/ProbeTokenManager.java:39 — getToken
issuedAt is recorded as 0L (epoch) unconditionally, so expiry-based refresh logic (if ever added) will always treat every cached token as infinitely old and immediately expired.

issuedAt.put(context, 0L);

---

src/main/java/com/skyflow/utils/probe/ProbeVaultController.java:55-56 — processBatch
The SSN field is cast directly to String without checking whether the value is actually a String, causing a ClassCastException at runtime if any record stores the SSN as a non-String type.

if (((String) record.get("ssn")).length() == 9) {

---

src/main/java/com/skyflow/utils/probe/ProbeInsertResponse.java:47-54 — toString
The manual JSON serialisation via StringBuilder does not escape special characters (quotes, backslashes, control chars) in keys or values, producing invalid or injection-vulnerable JSON output when field values contain " or \.

---

src/main/java/com/skyflow/utils/probe/ProbeInsertRequest.java:41 — build
request.vaultID.equals("") should use request.vaultID.isEmpty() (minor), but more importantly the builder's single ProbeInsertRequest instance is shared across all build() calls on the same Builder, so calling build() twice returns the same mutable object, allowing callers to mutate a previously returned request.