Keystone-classic: Admin API: Update List Item Broken Due to CSRF Issue

[christroutner]

Steps to reproduce the behavior

I first reported this bug on this issue thread. I thought it might help if I filed it as its own separate issue. Here is the summary from that thread:

I ran the test script in the keystone-api-tests repository while running KeystoneJS under node inspector. Keep in mind that I'm running the latest master commit (cbf9ff4).

When I ran the post(/keystone/api/users/userID) test again, the file keystone/admin/client/utils/List.js didn't get executed at all. Instead, keystone/admin/server/api/item/update.js was executed. That file was throwing a 403 error because of an invalid CSRF token. Here is the code from that file. I stopped it at the debugger statement:

(function (exports, require, module, __filename, __dirname) { module.exports = function (req, res) {
    var keystone = req.keystone;
    if (!keystone.security.csrf.validate(req)) {
        debugger;
        return res.apiError(403, 'invalid csrf');
    }
    req.list.model.findById(req.params.id, function (err, item) {
        if (err) return res.status(500).json({ err: 'database error', detail: err });
        if (!item) return res.status(404).json({ err: 'not found', id: req.params.id });
        req.list.validateInput(item, req.body, function (err) {
            if (err) return res.status(400).json(err);
            req.list.updateItem(item, req.body, { files: req.files }, function (err) {
                if (err) return res.status(500).json(err);
                res.json(req.list.getData(item));
            });
        });
    });
};

});

I manually ran req.keystone.security.csrf.getSecret(req) and it returned the CSRF token string "CunSnobyfWscEg==". Next I manually ran req.keystone.security.csrf.validate(req), which returned false. I ran req.keystone.security.csrf.requestToken(req) and it returned an empty string.

During this entire time, I'm logged in and can access the Admin UI at /keystone, so this clearly seems to be a CSRF issue.

Any thoughts?

Expected behavior

Successful update of the list item.

Actual behavior

403 error returned due to invalid CSRF.

[uhhhh2]

I modified the code to print out everything. Here is what I got:

exports.validate: arguments length: 1
exports.requestToken: req has body
exports.requestToken: body keys: 
exports.requestToken: req has headers
exports.requestToken: header keys: host,user-agent,accept,accept-language,accept-encoding,x-requested-with,referer,cookie,connection,content-length
exports.requestToken: cookie: "language=en-US; this.sid=s%3AIhyyQkp-ghfP6LqTr5XKneA4yMJmGTDq.WaFNDxqNQKEfZ1K3u6e%2FZyvrNLBlOAYklUWr3Z0FYbA; XSRF-TOKEN=QtwLRt2HJFd6117cf6866d9f93ab2fd86f02c3ea5432faf8ce"
exports.requestToken: req has query
exports.requestToken: query keys: 
exports.requestToken: give up
exports.requestToken: tok: ""
exports.validate: Token: ""
exports.validate: typeof Token: "string"
exports.validate: sliced token: ""
exports.validate: session info: "3ozALJt0moF8wA=="
exports.validate: tokenized token: "f98ea4f120ec79640623a526a97f2c7d8896f443"

And here is the modified area of code (from /lib/security/csrf.js):

exports.requestToken = function (req) {
    var tok = '';
    if(req.body){
        console.log("exports.requestToken: req has body");
        console.log("exports.requestToken: body keys: " + Object.keys(req.body));
    }
    if(req.headers){
        console.log("exports.requestToken: req has headers");
        console.log("exports.requestToken: header keys: " + Object.keys(req.headers));
        console.log("exports.requestToken: cookie: \"" + req.headers.cookie +"\"");
    }
    if(req.query){
        console.log("exports.requestToken: req has query");
        console.log("exports.requestToken: query keys: " + Object.keys(req.query));
    }
    if (req.body && req.body[exports.TOKEN_KEY]){
        console.log("exports.requestToken: case1");
        tok= req.body[exports.TOKEN_KEY];
    } else if (req.query && req.query[exports.TOKEN_KEY]) {
        console.log("exports.requestToken: case2");
        tok= req.query[exports.TOKEN_KEY];
    } else if (req.headers && req.headers[exports.XSRF_HEADER_KEY]) {
        console.log("exports.requestToken: case3");
        tok= req.headers[exports.XSRF_HEADER_KEY];
    } else if (req.headers && req.headers[exports.CSRF_HEADER_KEY]) {
        console.log("exports.requestToken: case4");
        tok= req.headers[exports.CSRF_HEADER_KEY];
    } else{
        console.log("exports.requestToken: give up");
    }
    console.log("exports.requestToken: tok: \"" + tok + "\"");
    return tok;
};

exports.validate = function (req, token) {
    // Allow environment variable to disable check
    if (DISABLE_CSRF) return true;
    //console.log(arguments);
    console.log("exports.validate: arguments length: " + arguments.length);
    if (arguments.length === 1) {
        token = exports.requestToken(req);
    }
    console.log("exports.validate: Token: \"" + token + "\"");
    console.log("exports.validate: typeof Token: \"" + (typeof token) + "\"");
    if (typeof token !== 'string') {
        return false;
    }
    var slicedToken = token.slice(0, exports.SECRET_LENGTH);
    var sessionStuff = req.session[exports.SECRET_KEY];
    var tokenizedToken = tokenize(slicedToken, sessionStuff);
    console.log("exports.validate: sliced token: \"" + slicedToken + "\"");
    console.log("exports.validate: session info: \"" + sessionStuff + "\"");
    console.log("exports.validate: tokenized token: \"" + tokenizedToken + "\"");
    return scmp(token, tokenizedToken);
};

The token is inside the cookie but isn't reachable with any of the techniques in the code.

[uhhhh2]

Made a few more changes. Below are the results:

exports.validate: arguments length: 1
exports.requestToken: req has body
exports.requestToken: body keys: 
exports.requestToken: req has headers
exports.requestToken: header keys: host,connection,content-length,accept,origin,x-requested-with,user-agent,dnt,referer,accept-encoding,accept-language,cookie
exports.requestToken: cookie: "language=en-US; keystone.uid=s%3A57dc73483e17a60e3bcce0d9%3Ai7%2BCN1jQkyZG0P55DLMIOWf9Tn8O9wPFO3UG%2FjfmkHE.RsJpve4LPAdYVmG0Mf1gFQLMmXsddhBYGEWJtDK0vB4; this.sid=s%3Aom5OvtxuIwLo-882HH3LozkcQsXV_LY8.8EwazB0%2BZg2NbdXNcyslOvTJEwgN%2BKDxyJDWC0FCNiQ; XSRF-TOKEN=MdV6vN9TeT79ae9fea7a535b11f2af516225653176dd5065e6"
exports.requestToken: typeof cookie: "string"
exports.requestToken: cookies (plural): "[object Object]"
exports.requestToken: cookies (plural).keys: "language,XSRF-TOKEN"
exports.requestToken: req has query
exports.requestToken: query keys: 
exports.requestToken: case8
exports.requestToken: tok: "MdV6vN9TeT79ae9fea7a535b11f2af516225653176dd5065e6"
exports.validate: Token: "MdV6vN9TeT79ae9fea7a535b11f2af516225653176dd5065e6"
exports.validate: typeof Token: "string"
exports.validate: sliced token: "MdV6vN9TeT"
exports.validate: session info: "bNvPqBEJrPO0Lw=="
exports.validate: tokenized token: "MdV6vN9TeT79ae9fea7a535b11f2af516225653176dd5065e6"

And below are the changes (same file as last comment). The major changes are the addition of cases 5-8, and a few more debugging statements I added in the process of coming up with cases 5-8.

exports.requestToken = function (req) {
    var tok = '';
    if(req.body){
        console.log("exports.requestToken: req has body");
        console.log("exports.requestToken: body keys: " + Object.keys(req.body));
    }
    if(req.headers){
        console.log("exports.requestToken: req has headers");
        console.log("exports.requestToken: header keys: " + Object.keys(req.headers));
        console.log("exports.requestToken: cookie: \"" + req.headers.cookie +"\"");
        console.log("exports.requestToken: typeof cookie: \"" + (typeof req.headers.cookie) +"\"");
        console.log("exports.requestToken: cookies (plural): \"" + req.cookies +"\"");
        console.log("exports.requestToken: cookies (plural).keys: \"" + Object.keys(req.cookies) +"\"");
    }
    if(req.query){
        console.log("exports.requestToken: req has query");
        console.log("exports.requestToken: query keys: " + Object.keys(req.query));
    }
    if (req.body && req.body[exports.TOKEN_KEY]){
        console.log("exports.requestToken: case1");
        tok= req.body[exports.TOKEN_KEY];
    } else if (req.query && req.query[exports.TOKEN_KEY]) {
        console.log("exports.requestToken: case2");
        tok= req.query[exports.TOKEN_KEY];
    } else if (req.headers && req.headers[exports.XSRF_HEADER_KEY]) {
        console.log("exports.requestToken: case3");
        tok= req.headers[exports.XSRF_HEADER_KEY];
    } else if (req.headers && req.headers[exports.CSRF_HEADER_KEY]) {
        console.log("exports.requestToken: case4");
        tok= req.headers[exports.CSRF_HEADER_KEY];
    } else if (req.cookies && req.cookies[exports.TOKEN_KEY]){
        console.log("exports.requestToken: case5");
        tok= req.cookies[exports.TOKEN_KEY];
    } else if (req.cookies && req.cookies[exports.XSRF_HEADER_KEY]) {
        console.log("exports.requestToken: case6");
        tok= req.cookies[exports.XSRF_HEADER_KEY];
    } else if (req.cookies && req.cookies[exports.CSRF_HEADER_KEY]) {
        console.log("exports.requestToken: case7");
        tok= req.cookies[exports.CSRF_HEADER_KEY];
    } else if (req.cookies && req.cookies[exports.XSRF_COOKIE_KEY]) {
        console.log("exports.requestToken: case8");
        tok= req.cookies[exports.XSRF_COOKIE_KEY];
    } else{
        console.log("exports.requestToken: give up");
    }
    console.log("exports.requestToken: tok: \"" + tok + "\"");
    return tok;
};

Also, if my interpretation of the csrf.js file is correct, this would have impacted anything that didn't use a "safe" method like GET. So this would impact any Delete use cases as well.

[uhhhh2]

UPDATED: Create, update, and delete work for authenticated users (with admin access) with the version of /lib/security/csrf.js attached to this comment. I have NOT tested these with unauthenticated or non-admin users.

Attached is a cleaned up version of /lib/security/csrf.js. I ran the npm tests on a separate copy of Keystone (pulled from master at night on 2016-09-17), with the modified /lib/security/csrf.js put in. The results are attached as testresults.txt.

As for the test cases that fail without this, the update test case was the one already in the keystone-api-tests repository that @christroutner mentioned earlier. I have also created a create test case and a delete test case based on the pre-existing update one. Both new cases are in the tests.html file (also attached).

The file names and/or extensions may be different than the text indicated in the link (extensions had to be changed for some files to be attached).

csrf.js
testresults.txt
tests.html

[christroutner]

I've verified that the updated csrf.js file that @uhhhh2 posted works. I've updated the keystone-api-tests repository to reflect a successful outcome.

Hey @mxstbr can we get this accepted into the main repository? Should I create my own fork and submit a pull request?