fix: keep a stranger on the relay, where the protocols already refuse them (#71)

`/ablage/sync/1.0.0` is gated by the admission dialog, and for a while that was
believed to cover everything. It does not: bitswap is a second protocol on the
same node and serves any block it holds to anyone who names the address. That
is #43, and it was measured rather than argued.

What could not be settled at the time was why. `@helia/bitswap` registers its
handler with `runOnLimitedConnection: false`, so a relayed connection should
already have been refused - and the first reading said the leak happened over
one anyway, concluding it was older than DCUtR.

That reading was wrong, and the `holePunch: false` switch from #66 is what
settles it. With no DCUtR and no `/webrtc` address the circuit is the only path
two peers can have, and an unadmitted read over it times out. So the earlier
`limited: true` observation was a second, unlimited connection standing beside
the one being reported.

The hole is the upgrade, not the circuit. A stranger is refused for as long as
they are stuck on the relay and served the moment DCUtR gets them off it,
because a direct connection is unlimited and every protocol on the node becomes
reachable at once.

So `peer.js` closes a direct connection to a peer that was neither scanned nor
admitted. Narrow on purpose: it inherits every gate the relayed path already
has, instead of adding a second list of protocols to keep in step.

Two things had to be narrowed before it was right, and both were the same
mistake - a guard aimed at strangers hitting our own people first.

It closed the relay connection. The relay is unlimited, and a peer this node
has no relationship with by construction. Discovery went with it and neither
peer heard the other at all. Only `/webrtc` is a link to another browser;
everything else is infrastructure.

It closed scanned peers. `acceptOffer` and `acceptAnswer` recorded the scan
*after* the call that establishes the connection, so `connection:open` fired
first and a connection that existed because somebody scanned a code looked
exactly like a stranger's. Ten QR specs failed there. The scan is recorded
before the session call now, still only after `decodePayload` verifies the
signature - an unverifiable code was never consent and still is not.

Three specs, and the mutation check shows they test three things: disable the
guard and only the third fails.

  a stranger over a direct connection      reads      - the protocol hole, unchanged
  a stranger with only a circuit           reads nothing
  a stranger with the hole punch available never gets the direct connection

171 unit tests, 443 browser tests green.

Withdrawal is split out as #70: somebody admitted once keeps every address they
saw, and taking that back needs the blocks to be useless without a key.

Author: NiKrause

package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "ablage",
-  "version": "0.1.23",
+  "version": "0.1.24",
   "private": true,
   "type": "module",
   "description": "A folder that stays the same on two devices. No account, nothing in the middle.",

package.json

@@ -478,6 +478,16 @@ const letGo = new Set()
 /** Who may write into this folder. The QR scan is the only automatic yes. */
 const admitted = admission({ key: scoped('ablage.admitted', shareId) })
 
+/**
+ * Who was let in during this session, remembered or not.
+ *
+ * Read by the guard in `peer.js`, which closes a *direct* connection to anyone
+ * this device has no relationship with - see #43. Kept beside the stored set
+ * rather than inside it: this one is deliberately forgotten on reload, because
+ * a one-time yes was never meant to outlive the session.
+ */
+const letIn = new Set()
+
 /** This folder's id and name, for the message a switch sends. */
 let folder = { id: null, name: null }
 
@@ -903,6 +913,14 @@ async function render () {
  *   replaces that peer's provider rather than accumulating one per attempt.
  */
 function attach (stream, peerId) {
+  // **Admitted for this session, whether or not the box was ticked.**
+  //
+  // `admitted.remembered()` only knows the peers somebody chose to keep. A
+  // one-time yes is just as much a yes, and the guard in `peer.js` has to hear
+  // about it - otherwise the direct connection to somebody who was let in a
+  // moment ago gets closed as a stranger's.
+  letIn.add(String(peerId))
+
   const send = message => stream.send(encode(JSON.stringify(message)))
 
   // Its own binding, read by its own loop below. Reading the shared one was
@@ -1031,6 +1049,10 @@ async function start () {
     // during the introduction's check and heard nobody for ever, which is what
     // "connected, and no peers anywhere" turned out to mean.
     relayBootstrapAddrs: startupRelays(localStore, RELAY_ADDRESSES_KEY),
+
+    // Both halves of "does this peer have a relationship with us": the ones
+    // somebody chose to keep, and the ones let in since this page loaded.
+    admitted: id => letIn.has(String(id)) || admitted.remembered(String(id)),
     onSyncStream: (stream, peerId) => {
       // `peer.arrivedByScan`, not the address. A peer that hole-punched out of
       // the relay has a `/webrtc/p2p/<id>` address with no circuit in it -

src/app/main.js

@@ -174,7 +174,7 @@ window.__ablage = {
    * step between "they see each other" and "they sync", and it is where the
    * report of two devices that find each other and do nothing points.
    */
-  meetAndDial: async ({ holePunch = true } = {}) => {
+  meetAndDial: async ({ holePunch = true, admitAll = false } = {}) => {
     const { createPeer } = await import('./peer.js')
 
     const heard = new Set()
@@ -192,7 +192,13 @@ window.__ablage = {
       // With this off there is no DCUtR and no `/webrtc` address, so the
       // circuit is the only path that will ever exist - which is what two
       // phones on mobile data have.
-      holePunch
+      holePunch,
+
+      // A spec measuring *transport* - does DCUtR get these two off the relay -
+      // has to say so, because a node now closes a direct connection to anyone
+      // it has no relationship with. Saying it out loud beats a spec that
+      // silently measures the guard instead of the hole punch.
+      admitted: () => admitAll
     })
 
     peer.node.addEventListener('peer:discovery', event => heard.add(event.detail.id.toString()))
@@ -263,12 +269,22 @@ window.__ablage = {
    * comes back with the bytes, an unadmitted peer can read a file whose address
    * it knows.
    */
-  bitswapAcrossTheRelay: async () => {
+  bitswapAcrossTheRelay: async ({ holePunch = true, admitAll = false } = {}) => {
     const { createPeer } = await import('./peer.js')
     const { createContent } = await import('./content.js')
 
     const start = async () => {
-      const peer = await createPeer({ relayOptIn: true, relayBootstrapAddrs: await relayAddresses() })
+      // With the hole punch off there is no DCUtR and no `/webrtc` address, so
+      // the circuit is the only path the two can ever have. That is what makes
+      // "bitswap refuses limited connections" a claim this harness can test
+      // rather than hope for: with a direct path available it is never put to
+      // the question.
+      const peer = await createPeer({
+        relayOptIn: true,
+        relayBootstrapAddrs: await relayAddresses(),
+        holePunch,
+        admitted: () => admitAll
+      })
       const heard = new Set()
 
       peer.node.addEventListener('peer:discovery', event => heard.add(event.detail.id.toString()))

src/harness.js

@@ -9,7 +9,7 @@ import { identify, identifyPush } from '@libp2p/identify'
 import { ping } from '@libp2p/ping'
 import { webRTC } from '@libp2p/webrtc'
 import { webSockets } from '@libp2p/websockets'
-import { decodePayload, QRSession, QR_TYPE_OFFER, webRTCQR } from '@le-space/libp2p-webrtc-qr'
+import { decodePayload, QRSession, QR_TYPE_ANSWER, QR_TYPE_OFFER, webRTCQR } from '@le-space/libp2p-webrtc-qr'
 import { createLibp2p } from 'libp2p'
 
 import { denyDial, relayBootstrapList } from './relay-policy.js'
@@ -146,7 +146,20 @@ export async function createPeer ({
    * So this exists to take the direct path away: no DCUtR to arrange one, and
    * no `/webrtc` address to arrange it to.
    */
-  holePunch = true
+  holePunch = true,
+
+  /**
+   * Does this peer have a relationship with this device?
+   *
+   * Consulted before a *direct* connection to them is allowed to stand. The
+   * default answers no to everybody, which is the conservative half: a node
+   * whose caller wires nothing up keeps strangers on the relay, where the
+   * protocols that matter already refuse them.
+   *
+   * Scanning a code is the other way in and is not asked about here - the scan
+   * was the consent, and `scanned` records the act.
+   */
+  admitted = () => false
 } = {}) {
   let session = null
 
@@ -316,6 +329,90 @@ export async function createPeer ({
     }
   })
 
+  /**
+   * Record a scanned peer *before* the connection to them exists.
+   *
+   * `session.acceptOffer` and `session.acceptAnswer` are what establish the
+   * WebRTC connection, so the guard below sees `connection:open` while they are
+   * still running. Recording the scan afterwards left a window in which a
+   * connection that exists *because* somebody scanned a code looked exactly
+   * like a stranger's and was closed - ten QR specs failed there, which is how
+   * the ordering was found.
+   *
+   * Still only after verification. `decodePayload` checks the signature, and an
+   * offer that does not decode is not consent to anything - that property is
+   * older than the guard and is kept.
+   *
+   * @param {string} payload
+   * @param {unknown} type
+   */
+  async function noteScan (payload, type) {
+    try {
+      const { peerId } = await decodePayload(payload, type)
+
+      scanned.add(String(peerId))
+    } catch {
+      // Unverifiable, so not consent. The peer is asked about rather than
+      // admitted, which is the direction to fail in.
+    }
+  }
+
+  /**
+   * **Keep a stranger on the relay.**
+   *
+   * `/ablage/sync/1.0.0` is gated by the admission dialog, and for a while that
+   * was believed to cover everything. It does not: bitswap is a second protocol
+   * on the same node, and it serves any block it holds to anyone who names the
+   * address. Measured, and it is #43.
+   *
+   * What was *not* known until it could be measured in isolation is that
+   * bitswap already refuses a relayed connection - `runOnLimitedConnection:
+   * false` is its default and it works. Over a circuit that is the only path
+   * two peers have, an unadmitted read times out. The leak is not the circuit;
+   * it is the moment DCUtR gets them off it, because a direct connection is
+   * unlimited and every protocol on the node becomes reachable at once.
+   *
+   * So this closes the direct connection rather than gating each protocol on
+   * it. Narrow, and it inherits every gate the relayed path already has instead
+   * of adding a new list to keep in step.
+   *
+   * **What it does not fix.** Somebody admitted once keeps what they saw and
+   * can fetch those blocks again afterwards; taking that back needs the bytes
+   * to be useless without a key, which is a different change.
+   */
+  node.addEventListener('connection:open', event => {
+    const connection = event.detail
+
+    // Relayed, and therefore already refused by the protocols that matter.
+    if (connection?.limits != null) return
+
+    /**
+     * **Only a link to another browser, never one to infrastructure.**
+     *
+     * The first version of this closed anything unlimited, and the relay is
+     * exactly that: a plain WebSocket this node dialled, to a peer it has no
+     * relationship with by construction. Closing it took discovery down with
+     * it and the two peers never heard each other at all.
+     *
+     * Two browsers can only ever reach each other directly over WebRTC - a
+     * hole punch reads `/p2p-circuit/webrtc/p2p/…`, a scanned session reads
+     * `/webrtc/p2p/…`. Anything without `/webrtc` in it is a server, and this
+     * is not about servers.
+     */
+    if (!String(connection.remoteAddr ?? '').includes('/webrtc')) return
+
+    const id = String(connection.remotePeer)
+
+    // The scan was the consent, and the QR path is direct by construction.
+    if (scanned.has(id)) return
+    if (admitted(id)) return
+
+    connection.close().catch(() => {
+      // A connection that will not close is one libp2p is already tearing
+      // down. Nothing better to do here than let it.
+    })
+  })
+
   session = new QRSession(node, rtcConfiguration != null ? { rtcConfiguration } : {})
 
   // Positional, like the demo. Destructuring `{ stream }` here reports itself as
@@ -503,25 +600,20 @@ export async function createPeer ({
      * consent to anything.
      */
     acceptOffer: async offer => {
-      const answer = await session.acceptOffer(offer)
+      await noteScan(offer, QR_TYPE_OFFER)
 
-      try {
-        const { peerId } = await decodePayload(offer, QR_TYPE_OFFER)
-
-        scanned.add(String(peerId))
-      } catch {
-        // It decoded a moment ago inside `acceptOffer`, so this is close to
-        // unreachable. If it ever happens the peer is asked about rather than
-        // admitted, which is the direction to fail in.
-      }
-
-      return answer
+      return session.acceptOffer(offer)
     },
 
     /** Back on the offering side: read the reply and connect. */
     acceptAnswer: async answer => {
+      await noteScan(answer, QR_TYPE_ANSWER)
+
       const { peerId } = await session.acceptAnswer(answer)
 
+      // Again, and this one is authoritative: the session knows who it
+      // actually connected to, while the line above only knows who signed the
+      // code. They agree in every case that works.
       scanned.add(peerId.toString())
       return peerId.toString()
     },