jmap/rss/remotestorage/terminal: protocol-conformance fixes

melvincarvalho · melvincarvalho · commit e54aba275352 · 2026-07-11T10:34:04.000+02:00
Audit correctness findings:
- jmap Email/set create now echoes server-set props per RFC 8621 §4.6
  (blobId = sha256 of the stored JSON, threadId = id for degenerate
  one-message threads, size = byte length) — a real client caches these
  instead of re-fetching.
- rss Atom output now emits the required feed-level &lt;author&gt; (RFC 4287
  §4.1.1; W3C Feed Validator rejected it before). RSS 2.0 path untouched.
- remotestorage GET no longer forwards the host content-length over a
  fetch-decoded stream — matches the webdav sibling (let fastify frame).
- terminal query-token compare is now constant-time (sha256 +
  timingSafeEqual), no length early-exit leak.
diff --git a/jmap/README.md b/jmap/README.md
@@ -126,7 +126,14 @@ The stored resource is readable as a plain pod document too — `GET
   — the same wall as micropub's media endpoint. See Findings #5.
 - **No threads** (`Thread/get`), no `Email/import`, no
   `EmailSubmission/*` — this stores and organizes mail; it does not send
-  SMTP.
+  SMTP. `Email/set` create still returns the RFC 8621 §4.6 server-set
+  properties (`id`, `blobId`, `threadId`, `size`) so a client can cache them
+  without a re-fetch, but two are **degenerate and documented**: `threadId`
+  equals the message id (every message is its own single-message thread,
+  since there is no `Thread/get`), and `blobId` is a SHA-256 hash of the
+  stored message JSON (there is no separate blob store — the JSON resource
+  *is* the blob; see also Findings #5). `size` is the honest byte length of
+  that stored representation.
 - **One mailbox per message** (`maxMailboxesPerEmail: 1`, advertised): a
   message is one resource in one container, so JMAP's "email in several
   mailboxes at once" doesn't map to LDP containment.
diff --git a/jmap/plugin.js b/jmap/plugin.js
@@ -370,18 +370,35 @@ export async function activate(api) {
         continue;
       }
       const id = newEmailId();
-      const email = { id, mailboxIds: { [mb.id]: true }, keywords: {} };
+      // threadId is degenerate: this server has no Thread/get, so every
+      // message is its own single-message thread (threadId = the message id).
+      // Persisting it keeps Email/get consistent with the create response.
+      const email = { id, threadId: id, mailboxIds: { [mb.id]: true }, keywords: {} };
       for (const p of EMAIL_PROPS) if (spec[p] !== undefined) email[p] = spec[p];
       email.receivedAt = typeof spec.receivedAt === 'string' ? spec.receivedAt : new Date().toISOString();
       email.preview = previewOf(spec);
+      // The stored representation is exactly what writeEmail PUTs — its byte
+      // length is `size`, and its content hash is a stable blobId (there is no
+      // separate blob store; the message JSON IS the blob).
+      const stored = JSON.stringify(email, null, 2);
       const put = await writeEmail(ctx.podPath, mb, id, email, ctx.auth);
       if (put.status === 401 || put.status === 403) {
         notCreated[cid] = { type: 'forbidden' };
       } else if (!(put.ok || put.status === 204)) {
         notCreated[cid] = { type: 'serverFail', description: `pod storage rejected the message (${put.status})` };
       } else {
-        // Server-set properties back to the client, per RFC 8620 §5.3.
-        created[cid] = { id, receivedAt: email.receivedAt, preview: email.preview };
+        // Echo EVERY server-set property the client didn't supply, per
+        // RFC 8620 §5.3 / RFC 8621 §4.6 — a client caches these from the
+        // response instead of re-fetching. blobId/threadId are degenerate but
+        // stable and honest (see README Findings): no blob store, no threads.
+        created[cid] = {
+          id,
+          blobId: crypto.createHash('sha256').update(stored).digest('base64url').slice(0, 24),
+          threadId: id,
+          size: Buffer.byteLength(stored),
+          receivedAt: email.receivedAt,
+          preview: email.preview,
+        };
       }
     }
 
diff --git a/jmap/test.js b/jmap/test.js
@@ -182,6 +182,13 @@ describe('jmap plugin', () => {
     assert.ok(args.created?.m1?.id, `not created: ${JSON.stringify(args)}`);
     assert.ok(args.created.m1.receivedAt, 'no server-set receivedAt');
     assert.match(args.created.m1.preview, /Hello over a pod/);
+    // RFC 8621 §4.6: the create response echoes every server-set property so
+    // the client caches them instead of re-fetching (blobId/threadId/size).
+    assert.ok(args.created.m1.blobId, 'no server-set blobId');
+    assert.strictEqual(args.created.m1.threadId, args.created.m1.id,
+      'threadId is the degenerate per-message id (one-message threads)');
+    assert.ok(Number.isInteger(args.created.m1.size) && args.created.m1.size > 0,
+      `no server-set size: ${JSON.stringify(args.created.m1)}`);
     assert.notStrictEqual(args.oldState, args.newState, 'state string did not change on create');
     emailId = args.created.m1.id;
 
diff --git a/remotestorage/plugin.js b/remotestorage/plugin.js
@@ -244,7 +244,11 @@ export async function activate(api) {
     if (relayRefusal(res, reply)) return reply;
     if (res.status === 404) return jsonReply(reply, 404, { error: 'Not found' });
     cors(reply).code(res.status).header('cache-control', 'no-cache');
-    for (const h of ['content-type', 'etag', 'content-range', 'content-length']) {
+    // content-length deliberately NOT forwarded: the body is streamed via
+    // Readable.fromWeb(res.body) below, already content-decoded by fetch, so
+    // the host's length can be wrong if it ever content-encodes. Let fastify
+    // frame the stream itself — matches the webdav/ sibling (webdav GET).
+    for (const h of ['content-type', 'etag', 'content-range']) {
       const v = res.headers.get(h);
       if (v) reply.header(h, v);
     }
diff --git a/remotestorage/test.js b/remotestorage/test.js
@@ -179,6 +179,11 @@ describe('remotestorage plugin', () => {
     assert.strictEqual(get.headers.get('etag'), firstEtag,
       'GET ETag equals the ETag PUT reported (host ETag passed through both ways)');
     assert.match(get.headers.get('content-type') || '', /text\/plain/);
+    // The streamed GET body is fetch-decoded, so the plugin must NOT forward a
+    // host content-length that could be wrong under content-encoding — it lets
+    // fastify frame the stream (matches the webdav/ sibling).
+    assert.strictEqual(get.headers.get('content-length'), null,
+      'GET must not forward a host content-length over the fetch-decoded stream');
 
     // The resource is created in the pod's own namespace — visible over LDP.
     const ldp = await fetch(`${base}/alice/remotestorage/documents/notes/todo.txt`, {
diff --git a/rss/plugin.js b/rss/plugin.js
@@ -165,13 +165,16 @@ function mapEntry({ url, path }, contentType, bodyText, listingModified) {
 
 // ----------------------------------------------------------- feed rendering
 
-function renderAtom({ feedTitle, feedId, selfUrl, updated, entries }) {
+function renderAtom({ feedTitle, feedAuthor, feedId, selfUrl, updated, entries }) {
   const parts = [
     '<?xml version="1.0" encoding="utf-8"?>',
     '<feed xmlns="http://www.w3.org/2005/Atom">',
     `  <title>${xmlEscape(feedTitle)}</title>`,
     `  <id>${xmlEscape(feedId)}</id>`,
     `  <updated>${xmlEscape(updated)}</updated>`,
+    // RFC 4287 §4.1.1: a feed-level author satisfies the "≥1 author" rule for
+    // every entry at once (RSS 2.0 has no equivalent requirement).
+    `  <author><name>${xmlEscape(feedAuthor)}</name></author>`,
     `  <link rel="self" type="application/atom+xml" href="${xmlEscape(selfUrl)}"/>`,
     `  <link rel="alternate" href="${xmlEscape(feedId)}"/>`,
   ];
@@ -233,6 +236,9 @@ export async function activate(api) {
   const defaultContainer = api.config.defaultContainer || null;
   const maxItems = api.config.maxItems ?? DEFAULT_MAX_ITEMS;
   const feedTitle = api.config.title || 'Solid pod feed';
+  // RFC 4287 §4.1.1 requires an atom:feed to carry ≥1 atom:author. Derive the
+  // name from config.author, else the feed title, else a sensible default.
+  const feedAuthor = api.config.author || feedTitle || 'Solid pod feed';
 
   /** Which container path this request addresses ('/alice/blog/'), or null. */
   function resolveContainer(request) {
@@ -335,6 +341,7 @@ export async function activate(api) {
     const updated = result.entries.length ? result.entries[0].updated : new Date().toISOString();
     const model = {
       feedTitle,
+      feedAuthor,
       feedId: result.containerUrl,
       selfUrl: baseUrl + request.raw.url,
       updated,
diff --git a/rss/test.js b/rss/test.js
@@ -141,6 +141,10 @@ describe('rss feed plugin', () => {
     assert.ok(xml.includes(`<id>${base}/alice/blog/</id>`), xml);
     assert.ok(xml.includes('<link rel="self"'), xml);
     assert.match(xml, /<updated>\d{4}-\d\d-\d\dT/, 'feed needs an <updated>');
+    // RFC 4287 §4.1.1: an atom:feed must carry ≥1 atom:author. Here it is
+    // derived from the feed title (no config.author) and XML-escaped.
+    assert.ok(xml.includes('<author><name>Alice&apos;s journal</name></author>'),
+      'atom feed needs a feed-level <author><name>');
 
     // The three titles, drawn from schema:headline, schema:name, dcterms:title.
     for (const t of ['A quiet Tuesday', 'First light', 'Storm over the bay']) {
diff --git a/terminal/plugin.js b/terminal/plugin.js
@@ -22,6 +22,7 @@
 // Attribution: adapted from JavaScriptSolidServer/src/terminal/index.js.
 
 import { spawn } from 'node:child_process';
+import { createHash, timingSafeEqual } from 'node:crypto';
 
 // Wire protocol (matches core): raw text/binary chunks are shell stdio;
 // JSON envelopes carry lifecycle signals.
@@ -42,6 +43,9 @@ export async function activate(api) {
   //     (browsers cannot set Authorization on a WebSocket handshake).
   const allowAgents = Array.isArray(cfg.allowAgents) ? cfg.allowAgents.filter(Boolean) : null;
   const token = typeof cfg.token === 'string' && cfg.token ? cfg.token : null;
+  // Pre-hash the expected token so the compare is constant-time over
+  // fixed-width digests — no early-exit that would leak the token length.
+  const tokenDigest = token ? createHash('sha256').update(token).digest() : null;
   if ((!allowAgents || allowAgents.length === 0) && !token) {
     throw new Error(
       'terminal: refusing to boot an open shell — set config.allowAgents ' +
@@ -71,14 +75,14 @@ export async function activate(api) {
   // Verify the handshake before spawning anything. Returns an agent label
   // string when authorized, or null.
   async function authorize(request) {
-    // Browser fallback: a shared token in the query string.
+    // Browser fallback: a shared token in the query string. Constant-time
+    // over sha256 digests so neither the token nor its LENGTH leaks via an
+    // early-exit length check (the metrics/ pattern).
     if (token) {
       const qToken = request.query?.token;
-      if (typeof qToken === 'string' && qToken.length === token.length) {
-        // constant-time-ish compare
-        let diff = 0;
-        for (let i = 0; i < token.length; i++) diff |= qToken.charCodeAt(i) ^ token.charCodeAt(i);
-        if (diff === 0) return 'token';
+      if (typeof qToken === 'string') {
+        const presented = createHash('sha256').update(qToken).digest();
+        if (timingSafeEqual(presented, tokenDigest)) return 'token';
       }
     }
     // Primary: a verified agent from a Bearer/DPoP/NIP-98/… credential.
diff --git a/terminal/test.js b/terminal/test.js
@@ -62,6 +62,11 @@ describe('terminal plugin', () => {
     const bad = await open(`${jss.wsBase}/terminal?token=nope`);
     assert.strictEqual(bad.opened, false, 'wrong token must not open a usable session');
 
+    // A wrong token of the SAME length as the real one is also refused — the
+    // compare is constant-time over sha256 digests, no length early-exit.
+    const sameLen = await open(`${jss.wsBase}/terminal?token=${'x'.repeat(TOKEN.length)}`);
+    assert.strictEqual(sameLen.opened, false, 'same-length wrong token must be refused');
+
     const none = await open(`${jss.wsBase}/terminal`);
     assert.strictEqual(none.opened, false, 'missing token must be refused');
   });
