Conversation
Merged changes from main branch including: - Workflow visualization features (new WorkflowRenderer-based FlowChart) - Document conversion service - SSE event buffer repository - BM25 indexer and search features - Security updates (cryptography, pillow, authlib) - Feature flags (openfeature-sdk, cachetools) - Cloud storage support (google-cloud-storage, azure-storage-blob) Preserved chat-sharing branch features: - ShareButton component - SharedSessionPage - ShareRepository and share models Changes made during merge: - Updated SharedFlowChartPanel to use new WorkflowRenderer architecture - Replaced nanoid dependency with Python secrets module for share ID generation - Fixed alembic migration chain (share_links now depends on doc_conv_cache) - Updated SharedFlowChartPanel to use SharedVisualizerStepCard (no context dependency) - Fixed SharedSessionPage to display user messages from user_message field
…onDownloadOverride props, removed SharedArtifactPanel, and fixed message bubble styling to match ChatPage.
✅ FOSSA Guard: Licensing (
|
✅ FOSSA Guard: Vulnerability (
|
WhiteSource Policy Violation Summary✅︎ No Blocking Whitesource Policy Violations found in solaceai/solace-agent-mesh-ui-pr-1155! |
amir-ghasemi
left a comment
amir-ghasemi
left a comment
There was a problem hiding this comment.
PR Review: Chat Session Sharing
Found 4 issues (2 🔴 Critical, 2 🟡 Important). Great feature overall — the access control architecture is solid, but there are two security gaps in how it's wired together.
See inline comments for details.
|
|
||
| # Check access permissions | ||
| can_access, reason = share_link.can_be_accessed_by_user(user_id, user_email) | ||
| if not can_access: |
There was a problem hiding this comment.
🔴 Critical: User-specific access control is silently bypassed
When a share has specific users added (user-specific sharing), get_shared_session_view calls can_be_accessed_by_user without the shared_user_emails parameter. The method's logic only enforces user-specific access when that list is provided — without it, it falls through to the general require_authentication check.
Impact: If an owner sets up user-specific sharing but leaves require_authentication=False (the default), ANY unauthenticated user can view the session, completely bypassing the user-specific access list.
Current code:
can_access, reason = share_link.can_be_accessed_by_user(user_id, user_email)Required fix:
shared_user_emails = self.repository.find_share_user_emails(db, share_id)
can_access, reason = share_link.can_be_accessed_by_user(user_id, user_email, shared_user_emails)Note: check_user_access_to_share (line ~833) correctly fetches shared_user_emails — same pattern needed here.
|
|
||
| # Check access permissions | ||
| can_access, reason = share_link.can_be_accessed_by_user(user_id, user_email) | ||
| if not can_access: |
There was a problem hiding this comment.
🔴 Critical: Same user-specific access control bypass in the artifact endpoint
get_shared_artifact_content calls can_be_accessed_by_user directly on the entity without fetching shared_user_emails from the DB. This is the same bypass as in get_shared_session_view — a user with a public share link (require_authentication=False) can download artifacts even when the share was intended for specific users only.
Required fix:
shared_user_emails = share_repo.find_share_user_emails(db, share_id)
can_access, reason = share_link.can_be_accessed_by_user(user_id, user_email, shared_user_emails)| log.error(f"Failed to add user {email} to share {share_id}: {e}") | ||
|
|
||
| return BatchAddShareUsersResponse( | ||
| added_count=len(added_users), |
There was a problem hiding this comment.
🟡 Important: add_share_users never commits — user additions silently lost
This method calls repository.add_share_user() which only does db.flush(), but add_share_users never calls db.commit(). When the DB session ends, the inserts are rolled back. The caller (router) also doesn't commit.
Compare with create_share_link (line ~117) and delete_share_link (line ~308) which both call db.commit() after write operations.
Same issue exists in delete_share_users — delete_share_users_batch does a query delete but there's no db.commit() anywhere in the call chain.
Required fix: Add db.commit() at the end of both add_share_users and delete_share_users before returning the response.
| return request.state.user_id | ||
| return None | ||
| except: | ||
| return None |
There was a problem hiding this comment.
🟡 Important: Bare except: swallows KeyboardInterrupt and SystemExit
Bare except: catches all exceptions including BaseException subclasses like KeyboardInterrupt, SystemExit, and GeneratorExit. This can prevent the process from shutting down cleanly.
Same issue on the next except: block (~line 74).
Fix: Change both bare except: to except Exception:
# before
except:
return None
# after
except Exception:
return None
amir-ghasemi
left a comment
amir-ghasemi
left a comment
There was a problem hiding this comment.
PR Review: Chat Sharing Feature
Deep review covering security, behavioral correctness, architecture, and test coverage. Found 4 critical issues and 6 important issues — see inline comments below.
Summary
| Severity | Count | Categories |
|---|---|---|
| 🔴 Critical | 4 | Auth bypass, wrong HTTP codes, data corruption |
| 🟡 Important | 6 | Architecture, atomicity, DB integrity, missing tests |
All issues are unambiguous and need addressing before merge.
| Tuple of (can_access: bool, reason: str) | ||
| """ | ||
| # Check user-specific sharing first (if there are shared users) | ||
| if shared_user_emails: |
There was a problem hiding this comment.
🔴 Critical: Authorization Bypass — empty user list falls through to public access
When all users are removed from a user-specific share, shared_user_emails becomes [], which is falsy in Python. The if shared_user_emails: guard is skipped entirely, and the code falls through to the require_login check. Since user-specific shares are created with require_login=True, any authenticated user with the share URL can now access the session after the owner removes the last allowed user.
Exploit scenario:
- Owner creates a user-specific share (shared with
alice@company.com) - Owner later removes Alice via
DELETE /{share_id}/users shared_user_emailsis now[]- An attacker who has the URL logs in and accesses the full chat session
Required fix: Add an explicit access_type column to SharedLinkModel and check it directly. When access_type == "user-specific", deny all access when the user list is empty — do not fall through.
# Current (buggy):
if shared_user_emails:
...
# Falls through to require_login check when list is empty
# Fix option A: store access_type on the model and check it
if self.access_type == "user_specific":
if not user_email or user_email.lower() not in [e.lower() for e in shared_user_emails]:
return False, "not_shared_with_user"
return True, "user_specific"
# Fix option B: deny if no users remain on a user-specific share
if not shared_user_emails and self.require_login and not self.require_same_domain:
# Could be user-specific with all users removed — safest to deny
return False, "no_allowed_users"Missing test: test_share_access_control.py has no test for the scenario where all users are removed. Add: test_removing_last_user_from_user_specific_share_denies_access_to_all
| """ | ||
| import json as json_module | ||
|
|
||
| anonymized = task.copy() |
There was a problem hiding this comment.
🔴 Critical: Shallow copy mutates the original task's bubble metadata
task.copy() creates a shallow copy — nested objects like metadata and chat_bubbles are shared references. The subsequent bubble.pop("userId", None) / bubble.pop("sessionId", None) calls mutate the original task object's bubbles, not just the copy.
This causes two problems:
- Data corruption: The caller's original task dict is silently modified, stripping fields it may need elsewhere.
- Race condition: If the task is cached in memory and two concurrent requests (one from the owner, one from a public viewer) call
anonymize_chat_tasksimultaneously, the owner's view may lose itsuserId/sessionIdfields.
Required fix: Use copy.deepcopy(task) instead of task.copy().
| anonymized = task.copy() | |
| import copy | |
| anonymized = copy.deepcopy(task) |
Note: The existing test test_original_task_dict_is_not_mutated only checks the top-level user_id field. It does not verify that nested bubble metadata is unaffected. Add a test that confirms task["message_bubbles"][0]["metadata"]["userId"] is still present after calling anonymize_chat_task.
|
|
||
| return formatted_events | ||
|
|
||
| def _build_share_link_response(self, share_link: ShareLink, base_url: str, db: DBSession = None) -> ShareLinkResponse: |
There was a problem hiding this comment.
🔴 Critical: _build_share_link_response is never called with db — access_type is always wrong for user-specific shares
This method accepts an optional db: DBSession = None parameter and only computes has_shared_users when db is provided. But all 4 call sites omit db:
- Line 87:
self._build_share_link_response(existing, base_url)(create — existing link) - Line 126:
self._build_share_link_response(saved_link, base_url)(create — new link) - Line 168:
self._build_share_link_response(share_link, base_url)(get by session) - Line 274:
self._build_share_link_response(updated_link, base_url)(update)
As a result, has_shared_users is always False, and get_access_type(False) never returns "user-specific". Any share that has been set up with user-specific access will be reported as "public", "authenticated", or "domain-restricted" from these endpoints — causing the frontend to display the wrong sharing mode and hide the user list UI.
Fix: Pass db at all 4 call sites — all callers already have db in scope:
return self._build_share_link_response(saved_link, base_url, db=db)
| ) | ||
|
|
||
| except ValueError as e: | ||
| raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=str(e)) |
There was a problem hiding this comment.
🔴 Critical: "Not found" maps to HTTP 403 instead of 404
delete_share_link in the service raises ValueError for two distinct cases:
"Share link {share_id} not found"→ should be 404"Not authorized to delete this share link"→ should be 403
But the router maps all ValueError to 403, so a caller deleting a non-existent share ID receives a 403 with the message "Share link X not found" — semantically contradictory.
This is already fixed correctly in the three adjacent user management endpoints (lines 558–561, 597–600, 632–635) which inspect the message:
if "not found" in str(e).lower():
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e))
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=str(e))Apply the same pattern here:
| raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=str(e)) | |
| except ValueError as e: | |
| if "not found" in str(e).lower(): | |
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) | |
| raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=str(e)) |
| return updated_link | ||
|
|
||
| except ValueError as e: | ||
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) |
There was a problem hiding this comment.
🔴 Critical: update_share_link maps "not found" AND "not authorized" ValueError both to HTTP 400
The service raises ValueError for three cases in update_share_link:
"Share link {share_id} not found"→ should be 404"Not authorized to modify this share link"→ should be 403- Validation errors (invalid domains, etc.) → 400 is correct
All three are mapped to HTTP_400_BAD_REQUEST here. This means:
- A caller updating a non-existent share ID gets a misleading 400 (not 404)
- A caller who does not own the share gets a misleading 400 (not 403)
Fix: Apply the same string-inspection pattern used by the user management endpoints:
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) | |
| except ValueError as e: | |
| if "not found" in str(e).lower(): | |
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) | |
| if "not authorized" in str(e).lower(): | |
| raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail=str(e)) | |
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) |
| """ | ||
| try: | ||
| user_emails = [share.user_email for share in request.shares] | ||
| access_level = request.shares[0].access_level if request.shares else "RESOURCE_VIEWER" |
There was a problem hiding this comment.
🔴 Critical: Per-user access_level is silently discarded — only the first user's level is used for all
BatchAddShareUsersRequest contains a list of AddShareUserRequest objects, each carrying its own access_level. This line extracts only shares[0].access_level and passes it as the single level for ALL users in the batch. If a caller submits two users with different access levels (e.g., ADMIN and RESOURCE_VIEWER), both always receive the first user's level — the remaining are silently dropped.
Fix: Either enforce that all users in a batch must share the same access_level with a validation error if they differ, or thread per-user levels through to the service:
# Option A: validate uniformity upfront
levels = {s.access_level for s in request.shares}
if len(levels) > 1:
raise HTTPException(status_code=400, detail="All users in a batch must have the same access level")
access_level = next(iter(levels))
# Option B: pass (email, level) pairs to the service
user_pairs = [(s.user_email, s.access_level) for s in request.shares]
| try: | ||
| # First get the share link to verify access and get the original user_id | ||
| from ..repository.share_repository import ShareRepository | ||
| share_repo = ShareRepository() |
There was a problem hiding this comment.
🟡 Important: Router directly instantiates ShareRepository — architecture violation + redundant access check
This route handler already receives share_service: ShareService via DI (line ~353), and ShareService already holds self.repository. Directly constructing ShareRepository() bypasses the service layer.
Additionally, the manual access check here (lines 370–388) is fully duplicated by the subsequent share_service.get_shared_session_view(...) call at line ~391, which independently re-fetches the share link and re-runs can_be_accessed_by_user. The first check provides no protection that the second does not already provide.
Fix: Remove the ShareRepository import from the router and replace lines 370–388 with a call to the existing service method:
# share_service.check_user_access_to_share already encapsulates:
# find_by_share_id + find_share_user_emails + can_be_accessed_by_user
try:
await share_service.check_user_access_to_share(db, share_id, user_id, user_email)
except PermissionError as e:
if "Authentication required" in str(e):
raise HTTPException(status_code=401, detail=str(e))
raise HTTPException(status_code=403, detail=str(e))
except ValueError as e:
raise HTTPException(status_code=404, detail=str(e))
|
|
||
| # Save to database | ||
| saved_link = self.repository.save(db, share_link) | ||
| db.commit() |
There was a problem hiding this comment.
🟡 Important: Two db.commit() calls break atomicity in create_share_link
There are two commits in this method: one at line 117 (after saving the SharedLink) and one at line 121 (after _mark_session_artifacts_public). If _mark_session_artifacts_public raises an exception between them, the SharedLink row is permanently written but artifacts are not — the operation is partially complete with no rollback possible.
Remove the first commit at line 117; keep only the one after _mark_session_artifacts_public completes:
# Remove the db.commit() here (line 117)
await self._mark_session_artifacts_public(db, session_id, saved_link.share_id)
db.commit() # Single commit covers both operations atomicallyNote: _mark_session_artifacts_public is currently a stub (TODO). This is low-risk today but will become a real atomicity hazard once implemented.
| __tablename__ = "shared_artifacts" | ||
|
|
||
| id = Column(BigInteger, primary_key=True, autoincrement=True) | ||
| share_id = Column(String(21), nullable=False, index=True) |
There was a problem hiding this comment.
🟡 Important: Missing ForeignKey on SharedArtifactModel.share_id — orphaned artifacts possible
SharedLinkUserModel.share_id (line 40) correctly uses ForeignKey("shared_links.share_id", ondelete="CASCADE"). SharedArtifactModel.share_id has no such constraint, so:
- Artifacts can reference non-existent share links
- Deleting a share link does NOT cascade-delete its artifacts at the DB level
- Orphaned artifact rows can accumulate permanently if the application-level cleanup in
delete_share_linkis ever skipped
Today _mark_session_artifacts_public is a stub so shared_artifacts is never populated, but this needs to be fixed before that TODO is implemented.
Fix:
| share_id = Column(String(21), nullable=False, index=True) | |
| share_id = Column(String(21), ForeignKey("shared_links.share_id", ondelete="CASCADE"), nullable=False, index=True) |
Also add shared_artifacts to the SharedLinkModel relationship with cascade="all, delete-orphan".
| service.repository.find_by_share_id.return_value = make_share_link(user_id="real-owner") | ||
|
|
||
| with pytest.raises(ValueError, match="Not authorized"): | ||
| service.delete_share_users(MagicMock(), "share123", "attacker", ["alice@example.com"]) |
There was a problem hiding this comment.
🟡 Important: Critical missing tests — empty user list security regression and shallow copy mutation
The test suite has good coverage overall but is missing two high-priority scenarios:
1. Empty user list after delete_share_users (security regression)
There is no test verifying what happens when all users are removed from a user-specific share. Due to the if shared_user_emails: truthiness check in can_be_accessed_by_user, removing the last user causes the share to fall through to require_login — making it accessible to any authenticated user. This is the most critical untested path:
def test_removing_last_user_from_user_specific_share_denies_all_access():
"""Removing all users from a user-specific share must not open it to everyone."""
share = ShareLink(
share_id="test123", user_id="owner", session_id="sess1",
require_login=True, require_same_domain=False, # flags set by user-specific creation
deleted_at=None, allowed_domains=None,
)
# After all users removed, list is empty
can_access, reason = share.can_be_accessed_by_user(
user_id="attacker", user_email="attacker@example.com", shared_user_emails=[]
)
assert not can_access, "Share with no allowed users must deny everyone"2. Shallow copy mutation of bubble metadata
The existing test_original_task_dict_is_not_mutated only checks the top-level user_id. It does not verify that nested bubble metadata is unaffected. Add:
def test_original_task_bubble_metadata_is_not_mutated():
bubbles = [{"role": "user", "content": "Hi", "metadata": {"userId": "u1", "sessionId": "s1"}}]
task = {"id": "t1", "session_id": "sess1", "user_id": "uid", "message_bubbles": bubbles}
anonymize_chat_task(task)
assert task["message_bubbles"][0]["metadata"]["userId"] == "u1", "Original bubble must not be mutated"
assert task["message_bubbles"][0]["metadata"]["sessionId"] == "s1", "Original bubble must not be mutated"3. HTTP status coverage for DELETE /{share_id}
No test exercises the DELETE /{share_id} endpoint. Add tests verifying that a missing share returns 404 (not 403) and a non-owner delete returns 403.
…ring functionality
…ve user messaging
Combines viewer indicator feature with fork/share functionality: - Keep viewer indicator with UserLock icon and tooltip - Add fork & continue button for non-owners - Add go to chat button for owners - Keep side panel collapsed by default - Retain public access label as empty string for cleaner UI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
2 participants
This pull request introduces a new chat session sharing feature in the frontend, along with significant improvements to the artifact panel to support read-only and custom download behaviors. The main changes include a new API client for sharing, UI integration for sharing chat sessions, and enhanced artifact handling for shared or restricted contexts.
Session Sharing Feature:
shareApi.ts) to handle all chat sharing operations, including creating, retrieving, updating, deleting, and listing share links, as well as viewing shared sessions and fetching shared artifact content. Also includes utility functions for clipboard and access type display.ShareDialogcomponent for sharing sessions. [1] [2] [3] [4] [5] [6]Artifact Panel Enhancements:
ArtifactPanel.tsx) to support a read-only mode, hiding delete and edit actions when appropriate (e.g., in shared/public views). Also allows for a custom download handler to be injected, enabling support for downloading artifacts from shared sessions. [1] [2] [3] [4] [5] [6] [7]ArtifactCardandArtifactMessagecomponents to respect the newreadOnlyandonDownloadOverrideprops, ensuring correct behavior in both normal and shared contexts. [1] [2] [3]These changes lay the groundwork for robust session sharing and secure artifact access in read-only or public scenarios.
References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17]