Skip to content

feat: allow widget embed to pass end-user identity via data-end-user-id#834

Draft
yiboyasss wants to merge 11 commits into
xorbitsai:mainfrom
yiboyasss:feat/widget-end-user-scoping
Draft

feat: allow widget embed to pass end-user identity via data-end-user-id#834
yiboyasss wants to merge 11 commits into
xorbitsai:mainfrom
yiboyasss:feat/widget-end-user-scoping

Conversation

@yiboyasss

@yiboyasss yiboyasss commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Closes #831

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for identifying widget guests using a data-end-user-id attribute from the embedding page's script tag, falling back to an anonymous cached ID if none is provided. Feedback points out that because the user ID can contain arbitrary characters, the guestId should be URL-encoded when constructing the iframe URL to prevent malformed URLs or query parameter injection.

Comment thread frontend/public/widget.js Outdated
@yiboyasss yiboyasss marked this pull request as draft July 9, 2026 10:42
@yiboyasss yiboyasss marked this pull request as ready for review July 10, 2026 07:27
@yiboyasss yiboyasss force-pushed the feat/widget-end-user-scoping branch from cf16e14 to 1be94df Compare July 10, 2026 07:28
@yiboyasss

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enables returning widget guests with a stable end-user identity (via data-end-user-id) to resume their previous conversations across different devices or browsers. It introduces a new /tasks/latest endpoint on the backend to retrieve the most recent task for a guest and updates the frontend widget to fetch this task when local storage is empty. The review feedback points out critical issues in the SQLAlchemy query in get_latest_public_chat_task, specifically the invalid use of .as_string() instead of .astext and incorrect usage of .is_() for non-null channel IDs, which would cause runtime and syntax errors.

Comment thread src/xagent/web/api/public_chat_access.py
@yiboyasss

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a feature allowing widget guests to resume conversations across devices by passing a stable end-user identity (data-end-user-id) from the embedding page, supported by a new backend endpoint /tasks/latest to fetch the most recent task. However, the review highlights a critical security vulnerability (Broken Object Level Authorization) because the end-user identity is trusted by the backend without cryptographic verification. To prevent user impersonation and unauthorized access to chat histories, it is highly recommended to implement HMAC-SHA256 verification for the end-user identity.

Comment thread frontend/public/widget.js Outdated
Comment thread src/xagent/web/api/widget.py

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR extends the embeddable chat widget so an embedding page can pass a specific end-user identifier via a new data-end-user-id attribute, instead of relying solely on the random per-browser guest_id the widget generated before. It also adds a GET /api/widget/tasks/latest endpoint (plus get_latest_public_chat_task and frontend wiring) so a returning end-user with no local cache — e.g. on a new device or browser — can auto-resume their most recent conversation. The identifier flows client → ?guest_id= query param → the widget JWT's guest_id claim, and the new endpoint resolves the latest task by matching that claim. Linked issue #831 (P0).

Approach verdict: wrong-direction (on the security dimension)

The transport and resume plumbing is minimal and reuses the existing agent_config["guest_id"] abstraction cleanly. But the feature ships "scope sessions to a specific user" on top of an unauthenticated, client-forgeable identifier, and adds a lookup endpoint keyed on that same identifier — which is a cross-user data-exposure design change, not just a transport addition.

Design-level concerns:

  • Client-forgeable identity used for access scoping. data-end-user-id?guest_id= → the guest_id JWT claim, with zero server-side verification. The embedding server (which knows the real logged-in user) never signs this assertion. The codebase already has a server-signed primitive — embed_ticket, validated against widget_key + allowlist — that this feature bypasses entirely rather than extending. The end-user identity should be bound into a server-signed assertion, not accepted as a trusted plaintext client claim.
  • New enumeration surface. /tasks/latest lets any holder of a widget JWT resolve the latest task for an arbitrary guest_id, materially widening blast radius versus the pre-existing gap (see Finding A below).
  • Issue's open question left unresolved. #831 explicitly asked whether history/memory already scope to guest_id. History (tasks) is scoped by guest_id; agent memory is not scoped to guest_id at all. If widget-agent memory is ever enabled, all end-users of one widget would share a single memory namespace. This is silently unaddressed.
  • "Tenant" scoping named in the issue is not addressed. Only the agent-owner user_id + agent_id implicitly separate tenants; no explicit tenant concept is introduced.

Findings

Critical

A — Cross-user impersonation + conversation-history disclosure via unauthenticated guest_id + new /tasks/latest endpoint. (see inline comment on src/xagent/web/api/widget.py:303)

The raw trust gap (client-supplied guest_id copied verbatim into the JWT with no binding to a verified identity — widget.py:57, widget.py:261) is pre-existing and byte-identical to base. But this PR makes it practically exploitable: previously guest_id was a high-entropy random token an attacker could not guess; data-end-user-id now invites site owners to set it to a guessable identifier (email, customer ID), and the new /tasks/latest (auth-gated only by any valid widget JWT, matching guest_id by string equality with no ownership check) turns a guessed id into a cross-device history lookup.

Confirmed end-to-end exploit: POST /auth with a guessed guest_id (using the tenant's public widget_key) → valid JWT → GET /tasks/latest returns the victim's task_idchat/ws/{task_id} passes get_task_for_public_context (guest_id string matches) → full history streamed on connect, plus the ability to post as that identity.

Fix direction: bind the end-user identity into a server-signed assertion (e.g. via the existing embed_ticket, validated against widget_key + allowlist) rather than trusting data-end-user-id/guest_id as plaintext; and/or scope /tasks/latest to require something the server can verify belongs to the caller. Add a regression test proving guest A cannot retrieve guest B's task via this endpoint.

Medium

C — "anonymous" fallback becomes a live cross-visitor collision under the new auto-resume path. (see inline comment on frontend/src/components/widget/public-agent-chat-page.tsx:303)

The fallback is pre-existing and was previously inert — with no cached task id the widget always started a fresh conversation. This PR's new /tasks/latest auto-resume wiring makes the collision live: two unrelated visitors who reach the iframe URL directly (no guest_id query param, or an embed omitting data-end-user-id) both resolve to guest_id="anonymous" and get silently dropped into the same most-recent shared conversation.

Fix: skip /tasks/latest when guest_id === "anonymous", or reject/normalize empty guest_id server-side.

Minor

B — get_latest_public_chat_task uses .is_() unconditionally for channel_id. (see inline comment on src/xagent/web/api/public_chat_access.py:272)

Would raise a DB-level SQL error if channel_id were ever non-None; safe today only because authenticate_widget hardcodes channel_id: None. The sibling get_task_for_public_context already uses the correct conditional pattern — match it.

Testing

There is zero test coverage for any new behavior: no test references get_latest_public_chat_task, /tasks/latest, LatestTaskResponse, end_user_id, or data-end-user-id, and no existing test exercises the widget.py handlers at all. For a P0 access-scoping feature, the most important missing test is a negative isolation test proving guest A cannot retrieve guest B's task via /tasks/latest — that should land with the Finding A remediation.

Simplification opportunities

Lean already — no over-engineering found in this diff.

Comment thread src/xagent/web/api/widget.py
Comment thread frontend/src/components/widget/public-agent-chat-page.tsx
Comment thread src/xagent/web/api/public_chat_access.py Outdated
@yiboyasss yiboyasss marked this pull request as draft July 10, 2026 09:18
@yiboyasss yiboyasss marked this pull request as ready for review July 10, 2026 10:35

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — PR #834 (closes #831)

Update summary

Since the CHANGES_REQUESTED review, the trust boundary was closed. 7ba52aac makes data-end-user-id require a verified HMAC-SHA256(agent.widget_end_user_secret, end_user_id) signature (constant-time hmac.compare_digest, hard 403 on mismatch/missing); cdda069e adds owner-only UI to view/rotate that secret; d633512a replaces the shared "anonymous" fallback with a random per-browser direct_<rand> guest id; c2127683 moves the guest_id/end_user_id either-required check out of a model_validator. The critical BOLA/IDOR gap the human reviewer flagged is now closed.

Design verdict

Sound (acceptable-with-one-reservation). end_user_id is only honored with a valid server-side HMAC signature over the per-agent widget_end_user_secret; the secret mirrors the existing widget_key lifecycle (owner-only, auto-generated, rotatable, never shipped to the browser), and a reserved-namespace guard blocks forging a "verified" identity via the plain guest_id field.

D1 (reservation, design-level): the signature is a static, non-expiring bearer credential per end_user_id — no timestamp/nonce binding — placed in the iframe URL (query string + HTML attribute). A leaked (end_user_id, signature) pair is a permanent impersonation token until the owner rotates the secret, which invalidates every end-user at once (no per-user revocation). Defensible v1 tradeoff, but it's the natural next security gap. Recommend documenting the limitation (CHANGELOG or code comment); ideally bind an expiry into the signed payload in a future iteration.

Prior findings checklist

  1. FIXED — widget.js unencoded guestId in iframe URL: frontend/public/widget.js:146-158 runs every dynamic value through encodeURIComponent.
  2. FIXEDpublic_chat_access.py .as_string() + unconditional .is_(): src/xagent/web/api/public_chat_access.py:276 uses .astext; :272-274 conditionally branch channel_id.is_(...) vs == ..., matching get_task_for_public_context.
  3. FIXED (CRITICAL) — BOLA/IDOR plaintext identity trust: src/xagent/web/api/widget.py:267-309 now requires hmac.compare_digest verification (403 on mismatch/missing) plus a reserved-namespace guard at :306-309; secret minted server-side (src/xagent/web/api/agents.py:972-1032), never exposed to the browser. Original exploit chain no longer works; legacy anonymous path preserved.
  4. FIXED (MEDIUM)"anonymous" cross-visitor collision: frontend/src/components/widget/public-agent-chat-page.tsx:44-53 generates a persisted random direct_<rand> id; the "anonymous" string at :45 is an SSR-only guard that can't reach a real request.
  5. REFACTORED (design) — "extend embed_ticket instead of a new plaintext claim": resolved via a purpose-built scheme, not duplication. embed_ticket (widget.py:176-247, 60s-TTL JWT proving page origin) and the new end_user_signature (widget.py:267-304, durable HMAC proving the embedding server vouches for an identity) solve different problems and compose in the same /auth call. Ties into D1 (no expiry on the durable secret).
  6. NOT FIXED (design, open) — agent memory not scoped to guest_id: src/xagent/web/user_isolated_memory.py and dynamic_memory_store.py scope only by current_user_id; no guest/end-user parameter. Not touched by this PR. Open gap, not a blocker (#831 only raised the question); track separately.
  7. NOT FIXED (design, open) — no explicit "tenant" scoping: isolation still relies solely on implicit agent_id/user_id boundaries; no tenant entity/column introduced. Same disposition as #6.

New findings (this round)

MAJOR — frontend test coverage regression (frontend/src/components/build/deploy-agent-dialog.tsx)
agent-widget-settings-dialog.test.tsx (340 lines) was deleted with its component; the security-sensitive UI it covered (widget-key rotation, end-user secret display/rotation, allowed-domains validation, advanced-options toggle) now lives in deploy-agent-dialog.tsx with zero test coverage — no test references deploy-agent-dialog, endUserSecret, or rotateAgentWidgetKey.
Fix: add coverage for the merged dialog, at minimum for secret/key rotation.

MINOR — src/xagent/web/api/widget.py:58,71 — stale comments name _resolve_verified_guest_id; the actual function is _resolve_authenticated_guest_id (defined :267, called :326). Rename in comments.

MINOR — frontend/src/components/build/deploy-agent-dialog.tsx:312-314handleCopyEndUserSecret calls navigator.clipboard.writeText directly, no success check / insecure-context fallback, unlike sibling handleCopyWidgetKey (:270-272) which uses copyToClipboard() from frontend/src/lib/clipboard.ts. On an insecure context / older browser the copy silently fails while a success toast still shows. Use copyToClipboard().

MINOR — frontend/src/components/build/deploy-agent-dialog.tsx:662 — advanced-options <summary> uses hover:text-black (not dark-theme aware); 5 sibling elements (e.g. :521) use hover:text-foreground. Wrong contrast in dark mode.

MINOR — frontend/public/widget.js:148endUserId.substring(0, 256) truncates the id in the URL client-side, but the embedding server signs the full id; an over-256-char id yields a signature mismatch → confusing 403 Invalid end-user signature. Bounded by the backend max_length=256 validator (not a security bypass) but a footgun. Reject over-length ids explicitly instead of silently truncating.

MINOR (D2) — dropped security-guidance string — the deleted agent-widget-settings-dialog.tsx rendered an inline warning next to the allowed-domains input (i18n key appWidget.dialog.allowedDomainsSecurityNote: allowed domains are a browser-level restriction, not a security boundary). The key and its rendering were deleted during the merge into deploy-agent-dialog.tsx with no replacement. Minor UX/doc regression — confirm the deletion was intentional.

Simplification opportunities

  • src/xagent/web/services/agent_store.py:49-57new_widget_key() and new_widget_end_user_secret() have byte-identical bodies (return secrets.token_urlsafe(32)), differ only in docstring; 5 call sites, none depend on distinctness. Merge into one new_widget_secret(). (~5 lines)
  • frontend/src/components/widget/public-agent-chat-page.tsx:44-51getOrCreateDirectFallbackGuestId() hand-rolls Math.random().toString(36) twice; the existing generateId() at frontend/src/lib/utils.ts:181-183 (already imported in this file, no format validation downstream) is a one-line drop-in. (~4 lines)
  • Clipboard dedupe is already counted under the MINOR finding above — not double-counted here.

Net: ~10 lines removed.

Testing

Backend: 7/7 pass in tests/web/api/test_widget_end_user_identity.py (happy path, cross-device resume, isolation, missing/forged/cross-identity signature rejection, either-required validation, reserved-namespace forge rejection, no-secret 403, rotation invalidation). Frontend: not runnable in the review environment (node_modules absent) — and per the MAJOR finding there is no test for the merged dialog to run anyway.

@yiboyasss yiboyasss force-pushed the feat/widget-end-user-scoping branch from c212768 to b854553 Compare July 13, 2026 02:46

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up re-review (round 2) — commit b854553e

Since the last (COMMENTED) review on c2127683, the author pushed b854553e fix(widget): address PR re-review findings on the end-user identity feature. That commit added real behavioral frontend test coverage for the deploy dialog, landed the outstanding minor fixes (stale comment, copy-helper reuse, dark-theme hover, client-side truncation → hard reject, restored security note, merged duplicate secret generators), and explicitly documented the D1 static-signature tradeoff in code. The remaining prior findings are resolved except two low-priority carry-forwards. One new MAJOR correctness issue surfaced this round (F1).

Design verdict

Sound — acceptable with reservations (reconfirmed). end_user_id is honored in exactly one place (_resolve_authenticated_guest_id), invoked unconditionally inside the sole widget-guest-token endpoint. Signed identities become guest_id = "verified_end_user:<id>", consistently scoped across read paths; reserved-namespace guard holds and is tested; missing-secret agents 403 on signed attempts; hmac.compare_digest used; rotation invalidates old signatures; migration DAG has exactly 1 head. The one known reservation (D1) is now documented as an accepted v1 tradeoff.

Prior findings checklist

  1. D1 — signature has no expiry/nonceWAIVED. Not implemented, but now documented as an accepted limitation (widget.py:270-286). Meets the "at minimum document it" bar; acceptable v1 tradeoff.
  2. MAJOR — frontend test coverage regressionFIXED. New deploy-agent-dialog.test.tsx (278 lines) with genuine behavioral assertions (fetch/display, rotate-with-confirm asserting value change, copy, decline-rotation negative path, collapse/expand DOM state, allowed-domain PUT payload). Residual gaps tracked as F2.
  3. MINOR — stale _resolve_verified_guest_id commentFIXED. Comments at widget.py:58,71 now match the real _resolve_authenticated_guest_id.
  4. MINOR — handleCopyEndUserSecret bypassed copyToClipboard()FIXED. Now calls the helper (deploy-agent-dialog.tsx:314). Two other copy handlers still bypass it — see Simplification.
  5. MINOR — hover:text-black not dark-theme awareFIXED. No occurrences remain; <summary> (line 668) uses hover:text-foreground.
  6. MINOR — widget.js client-side truncation vs full-string signatureFIXED (approach changed). IDs over 256 chars are now rejected (anonymous fallback + console error) instead of silently truncated (widget.js:119-132), eliminating the signature-mismatch footgun.
  7. MINOR (D2) — dropped allowed-domains security guidanceFIXED, with a new minor i18n regression. Warning restored (deploy-agent-dialog.tsx:603) but references a missing i18n key — see i18n gap below.
  8. Simplification — duplicate new_widget_key/new_widget_end_user_secretFIXED. Merged into new_widget_secret() (agent_store.py:49); all 5 call sites updated, no stragglers.
  9. Simplification — hand-rolled random id in getOrCreateDirectFallbackGuestIdNOT FIXED. public-agent-chat-page.tsx:44-49 unchanged. Low priority, carried forward.

New findings this round

F1 (MAJOR — correctness/availability) — src/xagent/web/api/widget.py:308-311. hmac.compare_digest(expected_signature, request.end_user_signature or "") raises TypeError: comparing strings with non-ASCII characters is not supported when end_user_signature contains any non-ASCII character. The field (widget.py:82) has only max_length=128, no charset validation, and authenticate_widget (widget.py:320-334) does not wrap the call in try/except. Reproduced: hmac.compare_digest('a'*62, 'ü'*20) raises. Impact: any client can turn a forged/invalid signature attempt into an unhandled 500 instead of the intended 403 by including one non-ASCII char — reachable via a single normal POST /auth, no special access. Not data exposure (auth still fails), but a trivially-triggered unhandled-exception path in a security-sensitive endpoint that breaks the "403 on invalid signature" contract the tests assert. Fix: validate end_user_signature against ^[0-9a-f]{64}$ (exact hex digest length) before comparing and return 403 on mismatch, or encode both sides to bytes before hmac.compare_digest.

Minor (test coverage):

  • F2 — deploy-agent-dialog.test.tsx: invalid-domain rejection (isValidAllowedDomain/HOST_PATTERN, agent-widget-config.ts:29) and domain removal are untested; only happy-path add is covered.
  • F3 — frontend/public/widget.js: no tests at all, despite new branching (>256 reject, end_user_id-without-signature fallback, URL assembly). Truncation fix (#6) has no regression guard.
  • F4 — tests/web/api/test_widget_end_user_identity.py: missing edge cases — empty/whitespace end_user_id, over-256-char id (422), the F1 non-ASCII-signature case, and precedence when both guest_id and end_user_id are supplied.

Trivial nits:

  • F5 — length-gating inconsistency: widget.js:122 counts UTF-16 code units, Pydantic max_length=256 (widget.py:78) counts Unicode code points, HMAC runs over UTF-8 bytes. Not exploitable (signature covers exact bytes sent); a multibyte id could pass one gate and error less actionably at another. Optional normalization.
  • F6 — migration date inversion: 20260710_add_agent_widget_end_user_secret.py's down_revision points to 20260711_add_trace_events_task_idx — child dated a day before its parent. Alembic resolves fine (1 head); optional rename for scan-by-filename clarity.

i18n gap (minor): the restored security note references deploy_agent.access_control.allowed_domains_security_note, which doesn't exist in en.ts or zh.ts; the component silently falls back to a hardcoded English string. English users see the warning, but zh-locale users get English instead of a translation. Fix: add the key to both locale files.

D2 (minor, design consistency): GET /agents/{id}/widget-end-user-secret (agents.py:983-988) auto-generates and persists a secret on read if missing — mirrors the existing widget-key GET pattern, so it's consistent with prior art, not a new inconsistency. Compounding effect: deploy-agent-dialog.tsx:127-136 fetches it eagerly in a useEffect gated only on activeView === "embed", so merely opening Deploy → Embed silently provisions a signing secret. Low impact, flagged for awareness.

Simplification opportunities

  • agents.py:900-1039yagni: get_agent_widget_end_user_secret/rotate_agent_widget_end_user_secret duplicate the control flow of the widget-key pair (owned-agent lookup, generate/rotate, 404/500), differing only in field name and response model. A _get_or_rotate_widget_secret(user_id, agent_id, field_name, response_cls, rotate) helper collapses ~120 lines to ~25 + 4 thin wrappers (decorators/docstrings/OpenAPI stay per-route). ~40-50 lines saved.
  • deploy-agent-dialog.tsx:306,346delete: handleCopySnippet and handleCopyShareLink call navigator.clipboard.writeText(...) directly instead of the imported copyToClipboard() (used by 3 of 5 handlers), missing the non-secure-context/legacy fallback. Swap both to copyToClipboard().
  • public-agent-chat-page.tsx:44-49stdlib: getOrCreateDirectFallbackGuestId() still hand-rolls Math.random().toString(36) twice (carried forward). Use crypto.randomUUID() or the existing generateId() (frontend/src/lib/utils.ts:186).
  • Dropped: agent-widget-config.ts:73-131's 4 fetch/rotate functions are each ~9-12 lines with distinct return types and one caller each — a generic helper trades 4 self-documenting names for magic-string dispatch. Net-negative on readability; not worth it.

Testing

  • Backend: 7/7 pass in tests/web/api/test_widget_end_user_identity.py (re-verified against current HEAD).
  • Alembic: exactly 1 migration head; chain resolves cleanly despite the rebase onto newer main.
  • Frontend: could not execute (no node_modules in the review environment); deploy-agent-dialog.test.tsx verified by careful static read and judged to contain real behavioral assertions.

Verdict

Solid progress — the test regression is genuinely closed and the minor cleanups landed. Please address F1 before merge (trivially-triggered 500 on a security endpoint, contradicts the invalid-signature 403 contract); the test-coverage minors (F2-F4) and the i18n gap are worth folding in while you're here. F5/F6, D2, and the simplifications are optional.

@yiboyasss yiboyasss marked this pull request as draft July 13, 2026 06:34

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR lets an embedding page bind a widget session to a real end user instead of an anonymous per-device guest_id. It adds an HMAC-SHA256-signed data-end-user-id attribute (signed server-side with a new owner-only widget_end_user_secret that mirrors the existing widget_key lifecycle and is never sent to the browser), a reserved-namespace guard so a "verified" identity can't be forged through the plain guest_id field, and a /tasks/latest endpoint so a signed identity resolves the same session across devices. This is the right security model: the server verifies a signed assertion rather than trusting client-supplied identity.

Approach: Sound

The signing/verification design is correct and the owner-only secret handling is appropriate. A few design-level notes carried forward — all informational, none blocking for v1:

  • D1 (static, non-expiring signature) — WAIVED. No timestamp/nonce, so a leaked (end_user_id, signature) pair is a standing impersonation token until secret rotation, and rotation invalidates all end-users at once (no per-user revocation). Now explicitly documented as an accepted v1 tradeoff in src/xagent/web/api/widget.py:276-282.
  • Memory not scoped to guest_id — not touched by this PR (widget agents default memory off); if widget-agent memory is ever enabled, all end-users of a widget would share the owner's namespace. Track separately.
  • No explicit tenant scoping — isolation relies on implicit agent_id/user_id boundaries; tenant_42:user_007-style end_user_id strings are convention, not enforced.
  • No anonymous → end-user migration path — pre-existing guest_xxx sessions can't be reattached once an embed adopts data-end-user-id. Acceptable for v1; worth a one-line callout in the PR description or docs.

Update summary

Since the last review, one commit landed (bc6ac3a2): fixed the MAJOR non-ASCII signature crash (F1), deduped the widget-secret endpoints behind a shared helper, and backfilled the missing tests.

Prior findings checklist

  1. FIXED — F1 (MAJOR): non-ASCII signature crash. src/xagent/web/api/widget.py:303-316 now encodes both expected_signature and request.end_user_signature to UTF-8 bytes before hmac.compare_digest, so a non-ASCII signature cleanly yields 403 instead of a TypeError/500. Regression test test_non_ascii_signature_is_rejected_not_a_server_error passes.
  2. FIXED — F2 (minor): missing domain-reject / domain-removal tests. deploy-agent-dialog.test.tsx:279-313 now covers both.
  3. FIXED — F3 (minor): widget.js had zero coverage. New public-widget-script.test.ts readFileSync+evals the real widget.js (authentic coverage, not a reimplementation).
  4. PARTIAL — F4 (minor): identity edge cases. Over-256→422, non-ASCII signature, and precedence are covered. Still missing: empty/whitespace end_user_id test.
  5. NOT FIXED — F5 (trivial): length-gate inconsistency. widget.js:122 counts UTF-16 code units; backend Pydantic max_length=256 counts code points. Not exploitable, just inconsistent.
  6. NOT FIXED — F6 (trivial): migration date inversion. down_revision dated a day before the child migration. Single alembic head confirmed, no functional issue.
  7. FIXED — i18n gap. allowed_domains_security_note now exists in both en.ts:3204 and zh.ts:3204.
  8. FIXED — endpoint dedup. New shared _get_or_rotate_widget_secret (agents.py:905-937) parameterized helper; 4 thin wrappers call it. ~40-50 lines saved.
  9. FIXED — clipboard handlers. All 5 handleCopy* now route through the shared copyToClipboard().
  10. NOT FIXED — getOrCreateDirectFallbackGuestId hand-rolled id. Now uses crypto.randomUUID() as primary path (improvement); Math.random() fallback unchanged. Low priority — the originally-suggested generateId() swap doesn't actually fit (weaker collision resistance).

New findings this round

None above minor/simplification level. The sole prior MAJOR (F1) is fixed and confirmed by a passing regression test.

Simplification opportunities

  • deploy-agent-dialog.tsx (handleCopy* handlers): all 5 repeat the same copy→feedback shape. Extract copyWithFeedback(value, setCopied, successMsg, errorMsg). (~10 lines)
  • public_chat_access.py:232-234 (pre-existing, out of this PR's diff) & :272-274 (added by this PR): is_(None) if x is None else == x ternary is redundant — Column.__eq__ already emits IS NULL for None (verified against SQLAlchemy 2.0.47). A bare Task.channel_id == access_context.channel_id is behavior-preserving. (~4 lines in the new function; the pre-existing sibling is optional drive-by cleanup, not in scope)
  • public-agent-chat-page.tsx:46-54 & widget.js:138-141: identical localStorage-cache + random-id pattern hand-duplicated in both files (comment says "exactly like widget.js does"). Drift risk — consider a shared helper.

net: ~-15 to -20 lines possible.

Testing

  • Backend (run this round): PYTHONPATH=src pytest tests/web/api/test_widget_end_user_identity.py10/10 passed, including the F1 regression test.
  • Frontend: verified by static read only — no node_modules in this review environment.

Verdict

The one MAJOR issue (F1, non-ASCII signature → 500) is fixed and confirmed by a passing regression test. Everything remaining is minor/trivial or a design note already accepted as a v1 tradeoff. Non-blocking follow-ups, all optional: F4 (empty-string test), F5/F6 (trivial nits), and the simplifications above.

Comment thread src/xagent/web/api/public_chat_access.py Outdated
Comment thread frontend/public/widget.js
Comment thread frontend/src/components/widget/public-agent-chat-page.tsx
Comment thread src/xagent/migrations/versions/20260710_add_agent_widget_end_user_secret.py Outdated
Comment thread frontend/src/components/build/deploy-agent-dialog.tsx Outdated
@yiboyasss yiboyasss force-pushed the feat/widget-end-user-scoping branch from bc6ac3a to 2b1bef3 Compare July 13, 2026 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(widget): scope widget sessions to a specific end user and tenant

3 participants