ABP/Android proxy service

src/org/adblockplus/brazil/RequestHandler.java

 package org.adblockplus.brazil;
 
 import java.io.EOFException;
 import java.io.FilterInputStream;
 import java.io.FilterOutputStream;
 import java.io.IOException;
 import java.io.InterruptedIOException;
 import java.io.OutputStream;
 import java.net.ConnectException;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.UnknownHostException;
 import java.util.List;
-import java.util.Properties;
+import java.util.regex.Pattern;
 import java.util.zip.GZIPInputStream;
 import java.util.zip.InflaterInputStream;
 
 import org.adblockplus.ChunkedOutputStream;
 import org.adblockplus.android.AdblockPlus;
 import org.literateprograms.BoyerMoore;
 
-import sunlabs.brazil.server.Handler;
 import sunlabs.brazil.server.Request;
 import sunlabs.brazil.server.Server;
 import sunlabs.brazil.util.MatchString;
 import sunlabs.brazil.util.http.HttpInputStream;
 import sunlabs.brazil.util.http.HttpRequest;
 import sunlabs.brazil.util.http.MimeHeaders;
 import android.util.Log;
 
 /**
  * The <code>RequestHandler</code> implements a proxy service optionally
@@ skipping 22 matching lines @@
  * 
  * <pre>
  * handler=adblock
  * adblock.class=org.adblockplus.brazil.RequestHandler
  * </pre>
  * 
  * See the description under {@link sunlabs.brazil.server.Handler#respond
  * respond} for a more detailed explanation.
  */
 
-public class RequestHandler implements Handler
+public class RequestHandler extends BaseRequestHandler
 {
-  public static final String PROXY_HOST = "proxyHost";
-  public static final String PROXY_PORT = "proxyPort";
-  public static final String AUTH = "auth";
-
   private AdblockPlus application;
-  private String prefix;
-
   private String via;
-
-  private String proxyHost;
-  private int proxyPort = 80;
-  private String auth;
-
-  private boolean shouldLog; // if true, log all headers

[Felix Dahlke, 2012/11/09 06:04:32]

Maybe call it shouldLogHeaders then? That'd make the comment redundant. I'm not
a fan of comments where naming suffices, I've seen them get invalid over time
way too often.

[Andrey Novikov, 2012/11/09 09:23:47]

Done.

+  private static Pattern RE_HTTP = Pattern.compile("^https?:");
+
+  private boolean shouldLogHeaders;
 
   @Override
   public boolean init(Server server, String prefix)
   {
-    this.prefix = prefix;
+    super.init(server, prefix);
+
     application = AdblockPlus.getApplication();
-
-    Properties props = server.props;
-
-    proxyHost = props.getProperty(prefix + PROXY_HOST);
-
-    String s = props.getProperty(prefix + PROXY_PORT);
-    try
-    {
-      proxyPort = Integer.decode(s).intValue();
-    }
-    catch (Exception e)
-    {

[Felix Dahlke, 2012/11/09 06:04:32]

It seems this case occurs when the port is not set, so it's not an error worth
logging. I'd like a small comment like "// Use the default port" though.

[Andrey Novikov, 2012/11/09 09:23:47]

Done.

-    }
-
-    auth = props.getProperty(prefix + AUTH);
-
-    shouldLog = (props.getProperty(prefix + "proxylog") != null);
-
+    shouldLogHeaders = (server.props.getProperty(prefix + "proxylog") != null);
     via = " " + server.hostName + ":" + server.listen.getLocalPort() + " (" + server.name + ")";
 
     return true;
   }
 
   @Override
   public boolean respond(Request request) throws IOException
   {
     boolean block = false;
     String reqHost = null;
     String refHost = null;
 
+    String referrer = request.getRequestHeader("referer");
+    
     try
     {
       reqHost = (new URL(request.url)).getHost();
-      refHost = (new URL(request.getRequestHeader("referer"))).getHost();
+      if (referrer != null)
+        refHost = (new URL(referrer)).getHost();
     }
     catch (MalformedURLException e)
     {

[Felix Dahlke, 2012/11/09 06:04:32]

Can we really safely go on in this case? At least worth a comment IMO.

[Andrey Novikov, 2012/11/09 09:23:47]

We are transparent, it's not our deal if it's malformed. Just have to catch
because of Java API. 

[Felix Dahlke, 2012/11/09 14:40:46]

Hm, but should we just go on with a null reqHost and/or refHost?

[Andrey Novikov, 2012/11/12 08:53:12]

In fact this exception is for referrer, request can hardly be malformed at this
point, it should be processed earlier by third-party code. I can split it in two
catches if you wish.

+      // We are transparent, it's not our deal if it's malformed.
     }
 
     try
     {
-      block = application.matches(request.url, request.query, reqHost, refHost, request.getRequestHeader("accept"));
+      if (referrer != null)
+        block = application.matches(request.url, request.query, reqHost, refHost, request.getRequestHeader("accept"));
     }
     catch (Exception e)
     {
       Log.e(prefix, "Filter error", e);
     }
 
-    request.log(Server.LOG_LOG, prefix, block + ": " + request.url);
+    request.log(Server.LOG_LOG, prefix, block + ": " + request.url + " ("+ refHost +")");
+
+    int count = request.server.requestCount;
+    if (shouldLogHeaders)
+    {
+      System.err.println(dumpHeaders(count, request, request.headers, true));
+    }
 
     if (block)
     {
-      request.sendError(502, "Blocked by Adblock Plus");
+      request.sendHeaders(204, null, 0);
       return true;
     }
 
     // Do not further process non-http requests
-    if (request.url.startsWith("http:") == false && request.url.startsWith("https:") == false)

[Felix Dahlke, 2012/11/09 06:04:32]

How about if (!request.url.matches("^https?:")) ?

[Andrey Novikov, 2012/11/09 09:23:47]

I thought it would be faster...

[Felix Dahlke, 2012/11/09 14:40:46]

Premature optimisation? :) I think you should go with whatever you find most
readable, unless there is an actual issue.

[Andrey Novikov, 2012/11/12 08:53:12]

Done.

+    if (!RE_HTTP.matcher(request.url).find())
     {
       return false;
     }
 
     String url = request.url;
 
     if ((request.query != null) && (request.query.length() > 0))
     {
       url += "?" + request.query;
-    }
-
-    int count = request.server.requestCount;
-    if (shouldLog)
-    {
-      System.err.println(dumpHeaders(count, request, request.headers, true));
     }
 
     /*
      * "Proxy-Connection" may be used (instead of just "Connection")
      * to keep alive a connection between a client and this proxy.
      */
     String pc = request.headers.get("Proxy-Connection");
     if (pc != null)
     {
       request.connectionHeader = "Proxy-Connection";
@@ skipping 19 matching lines @@
 
       if (request.postData != null)
       {
         OutputStream out = target.getOutputStream();
         out.write(request.postData);
         out.close();
       }
 
       target.connect();
 
-      if (shouldLog)
+      if (shouldLogHeaders)
       {
         System.err.println("      " + target.status + "\n" + dumpHeaders(count, request, target.responseHeaders, false));
       }
       HttpRequest.removePointToPointHeaders(target.responseHeaders, true);
 
       request.setStatus(target.getResponseCode());
       target.responseHeaders.copyTo(request.responseHeaders);
       try
       {
         request.responseHeaders.add("Via", target.status.substring(0, 8) + via);
@@ skipping 70 matching lines @@
           request.responseHeaders.remove("Content-Length");
           request.responseHeaders.remove("Content-Encoding");
           out = new ChunkedOutputStream(request.out);
           request.responseHeaders.add("Transfer-Encoding", "chunked");
           size = Integer.MAX_VALUE;
         }
 
         request.sendHeaders(-1, null, -1);
 
         byte[] buf = new byte[Math.min(4096, size)];
-
-        Log.e(prefix, request.url);

[Felix Dahlke, 2012/11/09 06:04:32]

Log.d()?

[Andrey Novikov, 2012/11/09 09:23:47]

Done.

 
         boolean sent = selectors == null;
         // TODO Do we need to set encoding here?
         BoyerMoore matcher = new BoyerMoore("<html".getBytes());
 
         while (size > 0)
         {
           out.flush();
 
           count = in.read(buf, 0, Math.min(buf.length, size));
@@ skipping 24 matching lines @@
             out.write(buf, 0, count);
           }
           catch (IOException e)
           {
             break;
           }
         }
         // The correct way would be to close ChunkedOutputStream
         // but we can not do it because underlying output stream is
         // used later in caller code. So we use this ugly hack:
-        try
-        {
+        if (out instanceof ChunkedOutputStream)
           ((ChunkedOutputStream) out).writeFinalChunk();

[Felix Dahlke, 2012/11/09 06:04:32]

Why use instanceof? That would avoid class cast exceptions.

[Andrey Novikov, 2012/11/09 09:23:47]

Done.

-        }
-        catch (ClassCastException e)
-        {
-          // ignore
-        }
       }
     }
     catch (InterruptedIOException e)
     {
       /*
        * Read timeout while reading from the remote side. We use a
        * read timeout in case the target never responds.
        */
       request.sendError(408, "Timeout / No response");
     }
@@ skipping 53 matching lines @@
     }
 
     for (int i = 0; i < headers.size(); i++)
     {
       sb.append(prompt).append(headers.getKey(i));
       sb.append(": ").append(headers.get(i)).append("\n");
     }
     return (sb.toString());
   }
 }