76 lines
6.2 KiB
Markdown
76 lines
6.2 KiB
Markdown
# Differential Security & Code Review Report
|
|
|
|
## 1. Executive Summary
|
|
This review covers the last 10 commits focusing on the ForgeLM Tauri React UI, Rust inference engine, and ForgeAgent integrations. The analysis prioritized high-risk areas including inference performance degradation, prompt leakage/injection, UI state management, and authentication/security boundaries.
|
|
|
|
**Overall Risk Level:** **MEDIUM**
|
|
The codebase introduces several defensive mechanisms against prompt leakage and UI state inconsistencies. However, it also introduces aggressive repetition penalties that may severely degrade code generation capabilities, and a prompt leakage regex that is overly broad and prone to false positives.
|
|
|
|
---
|
|
|
|
## 2. Detailed Findings by Risk Category
|
|
|
|
### 2.1 Inference Server & Performance Degradation (HIGH RISK)
|
|
**Files Affected:**
|
|
- `crates/inference-core/src/sampler.rs`
|
|
- `crates/inference-core/src/session.rs`
|
|
- `crates/inference-server/src/compat.rs`
|
|
- `crates/llama-sys/src/lib.rs`
|
|
|
|
**Analysis:**
|
|
1. **Aggressive Repetition Penalty:** In `crates/inference-server/src/compat.rs`, the `repeat_penalty` has been increased from `1.05` to `1.3`, and `repeat_last_n` set to `512` when no tools are provided.
|
|
- *Impact:* A repetition penalty of 1.3 is extremely aggressive for LLMs. While it prevents loops, it will severely degrade the model's ability to generate code, JSON, or any structured text that naturally requires repeating keywords, brackets, or variable names.
|
|
- *Recommendation:* Revert to `1.1` - `1.15` for general chat, or make the penalty configurable per-request.
|
|
2. **TPS Degradation Monitor is Passive:** `GenerationSession` now includes a TPS degradation monitor (`TpsDegradationConfig`).
|
|
- *Impact:* The monitor correctly identifies when TPS drops below 30.0 for 5 seconds, but it only emits a `tracing::warn!`. It does not abort the generation or notify the client, leaving the client hanging on a degraded session.
|
|
- *Recommendation:* Expose the degradation state to the server router to allow early termination or client-side warnings via SSE events.
|
|
|
|
### 2.2 Prompt Injection & Transcript Leakage (MEDIUM RISK)
|
|
**Files Affected:**
|
|
- `tauri-app/src/lib/chat.ts`
|
|
- `crates/inference-server/src/compat.rs`
|
|
|
|
**Analysis:**
|
|
1. **Overly Broad Regex Truncation:** To prevent the model from leaking transcript roles, the frontend introduced `TRANSCRIPT_LEAKAGE_PATTERN = /(?:^|\n)\s*(?:System|User|Assistant|Human):/i;`. The `stripTranscriptLeakage` function aggressively cuts off any text matching this regex.
|
|
- *Impact:* This regex will trigger false positives. If a user asks "What is the role of a System Administrator:" or the model generates a markdown list like:
|
|
```
|
|
Roles:
|
|
- User: Performs actions
|
|
- System: Manages resources
|
|
```
|
|
The response will be abruptly truncated at "User:".
|
|
- *Recommendation:* Rely strictly on exact backend `stop_sequences` (which was already implemented via `extra_stop_sequences` in `compat.rs`) rather than fuzzy frontend regex truncation, or significantly tighten the regex to expect specific formatting.
|
|
2. **Backend Stop Sequences:** The backend correctly injects `\nUser:`, `\nSystem:`, etc., into `extra_stop_sequences` for the `Generic` chat template. This is a secure and standard approach.
|
|
|
|
### 2.3 UI State Corruption (LOW/MEDIUM RISK)
|
|
**Files Affected:**
|
|
- `tauri-app/src/features/runtime/useRuntimeManager.ts`
|
|
- `tauri-app/src/components/ChatView.tsx`
|
|
|
|
**Analysis:**
|
|
1. **Hidden Side-Effects in State Management:** In `useRuntimeManager.ts`, an auto-scan was added inside `commitSavedSnapshot` via a `useRef` hook (`scanModelsRef.current?.(modelsDir)`).
|
|
- *Impact:* While this fixes a stale UI issue, calling an async state-mutating function from within another state reconciliation flow (inside a `.then` block) can lead to race conditions. If the user rapidly toggles settings, the delayed scan could overwrite a newer draft state.
|
|
- *Recommendation:* Decouple the model scanning from the snapshot commit, or use a dedicated `useEffect` that watches for `runtimeSnapshot.config.models_dir` changes.
|
|
2. **Massive ChatView Refactor:** The redesign to Manus-style tokens (`AgenticToggle`, `StreamingBubble`, etc.) successfully decomposes a monolithic component and appropriately isolates states. The `messageQueue` state is correctly maintained.
|
|
|
|
### 2.4 Auth & Multi-Agent ForgeAgent Integration (MEDIUM RISK)
|
|
**Files Affected:**
|
|
- `tauri-app/src-tauri/src/lib.rs`
|
|
|
|
**Analysis:**
|
|
1. **Missing Input Sanitization in Agent Prompts:** The `start_forgeagent_session_command` embeds the raw `prompt` directly into the JSON body sent to the local inference server:
|
|
```json
|
|
{ "role": "user", "content": &prompt }
|
|
```
|
|
- *Impact:* While this is a local server, if the ForgeAgent system relies on the model outputting a strict JSON schema (which it parses via `serde_json::from_str`), a prompt injection could force the model to break out of the JSON structure. The code handles this gracefully by falling back to treating the raw text as a summary (`ForgeAgentPlanCompletedPayload { summary: content }`), preventing crashes.
|
|
- *Recommendation:* Enforce JSON schema validation natively on the inference server using `response_format: { type: "json_object" }` if supported by the engine, rather than just appending "Limite a cadeia de raciocínio..." to the system prompt.
|
|
2. **Duplicate Error Handling Flow:** In `start_forgeagent_session_command`, if the runtime is not ready, it emits an error event via `app_handle.emit` AND returns an `Err("Runtime not ready")`. This may cause the frontend to display duplicate toast notifications or error states.
|
|
|
|
---
|
|
|
|
## 3. Summary of Recommendations
|
|
|
|
1. **Revert Repetition Penalty:** Lower `repeat_penalty` in `compat.rs` from `1.3` to `1.15` to restore coding and structured generation capabilities.
|
|
2. **Refine Leakage Prevention:** Remove the broad `TRANSCRIPT_LEAKAGE_PATTERN` regex in `chat.ts` and rely entirely on the exact `stop_sequences` enforced by the backend.
|
|
3. **Handle TPS Degradation:** Implement an active response to TPS degradation (e.g., aborting the stream with a specific error chunk) rather than just logging.
|
|
4. **Fix Race Conditions:** Move the model auto-scanning logic into a reactive `useEffect` tied to the active models directory rather than an imperative ref call during snapshot commits. |