auth: serve /.well-known/did/nostr/<pubkey>.json (#407) - #408
Conversation
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).
There was a problem hiding this comment.
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:nostrDID docs from/.well-known/did/nostr/:pubkey(.json|.jsonld)?, backed by a lazy pubkey→account index. - Registers the new route in
src/server.jsahead of theGET /*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 emitalsoKnownAsusing that scheme, so resolution will silently fail. Consider acceptinghttp://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.
| * 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; |
| 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; | ||
| } |
| /** | ||
| * 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) { |
| // 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'); |
| } | ||
|
|
||
| async function readJsonOrEmpty(file) { | ||
| try { return await fs.readJson(file); } catch { return null; } |
| 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)' }); |
| 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); |
| 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.
| // on this same pod. | ||
| const localWebId = await resolveDidNostrLocally(event.pubkey); | ||
| if (localWebId) { | ||
| return { webId: localWebId, error: null }; |
| 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 |
| // 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 }); | ||
| } |
| * 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. |
| * 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) { |
| 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 | ||
|
|
| 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.
There was a problem hiding this comment.
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 singleresolverUrlstring. 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 viaresolveDidNostrLocallyinverifyNostrAuth).
* 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) {
| const wellKnownDidNostr = buildWellKnownDidNostrHandler(); | ||
| fastify.get('/.well-known/did/nostr/:pubkeyAndExt', wellKnownDidNostr); |
| 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(); |
| // 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.
There was a problem hiding this comment.
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
resolveDidNostrToWebIdcurrently only considersalsoKnownAsentries that start withhttps://, which prevents DID docs that legitimately point athttp://WebIDs from resolving in non-production/dev setups (even though the SSRF validator allows HTTP whenNODE_ENV !== 'production'). Consider accepting bothhttp://andhttps://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 singleresolverUrlstring. 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) {
| 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 }; | ||
|
|
| 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' }); |
| import { extractNostrPubkeysFromProfile } from '../src/auth/nostr.js'; | ||
|
|
||
| const TEST_HOST = '127.0.0.1'; | ||
| const TEST_DATA_DIR = './data'; |
| 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 |
| // 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.
There was a problem hiding this comment.
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 acceptsalsoKnownAsentries starting withhttps://, which prevents resolution of validhttp://WebIDs in non-production/dev deployments (the rest of the function already allows HTTP whenNODE_ENV !== 'production'). Consider accepting bothhttp://andhttps://here (or validating viavalidateExternalUrlafter 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://'));
}
| 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.
| } | ||
| if (!account?.podName || !account?.webId) continue; | ||
| const profilePath = path.join(dataRoot, account.podName, 'profile', 'card.jsonld'); | ||
| let profile; |
| return reply | ||
| .header('Content-Type', contentType) | ||
| .header('Cache-Control', 'max-age=3600') | ||
| .header('Nostr-Timestamp', String(Math.floor(lastModifiedDate.getTime() / 1000))) |
| * @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) { |
| 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.
| 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.
| export async function resolveDidNostrToWebId(pubkey, resolverUrl = DEFAULT_DID_RESOLVER) { | ||
| if (!pubkey || pubkey.length !== 64) { | ||
| return null; | ||
| } |
| } | ||
|
|
||
| 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.
| 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 |
| 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.
| 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.
| // 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> |
| 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) { |
| 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).
| // POST — wrong for this namespace and confusing to CORS | ||
| // preflights. | ||
| const optionsForReadOnlyNamespace = async (request, reply) => reply.code(204) | ||
| .header('Allow', 'GET, HEAD, OPTIONS') |
| 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'); | ||
| }); |
| // 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).
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 forGET /.well-known/did/nostr/:pubkeyAndExt. Accepts<pubkey>.json,<pubkey>.jsonld, and bare<pubkey>. Generates a CID-shaped DID doc withalsoKnownAs: [<webId>]derived from the account record.Cache-Control: max-age=3600,Nostr-Timestamp(resolver clock at answer time),Last-Modified(profile mtime).Cache-Control: max-age=60,Nostr-Timestamp. Short TTL so newly added keys surface fast.Cache-Control: no-store,Nostr-Timestamp.application/did+jsonfor.jsonand bare;application/did+ld+jsonfor the.jsonldalias.<DATA_ROOT>/.idp/accounts/_webid_index.jsonby 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.jsonldwork 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@idmust equalaccount.webId, VM controller must be in the profile's expected controller set, VM id must be inauthentication. 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 wildcardGET /*so it actually matches). Asyncfastify.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 ofidpEnabled(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— exportsextractNostrPubkeysFromProfile()for the index.verifyNostrAuthnow resolves Nostr pubkeys against the in-process local index first (no HTTP, no SSRF surface), gated onrequest.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 insrc/auth/nostr-keys.jsto avoid a circular dependency between the verifier and the publisher.src/auth/did-nostr.js—resolveDidNostrToWebId()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 v1verificationMethodreferenced fromauthentication(the JSS-native shape) OR a classicowl:sameAstodid:nostr:<pubkey>. SSRF + redirect hardened:fetchWithRedirectGuardre-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
@idprofiles, auth-membership filter, JWK BIP-340 even-y validation, path containment).did-nostr(resolver, CID-VM backlink, SSRF/redirect/size hardening, LRU cap, cross-resolver cache isolation, real-world resolver round-trip).Known gaps (deferred follow-ups)
didResolverdiscovery is mentioned in the spec but not implemented hereRefs
/.well-known/did/nostr/<pubkey>.jsonpath and required headers