Skip to content

Conversation

@Gkrumbach07
Copy link
Collaborator

No description provided.

This commit introduces a new endpoint for exporting session chat data in JSON format. The export includes AG-UI events and legacy messages, if available. The frontend has been updated to support this feature, allowing users to download the exported data directly from the session details modal.

Key changes:
- Added `HandleExportSession` function in the websocket package to handle export requests.
- Created a new API route for exporting sessions in `routes.go`.
- Implemented frontend logic to trigger the export and handle file downloads in `session-details-modal.tsx`.
- Updated session header component to pass project name for export functionality.

This enhancement improves data accessibility for users, enabling them to save session data for further analysis or record-keeping.
This commit replaces the use of `oc patch` commands with `oc set env` for updating environment variables in the GitHub workflows for frontend, backend, and operator deployments. This change simplifies the syntax and improves readability. Additionally, the frontend session details modal has been enhanced to manage export functionality more effectively, including loading export data and handling multiple export options for AG-UI events and legacy messages.
@github-actions

This comment has been minimized.

This commit adds security measures to the session export process in the backend, including user authentication, permission verification, and session name validation to prevent path traversal attacks. The frontend has been updated to utilize a new React Query hook for fetching export data, streamlining the export process in the session details modal. Additionally, the code has been refactored for better readability and maintainability.
@github-actions
Copy link
Contributor

github-actions bot commented Dec 18, 2025

Claude Code Review

Summary

This PR adds session export functionality, allowing users to download chat data as JSON. The implementation includes backend security measures, proper RBAC validation, React Query integration, and enhanced workflow deployment scripts. Overall implementation is strong with excellent security practices, though there are a few areas for improvement.

Issues by Severity

🚫 Blocker Issues

None - code is safe to merge after addressing critical issues below.

🔴 Critical Issues

1. Missing splitLines function in export.go

  • Location: components/backend/websocket/export.go:208
  • Issue: splitLines(data) is called but never defined in the file
  • Impact: Code will not compile
  • Fix: Add the splitLines function or use bytes.Split(data, []byte("\n"))

2. Type safety issue in frontend export response

  • Location: components/frontend/src/services/api/sessions.ts:186
  • Issue: aguiEvents: unknown[] violates zero-any policy (should be typed)
  • Impact: Loses type safety, makes it harder to catch bugs
  • Fix: Define proper types or use a more specific interface

3. Missing error handling in frontend proxy

  • Location: components/frontend/src/app/api/projects/[name]/agentic-sessions/[sessionName]/export/route.ts:24
  • Issue: No check for resp.ok before processing response
  • Impact: Errors from backend may not be properly propagated
  • Fix: Add error handling:
if (!resp.ok) {
  return new Response(data, { status: resp.status })
}

🟡 Major Issues

4. Potential XSS in filename

  • Location: components/backend/websocket/export.go:157
  • Issue: sessionName used in Content-Disposition header without sanitization
  • Impact: Could allow header injection attacks if validation fails
  • Recommendation: Use fmt.Sprintf("attachment; filename=\"%s-export.json\"", sessionName) (add quotes) and ensure sessionName validation is bulletproof

5. Memory efficiency concern

  • Location: components/backend/websocket/export.go:107-125
  • Issue: Loading entire JSONL files into memory, then pretty-printing them
  • Impact: Large session exports could cause memory spikes
  • Recommendation: Consider streaming response or adding size limits

6. Error propagation in modal

  • Location: components/frontend/src/components/session-details-modal.tsx:73-93
  • Issue: No error handling in download callbacks - errors are silently caught
  • Impact: Users won't know if export fails
  • Fix: Add error toast:
} catch (error) {
  errorToast('Failed to export chat')
  throw error
} finally {

🔵 Minor Issues

7. Inconsistent error logging

  • Location: components/backend/websocket/export.go:141
  • Issue: Legacy message errors are logged as warnings, but still non-fatal
  • Recommendation: Consider logging level (Info vs Warning) - seems appropriate as-is

8. Unused sessionName parameter check

  • Location: components/backend/websocket/export.go:70-73
  • Issue: Validation happens after RBAC check - optimization opportunity
  • Recommendation: Move validation before RBAC to fail fast

9. React Query cache time

  • Location: components/frontend/src/services/queries/use-sessions.ts:322
  • Issue: 60-second staleTime seems long for export data
  • Recommendation: Export data rarely changes during modal lifetime, so this is actually fine - consider reducing if users report stale data

Positive Highlights

Excellent Security Implementation

  • Proper user token authentication (GetK8sClientsForRequest)
  • RBAC checks before file access
  • Path traversal prevention with multiple layers
  • Input validation following K8s DNS label standards

Clean Architecture

  • Follows established backend patterns perfectly
  • Proper separation of concerns (API layer, React Query hooks, UI)
  • Consistent with project structure

React Query Best Practices

  • Proper query key structure
  • Conditional fetching (enabled only when modal open)
  • Good caching strategy

User Experience

  • Loading states on buttons
  • Conditional legacy export button
  • Helpful toast notifications

Code Quality

  • Well-commented security checks
  • Clear function names
  • Proper error messages

Recommendations

Priority 1 (Must Fix)

  1. Add missing splitLines function to export.go
  2. Fix type safety in SessionExportResponse
  3. Add error handling to frontend proxy route

Priority 2 (Should Fix)
4. Add error handling to download callbacks in modal
5. Consider adding file size limits to prevent memory issues

Priority 3 (Nice to Have)
6. Move sessionName validation before RBAC check
7. Add quotes around filename in Content-Disposition header

Security Review Checklist

✅ User token authentication required
✅ RBAC checks performed
✅ Path traversal prevention implemented
✅ Input validation (K8s DNS label format)
✅ No tokens in logs
✅ Proper error messages (no internal details exposed)
⚠️ Consider sanitizing filename in Content-Disposition header

Testing Recommendations

Before merging, verify:

  • Export works with sessions that have only AG-UI events
  • Export works with sessions that have legacy messages
  • Export returns 404 for non-existent sessions
  • Export returns 403 for unauthorized access
  • Frontend handles backend errors gracefully
  • Large session exports don't cause memory issues

Overall Assessment: Strong implementation with excellent security practices. Fix the compilation error and type safety issues, then this is ready to merge. The backend security implementation is exemplary and follows all project patterns correctly.


🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

  1. CLAUDE.md - Master project instructions, development standards
  2. backend-development.md - Go backend, K8s integration patterns
  3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
  4. security-standards.md - Auth, RBAC, token handling
  5. k8s-client-usage.md - User token vs service account patterns
  6. error-handling.md - Consistent error patterns
  7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.

@Gkrumbach07 Gkrumbach07 merged commit d2bab0e into ambient-code:main Dec 18, 2025
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant