fix(notifications): render <openhuman-link> tags in notification bodies (#2279) (#2339)

Co-authored-by: sanil-23 <sanil@alphahuman.xyz> 
Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>

app/src/components/notifications/NotificationBody.tsx

@@ -0,0 +1,67 @@
+/**
+ * Shared `<openhuman-link>` rendering for notification surfaces
+ * (`NotificationCard` and the `Notifications` page).
+ *
+ * Notification bodies emitted by Rust error helpers can contain
+ * `<openhuman-link path="…">…</openhuman-link>` tags (e.g. morning-briefing
+ * failures pointing users at Discord / Settings). Without parsing, the raw
+ * tag leaks as literal text. This module mirrors the chat-side pill so the
+ * tag renders as a clickable pill instead.
+ *
+ * **Why this lives here and not in a global shared spot:** the chat-side
+ * `OpenhumanLinkPill` is a non-exported function inside `AgentMessageBubble.tsx`
+ * (`app/src/pages/conversations/`). Extracting from chat would change the chat
+ * render path — out of scope for this fix. Instead, we keep the grammar / parsing
+ * shared (reuses `parseBubbleSegments` from conversations) but reimplement the
+ * pill locally. Both notification surfaces share *this* file so the diff stays
+ * testable with one Vitest suite.
+ *
+ * Safety: this component renders **only** text + button elements. It never
+ * uses `dangerouslySetInnerHTML`, never sets an `href`, and the dispatched
+ * `OPENHUMAN_LINK_EVENT` is consumed by `OpenhumanLinkModal`, which hard-
+ * allowlists `path` values before routing. See `OpenhumanLinkModal.tsx`
+ * `ALLOWED_PATHS_SET`.
+ */
+import { parseBubbleSegments } from '../../pages/conversations/utils/format';
+import { OPENHUMAN_LINK_EVENT } from '../OpenhumanLinkModal';
+
+function NotificationLinkPill({ path, label }: { path: string; label: string }) {
+  return (
+    <button
+      type="button"
+      onClick={e => {
+        // Don't trigger the surrounding card / row click — the pill is its
+        // own action.
+        e.stopPropagation();
+        window.dispatchEvent(new CustomEvent(OPENHUMAN_LINK_EVENT, { detail: { path } }));
+      }}
+      className="inline-flex items-center gap-1 rounded-full border border-primary-200 bg-primary-50 px-2 py-0.5 text-[11px] font-medium text-primary-700 transition-colors hover:bg-primary-100">
+      {label}
+      <svg className="h-2.5 w-2.5" viewBox="0 0 24 24" fill="none" stroke="currentColor">
+        <path
+          strokeLinecap="round"
+          strokeLinejoin="round"
+          strokeWidth={2}
+          d="M5 12h14M13 6l6 6-6 6"
+        />
+      </svg>
+    </button>
+  );
+}
+
+export default function NotificationBody({ body }: { body: string }) {
+  const segments = parseBubbleSegments(body);
+  return (
+    <>
+      {segments.map((seg, i) =>
+        seg.kind === 'link' ? (
+          <NotificationLinkPill key={i} path={seg.path} label={seg.label} />
+        ) : (
+          // React auto-escapes text content, so any other markup in the body
+          // (e.g. `<script>…</script>`) renders as literal text.
+          <span key={i}>{seg.text}</span>
+        )
+      )}
+    </>
+  );
+}

app/src/components/notifications/NotificationCard.test.tsx

@@ -1,50 +1,175 @@
-import { render, screen } from '@testing-library/react';
+/**
+ * Markup-leak guard for `<openhuman-link>` rendering in notification bodies.
+ *
+ * These tests are the airtight contract for issue #2279 (Bug A) — they assert:
+ *   - well-formed tags become pills (no raw `<openhuman-link>` text leaks),
+ *   - attacker-influenceable `path` values can't become a navigable
+ *     `javascript:` link, and
+ *   - non-link bodies and stray markup render as literal, auto-escaped text
+ *     (no `<script>` element gets injected into the DOM).
+ *
+ * NotificationCard and the Notifications page both render bodies the same way,
+ * so a single suite covers both call sites — we render NotificationCard for
+ * the local body component (since the page uses an identical inline helper)
+ * and the page exports its own helper too.
+ */
+import { fireEvent, render, screen, within } from '@testing-library/react';
 import { describe, expect, it, vi } from 'vitest';
 
 import type { IntegrationNotification } from '../../types/notifications';
-import NotificationCard, { formatNotificationBodyPreview } from './NotificationCard';
+import { OPENHUMAN_LINK_EVENT } from '../OpenhumanLinkModal';
+import NotificationCard from './NotificationCard';
 
-function notification(overrides: Partial<IntegrationNotification> = {}): IntegrationNotification {
+function makeNotification(body: string): IntegrationNotification {
   return {
-    id: 'notif-1',
-    provider: 'cron',
-    account_id: 'morning_briefing',
-    title: 'morning_briefing',
-    body: 'Morning briefing ready.',
+    id: 'n-1',
+    provider: 'gmail',
+    title: 'Morning briefing',
+    body,
     raw_payload: {},
-    importance_score: 0.65,
-    triage_action: 'react',
-    triage_reason: 'Scheduled delivery',
     status: 'unread',
     received_at: new Date().toISOString(),
-    scored_at: new Date().toISOString(),
-    ...overrides,
   };
 }
 
-describe('formatNotificationBodyPreview', () => {
-  it('strips openhuman-link markup while preserving the label', () => {
+function renderCard(body: string) {
+  return render(<NotificationCard notification={makeNotification(body)} onMarkRead={vi.fn()} />);
+}
+
+describe('NotificationCard <openhuman-link> rendering', () => {
+  it('renders an <openhuman-link> tag as a pill (no raw tag leaks)', () => {
+    const body = '<openhuman-link path="community/discord">Report on Discord</openhuman-link>';
+    renderCard(body);
+
+    const bodyEl = screen.getByTestId('notification-card-body');
+    // The pill is a <button> with the label as accessible name. The outer
+    // notification card is also a <button> wrapping everything, so we scope
+    // the query to the body element.
+    const pill = within(bodyEl).getByRole('button', { name: /Report on Discord/i });
+    expect(pill).toBeInTheDocument();
+
+    // Critically: the raw tag text must NOT appear anywhere in the rendered DOM.
+    expect(bodyEl.textContent ?? '').not.toContain('<openhuman-link');
+    expect(bodyEl.textContent ?? '').not.toContain('</openhuman-link>');
+  });
+
+  it('does NOT emit a navigable javascript: link for a malicious path (XSS guard)', () => {
+    const body = '<openhuman-link path="javascript:alert(1)">click me</openhuman-link>';
+    const { container } = renderCard(body);
+
+    // No <a href="javascript:..."> anywhere in the rendered tree. The pill is
+    // a <button>, never an <a>, but we assert the absolute invariant directly.
+    const anchors = container.querySelectorAll('a');
+    for (const a of anchors) {
+      const href = a.getAttribute('href') ?? '';
+      expect(href.toLowerCase().startsWith('javascript:')).toBe(false);
+    }
+
+    // Even though the pill exists (the parser doesn't allowlist `path` — the
+    // modal listener does), clicking it MUST NOT navigate. We verify by
+    // listening for the dispatched custom event and confirming the path is
+    // exactly what was parsed — the OpenhumanLinkModal listener (which
+    // hard-allowlists paths) is what stops the dangerous string from doing
+    // anything. See OpenhumanLinkModal.tsx ALLOWED_PATHS_SET.
+    const seen: string[] = [];
+    const listener = (e: Event) => {
+      const detail = (e as CustomEvent<{ path: string }>).detail;
+      seen.push(detail?.path ?? '');
+    };
+    window.addEventListener(OPENHUMAN_LINK_EVENT, listener);
+    try {
+      const bodyEl = screen.getByTestId('notification-card-body');
+      const pill = within(bodyEl).getByRole('button', { name: /click me/i });
+      pill.click();
+    } finally {
+      window.removeEventListener(OPENHUMAN_LINK_EVENT, listener);
+    }
+
+    // The dispatched event payload is exactly what was parsed — but `OpenhumanLinkModal`
+    // (the listener that actually opens UI) hard-allowlists paths, so the
+    // `javascript:` string never gets to act on anything. Both halves of the
+    // contract are asserted: dispatch is faithful, navigation is impossible.
+    expect(seen).toEqual(['javascript:alert(1)']);
+
+    // No href injection regardless of whether the pill rendered.
     expect(
-      formatNotificationBodyPreview(
-        'You can <openhuman-link path="community/discord">Report on Discord</openhuman-link>.'
-      )
-    ).toBe('You can Report on Discord.');
+      Array.from(container.querySelectorAll<HTMLElement>('[href]')).every(el => {
+        const href = el.getAttribute('href') ?? '';
+        return !href.toLowerCase().startsWith('javascript:');
+      })
+    ).toBe(true);
   });
-});
-
-describe('NotificationCard', () => {
-  it('does not render raw openhuman-link markup in the body preview', () => {
-    const rendered = render(
-      <NotificationCard
-        notification={notification({
-          body: 'Something went wrong.\n<openhuman-link path="community/discord">Report on Discord</openhuman-link>',
-        })}
-        onMarkRead={vi.fn()}
-      />
-    );
-
-    expect(screen.getByText(/Report on Discord/)).toBeInTheDocument();
-    expect(rendered.container.textContent).not.toContain('<openhuman-link');
-    expect(rendered.container.textContent).not.toContain('</openhuman-link>');
+
+  it('renders a plain-text body as the literal string (regression guard)', () => {
+    renderCard('plain text without any tags');
+
+    const bodyEl = screen.getByTestId('notification-card-body');
+    expect(bodyEl).toHaveTextContent('plain text without any tags');
+    // No pill rendered for tag-free body — scope to the body element to ignore
+    // the outer card <button>.
+    expect(within(bodyEl).queryByRole('button')).toBeNull();
+  });
+
+  it('renders <script> as literal text — does not inject a <script> element', () => {
+    const body = '<script>alert(1)</script>';
+    const { container } = renderCard(body);
+
+    // React auto-escapes anything that's not an `<openhuman-link>` segment.
+    const bodyEl = screen.getByTestId('notification-card-body');
+    expect(bodyEl.textContent).toBe('<script>alert(1)</script>');
+
+    // Hard guarantee: no actual <script> element anywhere in the rendered tree.
+    expect(container.querySelector('script')).toBeNull();
+  });
+
+  it('renders mixed text + link segments in order', () => {
+    const body = 'Before <openhuman-link path="community/discord">Discord</openhuman-link> after';
+    renderCard(body);
+
+    const bodyEl = screen.getByTestId('notification-card-body');
+    expect(bodyEl).toHaveTextContent(/Before/);
+    expect(bodyEl).toHaveTextContent(/after/);
+    expect(within(bodyEl).getByRole('button', { name: /Discord/i })).toBeInTheDocument();
+    expect(bodyEl.textContent ?? '').not.toContain('<openhuman-link');
+  });
+
+  // Keyboard-activation coverage for the `<div role="button">` wrapper we use
+  // instead of a real `<button>` (a real button can't legally contain the
+  // `<openhuman-link>` pill which is also a button). Exercises the `onKeyDown`
+  // branch so the diff-coverage gate sees those lines hit.
+  it('activates the card body via Enter and Space keys', () => {
+    const onMarkRead = vi.fn();
+    const notification = makeNotification('plain body, no pill so no inner button');
+    // Plain text body, so the only role=button in the card is the outer wrapper.
+    render(<NotificationCard notification={notification} onMarkRead={onMarkRead} />);
+
+    const wrapper = screen.getByRole('button');
+    fireEvent.keyDown(wrapper, { key: 'Enter' });
+    fireEvent.keyDown(wrapper, { key: ' ' });
+    // Status is `unread` (set by makeNotification) and no `onNavigate` was
+    // passed, so handleBodyClick falls through to onMarkRead each time.
+    expect(onMarkRead).toHaveBeenCalledTimes(2);
+    expect(onMarkRead).toHaveBeenCalledWith(notification.id);
+
+    // Other keys must NOT activate — guard against the branch being too loose.
+    onMarkRead.mockClear();
+    fireEvent.keyDown(wrapper, { key: 'a' });
+    fireEvent.keyDown(wrapper, { key: 'Tab' });
+    expect(onMarkRead).not.toHaveBeenCalled();
+  });
+
+  // Bubbling guard: pressing Enter/Space while focused on the inner pill must
+  // NOT also activate the surrounding card. Without `e.target !== e.currentTarget`
+  // the keydown would bubble up and trigger `handleBodyClick` accidentally.
+  it('does NOT activate the card when keydown bubbles from the inner pill', () => {
+    const onMarkRead = vi.fn();
+    const body = '<openhuman-link path="community/discord">Discord</openhuman-link>';
+    render(<NotificationCard notification={makeNotification(body)} onMarkRead={onMarkRead} />);
+
+    const bodyEl = screen.getByTestId('notification-card-body');
+    const pill = within(bodyEl).getByRole('button', { name: /Discord/i });
+    fireEvent.keyDown(pill, { key: 'Enter' });
+    fireEvent.keyDown(pill, { key: ' ' });
+    expect(onMarkRead).not.toHaveBeenCalled();
   });
 });

app/src/components/notifications/NotificationCard.tsx

@@ -1,5 +1,6 @@
 import { useT } from '../../lib/i18n/I18nContext';
 import type { IntegrationNotification } from '../../types/notifications';
+import NotificationBody from './NotificationBody';
 
 // ─────────────────────────────────────────────────────────────────────────────
 // Helpers
@@ -44,17 +45,6 @@ function scoreBadgeClass(score: number): string {
   return 'bg-sage-500/20 text-green-700 border-green-200';
 }
 
-const OPENHUMAN_LINK_TAG =
-  /<openhuman-link\s+path=(?:"[^"]*"|'[^']*')\s*>([\s\S]*?)<\/openhuman-link>/gi;
-
-export function formatNotificationBodyPreview(body: string): string {
-  return body
-    .replace(OPENHUMAN_LINK_TAG, '$1')
-    .replace(/[ \t]+\n/g, '\n')
-    .replace(/\n{3,}/g, '\n\n')
-    .trim();
-}
-
 // ─────────────────────────────────────────────────────────────────────────────
 // Component
 // ─────────────────────────────────────────────────────────────────────────────
@@ -69,7 +59,6 @@ interface Props {
 const NotificationCard = ({ notification: n, onMarkRead, onNavigate, onDismiss }: Props) => {
   const { t } = useT();
   const isUnread = n.status === 'unread';
-  const bodyPreview = n.body ? formatNotificationBodyPreview(n.body) : '';
 
   const handleBodyClick = () => {
     if (onNavigate) {
@@ -92,8 +81,24 @@ const NotificationCard = ({ notification: n, onMarkRead, onNavigate, onDismiss }
           )}
         </div>
 
-        <button
+        {/* `role="button"` + key handler instead of a real `<button>` because
+            this wrapper contains `NotificationLinkPill` (also a `<button>`),
+            and nested interactive elements break keyboard / screen-reader
+            behaviour (HTML spec disallows it). */}
+        <div
+          role="button"
+          tabIndex={0}
           onClick={handleBodyClick}
+          onKeyDown={e => {
+            // Ignore bubbled keydown from inner controls (e.g. the link pill).
+            // Without this, pressing Enter/Space on a focused pill would also
+            // activate the card body.
+            if (e.target !== e.currentTarget) return;
+            if (e.key === 'Enter' || e.key === ' ') {
+              e.preventDefault();
+              handleBodyClick();
+            }
+          }}
           className="flex-1 min-w-0 text-left focus:outline-none focus-visible:ring-2 focus-visible:ring-primary-500 focus-visible:ring-offset-1 rounded-sm">
           {/* Header row: provider badge + timestamp */}
           <div className="flex items-center gap-2 mb-1">
@@ -129,13 +134,15 @@ const NotificationCard = ({ notification: n, onMarkRead, onNavigate, onDismiss }
             {n.title}
           </p>
 
-          {/* Body preview */}
-          {bodyPreview && (
-            <p className="text-xs text-stone-500 dark:text-neutral-400 mt-0.5 line-clamp-2">
-              {bodyPreview}
+          {/* Body preview — `<openhuman-link>` tags render as pills */}
+          {n.body && (
+            <p
+              data-testid="notification-card-body"
+              className="text-xs text-stone-500 dark:text-neutral-400 mt-0.5 line-clamp-2">
+              <NotificationBody body={n.body} />
             </p>
           )}
-        </button>
+        </div>
         {onDismiss && (
           <button
             onClick={() => onDismiss(n.id)}

app/src/pages/Notifications.tsx

@@ -1,6 +1,7 @@
 import { useMemo } from 'react';
 import { useNavigate } from 'react-router-dom';
 
+import NotificationBody from '../components/notifications/NotificationBody';
 import NotificationCenter from '../components/notifications/NotificationCenter';
 import { useT } from '../lib/i18n/I18nContext';
 import { resolveSystemRoute } from '../lib/notificationRouter';
@@ -104,9 +105,24 @@ const Notifications = () => {
           <ul className="divide-y divide-stone-100 dark:divide-neutral-800">
             {items.map(item => (
               <li key={item.id} data-testid="notification-item">
-                <button
-                  type="button"
+                {/* `role="button"` instead of a real `<button>` — the row body
+                    contains `NotificationLinkPill` (also a `<button>`), and
+                    nested interactive elements break keyboard / screen-reader
+                    behaviour (HTML spec disallows it). */}
+                <div
+                  role="button"
+                  tabIndex={0}
                   onClick={() => handleClick(item)}
+                  onKeyDown={e => {
+                    // Ignore bubbled keydown from inner controls (e.g. the
+                    // link pill). Without this, pressing Enter/Space on a
+                    // focused pill would also activate the row.
+                    if (e.target !== e.currentTarget) return;
+                    if (e.key === 'Enter' || e.key === ' ') {
+                      e.preventDefault();
+                      handleClick(item);
+                    }
+                  }}
                   className={`w-full text-left px-4 py-3 hover:bg-stone-50 dark:hover:bg-neutral-800/60 transition-colors ${
                     item.read
                       ? 'bg-white dark:bg-neutral-900'
@@ -128,15 +144,17 @@ const Notifications = () => {
                       <p className="mt-1 text-sm font-semibold text-stone-900 dark:text-neutral-100 truncate">
                         {item.title}
                       </p>
-                      <p className="mt-0.5 text-sm text-stone-600 dark:text-neutral-300 line-clamp-2">
-                        {item.body}
+                      <p
+                        data-testid="notification-item-body"
+                        className="mt-0.5 text-sm text-stone-600 dark:text-neutral-300 line-clamp-2">
+                        <NotificationBody body={item.body} />
                       </p>
                     </div>
                     <span className="text-[11px] text-stone-400 dark:text-neutral-500 whitespace-nowrap">
                       {formatTime(item.timestamp, t)}
                     </span>
                   </div>
-                </button>
+                </div>
               </li>
             ))}
           </ul>

app/src/pages/__tests__/Notifications.test.tsx

@@ -0,0 +1,141 @@
+/**
+ * Coverage guard for the new `<div role="button">` row wrapper + onKeyDown
+ * handler in `Notifications.tsx` (Bug A of #2279). The wrapper replaces a
+ * `<button>` because rows now contain `NotificationLinkPill` (also a
+ * `<button>`), and nested interactive elements are invalid HTML.
+ *
+ * Scoped to behavioural assertions: keyboard activation dispatches
+ * `markRead` and navigates. The body-rendering path (`<openhuman-link>`
+ * parsing, pill, XSS guards) is covered once by `NotificationCard.test.tsx`
+ * via the shared `NotificationBody` component.
+ */
+import { configureStore } from '@reduxjs/toolkit';
+import { fireEvent, render, screen, within } from '@testing-library/react';
+import { Provider } from 'react-redux';
+import { MemoryRouter } from 'react-router-dom';
+import { describe, expect, it, vi } from 'vitest';
+
+import notificationsReducer, {
+  type NotificationCategory,
+  type NotificationItem,
+} from '../../store/notificationSlice';
+import Notifications from '../Notifications';
+
+const { navigate } = vi.hoisted(() => ({ navigate: vi.fn() }));
+
+vi.mock('react-router-dom', async () => {
+  const actual = await vi.importActual<typeof import('react-router-dom')>('react-router-dom');
+  return { ...actual, useNavigate: () => navigate };
+});
+
+vi.mock('../../lib/notificationRouter', () => ({ resolveSystemRoute: () => '/some-route' }));
+
+vi.mock('../../components/notifications/NotificationCenter', () => ({
+  default: () => <div data-testid="notification-center-stub" />,
+}));
+
+vi.mock('../../lib/i18n/I18nContext', () => ({ useT: () => ({ t: (k: string) => k }) }));
+
+function makeItem(
+  id: string,
+  body: string,
+  overrides: Partial<NotificationItem> = {}
+): NotificationItem {
+  return {
+    id,
+    title: 'A title',
+    body,
+    category: 'system' as NotificationCategory,
+    timestamp: Date.now(),
+    read: false,
+    ...overrides,
+  };
+}
+
+function renderPage(items: NotificationItem[]) {
+  const store = configureStore({
+    reducer: { notifications: notificationsReducer },
+    preloadedState: {
+      notifications: {
+        items,
+        preferences: {
+          messages: true,
+          agents: true,
+          skills: true,
+          system: true,
+          meetings: true,
+          reminders: true,
+          important: true,
+        },
+        integrationItems: [],
+        integrationUnreadCount: 0,
+        integrationLoading: false,
+        integrationError: null,
+      },
+    },
+  });
+  return {
+    store,
+    ...render(
+      <Provider store={store}>
+        <MemoryRouter>
+          <Notifications />
+        </MemoryRouter>
+      </Provider>
+    ),
+  };
+}
+
+describe('Notifications page row wrapper', () => {
+  it('renders <openhuman-link> body via the shared NotificationBody', () => {
+    renderPage([
+      makeItem('n-1', '<openhuman-link path="community/discord">Discord</openhuman-link>'),
+    ]);
+
+    const bodyEl = screen.getByTestId('notification-item-body');
+    // Pill rendered (so the body uses the shared component), raw tag absent.
+    expect(within(bodyEl).getByRole('button', { name: /Discord/i })).toBeInTheDocument();
+    expect(bodyEl.textContent ?? '').not.toContain('<openhuman-link');
+  });
+
+  it('activates a row via Enter and Space keys', () => {
+    const { store } = renderPage([makeItem('n-1', 'plain body')]);
+
+    // The row wrapper is the only role=button inside the row (plain body, no pill).
+    const row = screen.getByTestId('notification-item');
+    const wrapper = within(row).getByRole('button');
+
+    fireEvent.keyDown(wrapper, { key: 'Enter' });
+    expect(navigate).toHaveBeenCalledWith('/some-route');
+    expect(store.getState().notifications.items[0].read).toBe(true);
+
+    navigate.mockClear();
+    fireEvent.keyDown(wrapper, { key: ' ' });
+    expect(navigate).toHaveBeenCalledWith('/some-route');
+  });
+
+  it('does NOT activate on other keys (Tab, letters)', () => {
+    renderPage([makeItem('n-1', 'plain body')]);
+    const row = screen.getByTestId('notification-item');
+    const wrapper = within(row).getByRole('button');
+
+    fireEvent.keyDown(wrapper, { key: 'Tab' });
+    fireEvent.keyDown(wrapper, { key: 'a' });
+    expect(navigate).not.toHaveBeenCalled();
+  });
+
+  // Bubbling guard: Enter/Space on a focused inner pill must NOT also activate
+  // the row. CodeRabbit catch on PR #2339.
+  it('does NOT activate a row when keydown bubbles from the inner pill', () => {
+    const { store } = renderPage([
+      makeItem('n-1', '<openhuman-link path="community/discord">Discord</openhuman-link>'),
+    ]);
+
+    const bodyEl = screen.getByTestId('notification-item-body');
+    const pill = within(bodyEl).getByRole('button', { name: /Discord/i });
+    fireEvent.keyDown(pill, { key: 'Enter' });
+    fireEvent.keyDown(pill, { key: ' ' });
+    expect(navigate).not.toHaveBeenCalled();
+    expect(store.getState().notifications.items[0].read).toBe(false);
+  });
+});