Conversation
There was a problem hiding this comment.
Pull request overview
Hotfix 1.5.9 updates frontend download behavior and platform-admin log date handling, and replaces the orchestrator-based conversation export with a backend-implemented export that uploads shareable exports to blob storage.
Changes:
- Gallery downloads now request a blob SAS URL and open it directly in a new tab.
- Platform admin user-activity log queries now convert start/end dates to local-day Unix timestamps (start-of-day / end-of-day).
- Added a new backend conversation export implementation and wired
/api/conversations/exportto use it.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| frontend/src/pages/gallery/Gallery.tsx | Switches downloads from /api/download redirect to generating a SAS URL via the API. |
| frontend/src/api/api.ts | Adds local start/end-of-day timestamp helpers and uses them for user-activity log query params. |
| backend/shared/conversation_export.py | New module to render conversation exports (HTML/JSON), embed images, upload to blob storage, and return a share URL. |
| backend/app.py | Replaces orchestrator call path for conversation export with the new in-process export implementation. |
| conversation_id = data.get("id") | ||
| user_id = data.get("user_id") | ||
| format = data.get("format", "html") | ||
|
|
||
| if not conversation_id or not user_id: | ||
| return jsonify({"error": "Missing conversation ID or user ID"}), 400 | ||
| if format not in ["html", "json"]: | ||
| return jsonify({"error": "Invalid export format. Supported formats: html, json"}), 400 | ||
|
|
||
| # Get the function key from Azure Key Vault | ||
| try: | ||
| keySecretName = "orchestrator-host--functionKey" | ||
| functionKey = clients.get_azure_key_vault_secret(keySecretName) | ||
| if not functionKey: | ||
| raise ValueError(f"Function key {keySecretName} is empty") | ||
| except Exception as e: | ||
| logging.exception( | ||
| "[webbackend] exception getting orchestrator function key" | ||
| ) | ||
| return ( | ||
| jsonify( | ||
| { | ||
| "error": f"Check orchestrator's function key was generated in Azure Portal and try again. ({keySecretName} not found in key vault)" | ||
| } | ||
| ), | ||
| 500, | ||
| ) | ||
|
|
||
| # Prepare the payload for the orchestrator | ||
| payload = json.dumps( | ||
| {"id": conversation_id, "user_id": user_id, "format": format} | ||
| ) | ||
|
|
||
| # Set up headers with the function key | ||
| headers = {"Content-Type": "application/json", "x-functions-key": functionKey} | ||
|
|
||
| # Call the orchestrator export endpoint | ||
| orchestrator_export_url = ORCHESTRATOR_URI + "/api/conversations" | ||
| response = requests.post(orchestrator_export_url, headers=headers, data=payload) | ||
|
|
||
| logging.info(f"[webbackend] Export response status: {response.status_code}") | ||
|
|
||
| if response.status_code != 200: | ||
| logging.error(f"[webbackend] Error from orchestrator: {response.text}") | ||
| return ( | ||
| jsonify({"error": "Error contacting orchestrator for export"}), | ||
| response.status_code, | ||
| ) | ||
| result = export_conversation(conversation_id, user_id, format) |
There was a problem hiding this comment.
user_id is taken from the request body and passed into export_conversation(...). Since get_conversation(conversation_id, user_id) uses the provided user_id as the Cosmos partition key, a logged-in caller could export another user's conversation by submitting that user's user_id + conversation ID. Please derive user_id from the authenticated principal (e.g., X-MS-CLIENT-PRINCIPAL-ID / get_client_principal()) and ignore or strictly validate the payload user_id.
| format = data.get("format", "html") | ||
|
|
There was a problem hiding this comment.
This handler uses a local variable named format, which shadows Python’s built-in format() function. Renaming it (e.g., export_format) would avoid confusion and make stack traces/debugging clearer.
| return jsonify({"error": result.get("error", "Conversation export failed")}), 500 | ||
|
|
There was a problem hiding this comment.
On failure, this endpoint always returns HTTP 500 with result.get("error"), including cases like “Conversation not found or access denied”. That makes it hard for clients to distinguish user errors (404/403/400) from server errors. Please map expected failure modes to appropriate status codes (e.g., 404 when not found, 403 when access denied, 400 for invalid input) and reserve 500 for unexpected exceptions.
| return jsonify({"error": result.get("error", "Conversation export failed")}), 500 | |
| error_message = result.get("error", "Conversation export failed") | |
| normalized_error = str(error_message).lower() | |
| if "not found" in normalized_error: | |
| status_code = 404 | |
| elif ( | |
| "access denied" in normalized_error | |
| or "forbidden" in normalized_error | |
| or "unauthorized" in normalized_error | |
| ): | |
| status_code = 403 | |
| elif "invalid" in normalized_error or "bad request" in normalized_error: | |
| status_code = 400 | |
| else: | |
| status_code = 500 | |
| return jsonify({"error": error_message}), status_code |
| def parse_markdown_to_html(text): | ||
| """ | ||
| Convert markdown content to HTML. | ||
| """ | ||
| if not text: | ||
| return "" | ||
|
|
||
| text = embed_images_in_markdown(text) | ||
|
|
||
| md = markdown.Markdown( | ||
| extensions=[ | ||
| "fenced_code", | ||
| "tables", | ||
| "toc", | ||
| "nl2br", | ||
| "sane_lists", | ||
| ] | ||
| ) | ||
| return md.convert(text) |
There was a problem hiding this comment.
parse_markdown_to_html converts untrusted conversation content directly to HTML with Python-Markdown, which by default allows raw HTML. Since the exported file is served as text/html via a shareable SAS URL, this is an XSS vector (e.g., a user message containing <script> or event-handler attributes). Please sanitize the rendered HTML (e.g., via bleach allowlist) and/or disable raw HTML passthrough before embedding into the export template.
| def fetch_image_from_blob(image_path): | ||
| """ | ||
| Fetch image bytes from Blob Storage. | ||
| """ | ||
| try: | ||
| blob_service_client = get_blob_service_client() | ||
| blob_client = blob_service_client.get_blob_client( | ||
| container="documents", blob=image_path | ||
| ) | ||
| image_data = blob_client.download_blob().readall() | ||
| logging.info( | ||
| "Successfully fetched image from container documents: %s", image_path | ||
| ) | ||
| return image_data |
There was a problem hiding this comment.
fetch_image_from_blob(image_path) downloads blobs from the documents container based solely on the markdown image URL. Because image_url comes from conversation content, this allows exporting/embedding arbitrary blobs that the server identity can access (potential data exfiltration). Please validate that image_path is within an allowed prefix for the current user/conversation (and ideally reuse the same access checks used by /api/generate-sas-url) before downloading.
| blob_name=blob_path, | ||
| account_key=account_key, | ||
| permission=BlobSasPermissions(read=True), | ||
| expiry=datetime.now(timezone.utc) + timedelta(days=3652), |
There was a problem hiding this comment.
The SAS URL generated for exported conversations uses an expiry of timedelta(days=3652) (~10 years). This greatly increases blast radius if the URL leaks (it effectively becomes a long-lived public link). Please reduce the expiry to a short duration and/or implement a revocable share mechanism (e.g., short-lived SAS with re-generation, stored share records, or a backend proxy endpoint with auth).
| expiry=datetime.now(timezone.utc) + timedelta(days=3652), | |
| expiry=datetime.now(timezone.utc) + timedelta(days=1), |
| text-decoration: none; | ||
| border-bottom: 1px solid transparent; | ||
| transition: border-color 0.2s; | ||
| display: none; |
There was a problem hiding this comment.
The CSS rule for .content a sets display: none;, which hides all links in exported conversations. If links are expected to be visible/clickable in exports, this is likely unintended; consider removing display: none (or using a safer styling approach if the intent is to visually de-emphasize links).
| display: none; |
JIRA Ticket
PROJ-XXX
Description
[Describe your changes here]
Checklist