Skip to content

auth: serve /.well-known/did/nostr/<pubkey>.json (#407) - #408

Merged
melvincarvalho merged 21 commits into
gh-pagesfrom
issue-407-well-known-did-nostr
May 10, 2026
Merged

auth: serve /.well-known/did/nostr/<pubkey>.json (#407)#408
melvincarvalho merged 21 commits into
gh-pagesfrom
issue-407-well-known-did-nostr

Conversation

@melvincarvalho

@melvincarvalho melvincarvalho commented May 10, 2026

Copy link
Copy Markdown
Contributor

Closes #407. Refs #403/#405 (typed-username fallback this supersedes for local accounts), #4/#386 (cross-protocol unification).

JSS now publishes did:nostr DID documents at the spec-canonical HTTP-resolution path for any account whose profile carries a Nostr Multikey verificationMethod. Each pod becomes its own authoritative DID resolver — local index and global resolver are now the same mechanism, just sharded by host.

After this lands and deploys, the IdP Sign in with Schnorr button works end-to-end without typing a username for local accounts. The typed-username fallback (#405) stays as a third-tier safety net.

What ships

  • src/idp/well-known-did-nostr.js — Fastify handler for GET /.well-known/did/nostr/:pubkeyAndExt. Accepts <pubkey>.json, <pubkey>.jsonld, and bare <pubkey>. Generates a CID-shaped DID doc with alsoKnownAs: [<webId>] derived from the account record.
    • Per-status header policy:
      • 200: Cache-Control: max-age=3600, Nostr-Timestamp (resolver clock at answer time), Last-Modified (profile mtime).
      • 404: Cache-Control: max-age=60, Nostr-Timestamp. Short TTL so newly added keys surface fast.
      • 400: Cache-Control: no-store, Nostr-Timestamp.
    • Content-Type: application/did+json for .json and bare; application/did+ld+json for the .jsonld alias.
  • Pubkey index — lazily built from <DATA_ROOT>/.idp/accounts/_webid_index.json by scanning each account's profile (path derived from the account WebID's pathname, with containment check, so root-level single-user pods at <DATA_ROOT>/profile/card.jsonld work alongside named pods). 5-min TTL with rebuild dedup (one in-flight rebuild at a time). 64 KB per-profile size cap. CID-semantics validation: profile @id must equal account.webId, VM controller must be in the profile's expected controller set, VM id must be in authentication. JWK entries also require BIP-340 even-y match. Duplicate-pubkey detection drops ambiguous bindings + logs loudly. Per-account try/catch so one corrupt file can't 500 NIP-98 traffic. Real-time hook on the LDP write path is a follow-up.
  • src/server.js — registers the route at the top level (before the LDP wildcard GET /* so it actually matches). Async fastify.register(...) so the well-known module is dynamic-imported only when IdP is enabled — non-IdP deployments don't pay the IdP/accounts module startup cost. HEAD shares the GET handler. PUT/POST/PATCH/DELETE return 405 across the entire /.well-known/did/nostr/* subtree REGARDLESS of idpEnabled (the /.well-known/* namespace bypasses WAC unconditionally, so without explicit method blocks the wildcard write handlers would accept unauthenticated writes — even on non-IdP pods).
  • src/auth/nostr.js — exports extractNostrPubkeysFromProfile() for the index. verifyNostrAuth now resolves Nostr pubkeys against the in-process local index first (no HTTP, no SSRF surface), gated on request.idpEnabled, before falling back to the external did:nostr resolver for cross-pod identities. The local-resolver module is dynamic-imported only on IdP deployments. Helpers (decodeFFormSecp256k1, extractNostrPubkeysFromProfile, pubkeyFromValidatedJwk) live in src/auth/nostr-keys.js to avoid a circular dependency between the verifier and the publisher.
  • src/auth/did-nostr.jsresolveDidNostrToWebId() resolves via the configured external resolver URL and ALWAYS verifies the WebID profile actually claims the pubkey (no same-origin shortcut — multi-tenant origins make "same origin = same control" false). The backlink check accepts either a CID v1 verificationMethod referenced from authentication (the JSS-native shape) OR a classic owl:sameAs to did:nostr:<pubkey>. SSRF + redirect hardened: fetchWithRedirectGuard re-validates SSRF on every redirect hop, refuses cross-origin redirects, caps body at 1 MB, and applies a 5-hop redirect limit. Resolution cache is a bounded LRU keyed on (resolverUrl, pubkey) with separate TTLs for transient failures (1m) and steady-state answers (5m).

Test plan

  • 25 module tests for the well-known endpoint and helpers (DID-doc shape, headers, alias suffixes, 404/400 with consistent headers, HEAD parity, multi-segment 405, root-pod indexing, relative-@id profiles, auth-membership filter, JWK BIP-340 even-y validation, path containment).
  • 22 module tests for did-nostr (resolver, CID-VM backlink, SSRF/redirect/size hardening, LRU cap, cross-resolver cache isolation, real-world resolver round-trip).
  • Full suite: 710 → 745+ pass, no regressions.
  • Manual on solid.social: with no username typed, click Sign in with Schnorr → resolved via local well-known DID doc → logged in as WebID.

Known gaps (deferred follow-ups)

  • Index rebuild only on TTL expiry; a write-path hook on LDP PUT/PATCH would update immediately
  • Cross-pod resolution still relies on the existing nostr.social fallback; per-pod NIP-05 didResolver discovery is mentioned in the spec but not implemented here

Refs

  • did:nostr spec § HTTP Resolution — defines the /.well-known/did/nostr/<pubkey>.json path and required headers
  • nostr.social and nostr.rocks both implement the same path

JSS now publishes did:nostr DID documents at the spec-canonical
HTTP-resolution path for any local account whose profile carries
a Nostr Multikey verificationMethod. Each pod becomes its own
authoritative DID resolver — closing the loop that was forcing
the IdP "Sign in with Schnorr" flow to fall back to typed-username
hint (#403/#405).

Pieces:

- src/idp/well-known-did-nostr.js: Fastify handler. Lazily builds
  a pubkey → accountId index by scanning <DATA_ROOT>/.idp/accounts/_webid_index.json
  and reading each account's profile for f-form Multikey + JsonWebKey
  entries. 5-min TTL; rebuild on miss. Production hook on LDP write
  path is a follow-up.
- Generates a CID-shaped DID doc (`@context` per spec, `type:DIDNostr`,
  `alsoKnownAs:[<webId>]` from the account record, Multikey VM
  derived deterministically from the pubkey). Headers per spec:
  Content-Type application/did+json (or +ld+json for .jsonld alias),
  Cache-Control max-age=3600, Nostr-Timestamp, Last-Modified.
- Accepts <pubkey>.json, <pubkey>.jsonld, and bare <pubkey> on the
  same handler. 400 on non-hex / wrong-length, 404 when no local
  account claims the pubkey.

- src/server.js: register the route directly (before the LDP wildcard
  GET /* handler). Registering inside the IdP plugin let the wildcard
  swallow the dynamic-segment + .json path before our route could
  match.

- src/auth/nostr.js: extracted extractNostrPubkeysFromProfile() —
  enumerates every Nostr-shaped pubkey in a profile (Multikey or
  JWK x-coord). Used by the index rebuild.

- src/auth/nostr.js: verifyNostrAuth's DID-doc fallback now passes
  the request's own host as the first resolver via buildResolverList(),
  ahead of the configured external resolver. Same-pod sign-ins
  resolve with no third-party hop.

- src/auth/did-nostr.js: resolveDidNostrToWebId now accepts an array
  of resolver URLs, tries each in order, returns the first hit.
  verifyWebIdBacklink gains a same-origin shortcut: a DID doc served
  from the same origin as the WebID is authoritative for that origin
  and doesn't need a bidirectional sameAs check (which a JSS profile
  doesn't carry by default — it asserts the pubkey via
  verificationMethod, not via sameAs). This is the key change that
  makes the flow zero-typing for local users without requiring
  changes to the profile shape.

Tests: 6 integration tests (well-known endpoint live under a real
JSS server) + 4 unit tests for extractNostrPubkeysFromProfile.
Full suite 710 → 720 pass, no regressions.

Closes #407. Refs #403/#405 (typed-username fallback this supersedes
for local users), #4/#386 (cross-protocol unification).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds first-party did:nostr HTTP resolution to JSS pods by publishing DID documents at the spec-canonical /.well-known/did/nostr/<pubkey>.json path for locally hosted accounts, and updates the existing resolver to try the current pod before falling back to external resolvers—enabling “Sign in with Schnorr” flows to resolve local users without typed usernames.

Changes:

  • Adds a new Fastify handler to serve did:nostr DID docs from /.well-known/did/nostr/:pubkey(.json|.jsonld)?, backed by a lazy pubkey→account index.
  • Registers the new route in src/server.js ahead of the GET /* wildcard so it matches.
  • Updates Nostr auth resolution to try local well-known resolution first, and extends the DID resolver to accept an ordered list of resolver base URLs; adds integration/unit tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
test/well-known-did-nostr.test.js Adds integration tests for the new well-known endpoint and unit tests for profile pubkey extraction.
src/server.js Registers the /.well-known/did/nostr/:pubkeyAndExt route before the LDP wildcard route.
src/idp/well-known-did-nostr.js Implements the well-known did:nostr handler and the on-disk-derived pubkey index (TTL cached).
src/auth/nostr.js Exports extractNostrPubkeysFromProfile() and switches DID resolution to use a local-first resolver list.
src/auth/did-nostr.js Allows trying multiple resolver base URLs and adds a same-origin shortcut to avoid backlink verification.
Comments suppressed due to low confidence (1)

src/auth/did-nostr.js:132

  • When extracting a WebID from didDoc.alsoKnownAs, the code only accepts values starting with https://. In local/dev/test deployments JSS frequently serves pods over plain HTTP (e.g. test helpers use http://127.0.0.1), and this PR’s new local well-known publisher will emit alsoKnownAs using that scheme, so resolution will silently fail. Consider accepting http:// as well (at least in non-production or when same-origin with the fetched DID doc).
    // Extract WebID from alsoKnownAs (array) or profile.webid or profile.sameAs
    let webId = null;

    if (Array.isArray(didDoc.alsoKnownAs) && didDoc.alsoKnownAs.length > 0) {
      // Find first HTTP(S) URL that looks like a WebID
      webId = didDoc.alsoKnownAs.find(aka =>
        typeof aka === 'string' && aka.startsWith('https://'));
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/auth/nostr.js Outdated
Comment on lines +503 to +521
* Local-first: try this pod's own well-known DID-doc endpoint
* (#407 — a JSS pod is its own DID resolver for its accounts) before
* falling back to the configured external resolver. For same-pod
* sign-ins this is a zero-network self-resolve; cross-pod identities
* still resolve via nostr.social etc.
*/
function buildResolverList(request) {
const list = [];
const headers = request.headers || {};
const proto = firstHeaderValue(headers['x-forwarded-proto']) || request.protocol || 'https';
const host = firstHeaderValue(headers['x-forwarded-host'])
|| request.hostname
|| firstHeaderValue(headers.host);
if (host && /^[A-Za-z0-9.\-:[\]]+$/.test(host)) {
list.push(`${proto.toLowerCase()}://${host}/.well-known/did/nostr`);
}
// Fallback: keep the existing external resolver as last resort.
list.push('https://nostr.social/.well-known/did/nostr');
return list;
Comment thread src/auth/did-nostr.js Outdated
Comment on lines +102 to +118
try {
// Fetch DID document
const didUrl = `${resolverUrl}/${pubkey}.json`;
const didRes = await fetchWithTimeout(didUrl, {
headers: { 'Accept': 'application/did+json, application/json' }
});

if (!didRes.ok) {
// Try each resolver in order; first success wins. Track which URL
// the doc came from so verifyWebIdBacklink can apply the
// same-origin shortcut (an authoritative DID doc served from the
// WebID's own host doesn't need a bidirectional sameAs check).
let didDoc = null;
let foundAtUrl = null;
for (const resolverUrl of resolvers) {
const didUrl = `${resolverUrl}/${pubkey}.json`;
const didRes = await fetchWithTimeout(didUrl, {
headers: { 'Accept': 'application/did+json, application/json' }
}).catch(() => null);
if (didRes && didRes.ok) {
didDoc = await didRes.json();
foundAtUrl = didUrl;
break;
}
Comment thread src/auth/did-nostr.js Outdated
Comment on lines 39 to 56
/**
* Fetch with timeout
*/
/**
* Are two URLs same-origin? Used by the DID-doc resolver: a doc
* served from the same host as the WebID it claims is authoritative
* for that origin and doesn't need a bidirectional check.
*/
function sameOrigin(urlA, urlB) {
if (typeof urlA !== 'string' || typeof urlB !== 'string') return false;
try {
return new URL(urlA).origin === new URL(urlB).origin;
} catch {
return false;
}
}

async function fetchWithTimeout(url, options = {}, timeout = 5000) {
Comment thread src/idp/well-known-did-nostr.js Outdated
Comment on lines +41 to +67
// Match the layout in src/idp/accounts.js — accounts live under
// <DATA_ROOT>/.idp/accounts. Computed lazily so DATA_ROOT changes
// (test setup, env override) are picked up.
function getAccountsDir() {
const dataRoot = process.env.DATA_ROOT || './data';
return path.join(dataRoot, '.idp', 'accounts');
}
function getWebIdIndexPath() {
return path.join(getAccountsDir(), '_webid_index.json');
}

async function readJsonOrEmpty(file) {
try { return await fs.readJson(file); } catch { return null; }
}

async function rebuildPubkeyIndex({ dataRoot }) {
const idx = new Map();
const webIdIndex = await readJsonOrEmpty(getWebIdIndexPath());
if (!webIdIndex) {
pubkeyIndex = idx;
indexBuiltAt = Date.now();
return;
}
for (const [, accountId] of Object.entries(webIdIndex)) {
const account = await findById(accountId);
if (!account?.podName) continue;
const profilePath = path.join(dataRoot, account.podName, 'profile', 'card.jsonld');
Comment thread src/idp/well-known-did-nostr.js Outdated
}

async function readJsonOrEmpty(file) {
try { return await fs.readJson(file); } catch { return null; }
Comment thread src/idp/well-known-did-nostr.js Outdated
if (!/^[0-9a-f]{64}$/i.test(pubkey)) {
return reply.code(400)
.header('Content-Type', 'application/json')
.send({ error: 'pubkey must be 64 hex chars (lowercase)' });
Comment on lines +165 to +170
return reply
.header('Content-Type', contentType)
.header('Cache-Control', 'max-age=3600')
.header('Nostr-Timestamp', String(Math.floor(Date.now() / 1000)))
.header('Last-Modified', new Date().toUTCString())
.send(didDoc);
Comment thread src/idp/well-known-did-nostr.js Outdated
Comment on lines +75 to +79
for (const { pubkey } of extractNostrPubkeysFromProfile(profile)) {
// First-write wins; if two accounts somehow declare the same
// pubkey, the first one resolved keeps the binding.
if (!idx.has(pubkey)) idx.set(pubkey, accountId);
}
Eight findings, two of them genuine security bugs.

1. SSRF gadget in buildResolverList (#408 line 521). The new
   "try request host first" did:nostr resolution fed
   request-controlled Host / X-Forwarded-* headers into a fetch().
   An attacker could craft headers to force outbound fetches to
   arbitrary internal hosts. Eliminated entirely by switching local
   resolution to an in-process function call:
   `resolveDidNostrLocally(pubkey)` — no HTTP, no SSRF surface.
   resolveDidNostrToWebId reverts to single-resolver signature
   (the existing external nostr.social fallback).

2. SSRF on did:nostr external fetches (line 118). The existing
   resolveDidNostrToWebId / verifyWebIdBacklink fetches had no
   SSRF protection. Now both go through validateExternalUrl with
   blockPrivateIPs/resolveDNS/requireHttps-in-prod (matching the
   LWS-CID verifier's policy). The DEFAULT_DID_RESOLVER is
   trusted, but operators can configure others.

3. Index didn't filter by `authentication` membership (line 79).
   A VM present in verificationMethod but intentionally NOT in
   authentication (revocation pending, assertion-only, etc.)
   would still get a published DID doc that asserted authentication
   — defeating the user's exclusion. Now collects authentication
   IDs first and indexes only matching VMs. New test: a key
   pushed into verificationMethod without authentication
   membership returns 404.

4. Last-Modified always now() (line 170). Now reflects the
   underlying profile file's mtime so conditional GET / cache
   freshness work correctly. Index value bumped to
   `{ accountId, mtimeMs }`.

5. readJsonOrEmpty swallowed all errors (line 53). Now returns
   null only on ENOENT; logs other errors via console.error so
   operational issues (parse error, perms) aren't silent.

6. dataRoot parameter was misleading vs accounts.js (line 67).
   Documented the constraint in the doc comment: the parameter
   only differs meaningfully from process.env.DATA_ROOT in
   non-default deployments, and findById/account-index lookups
   always go through DATA_ROOT.

7. 400 message claimed lowercase but regex accepted uppercase
   (line 143). Now: lowercase pubkey before regex check, regex
   itself is lowercase-only, message just says "64 hex chars".

8. Stale "Fetch with timeout" JSDoc above sameOrigen (line 56).
   Removed.

Test count: 10 → 11 in the new module. Full suite: 720, no
regressions.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Comment thread src/auth/nostr.js Outdated
Comment on lines +290 to +293
// on this same pod.
const localWebId = await resolveDidNostrLocally(event.pubkey);
if (localWebId) {
return { webId: localWebId, error: null };
Comment thread src/idp/well-known-did-nostr.js Outdated
Comment on lines +96 to +100
const authIds = collectAuthenticationIds(profile);
for (const { pubkey, vm } of extractNostrPubkeysFromProfile(profile)) {
const vmId = absolutize(vm.id || vm['@id'], stripHashIfAny(profile['@id']));
if (!vmId || !authIds.has(vmId)) continue;
// First-write wins; if two accounts somehow declare the same
Comment thread src/idp/well-known-did-nostr.js Outdated
Comment on lines +100 to +103
// First-write wins; if two accounts somehow declare the same
// pubkey, the first one resolved keeps the binding.
if (!idx.has(pubkey)) idx.set(pubkey, { accountId, mtimeMs });
}
Comment thread src/idp/well-known-did-nostr.js Outdated
Comment on lines +53 to +56
* Read a JSON file, returning null only when it doesn't exist.
* Other failures (parse error, permission denied, etc.) propagate
* via console.error so operational issues aren't silently swallowed
* — they'd otherwise disable DID-doc publishing without any signal.
Comment thread src/auth/did-nostr.js
Comment on lines +81 to 85
* trailing `/<pubkey>.json`). Defaults to the configured
* DEFAULT_DID_RESOLVER (nostr.social).
* @returns {Promise<string|null>} WebID URL or null
*/
export async function resolveDidNostrToWebId(pubkey, resolverUrl = DEFAULT_DID_RESOLVER) {
Comment thread src/auth/nostr.js Outdated
Comment on lines 28 to 31
import { resolveDidNostrLocally } from '../idp/well-known-did-nostr.js';
import { fetchCidDocument } from './cid-doc-fetch.js';
import { normalizeControllers } from './lws-cid.js'; // shared JSON-LD controller helper

Comment on lines +44 to +48
function getAccountsDir() {
const dataRoot = process.env.DATA_ROOT || './data';
return path.join(dataRoot, '.idp', 'accounts');
}
function getWebIdIndexPath() {
Seven findings, all real.

1. Circular import nostr.js ↔ well-known-did-nostr.js. Extracted
   the shared helpers (extractNostrPubkeysFromProfile,
   decodeFFormSecp256k1) to src/auth/nostr-keys.js. Both callers
   now import from there one-directionally. nostr.js re-exports
   extractNostrPubkeysFromProfile for back-compat with the
   existing test.

2. Indexer didn't validate CID semantics — could publish a DID doc
   asserting `pubkey → webId` even when the underlying profile
   was inconsistent. Now mirrors the resource-side checks before
   indexing:
     - profile @id MUST equal account.webId (no fragment-swap)
     - VM controller MUST be in expected controller set
     - VM MUST be referenced from authentication (already had this)
   Profiles failing any check are skipped with no index entry.

3. Duplicate-pubkey "first-write wins" was silent. Now tracks every
   account that claims each pubkey, drops ambiguous ones from the
   index, and logs loudly via console.error. Resolution returns 404
   for ambiguous keys instead of an arbitrary pick.

4. resolveDidNostrLocally fired even with IdP disabled, hitting
   <DATA_ROOT>/.idp/accounts on every NIP-98 request. Gated behind
   request.idpEnabled so non-IdP deployments don't touch IdP
   storage.

5. readJsonOrEmpty doc said "null only on ENOENT" but actually
   returned null on any error (after logging non-ENOENT). Fixed
   the doc to match: ENOENT → silent null, other errors → null
   with console.error so the operational issue surfaces.

6. dataRoot parameter was misleading — only affected profile reads,
   not the account-index path which derives from process.env.DATA_ROOT.
   Removed the parameter entirely; everything now reads from the
   env. Single source of truth.

7. PR description mentioned "array of resolver URLs" but the
   implementation reverted to single resolverUrl in pass 1. Will
   update the PR body separately.

Test count: 11 → 11 in module (covered by existing
authentication-membership test). Full suite: 720 → 721 pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/auth/did-nostr.js:86

  • PR description says resolveDidNostrToWebId() now accepts an array of resolver URLs and tries each in order, but the implementation here still only accepts a single resolverUrl string. Either update the implementation to match the described API/behavior or adjust the PR description so it reflects the shipped code path (local resolution is currently done via resolveDidNostrLocally in verifyNostrAuth).
 * Resolve did:nostr pubkey to WebID via DID document.
 *
 * Local users are resolved by `resolveDidNostrLocally` in the auth
 * caller (well-known-did-nostr.js exports an in-process function) —
 * this resolver is the cross-pod fallback that fetches an external
 * DID doc, so all fetches run through the SSRF guard.
 *
 * @param {string} pubkey - 64-char hex Nostr pubkey
 * @param {string} [resolverUrl] - DID resolver base URL (without the
 *   trailing `/<pubkey>.json`). Defaults to the configured
 *   DEFAULT_DID_RESOLVER (nostr.social).
 * @returns {Promise<string|null>} WebID URL or null
 */
export async function resolveDidNostrToWebId(pubkey, resolverUrl = DEFAULT_DID_RESOLVER) {
  if (!pubkey || pubkey.length !== 64) {

Comment thread src/server.js Outdated
Comment on lines +661 to +662
const wellKnownDidNostr = buildWellKnownDidNostrHandler();
fastify.get('/.well-known/did/nostr/:pubkeyAndExt', wellKnownDidNostr);
Comment thread test/well-known-did-nostr.test.js Outdated
Comment on lines +43 to +48
before(async () => {
// IdP must be enabled — pod creation only writes an account
// record (the index this endpoint reads from) when the IdP is
// running. Pods without IdP are out of scope for this MVP.
await startTestServer({ idp: true, idpIssuer: 'http://127.0.0.1' });
baseUrl = getBaseUrl();
Comment thread src/server.js Outdated
// GET /* handler below and never reaches our route.
if (idpEnabled) {
const wellKnownDidNostr = buildWellKnownDidNostrHandler();
fastify.get('/.well-known/did/nostr/:pubkeyAndExt', wellKnownDidNostr);
Three findings, all real.

1. /.well-known/* bypasses the WAC preHandler (correct — it's a
   public namespace). But that means the wildcard write handlers
   (PUT/POST/PATCH/DELETE /*) would accept unauthenticated writes
   under /.well-known/did/nostr/<anything>, creating files on disk
   that the GET handler would then ignore (it only reads the
   account index). Storage abuse vector.

   Fix: register explicit method handlers for the namespace that
   return 405 Method Not Allowed with an Allow header. Fastify's
   route specificity beats the wildcard, so writes never reach the
   LDP layer.

2. HEAD requests fell through to the wildcard HEAD /* handler,
   which looked for an on-disk file and returned 404 even when
   GET returned 200. Inconsistent.

   Fix: register HEAD with the same handler as GET so headers
   (Content-Type, Cache-Control, Last-Modified) match.

3. The integration test hard-coded idpIssuer to
   `http://127.0.0.1` (no port) while the helper bound to an
   OS-assigned ephemeral port. The mismatch was harmless for the
   GET-only tests we had, but oidc-provider behavior depends on
   the issuer being accurate, and the divergence from every
   other IdP test in the suite was a footgun.

   Fix: switched to the established pattern from test/idp.test.js
   — pick an available port up front via a tiny net.createServer
   helper, build baseUrl, pass it as idpIssuer, listen on that
   port. No more lying about the port.

Tests: added two new ones to lock the new behavior in:
  - HEAD returns 200 with the same headers as GET, empty body
  - PUT/POST/PATCH/DELETE all return 405 with Allow header
Total: 11 → 13 in module, 721 → 723 in full suite.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

src/auth/did-nostr.js:132

  • resolveDidNostrToWebId currently only considers alsoKnownAs entries that start with https://, which prevents DID docs that legitimately point at http:// WebIDs from resolving in non-production/dev setups (even though the SSRF validator allows HTTP when NODE_ENV !== 'production'). Consider accepting both http:// and https:// here and relying on the existing URL validation policy to enforce HTTPS where required.
    // Extract WebID from alsoKnownAs (array) or profile.webid or profile.sameAs
    let webId = null;

    if (Array.isArray(didDoc.alsoKnownAs) && didDoc.alsoKnownAs.length > 0) {
      // Find first HTTP(S) URL that looks like a WebID
      webId = didDoc.alsoKnownAs.find(aka =>
        typeof aka === 'string' && aka.startsWith('https://'));
    }

src/auth/did-nostr.js:86

  • The PR description mentions resolveDidNostrToWebId() accepting an array of resolver URLs and trying each in order, but the implementation still only accepts a single resolverUrl string. Either update the function to support the described array behavior (and update callers) or adjust the PR description/docs to match the shipped API.
 * Resolve did:nostr pubkey to WebID via DID document.
 *
 * Local users are resolved by `resolveDidNostrLocally` in the auth
 * caller (well-known-did-nostr.js exports an in-process function) —
 * this resolver is the cross-pod fallback that fetches an external
 * DID doc, so all fetches run through the SSRF guard.
 *
 * @param {string} pubkey - 64-char hex Nostr pubkey
 * @param {string} [resolverUrl] - DID resolver base URL (without the
 *   trailing `/<pubkey>.json`). Defaults to the configured
 *   DEFAULT_DID_RESOLVER (nostr.social).
 * @returns {Promise<string|null>} WebID URL or null
 */
export async function resolveDidNostrToWebId(pubkey, resolverUrl = DEFAULT_DID_RESOLVER) {
  if (!pubkey || pubkey.length !== 64) {

Comment thread src/auth/nostr.js Outdated
Comment on lines 28 to 33
import { resolveDidNostrLocally } from '../idp/well-known-did-nostr.js';
import { fetchCidDocument } from './cid-doc-fetch.js';
import { normalizeControllers } from './lws-cid.js'; // shared JSON-LD controller helper
import { decodeFFormSecp256k1, extractNostrPubkeysFromProfile } from './nostr-keys.js'; // re-exported for back-compat
export { extractNostrPubkeysFromProfile };

Comment on lines +267 to +277
if (!/^[0-9a-f]{64}$/.test(pubkey)) {
return reply.code(400)
.header('Content-Type', 'application/json')
.send({ error: 'pubkey must be 64 hex chars' });
}
const found = await findAccountByNostrPubkey(pubkey);
if (!found?.account) {
return reply.code(404)
.header('Cache-Control', 'max-age=60')
.header('Content-Type', 'application/json')
.send({ error: 'no local account claims this pubkey' });
Comment thread test/well-known-did-nostr.test.js Outdated
import { extractNostrPubkeysFromProfile } from '../src/auth/nostr.js';

const TEST_HOST = '127.0.0.1';
const TEST_DATA_DIR = './data';
Comment on lines +78 to +80
await server.listen({ port, host: TEST_HOST });
process.env.DATA_ROOT = path.resolve(TEST_DATA_DIR);
// IdP-enabled pod creation requires email + password (so the
Comment thread src/idp/well-known-did-nostr.js Outdated
// diagnose; better to refuse and log loudly.
const seenAccounts = new Map(); // pubkey -> Set<accountId>
for (const [, accountId] of Object.entries(webIdIndex)) {
const account = await findById(accountId);
Five findings, all real.

1. Non-IdP deployments paid the IdP/accounts module startup cost
   (transitively bcryptjs etc.) just by loading the NIP-98 verifier
   in src/auth/nostr.js — and at server startup via the static
   import in src/server.js. Both now lazy-load the well-known
   module:
     - src/auth/nostr.js: dynamic import inside the
       `if (request.idpEnabled)` branch
     - src/server.js: same logic moved into an async
       fastify.register(...) plugin so the dynamic import lives
       inside the IdP-only path without making createServer async
   Cost only paid on IdP deployments now.

2. 404/400 error responses omitted Nostr-Timestamp and used
   different cache policies than 200, with no documentation of
   the divergence. Aligned to a documented per-status policy:
     - 200  Cache-Control: max-age=3600 + Last-Modified (profile mtime)
     - 404  Cache-Control: max-age=60   (short TTL — newly added
                                         keys surface fast)
     - 400  Cache-Control: no-store     (malformed; never cache)
   Nostr-Timestamp is now set on EVERY response (per the did:nostr
   spec recommendation that clients correlate the resolver clock
   with the answer). Last-Modified stays 200-only — there's no
   "underlying resource" mtime for an error response.

3. Test used TEST_DATA_DIR='./data' which is also JSS's default
   data root — running the suite would clobber a developer's
   local pod data, and could race with other suites that use the
   shared helper's `./data`. Switched to a dedicated
   './test-data-well-known-did-nostr' that's isolated to this
   suite and removed in after().

4. Suite mutated process.env.DATA_ROOT but never restored it,
   leaking the test value into anything that ran after.
   Captures the original in `originalDataRoot` (including
   undefined → unset) and restores in after(). Mirrors the
   pattern in test/idp-change-password.test.js.

5. rebuildPubkeyIndex() called findById(accountId) without a
   try/catch. A single corrupt or unreadable account JSON would
   throw mid-loop and abort the entire index rebuild — turning
   this endpoint AND in-process NIP-98 local resolution into
   500s for every user until the bad file was found. Now wrapped
   per-account: log + skip, keep going for the rest.

Tests: existing 13 still pass; the 404/400 tests now also assert
the new Nostr-Timestamp + Cache-Control headers. Full suite 723/723.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/auth/did-nostr.js:133

  • resolveDidNostrToWebId() only accepts alsoKnownAs entries starting with https://, which prevents resolution of valid http:// WebIDs in non-production/dev deployments (the rest of the function already allows HTTP when NODE_ENV !== 'production'). Consider accepting both http:// and https:// here (or validating via validateExternalUrl after selection) so local/test environments can resolve WebIDs correctly.
    // Extract WebID from alsoKnownAs (array) or profile.webid or profile.sameAs
    let webId = null;

    if (Array.isArray(didDoc.alsoKnownAs) && didDoc.alsoKnownAs.length > 0) {
      // Find first HTTP(S) URL that looks like a WebID
      webId = didDoc.alsoKnownAs.find(aka =>
        typeof aka === 'string' && aka.startsWith('https://'));
    }

Comment thread src/idp/well-known-did-nostr.js Outdated
Comment on lines +177 to +187
function collectAuthenticationIds(profile) {
const out = new Set();
const auth = profile?.authentication;
const baseUrl = stripHashIfAny(profile?.['@id'] || profile?.id || '');
const list = Array.isArray(auth) ? auth : (auth ? [auth] : []);
for (const ent of list) {
let id;
if (typeof ent === 'string') id = ent;
else if (ent && typeof ent === 'object') id = ent['@id'] || ent.id;
if (id) out.add(absolutize(id, baseUrl));
}
One finding, real bug.

Profiles with a relative subject (`"@id": "#me"`) hit a silent
correctness gap in `collectAuthenticationIds()`: it re-derived the
base URL from `profile['@id']`, which `stripHashIfAny()` couldn't
turn into a usable absolute. Authentication entries stayed
relative, the later `authIds.has(vmId)` check could never match
even when the VM was authenticated, and the indexer would skip
the account — looking like "no local mapping" rather than a bug.

Fix: caller passes the already-validated absolute subject as the
base. The validation in rebuildPubkeyIndex (line 122) absolutizes
the subject against `account.webId`, so by the time we get here
we have a known-absolute string. Pass that down instead of
re-deriving.

Added a regression test that writes a profile with a relative
`@id` AND a relative `authentication` entry; if the base is
honored, the VM is published and the test passes. Test count
13 → 14 in module, 723 → 724 in full suite.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Comment on lines +100 to +103
}
if (!account?.podName || !account?.webId) continue;
const profilePath = path.join(dataRoot, account.podName, 'profile', 'card.jsonld');
let profile;
Comment thread src/idp/well-known-did-nostr.js Outdated
return reply
.header('Content-Type', contentType)
.header('Cache-Control', 'max-age=3600')
.header('Nostr-Timestamp', String(Math.floor(lastModifiedDate.getTime() / 1000)))
Comment thread src/auth/did-nostr.js
Comment on lines 79 to 85
* @param {string} pubkey - 64-char hex Nostr pubkey
* @param {string} resolverUrl - DID resolver base URL
* @param {string} [resolverUrl] - DID resolver base URL (without the
* trailing `/<pubkey>.json`). Defaults to the configured
* DEFAULT_DID_RESOLVER (nostr.social).
* @returns {Promise<string|null>} WebID URL or null
*/
export async function resolveDidNostrToWebId(pubkey, resolverUrl = DEFAULT_DID_RESOLVER) {
Comment on lines +66 to +70
before(async () => {
// IdP must be enabled — pod creation only writes an account
// record (the index this endpoint reads from) when the IdP is
// running. Pods without IdP are out of scope for this MVP.
//
Four findings, three code changes + a PR-description update.

1. Root-level pods were never indexed. The indexer hard-coded
   `<DATA_ROOT>/<podName>/profile/card.jsonld`, but single-user
   root pods store the profile at `<DATA_ROOT>/profile/card.jsonld`
   with no podName subdirectory — even though the seeded account
   record has `podName: 'me'`. So `dataRoot/me/profile/card.jsonld`
   would silently miss, and root-pod Nostr keys never made it into
   the index.

   Fix: derive the on-disk profile path from the account WebID's
   pathname instead of from `podName`. WebID pathname is
   `/profile/card.jsonld` for root and `/alice/profile/card.jsonld`
   for named — joining either with dataRoot yields the actual
   on-disk path. podName isn't even read anymore.

2. Nostr-Timestamp had inconsistent semantics across status codes:
   200 used the profile mtime, 400/404 used the current time.
   That defeats the spec-recommended "correlate the resolver's
   clock with the answer" purpose. Aligned: Nostr-Timestamp is
   ALWAYS the resolver's clock at answer time. Last-Modified
   stays 200-only and continues to track the underlying profile
   mtime (which is what conditional-GET clients actually want).

3. PR description still referenced "array of resolver URLs" and
   "same-origin sameAs check" that didn't match the shipped code.
   Updated PR body via REST patch (`gh pr edit` hit a deprecated-
   Projects-classic GraphQL error). Now matches what's in the
   tree: single resolverUrl, same-origin DID-doc shortcut.

4. Test suite only covered named-pod layouts. Added a regression
   test that:
     - writes a profile at <TEST_DATA_DIR>/profile/card.jsonld
       with a Nostr Multikey VM
     - synthesizes a matching account record with podName='me'
       (intentionally divergent from the on-disk layout)
     - injects it into _webid_index.json
     - hits the well-known endpoint and asserts the DID doc
       comes back with alsoKnownAs pointing at the root-pod WebID

Test count: 14 → 15 in module, 724 → 725 in full suite.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comment thread src/idp/well-known-did-nostr.js Outdated
let profilePath;
try {
const webIdUrl = new URL(account.webId);
profilePath = path.join(dataRoot, webIdUrl.pathname);
One finding (the others on this round were stale repeats of
already-fixed items from earlier passes).

The pass-6 commit derived the on-disk profile path from the
account WebID's pathname:

    profilePath = path.join(dataRoot, webIdUrl.pathname);

Copilot flagged that `webIdUrl.pathname` starts with `/`. The
specific claim — "path.join discards dataRoot" — is wrong (Node's
path.join keeps both segments, that's path.resolve's behavior).
But the underlying concern is real: if an account record ever
contained a webId whose pathname has `..` segments, the join +
read would happily traverse outside DATA_ROOT.

Operators are the only writers to account records, so this is
defense-in-depth rather than a remote-attacker vector. Still cheap
to harden:

  - strip leading `/` so the pathname is treated as a relative segment
  - resolve dataRoot and the joined path to absolute
  - assert the result is dataRootAbs OR starts with dataRootAbs + sep
  - skip + log loudly if not

Added a regression test that injects a malicious account record
with `webId: <baseUrl>/../../../etc/passwd#me`, hits the endpoint,
and asserts:
  - 404 for the unrelated query (request doesn't 500)
  - the evil account is silently skipped (no traversal occurred —
    if it had, fs.readFile would have been called on /etc/passwd
    and either thrown or returned binary, both of which would
    propagate as a 500 from the handler)

Test count: 15 → 16 in module, 725 → 726 in full suite.
@melvincarvalho
melvincarvalho requested a review from Copilot May 10, 2026 01:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comment thread src/auth/did-nostr.js
Comment on lines 184 to 187
export async function resolveDidNostrToWebId(pubkey, resolverUrl = DEFAULT_DID_RESOLVER) {
if (!pubkey || pubkey.length !== 64) {
return null;
}
Comment thread src/auth/did-nostr.js
}

cache.set(cacheKey, { webId: null, timestamp: Date.now() });
setCacheEntry(cacheKey, { webId: null, timestamp: Date.now() });
Two findings, both real.

1. Pubkey input validation was length-only. Since the pubkey
   comes off an attacker-controlled NIP-98 event and is
   interpolated into both the resolver URL path
   (`<resolverUrl>/<pubkey>.json`) and the cache key, characters
   like `/` would turn the resolution into an arbitrary-path
   fetch on the resolver origin and create misleading cache
   entries. e.g. pubkey = `<31 chars>/<32 chars>` of total
   length 64 would request `<resolverUrl>/<31 chars>/<32 chars>.json`
   — a different path entirely.

   Fix: require `/^[0-9a-f]{64}$/i.test(pubkey)`. Also
   normalize via toLowerCase once up front so cache and URL
   are case-stable.

   Regression test: drives a 64-char pubkey containing `/`
   against an unreachable resolver and asserts a clean null
   (no request, no cache entry). Plus too-short, too-long,
   non-string variants.

2. verifyWebIdBacklink returned `false` uniformly for every
   non-success case — both transient backlink-fetch failures
   (network / SSRF refusal / redirect cap / timeout / 5xx)
   AND the steady-state "fetched OK but no linkage" answer.
   The caller cached both as steady-state nulls (5-minute
   CACHE_TTL) instead of failures (1-minute FAILURE_CACHE_TTL).
   A WebID host having a 30-second blip pinned a null answer
   for 5 minutes.

   Fix: tri-state semantics for verifyWebIdBacklink.
     - `true` — linkage found
     - `false` — fetched OK, no linkage (verified absence)
     - throws TransientBacklinkError — fetch failed transiently
   Caller catches the new error and caches with `failureTtl: true`.
   5xx now also classifies as transient (it's "try again",
   not "no"). 4xx stays as verified absence.

   Removed the outer try/catch's swallow-all-into-false
   pattern; transient errors now surface to the caller for
   the right TTL classification.

Test count: 24 → 26 in did-nostr (+ 2 input-validation tests),
748 → 750 in full suite.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comment thread src/auth/did-nostr.js Outdated
Comment on lines +489 to +493
const vmCtrls = collectIds(vm.controller);
if (vmCtrls.length === 0) {
// No explicit controller: per CID v1 the VM's controller
// defaults to the VM's own `id` base. We accept that only
// if the profile subject is itself in expectedControllers
Comment on lines +115 to +134
try {
const stat = await fs.stat(profilePath);
mtimeMs = stat.mtimeMs;
// Size cap to bound per-rebuild memory/CPU. A user can write
// their own profile, and TTL-expired rebuilds can be triggered
// by attacker-driven NIP-98 traffic — without this an
// adversarially-large profile could pin the event loop on
// JSON.parse during the rebuild loop.
if (stat.size > MAX_PROFILE_BYTES) {
console.error(
`well-known-did-nostr: skipping account ${accountId} ` +
`— profile size ${stat.size} > ${MAX_PROFILE_BYTES} bytes`,
);
continue;
}
const text = await fs.readFile(profilePath, 'utf8');
profile = JSON.parse(text);
} catch {
continue; // unreadable / non-existent — skip
}
Two findings, both real.

1. checkCidVmBacklink had a permissive branch that accepted a
   verificationMethod with NO explicit `controller`, based only
   on the VM ID and profile subject sharing an origin. That
   made backlink looser than the resource-side LWS10-CID
   verifier (in src/auth/nostr.js) and the well-known indexer
   (in src/idp/well-known-did-nostr.js), neither of which
   accepts a controller-less VM. Result: did:nostr resolution
   could approve a binding the resource-side verifier would
   later reject — a key would "work" via DID resolution but
   401 on direct CID verification, surfacing as inconsistent
   binding rules across the stack.

   Fix: drop the origin-fallback branch. CID v1 is now
   uniformly strict — VM MUST declare an explicit `controller`
   AND that controller must intersect the profile's expected
   controller set. The resource-side verifier already enforced
   this; backlink now matches.

   Test added: a profile with a controller-less VM (everything
   else CID-correct) → backlink returns false. Documents the
   strictness alignment so future regressions are caught.

2. Profile read/parse failures in rebuildPubkeyIndex were
   silently swallowed (`catch { continue; }`). When a local
   account's profile is unreadable or malformed JSON, the
   well-known endpoint returns 404 with zero log output —
   "why isn't my pubkey publishing?" was undebuggable
   without grepping silence.

   Fix: rate-limited per-account log on stat/read/parse
   failures. First occurrence per account per hour fires a
   `console.error` with accountId + profilePath + error code/
   message. Tracker is bounded (10k entries) so it can't grow
   without limit. Cleared by `_resetIndexForTests`.

   Test added: synthesize an account with a malformed JSON
   profile, hit the well-known endpoint, capture
   console.error, assert the diagnostic mentions both the
   accountId and the profile path.

Test count: 26 → 27 in did-nostr (+ controller-less reject),
27 → 28 in well-known module (+ profile-failure log), 750 → 752
in full suite.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comment thread test/did-nostr.test.js
let server;
let port;
let mode = 'attack'; // 'attack' | 'legit'
let attackPubkey;
One finding, cleanup.

The "Same-origin shortcut removed" describe block set up a local
HTTP server (with `http`, `server`, `port`, `mode` vars and a
listen/close lifecycle) but the tests inside it call
`_checkCidVmBacklinkForTests` with in-memory objects and never
hit the server. The scaffolding was a leftover from an earlier
attempt to drive the live resolver against loopback —
abandoned once I confirmed validateExternalUrl
unconditionally blocks 127.0.0.1.

Removed: http import, server creation, handler bodies, listen/
close in before/after, port plumbing. Kept the on-curve key
derivation (still needed by the in-memory tests) and the
clearCache() reset.

Net: same 3 tests, less setup, faster, no port allocation.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Comment thread src/idp/well-known-did-nostr.js Outdated
// In-memory pubkey → accountId index. Built lazily from disk; rebuilt
// when the TTL expires. Real production wants a write-path hook on
// LDP PUT/PATCH so updates are immediate; that's filed as a follow-up.
let pubkeyIndex = null; // Map<pubkeyHex, accountId>
Comment thread src/idp/well-known-did-nostr.js Outdated
Comment on lines +317 to +326
const entry = pubkeyIndex.get(lower);
if (!entry) return null;
// findById can throw on parse/permission errors. Treating it as a
// cache miss keeps DID-doc requests AND the in-process
// resolveDidNostrLocally call in src/auth/nostr.js from turning
// into 500s when a single account file is corrupt.
let account;
try {
account = await findById(entry.accountId);
} catch (err) {
Comment thread src/auth/did-nostr.js Outdated
Comment on lines +356 to +357
if (backlinkRes.status < 200 || backlinkRes.status >= 300) {
// Client error or redirect that didn't resolve — verified absence.
Three findings, all real.

1. Stale type comment on `pubkeyIndex`. The Map values are
   `{ accountId, mtimeMs }` (since pass-2), not bare accountIds.
   Updated to reflect the current shape (and the new addition
   below).

2. `findAccountByNostrPubkey()` re-read the account JSON from
   disk on EVERY lookup via `findById(accountId)`. That's the
   NIP-98 auth hot path (every signed request via
   `resolveDidNostrLocally`) AND every DID-doc request. The
   webId we need is already known at index-build time — there's
   no reason to re-fetch it per request.

   Fix: store `webId` directly on the index entry (now
   `{ accountId, webId, mtimeMs }`). The lookup hot path now
   answers from RAM with zero filesystem I/O. The rebuild
   loop still uses `findById` (it's the only place that needs
   the full account record), and that's already wrapped in
   per-account try/catch so a corrupt file can't break the
   rebuild.

3. `verifyWebIdBacklink()` only treated 5xx as transient.
   Two more 4xx codes are conventionally transient and should
   match:
     - 408 Request Timeout — server timed itself out, retry
     - 429 Too Many Requests — rate-limited, definitely retry
   Pre-fix, both got pinned as "verified absence" with the
   full 5-minute CACHE_TTL. Now classified as transient and
   re-tried after the 1-minute FAILURE_CACHE_TTL.

   Other 4xx (404, 410, etc.) stay as verified absence — the
   host answered authoritatively that the resource doesn't
   exist; caching that for the steady-state TTL is correct.

No new tests — the existing transient-vs-absence tests cover
the classification logic; 408/429 are the same shape as 5xx.
The webId-on-index change is exercised by every existing
well-known integration test (DID-doc generation reads
`account.webId` from the lookup result).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Comment thread src/server.js Outdated
// POST — wrong for this namespace and confusing to CORS
// preflights.
const optionsForReadOnlyNamespace = async (request, reply) => reply.code(204)
.header('Allow', 'GET, HEAD, OPTIONS')
Comment on lines +426 to +441
before(async () => {
const port = await getAvailablePort();
baseUrl = `http://${TEST_HOST}:${port}`;
server = createServer({
logger: false,
root: TEST_DATA_DIR + '-noidp',
idp: false, // <-- the point of the test
forceCloseConnections: true,
});
await server.listen({ port, host: TEST_HOST });
});

after(async () => {
await server.close();
await fs.remove(TEST_DATA_DIR + '-noidp');
});
Comment thread test/did-nostr.test.js Outdated
Comment on lines +349 to +356
// entries — that's a slow test. Instead drive +50 past the cap
// by using a very low CACHE_MAX_ENTRIES would be ideal, but
// we can't mutate the const from the test. Compromise: do a
// bounded check that the cache size never exceeds the cap,
// using an unreachable URL so each call resolves quickly.
// Skip this on CI where it'd be too slow — the LRU logic
// itself is mechanical (set + check size + delete oldest)
// and proven by the smaller-scale assertion below.
Three findings.

1. The OPTIONS handler for /.well-known/did/nostr/* returned a
   bare 204 with only `Allow: GET, HEAD, OPTIONS`. CORS preflights
   from a browser at a different origin would refuse to follow up
   because no Access-Control-* headers were set — the read-only
   namespace was effectively non-CORS-able.

   Fix: call getCorsHeaders(request.headers.origin) for the full
   CORS header set, then override `Access-Control-Allow-Methods`
   with the restricted GET/HEAD/OPTIONS list. ACAO/ACAH/ACAC/
   max-age all match what the rest of the server returns.

   Test now also asserts:
     - access-control-allow-methods has GET, HEAD, OPTIONS
     - access-control-allow-methods does NOT have PUT (or any
       write method)
     - access-control-allow-origin and -headers are present
   Driven with `Origin: https://other.example` so the
   cross-origin behavior is explicit.

2. The non-IdP describe block called `createServer` (which
   mutates process.env.DATA_ROOT) but didn't save/restore the
   original value in after(). Subsequent tests in the same
   process saw the test's DATA_ROOT — exactly the kind of
   cross-suite leakage that surfaced in the full-suite run as
   a flaky failure in nostr-cid-vm.test.js (which passed in
   isolation).

   Fix: capture `originalDataRoot` at suite scope, restore
   (or delete if originally undefined) in after(). Mirrors the
   existing pattern in the first describe block.

3. The "evicts oldest entries past the LRU cap" test had a stale
   "Skip this on CI where it'd be too slow" comment that didn't
   match the test (the test isn't skipped and only adds 5
   entries). Renamed to
   "cache stays at-or-under CACHE_MAX_ENTRIES after a burst of
   misses" and trimmed the comment to match what's actually
   asserted.

Test count: same 752 (the OPTIONS test was reshaped, not
duplicated).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

auth: serve /.well-known/did/nostr/<pubkey>.json so the pod is its own DID resolver

2 participants