ADFA-4954 fix(kolibri): validate the node id, and cover the one class that talks to the network

Two findings from reviewing the previous commit.

fetchTree interpolated its argument straight into a URL path. No caller reaches
it with anything but an already-normalised Channel.rootNodeId(), but the method
is public and a value carrying a slash or a query string would have addressed
somewhere else entirely. It now normalises first and fails closed, the way
ModuleName, AdbShellCommand and ArchiveEntry do at their own boundaries.

StudioTreeSource had no test. The previous commit reported 99 passing tests and
every one of them was pure logic, so the only class in the slice that touches
the network was the only one unverified — while mockwebserver was already a test
dependency and HttpOutboundSenderTest already showed the pattern. Adds 11 tests
over MockWebServer: the happy path and the request path actually built, id
normalisation, six rejected ids that must never become a request, 404, 503,
non-JSON, empty body, a 9 MB body against the 8 MB ceiling, a paged child level,
and a base URL with no trailing slash.

Also readAll now uses try-with-resources; a read() that threw part-way leaked
the stream.

Author: luisguzman-adfa

controller/app/src/main/java/org/iiab/controller/kolibri/data/StudioTreeSource.java

@@ -11,6 +11,7 @@
 
 import android.util.Log;
 
+import org.iiab.controller.kolibri.domain.ChannelId;
 import org.iiab.controller.kolibri.domain.TopicNode;
 import org.json.JSONObject;
 
@@ -66,10 +67,17 @@ public StudioTreeSource(String baseUrl) {
      *         a usable node
      */
     public TopicNode fetchTree(String nodeId) {
-        if (nodeId == null || nodeId.trim().isEmpty()) {
+        // Validate before the id reaches a URL path, not after. Every caller
+        // today passes an already-normalised Channel.rootNodeId(), but this
+        // method is public and a value with a slash or a query string in it
+        // would build a request to somewhere else entirely. Fail closed, the
+        // way ModuleName, AdbShellCommand and ArchiveEntry do at their own
+        // boundaries.
+        String id = ChannelId.normalise(nodeId);
+        if (id == null) {
             return null;
         }
-        String url = baseUrl + "/api/public/v2/contentnode_tree/" + nodeId.trim();
+        String url = baseUrl + "/api/public/v2/contentnode_tree/" + id;
         try {
             String body = httpGet(url);
             if (body.isEmpty()) {
@@ -105,19 +113,21 @@ private static String readAll(InputStream is) throws Exception {
         if (is == null) {
             return "";
         }
-        ByteArrayOutputStream buf = new ByteArrayOutputStream();
-        byte[] chunk = new byte[8192];
-        int n;
-        int total = 0;
-        while ((n = is.read(chunk)) != -1) {
-            total += n;
-            if (total > MAX_BYTES) {
-                is.close();
-                throw new Exception("response over " + (MAX_BYTES / (1024 * 1024)) + " MB");
+        // try-with-resources rather than a close() at each exit: a read() that
+        // throws part-way used to leak the stream.
+        try (InputStream in = is) {
+            ByteArrayOutputStream buf = new ByteArrayOutputStream();
+            byte[] chunk = new byte[8192];
+            int n;
+            int total = 0;
+            while ((n = in.read(chunk)) != -1) {
+                total += n;
+                if (total > MAX_BYTES) {
+                    throw new Exception("response over " + (MAX_BYTES / (1024 * 1024)) + " MB");
+                }
+                buf.write(chunk, 0, n);
             }
-            buf.write(chunk, 0, n);
+            return buf.toString(StandardCharsets.UTF_8.name());
         }
-        is.close();
-        return buf.toString(StandardCharsets.UTF_8.name());
     }
 }

controller/app/src/test/java/org/iiab/controller/kolibri/data/StudioTreeSourceTest.java

@@ -0,0 +1,161 @@
+package org.iiab.controller.kolibri.data;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+
+import okhttp3.mockwebserver.MockResponse;
+import okhttp3.mockwebserver.MockWebServer;
+import okhttp3.mockwebserver.RecordedRequest;
+
+import org.iiab.controller.kolibri.domain.TopicNode;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Unit tests for {@link StudioTreeSource} against a local {@link MockWebServer},
+ * following {@code HttpOutboundSenderTest}.
+ *
+ * <p>This is the only class in the Kolibri catalog slice that touches the
+ * network, so it is the only one whose failure modes cannot be reasoned about
+ * from pure logic: what it does with a 404, with a body that is not JSON, with a
+ * response too large to hold on a phone, and — the one that matters for security
+ * — what it does with a node id that is not a node id.
+ *
+ * <p>Runs on a plain JVM: {@code android.util.Log} is stubbed by the module's
+ * {@code testOptions.unitTests.returnDefaultValues}.
+ */
+public class StudioTreeSourceTest {
+
+    private static final String CH = "c150ea1d69495d37b5b0ac6f017e9bfb";
+    private static final String NODE = "23a7dc9c73635cd2abbd3e8aab13c3ca";
+
+    private MockWebServer server;
+    private StudioTreeSource source;
+
+    @Before
+    public void setUp() throws Exception {
+        server = new MockWebServer();
+        server.start();
+        // Trailing slash on purpose: the constructor has to strip it, or every
+        // URL it builds carries a double slash.
+        source = new StudioTreeSource(server.url("/").toString());
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        server.shutdown();
+    }
+
+    private static MockResponse json(String body) {
+        return new MockResponse().setResponseCode(200)
+                .setHeader("Content-Type", "application/json").setBody(body);
+    }
+
+    private static String treeJson() {
+        return "{\"id\":\"" + CH + "\",\"title\":\"3asafeer\",\"kind\":\"topic\","
+                + "\"is_leaf\":false,\"lft\":1,\"rght\":360,\"children\":{\"more\":null,"
+                + "\"results\":[{\"id\":\"" + NODE + "\",\"title\":\"Beginner\","
+                + "\"kind\":\"video\",\"is_leaf\":true,"
+                + "\"files\":[{\"file_size\":800,\"preset\":\"high_res_video\"}]}]}}";
+    }
+
+    @Test
+    public void readsATreeAndHitsTheExpectedPath() throws Exception {
+        server.enqueue(json(treeJson()));
+
+        TopicNode t = source.fetchTree(CH);
+        assertEquals(CH, t.id());
+        assertEquals(1, t.children().size());
+        assertEquals(800L, t.subtreeBytes());
+        assertEquals(179, t.descendantCount());
+
+        RecordedRequest req = server.takeRequest();
+        assertEquals("/api/public/v2/contentnode_tree/" + CH, req.getPath());
+        assertEquals("GET", req.getMethod());
+        assertEquals("application/json", req.getHeader("Accept"));
+    }
+
+    @Test
+    public void aHyphenatedIdIsNormalisedBeforeItReachesTheUrl() throws Exception {
+        server.enqueue(json(treeJson()));
+        source.fetchTree("c150ea1d-6949-5d37-b5b0-ac6f017e9bfb");
+        assertEquals("/api/public/v2/contentnode_tree/" + CH,
+                server.takeRequest().getPath());
+    }
+
+    @Test
+    public void anIdThatIsNotANodeIdNeverBecomesARequest() throws Exception {
+        // The point of validating before building the URL: a value carrying a
+        // slash or a query string would address something else entirely.
+        for (String bad : Arrays.asList("../../admin", "abc?x=1", "bisan-sukod",
+                "", "   ", null)) {
+            assertNull("expected null for " + bad, source.fetchTree(bad));
+        }
+        assertEquals("no request should have been made", 0, server.getRequestCount());
+    }
+
+    @Test
+    public void aNotFoundIsNullRatherThanAnException() throws Exception {
+        server.enqueue(new MockResponse().setResponseCode(404).setBody("{\"detail\":\"nope\"}"));
+        assertNull(source.fetchTree(CH));
+    }
+
+    @Test
+    public void aServerErrorIsNull() throws Exception {
+        server.enqueue(new MockResponse().setResponseCode(503));
+        assertNull(source.fetchTree(CH));
+    }
+
+    @Test
+    public void aBodyThatIsNotJsonIsNull() throws Exception {
+        server.enqueue(json("<html>maintenance</html>"));
+        assertNull(source.fetchTree(CH));
+    }
+
+    @Test
+    public void anEmptyBodyIsNull() throws Exception {
+        server.enqueue(json(""));
+        assertNull(source.fetchTree(CH));
+    }
+
+    @Test
+    public void jsonWithoutAUsableIdIsNull() throws Exception {
+        server.enqueue(json("{\"title\":\"no id here\"}"));
+        assertNull(source.fetchTree(CH));
+    }
+
+    @Test
+    public void anOversizedResponseIsRefusedInsteadOfFillingTheHeap() throws Exception {
+        // A phone must not try to hold an unbounded body. 9 MB is over the 8 MB
+        // ceiling; the read aborts and the caller gets null.
+        byte[] filler = new byte[9 * 1024 * 1024];
+        Arrays.fill(filler, (byte) 'x');
+        server.enqueue(json("{\"pad\":\"" + new String(filler, StandardCharsets.UTF_8) + "\"}"));
+        assertNull(source.fetchTree(CH));
+    }
+
+    @Test
+    public void aPagedChildLevelComesBackWithAnUnknownSubtreeSize() throws Exception {
+        server.enqueue(json("{\"id\":\"" + CH + "\",\"kind\":\"topic\",\"is_leaf\":false,"
+                + "\"children\":{\"more\":{\"cursor\":\"abc\"},\"results\":[{\"id\":\""
+                + NODE + "\",\"is_leaf\":true,\"files\":[{\"file_size\":10}]}]}}"));
+        TopicNode t = source.fetchTree(CH);
+        assertEquals(1, t.children().size());
+        assertFalse(t.hasSubtreeSize());
+    }
+
+    @Test
+    public void aBaseUrlWithoutATrailingSlashWorksToo() throws Exception {
+        String noSlash = server.url("/").toString().replaceAll("/$", "");
+        server.enqueue(json(treeJson()));
+        assertTrue(new StudioTreeSource(noSlash).fetchTree(CH) != null);
+        assertEquals("/api/public/v2/contentnode_tree/" + CH,
+                server.takeRequest().getPath());
+    }
+}