Update #9048 based on reviews

3 files changed, 65 insertions, 32 deletions

diff --git a/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java b/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java
index da4dfa08dc..1308e7aac7 100644
--- a/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java
+++ b/src/com/vaadin/terminal/gwt/client/ApplicationConnection.java
@@ -76,6 +76,10 @@ import com.vaadin.terminal.gwt.server.AbstractCommunicationManager;
  * Entry point classes (widgetsets) define <code>onModuleLoad()</code>.
  */
 public class ApplicationConnection {
+    private static final String CONNECTOR_PROTOCOL_PREFIX = "connector://";
+
+    public static final String CONNECTOR_RESOURCE_PREFIX = "APP/CONNECTOR";
+
     // This indicates the whole page is generated by us (not embedded)
     public static final String GENERATED_BODY_CLASSNAME = "v-generated-body";
 
@@ -2285,9 +2289,12 @@ public class ApplicationConnection {
         }
         if (uidlUri.startsWith("app://")) {
             uidlUri = getAppUri() + uidlUri.substring(6);
-        } else if (uidlUri.startsWith("connector://")) {
-            uidlUri = getAppUri() + "APP/CONNECTOR/"
-                    + uidlUri.substring("connector://".length());
+        } else if (uidlUri.startsWith(CONNECTOR_PROTOCOL_PREFIX)) {
+            // getAppUri *should* always end with /
+            // substring *should* always start with / (connector:///foo.bar
+            // without connector://)
+            uidlUri = getAppUri() + CONNECTOR_RESOURCE_PREFIX
+                    + uidlUri.substring(CONNECTOR_PROTOCOL_PREFIX.length());
         }
         return uidlUri;
     }
diff --git a/src/com/vaadin/terminal/gwt/server/AbstractApplicationServlet.java b/src/com/vaadin/terminal/gwt/server/AbstractApplicationServlet.java
index 6930961497..7d88b432f4 100644
--- a/src/com/vaadin/terminal/gwt/server/AbstractApplicationServlet.java
+++ b/src/com/vaadin/terminal/gwt/server/AbstractApplicationServlet.java
@@ -136,8 +136,6 @@ public abstract class AbstractApplicationServlet extends HttpServlet implements
 
     static final String UPLOAD_URL_PREFIX = "APP/UPLOAD/";
 
-    static final String CONNECTOR_RESOURCE_PREFIX = "/APP/CONNECTOR/";
-
     /**
      * Called by the servlet container to indicate to a servlet that the servlet
      * is being placed into service.
@@ -400,8 +398,10 @@ public abstract class AbstractApplicationServlet extends HttpServlet implements
 
             if (requestType == RequestType.CONNECTOR_RESOURCE) {
                 String pathInfo = getRequestPathInfo(request);
+                // + 2 to also remove beginning and ending slashes
                 String resourceName = pathInfo
-                        .substring(CONNECTOR_RESOURCE_PREFIX.length());
+                        .substring(ApplicationConnection.CONNECTOR_RESOURCE_PREFIX
+                                .length() + 2);
 
                 final String mimetype = getServletContext().getMimeType(
                         resourceName);
@@ -1295,7 +1295,8 @@ public abstract class AbstractApplicationServlet extends HttpServlet implements
 
     private boolean isConnectorResourceRequest(HttpServletRequest request) {
         String path = getRequestPathInfo(request);
-        if (path != null && path.startsWith(CONNECTOR_RESOURCE_PREFIX)) {
+        if (path != null
+                && path.startsWith('/' + ApplicationConnection.CONNECTOR_RESOURCE_PREFIX + '/')) {
             return true;
         }
         return false;
diff --git a/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java b/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java
index 4ee300edac..8c4377ac64 100644
--- a/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java
+++ b/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java
@@ -160,7 +160,7 @@ public abstract class AbstractCommunicationManager implements Serializable {
 
     private Connector highlightedConnector;
 
-    private Map<String, Class<?>> connectoResourceContexts = new HashMap<String, Class<?>>();
+    private Map<String, Class<?>> connectorResourceContexts = new HashMap<String, Class<?>>();
 
     /**
      * TODO New constructor - document me!
@@ -1217,49 +1217,50 @@ public abstract class AbstractCommunicationManager implements Serializable {
         writePerformanceData(outWriter);
     }
 
-    private String registerResource(String resource, Class<?> context) {
+    /**
+     * Resolves a resource URI, registering the URI with this
+     * {@code AbstractCommunicationManager} if needed and returns a fully
+     * qualified URI.
+     */
+    private String registerResource(String resourceUri, Class<?> context) {
         try {
-            URI uri = new URI(resource);
+            URI uri = new URI(resourceUri);
             String protocol = uri.getScheme();
 
             if ("connector".equals(protocol)) {
-                return registerContextResource(uri, context);
+                // Strip initial slash
+                String resourceName = uri.getPath().substring(1);
+                return registerConnecctorResource(resourceName, context);
             }
 
             if (protocol != null || uri.getHost() != null) {
-                return resource;
-            }
-
-            String path = uri.getPath();
-            if (path.startsWith("/")) {
-                return resource;
+                return resourceUri;
             }
 
-            // Default if just simple relative url
-            return registerContextResource(uri, context);
+            // Bare path interpreted as connector resource
+            return registerConnecctorResource(resourceUri, context);
         } catch (URISyntaxException e) {
             getLogger().log(Level.WARNING,
-                    "Could not parse resource url " + resource, e);
-            return resource;
+                    "Could not parse resource url " + resourceUri, e);
+            return resourceUri;
         }
     }
 
-    private String registerContextResource(URI uri, Class<?> context) {
-        String path = uri.getPath();
-        synchronized (connectoResourceContexts) {
-            // Connector resource
-            if (connectoResourceContexts.containsKey(path)) {
-                Class<?> oldContext = connectoResourceContexts.get(path);
+    private String registerConnecctorResource(String name, Class<?> context) {
+        synchronized (connectorResourceContexts) {
+            // Add to map of names accepted by serveConnectorResource
+            if (connectorResourceContexts.containsKey(name)) {
+                Class<?> oldContext = connectorResourceContexts.get(name);
                 getLogger().warning(
-                        "Resource " + path + " defined by both " + context
+                        "Resource " + name + " defined by both " + context
                                 + " and " + oldContext + ". Resource from "
                                 + oldContext + " will be used.");
             } else {
-                connectoResourceContexts.put(path, context);
+                connectorResourceContexts.put(name, context);
             }
         }
 
-        return "connector://" + path;
+        return "connector:///" + name;
     }
 
     /**
@@ -2385,29 +2386,53 @@ public abstract class AbstractCommunicationManager implements Serializable {
         return initialUIDL;
     }
 
+    /**
+     * Serve a connector resource from the classpath if the resource has
+     * previously been registered by calling
+     * {@link #registerResource(String, Class)}. Sending arbitrary files from
+     * the classpath is prevented by only accepting resource names that have
+     * explicitly been registered. Resources can currently only be registered by
+     * including a {@link JavaScript} or {@link StyleSheet} annotation on a
+     * Connector class.
+     * 
+     * @param resourceName
+     * @param request
+     * @param response
+     * @param mimetype
+     * @throws IOException
+     */
     public void serveConnectorResource(String resourceName,
             WrappedRequest request, WrappedResponse response, String mimetype)
             throws IOException {
+        // Security check: avoid accidentally serving from the root of the
+        // classpath instead of relative to the context class
         if (resourceName.startsWith("/")) {
             response.sendError(HttpServletResponse.SC_NOT_FOUND, resourceName);
             return;
         }
 
+        // Check that the resource name has been registered
         Class<?> context;
-        synchronized (connectoResourceContexts) {
-            context = connectoResourceContexts.get(resourceName);
+        synchronized (connectorResourceContexts) {
+            context = connectorResourceContexts.get(resourceName);
         }
 
+        // Security check: don't serve resource if the name hasn't been
+        // registered in the map
         if (context == null) {
             response.sendError(HttpServletResponse.SC_NOT_FOUND, resourceName);
             return;
         }
 
+        // Resolve file relative to the location of the context class
         InputStream in = context.getResourceAsStream(resourceName);
         if (in == null) {
             response.sendError(HttpServletResponse.SC_NOT_FOUND, resourceName);
             return;
         }
+
+        // TODO Check and set cache headers
+
         OutputStream out = null;
         try {
             if (mimetype != null) {