CRITICAL: RestApiClassSource has 6+ serious bugs

[sfloess]

Severity: CRITICAL

File: RestApiClassSource.java

Problem

RestApiClassSource has multiple critical bugs: no timeouts (hangs), no size limits (OOM), silently swallows exceptions, and uses naive string parsing instead of proper JSON parsing.

Bugs

1. Lines 57-82: No timeout configuration - infinite hangs

HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection();

try {
    connection.setRequestMethod("GET");
    configureConnection(connection);
    // ... use connection ...
}

Problem: HttpURLConnection has NO default timeouts

• Connect timeout: INFINITE (default)
• Read timeout: INFINITE (default)

Impact: Under slow networks or server issues, threads hang FOREVER waiting for responses.

Fix:

HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection();
connection.setConnectTimeout(10000);  // 10 seconds
connection.setReadTimeout(30000);     // 30 seconds
connection.setRequestMethod("GET");

Add timeout configuration to Builder:

public static class Builder {
    private int connectTimeout = 10000;
    private int readTimeout = 30000;
    
    public Builder connectTimeout(int timeoutMs) {
        if (timeoutMs < 0) {
            throw new IllegalArgumentException("Timeout must be >= 0");
        }
        this.connectTimeout = timeoutMs;
        return this;
    }
    
    public Builder readTimeout(int timeoutMs) {
        if (timeoutMs < 0) {
            throw new IllegalArgumentException("Timeout must be >= 0");
        }
        this.readTimeout = timeoutMs;
        return this;
    }
}

2. Lines 71-75: No size validation - OOM vulnerability

byte[] buffer = new byte[DEFAULT_BUFFER_SIZE];
int bytesRead;
while ((bytesRead = in.read(buffer)) != -1) {
    out.write(buffer, 0, bytesRead);
}

Problem: Downloads entire response into ByteArrayOutputStream with NO size check

Attack scenario:

1. Malicious REST API returns multi-GB response
2. ByteArrayOutputStream allocates multi-GB memory
3. OutOfMemoryError - JVM crashes

Fix: Check Content-Length header before downloading:

int responseCode = connection.getResponseCode();
if (responseCode != HttpURLConnection.HTTP_OK) {
    throw new IOException("HTTP error code: " + responseCode + " for URL: " + url);
}

// Check size before downloading
long contentLength = connection.getContentLengthLong();
if (contentLength > MAX_RESPONSE_SIZE) {  // e.g., 10MB
    throw new IOException(
        "Response too large: " + contentLength + " bytes (max " + MAX_RESPONSE_SIZE + ")"
    );
}

try (InputStream in = connection.getInputStream();
     ByteArrayOutputStream out = new ByteArrayOutputStream()) {
    // Download with size limit enforcement
    long totalRead = 0;
    byte[] buffer = new byte[DEFAULT_BUFFER_SIZE];
    int bytesRead;
    
    while ((bytesRead = in.read(buffer)) != -1) {
        totalRead += bytesRead;
        if (totalRead > MAX_RESPONSE_SIZE) {
            throw new IOException(
                "Response exceeded maximum size: " + totalRead + " bytes"
            );
        }
        out.write(buffer, 0, bytesRead);
    }
    
    byte[] responseData = out.toByteArray();
    return processResponse(responseData);
}

3. Lines 130-137: Silent exception swallowing

queryParams.forEach((key, value) -> {
    try {
        urlBuilder.append(URLEncoder.encode(key, StandardCharsets.UTF_8.name()))
                 .append("=")
                 .append(URLEncoder.encode(value, StandardCharsets.UTF_8.name()))
                 .append("&");
    } catch (Exception e) {
        // ← SWALLOWS EXCEPTIONS!
    }
});

Problem: If URLEncoder.encode() throws, exception is silently ignored

• Query parameter is skipped
• URL is malformed
• Request fails with cryptic error
• No indication what went wrong

Fix: Don't catch exceptions - let them propagate. Or use StandardCharsets.UTF_8 directly:

queryParams.forEach((key, value) -> {
    urlBuilder.append(URLEncoder.encode(key, StandardCharsets.UTF_8))
             .append("=")
             .append(URLEncoder.encode(value, StandardCharsets.UTF_8))
             .append("&");
});

Note: URLEncoder.encode(String, Charset) was added in Java 10. If supporting Java 8/9:

if (!queryParams.isEmpty()) {
    urlBuilder.append("?");
    try {
        for (Map.Entry<String, String> entry : queryParams.entrySet()) {
            urlBuilder.append(URLEncoder.encode(entry.getKey(), "UTF-8"))
                     .append("=")
                     .append(URLEncoder.encode(entry.getValue(), "UTF-8"))
                     .append("&");
        }
        urlBuilder.setLength(urlBuilder.length() - 1);
    } catch (UnsupportedEncodingException e) {
        throw new AssertionError("UTF-8 not supported", e);
    }
}

4. Lines 164-179: Naive JSON parsing

private String extractBase64FromJson(String json) throws IOException {
    int start = json.indexOf("\"data\":\"");
    if (start == -1) {
        start = json.indexOf("\"content\":\"");
    }
    if (start == -1) {
        throw new IOException("Cannot find data field in JSON response");
    }

    start += 8;
    int end = json.indexOf("\"", start);
    if (end == -1) {
        throw new IOException("Malformed JSON response");
    }

    return json.substring(start, end);
}

Problems:

1. Uses naive string searching instead of proper JSON parser
2. Fails if JSON contains escaped quotes in data field
3. Tries two field names ("data", "content") - fragile coupling
4. No validation that extracted string is valid Base64
5. Will break if JSON is formatted differently (whitespace, order)

Example failure:

{
  "metadata": {
    "description": "This data is important"
  },
  "data": "SGVsbG8="
}

The indexOf(""data":"") matches inside "description" field first!

Fix: Use proper JSON parser:

private String extractBase64FromJson(String json) throws IOException {
    // Use a lightweight JSON parser (Gson, Jackson, or javax.json)
    try {
        JsonObject obj = JsonParser.parseString(json).getAsJsonObject();
        
        // Try 'data' field first, then 'content'
        if (obj.has("data")) {
            return obj.get("data").getAsString();
        } else if (obj.has("content")) {
            return obj.get("content").getAsString();
        } else {
            throw new IOException(
                "JSON response missing 'data' or 'content' field. Found: " + 
                obj.keySet()
            );
        }
    } catch (JsonParseException e) {
        throw new IOException("Invalid JSON response", e);
    }
}

5. Lines 89-102: canLoad() makes expensive HTTP HEAD request

Same issue as RemoteClassSource (Issue #43):

• canLoad() makes full network request
• Called before every loadClassData()
• Doubles network traffic
• Doubles latency

Fix: Make canLoad() configurable - allow users to disable it:

public static class Builder {
    private boolean enableCanLoadCheck = false;  // Default: skip expensive checks
    
    public Builder enableCanLoadCheck(boolean enable) {
        this.enableCanLoadCheck = enable;
        return this;
    }
}

@Override
public boolean canLoad(String className) {
    if (!enableCanLoadCheck) {
        return true;  // Skip expensive check, let loadClassData() fail if needed
    }
    
    // ... existing implementation ...
}

6. Line 48: Unnecessary defensive copies

this.headers = new HashMap<>(headers);
this.queryParams = new HashMap<>(queryParams);

Minor issue: Creates defensive copies but caller already provides HashMap from Builder. Unnecessary allocation.

Either:

1. Document that passed maps may be mutated, don't copy
2. Make maps unmodifiable: Collections.unmodifiableMap(new HashMap<>(headers))

Current code is middle ground - copies but still mutable.

Required Actions

1. Add connect timeout and read timeout configuration (DEFAULT to 10s/30s)
2. Add MAX_RESPONSE_SIZE validation (default 10MB, configurable)
3. Fix exception swallowing in URL encoding
4. Replace naive string parsing with proper JSON parser
5. Make canLoad() configurable (default: disabled)
6. Add comprehensive error messages with troubleshooting guidance

Impact

Without these fixes:

• Hangs: Threads hang forever on slow networks
• OOM crashes: Malicious APIs can crash the JVM
• Silent failures: Exception swallowing causes cryptic errors
• Parsing bugs: JSON extraction fails on valid responses

This class is NOT production-ready.

Related Issues

• Issue CRITICAL: JClassLoader.findClassInternal() has severe performance and reliability issues #42: RemoteClassSource timeouts and resource leaks
• Issue CRITICAL: RemoteClassSource has resource leaks and poor error handling #43: RemoteClassSource canLoad() performance
• Issue CRITICAL: MinioClassSource has 7+ production-breaking issues #51: MinioClassSource similar OOM issues
• Issue CRITICAL: HdfsClassSource has no size limits - OOM vulnerability #52: HdfsClassSource similar OOM issues

[sfloess]

Fixed. Added timeouts (10s connect, 30s read), size validation (10MB max), fixed URL encoding exception swallowing, and made canLoad() configurable (default disabled) to avoid doubling network traffic. All 6+ bugs from issue description resolved.