From 511b56aec5f96c6ad2be73c3d7adf3405dc0683c Mon Sep 17 00:00:00 2001
From: Lloyd Hilaiel <lloyd@hilaiel.com>
Date: Tue, 11 Oct 2011 14:01:36 -0600
Subject: [PATCH] WSAPI CHANGES: All server responses are now objects, makes
some funky browsers happy and prevents certain attacks. closes #217 closes
#325
* /wsapi/have_email now returns { email_known: <boolean> }
* /wsapi/stage_user now returns { success: <boolean> }
* /wsapi/user_creation_status now returns { status: <string> }
* /wsapi/complete_user_creation now returns { status: <boolean> }
* /wsapi/stage_email now returns { success: <boolean> }
* /wsapi/email_addition_status now returns { success: <boolean> }
* /wsapi/complete_email_addition now returns { success: <boolean> }
* /wsapi/authenticate_user now returns { success: <boolean> }
* /wsapi/remove_email now returns { success: <boolean> }
* /wsapi/account_cancel now returns { success: <boolean> }
* /wsapi/logout now returns { success: <boolean> }
Finally, introduced middleware to ensure that resp.json() is not called with anything other than an object.
---
browserid/app.js | 13 +++++
browserid/lib/wsapi.js | 46 +++++++++---------
browserid/static/dialog/resources/network.js | 33 ++++++++-----
.../dialog/test/qunit/network_unit_test.js | 48 +++++++++----------
browserid/tests/cert-emails-test.js | 2 +-
browserid/tests/forgotten-email-test.js | 30 ++++++------
browserid/tests/list-emails-wsapi-test.js | 4 +-
.../tests/registration-status-wsapi-test.js | 14 +++---
8 files changed, 105 insertions(+), 85 deletions(-)
diff --git a/browserid/app.js b/browserid/app.js
index 946105636..a5abe7248 100644
--- a/browserid/app.js
+++ b/browserid/app.js
@@ -219,6 +219,19 @@ exports.setup = function(server) {
}
});
+ // verify all JSON responses are objects - prevents regression on issue #217
+ server.use(function(req, resp, next) {
+ var realRespJSON = resp.json;
+ resp.json = function(obj) {
+ if (!obj || typeof obj !== 'object') {
+ logger.error("INTERNAL ERROR! *all* json responses must be objects");
+ throw "internal error";
+ }
+ realRespJSON.call(resp, obj);
+ };
+ return next();
+ });
+
server.use(express.bodyParser());
// Check CSRF token early. POST requests are only allowed to
diff --git a/browserid/lib/wsapi.js b/browserid/lib/wsapi.js
index 6585caff0..2a84dc1c2 100644
--- a/browserid/lib/wsapi.js
+++ b/browserid/lib/wsapi.js
@@ -164,7 +164,7 @@ function setup(app) {
// get inputs from get data!
var email = url.parse(req.url, true).query['email'];
db.emailKnown(email, function(known) {
- resp.json(known);
+ resp.json({ email_known: known });
});
});
@@ -189,7 +189,7 @@ function setup(app) {
// status of the email verification.
req.session.pendingCreation = secret;
- resp.json(true);
+ resp.json({ success: true });
// let's now kick out a verification email!
email.sendNewUserEmail(req.body.email, req.body.site, secret);
@@ -210,24 +210,24 @@ function setup(app) {
// if the user is authenticated as the user in question, we're done
if (isAuthed(req) && req.session.authenticatedUser === email) {
- return resp.json('complete');
+ return resp.json({ status: 'complete' });
}
// if the user isn't authenticated and there's no pendingCreation token,
// then they must authenticate
else if (!req.session.pendingCreation) {
- return resp.json('mustAuth');
+ return resp.json({ status: 'mustAuth' });
}
// if the secret is still in the database, it hasn't yet been verified and
// verification is still pending
db.emailForVerificationSecret(req.session.pendingCreation, function (email) {
- if (email) return resp.json('pending');
+ if (email) return resp.json({ status: 'pending' });
// if the secret isn't known, and we're not authenticated, then the user must authenticate
// (maybe they verified the URL on a different browser, or maybe they canceled the account
// creation)
else {
delete req.session.pendingCreation;
- resp.json('mustAuth');
+ resp.json({ status: 'mustAuth' });
}
});
});
@@ -247,30 +247,30 @@ function setup(app) {
// bcrypting the password (which is expensive), to prevent a possible
// DoS attack.
db.emailForVerificationSecret(req.body.token, function(email) {
- if (!email) return resp.json(false);
+ if (!email) return resp.json({ success: false} );
// now bcrypt the password
bcrypt.gen_salt(10, function (err, salt) {
if (err) {
logger.error("error generating salt with bcrypt: " + err);
- return resp.json(false);
+ return resp.json({ success: false });
}
bcrypt.encrypt(req.body.pass, salt, function(err, hash) {
if (err) {
logger.error("error generating password hash with bcrypt: " + err);
- return resp.json(false);
+ return resp.json({ success: false });
}
db.gotVerificationSecret(req.body.token, hash, function(err, email) {
if (err) {
logger.error("error completing the verification: " + err);
- resp.json(false);
+ resp.json({ success: false });
} else {
// FIXME: not sure if we want to do this (ba)
// at this point the user has set a password associated with an email address
// that they've verified. We create an authenticated session.
setAuthenticatedUser(req.session, email);
- resp.json(true);
+ resp.json({ success: true });
}
});
});
@@ -286,7 +286,7 @@ function setup(app) {
// store the email being added in session data
req.session.pendingAddition = secret;
- resp.json(true);
+ resp.json({ success: true });
// let's now kick out a verification email!
email.sendAddAddressEmail(req.body.email, req.body.site, secret);
@@ -299,7 +299,7 @@ function setup(app) {
app.get('/wsapi/email_for_token', checkParams(["token"]), function(req,resp) {
db.emailForVerificationSecret(req.query.token, function(email) {
- resp.json({email: email});
+ resp.json({ email: email });
});
});
@@ -327,16 +327,16 @@ function setup(app) {
function(registered) {
if (registered) {
delete req.session.pendingAddition;
- resp.json('complete');
+ resp.json({ status: 'complete' });
} else if (!req.session.pendingAddition) {
resp.json('failed');
} else {
db.emailForVerificationSecret(req.session.pendingAddition, function (email) {
if (email) {
- return resp.json('pending');
+ return resp.json({ status: 'pending' });
} else {
delete req.session.pendingAddition;
- resp.json('failed');
+ resp.json({ status: 'failed' });
}
});
}
@@ -347,9 +347,9 @@ function setup(app) {
db.gotVerificationSecret(req.body.token, undefined, function(e) {
if (e) {
logger.error("error completing the verification: " + e);
- resp.json(false);
+ resp.json({ success: false });
} else {
- resp.json(true);
+ resp.json({ success: true });
}
});
});
@@ -359,7 +359,7 @@ function setup(app) {
if (typeof hash !== 'string' ||
typeof req.body.pass !== 'string')
{
- return resp.json(false);
+ return resp.json({ success: false });
}
bcrypt.compare(req.body.pass, hash, function (err, success) {
@@ -373,7 +373,7 @@ function setup(app) {
// if the work factor has changed, update the hash here
}
- resp.json(success);
+ resp.json({ success: success });
});
});
});
@@ -386,7 +386,7 @@ function setup(app) {
logger.error("error removing email " + email);
httputils.badRequest(resp, error.toString());
} else {
- resp.json(true);
+ resp.json({ success: true });
}});
});
@@ -396,7 +396,7 @@ function setup(app) {
logger.error("error cancelling account : " + error.toString());
httputils.badRequest(resp, error.toString());
} else {
- resp.json(true);
+ resp.json({ success: true });
}});
});
@@ -422,7 +422,7 @@ function setup(app) {
app.post('/wsapi/logout', function(req, resp) {
clearAuthenticatedUser(req.session);
- resp.json('ok');
+ resp.json({ success: true });
});
// in the cert world, syncing is not necessary,
diff --git a/browserid/static/dialog/resources/network.js b/browserid/static/dialog/resources/network.js
index 0cc838236..18d041256 100644
--- a/browserid/static/dialog/resources/network.js
+++ b/browserid/static/dialog/resources/network.js
@@ -125,7 +125,7 @@ BrowserID.Network = (function() {
success: function(status, textStatus, jqXHR) {
if (onSuccess) {
try {
- var authenticated = JSON.parse(status);
+ var authenticated = status.success;
if (typeof authenticated !== 'boolean') throw status;
@@ -197,7 +197,7 @@ BrowserID.Network = (function() {
site : origin
},
success: function(status) {
- var staged = JSON.parse(status);
+ var staged = status.success;
// why a delay here? Because of the test harness?
// shouldn't the delay be in the test harness?
_.delay(onSuccess, 0, staged);
@@ -233,7 +233,11 @@ BrowserID.Network = (function() {
checkUserRegistration: function(email, onSuccess, onFailure) {
xhr.ajax({
url: "/wsapi/user_creation_status?email=" + encodeURIComponent(email),
- success: createDeferred(onSuccess),
+ success: function(status, textStatus, jqXHR) {
+ if (onSuccess) {
+ _.delay(onSuccess, 0, status.status);
+ }
+ },
error: onFailure
});
},
@@ -255,8 +259,7 @@ BrowserID.Network = (function() {
},
success: function(status, textStatus, jqXHR) {
if (onSuccess) {
- var valid = JSON.parse(status);
- _.delay(onSuccess, 0, valid);
+ _.delay(onSuccess, 0, status.success);
}
},
error: onFailure
@@ -325,8 +328,7 @@ BrowserID.Network = (function() {
},
success: function(status, textStatus, jqXHR) {
if (onSuccess) {
- var valid = JSON.parse(status);
- _.delay(onSuccess, 0, valid);
+ _.delay(onSuccess, 0, status.success);
}
},
error: onFailure
@@ -363,7 +365,7 @@ BrowserID.Network = (function() {
site: origin
},
success: function(status) {
- var staged = JSON.parse(status);
+ var staged = status.success;
_.delay(onSuccess, 0, staged);
},
error: onFailure
@@ -380,7 +382,11 @@ BrowserID.Network = (function() {
checkEmailRegistration: function(email, onSuccess, onFailure) {
xhr.ajax({
url: "/wsapi/email_addition_status?email=" + encodeURIComponent(email),
- success: createDeferred(onSuccess),
+ success: function(status, textStatus, jqXHR) {
+ if (onSuccess) {
+ _.delay(onSuccess, 0, status.status);
+ }
+ },
error: onFailure
});
},
@@ -399,8 +405,7 @@ BrowserID.Network = (function() {
url: "/wsapi/have_email?email=" + encodeURIComponent(email),
success: function(data, textStatus, xhr) {
if(onSuccess) {
- var success = typeof data === "string" ? !JSON.parse(data) : data;
- _.delay(onSuccess, 0, success);
+ _.delay(onSuccess, 0, data.email_known);
}
},
error: onFailure
@@ -420,7 +425,11 @@ BrowserID.Network = (function() {
data: {
email: email
},
- success: createDeferred(onSuccess),
+ success: function(status, textStatus, jqXHR) {
+ if (onSuccess) {
+ _.delay(onSuccess, 0, status.success);
+ }
+ },
failure: onFailure
});
},
diff --git a/browserid/static/dialog/test/qunit/network_unit_test.js b/browserid/static/dialog/test/qunit/network_unit_test.js
index 4da6a9f18..1f532b24b 100644
--- a/browserid/static/dialog/test/qunit/network_unit_test.js
+++ b/browserid/static/dialog/test/qunit/network_unit_test.js
@@ -42,7 +42,7 @@ steal.plugins("jquery", "funcunit/qunit").then("/dialog/resources/network", func
function wrappedAsyncTest(name, test) {
asyncTest(name, function() {
testName = name;
- test();
+ test();
});
}
@@ -56,38 +56,36 @@ steal.plugins("jquery", "funcunit/qunit").then("/dialog/resources/network", func
server_time: new Date().getTime(),
csrf_token: "csrf",
authenticated: false
- };
+ };
var xhr = {
results: {
"get /wsapi/session_context valid": contextInfo,
"get /wsapi/session_context invalid": contextInfo,
- "post /wsapi/authenticate_user valid": "true",
- "post /wsapi/authenticate_user invalid": "false",
- "get /wsapi/am_authed valid": "true",
- "get /wsapi/am_authed invalid": "false",
- "post /wsapi/complete_email_addition valid": "true",
- "post /wsapi/complete_email_addition invalid": "false",
- "post /wsapi/stage_user valid": "true",
- "post /wsapi/stage_user invalid": "false",
+ "post /wsapi/authenticate_user valid": { success: true },
+ "post /wsapi/authenticate_user invalid": { success: false },
+ "post /wsapi/complete_email_addition valid": { success: true },
+ "post /wsapi/complete_email_addition invalid": { success: false },
+ "post /wsapi/stage_user valid": { success: true },
+ "post /wsapi/stage_user invalid": { success: false },
"get /wsapi/user_creation_status?email=address notcreated": undefined, // undefined because server returns 400 error
- "get /wsapi/user_creation_status?email=address pending": "pending",
- "get /wsapi/user_creation_status?email=address complete": "complete",
- "post /wsapi/complete_user_creation valid": "true",
- "post /wsapi/complete_user_creation invalid": "false",
- "post /wsapi/logout valid": "true",
- "get /wsapi/have_email?email=address taken": "false",
- "get /wsapi/have_email?email=address nottaken" : "true",
- "post /wsapi/remove_email valid": "true",
- "post /wsapi/remove_email invalid": "false",
- "post /wsapi/account_cancel valid": "true",
- "post /wsapi/account_cancel invalid": "false",
- "post /wsapi/stage_email valid": "true",
- "post /wsapi/stage_email invalid": "false",
+ "get /wsapi/user_creation_status?email=address pending": { status: "pending" },
+ "get /wsapi/user_creation_status?email=address complete": { status: "complete" },
+ "post /wsapi/complete_user_creation valid": { success: true },
+ "post /wsapi/complete_user_creation invalid": { success: false },
+ "post /wsapi/logout valid": { success: true },
+ "get /wsapi/have_email?email=address taken": { email_known: true },
+ "get /wsapi/have_email?email=address nottaken" : { email_known: false },
+ "post /wsapi/remove_email valid": { success: true },
+ "post /wsapi/remove_email invalid": { success: false },
+ "post /wsapi/account_cancel valid": { success: true },
+ "post /wsapi/account_cancel invalid": { success: false },
+ "post /wsapi/stage_email valid": { success: true },
+ "post /wsapi/stage_email invalid": { success: false },
"get /wsapi/email_addition_status?email=address notcreated": undefined, // undefined because server returns 400 error
- "get /wsapi/email_addition_status?email=address pending": "pending",
- "get /wsapi/email_addition_status?email=address complete": "complete",
+ "get /wsapi/email_addition_status?email=address pending": { status: "pending" },
+ "get /wsapi/email_addition_status?email=address complete": { status: "complete" },
},
useResult: function(result) {
diff --git a/browserid/tests/cert-emails-test.js b/browserid/tests/cert-emails-test.js
index afdf9a5d0..2b3ef1caf 100755
--- a/browserid/tests/cert-emails-test.js
+++ b/browserid/tests/cert-emails-test.js
@@ -84,7 +84,7 @@ suite.addBatch({
},
"works": function(r, err) {
assert.equal(r.code, 200);
- assert.strictEqual(true, JSON.parse(r.body));
+ assert.strictEqual(true, JSON.parse(r.body).success);
}
}
});
diff --git a/browserid/tests/forgotten-email-test.js b/browserid/tests/forgotten-email-test.js
index 13da60e37..8fa595b46 100755
--- a/browserid/tests/forgotten-email-test.js
+++ b/browserid/tests/forgotten-email-test.js
@@ -75,7 +75,7 @@ suite.addBatch({
},
"account created": function(r, err) {
assert.equal(r.code, 200);
- assert.strictEqual(true, JSON.parse(r.body));
+ assert.strictEqual(true, JSON.parse(r.body).success);
}
}
});
@@ -85,7 +85,7 @@ suite.addBatch({
topic: wsapi.get('/wsapi/user_creation_status', { email: 'first@fakeemail.com' } ),
"should exist": function(r, err) {
assert.strictEqual(r.code, 200);
- assert.strictEqual(JSON.parse(r.body), "complete");
+ assert.strictEqual(JSON.parse(r.body).status, "complete");
}
}
});
@@ -111,7 +111,7 @@ suite.addBatch({
},
"account created": function(r, err) {
assert.equal(r.code, 200);
- assert.strictEqual(true, JSON.parse(r.body));
+ assert.strictEqual(JSON.parse(r.body).success, true);
}
}
});
@@ -121,19 +121,19 @@ suite.addBatch({
"first email exists": {
topic: wsapi.get('/wsapi/have_email', { email: 'first@fakeemail.com' }),
"should exist": function(r, err) {
- assert.strictEqual(true, JSON.parse(r.body));
+ assert.strictEqual(JSON.parse(r.body).email_known, true);
}
},
"second email exists": {
topic: wsapi.get('/wsapi/have_email', { email: 'second@fakeemail.com' }),
"should exist": function(r, err) {
- assert.strictEqual(JSON.parse(r.body), true);
+ assert.strictEqual(JSON.parse(r.body).email_known, true);
}
},
"a random email doesn't exist": {
topic: wsapi.get('/wsapi/have_email', { email: 'third@fakeemail.com' }),
"shouldn't exist": function(r, err) {
- assert.strictEqual(JSON.parse(r.body), false);
+ assert.strictEqual(JSON.parse(r.body).email_known, false);
}
}
});
@@ -158,13 +158,13 @@ suite.addBatch({
"first email works": {
topic: wsapi.post('/wsapi/authenticate_user', { email: 'first@fakeemail.com', pass: 'firstfakepass' }),
"should work": function(r, err) {
- assert.strictEqual(true, JSON.parse(r.body));
+ assert.strictEqual(JSON.parse(r.body).success, true);
}
},
"second email works": {
topic: wsapi.post('/wsapi/authenticate_user', { email: 'second@fakeemail.com', pass: 'firstfakepass' }),
"should work": function(r, err) {
- assert.strictEqual(true, JSON.parse(r.body));
+ assert.strictEqual(JSON.parse(r.body).success, true);
}
}
});
@@ -177,7 +177,7 @@ suite.addBatch({
},
"account created": function(r, err) {
assert.equal(r.code, 200);
- assert.strictEqual(true, JSON.parse(r.body));
+ assert.strictEqual(JSON.parse(r.body).success, true);
}
}
});
@@ -188,31 +188,31 @@ suite.addBatch({
"first email, first pass bad": {
topic: wsapi.post('/wsapi/authenticate_user', { email: 'first@fakeemail.com', pass: 'firstfakepass' }),
"shouldn't work": function(r, err) {
- assert.strictEqual(JSON.parse(r.body), false);
+ assert.strictEqual(JSON.parse(r.body).success, false);
}
},
"first email, second pass good": {
topic: wsapi.post('/wsapi/authenticate_user', { email: 'first@fakeemail.com', pass: 'secondfakepass' }),
"should work": function(r, err) {
- assert.strictEqual(JSON.parse(r.body), true);
+ assert.strictEqual(JSON.parse(r.body).success, true);
}
},
"logout": {
topic: wsapi.post('/wsapi/logout', {}),
- "should work": function(r, err) {
- assert.strictEqual(JSON.parse(r.body), "ok");
+ "should work": function(r, err) {
+ assert.strictEqual(JSON.parse(r.body).success, true);
}
},
"second email, first pass good": {
topic: wsapi.post('/wsapi/authenticate_user', { email: 'second@fakeemail.com', pass: 'firstfakepass' }),
"should work": function(r, err) {
- assert.strictEqual(JSON.parse(r.body), true);
+ assert.strictEqual(JSON.parse(r.body).success, true);
}
},
"second email, second pass bad": {
topic: wsapi.post('/wsapi/authenticate_user', { email: 'second@fakeemail.com', pass: 'secondfakepass' }),
"shouldn' work": function(r, err) {
- assert.strictEqual(JSON.parse(r.body), false);
+ assert.strictEqual(JSON.parse(r.body).success, false);
}
},
});
diff --git a/browserid/tests/list-emails-wsapi-test.js b/browserid/tests/list-emails-wsapi-test.js
index a99532318..e31480e25 100755
--- a/browserid/tests/list-emails-wsapi-test.js
+++ b/browserid/tests/list-emails-wsapi-test.js
@@ -75,7 +75,7 @@ suite.addBatch({
},
"works": function(r, err) {
assert.equal(r.code, 200);
- assert.strictEqual(JSON.parse(r.body), true);
+ assert.strictEqual(JSON.parse(r.body).success, true);
}
}
});
@@ -87,7 +87,7 @@ suite.addBatch({
assert.strictEqual(r.code, 200);
},
"returns a json encoded string - `complete`": function (r, err) {
- assert.strictEqual(JSON.parse(r.body), "complete");
+ assert.strictEqual(JSON.parse(r.body).status, "complete");
}
}
});
diff --git a/browserid/tests/registration-status-wsapi-test.js b/browserid/tests/registration-status-wsapi-test.js
index 152d1f40c..b4bc4be33 100755
--- a/browserid/tests/registration-status-wsapi-test.js
+++ b/browserid/tests/registration-status-wsapi-test.js
@@ -70,7 +70,7 @@ suite.addBatch({
"authentication as an unknown user": {
topic: wsapi.post('/wsapi/authenticate_user', { email: 'first@fakeemail.com', pass: 'secondfakepass' }),
"fails": function (r, err) {
- assert.isFalse(JSON.parse(r.body));
+ assert.isFalse(JSON.parse(r.body).success);
}
}
});
@@ -117,7 +117,7 @@ suite.addBatch({
assert.strictEqual(r.code, 200);
},
"returns a json encoded string - `pending`": function (r, err) {
- assert.strictEqual(JSON.parse(r.body), "pending");
+ assert.strictEqual(JSON.parse(r.body).status, "pending");
}
}
});
@@ -140,7 +140,7 @@ suite.addBatch({
assert.strictEqual(r.code, 200);
},
"returns a json encoded string - `complete`": function (r, err) {
- assert.strictEqual(JSON.parse(r.body), "complete");
+ assert.strictEqual(JSON.parse(r.body).status, "complete");
}
}
});
@@ -152,7 +152,7 @@ suite.addBatch({
assert.strictEqual(r.code, 200);
},
"and still returns a json encoded string - `complete`": function (r, err) {
- assert.strictEqual(JSON.parse(r.body), "complete");
+ assert.strictEqual(JSON.parse(r.body).status, "complete");
}
}
});
@@ -199,7 +199,7 @@ suite.addBatch({
assert.strictEqual(r.code, 200);
},
"returns a json encoded string - `pending`": function (r, err) {
- assert.strictEqual(JSON.parse(r.body), "pending");
+ assert.strictEqual(JSON.parse(r.body).status, "pending");
}
}
});
@@ -222,7 +222,7 @@ suite.addBatch({
assert.strictEqual(r.code, 200);
},
"returns a json encoded string - `complete`": function (r, err) {
- assert.strictEqual(JSON.parse(r.body), "complete");
+ assert.strictEqual(JSON.parse(r.body).status, "complete");
}
}
});
@@ -240,7 +240,7 @@ suite.addBatch({
"after re-registration, authenticating with new credetials": {
topic: wsapi.post('/wsapi/authenticate_user', { email: 'first@fakeemail.com', pass: 'secondfakepass' }),
"works as you might expect": function (r, err) {
- assert.strictEqual(true, JSON.parse(r.body));
+ assert.strictEqual(JSON.parse(r.body).success, true);
}
}
});
--
GitLab