mirror of
https://github.com/NginxProxyManager/nginx-proxy-manager.git
synced 2026-09-19 16:24:09 +01:00
Invalidate tokens issued before a password change
Tokens are stateless JWTs, so changing a password left every session that the old one had opened working until its own expiry, up to a day later. That is the case the password change is meant to close: an administrator resetting a compromised account did not evict whoever was already in it. The auth row already records when the password last changed, so no migration is needed: `Access.init()` reads it alongside the user it already loads and refuses a token whose `iat` is older. Both sides are compared as whole seconds, which is all `iat` carries, so a token minted in the same second as the change is kept. Postgres stores that column to the microsecond, which is why the comparison is not done in milliseconds. It is reported as 401 rather than the usual 403 because that is what the frontend clears the session on, so the browser holding the dead token lands on the login page instead of a page full of errors, and `can()` lets that one error through unwrapped for the same reason. Only the password does this. A user row changing (a rename, an avatar, permissions) does not, and a user with no password auth row, which is what a login through an external provider looks like, is not affected.
This commit is contained in:
@@ -12,6 +12,7 @@ import { fileURLToPath } from "node:url";
|
||||
import Ajv from "ajv/dist/2020.js";
|
||||
import _ from "lodash";
|
||||
import { access as logger } from "../logger.js";
|
||||
import authModel from "../models/auth.js";
|
||||
import proxyHostModel from "../models/proxy_host.js";
|
||||
import TokenModel from "../models/token.js";
|
||||
import userModel from "../models/user.js";
|
||||
@@ -80,6 +81,29 @@ export default function (tokenString) {
|
||||
if (!ok) {
|
||||
throw new errs.AuthError("Invalid token scope for User");
|
||||
}
|
||||
|
||||
// A token issued before the password was last changed is no longer valid: taking an account
|
||||
// back from whoever has the old password has to end the sessions that password opened.
|
||||
const auth = await authModel
|
||||
.query()
|
||||
.where("user_id", "=", user.id)
|
||||
.where("type", "=", "password")
|
||||
.first();
|
||||
|
||||
if (auth && typeof tokenData.iat === "number") {
|
||||
// SQLite gives this back as a local time string, the other drivers as a Date.
|
||||
const changedAt =
|
||||
auth.modified_on instanceof Date
|
||||
? auth.modified_on.getTime()
|
||||
: Date.parse(String(auth.modified_on).replace(" ", "T"));
|
||||
|
||||
// Whole seconds on both sides, which is all `iat` carries, so a token issued in the
|
||||
// same second as the change is kept. Postgres stores this column to the microsecond.
|
||||
if (!Number.isNaN(changedAt) && tokenData.iat < Math.floor(changedAt / 1000)) {
|
||||
throw new errs.TokenRevokedError("Token was issued before the password was changed");
|
||||
}
|
||||
}
|
||||
|
||||
initialised = true;
|
||||
userRoles = user.roles;
|
||||
permissions = user.permissions;
|
||||
@@ -268,6 +292,11 @@ export default function (tokenString) {
|
||||
err.permission = permission;
|
||||
err.permission_data = data;
|
||||
logger.error(permission, data, err.message);
|
||||
// A revoked token is not a permission problem, and the client can tell: the frontend
|
||||
// clears the session on a 401 and on nothing else.
|
||||
if (err instanceof errs.TokenRevokedError) {
|
||||
throw err;
|
||||
}
|
||||
throw new errs.PermissionError("Permission Denied", err);
|
||||
}
|
||||
},
|
||||
|
||||
@@ -22,6 +22,15 @@ const errs = {
|
||||
this.status = 404;
|
||||
},
|
||||
|
||||
TokenRevokedError: function (message, previous) {
|
||||
Error.captureStackTrace(this, this.constructor);
|
||||
this.name = this.constructor.name;
|
||||
this.previous = previous;
|
||||
this.message = message;
|
||||
this.public = true;
|
||||
this.status = 401;
|
||||
},
|
||||
|
||||
AuthError: function (message, messageI18n, previous) {
|
||||
Error.captureStackTrace(this, this.constructor);
|
||||
this.name = this.constructor.name;
|
||||
|
||||
@@ -31,6 +31,49 @@ describe('Users endpoints', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('Should reject a token that was issued before the password changed', () => {
|
||||
// The token carries whole seconds, so it has to predate the change by one.
|
||||
cy.wait(1100);
|
||||
|
||||
cy.task('backendApiPut', {
|
||||
token: token,
|
||||
path: '/api/users/me/auth',
|
||||
data: {
|
||||
type: 'password',
|
||||
current: 'changeme',
|
||||
secret: 'changeme2'
|
||||
}
|
||||
}).then(() => {
|
||||
cy.task('backendApiGet', {
|
||||
token: token,
|
||||
path: '/api/users/me',
|
||||
returnOnError: true
|
||||
}).then((data) => {
|
||||
expect(data).to.have.property('error');
|
||||
expect(data.error).to.have.property('code');
|
||||
expect(data.error.code).to.equal(401);
|
||||
});
|
||||
|
||||
// Put the password back, the rest of the suite shares this user, and take a
|
||||
// token minted after the change: restoring it invalidates the one that made it.
|
||||
cy.getToken(null, {secret: 'changeme2'}).then((tempToken) => {
|
||||
cy.task('backendApiPut', {
|
||||
token: tempToken,
|
||||
path: '/api/users/me/auth',
|
||||
data: {
|
||||
type: 'password',
|
||||
current: 'changeme2',
|
||||
secret: 'changeme'
|
||||
}
|
||||
}).then(() => {
|
||||
cy.getToken().then((freshToken) => {
|
||||
token = freshToken;
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
it('Should be able to update yourself', () => {
|
||||
cy.task('backendApiPut', {
|
||||
token: token,
|
||||
|
||||
Reference in New Issue
Block a user