Enhance server, UI, and conversation features with various improvements#50
Closed
friuns2 wants to merge 22 commits intofriuns2:mainfrom
Closed
Enhance server, UI, and conversation features with various improvements#50friuns2 wants to merge 22 commits intofriuns2:mainfrom
friuns2 wants to merge 22 commits intofriuns2:mainfrom
Conversation
Review Summary by QodoComprehensive enhancement with context-aware runtime selection, thread compacting, MCP support, and improved UI/UX
WalkthroughsDescription• **Context-aware runtime selection**: Refactored model, reasoning effort, and speed mode selection to be per-thread instead of global with persistent storage • **Thread compacting support**: Implemented thread context compaction functionality with API calls and UI state management • **MCP tool calls and collaboration**: Added support for MCP tool calls and collaborative agent tool calls with new message types and UI components • **Git worktree detection**: Implemented git worktree listing and selection with read-only mode for alternate worktrees • **Session status overview**: Added API to expose session configuration details (model, reasoning, speed, MCP servers) • **Custom host binding**: Added --host CLI option and --strict-local-auth flag for flexible server configuration • **Secure transport detection**: Enhanced authentication with HTTPS detection via headers and configurable localhost trust • **Message editing**: Implemented past message edit functionality with dedicated composer panel and rollback support • **Slash commands**: Added /compact, /review, /fork, /resume, /model, /plan, /collab, /status, /mcp commands in composer • **Thread filtering**: Added thread status filter (all/running/new) with localStorage persistence and mark-all-seen capability • **Improved mobile detection**: Enhanced mobile layout detection with pointer capability checks and responsive adjustments • **Settings UI refactor**: Restructured settings panel with session status, MCP management, quota tracking, and removed legacy account management • **Live overlay enhancements**: Added activity summaries, background agents tracking, and timestamp updates • **Symlink conversion**: Replaced symlink creation with plain file conversion logic in skills routes • **Local launcher script**: New bash script for launching codexUI from local repository with Serena MCP compatibility Diagramflowchart LR
A["User Settings"] -->|"Configure model,<br/>reasoning, speed"| B["Context-aware<br/>Runtime Selection"]
B -->|"Per-thread storage"| C["useDesktopState"]
D["Thread Operations"] -->|"Compact context"| E["compactThread API"]
E -->|"Manage state"| C
F["MCP Servers"] -->|"Tool calls"| G["Message Types"]
H["Git Worktrees"] -->|"Select target"| I["ReviewPane"]
I -->|"Read-only mode"| J["Alternate Worktree"]
K["Composer"] -->|"Slash commands"| L["Enhanced UX"]
M["Thread List"] -->|"Filter & organize"| N["Status Filter"]
O["Session Config"] -->|"Display overview"| P["Settings Panel"]
File Changes1. src/composables/useDesktopState.ts
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+28
to
+37
| function requestUsesSecureTransport(req: Request): boolean { | ||
| if (req.secure) return true | ||
| const forwardedProto = req.headers['x-forwarded-proto'] | ||
| if (typeof forwardedProto === 'string') { | ||
| return forwardedProto.split(',').some((value) => value.trim().toLowerCase() === 'https') | ||
| } | ||
| if (Array.isArray(forwardedProto)) { | ||
| return forwardedProto.some((value) => value.trim().toLowerCase() === 'https') | ||
| } | ||
| return false |
There was a problem hiding this comment.
1. Spoofable secure auth cookie 🐞 Bug ⛨ Security
requestUsesSecureTransport() trusts the client-controlled x-forwarded-proto header even though the Express app does not enable trust proxy, so a plain-HTTP client can force the login handler to set a Secure cookie that the browser will never send back over HTTP (breaking auth). This can cause users to be stuck in the login page even with the correct password.
Agent Prompt
### Issue description
`src/server/authMiddleware.ts` sets the auth cookie with the `Secure` attribute based on `requestUsesSecureTransport()`, which currently trusts `x-forwarded-proto`. Because the Express app in `src/server/httpServer.ts` does not configure `trust proxy`, `x-forwarded-proto` must be treated as untrusted user input; spoofing it can cause the server to set a `Secure` cookie on an HTTP connection, breaking subsequent authentication.
### Issue Context
- If the app is not behind a trusted reverse proxy, `x-forwarded-proto` is client-controlled.
- If the app *is* behind a trusted reverse proxy, Express should be configured to trust that proxy so `req.secure` becomes the canonical signal.
### Fix Focus Areas
- src/server/authMiddleware.ts[28-37]
- src/server/authMiddleware.ts[141-146]
- src/server/httpServer.ts[72-86]
### Desired fix
- Stop directly trusting `x-forwarded-proto` unless the server is explicitly configured to trust proxies.
- Simplest safe change: make `requestUsesSecureTransport` return `req.secure` only.
- If proxy support is required: add an explicit `trustProxy`/`behindProxy` option, call `app.set('trust proxy', ...)` in `createServer`, and then rely on `req.secure` (which will incorporate `x-forwarded-proto` only when trusted).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1022
to
+1032
| await mkdir(skillsDir, { recursive: true }) | ||
| let content = '' | ||
| try { | ||
| const current = await lstat(codexAgentsPath) | ||
| if (current.isSymbolicLink()) { | ||
| const existingTarget = await readlink(codexAgentsPath) | ||
| if (existingTarget === relativeTarget) return | ||
| content = await readFile(codexAgentsPath, 'utf8') | ||
| } catch { | ||
| try { | ||
| content = await readFile(skillsAgentsPath, 'utf8') | ||
| } catch { | ||
| content = '' | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Symlink read during repair 🐞 Bug ⛨ Security
ensureCodexAgentsFilesArePlainFiles() identifies symlinks/non-files as "unsafe" via lstat(), but then calls readFile(codexAgentsPath) before removing the unsafe path, which follows symlinks and reads the target content. That content can then be written into AGENTS.md and later committed/pushed during skills sync, unintentionally persisting sensitive file contents.
Agent Prompt
### Issue description
`ensureCodexAgentsFilesArePlainFiles()` is meant to remediate "unsafe" AGENTS.md paths (symlinks, directories, etc.), but it currently reads from `codexAgentsPath` even when that path was classified as unsafe. Because `readFile()` follows symlinks, this dereferences the unsafe path and can ingest arbitrary file contents.
### Issue Context
- `getPathState()` uses `lstat()` and returns `unsafe` for any non-regular file.
- The current implementation reads `codexAgentsPath` first regardless of state, then deletes and rewrites files.
- Downstream, skills sync stages/commits the working tree, which can persist/push `AGENTS.md`.
### Fix Focus Areas
- src/server/skillsRoutes.ts[999-1042]
- src/server/skillsRoutes.ts[841-918]
### Desired fix
- Never `readFile()` from a path that is classified as `unsafe`.
- Choose content only from safe sources:
- If `codexAgentsState === 'file'`, read `codexAgentsPath`.
- Else if `skillsAgentsState === 'file'`, read `skillsAgentsPath`.
- Else use empty string.
- Optionally, cap file size read to avoid large-file issues.
- Then perform the removal + rewrite for any unsafe path(s) using the chosen safe `content`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Consolidate live stage summaries into the conversation timeline, hide redundant stage summary rows, and normalize reasoning effort values to the currently supported set.
Rework the composer control layout for mobile, keep Enter-to-send disabled on mobile, and hide the desktop review affordance from narrow layouts.
Track the new openspec change set in git and update manual verification coverage for the timeline and mobile composer work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.