Permalink
Browse files

Some review comments.

  • Loading branch information...
1 parent 4b006a3 commit d6c633e4fa72616edba025411293c3dae3cc9631 Moxie Marlinspike committed Apr 30, 2012
@@ -55,13 +55,15 @@ ContentPolicy.prototype = {
return false;
}
+ // REVIEW 2012-04-27 <moxie> -- Strip the scheme off this check.
var proxy = this.getProxyForURL(aContentLocation.spec);
if (aContentLocation.scheme == "http" && proxy != null)
this.upgradeToHttpsIfPossible(aContentLocation, proxy);
if (this.autoCompleteExpression.test(aContentLocation.spec) && !proxy.hasCachedIdentity()) {
try {
+ // REVIEW 2012-04-27 <moxie> -- 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 <moxie> -- 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");
@@ -147,6 +147,10 @@ GoogleSharingManager.prototype = {
applyFilter : function(protocolService, uri, otherProxies) {
if (!this.enabled) return proxy;
+ // REVIEW 2012-04-27 <moxie> -- 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);
@@ -30,6 +30,8 @@ function Prefetcher(urlScheme, host, port) {
}
Prefetcher.prototype.parseIdentity = function(response) {
+ // REVIEW 2012-04-27 <moxie> -- 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 <moxie> -- 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 <moxie> -- 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 <moxie> -- 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 <moxie> -- 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 <moxie> -- 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)){
@@ -45,6 +45,8 @@ function Proxy() {
Proxy.prototype.initializePrefetcher = function() {
if (this.prefetcher == null) {
+ // REVIEW 2012-04-26 <moxie> -- 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 <moxie> -- 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 <moxie> -- 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 <moxie> -- There should be no more distinction
+// between encrypted and normal filters.
Proxy.prototype.setEncryptedFilters = function(filters) {
this.encryptedFilters = filters;
};
@@ -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 <moxie> -- 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 <moxie> -- 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 <moxie> -- 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 <moxie> -- What if someone intentionally
+ // remove this?
if(!hasNewProxy){
this.proxies.unshift(this.getDefaultProxy());
}
View
@@ -1,4 +1,6 @@
+// REVIEW 2012-04-27 <moxie> -- 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)',

0 comments on commit d6c633e

Please sign in to comment.