From d6c633e4fa72616edba025411293c3dae3cc9631 Mon Sep 17 00:00:00 2001 From: Moxie Marlinspike Date: Mon, 30 Apr 2012 10:18:44 -0700 Subject: [PATCH] Some review comments. --- firefox/components/ContentPolicy.js | 4 ++++ firefox/components/GoogleSharingManager.js | 4 ++++ firefox/components/Prefetcher.js | 14 ++++++++++++++ firefox/components/Proxy.js | 9 +++++++++ firefox/components/ProxyManager.js | 11 +++++++++++ server/useragents.js | 2 ++ 6 files changed, 44 insertions(+) diff --git a/firefox/components/ContentPolicy.js b/firefox/components/ContentPolicy.js index cc37afe..e2fc44b 100644 --- a/firefox/components/ContentPolicy.js +++ b/firefox/components/ContentPolicy.js @@ -55,6 +55,7 @@ ContentPolicy.prototype = { return false; } + // REVIEW 2012-04-27 -- Strip the scheme off this check. var proxy = this.getProxyForURL(aContentLocation.spec); if (aContentLocation.scheme == "http" && proxy != null) @@ -62,6 +63,7 @@ ContentPolicy.prototype = { if (this.autoCompleteExpression.test(aContentLocation.spec) && !proxy.hasCachedIdentity()) { try { + // REVIEW 2012-04-27 -- This doesn't seem necessary any longer? proxy.fetchSharedIdentity(true); } catch (e) { dump("Got exception on prefetch: " + e + "\n"); @@ -95,6 +97,8 @@ ContentPolicy.prototype = { }, upgradeSpecToHttpsIfPossible: function(spec) { + // REVIEW 2012-04-27 -- These should be consolidated into and match the regexps + // in Proxy.js spec = spec.replace(/^http:\/\/(www\.)?google\.com\/$/ig, "https://encrypted.google.com/"); spec = spec.replace(/^http:\/\/(www\.)?google\.com\/(search||webhp||#)?/ig, "https://encrypted.google.com/$2"); diff --git a/firefox/components/GoogleSharingManager.js b/firefox/components/GoogleSharingManager.js index d7c7165..6c2c57d 100755 --- a/firefox/components/GoogleSharingManager.js +++ b/firefox/components/GoogleSharingManager.js @@ -147,6 +147,10 @@ GoogleSharingManager.prototype = { applyFilter : function(protocolService, uri, otherProxies) { if (!this.enabled) return proxy; + // REVIEW 2012-04-27 -- We should make sure that we're not + // proxying SSL traffic [if uri.scheme.tolower() != "https" {return}], + // not include teh scheme in the request uri and have all the filters + // be scheme-agnostic. var requestUri = uri.scheme + "://" + uri.host + uri.path; var proxy = this.connectionManager.getProxyForURL(requestUri); diff --git a/firefox/components/Prefetcher.js b/firefox/components/Prefetcher.js index 01f74bb..fe7e05c 100755 --- a/firefox/components/Prefetcher.js +++ b/firefox/components/Prefetcher.js @@ -30,6 +30,8 @@ function Prefetcher(urlScheme, host, port) { } Prefetcher.prototype.parseIdentity = function(response) { + // REVIEW 2012-04-27 -- Why does the respose have escaped quotes in it? We should + // just fix that, no? // kind of hacky, response comes back wrapped in quotes, and internal quotes are escaped. response = response.substring(1,response.length-1); @@ -49,12 +51,16 @@ Prefetcher.prototype.sendRequest = function(request, async,method,data) { var thisInstance = this; if (async) { if(method == "PUT"){ + // REVIEW 2012-04-27 -- Remove comments, if the code isn't clear it should + // be refactored. + // we don't care about the response for checking in identity cookieRequest.onreadystatechange = function(){}; } else { cookieRequest.onreadystatechange = function() { if (cookieRequest.readyState == 4) { if(cookieRequest.status == 200) { + // REVIEW 2012-04-27 -- Fix tabs. thisInstance.parseIdentity(cookieRequest.responseText); thisInstance.outstandingAsyncRequest = false; } else { @@ -92,7 +98,13 @@ Prefetcher.prototype.fetchCachedSharedIdentity = function() { return this.cachedIdentity; + // REVIEW 2012-04-27 -- This feels like overloading of this function to me, + // and it should be recomposed up into fetchIdentity, or both methods should be recomposed + // into a separate collection of functions. } else if(this.hasBackupIdentity()){ + // REVIEW 2012-04-27 -- Remove comments, replace with identically-named functions + // if necessary. + // check in old identity this.sendRequest(this.urlScheme + this.host + ":" + this.port + "/identity",true,"PUT",JSON.stringify(this.cachedIdentity)); // swap with backup identity @@ -112,6 +124,8 @@ Prefetcher.prototype.updateSharedIdentity = function(cookie, domain, path) { } }; +// REVIEW 2012-04-27 -- This class should kick off an async identity +// request on startup so that there's never a blocking action from the chrome. Prefetcher.prototype.fetchSharedIdentity = function(async) { var identity = this.fetchCachedSharedIdentity(); if (identity == null && !(async && this.outstandingAsyncRequest)){ diff --git a/firefox/components/Proxy.js b/firefox/components/Proxy.js index 4114287..a5f9291 100755 --- a/firefox/components/Proxy.js +++ b/firefox/components/Proxy.js @@ -45,6 +45,8 @@ function Proxy() { Proxy.prototype.initializePrefetcher = function() { if (this.prefetcher == null) { + // REVIEW 2012-04-26 -- Whis is this port hardcoded + // as 80? The default HTTP port is even 8080 for proxy.abine.com? this.prefetcher = new Prefetcher("http://", this.host,80); } }; @@ -77,6 +79,10 @@ Proxy.prototype.initializeProxyInfo = function() { this.encryptedProxyInfo = proxyService.newProxyInfo("http", this.getHost(), this.getSSLPort(), 1, 0, null); }; + +// REVIEW 2012-04-26 -- These filters should all probably be made to be +// protocol agnostic, and the code calling in for a filter match should handle forcing +// the upgrade to https and passing everything post-protocol specifier. Proxy.prototype.setDefaultFilters = function() { var javascriptApiPaths = "|\\/jsapi|\\/uds\\/|\\/books\\/api\\.js|\\/friendconnect\\/script\\/friendconnect\\.js|\\/s2\\/sharing\\/js|\\/maps\\?.*file=(googleapi|api).*" + @@ -141,6 +147,7 @@ Proxy.prototype.setHost = function(host) { this.host = host; }; +// REVIEW 2012-04-26 -- No such thing as SSL port. Proxy.prototype.getSSLPort = function() { return this.sslPort; }; @@ -173,6 +180,8 @@ Proxy.prototype.setFilters = function(filters) { this.filters = filters; }; +// REVIEW 2012-04-26 -- There should be no more distinction +// between encrypted and normal filters. Proxy.prototype.setEncryptedFilters = function(filters) { this.encryptedFilters = filters; }; diff --git a/firefox/components/ProxyManager.js b/firefox/components/ProxyManager.js index d436cc6..8b5cee5 100755 --- a/firefox/components/ProxyManager.js +++ b/firefox/components/ProxyManager.js @@ -128,6 +128,9 @@ ProxyManager.prototype.disableAllProxies = function() { ProxyManager.prototype.getDefaultProxy = function() { var proxy = new Proxy(); proxy.setHost("proxy.abine.com"); + // REVIEW 2012-04-26 -- No such thing as an SSL + // port any longer. There should now be a single property + // called 'port'. proxy.setSSLPort(8080); proxy.setHTTPPort(8080); proxy.setSSLEnabled(true); @@ -150,6 +153,10 @@ ProxyManager.prototype.loadPreferences = function() { var rootElement = settings.getElementsByTagName("googlesharing"); this.enabled = (rootElement.item(0).getAttribute("enabled") == "true"); this.shortStatus = (rootElement.item(0).getAttribute("shortStatus") == "true"); + + // REVIEW 2012-04-26 -- We need to remove this option and everything + // that connects to it, we'll be tunneling everything via SSL now and not doing + // upgrades to the proxy. this.upgradeToSsl = (rootElement.item(0).getAttribute("upgrade-ssl") == "true"); if (!rootElement.item(0).hasAttribute("upgrade-ssl")) { @@ -166,10 +173,14 @@ ProxyManager.prototype.loadPreferences = function() { var proxy = new Proxy(); proxy.deserialize(element); + // REVIEW 2012-04-26 -- This hostname should be + // a constant defined in a single place. if(proxy.getHost() == "proxy.abine.com") hasNewProxy = true; this.proxies.push(proxy); } + // REVIEW 2012-04-26 -- What if someone intentionally + // remove this? if(!hasNewProxy){ this.proxies.unshift(this.getDefaultProxy()); } diff --git a/server/useragents.js b/server/useragents.js index 13638cf..717f956 100644 --- a/server/useragents.js +++ b/server/useragents.js @@ -1,4 +1,6 @@ +// REVIEW 2012-04-27 -- All the UAs need to be for FF. +// Google sends different browser-specific JS for different browsers. exports.userAgents = ['Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0', 'Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2', 'Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; Trident/5.0)',