fix: close the gaps a security review found in the default install

Reported against 2.2.1 by someone reading the source. Every finding held.

**Sign-in had no ceiling.** Failures were logged with the caller's address
and nothing more. Two counters now — one per address, one per account —
because the two attacks look different: one source working through many
accounts is caught by the first, many sources working on one account by the
second, and behind a proxy only the second still means anything.

The count is kept in the process that serves the request. PLANKA needs no
Redis and the stock deployment is one container; run several and each keeps
its own count, which multiplies the ceiling by their number. That trade is
written where the limits are configured.

**The second factor could be guessed at leisure.** Six digits, and a
pending token that stayed valid for its full ten minutes however many codes
were wrong. Wrong codes are now counted on the session row — in the
database, so the count survives a restart and holds across every process —
and when the budget is spent the session is destroyed. After that even the
right code is refused and the login starts over from the password.

**Avatars, background images and favicons** checked the token's signature
and nothing else, so a revoked session, a deactivated account or a changed
password all kept working there for as long as the signature lasted, which
is a year by default. The five checks the API makes now live in one helper
that both use, rather than the shortened copy that had drifted from it.

**A link attachment's favicon** was fetched from wherever the URL pointed.
Storing a link is harmless — it is a string the user typed — but fetching
its icon is a request the server makes to an address the user chose, and
whether an icon came back reported on what is reachable from inside the
network. Server-side fetches now refuse private, loopback and link-local
addresses, `169.254.169.254` among them. The attachment is still created:
linking to an internal wiki is a legitimate thing to do, and it was the
server's own request that had to stop.

**The signing key.** Our own compose file ships `notsecretkey`, and it is
printed in the documentation — so on any instance that copied it, anyone can
sign a token for any account. PLANKA now says so on every start, and keeps
saying it, along with a key that is missing or shorter than 32 characters.
The placeholder carries the warning inline, where it is copied from.

**The backup script** wrote password hashes, live sessions, TOTP secrets and
SMTP credentials to an unencrypted archive. `BACKUP_PASSPHRASE` now encrypts
it, and without one the script says what it just put on disk. It also says
what it is — an example for the stock compose stack, not a backup concept —
and names the window between the database dump and the file copy, which no
ordering closes.
This commit is contained in:
Daniel Hiller
2026-08-28 20:55:54 +02:00
parent 6def0330c2
commit de4d768831
15 changed files with 497 additions and 63 deletions
@@ -123,6 +123,9 @@ const Errors = {
TERMS_ACCEPTANCE_REQUIRED: {
termsAcceptanceRequired: 'Terms acceptance required',
},
RATE_LIMIT_EXCEEDED: {
rateLimitExceeded: 'Rate limit exceeded',
},
};
module.exports = {
@@ -153,6 +156,9 @@ module.exports = {
invalidPassword: {
responseType: 'unauthorized',
},
rateLimitExceeded: {
responseType: 'conflict',
},
termsAcceptanceRequired: {
responseType: 'forbidden',
},
@@ -166,6 +172,36 @@ module.exports = {
async fn(inputs) {
const remoteAddress = getRemoteAddress(this.req);
// Counted before the lookup, so a script cannot make the database do the
// work of telling it that an account does not exist. Two counters: one
// source against many accounts is caught per address, many sources against
// one account per identifier — and behind a proxy only the second still
// means anything, which is why both are here.
const identifier = inputs.emailOrUsername.trim().toLowerCase();
// eslint-disable-next-line no-restricted-syntax
for (const [key, max] of [
[`auth:ip:${remoteAddress}`, sails.config.custom.authRateLimitMaxPerIp],
[
// Hashed so the key that lives in memory for the window is not the
// address itself.
`auth:identifier:${sails.helpers.utils.hash(identifier)}`,
sails.config.custom.authRateLimitMaxPerIdentifier,
],
]) {
const { isExceeded } = sails.helpers.utils.checkRateLimit.with({
key,
windowSeconds: sails.config.custom.authRateLimitWindow,
max,
});
if (isExceeded) {
sails.log.warn(`Login rate limit hit (IP: ${remoteAddress})`);
throw Errors.RATE_LIMIT_EXCEEDED;
}
}
const user = await User.qm.getOneActiveByEmailOrUsername(inputs.emailOrUsername);
if (!user) {
@@ -157,6 +157,24 @@ module.exports = {
if (!codeAccepted) {
sails.log.warn(`Invalid TOTP code! (IP: ${remoteAddress})`);
// Incremented in the database rather than read-then-written, so two
// requests racing on the same pending token cannot each see the old
// count and spend the budget twice.
const queryResult = await sails.sendNativeQuery(
'UPDATE session SET pending_token_attempts = pending_token_attempts + 1, updated_at = $1 WHERE id = $2 RETURNING pending_token_attempts',
[new Date().toISOString(), session.id],
);
const [row] = queryResult.rows;
if (row && row.pending_token_attempts > sails.config.custom.totpMaxAttempts) {
sails.log.warn(`TOTP attempts exhausted, dropping session (IP: ${remoteAddress})`);
await Session.qm.deleteOneById(session.id);
throw Errors.INVALID_PENDING_TOKEN;
}
throw Errors.INVALID_TOTP_CODE;
}
@@ -16,7 +16,14 @@ module.exports = {
async fn(inputs) {
const { hostname } = new URL(inputs.url);
if (!sails.helpers.utils.isPreloadedFaviconExists(hostname)) {
// The link itself is only a stored string and stays whatever the user
// typed. Fetching its favicon, though, is a request the SERVER makes to an
// address the user chose — without a guard an editor could aim it at
// `169.254.169.254` or an internal service and read the outcome from
// whether an icon appeared.
const isSafe = await sails.helpers.utils.isSafeRemoteUrl(inputs.url);
if (isSafe && !sails.helpers.utils.isPreloadedFaviconExists(hostname)) {
await sails.helpers.utils.downloadFavicon(inputs.url);
}
@@ -0,0 +1,73 @@
/*!
* Copyright (c) 2026 PLANKA Software GmbH
* Licensed under the Fair Use License: https://github.com/plankanban/planka/blob/master/LICENSE.md
*/
// A fixed-window counter, held in the process that serves the request.
//
// PLANKA needs no Redis and the stock deployment is a single container, so the
// count lives in memory rather than in shared storage. Run several processes
// and each keeps its own, which multiplies the effective ceiling by their
// number — enough to blunt a script, not a substitute for a limiter in front of
// the app. That trade is stated where the limits are configured.
//
// The verdict comes back in the return value rather than as an exit: an exit
// reaches an `await` as an Error wrapping its name, which is easy to catch
// wrongly and easy to catch wrongly in silence.
const buckets = new Map();
// Beyond this many live keys, sweep what has expired before adding more. An
// attacker can mint keys freely — one per made-up account name — so the map
// must not be allowed to grow with them.
const SWEEP_THRESHOLD = 10000;
const sweep = (now) => {
buckets.forEach((bucket, key) => {
if (bucket.expiresAt <= now) {
buckets.delete(key);
}
});
};
module.exports = {
inputs: {
key: {
type: 'string',
required: true,
},
windowSeconds: {
type: 'number',
required: true,
},
max: {
type: 'number',
required: true,
},
},
sync: true,
fn(inputs) {
const now = Date.now();
if (buckets.size > SWEEP_THRESHOLD) {
sweep(now);
}
const bucket = buckets.get(inputs.key);
if (!bucket || bucket.expiresAt <= now) {
buckets.set(inputs.key, {
count: 1,
expiresAt: now + inputs.windowSeconds * 1000,
});
return { count: 1, isExceeded: inputs.max < 1 };
}
bucket.count += 1;
return { count: bucket.count, isExceeded: bucket.count > inputs.max };
},
};
@@ -0,0 +1,107 @@
/*!
* Copyright (c) 2026 PLANKA Software GmbH
* Licensed under the Fair Use License: https://github.com/plankanban/planka/blob/master/LICENSE.md
*/
// SSRF guard for server-side fetches of user-supplied URLs (link previews).
// Rejects anything but http/https and any host that resolves to a private,
// loopback, link-local or otherwise-internal address — so a pasted
// `http://169.254.169.254/…` or `http://localhost:1337` can't reach the
// internal network. When an outgoing proxy is configured, egress policy is the
// proxy's responsibility and we defer to it.
const net = require('net');
const dns = require('dns').promises;
const { URL } = require('url');
const isPrivateIPv4 = (ip) => {
const parts = ip.split('.').map((octet) => parseInt(octet, 10));
if (parts.length !== 4 || parts.some((octet) => Number.isNaN(octet))) {
return true;
}
const [a, b] = parts;
return (
a === 0 || // "this" network
a === 10 ||
a === 127 || // loopback
(a === 100 && b >= 64 && b <= 127) || // CGNAT 100.64.0.0/10
(a === 169 && b === 254) || // link-local (cloud metadata)
(a === 172 && b >= 16 && b <= 31) || // 172.16.0.0/12
(a === 192 && b === 168)
);
};
const LOCAL_IPV6_PREFIXES = ['fe8', 'fe9', 'fea', 'feb', 'fc', 'fd'];
const isPrivateIPv6 = (ip) => {
const address = ip.toLowerCase();
if (address === '::1' || address === '::') {
return true;
}
// Link-local (fe80::/10) and unique-local (fc00::/7 → fc/fd prefixes).
if (LOCAL_IPV6_PREFIXES.some((prefix) => address.startsWith(prefix))) {
return true;
}
// IPv4-mapped (::ffff:a.b.c.d) — check the embedded v4.
const mapped = address.match(/::ffff:(\d+\.\d+\.\d+\.\d+)$/);
if (mapped) {
return isPrivateIPv4(mapped[1]);
}
return false;
};
const isPrivateAddress = (ip) => {
const family = net.isIP(ip);
if (family === 4) {
return isPrivateIPv4(ip);
}
if (family === 6) {
return isPrivateIPv6(ip);
}
return true; // not a recognizable IP → treat as unsafe
};
module.exports = {
inputs: {
url: {
type: 'string',
required: true,
},
},
async fn(inputs) {
let parsed;
try {
parsed = new URL(inputs.url);
} catch (error) {
return false;
}
if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
return false;
}
// Egress through a trusted proxy → let the proxy enforce destination policy.
if (sails.config.custom.outgoingProxy) {
return true;
}
const host = parsed.hostname.toLowerCase();
if (host === 'localhost' || host.endsWith('.localhost')) {
return false;
}
if (net.isIP(host)) {
return !isPrivateAddress(host);
}
let addresses;
try {
addresses = await dns.lookup(host, { all: true });
} catch (error) {
return false;
}
return addresses.length > 0 && addresses.every(({ address }) => !isPrivateAddress(address));
},
};
@@ -0,0 +1,69 @@
/*!
* Copyright (c) 2026 PLANKA Software GmbH
* Licensed under the Fair Use License: https://github.com/plankanban/planka/blob/master/LICENSE.md
*/
// Everything that has to be true before an access token means anything, in one
// place. A valid signature is the first of five, not the whole answer: a token
// keeps its signature after the session behind it was revoked, after the
// account was deactivated, and after the password was changed — and the default
// lifetime is a year.
//
// It lives here rather than inside the `current-user` hook because the routes
// that serve avatars, background images and favicons need the same answer, and
// when they had their own shortened version of it they let revoked tokens
// through for as long as the signature lasted.
//
// Returns null whenever the token should not be honoured, so a caller cannot
// mistake "no session" for a session.
module.exports = {
inputs: {
accessToken: {
type: 'string',
required: true,
},
// The paired cookie value, where the session was issued with one. A session
// that carries an httpOnlyToken is only valid alongside its cookie.
httpOnlyToken: {
type: 'string',
allowNull: true,
},
},
async fn(inputs) {
let payload;
try {
payload = sails.helpers.utils.verifyJwtToken(inputs.accessToken);
} catch (error) {
return null;
}
const session = await Session.qm.getOneUndeletedByAccessToken(inputs.accessToken);
if (!session) {
return null;
}
if (session.httpOnlyToken && inputs.httpOnlyToken !== session.httpOnlyToken) {
return null;
}
const user = await User.qm.getOneById(payload.subject, {
withDeactivated: false,
});
if (!user) {
return null;
}
if (user.passwordChangedAt > payload.issuedAt) {
return null;
}
return {
session,
user,
};
},
};
+7 -34
View File
@@ -15,42 +15,15 @@ module.exports = function defineCurrentUserHook(sails) {
const TOKEN_PATTERN = /^Bearer /;
const API_KEY_HEADER_NAME = 'x-api-key';
const getSessionAndUserByAccessToken = async (accessToken, httpOnlyToken) => {
let payload;
try {
payload = sails.helpers.utils.verifyJwtToken(accessToken);
} catch (error) {
return null;
}
const session = await Session.qm.getOneUndeletedByAccessToken(accessToken);
if (!session) {
return null;
}
if (session.httpOnlyToken && httpOnlyToken !== session.httpOnlyToken) {
return null;
}
const user = await User.qm.getOneById(payload.subject, {
withDeactivated: false,
// The five checks a token has to pass live in `utils/resolveAccessToken`,
// shared with the routes that serve avatars and background images so the two
// cannot answer differently.
const getSessionAndUserByAccessToken = (accessToken, httpOnlyToken) =>
sails.helpers.utils.resolveAccessToken.with({
accessToken,
httpOnlyToken: httpOnlyToken || null,
});
if (!user) {
return null;
}
if (user.passwordChangedAt > payload.issuedAt) {
return null;
}
return {
session,
user,
};
};
const getUserByApiKey = (apiKey) => {
const apiKeyHash = sails.helpers.utils.hash(apiKey);
+7
View File
@@ -28,6 +28,13 @@ module.exports = {
allowNull: true,
columnName: 'pending_token',
},
// Wrong second-factor codes entered against this pending token. Bounded by
// `totpMaxAttempts`; reaching it destroys the session.
pendingTokenAttempts: {
type: 'number',
defaultsTo: 0,
columnName: 'pending_token_attempts',
},
httpOnlyToken: {
type: 'string',
isNotEmptyString: true,
+50 -19
View File
@@ -3,26 +3,57 @@
* (sails.config.bootstrap)
*
* A function that runs just before your Sails app gets lifted.
* > Need more flexibility? You can also create a hook.
*
* For more information on seeding your app with fake data, check out:
* https://sailsjs.com/config/bootstrap
*/
// The value our own example compose file ships. Anyone who copied that file
// and never read the paragraph next to it is running with a signing key that
// is published in our documentation — which means anyone can mint a token for
// any account on their instance.
const EXAMPLE_SECRET_KEY = 'notsecretkey';
// What the documentation asks for, and what `openssl rand -hex 32` produces.
const MIN_SECRET_KEY_LENGTH = 32;
const RULE = '─'.repeat(72);
// Loud on purpose. This is not a misconfiguration that degrades a feature; it
// is the difference between sessions that can be forged and sessions that
// cannot, and it is invisible from inside a working instance.
const warn = (headline, detail) => {
sails.log.warn(RULE);
sails.log.warn(`SECURITY: ${headline}`);
detail.forEach((line) => sails.log.warn(` ${line}`));
sails.log.warn(RULE);
};
module.exports.bootstrap = async () => {
// By convention, this is a good place to set up fake data during development.
//
// For example:
// ```
// // Set up fake development data (or if we already have some, avast)
// if (await User.count() > 0) {
// return;
// }
//
// await User.createEach([
// { emailAddress: 'ry@example.com', fullName: 'Ryan Dahl', },
// { emailAddress: 'rachael@example.com', fullName: 'Rachael Shaw', },
// // etc.
// ]);
// ```
const secretKey = sails.config.session.secret;
if (!secretKey) {
warn('SECRET_KEY is not set.', [
'Every access token is signed with it. Without one, sessions cannot be',
'trusted. Generate a key with: openssl rand -hex 32',
]);
return;
}
if (secretKey === EXAMPLE_SECRET_KEY) {
warn('SECRET_KEY is still the value from the example configuration.', [
'It is published in our documentation, so anyone can sign a token for any',
'account on this instance. Replace it now: openssl rand -hex 32',
'',
'Changing it invalidates every token already issued — everyone signs in',
'again once, and that is the whole cost.',
]);
return;
}
if (secretKey.length < MIN_SECRET_KEY_LENGTH) {
warn(`SECRET_KEY is shorter than ${MIN_SECRET_KEY_LENGTH} characters.`, [
`It is ${secretKey.length}. A short key is a guessable key, and guessing it`,
'means forging sessions. Generate a proper one: openssl rand -hex 32',
]);
}
};
+18
View File
@@ -47,6 +47,24 @@ module.exports.custom = {
maxUploadFileSize: envToBytes(process.env.MAX_UPLOAD_FILE_SIZE),
tokenExpiresIn: (parseInt(process.env.TOKEN_EXPIRES_IN, 10) || 365) * 24 * 60 * 60,
// A second factor needs a harder stop than a time window: six digits fall to
// patience alone. After this many wrong codes the pending session is
// destroyed and the login starts over from the password, so the ten minutes
// a pending token is valid for stop being ten minutes of free guessing.
totpMaxAttempts: parseInt(process.env.TOTP_MAX_ATTEMPTS, 10) || 5,
// Ceiling on sign-in attempts, counted per client address and per account.
// The two attacks look different: one source working through many accounts is
// caught by the first, many sources working on one account by the second.
//
// Counted in the process that serves the request, not in shared storage —
// PLANKA needs no Redis, and the stock deployment is a single container. Run
// several and each keeps its own count, so the effective ceiling multiplies
// by the number of processes. Put a limiter in your proxy if that matters.
authRateLimitWindow: parseInt(process.env.AUTH_RATE_LIMIT_WINDOW, 10) || 60,
authRateLimitMaxPerIp: parseInt(process.env.AUTH_RATE_LIMIT_MAX_PER_IP, 10) || 30,
authRateLimitMaxPerIdentifier: parseInt(process.env.AUTH_RATE_LIMIT_MAX_PER_IDENTIFIER, 10) || 10,
storageLimit: envToBytes(process.env.STORAGE_LIMIT),
activeUsersLimit: envToNumber(process.env.ACTIVE_USERS_LIMIT),
+18 -4
View File
@@ -88,14 +88,28 @@ const serveStatic = async (prefix, getPathSegment, req, res) => {
return serveStatic(prefix, getPathSegment, req, res);
}; */
const protectedStaticDirServer = (prefix, getPathSegment) => (req, res, next) => {
// Avatars, background images and favicons. These used to check the token's
// signature and nothing else, which meant a revoked session, a deactivated
// account or a changed password all kept working here for as long as the
// signature lasted — a year by default. They now ask the same question the API
// asks, through the same helper.
const protectedStaticDirServer = (prefix, getPathSegment) => async (req, res, next) => {
if (!req.url.startsWith(prefix)) {
return next();
}
try {
sails.helpers.utils.verifyJwtToken(req.cookies.accessToken);
} catch (error) {
const { accessToken, httpOnlyToken } = req.cookies;
if (!accessToken) {
return res.sendStatus(401);
}
const sessionAndUser = await sails.helpers.utils.resolveAccessToken.with({
accessToken,
httpOnlyToken: httpOnlyToken || null,
});
if (!sessionAndUser) {
return res.sendStatus(401);
}
@@ -0,0 +1,25 @@
/*!
* Copyright (c) 2026 PLANKA Software GmbH
* Licensed under the Fair Use License: https://github.com/plankanban/planka/blob/master/LICENSE.md
*/
// Counts wrong second-factor codes against the one half-finished login they
// were entered into. A pending token stays valid for ten minutes, and six
// digits fall to patience — without a counter those ten minutes are ten
// minutes of free guessing. When the count runs out the session is destroyed
// and the login starts over from the password.
//
// It lives on the row rather than in memory so it survives a restart and holds
// across every process serving the instance.
module.exports.up = (knex) =>
knex.schema.alterTable('session', (table) => {
// The default belongs on the column: sessions are also created by raw
// inserts that never pass through the model, and those would write NULL.
table.integer('pending_token_attempts').notNullable().defaultTo(0);
});
module.exports.down = (knex) =>
knex.schema.alterTable('session', (table) => {
table.dropColumn('pending_token_attempts');
});