-
Notifications
You must be signed in to change notification settings - Fork 99
Added retention policy UI for groups and public workspaces #730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,6 @@ | ||||||||||||||||||||||||||||||||||||||
| // static/js/public_workspace.js | ||||||||||||||||||||||||||||||||||||||
| import { showToast } from "./chat/chat-toast.js"; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
| import { showToast } from "./chat/chat-toast.js"; |
paullizer marked this conversation as resolved.
Show resolved
Hide resolved
paullizer marked this conversation as resolved.
Show resolved
Hide resolved
paullizer marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue was flagged in previous review (Comment 7) but has not been fixed. The select options include value "default", but this value is sent directly to the API endpoint which only accepts integers or 'none'. When a user tries to save while "default" is selected, the backend will reject it with a 400 error. Either map "default" to a backend-supported representation (e.g., omit the field or use null), or update the backend to handle "default" appropriately.
| const retentionData = { | |
| conversation_retention_days: convSelect.value, | |
| document_retention_days: docSelect.value | |
| }; | |
| const retentionData = {}; | |
| if (convSelect.value !== 'default') { | |
| retentionData.conversation_retention_days = convSelect.value; | |
| } | |
| if (docSelect.value !== 'default') { | |
| retentionData.document_retention_days = docSelect.value; | |
| } |
paullizer marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This XSS vulnerability was flagged in previous review (Comment 3) but has not been fixed. Setting innerHTML with unsanitized error.message allows potential DOM-based XSS if the error message contains HTML markup. The error message could originate from server responses or thrown exceptions. Use textContent for the error message portion or construct the element safely using DOM methods instead of template literals in innerHTML.
| statusSpan.innerHTML = `<span class="text-danger"><i class="bi bi-exclamation-circle-fill"></i> Error: ${error.message}</span>`; | |
| // Build the error status content safely without injecting HTML from error.message | |
| statusSpan.innerHTML = ''; | |
| const errorSpan = document.createElement('span'); | |
| errorSpan.className = 'text-danger'; | |
| const icon = document.createElement('i'); | |
| icon.className = 'bi bi-exclamation-circle-fill'; | |
| errorSpan.appendChild(icon); | |
| // Add a space and "Error: " label | |
| errorSpan.appendChild(document.createTextNode(' Error: ')); | |
| // Append the error message as text to avoid XSS | |
| const messageText = error && typeof error.message === 'string' ? error.message : 'Unknown error'; | |
| errorSpan.appendChild(document.createTextNode(messageText)); | |
| statusSpan.appendChild(errorSpan); |
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function savePublicRetentionSettings is called via inline onclick handler in the template (line 439 of public_workspaces.html), but this function is not exported to the window object. In ES6 modules, functions are not automatically global. You need to add window.savePublicRetentionSettings = savePublicRetentionSettings; and window.loadPublicRetentionSettings = loadPublicRetentionSettings; after the function definitions to make them accessible from inline onclick handlers. See how group_agents.js handles this pattern with window.fetchGroupAgents = fetchGroupAgents; (line 398).
| // Expose retention settings functions for inline onclick handlers in templates | |
| window.loadPublicRetentionSettings = loadPublicRetentionSettings; | |
| window.savePublicRetentionSettings = savePublicRetentionSettings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import path for showToast is incorrect. The file public_workspace.js is located in /static/js/public/, so the relative path ./chat/chat-toast.js would try to load from /static/js/public/chat/ which doesn't exist. The correct relative path should be ../chat/chat-toast.js to go up one directory level to /static/js/ and then into the chat/ subdirectory.