fix(admin): clear pending callbacks on ACL timeout/response — stop the leak (review AD-1/CS-2)
Some checks failed
Docker image / build-and-push-image (push) Has been cancelled
Some checks failed
Docker image / build-and-push-image (push) Has been cancelled
requestPermission's 10s approval timeout called resolve(undefined) but never clearPending, and a single admin's response cleared only its own id. So every timed-out approval, and (with multiple admin npubs) the non-responding admins' entries, leaked permanently in transport.pending — each retaining its closure over remotePubkey/keyName/method/the serialized sign_event payload, so growth tracked signing traffic. Scope: the admin-DM auth path (only when no web baseUrl is set). - Collect every issued request id; a single `finish()` resolves once and clears ALL of them, on both timeout and the first response. - A `settled` latch makes late/duplicate responses no-ops — which also stops a late 'always' approval from running allowAllRequestsFromKey after the request already resolved (review AD-2). - Guard nip19.decode so a malformed admin npub skips that admin instead of throwing through the loop. - Defense-in-depth: bound transport.pending at 1000 (evict oldest) so any other un-cleared path can't grow it without limit. tsc at baseline; daemon bundles; admin + nip46 suites green. Refs: transport review AD-1/CS-2/AD-2; #42
This commit is contained in:
parent
1754027249
commit
47a5071b4a
2 changed files with 35 additions and 6 deletions
|
|
@ -397,13 +397,33 @@ class AdminInterface {
|
||||||
return new Promise((resolve) => {
|
return new Promise((resolve) => {
|
||||||
console.log(`requesting permission for`, keyName, { remotePubkey, method });
|
console.log(`requesting permission for`, keyName, { remotePubkey, method });
|
||||||
|
|
||||||
|
const ids: string[] = [];
|
||||||
|
let settled = false;
|
||||||
|
// Resolve once and ALWAYS clear every pending sendRequest callback —
|
||||||
|
// on timeout AND on the first admin response. Previously the timeout
|
||||||
|
// cleared nothing and a single admin's response cleared only its own
|
||||||
|
// id, so every timed-out request and (with multiple admins) the
|
||||||
|
// non-responding admins' entries leaked in transport.pending for the
|
||||||
|
// process lifetime (review AD-1/CS-2). The `settled` latch also stops
|
||||||
|
// a late approval from acting after the request resolved (review AD-2).
|
||||||
|
const finish = (value: boolean | undefined) => {
|
||||||
|
if (settled) return;
|
||||||
|
settled = true;
|
||||||
|
for (const id of ids) this.transport.clearPending(id);
|
||||||
|
resolve(value);
|
||||||
|
};
|
||||||
|
|
||||||
// If an admin doesn't respond within 10 seconds, report timeout.
|
// If an admin doesn't respond within 10 seconds, report timeout.
|
||||||
setTimeout(() => {
|
setTimeout(() => finish(undefined), 10000);
|
||||||
resolve(undefined);
|
|
||||||
}, 10000);
|
|
||||||
|
|
||||||
for (const npub of this.npubs) {
|
for (const npub of this.npubs) {
|
||||||
const adminPubkey = nip19.decode(npub).data as string;
|
let adminPubkey: string;
|
||||||
|
try {
|
||||||
|
adminPubkey = nip19.decode(npub).data as string;
|
||||||
|
} catch {
|
||||||
|
console.log(`skipping malformed admin npub: ${npub}`);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
const params = JSON.stringify({
|
const params = JSON.stringify({
|
||||||
keyName,
|
keyName,
|
||||||
remotePubkey,
|
remotePubkey,
|
||||||
|
|
@ -419,17 +439,18 @@ class AdminInterface {
|
||||||
'nip44',
|
'nip44',
|
||||||
NIP46_ADMIN_RESPONSE_KIND,
|
NIP46_ADMIN_RESPONSE_KIND,
|
||||||
(res) => {
|
(res) => {
|
||||||
this.transport.clearPending(id);
|
if (settled) return; // ignore late / duplicate responses
|
||||||
this.requestPermissionResponse(
|
this.requestPermissionResponse(
|
||||||
remotePubkey,
|
remotePubkey,
|
||||||
keyName,
|
keyName,
|
||||||
method,
|
method,
|
||||||
param,
|
param,
|
||||||
resolve,
|
finish,
|
||||||
res
|
res
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
);
|
);
|
||||||
|
ids.push(id);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -132,6 +132,14 @@ export class Nip46Transport {
|
||||||
): string {
|
): string {
|
||||||
const id = Math.random().toString(36).substring(2, 12);
|
const id = Math.random().toString(36).substring(2, 12);
|
||||||
this.pending.set(id, cb);
|
this.pending.set(id, cb);
|
||||||
|
// Defense-in-depth bound: callers (admin requestPermission) clear pending
|
||||||
|
// entries on resolve/timeout, but cap the map so any un-cleared path can't
|
||||||
|
// grow it without limit — evict the oldest (it would time out anyway).
|
||||||
|
// (review AD-1/CS-2)
|
||||||
|
if (this.pending.size > 1000) {
|
||||||
|
const oldest = this.pending.keys().next().value;
|
||||||
|
if (oldest !== undefined) this.pending.delete(oldest);
|
||||||
|
}
|
||||||
const content = this.encrypt(remotePubkey, JSON.stringify({ id, method, params }), encryption);
|
const content = this.encrypt(remotePubkey, JSON.stringify({ id, method, params }), encryption);
|
||||||
const event = finalizeEvent(
|
const event = finalizeEvent(
|
||||||
{ kind, created_at: Math.floor(Date.now() / 1000), tags: [["p", remotePubkey]], content },
|
{ kind, created_at: Math.floor(Date.now() / 1000), tags: [["p", remotePubkey]], content },
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue