From de4d7688317829e2a0b665fe9dbf39eaf127b6f9 Mon Sep 17 00:00:00 2001 From: Daniel Hiller Date: Fri, 28 Aug 2026 20:22:15 +0200 Subject: [PATCH] fix: close the gaps a security review found in the default install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 9 ++ docker-backup.sh | 53 ++++++++- docker-compose.yml | 4 + .../api/controllers/access-tokens/create.js | 36 ++++++ .../controllers/access-tokens/verify-totp.js | 18 +++ .../api/helpers/attachments/process-link.js | 9 +- server/api/helpers/utils/check-rate-limit.js | 73 ++++++++++++ .../api/helpers/utils/is-safe-remote-url.js | 107 ++++++++++++++++++ .../api/helpers/utils/resolve-access-token.js | 69 +++++++++++ server/api/hooks/current-user/index.js | 41 ++----- server/api/models/Session.js | 7 ++ server/config/bootstrap.js | 69 +++++++---- server/config/custom.js | 18 +++ server/config/routes.js | 22 +++- .../20260827130000_add_totp_attempt_limit.js | 25 ++++ 15 files changed, 497 insertions(+), 63 deletions(-) create mode 100644 server/api/helpers/utils/check-rate-limit.js create mode 100644 server/api/helpers/utils/is-safe-remote-url.js create mode 100644 server/api/helpers/utils/resolve-access-token.js create mode 100644 server/db/migrations/20260827130000_add_totp_attempt_limit.js diff --git a/CHANGELOG.md b/CHANGELOG.md index c5aaff75..624d605e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ ## [Unreleased] +### Security + +* Limit sign-in attempts per client address and per account +* Destroy the pending session after repeated wrong two-factor codes, so a pending token can no longer be guessed against for its full lifetime +* Check the session, the account status and the password age — not only the token signature — before serving avatars, background images and favicons, so a revoked token no longer works there +* Refuse server-side fetches to private, loopback and link-local addresses when a link attachment's favicon is retrieved +* Warn on every start while `SECRET_KEY` is missing, too short, or still the value from the example configuration +* Encrypt the backup archive when `BACKUP_PASSPHRASE` is set, and say what an unencrypted one contains + ### Added * Add `db:clean-orphaned-records` script to report and remove reference rows left behind by an incomplete delete diff --git a/docker-backup.sh b/docker-backup.sh index 3add973d..82ae1091 100644 --- a/docker-backup.sh +++ b/docker-backup.sh @@ -1,4 +1,29 @@ #!/bin/bash +# +# An EXAMPLE, for the stock docker-compose stack and nothing else. It is not a +# backup concept, and PLANKA does not ship one: what has to be preserved depends +# on how you run it. On k3s with S3-backed uploads and a managed database +# cluster this script protects nothing — your object store and your database +# have their own answers, and those are the ones that count. +# +# Two things to know before relying on it: +# +# 1. The archive is written in the clear unless you set BACKUP_PASSPHRASE. It +# contains the whole database: password hashes, active sessions, TOTP +# secrets and recovery codes, SMTP credentials and any API keys. Treat an +# unencrypted archive as equivalent to shell access on the instance. +# +# 2. It runs against a live instance, so the database and the uploaded files +# are captured moments apart. The database goes first on purpose: something +# created in between leaves a file with no row, which is inert. The other +# order would leave rows pointing at files that were never copied. Neither +# order survives a deletion landing in the gap. For a backup with no such +# window, stop the app for the duration, or use your database's +# point-in-time recovery together with a volume snapshot. +# +# Usage: +# ./docker-backup.sh [target-directory] +# BACKUP_PASSPHRASE='…' ./docker-backup.sh [target-directory] # Stop on error set -e @@ -36,14 +61,32 @@ echo "Success!" echo echo -n "Exporting data volume ... " -docker run --rm --volumes-from "$DOCKER_CONTAINER_PLANKA" -v "$BACKUP_TEMP:/backup" node:22-alpine cp -r /app/data /backup/data +docker run --rm --volumes-from "$DOCKER_CONTAINER_PLANKA" -v "$BACKUP_TEMP:/backup" node:24-alpine cp -r /app/data /backup/data echo "Success!" echo -echo -n "Creating final tarball $BACKUP_DATETIME-backup.tgz ... " -tar -C "$BACKUP_DIR" -czf "$BACKUP_TEMP.tgz" "$BACKUP_DATETIME-backup" -echo "Success!" -echo +if [ -n "$BACKUP_PASSPHRASE" ]; then + echo -n "Creating encrypted archive $BACKUP_DATETIME-backup.tgz.enc ... " + tar -C "$BACKUP_DIR" -czf - "$BACKUP_DATETIME-backup" \ + | openssl enc -aes-256-cbc -pbkdf2 -iter 600000 -salt \ + -pass env:BACKUP_PASSPHRASE -out "$BACKUP_TEMP.tgz.enc" + echo "Success!" + echo + echo "Restore with:" + echo " openssl enc -d -aes-256-cbc -pbkdf2 -iter 600000 \\" + echo " -pass env:BACKUP_PASSPHRASE -in $BACKUP_DATETIME-backup.tgz.enc | tar -xzf -" + echo +else + echo -n "Creating final tarball $BACKUP_DATETIME-backup.tgz ... " + tar -C "$BACKUP_DIR" -czf "$BACKUP_TEMP.tgz" "$BACKUP_DATETIME-backup" + echo "Success!" + echo + echo "WARNING: this archive is NOT encrypted. It carries password hashes," + echo " active sessions, TOTP secrets and SMTP credentials in the" + echo " clear. Set BACKUP_PASSPHRASE to encrypt it, and store it" + echo " where you would store a copy of the database itself." + echo +fi echo -n "Cleaning up temporary files and directories ... " rm -rf "$BACKUP_TEMP" diff --git a/docker-compose.yml b/docker-compose.yml index d7383be9..7770c371 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -22,6 +22,10 @@ services: # secrets: # - database_password + # REPLACE THIS. Every access token is signed with it, so leaving the + # published example value means anyone can mint a token for any account + # on this instance. Generate one with: openssl rand -hex 32 + # PLANKA warns about this on every start until you change it. - SECRET_KEY=notsecretkey # Optionally store in secrets - then SECRET_KEY should not be set # - SECRET_KEY__FILE=/run/secrets/secret_key diff --git a/server/api/controllers/access-tokens/create.js b/server/api/controllers/access-tokens/create.js index f5269899..a3e4f985 100755 --- a/server/api/controllers/access-tokens/create.js +++ b/server/api/controllers/access-tokens/create.js @@ -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) { diff --git a/server/api/controllers/access-tokens/verify-totp.js b/server/api/controllers/access-tokens/verify-totp.js index d2e3d53b..6416b47b 100644 --- a/server/api/controllers/access-tokens/verify-totp.js +++ b/server/api/controllers/access-tokens/verify-totp.js @@ -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; } diff --git a/server/api/helpers/attachments/process-link.js b/server/api/helpers/attachments/process-link.js index e543b97f..7d95ddc3 100644 --- a/server/api/helpers/attachments/process-link.js +++ b/server/api/helpers/attachments/process-link.js @@ -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); } diff --git a/server/api/helpers/utils/check-rate-limit.js b/server/api/helpers/utils/check-rate-limit.js new file mode 100644 index 00000000..f146d978 --- /dev/null +++ b/server/api/helpers/utils/check-rate-limit.js @@ -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 }; + }, +}; diff --git a/server/api/helpers/utils/is-safe-remote-url.js b/server/api/helpers/utils/is-safe-remote-url.js new file mode 100644 index 00000000..c4d296ea --- /dev/null +++ b/server/api/helpers/utils/is-safe-remote-url.js @@ -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)); + }, +}; diff --git a/server/api/helpers/utils/resolve-access-token.js b/server/api/helpers/utils/resolve-access-token.js new file mode 100644 index 00000000..91e3ed62 --- /dev/null +++ b/server/api/helpers/utils/resolve-access-token.js @@ -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, + }; + }, +}; diff --git a/server/api/hooks/current-user/index.js b/server/api/hooks/current-user/index.js index 2dbc1ca3..071272e3 100644 --- a/server/api/hooks/current-user/index.js +++ b/server/api/hooks/current-user/index.js @@ -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); diff --git a/server/api/models/Session.js b/server/api/models/Session.js index 58d17de5..2f529d28 100755 --- a/server/api/models/Session.js +++ b/server/api/models/Session.js @@ -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, diff --git a/server/config/bootstrap.js b/server/config/bootstrap.js index 68434e7a..36e19bdd 100644 --- a/server/config/bootstrap.js +++ b/server/config/bootstrap.js @@ -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', + ]); + } }; diff --git a/server/config/custom.js b/server/config/custom.js index da4aaf8c..bcd201bb 100644 --- a/server/config/custom.js +++ b/server/config/custom.js @@ -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), diff --git a/server/config/routes.js b/server/config/routes.js index d3e5ae33..2250a963 100644 --- a/server/config/routes.js +++ b/server/config/routes.js @@ -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); } diff --git a/server/db/migrations/20260827130000_add_totp_attempt_limit.js b/server/db/migrations/20260827130000_add_totp_attempt_limit.js new file mode 100644 index 00000000..f9171361 --- /dev/null +++ b/server/db/migrations/20260827130000_add_totp_attempt_limit.js @@ -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'); + });