Spend an MCP token only for its own server, and only at its own address

.github/workflows/ci.yml

@@ -85,7 +85,7 @@ jobs:
       - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
         with:
           persist-credentials: false
-      - uses: azure/setup-helm@b9e51907a09c216f16ebe8536097933489208112 # v4.3.0
+      - uses: azure/setup-helm@9bc31f4ebc9c6b171d7bfbaa5d006ae7abdb4310 # v5.0.1
         with:
           version: v3.19.0
       # For the coherence check below, which is a Bun script like everything else here.

CHANGELOG.md

@@ -8,6 +8,35 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.
 
 ## Unreleased
 
+### A channel a Bot has spoken in unseen shows a dot
+
+The sidebar marks a channel when a Bot has said something since you last had it open: a dot beside
+the preview, the name a touch heavier. Opening the channel clears it, your own messages never set
+it, and the channel you are looking at never shows it. The marker is yours alone — per member, on
+the membership row like the pin — so one person reading does not clear anybody else's dot.
+
+The deployment gains one nullable column, via migration `0019`.
+
+### The API can reach Intelligence and sign-in when a NetworkPolicy is on
+
+`networkPolicy.enabled` wrote a rule for the API server that named DNS, the database and the Bots'
+computers, and nothing on 443. On a cluster that enforces policy the server could therefore reach
+neither CopilotKit Intelligence, nor an identity provider, nor a Bot: nobody could sign in and no
+conversation ran. Two of the five shipped `ci/` targets turn the policy on, and on GKE enforcement is
+the default and cannot be switched off.
+
+Nothing said so. The pod passed every probe and stayed Ready, because `/health` answers from a
+literal, so the first evidence was a timeout to a hostname that read as the internet being down.
+
+The API now reaches HTTP and HTTPS everywhere outside the cluster's private ranges, in every
+`computers.mode` rather than only `sandbox`, cut by the same exception list the computers' own policy
+uses. It still cannot address another pod, a node, or a cloud metadata endpoint.
+
+`mode: sandbox` had been working only because a rule meant for the Kubernetes API server carried no
+destination and so permitted everything. That rule now covers the API server alone, and
+`networkPolicy.kubernetesApiCidr` narrows it to your cluster's service range; left empty it stays as
+it was, because a chart cannot know that range.
+
 ### A finished turn shows the page it opened, not the one open now
 
 Reopening a conversation made every past turn fetch the screen as it is now, so an answer about
@@ -163,6 +192,46 @@ read that as a stolen token and revoke the whole connection. Every plugin call t
 token now locks the credential's vault row for the length of the exchange, so a second replica waits
 rather than races, and the rotated token is written back in the same transaction that held the lock.
 Nothing to configure; a connection just stops going stale under concurrent traffic.
+
+### An MCP token is spent only by its own server, and only at the address it was given
+
+Pointing a server at a credential is the one place this deployment takes a reference to a stored
+secret rather than the secret itself. Everywhere else, the value was typed into the same request that
+stores it: a Bot's key is minted from what an administrator pasted and the id it gets is nobody's to
+choose. So this is the one field where which secret and which address could be made to disagree, and
+the add settles the disagreement by spending the credential: the tool refresh runs before the call
+returns and sends what it decrypts to the URL from that same request.
+
+Two ways they could disagree, and both are now refused. A server could be pointed at any `mcp`
+credential in the vault, including one minted for a different vendor, so a token given to one server
+was deliverable to another. And re-adding a server with a different URL rewrote the address while
+keeping the credential, so the same token could be sent somewhere else entirely with no
+cross-server trick at all: the token really did belong to that server, and only the address moved.
+
+The second is why the first was not enough on its own. A credential now has to belong to the server
+it is attached to, and a server that already holds one cannot be re-added at a different address.
+Correcting a title or retrying an interrupted add sends the same URL and is unaffected. A server
+holding no credential can still be re-addressed, because there is nothing to misdirect. Moving a
+server that does hold one means removing it and adding it again with the token the new address is
+meant to have, which is the honest description of what has happened anyway.
+
+This matters more than "an administrator could misconfigure something". A stored credential cannot
+be read back by anybody, by design: the credentials screen answers that a credential exists and
+never what it is. These two shapes were the way around that, so a deployment where somebody has
+used them should treat the credentials involved as disclosed and rotate them.
+
+A token also stops outliving the server it was minted for. Re-adding a server without naming a
+credential used to clear the pointer while leaving the credential live, and removing a server retires
+its token by reading it off that pointer, so a cleared one meant the token survived its server and
+could be attached to a freshly created one at any address, where there was no longer a stored address
+to compare against. Three ordinary acts in a row and the binding above stopped meaning anything. The
+pointer now survives a re-add that names none, removal therefore finds and retires it, and a retired
+credential is refused rather than quietly attached to fail on its next call.
+
+Curated servers keep working as they did. Their URL comes from the catalogue rather than the
+request, and a per-instance hostname is matched against the vendor's own anchored pattern before
+anything is stored, so re-adding one cannot point it at an address of the caller's choosing.
+
 ### Knowledge searches instead of guessing
 
 A package can say which of its skills each coworker gets, and the fintech example gives Knowledge the
@@ -214,6 +283,60 @@ this port has to reach it another way**, which is what publishing it on every in
 This does not reach back in time. A deployment that has been running with the two on one network
 should assume a Bot could have read or written the database, and look at the trail with that in
 mind.
+### A credential in an MCP server address is refused in the query and the fragment too
+
+Refusing `https://user:token@vendor.example/mcp` closed the userinfo spelling of a credential in the
+address and left the two obvious ones open. `?token=`, `?api_key=` and their neighbours were still
+accepted, and the address is stored and named in the trail exactly as given: audit redaction keys on
+the field name, `url` is not a sensitive one, so the secret was written to `mcp_servers` and to an
+append-only audit row in clear text. That is the same disclosure the userinfo rule exists to prevent,
+one character away.
+
+A parameter whose name reads as a credential is now refused, in the query string and in the fragment,
+and the refusal points at the token field without repeating what was typed. The name is read rather
+than matched against a list, so `?auth_token=`, `?x-api-key=` and `?X-Amz-Signature=` are refused
+alongside `?token=`: a rule that only catches the spellings somebody thought of reads as a guard
+while behaving like a gap. The test is on the parameter name rather than on the presence of a query,
+because vendors route and version with parameters and a floor that refused every one of them would
+be one an operator works around instead of with. `https://mcp.example.com/mcp?workspace=acme&version=2`
+is unaffected, and so is an ordinary fragment. A credential written into the *path* is still
+accepted: it is indistinguishable from a route, and at least one hosted provider addresses servers
+that way. **A deployment where somebody has put a credential in an address should treat it as
+disclosed and rotate it**, for the same reason as before: the audit row cannot be deleted.
+
+`metadata.goog` is refused too. It is Google's own short name for the metadata server, published
+beside `metadata.google.internal`, and it carries a dot and none of the suffixes this check lists, so
+it read as an ordinary vendor name. The long spelling was only ever refused incidentally, by the
+`.internal` rule. Both are now named, so the address this check was written for is refused on purpose
+rather than by luck.
+### A curated MCP server is pointed at its own kind of credential too
+
+Adding a server by URL was made to check which credential it is being pointed at. Adding one from the
+catalogue, the other half of the same screen, took the same field from the same request and stored it
+unread, so a credential of any kind could be attached to a curated server and spent by the refresh
+that runs before the add returns.
+
+Worth being plain about the reach, because it is narrower than the path beside it. The column is a
+foreign key, so an id naming nothing was already refused by the database, and the one entry in the
+catalogue is reached with each person's own Google account, whose OAuth client is registered through
+its own call and sent to an address pinned in code. Nothing could be delivered to an address a caller
+chose. What was reachable was a credential of the wrong kind being accepted and spent on behalf of
+somebody who never agreed to it, and a malformed id arriving as a database error rather than as a
+refusal.
+
+The rule now comes from the entry: a server the deployment holds one token for takes that token, and
+a server answered as the person asking takes no credential when it is added, because its client
+arrives through the call that mints it. Both add paths ask the same question in the same words, so a
+credential that does not exist and one of the wrong kind are still refused identically and the
+endpoint cannot be used to ask which ids are real. Adding a curated server the way the admin screen
+does is unchanged.
+
+Adding a curated server that is already there no longer clears the credential it points at. The
+column holds the OAuth client that registering one put there, and re-adding the server to change an
+instance host said nothing about that client, but cleared it anyway: the credential row was left
+behind with nothing pointing at it and nothing to revoke it, and everybody who had connected their
+account was told the deployment has no client registered. A re-add that names no credential now
+leaves the one that is there alone.
 
 ### Name the private addresses an agent may live at
 

app/src/components/app-sidebar/app-sidebar.tsx

@@ -13,7 +13,12 @@ import {
   useQuery,
   useQueryClient,
 } from "@tanstack/react-query";
-import { Link, type LinkOptions, useNavigate } from "@tanstack/react-router";
+import {
+  Link,
+  type LinkOptions,
+  useNavigate,
+  useParams,
+} from "@tanstack/react-router";
 import { AnimatePresence, motion, useReducedMotion } from "motion/react";
 import type * as React from "react";
 import { useState } from "react";
@@ -123,6 +128,30 @@ export function pinnedFirst(channels: ChannelSummary[]): ChannelSummary[] {
   return [...channels].sort((a, b) => Number(b.pinned) - Number(a.pinned));
 }
 
+/**
+ * Whether a Bot has said something this member has not had on screen yet.
+ *
+ * A Bot's message, and only a Bot's: your own message carries a null agent id and reading your own
+ * words needs no marker. ISO-8601 strings compare correctly as strings, which is the same bet the
+ * server's recency sort already makes.
+ */
+export function hasUnseenActivity(channel: ChannelSummary): boolean {
+  if (channel.lastMessageAgentId === null || channel.lastMessageAt === null) {
+    return false;
+  }
+  return (
+    channel.lastReadAt === null || channel.lastMessageAt > channel.lastReadAt
+  );
+}
+
+/** Unseen activity somewhere you are not looking. The open channel never shows the dot. */
+export function isUnread(
+  channel: ChannelSummary,
+  openChannelId: string | undefined,
+): boolean {
+  return channel.id !== openChannelId && hasUnseenActivity(channel);
+}
+
 /**
  * A roster row that can animate.
  *
@@ -138,6 +167,13 @@ function ChannelRow({
   animateOrder: boolean;
 }) {
   const shouldReduceMotion = useReducedMotion();
+  // Whether this row is unread, as a boolean, for the same reason `Channel` computes `isOpen`
+  // that way: navigating re-renders the rows whose answer changed, not the whole roster.
+  const unread = useParams({
+    strict: false,
+    select: (params) =>
+      isUnread(channel, (params as { channelId?: string }).channelId),
+  });
   return (
     <motion.div
       animate={{ opacity: 1, transform: "translateY(0px)" }}
@@ -160,6 +196,7 @@ function ChannelRow({
             : undefined
         }
         pinned={channel.pinned}
+        unread={unread}
       />
     </motion.div>
   );

app/src/components/app-sidebar/channel.tsx

@@ -42,13 +42,15 @@ export const Channel = memo(function Channel({
   lastMessage,
   lastMessageAt,
   pinned,
+  unread,
 }: {
   channelId: string;
   participantIds: string[];
   name: string;
   lastMessage?: string;
   lastMessageAt?: string;
   pinned: boolean;
+  unread: boolean;
 }) {
   const queryClient = useQueryClient();
   const navigate = useNavigate();
@@ -112,7 +114,11 @@ export const Channel = memo(function Channel({
             </div>
             <div className="flex-col min-w-0 flex-1">
               <div className="flex flex-row items-center justify-between gap-2">
-                <span className="text-[14px] tracking-[-1%] truncate">
+                <span
+                  className={`text-[14px] tracking-[-1%] truncate ${
+                    unread ? "font-medium" : ""
+                  }`}
+                >
                   {name}
                 </span>
                 <div className="text-[12px] text-muted-foreground/70">
@@ -123,6 +129,10 @@ export const Channel = memo(function Channel({
                 <span className="min-w-0 flex-1 truncate text-[12px] leading-4 text-muted-foreground">
                   {lastMessage}
                 </span>
+                {unread ? (
+                  /* State about the message beats state about the row, so it sits first. */
+                  <span className="size-2 shrink-0 rounded-full bg-primary" />
+                ) : null}
                 {pinned ? (
                   <IconPinFilled className="size-3 shrink-0 text-muted-foreground/70" />
                 ) : null}

app/src/lib/channels/mutations.ts

@@ -1,6 +1,10 @@
-import { mutationOptions, type QueryClient } from "@tanstack/react-query";
+import {
+  mutationOptions,
+  type InfiniteData,
+  type QueryClient,
+} from "@tanstack/react-query";
 import { client, tryClient } from "@/lib/client";
-import { type AgentChannel, channelKeys } from "./queries";
+import { type AgentChannel, type ChannelPage, channelKeys } from "./queries";
 
 /**
  * Start a new channel with one or more coworkers.
@@ -66,6 +70,54 @@ export function setChannelPinnedMutationOptions(queryClient: QueryClient) {
   });
 }
 
+/**
+ * Stamp a channel read for this member, patching the cache before the wire answers.
+ *
+ * Patched in onMutate rather than refetched on success: the dot must clear the instant the channel
+ * opens, not a round-trip later. No rollback on failure and no invalidation — a mark-read that did
+ * not land is a dot that returns on the next refetch, which is the truth reasserting itself, and a
+ * refetch here would race the socket's own patches for nothing.
+ */
+export function markChannelReadMutationOptions(queryClient: QueryClient) {
+  return mutationOptions({
+    mutationFn: async (channelId: string) => {
+      await client(`/api/channels/${channelId}/read`, {
+        method: "PUT",
+        fallback: "Could not mark this channel read",
+      });
+    },
+    onMutate: (channelId) => {
+      const now = new Date().toISOString();
+      queryClient.setQueryData(
+        channelKeys.list(),
+        (data: InfiniteData<ChannelPage> | undefined) =>
+          data && {
+            ...data,
+            pages: data.pages.map((page) => ({
+              ...page,
+              channels: page.channels.map((row) =>
+                row.id === channelId
+                  ? {
+                      ...row,
+                      /*
+                       * The later of now and the row's own lastMessageAt: lastMessageAt comes from
+                       * another clock, and a marker stamped "now" by a clock running behind it
+                       * would leave the row still reading as unseen — and the dot still lit.
+                       */
+                      lastReadAt:
+                        row.lastMessageAt && row.lastMessageAt > now
+                          ? row.lastMessageAt
+                          : now,
+                    }
+                  : row,
+              ),
+            })),
+          },
+      );
+    },
+  });
+}
+
 /** Soft-delete a channel for everyone in it. The server keeps the transcript; the roster forgets. */
 export function deleteChannelMutationOptions(queryClient: QueryClient) {
   return mutationOptions({

app/src/lib/channels/queries.ts

@@ -26,6 +26,8 @@ export type ChannelSummary = AgentChannel & {
   createdAt: string;
   /** Whether this member pinned the channel. Pinned channels sort first in the roster. */
   pinned: boolean;
+  /** ISO-8601 when this member last had the channel open, or null for never. The caller's, only. */
+  lastReadAt: string | null;
 };
 
 export const channelKeys = {