Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit b6949332 authored by Remi NGUYEN VAN's avatar Remi NGUYEN VAN
Browse files

Allow localhost HTTP URLs for the capport API 

Allowing the capport API to be hosted on localhost makes it easy to
write fast, stable tests for the feature.
This was not possible because:
 - Pre-validation of the URL used Patterns.WEB_URL, which is
  over-restrictive and excludes domain names without TLD
 - NetworkMonitor needs the API to be hosted via HTTPS which is working
   as intended, however relaxing this requirement only for localhost
   (for testing) seems reasonable.

Bug: 156062304
Test: atest CaptivePortalApiTest in associated change
Change-Id: I5f2cdd02376785b152e5b9a6e798d797894ea45b
parent c416cfb4

src/android/net/ip/IpClient.java

+17 −3
Original line number Diff line number Diff line
@@ -64,7 +64,6 @@ import android.text.TextUtils; 
import android.util.LocalLog;

import android.util.Log;

import android.util.Pair;

import android.util.Patterns;

import android.util.SparseArray;



import androidx.annotation.NonNull;
@@ -86,6 +85,8 @@ import com.android.server.NetworkStackService.NetworkStackServiceManager; 
import java.io.FileDescriptor;

import java.io.PrintWriter;

import java.net.InetAddress;

import java.net.MalformedURLException;

import java.net.URL;

import java.nio.BufferUnderflowException;

import java.nio.ByteBuffer;

import java.util.ArrayList;
@@ -1255,9 +1256,9 @@ public class IpClient extends StateMachine { 
            }



            final String capportUrl = mDhcpResults.captivePortalApiUrl;

            // Uri.parse does no syntax check; do a simple regex check to eliminate garbage.

            // Uri.parse does no syntax check; do a simple check to eliminate garbage.

            // If the URL is still incorrect data fetching will fail later, which is fine.

            if (capportUrl != null && Patterns.WEB_URL.matcher(capportUrl).matches()) {

            if (isParseableUrl(capportUrl)) {

                NetworkInformationShimImpl.newInstance()

                        .setCaptivePortalApiUrl(newLp, Uri.parse(capportUrl));

            }
@@ -1295,6 +1296,19 @@ public class IpClient extends StateMachine { 
        return newLp;

    }



    private static boolean isParseableUrl(String url) {

        // Verify that a URL has a reasonable format that can be parsed as per the URL constructor.

        // This does not use Patterns.WEB_URL as that pattern excludes URLs without TLDs, such as on

        // localhost.

        if (url == null) return false;

        try {

            new URL(url);

            return true;

        } catch (MalformedURLException e) {

            return false;

        }

    }



    private static void addAllReachableDnsServers(

            LinkProperties lp, Iterable<InetAddress> dnses) {

        // TODO: Investigate deleting this reachability check.  We should be

src/com/android/server/connectivity/NetworkMonitor.java

+6 −1
Original line number Diff line number Diff line
@@ -2535,7 +2535,12 @@ public class NetworkMonitor extends StateMachine { 
            final String apiContent;

            try {

                final URL url = new URL(mCaptivePortalApiUrl.toString());

                if (!"https".equals(url.getProtocol())) {

                // Protocol must be HTTPS

                // (as per https://www.ietf.org/id/draft-ietf-capport-api-07.txt, #4).

                // Only allow HTTP on localhost, for testing.

                final boolean isLocalhostHttp =

                        "localhost".equals(url.getHost()) && "http".equals(url.getProtocol());

                if (!"https".equals(url.getProtocol()) && !isLocalhostHttp) {

                    validationLog("Invalid captive portal API protocol: " + url.getProtocol());

                    return null;

                }

tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java

+31 −1
Original line number Diff line number Diff line
@@ -1225,7 +1225,7 @@ public class NetworkMonitorTest { 
    @Test

    public void testIsCaptivePortal_CapportApiInvalidContent() throws Exception {

        assumeTrue(CaptivePortalDataShimImpl.isSupported());

        setStatus(mHttpsConnection, 204);

        setSslException(mHttpsConnection);

        setPortal302(mHttpConnection);

        setApiContent(mCapportApiConnection, "{SomeInvalidText");

        runNetworkTest(makeCapportLPs(), CELL_METERED_CAPABILITIES,
@@ -1236,6 +1236,36 @@ public class NetworkMonitorTest { 
        verify(mHttpConnection).getResponseCode();

    }



    private void runCapportApiInvalidUrlTest(String url) throws Exception {

        assumeTrue(CaptivePortalDataShimImpl.isSupported());

        setSslException(mHttpsConnection);

        setPortal302(mHttpConnection);

        final LinkProperties lp = new LinkProperties(TEST_LINK_PROPERTIES);

        lp.setCaptivePortalApiUrl(Uri.parse(url));

        runNetworkTest(makeCapportLPs(), CELL_METERED_CAPABILITIES,

                VALIDATION_RESULT_PORTAL, 0 /* probesSucceeded */,

                TEST_LOGIN_URL);



        verify(mCallbacks, never()).notifyCaptivePortalDataChanged(any());

        verify(mCapportApiConnection, never()).getInputStream();

        verify(mHttpConnection).getResponseCode();

    }



    @Test

    public void testIsCaptivePortal_HttpIsInvalidCapportApiScheme() throws Exception {

        runCapportApiInvalidUrlTest("http://capport.example.com");

    }



    @Test

    public void testIsCaptivePortal_FileIsInvalidCapportApiScheme() throws Exception {

        runCapportApiInvalidUrlTest("file://localhost/myfile");

    }



    @Test

    public void testIsCaptivePortal_InvalidUrlFormat() throws Exception {

        runCapportApiInvalidUrlTest("ThisIsNotAValidUrl");

    }



    @Test @IgnoreUpTo(Build.VERSION_CODES.Q)

    public void testIsCaptivePortal_CapportApiNotSupported() throws Exception {

        // Test that on a R+ device, if NetworkStack was compiled without CaptivePortalData support