Fix user-literal rewrite for anonymous requests

Currently any `currentUserLiteral` routes when accessed with a bad
token throw a 500 due to a SQL error that is raised because
`Model.findById` is invoked with `id={currentUserLiteral}`
(`id=me` in our case) when the url rewrite fails.

This commit changes the token middleware to return 401 Not Authorized
when the client is requesting a currentUserLiteral route without
a valid access token.
This commit is contained in:
Aaron Buchanan 2017-03-17 17:08:17 -07:00 committed by Miroslav Bajtoš
parent 0499d09d6b
commit fbf818b2dc
No known key found for this signature in database
GPG Key ID: 797723F23CE0A94A
2 changed files with 47 additions and 10 deletions

View File

@ -8,6 +8,7 @@
*/ */
'use strict'; 'use strict';
var g = require('../../lib/globalize');
var loopback = require('../../lib/loopback'); var loopback = require('../../lib/loopback');
var assert = require('assert'); var assert = require('assert');
var debug = require('debug')('loopback:middleware:token'); var debug = require('debug')('loopback:middleware:token');
@ -21,18 +22,33 @@ module.exports = token;
/* /*
* Rewrite the url to replace current user literal with the logged in user id * Rewrite the url to replace current user literal with the logged in user id
*/ */
function rewriteUserLiteral(req, currentUserLiteral) { function rewriteUserLiteral(req, currentUserLiteral, next) {
if (req.accessToken && req.accessToken.userId && currentUserLiteral) { if (!currentUserLiteral) return next();
const literalRegExp = new RegExp('/' + currentUserLiteral + '(/|$|\\?)', 'g');
if (req.accessToken && req.accessToken.userId) {
// Replace /me/ with /current-user-id/ // Replace /me/ with /current-user-id/
var urlBeforeRewrite = req.url; var urlBeforeRewrite = req.url;
req.url = req.url.replace( req.url = req.url.replace(literalRegExp,
new RegExp('/' + currentUserLiteral + '(/|$|\\?)', 'g'),
'/' + req.accessToken.userId + '$1'); '/' + req.accessToken.userId + '$1');
if (req.url !== urlBeforeRewrite) { if (req.url !== urlBeforeRewrite) {
debug('req.url has been rewritten from %s to %s', urlBeforeRewrite, debug('req.url has been rewritten from %s to %s', urlBeforeRewrite,
req.url); req.url);
} }
} else if (!req.accessToken && literalRegExp.test(req.url)) {
debug(
'URL %s matches current-user literal %s,' +
' but no (valid) access token was provided.',
req.url, currentUserLiteral);
var e = new Error(g.f('Authorization Required'));
e.status = e.statusCode = 401;
e.code = 'AUTHORIZATION_REQUIRED';
return next(e);
} }
next();
} }
function escapeRegExp(str) { function escapeRegExp(str) {
@ -105,23 +121,24 @@ function token(options) {
if (!enableDoublecheck) { if (!enableDoublecheck) {
// req.accessToken is defined already (might also be "null" or "false") and enableDoublecheck // req.accessToken is defined already (might also be "null" or "false") and enableDoublecheck
// has not been set --> skip searching for credentials // has not been set --> skip searching for credentials
rewriteUserLiteral(req, currentUserLiteral); return rewriteUserLiteral(req, currentUserLiteral, next);
return next();
} }
if (req.accessToken && req.accessToken.id && !overwriteExistingToken) { if (req.accessToken && req.accessToken.id && !overwriteExistingToken) {
// req.accessToken.id is defined, which means that some other middleware has identified a valid user. // req.accessToken.id is defined, which means that some other middleware has identified a valid user.
// when overwriteExistingToken is not set to a truthy value, skip searching for credentials. // when overwriteExistingToken is not set to a truthy value, skip searching for credentials.
rewriteUserLiteral(req, currentUserLiteral); return rewriteUserLiteral(req, currentUserLiteral, next);
return next();
} }
// continue normal operation (as if req.accessToken was undefined) // continue normal operation (as if req.accessToken was undefined)
} }
TokenModel.findForRequest(req, options, function(err, token) { TokenModel.findForRequest(req, options, function(err, token) {
req.accessToken = token || null; req.accessToken = token || null;
rewriteUserLiteral(req, currentUserLiteral);
var ctx = req.loopbackContext; var ctx = req.loopbackContext;
if (ctx && ctx.active) ctx.set('accessToken', token); if (ctx && ctx.active) ctx.set('accessToken', token);
next(err);
if (err) return next(err);
rewriteUserLiteral(req, currentUserLiteral, next);
}); });
}; };
} }

View File

@ -289,6 +289,26 @@ describe('loopback.token(options)', function() {
}); });
}); });
it('should generate a 401 on a current user literal route without an authToken',
function(done) {
var app = createTestApp(null, done);
request(app)
.get('/users/me')
.set('authorization', null)
.expect(401)
.end(done);
});
it('should generate a 401 on a current user literal route with invalid authToken',
function(done) {
var app = createTestApp(this.token, done);
request(app)
.get('/users/me')
.set('Authorization', 'invald-token-id')
.expect(401)
.end(done);
});
it('should skip when req.token is already present', function(done) { it('should skip when req.token is already present', function(done) {
var tokenStub = {id: 'stub id'}; var tokenStub = {id: 'stub id'};
app.use(function(req, res, next) { app.use(function(req, res, next) {