perf(koreader): check md5 before bcrypt when authenticating plugin requests

The KOReader plugin sends md5(password) as x-auth-key, which can never match the bcrypt hash of the
plaintext password. The auth guard ran a cost-12 bcrypt compare first, so every authenticated
request spent about 250 ms of CPU and stalled other traffic before the md5 fallback accepted it.

Check md5 first and keep bcrypt as the fallback for clients that send the plaintext password, so the
accepted credentials are unchanged. A plugin auth call drops from about 285 ms to 8 ms.

Bump the KOReader plugin to 2.2.0. The plugin changed after the 2.1.0 release shipped in v3.6.0
(series sorting, series selection, file browser exit, paged Shelves list) without a version bump.

Also fix three metadata-fetch tests that were failing on main since cfcd4ca52 kept single-digit
tokens in tokenizeTitleText. A volume digit in the query diluted the word overlap in scoreTitleMatch,
so an ebook candidate fell under the relevance floor and a manga outranked it. scoreTitleMatch now
drops lone digits again; symmetricTitleSimilarity keeps them to tell volumes apart.

Also align the pnpm version installed in both Dockerfiles with the packageManager field. Since
f2a7a1118 the images installed pnpm 11.22.0 while package.json pinned 11.28.9, so pnpm tried to
switch itself and failed the arm64 image build with a missing @pnpm/exe lockfile entry.

Closes #1983
Ref #2037
This commit is contained in:
neon
2026-11-02 04:05:48 +00:00
parent d9031a33cd
commit f79874317d
7 changed files with 60 additions and 26 deletions
+1 -1
View File
@@ -1,7 +1,7 @@
ARG NODE_IMAGE=node:26.12.1-alpine3.23@sha256:bc2b334006f15c7167bbeb2f08f28c2df883f7594a0470e04454ebe7560cfe68 ARG NODE_IMAGE=node:26.12.1-alpine3.23@sha256:bc2b334006f15c7167bbeb2f08f28c2df883f7594a0470e04454ebe7560cfe68
FROM ${NODE_IMAGE} AS base FROM ${NODE_IMAGE} AS base
RUN npm install -g pnpm@11.22.0 RUN npm install -g pnpm@11.28.9
# Stage 1: Build client # Stage 1: Build client
FROM base AS client-builder FROM base AS client-builder
+1 -1
View File
@@ -51,7 +51,7 @@ local BookOrbitSidecar = require("bookorbit_sidecar")
local BookOrbitSweep = require("bookorbit_sweep") local BookOrbitSweep = require("bookorbit_sweep")
local BookOrbitUpdater = require("bookorbit_updater") local BookOrbitUpdater = require("bookorbit_updater")
local PLUGIN_VERSION = "2.1.0" local PLUGIN_VERSION = "2.2.0"
local SYNC_STRATEGY = { local SYNC_STRATEGY = {
PROMPT = 1, PROMPT = 1,
@@ -96,6 +96,10 @@ describe('title-match', () => {
expect(scoreTitleMatch('Foundatun', 'Foundation')).toBeGreaterThan(0); expect(scoreTitleMatch('Foundatun', 'Foundation')).toBeGreaterThan(0);
}); });
it('does not let a volume digit in the query dilute the words a candidate shares', () => {
expect(scoreTitleMatch('The Faraway Paladin Volume 1', 'The Faraway Paladin: The Boy in the City of the Dead')).toBe(4);
});
it('returns zero when either side is empty after normalization', () => { it('returns zero when either side is empty after normalization', () => {
expect(scoreTitleMatch('', 'Dune')).toBe(0); expect(scoreTitleMatch('', 'Dune')).toBe(0);
expect(scoreTitleMatch('Dune', '???')).toBe(0); expect(scoreTitleMatch('Dune', '???')).toBe(0);
+8 -2
View File
@@ -81,6 +81,12 @@ export function tokenizeTitleText(value: string): string[] {
return value.split(' ').filter((token) => token.length > 1 || /^\p{N}$/u.test(token)); return value.split(' ').filter((token) => token.length > 1 || /^\p{N}$/u.test(token));
} }
// A lone digit tells volumes apart (symmetricTitleSimilarity), but in a fuzzy relevance match it dilutes the
// overlap ratio of a candidate that does not spell the volume number, pushing a right book under the floor.
function relevanceTokens(value: string): string[] {
return tokenizeTitleText(value).filter((token) => token.length > 1);
}
/** /**
* Tokens worth matching on. Falls back to the raw tokens when a title is nothing but stopwords, * Tokens worth matching on. Falls back to the raw tokens when a title is nothing but stopwords,
* so a book genuinely titled "It" or "Them" still has something to match. * so a book genuinely titled "It" or "Them" still has something to match.
@@ -134,8 +140,8 @@ export function scoreTitleMatch(query: string, candidate: string): number {
return TITLE_MATCH_SCORES.substring; return TITLE_MATCH_SCORES.substring;
} }
const queryTokens = significantTokens(tokenizeTitleText(normalizedQuery)); const queryTokens = significantTokens(relevanceTokens(normalizedQuery));
const candidateTokens = new Set(tokenizeTitleText(normalizedCandidate)); const candidateTokens = new Set(relevanceTokens(normalizedCandidate));
const overlapRatio = queryTokens.length > 0 ? queryTokens.filter((token) => candidateTokens.has(token)).length / queryTokens.length : 0; const overlapRatio = queryTokens.length > 0 ? queryTokens.filter((token) => candidateTokens.has(token)).length / queryTokens.length : 0;
// A minority of the query's meaningful words in common is not evidence of the same book; // A minority of the query's meaningful words in common is not evidence of the same book;
// "The Way of Kings" and "The Way We Were" share exactly one. // "The Way of Kings" and "The Way We Were" share exactly one.
@@ -163,9 +163,10 @@ describe('KoreaderAuthGuard', () => {
expect(mockUserService.findByIdWithPermissions).not.toHaveBeenCalled(); expect(mockUserService.findByIdWithPermissions).not.toHaveBeenCalled();
}); });
it('passes bcrypt authentication and sets koreaderUserId', async () => { it('passes bcrypt authentication for plaintext clients when no md5 is stored', async () => {
mockRequest.headers['x-auth-user'] = 'alice'; mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = 'secret'; mockRequest.headers['x-auth-key'] = 'secret';
mockRepo.findKoreaderUserByUsername.mockResolvedValueOnce(makeKoreaderUser({ passwordMd5: null }));
mockCompare.mockResolvedValueOnce(true); mockCompare.mockResolvedValueOnce(true);
await expect(guard.canActivate(mockContext as ExecutionContext)).resolves.toBe(true); await expect(guard.canActivate(mockContext as ExecutionContext)).resolves.toBe(true);
@@ -176,10 +177,9 @@ describe('KoreaderAuthGuard', () => {
expect(mockCreateHash).not.toHaveBeenCalled(); expect(mockCreateHash).not.toHaveBeenCalled();
}); });
it('passes MD5 authentication when bcrypt comparison fails', async () => { it('passes MD5 authentication for a plaintext key without running bcrypt', async () => {
mockRequest.headers['x-auth-user'] = 'alice'; mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = 'secret'; mockRequest.headers['x-auth-key'] = 'secret';
mockCompare.mockResolvedValueOnce(false);
const mockHash = { const mockHash = {
update: vi.fn().mockReturnThis(), update: vi.fn().mockReturnThis(),
@@ -191,6 +191,7 @@ describe('KoreaderAuthGuard', () => {
expect(mockCreateHash).toHaveBeenCalledWith('md5'); expect(mockCreateHash).toHaveBeenCalledWith('md5');
expect(mockHash.update).toHaveBeenCalledWith('secret'); expect(mockHash.update).toHaveBeenCalledWith('secret');
expect(mockCompare).not.toHaveBeenCalled();
expect(mockRequest.koreaderUserId).toBe(42); expect(mockRequest.koreaderUserId).toBe(42);
}); });
@@ -205,6 +206,7 @@ describe('KoreaderAuthGuard', () => {
mockCreateHash.mockReturnValueOnce(mockHash as unknown as ReturnType<typeof createHash>); mockCreateHash.mockReturnValueOnce(mockHash as unknown as ReturnType<typeof createHash>);
await expectGuardToReject(UnauthorizedException, 401, 'Invalid credentials'); await expectGuardToReject(UnauthorizedException, 401, 'Invalid credentials');
expect(mockCompare).toHaveBeenCalledWith('wrong-secret', 'bcrypt-hash');
expect(mockUserService.findByIdWithPermissions).not.toHaveBeenCalled(); expect(mockUserService.findByIdWithPermissions).not.toHaveBeenCalled();
}); });
@@ -213,7 +215,6 @@ describe('KoreaderAuthGuard', () => {
mockRequest.headers['x-auth-key'] = 'secret'; mockRequest.headers['x-auth-key'] = 'secret';
mockRepo.findKoreaderUserByUsername.mockResolvedValueOnce(makeKoreaderUser({ userId: 99 })); mockRepo.findKoreaderUserByUsername.mockResolvedValueOnce(makeKoreaderUser({ userId: 99 }));
mockUserService.findByIdWithPermissions.mockResolvedValueOnce(makeUser({ id: 99, username: 'alice' })); mockUserService.findByIdWithPermissions.mockResolvedValueOnce(makeUser({ id: 99, username: 'alice' }));
mockCompare.mockResolvedValueOnce(true);
await guard.canActivate(mockContext as ExecutionContext); await guard.canActivate(mockContext as ExecutionContext);
@@ -224,7 +225,6 @@ describe('KoreaderAuthGuard', () => {
it('returns 401 when user account is not found after credential validation', async () => { it('returns 401 when user account is not found after credential validation', async () => {
mockRequest.headers['x-auth-user'] = 'alice'; mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = 'secret'; mockRequest.headers['x-auth-key'] = 'secret';
mockCompare.mockResolvedValueOnce(true);
mockUserService.findByIdWithPermissions.mockResolvedValueOnce(null); mockUserService.findByIdWithPermissions.mockResolvedValueOnce(null);
await expectGuardToReject(UnauthorizedException, 401, 'Account not found or disabled'); await expectGuardToReject(UnauthorizedException, 401, 'Account not found or disabled');
@@ -233,7 +233,6 @@ describe('KoreaderAuthGuard', () => {
it('returns 401 when user account is disabled', async () => { it('returns 401 when user account is disabled', async () => {
mockRequest.headers['x-auth-user'] = 'alice'; mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = 'secret'; mockRequest.headers['x-auth-key'] = 'secret';
mockCompare.mockResolvedValueOnce(true);
mockUserService.findByIdWithPermissions.mockResolvedValueOnce(makeUser({ active: false })); mockUserService.findByIdWithPermissions.mockResolvedValueOnce(makeUser({ active: false }));
await expectGuardToReject(UnauthorizedException, 401, 'Account not found or disabled'); await expectGuardToReject(UnauthorizedException, 401, 'Account not found or disabled');
@@ -242,7 +241,6 @@ describe('KoreaderAuthGuard', () => {
it('returns 401 when user lacks koreader_sync permission', async () => { it('returns 401 when user lacks koreader_sync permission', async () => {
mockRequest.headers['x-auth-user'] = 'alice'; mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = 'secret'; mockRequest.headers['x-auth-key'] = 'secret';
mockCompare.mockResolvedValueOnce(true);
mockPermissionService.userHas.mockReturnValueOnce(false); mockPermissionService.userHas.mockReturnValueOnce(false);
await expectGuardToReject(UnauthorizedException, 401, 'KOReader sync permission revoked'); await expectGuardToReject(UnauthorizedException, 401, 'KOReader sync permission revoked');
@@ -251,7 +249,6 @@ describe('KoreaderAuthGuard', () => {
it('passes when user is superuser (bypasses permission check)', async () => { it('passes when user is superuser (bypasses permission check)', async () => {
mockRequest.headers['x-auth-user'] = 'alice'; mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = 'secret'; mockRequest.headers['x-auth-key'] = 'secret';
mockCompare.mockResolvedValueOnce(true);
mockUserService.findByIdWithPermissions.mockResolvedValueOnce(makeUser({ isSuperuser: true, permissions: [] })); mockUserService.findByIdWithPermissions.mockResolvedValueOnce(makeUser({ isSuperuser: true, permissions: [] }));
mockPermissionService.userHas.mockReturnValueOnce(true); mockPermissionService.userHas.mockReturnValueOnce(true);
@@ -259,15 +256,43 @@ describe('KoreaderAuthGuard', () => {
expect(mockPermissionService.userHas).toHaveBeenCalledWith(expect.objectContaining({ isSuperuser: true }), Permission.KoreaderSync); expect(mockPermissionService.userHas).toHaveBeenCalledWith(expect.objectContaining({ isSuperuser: true }), Permission.KoreaderSync);
}); });
it('passes when incoming key is exactly 32 hex chars matching md5 hash (pre-hashed key)', async () => { it('passes when incoming key is exactly 32 hex chars matching md5 hash (pre-hashed key) without running bcrypt', async () => {
const md5Key = 'abcdef0123456789abcdef0123456789'; const md5Key = 'abcdef0123456789abcdef0123456789';
mockRequest.headers['x-auth-user'] = 'alice'; mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = md5Key; mockRequest.headers['x-auth-key'] = md5Key;
mockCompare.mockResolvedValueOnce(false);
mockRepo.findKoreaderUserByUsername.mockResolvedValueOnce(makeKoreaderUser({ passwordMd5: md5Key })); mockRepo.findKoreaderUserByUsername.mockResolvedValueOnce(makeKoreaderUser({ passwordMd5: md5Key }));
await expect(guard.canActivate(mockContext as ExecutionContext)).resolves.toBe(true); await expect(guard.canActivate(mockContext as ExecutionContext)).resolves.toBe(true);
expect(mockCreateHash).not.toHaveBeenCalled(); expect(mockCreateHash).not.toHaveBeenCalled();
expect(mockCompare).not.toHaveBeenCalled();
});
it('matches a pre-hashed key regardless of hex case', async () => {
mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = 'ABCDEF0123456789ABCDEF0123456789';
mockRepo.findKoreaderUserByUsername.mockResolvedValueOnce(makeKoreaderUser({ passwordMd5: 'abcdef0123456789abcdef0123456789' }));
await expect(guard.canActivate(mockContext as ExecutionContext)).resolves.toBe(true);
expect(mockCompare).not.toHaveBeenCalled();
});
it('falls back to bcrypt when a 32-hex key is the plaintext password itself', async () => {
const hexPassword = '0123456789abcdef0123456789abcdef';
mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = hexPassword;
mockCompare.mockResolvedValueOnce(true);
await expect(guard.canActivate(mockContext as ExecutionContext)).resolves.toBe(true);
expect(mockCompare).toHaveBeenCalledWith(hexPassword, 'bcrypt-hash');
expect(mockCreateHash).not.toHaveBeenCalled();
});
it('returns 401 when a pre-hashed key does not match and bcrypt rejects it', async () => {
mockRequest.headers['x-auth-user'] = 'alice';
mockRequest.headers['x-auth-key'] = '00000000000000000000000000000000';
await expectGuardToReject(UnauthorizedException, 401, 'Invalid credentials');
expect(mockUserService.findByIdWithPermissions).not.toHaveBeenCalled();
}); });
it('returns 401 when md5 hash matches no password and no md5 stored', async () => { it('returns 401 when md5 hash matches no password and no md5 stored', async () => {
@@ -9,6 +9,8 @@ import { PermissionService } from '../../common/services/permission.service';
import { UserService } from '../user/user.service'; import { UserService } from '../user/user.service';
import { KoreaderRepository } from './koreader.repository'; import { KoreaderRepository } from './koreader.repository';
const MD5_HEX = /^[0-9a-f]{32}$/i;
@Injectable() @Injectable()
export class KoreaderAuthGuard implements CanActivate { export class KoreaderAuthGuard implements CanActivate {
constructor( constructor(
@@ -58,18 +60,15 @@ export class KoreaderAuthGuard implements CanActivate {
} }
private async verifyPassword(incoming: string, bcryptHash: string, md5Hash: string | null): Promise<boolean> { private async verifyPassword(incoming: string, bcryptHash: string, md5Hash: string | null): Promise<boolean> {
const bcryptMatch = await compare(incoming, bcryptHash); // The KOReader plugin sends md5(password), which can never match the bcrypt hash of the plaintext.
if (bcryptMatch) return true; // bcrypt costs hundreds of milliseconds of CPU, so it only runs when the cheap md5 check does not match.
if (md5Hash && this.matchesMd5(incoming, md5Hash)) return true;
if (md5Hash && incoming.length === 32 && /^[0-9a-f]{32}$/i.test(incoming)) { return compare(incoming, bcryptHash);
return incoming.toLowerCase() === md5Hash.toLowerCase(); }
}
if (md5Hash) { private matchesMd5(incoming: string, md5Hash: string): boolean {
const incomingMd5 = createHash('md5').update(incoming).digest('hex'); const incomingMd5 = MD5_HEX.test(incoming) ? incoming : createHash('md5').update(incoming).digest('hex');
return incomingMd5 === md5Hash.toLowerCase(); return incomingMd5.toLowerCase() === md5Hash.toLowerCase();
}
return false;
} }
} }
+1 -1
View File
@@ -4,7 +4,7 @@ ARG NODE_IMAGE=node:26.12.1-trixie-slim
FROM ${NODE_IMAGE} AS builder FROM ${NODE_IMAGE} AS builder
WORKDIR /app WORKDIR /app
RUN npm install -g pnpm@11.22.0 RUN npm install -g pnpm@11.28.9
# The image runs on CPU only: without this, onnxruntime-node downloads its CUDA provider on # The image runs on CPU only: without this, onnxruntime-node downloads its CUDA provider on
# linux/x64, which cannot load here (no CUDA or cuDNN libraries) and only adds size. # linux/x64, which cannot load here (no CUDA or cuDNN libraries) and only adds size.