diff --git a/src/anime-bridge/bridge-client.test.ts b/src/anime-bridge/bridge-client.test.ts index 62a75c6f..eb170648 100644 --- a/src/anime-bridge/bridge-client.test.ts +++ b/src/anime-bridge/bridge-client.test.ts @@ -4,7 +4,12 @@ import { AnimeBridgeClient, BridgeExtensionError } from './bridge-client'; import { BRIDGE_CONTEXT_KEY } from './types'; const EXTENSION_ID = 'a'.repeat(64); -const source = { apkBase64: 'QVBLLUJZVEVT', sourceId: 'source-1' }; +const APK_BASE64 = 'QVBLLUJZVEVT'; +const source = { + fingerprint: 'sha-1', + loadApkBase64: async () => APK_BASE64, + sourceId: 'source-1', +}; interface Recorded { url: string; @@ -69,7 +74,7 @@ test('getVideoList posts the APK and episode url with a bridge context preferenc assert.equal(calls[0]?.url, 'http://127.0.0.1:9/dalvik'); assert.equal(calls[0]?.body.method, 'getVideoList'); assert.deepEqual(calls[0]?.body.episodeData, { url: 'https://origin.example/ep/1' }); - assert.equal(calls[0]?.body.data, source.apkBase64); + assert.equal(calls[0]?.body.data, APK_BASE64); assert.deepEqual(calls[0]?.body.preferences, [{ key: BRIDGE_CONTEXT_KEY, sourceId: 'source-1' }]); assert.equal(videos.length, 1); }); @@ -81,12 +86,25 @@ test('a cached extension id replaces the APK upload on later calls', async () => await client.getVideoList(source, 'https://origin.example/ep/1'); await client.getVideoList(source, 'https://origin.example/ep/2'); - assert.equal(calls[0]?.body.data, source.apkBase64); + assert.equal(calls[0]?.body.data, APK_BASE64); assert.equal(calls[0]?.body.extensionId, undefined); assert.equal(calls[1]?.body.data, undefined); assert.equal(calls[1]?.body.extensionId, EXTENSION_ID); }); +test('an upgraded APK re-uploads instead of reusing the previous extension id', async () => { + const { fetchImpl, calls } = stubFetch(() => jsonResponse([], EXTENSION_ID)); + const client = new AnimeBridgeClient({ baseUrl: 'http://127.0.0.1:9', fetchImpl }); + + await client.getVideoList(source, 'https://origin.example/ep/1'); + // Same source id, new build in the same file: the id cache must miss. + const upgraded = { ...source, fingerprint: 'sha-2', loadApkBase64: async () => 'TkVXLUFQSw==' }; + await client.getVideoList(upgraded, 'https://origin.example/ep/2'); + + assert.equal(calls[1]?.body.extensionId, undefined); + assert.equal(calls[1]?.body.data, 'TkVXLUFQSw=='); +}); + test('a 409 re-uploads the APK once and succeeds', async () => { const { fetchImpl, calls } = stubFetch((call, index) => { if (index === 0) return jsonResponse([], EXTENSION_ID); @@ -101,7 +119,7 @@ test('a 409 re-uploads the APK once and succeeds', async () => { assert.equal(calls.length, 3); assert.equal(calls[1]?.body.extensionId, EXTENSION_ID); - assert.equal(calls[2]?.body.data, source.apkBase64); + assert.equal(calls[2]?.body.data, APK_BASE64); assert.equal(videos.length, 1); }); diff --git a/src/anime-bridge/bridge-client.ts b/src/anime-bridge/bridge-client.ts index a45988dc..2a20a076 100644 --- a/src/anime-bridge/bridge-client.ts +++ b/src/anime-bridge/bridge-client.ts @@ -13,8 +13,16 @@ const EXTENSION_ID_HEADER = 'x-mangatan-extension-id'; const EXTENSION_ID_PATTERN = /^[0-9a-f]{64}$/; export interface BridgeSource { - /** Base64-encoded extension APK. */ - apkBase64: string; + /** + * Identity of the APK's contents. Keys the extension-id cache, so an upgraded + * APK is re-uploaded instead of reusing the previous build's id. + */ + fingerprint: string; + /** + * Reads and base64-encodes the APK. Called only when the bridge actually + * needs the bytes, so multi-megabyte payloads are not held on the heap. + */ + loadApkBase64: () => Promise; /** Selects one source inside a multi-source (SourceFactory) APK. */ sourceId?: string; preferences?: BridgePreference[]; @@ -24,8 +32,19 @@ export interface BridgeClientOptions { /** Loopback base URL of the running bridge, e.g. `http://127.0.0.1:53112`. */ baseUrl: string; fetchImpl?: typeof fetch; + /** + * Per-request deadline. Node's `fetch` has none, so a sidecar that accepts + * the socket and then stalls would leave every call pending forever. + */ + requestTimeoutMs?: number; } +/** Extension calls can be slow (a source may scrape several pages). */ +const DEFAULT_REQUEST_TIMEOUT_MS = 60_000; + +/** The readiness probe is a local health check; it should answer at once. */ +const CAPABILITIES_TIMEOUT_MS = 5_000; + /** The bridge reports extension failures as HTTP 200 with an error body. */ export class BridgeExtensionError extends Error { readonly code?: number; @@ -46,15 +65,19 @@ export class BridgeExtensionError extends Error { export class AnimeBridgeClient { private readonly baseUrl: string; private readonly fetchImpl: typeof fetch; + private readonly requestTimeoutMs: number; private readonly extensionIds = new Map(); constructor(options: BridgeClientOptions) { this.baseUrl = options.baseUrl.replace(/\/+$/, ''); this.fetchImpl = options.fetchImpl ?? fetch; + this.requestTimeoutMs = options.requestTimeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS; } async getCapabilities(): Promise { - const response = await this.fetchImpl(`${this.baseUrl}/capabilities`); + const response = await this.fetchImpl(`${this.baseUrl}/capabilities`, { + signal: AbortSignal.timeout(Math.min(CAPABILITIES_TIMEOUT_MS, this.requestTimeoutMs)), + }); if (!response.ok) { throw new Error(`Anime bridge capabilities check failed (${response.status}).`); } @@ -153,7 +176,10 @@ export class AnimeBridgeClient { extras: Record, changedPreferenceKey?: string, ): Promise { - const cacheKey = source.sourceId ?? source.apkBase64; + // Keyed by APK contents, not by source id: an in-place upgrade keeps the + // same source id, and reusing its cached extension id would silently run + // the previous build (the bridge has no reason to answer 409). + const cacheKey = `${source.fingerprint}:${source.sourceId ?? ''}`; const cachedId = this.extensionIds.get(cacheKey); let response = await this.post(method, extras, source, cachedId, changedPreferenceKey); @@ -177,7 +203,7 @@ export class AnimeBridgeClient { return body; } - private post( + private async post( method: string, extras: Record, source: BridgeSource, @@ -188,7 +214,7 @@ export class AnimeBridgeClient { method, ...extras, preferences: this.buildPreferences(source, changedPreferenceKey), - ...(extensionId === undefined ? { data: source.apkBase64 } : { extensionId }), + ...(extensionId === undefined ? { data: await source.loadApkBase64() } : { extensionId }), }; return this.fetchImpl(`${this.baseUrl}/dalvik`, { @@ -198,6 +224,7 @@ export class AnimeBridgeClient { Accept: 'application/json', }, body: JSON.stringify(payload), + signal: AbortSignal.timeout(this.requestTimeoutMs), }); } } diff --git a/src/anime-bridge/extension-installer.test.ts b/src/anime-bridge/extension-installer.test.ts index 25f7ae7a..6f421386 100644 --- a/src/anime-bridge/extension-installer.test.ts +++ b/src/anime-bridge/extension-installer.test.ts @@ -138,6 +138,49 @@ test('an oversized download is refused even when the length header lies', async assert.equal(existsSync(path.join(dir, `${PKG}.apk`)), false); }); +test('the byte limit stops the read instead of buffering the whole body', async () => { + const dir = await mkdtemp(path.join(tmpdir(), 'subminer-install-')); + let pushed = 0; + // Endless body: if the limit were only checked after buffering, this hangs. + const body = new ReadableStream({ + pull(controller) { + pushed += 1; + controller.enqueue(new Uint8Array(512)); + }, + }); + const fetchImpl = (async () => new Response(body, { status: 200 })) as typeof fetch; + + await assert.rejects( + () => + installExtension({ + extensionsDir: dir, + extension: repoExtension(), + fetchImpl, + maxBytes: 1024, + }), + /larger than the 1024 byte limit/, + ); + // Only enough chunks to cross the limit were ever read. + assert.ok(pushed <= 4, `read ${pushed} chunks before aborting`); +}); + +test('a package name carrying path separators cannot escape the extensions dir', async () => { + const root = await mkdtemp(path.join(tmpdir(), 'subminer-install-')); + const dir = path.join(root, 'extensions'); + const escaping = `eu.kanade.tachiyomi.animeextension${path.sep}..${path.sep}..${path.sep}pwned`; + + await assert.rejects( + () => + installExtension({ + extensionsDir: dir, + extension: repoExtension({ pkg: escaping }), + fetchImpl: respondWith(apkBytes()), + }), + /not a valid file name/, + ); + assert.equal(existsSync(path.join(root, 'pwned.apk')), false); +}); + test('removeExtension deletes the file and tolerates a missing one', async () => { const dir = await mkdtemp(path.join(tmpdir(), 'subminer-install-')); const file = path.join(dir, `${PKG}.apk`); diff --git a/src/anime-bridge/extension-installer.ts b/src/anime-bridge/extension-installer.ts index 05060036..55c12b5d 100644 --- a/src/anime-bridge/extension-installer.ts +++ b/src/anime-bridge/extension-installer.ts @@ -15,11 +15,18 @@ export interface InstallExtensionOptions { fetchImpl?: typeof fetch; /** Guards against a mistyped repo serving something enormous. */ maxBytes?: number; + /** Cancels a stalled download; without it a hung repo blocks the install. */ + signal?: AbortSignal; + /** Applied when no `signal` is given, so a download can never hang forever. */ + timeoutMs?: number; } /** APKs are a few MB; anything far past that is not an extension. */ const DEFAULT_MAX_BYTES = 64 * 1024 * 1024; +/** Generous enough for a large APK on a slow link, short of hanging forever. */ +const DEFAULT_TIMEOUT_MS = 120_000; + const APK_MAGIC = [0x50, 0x4b, 0x03, 0x04]; // "PK\x03\x04" — APKs are zip archives. export function looksLikeApk(bytes: Uint8Array): boolean { @@ -35,8 +42,9 @@ export function looksLikeApk(bytes: Uint8Array): boolean { export async function installExtension(options: InstallExtensionOptions): Promise { const fetchImpl = options.fetchImpl ?? fetch; const maxBytes = options.maxBytes ?? DEFAULT_MAX_BYTES; + const signal = options.signal ?? AbortSignal.timeout(options.timeoutMs ?? DEFAULT_TIMEOUT_MS); - const response = await fetchImpl(options.extension.apkUrl); + const response = await fetchImpl(options.extension.apkUrl, { signal }); if (!response.ok) { throw new Error(`Downloading ${options.extension.name} failed (${response.status}).`); } @@ -46,21 +54,74 @@ export async function installExtension(options: InstallExtensionOptions): Promis throw new Error(`${options.extension.name} is larger than the ${maxBytes} byte limit.`); } - const bytes = new Uint8Array(await response.arrayBuffer()); - if (bytes.byteLength > maxBytes) { - throw new Error(`${options.extension.name} is larger than the ${maxBytes} byte limit.`); - } + const bytes = await readBounded(response, maxBytes, options.extension.name); if (!looksLikeApk(bytes)) { throw new Error(`${options.extension.name} did not download as an APK.`); } await mkdir(options.extensionsDir, { recursive: true }); - const target = path.join(options.extensionsDir, extensionFileName(options.extension.pkg)); + const target = resolveTarget(options.extensionsDir, options.extension.pkg); await writeFile(target, bytes); return target; } +/** + * Read the body incrementally and stop the moment the limit is passed. + * + * Buffering first and measuring afterwards would let a repo that lies about + * (or omits) `content-length` push an unbounded amount into memory before the + * check ever runs. + */ +async function readBounded( + response: Response, + maxBytes: number, + name: string, +): Promise { + const reader = response.body?.getReader(); + if (!reader) { + const bytes = new Uint8Array(await response.arrayBuffer()); + if (bytes.byteLength > maxBytes) { + throw new Error(`${name} is larger than the ${maxBytes} byte limit.`); + } + return bytes; + } + + const chunks: Uint8Array[] = []; + let total = 0; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + total += value.byteLength; + if (total > maxBytes) { + await reader.cancel(); + throw new Error(`${name} is larger than the ${maxBytes} byte limit.`); + } + chunks.push(value); + } + + const bytes = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + bytes.set(chunk, offset); + offset += chunk.byteLength; + } + return bytes; +} + +/** + * Defence in depth against a repository index that smuggles path separators + * into a package name: the write target must stay inside `extensionsDir`. + */ +function resolveTarget(extensionsDir: string, pkg: string): string { + const root = path.resolve(extensionsDir); + const target = path.resolve(root, extensionFileName(pkg)); + if (path.dirname(target) !== root) { + throw new Error(`Refusing to install ${pkg}: the package name is not a valid file name.`); + } + return target; +} + /** Delete an installed extension. Missing files are treated as already gone. */ export async function removeExtension(extensionsDir: string, pkg: string): Promise { - await rm(path.join(extensionsDir, extensionFileName(pkg)), { force: true }); + await rm(resolveTarget(extensionsDir, pkg), { force: true }); } diff --git a/src/anime-bridge/extension-repo.test.ts b/src/anime-bridge/extension-repo.test.ts index a645f76a..c139b8ce 100644 --- a/src/anime-bridge/extension-repo.test.ts +++ b/src/anime-bridge/extension-repo.test.ts @@ -87,6 +87,26 @@ test('malformed entries are skipped rather than failing the repo', () => { assert.equal(parsed.length, 1); }); +test('a package name that is not a plain identifier is rejected', () => { + // The package name becomes the on-disk file name, and a repo index is + // unauthenticated: path separators here would write outside the extensions + // directory even though the prefix check passes. + const parsed = parseRepoIndex(INDEX, [ + animeEntry({ pkg: 'eu.kanade.tachiyomi.animeextension/../../../../etc/cron.d/x' }), + animeEntry({ pkg: 'eu.kanade.tachiyomi.animeextension\\..\\evil' }), + animeEntry({ pkg: 'eu.kanade.tachiyomi.animeextension.all.ok' }), + ]); + assert.deepEqual( + parsed.map((extension) => extension.pkg), + ['eu.kanade.tachiyomi.animeextension.all.ok'], + ); +}); + +test('an apk file name with path characters is rejected', () => { + const parsed = parseRepoIndex(INDEX, [animeEntry({ apk: '../../../etc/passwd' })]); + assert.deepEqual(parsed, []); +}); + test('parseRepoIndex tolerates a non-array payload', () => { assert.deepEqual(parseRepoIndex(INDEX, { message: 'Not Found' }), []); assert.deepEqual(parseRepoIndex(INDEX, null), []); diff --git a/src/anime-bridge/extension-repo.ts b/src/anime-bridge/extension-repo.ts index 5639bfb2..0f07a024 100644 --- a/src/anime-bridge/extension-repo.ts +++ b/src/anime-bridge/extension-repo.ts @@ -9,6 +9,16 @@ /** Aniyomi extension packages carry this prefix; manga packages are ignored. */ const ANIME_PACKAGE_PREFIX = 'eu.kanade.tachiyomi.animeextension'; +/** + * A package name becomes the on-disk APK file name, and a repository index is + * unauthenticated content the user pointed us at. Only plain dotted identifiers + * are accepted, so nothing in an index can carry `/` or `..` into a file path. + */ +const PACKAGE_NAME_PATTERN = /^[A-Za-z0-9_.]+$/; + +/** The APK file name is appended to the repo URL, so keep it a bare name. */ +const APK_FILE_NAME_PATTERN = /^[A-Za-z0-9_.+-]+$/; + /** * Repos are identified by their index URL. The file name is not fixed: * `index.min.json` is the Aniyomi convention, but repositories publish under @@ -81,7 +91,8 @@ export function parseRepoIndex(indexUrl: string, payload: unknown): RepoExtensio if (raw === null || typeof raw !== 'object') continue; const pkg = typeof raw.pkg === 'string' ? raw.pkg : ''; const apk = typeof raw.apk === 'string' ? raw.apk : ''; - if (!pkg.startsWith(ANIME_PACKAGE_PREFIX) || apk.length === 0) continue; + if (!pkg.startsWith(ANIME_PACKAGE_PREFIX) || !PACKAGE_NAME_PATTERN.test(pkg)) continue; + if (apk.length === 0 || !APK_FILE_NAME_PATTERN.test(apk)) continue; const versionCode = Number(raw.code); extensions.push({ diff --git a/src/anime-bridge/extension-store.test.ts b/src/anime-bridge/extension-store.test.ts index 36dd424c..be2cb328 100644 --- a/src/anime-bridge/extension-store.test.ts +++ b/src/anime-bridge/extension-store.test.ts @@ -1,6 +1,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { mkdtemp, mkdir, writeFile } from 'node:fs/promises'; +import { createHash } from 'node:crypto'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { @@ -22,18 +23,36 @@ async function makeExtensionDir(files: Record): Promise } function fakeClient( - impl: (source: { apkBase64: string }) => Promise, + impl: (source: { fingerprint: string }) => Promise, ): AnimeBridgeClient { return { listAnimeSources: impl } as unknown as AnimeBridgeClient; } -test('readInstalledExtensions reads apks and base64-encodes them', async () => { +test('readInstalledExtensions fingerprints apks without holding their bytes', async () => { const dir = await makeExtensionDir({ 'my-source.apk': 'APK-BYTES' }); const extensions = await readInstalledExtensions(dir); assert.equal(extensions.length, 1); assert.equal(extensions[0]?.fallbackName, 'my-source'); - assert.equal(Buffer.from(extensions[0]!.apkBase64, 'base64').toString(), 'APK-BYTES'); + assert.equal(extensions[0]?.sha256, createHash('sha256').update('APK-BYTES').digest('hex')); +}); + +test('the fingerprint changes when an apk is replaced in place', async () => { + const dir = await makeExtensionDir({ 'my-source.apk': 'V1' }); + const before = (await readInstalledExtensions(dir))[0]?.sha256; + await writeFile(path.join(dir, 'my-source.apk'), 'V2'); + const after = (await readInstalledExtensions(dir))[0]?.sha256; + + assert.notEqual(before, after); +}); + +test('toBridgeSource reads the apk only when the bridge asks for it', async () => { + const dir = await makeExtensionDir({ 'lazy.apk': 'APK-BYTES' }); + const extension = (await readInstalledExtensions(dir))[0]!; + + const bridgeSource = toBridgeSource(extension); + assert.equal(bridgeSource.fingerprint, extension.sha256); + assert.equal(Buffer.from(await bridgeSource.loadApkBase64(), 'base64').toString(), 'APK-BYTES'); }); test('readInstalledExtensions ignores non-apk files and subdirectories', async () => { @@ -50,21 +69,15 @@ test('readInstalledExtensions returns empty for a missing directory', async () = }); test('toBridgeSource includes sourceId only when selecting inside a factory apk', () => { - const extension: InstalledExtension = { - file: '/x/a.apk', - fallbackName: 'a', - apkBase64: 'QQ==', - }; - assert.deepEqual(toBridgeSource(extension), { apkBase64: 'QQ==' }); - assert.deepEqual(toBridgeSource(extension, 'src-1'), { - apkBase64: 'QQ==', - sourceId: 'src-1', - }); + const extension: InstalledExtension = { file: '/x/a.apk', fallbackName: 'a', sha256: 'hash-a' }; + assert.equal(toBridgeSource(extension).sourceId, undefined); + assert.equal(toBridgeSource(extension, 'src-1').sourceId, 'src-1'); + assert.equal(toBridgeSource(extension, 'src-1').fingerprint, 'hash-a'); }); test('listExtensionSources flattens every source a factory apk provides', async () => { const extensions: InstalledExtension[] = [ - { file: '/x/multi.apk', fallbackName: 'multi', apkBase64: 'QQ==' }, + { file: '/x/multi.apk', fallbackName: 'multi', sha256: 'hash-a' }, ]; const client = fakeClient(async () => [ { id: 101, name: 'Source One', lang: 'en' }, @@ -82,7 +95,7 @@ test('listExtensionSources flattens every source a factory apk provides', async test('listExtensionSources falls back to the file name and a default language', async () => { const extensions: InstalledExtension[] = [ - { file: '/x/my-ext.apk', fallbackName: 'my-ext', apkBase64: 'QQ==' }, + { file: '/x/my-ext.apk', fallbackName: 'my-ext', sha256: 'hash-a' }, ]; const client = fakeClient(async () => [{ id: '1', name: ' ' }]); @@ -94,14 +107,14 @@ test('listExtensionSources falls back to the file name and a default language', test('listExtensionSources drops descriptors with no usable id', async () => { const client = fakeClient(async () => [{ name: 'No Id' }, { id: '', name: 'Empty' }]); const sources = await listExtensionSources(client, [ - { file: '/x/a.apk', fallbackName: 'a', apkBase64: 'QQ==' }, + { file: '/x/a.apk', fallbackName: 'a', sha256: 'hash-a' }, ]); assert.deepEqual(sources, []); }); test('toInstalledExtensionViews names an extension after the sources it provides', () => { const extensions: InstalledExtension[] = [ - { file: '/x/multi.apk', fallbackName: 'multi', apkBase64: 'QQ==' }, + { file: '/x/multi.apk', fallbackName: 'multi', sha256: 'hash-a' }, ]; const sources: ExtensionSource[] = [ { id: '1', name: 'One', lang: 'en', pkg: 'multi', file: '/x/multi.apk' }, @@ -115,7 +128,7 @@ test('toInstalledExtensionViews names an extension after the sources it provides test('toInstalledExtensionViews lists an extension that loaded nothing, with its reason', () => { const extensions: InstalledExtension[] = [ - { file: '/x/broken.apk', fallbackName: 'broken', apkBase64: 'QQ==' }, + { file: '/x/broken.apk', fallbackName: 'broken', sha256: 'hash-a' }, ]; // A broken APK is still installed, so it must stay listed and removable. @@ -127,12 +140,12 @@ test('toInstalledExtensionViews lists an extension that loaded nothing, with its test('one broken extension does not hide the working ones', async () => { const extensions: InstalledExtension[] = [ - { file: '/x/broken.apk', fallbackName: 'broken', apkBase64: 'QQ==' }, - { file: '/x/good.apk', fallbackName: 'good', apkBase64: 'Qg==' }, + { file: '/x/broken.apk', fallbackName: 'broken', sha256: 'hash-a' }, + { file: '/x/good.apk', fallbackName: 'good', sha256: 'hash-b' }, ]; const failures: string[] = []; const client = fakeClient(async (source) => { - if (source.apkBase64 === 'QQ==') throw new Error('dex2jar failed'); + if (source.fingerprint === 'hash-a') throw new Error('dex2jar failed'); return [{ id: '7', name: 'Good Source', lang: 'en' }]; }); diff --git a/src/anime-bridge/extension-store.ts b/src/anime-bridge/extension-store.ts index e1fcc7f0..4757dccb 100644 --- a/src/anime-bridge/extension-store.ts +++ b/src/anime-bridge/extension-store.ts @@ -1,4 +1,7 @@ +import { createReadStream } from 'node:fs'; import { readdir, readFile } from 'node:fs/promises'; +import { createHash } from 'node:crypto'; +import { pipeline } from 'node:stream/promises'; import path from 'node:path'; import type { AnimeBridgeClient } from './bridge-client'; import type { BridgeSource } from './bridge-client'; @@ -15,7 +18,11 @@ export interface InstalledExtension { file: string; /** File name without extension, used when the bridge reports no name. */ fallbackName: string; - apkBase64: string; + /** + * SHA-256 of the APK. Identifies the build rather than the slot, so the + * bridge's extension-id cache misses after an in-place upgrade. + */ + sha256: string; } export interface ExtensionSource { @@ -27,7 +34,14 @@ export interface ExtensionSource { file: string; } -/** Read every .apk in `directory`. A missing directory yields no extensions. */ +/** + * Discover every .apk in `directory`. A missing directory yields no extensions. + * + * Only a hash is kept, never the bytes: APKs run to several MB each and a + * base64 copy adds a third on top, so holding the whole set for the lifetime of + * the Anime Browser would cost far more than re-reading a file on the rare + * upload. Hashing streams, so peak memory stays flat regardless of APK size. + */ export async function readInstalledExtensions(directory: string): Promise { let entries; try { @@ -40,16 +54,21 @@ export async function readInstalledExtensions(directory: string): Promise a.name.localeCompare(b.name))) { if (!entry.isFile() || !entry.name.toLowerCase().endsWith('.apk')) continue; const file = path.join(directory, entry.name); - const bytes = await readFile(file); extensions.push({ file, fallbackName: entry.name.replace(/\.apk$/i, ''), - apkBase64: bytes.toString('base64'), + sha256: await hashFile(file), }); } return extensions; } +async function hashFile(file: string): Promise { + const hash = createHash('sha256'); + await pipeline(createReadStream(file), hash); + return hash.digest('hex'); +} + /** * Describe what is on disk, for the installed list in the Extensions tab. * @@ -75,9 +94,18 @@ export function toInstalledExtensionViews( }); } -/** The bridge payload for a specific source inside an extension. */ +/** + * The bridge payload for a specific source inside an extension. + * + * The APK is read on demand: after the first upload the bridge answers by + * extension id, so most calls never touch the file at all. + */ export function toBridgeSource(extension: InstalledExtension, sourceId?: string): BridgeSource { - return { apkBase64: extension.apkBase64, ...(sourceId ? { sourceId } : {}) }; + return { + fingerprint: extension.sha256, + loadApkBase64: async () => (await readFile(extension.file)).toString('base64'), + ...(sourceId ? { sourceId } : {}), + }; } /** diff --git a/src/anime-bridge/headers.test.ts b/src/anime-bridge/headers.test.ts index 8528d3ff..e6f2d9dc 100644 --- a/src/anime-bridge/headers.test.ts +++ b/src/anime-bridge/headers.test.ts @@ -28,6 +28,13 @@ test('toMpvHeaderFields joins entries and escapes commas in values', () => { assert.equal(fields, 'Referer: https://origin.example/,Cookie: a=1\\, b=2'); }); +test('toMpvHeaderFields escapes backslashes so a trailing one cannot eat the separator', () => { + const fields = toMpvHeaderFields({ Referer: 'https://origin.example/path\\', Cookie: 'a=1' }); + // Without doubling, the value's trailing backslash would escape the comma + // and merge Cookie into the Referer entry. + assert.equal(fields, 'Referer: https://origin.example/path\\\\,Cookie: a=1'); +}); + test('toMpvHeaderFields returns an empty string when there are no headers', () => { assert.equal(toMpvHeaderFields({}), ''); }); diff --git a/src/anime-bridge/headers.ts b/src/anime-bridge/headers.ts index 480b98e1..4d19e71d 100644 --- a/src/anime-bridge/headers.ts +++ b/src/anime-bridge/headers.ts @@ -19,11 +19,13 @@ export function parseOkHttpHeaders(headers: OkHttpHeaders | undefined): Record): string { return Object.entries(headers) - .map(([name, value]) => `${name}: ${value.replace(/,/g, '\\,')}`) + .map(([name, value]) => `${name}: ${value.replace(/\\/g, '\\\\').replace(/,/g, '\\,')}`) .join(','); } diff --git a/src/anime-bridge/mpv-playback.test.ts b/src/anime-bridge/mpv-playback.test.ts index 51cc8352..c6c9fd3e 100644 --- a/src/anime-bridge/mpv-playback.test.ts +++ b/src/anime-bridge/mpv-playback.test.ts @@ -57,6 +57,17 @@ test('the escape length counts the full header string including separators', () assert.ok(options.includes(`%${fields.length}%${fields}`)); }); +test('the escape length is counted in utf-8 bytes, not js string units', () => { + // An extension may put a non-ASCII value in a header; mpv reads %n% as a + // byte count, so counting string units would truncate the value. + const options = buildLoadfileOptions({ + stream: stream({ headers: { 'X-Title': '日本語' } }), + }); + const fields = 'X-Title: 日本語'; + assert.ok(options.includes(`%${Buffer.byteLength(fields, 'utf8')}%${fields}`)); + assert.ok(!options.includes(`%${fields.length}%`)); +}); + test('no header option is emitted when the stream carries no headers', () => { const options = buildLoadfileOptions({ stream: stream() }); assert.ok(!options.includes('http-header-fields')); diff --git a/src/anime-bridge/mpv-playback.ts b/src/anime-bridge/mpv-playback.ts index 3fa05e81..9cdbffb8 100644 --- a/src/anime-bridge/mpv-playback.ts +++ b/src/anime-bridge/mpv-playback.ts @@ -77,9 +77,13 @@ export function buildLoadfileOptions(options: BuildPlaybackOptions): string { * mpv splits `loadfile` options on commas and `=`-separates keys, so a value * containing either must be quoted. Percent-encoding is mpv's own escape for * embedded separators in option values. + * + * The count is in UTF-8 bytes of the decoded value, not JS string units, so a + * non-ASCII header value (extensions supply these) would otherwise under-count + * and mpv would cut the value short. */ function escapeOptionValue(value: string): string { - return `%${value.length}%${value}`; + return `%${Buffer.byteLength(value, 'utf8')}%${value}`; } /** diff --git a/src/anime-bridge/preference-store.test.ts b/src/anime-bridge/preference-store.test.ts new file mode 100644 index 00000000..ca36b3d8 --- /dev/null +++ b/src/anime-bridge/preference-store.test.ts @@ -0,0 +1,69 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtemp, readFile, readdir, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { PreferenceStore } from './preference-store'; + +async function storeFile(): Promise { + const dir = await mkdtemp(path.join(tmpdir(), 'subminer-prefs-')); + return path.join(dir, 'anime-preferences.json'); +} + +test('values round-trip through the file', async () => { + const file = await storeFile(); + await new PreferenceStore(file).set('src-1', [{ key: 'address' }]); + + assert.deepEqual(await new PreferenceStore(file).get('src-1'), [{ key: 'address' }]); +}); + +test('concurrent writes on a cold cache do not lose an update', async () => { + const file = await storeFile(); + const store = new PreferenceStore(file); + + // Both start before either has loaded; unserialized they would each get their + // own object and the later persist would drop the other's entry. + await Promise.all([store.set('src-1', [{ key: 'a' }]), store.set('src-2', [{ key: 'b' }])]); + + const reloaded = new PreferenceStore(file); + assert.deepEqual(await reloaded.get('src-1'), [{ key: 'a' }]); + assert.deepEqual(await reloaded.get('src-2'), [{ key: 'b' }]); +}); + +test('a clear racing a set is applied in order', async () => { + const file = await storeFile(); + const store = new PreferenceStore(file); + await store.set('pkg:src', [{ key: 'password' }]); + + await Promise.all([store.clear('pkg'), store.set('other:src', [{ key: 'x' }])]); + + const reloaded = new PreferenceStore(file); + assert.deepEqual(await reloaded.get('pkg:src'), []); + assert.deepEqual(await reloaded.get('other:src'), [{ key: 'x' }]); +}); + +test('the file is written owner-only and leaves no temporary behind', async () => { + const file = await storeFile(); + await new PreferenceStore(file).set('src-1', [{ key: 'password' }]); + + const { stat } = await import('node:fs/promises'); + assert.equal((await stat(file)).mode & 0o777, 0o600); + assert.deepEqual(await readdir(path.dirname(file)), [path.basename(file)]); +}); + +test('a corrupt file starts empty rather than blocking the browser', async () => { + const file = await storeFile(); + await writeFile(file, '{ not json'); + + assert.deepEqual(await new PreferenceStore(file).get('src-1'), []); +}); + +test('a write replaces the previous contents wholesale', async () => { + const file = await storeFile(); + const store = new PreferenceStore(file); + await store.set('src-1', [{ key: 'first' }]); + await store.set('src-1', [{ key: 'second' }]); + + const parsed = JSON.parse(await readFile(file, 'utf8')) as Record; + assert.deepEqual(parsed['src-1'], [{ key: 'second' }]); +}); diff --git a/src/anime-bridge/preference-store.ts b/src/anime-bridge/preference-store.ts index 30ca4d93..23a416d2 100644 --- a/src/anime-bridge/preference-store.ts +++ b/src/anime-bridge/preference-store.ts @@ -1,4 +1,4 @@ -import { readFile, writeFile, mkdir } from 'node:fs/promises'; +import { readFile, writeFile, rename, rm, mkdir } from 'node:fs/promises'; import path from 'node:path'; import type { BridgePreference } from './types'; @@ -11,11 +11,24 @@ import type { BridgePreference } from './types'; export class PreferenceStore { private readonly file: string; private cache: Record | null = null; + /** + * Mutations run one at a time. Two concurrent load-modify-persist cycles + * starting on a cold cache would each read their own object, and the later + * write would drop the earlier one's edit. + */ + private queue: Promise = Promise.resolve(); constructor(file: string) { this.file = file; } + private enqueue(operation: () => Promise): Promise { + const result = this.queue.then(operation, operation); + // Keep the chain alive after a rejection so one failure cannot wedge it. + this.queue = result.catch(() => undefined); + return result; + } + private async load(): Promise> { if (this.cache !== null) return this.cache; try { @@ -32,14 +45,18 @@ export class PreferenceStore { } async get(sourceId: string): Promise { - const all = await this.load(); - return all[sourceId] ?? []; + return this.enqueue(async () => { + const all = await this.load(); + return all[sourceId] ?? []; + }); } async set(sourceId: string, preferences: BridgePreference[]): Promise { - const all = await this.load(); - all[sourceId] = preferences; - await this.persist(all); + await this.enqueue(async () => { + const all = await this.load(); + all[sourceId] = preferences; + await this.persist(all); + }); } /** @@ -50,19 +67,35 @@ export class PreferenceStore { * package name and this clears anything recorded under it. */ async clear(prefix: string): Promise { - const all = await this.load(); - let changed = false; - for (const key of Object.keys(all)) { - if (key === prefix || key.startsWith(`${prefix}:`)) { - delete all[key]; - changed = true; + await this.enqueue(async () => { + const all = await this.load(); + let changed = false; + for (const key of Object.keys(all)) { + if (key === prefix || key.startsWith(`${prefix}:`)) { + delete all[key]; + changed = true; + } } - } - if (changed) await this.persist(all); + if (changed) await this.persist(all); + }); } + /** + * Write through a temporary file and rename into place. + * + * A write interrupted partway would otherwise leave truncated JSON, and + * `load()` treats unparseable content as empty — which would quietly discard + * every saved credential. + */ private async persist(all: Record): Promise { await mkdir(path.dirname(this.file), { recursive: true }); - await writeFile(this.file, JSON.stringify(all, null, 2), { mode: 0o600 }); + const temporary = `${this.file}.tmp`; + try { + await writeFile(temporary, JSON.stringify(all, null, 2), { mode: 0o600 }); + await rename(temporary, this.file); + } catch (error) { + await rm(temporary, { force: true }).catch(() => undefined); + throw error; + } } } diff --git a/src/anime-bridge/sidecar-process.test.ts b/src/anime-bridge/sidecar-process.test.ts new file mode 100644 index 00000000..0d9ac72b --- /dev/null +++ b/src/anime-bridge/sidecar-process.test.ts @@ -0,0 +1,47 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { EventEmitter } from 'node:events'; +import type { spawn as spawnType, ChildProcess } from 'node:child_process'; +import { allocatePort, startSidecar } from './sidecar-process'; +import type { BundleBinaries } from './sidecar-bundle'; + +const binaries: BundleBinaries = { + javaPath: '/nonexistent/java', + jarPath: '/tmp/MExtensionServer.jar', +}; + +/** A ChildProcess stand-in: an EventEmitter with the bits startSidecar touches. */ +function fakeChild(): ChildProcess { + const child = new EventEmitter(); + Object.assign(child, { stdout: null, stderr: null, kill: () => true }); + return child as unknown as ChildProcess; +} + +test('a failed spawn rejects instead of throwing an unhandled error event', async () => { + const port = await allocatePort(); + const child = fakeChild(); + const spawnImpl = (() => { + // Node emits `error` asynchronously when the binary cannot be executed. + queueMicrotask(() => child.emit('error', new Error('spawn ENOENT'))); + return child; + }) as unknown as typeof spawnType; + + await assert.rejects( + () => startSidecar({ binaries, port, readyTimeoutMs: 2000, spawnImpl }), + /could not start.*ENOENT/, + ); +}); + +test('an early exit is reported with its code rather than waiting out the deadline', async () => { + const port = await allocatePort(); + const child = fakeChild(); + const spawnImpl = (() => { + queueMicrotask(() => child.emit('exit', 1, null)); + return child; + }) as unknown as typeof spawnType; + + await assert.rejects( + () => startSidecar({ binaries, port, readyTimeoutMs: 2000, spawnImpl }), + /exited before becoming ready \(code 1/, + ); +}); diff --git a/src/anime-bridge/sidecar-process.ts b/src/anime-bridge/sidecar-process.ts index 5123d8f9..4675b410 100644 --- a/src/anime-bridge/sidecar-process.ts +++ b/src/anime-bridge/sidecar-process.ts @@ -7,6 +7,12 @@ import type { BundleBinaries } from './sidecar-bundle'; /** Cold start includes JVM boot plus AndroidCompat init; be generous. */ export const DEFAULT_READY_TIMEOUT_MS = 30_000; const READY_POLL_INTERVAL_MS = 500; +/** How long to wait for the child to go after each signal before escalating. */ +const STOP_TIMEOUT_MS = 5_000; + +function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} /** Ask the OS for a free loopback port, then hand it to the JVM. */ export async function allocatePort(): Promise { @@ -68,8 +74,20 @@ export async function startSidecar(options: StartSidecarOptions): Promise { - exited = { code, signal }; + let spawnError: Error | null = null; + const hasExited = new Promise((resolve) => { + child.once('exit', (code, signal) => { + exited = { code, signal }; + resolve(); + }); + // A ChildProcess is an EventEmitter: without this listener a failed spawn + // (a missing or non-executable java) throws in the main process instead of + // failing the readiness loop below. + child.once('error', (error: Error) => { + spawnError = error; + if (exited === null) exited = { code: null, signal: null }; + resolve(); + }); }); const stop = async (): Promise => { @@ -80,13 +98,24 @@ export async function startSidecar(options: StartSidecarOptions): Promise setTimeout(resolve, READY_POLL_INTERVAL_MS)); + await delay(READY_POLL_INTERVAL_MS); } await stop(); diff --git a/src/animeui/animeui.ts b/src/animeui/animeui.ts index f5a942e0..7ebb077e 100644 --- a/src/animeui/animeui.ts +++ b/src/animeui/animeui.ts @@ -488,18 +488,25 @@ api.onBridgeState(renderBridgeState); void (async () => { renderBridgeState({ stage: 'idle', progress: null, message: null }); - const state = await api.ensureBridge(); - renderBridgeState(state); + try { + const state = await api.ensureBridge(); + renderBridgeState(state); - const snapshot = await api.getSnapshot(); - renderSources(snapshot.sources, snapshot.selectedSourceId); + const snapshot = await api.getSnapshot(); + renderSources(snapshot.sources, snapshot.selectedSourceId); - if (state.stage === 'ready' && snapshot.sources.length > 0) { - searchInput.focus(); - await runSearch(''); - } else if (state.stage === 'ready') { - setStatus(state.message ?? 'No extensions installed.', 'error'); - } else { - setStatus(state.message ?? 'The extension bridge is not available.', 'error'); + if (state.stage === 'ready' && snapshot.sources.length > 0) { + searchInput.focus(); + await runSearch(''); + } else if (state.stage === 'ready') { + setStatus(state.message ?? 'No extensions installed.', 'error'); + } else { + setStatus(state.message ?? 'The extension bridge is not available.', 'error'); + } + } catch (error) { + // Without this the window keeps the "starting" banner up forever, with the + // search box disabled and nothing saying why. + renderBridgeState({ stage: 'failed', progress: null, message: describe(error) }); + setStatus(describe(error), 'error'); } })(); diff --git a/src/animeui/preferences-fields.ts b/src/animeui/preferences-fields.ts index 579dba48..fdf3cd39 100644 --- a/src/animeui/preferences-fields.ts +++ b/src/animeui/preferences-fields.ts @@ -19,6 +19,29 @@ function isSecretKey(view: SourcePreferenceView): boolean { return /password|token|api[-_ ]?key|secret/i.test(`${view.key} ${view.title}`); } +/** + * Structure of a schema, ignoring the values. + * + * A commit hands back the whole schema, but re-rendering on every save would + * throw away the "Saved" note, move focus off the control that was just + * committed, and rebuild a `multi` group out from under a second toggle that is + * still in flight. Only a structural change is worth a rebuild — which is + * exactly the case that matters, Jellyfin filling in its library picker after a + * successful login. + */ +function schemaShape(views: SourcePreferenceView[]): string { + return JSON.stringify( + views.map((view) => [ + view.key, + view.kind, + view.title, + view.summary, + view.entries, + view.entryValues, + ]), + ); +} + function renderPreferenceField( container: HTMLElement, view: SourcePreferenceView, @@ -46,10 +69,14 @@ function renderPreferenceField( state.removeAttribute('data-tone'); state.textContent = 'Saving…'; try { - const refreshed = await commit(view.key, value); + // Commits are chained so a second toggle cannot land before the first + // one's round trip finishes and overwrite it with a stale array. + const refreshed = await enqueueCommit(container, () => commit(view.key, value)); state.dataset.tone = 'ok'; state.textContent = 'Saved'; - renderPreferences(container, refreshed, commit); + if (schemaShape(refreshed) !== renderedShapes.get(container)) { + renderPreferences(container, refreshed, commit); + } } catch (error) { state.dataset.tone = 'error'; state.textContent = describe(error); @@ -120,11 +147,33 @@ function renderPreferenceField( return field; } +/** Shape currently on screen, per container, so a save can skip a no-op rebuild. */ +const renderedShapes = new WeakMap(); + +/** Serializes commits per container; see the comment in `save`. */ +const commitQueues = new WeakMap>(); + +function enqueueCommit( + container: HTMLElement, + run: () => Promise, +): Promise { + const previous = commitQueues.get(container) ?? Promise.resolve(); + const result = previous.then(run, run); + // Swallow here only so one failed commit does not wedge the queue; the + // caller still sees the rejection. + commitQueues.set( + container, + result.catch(() => undefined), + ); + return result; +} + export function renderPreferences( container: HTMLElement, views: SourcePreferenceView[], commit: PreferenceCommit, ): void { + renderedShapes.set(container, schemaShape(views)); container.replaceChildren(...views.map((view) => renderPreferenceField(container, view, commit))); if (views.length === 0) { const empty = document.createElement('p'); diff --git a/src/main/runtime/anime-bridge-installer.ts b/src/main/runtime/anime-bridge-installer.ts index 1703e575..dcc1fec5 100644 --- a/src/main/runtime/anime-bridge-installer.ts +++ b/src/main/runtime/anime-bridge-installer.ts @@ -17,6 +17,10 @@ import { export type InstallStage = 'locating' | 'downloading' | 'verifying' | 'extracting'; +/** Neither call has a default deadline, so a hung network would stall install. */ +const RELEASES_TIMEOUT_MS = 30_000; +const DOWNLOAD_TIMEOUT_MS = 300_000; + export interface InstallProgress { stage: InstallStage; /** 0-1 during download, otherwise null. */ @@ -119,6 +123,7 @@ export async function ensureBridgeBinaries(options: EnsureBridgeOptions): Promis options.onProgress?.({ stage: 'locating', progress: null }); const releasesResponse = await fetchImpl(BUNDLE_RELEASES_URL, { headers: { Accept: 'application/vnd.github+json' }, + signal: AbortSignal.timeout(RELEASES_TIMEOUT_MS), }); if (!releasesResponse.ok) { throw new Error(`Could not list anime bridge releases (${releasesResponse.status}).`); @@ -129,7 +134,9 @@ export async function ensureBridgeBinaries(options: EnsureBridgeOptions): Promis } options.onProgress?.({ stage: 'downloading', progress: 0 }); - const downloadResponse = await fetchImpl(asset.downloadUrl); + const downloadResponse = await fetchImpl(asset.downloadUrl, { + signal: AbortSignal.timeout(DOWNLOAD_TIMEOUT_MS), + }); if (!downloadResponse.ok) { throw new Error(`Downloading the anime bridge failed (${downloadResponse.status}).`); }