Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add secure method to tcp socket for firefox #12

Merged
merged 3 commits into from
Aug 12, 2014
Merged

Add secure method to tcp socket for firefox #12

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b50777e
Add secure method to tcp socket.
dborkan Aug 11, 2014
114533c
Add comment to secure method
dborkan Aug 12, 2014
5718b1c
Add test for starttls
dborkan Aug 12, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,17 @@ module.exports = function(grunt) {
grunt.loadNpmTasks('grunt-contrib-copy');
grunt.loadNpmTasks('grunt-contrib-jshint');

grunt.registerTask('freedom-firefox', [
grunt.registerTask('build', [
'jshint:providers',
'uglify'
]);
grunt.registerTask('writeJsonDir', 'Write', writeJsonDir);
grunt.registerTask('build_test', [
'freedom-firefox',
'build',
'copy:test',
'writeJsonDir'
]);
grunt.registerTask('default', ['freedom-firefox']);
grunt.registerTask('default', ['build']);

// Write the contents of the data directory in the test extension
// into a JSON file. We have to do this because files/directories
Expand Down
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "freedom-for-firefox",
"description": "Embracing a distributed web",
"version": "0.4.1",
"version": "0.4.2",
"homepage": "http://freedomjs.org",
"bugs": {
"url": "http://github.com/freedomjs/freedom-for-firefox/issues",
Expand All @@ -17,7 +17,7 @@
"url": "https://github.com/freedomjs/freedom-for-firefox"
},
"devDependencies": {
"freedom": "~0.5.0",
"freedom": "~0.5.5",
"grunt": "~0.4.2",
"es5-shim": "^3.1.1",
"es6-promise": "~0.1.1",
Expand All @@ -27,7 +27,7 @@
"grunt-contrib-jshint": "~0.8.0"
},
"peerDependencies": {
"freedom": "~0.5.0"
"freedom": "~0.5.5"
},
"scripts": {}
}
8 changes: 5 additions & 3 deletions providers/client_socket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,15 @@ ClientSocket.prototype._setupTransport = function(transport) {

};

ClientSocket.prototype.connect = function(hostname, port) {
ClientSocket.prototype.connect = function(hostname, port, startTls) {
if (typeof this.transport !== 'undefined') {
throw new Error('Socket already connected');
}

var transport = socketTransportService.createTransport([null],
0,
var socketTypes = startTls ? ['starttls'] : [null];
var numSocketTypes = startTls ? 1 : 0;
var transport = socketTransportService.createTransport(socketTypes,
numSocketTypes,
hostname,
port,
null);
Expand Down
20 changes: 19 additions & 1 deletion providers/tcp_socket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,28 @@ Socket_firefox.prototype.close = function(continuation) {
continuation();
};

// TODO: handle failures.
Socket_firefox.prototype.connect = function(hostname, port, continuation) {
this.clientSocket = new ClientSocket();
this.clientSocket.setOnDataListener(this._onData.bind(this));
this.clientSocket.connect(hostname, port);
this.clientSocket.connect(hostname, port, false);
this.hostname = hostname;
this.port = port;
continuation();
};

// TODO: handle failures.
Socket_firefox.prototype.secure = function(continuation) {
if (!this.hostname || !this.port || !this.clientSocket) {
continuation(undefined, {
"errcode": "SOCKET_NOT_CONNECTED",
"message": "Cannot Secure Not Connected Socket"
});
return;
}
this.clientSocket = new ClientSocket();
this.clientSocket.setOnDataListener(this._onData.bind(this));
this.clientSocket.connect(this.hostname, this.port, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this continue the existing TCP session, or start a new one? It looks like it makes a new connection to the host/port.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It continues the existing TCP session. The firefox API is weird and unlike in Chrome you don't get a unique socket identifier which you can upgrade to TLS, nor is there a socket object with any upgradeToTls method. Instead it appears to keep a mapping from hostname/port to a socket which can be upgraded to tls by creating a new nsISocketTransport object (not necessarily the same as creating a new connection) with type 'starttls'.

We will need to test this in the future and make sure it doesn't result in weird race conditions if we have 2 pieces of code both trying to connect to the same hostname/port and do a starttls flow (e.g. if there are 2 instances of a GTalk social provider that are both trying to connect to GTalk at the same time with different logins)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems worth a comment.

On Tue, Aug 12, 2014 at 8:51 AM, dborkan notifications@github.com wrote:

In providers/tcp_socket.js:

  • this.port = port;
  • continuation();
    +};

+// TODO: handle failures.
+Socket_firefox.prototype.secure = function(continuation) {

  • if (!this.hostname || !this.port || !this.clientSocket) {
  • continuation(undefined, {
  •  "errcode": "SOCKET_NOT_CONNECTED",

  •  "message": "Cannot Secure Not Connected Socket"
  • });
  • return;
  • }
  • this.clientSocket = new ClientSocket();
  • this.clientSocket.setOnDataListener(this._onData.bind(this));
  • this.clientSocket.connect(this.hostname, this.port, true);

It continues the existing TCP session. The firefox API is weird and unlike
in Chrome you don't get a unique socket identifier which you can upgrade to
TLS, nor is there a socket object with any upgradeToTls method. Instead it
appears to keep a mapping from hostname/port to a socket which can be
upgraded to tls by creating a new nsISocketTransport object (not
necessarily the same as creating a new connection) with type 'starttls'.

We will need to test this in the future and make sure it doesn't result in
weird race conditions if we have 2 pieces of code both trying to connect to
the same hostname/port and do a starttls flow (e.g. if there are 2
instances of a GTalk social provider that are both trying to connect to
GTalk at the same time with different logins)

—
Reply to this email directly or view it on GitHub
https://github.com/freedomjs/freedom-for-firefox/pull/12/files#r16121843
.

continuation();
};

Expand Down
4 changes: 2 additions & 2 deletions test/data/firefox_tests/tcp_socket.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ describe("sockets", function() {
serverSocket.disconnect();
done();
};
clientSocket.connect("localhost", 8081);
clientSocket.connect("localhost", 8081, false);
});

it("receives data", function(done) {
Expand All @@ -24,7 +24,7 @@ describe("sockets", function() {
done();
});
};
clientSocket.connect("localhost", 8081);
clientSocket.connect("localhost", 8081, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test of secure behavior would be fantastic, so we can make sure this doesn't break with future changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, unfortunately the tests are already failing with 1 failure, and I add a test that I intentionally try to make fail, it still says only 1 failure. I'm trying to test by running "grunt build_test", then going to build/test and running "cfx run". Is there some other way I should be testing in Firefox?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just figured out how to get tests running (tcp_socket.spec.js had been missing from test/lib/main.js)

clientSocket.write(str2ab(stringMessage));
});

Expand Down