Development#19
Open
khulnasoft-bot wants to merge 9 commits into
Open
Conversation
added 9 commits
April 1, 2026 10:19
Reviewer's GuideIntroduce development-focused behavior (debug logging, dev flags, Docker flows) and robustness improvements (plugin dir existence checks, safer file browsing and uploads), alongside formatting cleanups and some dependency/Docker build updates. Sequence diagram for user message upload and extension processingsequenceDiagram
actor User
participant WebUI as WebUI_API_message_communicate
participant Request as FastAPI_Request
participant Files as helpers_files
participant Ext as extension_system
participant MQ as message_queue
participant Agent as AgentContext
User->>WebUI: POST /api/message (form + attachments)
WebUI->>Request: request.files.getlist("attachments")
alt attachments_present
WebUI->>Files: get_abs_path("usr/uploads")
WebUI->>Files: os.makedirs(upload_folder_ext, exist_ok=True)
loop for each attachment
WebUI->>Files: save attachment to upload_folder_ext
WebUI->>Files: build path (upload_folder_int or upload_folder_ext)
WebUI-->>WebUI: append to attachment_paths
end
end
WebUI-->>WebUI: data = {message, attachment_paths}
WebUI->>Ext: call_extensions_async("user_message_ui", agent=context.get_agent(), data)
Ext-->>WebUI: possibly modified data
WebUI-->>WebUI: update message, attachment_paths from data
WebUI->>MQ: log_user_message(context, message, attachment_paths, message_id)
WebUI->>Agent: context.communicate(UserMessage(message, attachments, id))
Agent-->>WebUI: response
WebUI-->>User: response
Sequence diagram for secure plugin asset serving in the web UIsequenceDiagram
actor Browser
participant WebUI as run_ui__serve_plugin_asset
participant Plugins as helpers_plugins
participant Files as helpers_files
Browser->>WebUI: GET /usr/plugins/<plugin_name>/<asset_path>
WebUI->>Plugins: find_plugin_dir(plugin_name)
alt plugin_dir_not_found
Plugins-->>WebUI: None
WebUI-->>Browser: 404 Plugin not found
else plugin_dir_found
Plugins-->>WebUI: plugin_dir
WebUI->>Files: get_abs_path(plugin_dir, asset_path) as asset_file
WebUI->>Files: get_abs_path(plugin_dir, "webui") as webui_dir
WebUI->>Files: get_abs_path(plugin_dir, "extensions/webui") as webui_extensions_dir
WebUI->>Files: is_in_dir(asset_file, webui_dir)
WebUI->>Files: is_in_dir(asset_file, webui_extensions_dir)
alt not_in_allowed_dirs
WebUI-->>Browser: 403 Access denied
else in_allowed_dirs
WebUI->>Files: is_file(asset_file)
alt not_a_file
WebUI-->>Browser: 404 Asset not found
else is_file
WebUI-->>Browser: send_file(asset_file)
end
end
end
Updated class diagram for FileBrowser and related helpersclassDiagram
class FileBrowser {
-Path base_dir
+ALLOWED_EXTENSIONS: dict
+MAX_FILE_SIZE: int
+MAX_UPLOAD_DIR_SIZE: int
+FileBrowser()
-_validate_path(path: Union_str_Path) Path
-_check_file_size(file) bool
-_calculate_directory_size(directory: Path) int
-_is_allowed_file(filename: str, file) bool
-_get_file_extension(filename: str) str
-_get_files_via_ls(full_path: Path) Tuple_List_Dict_List_Dict
-_get_file_type(filename: str) str
+get_files(current_path: str) dict
+read_file(path: str, max_size: int) dict
+upload_file(file, current_path: str) dict
+delete_file(path: str) dict
}
class RuntimeHelper {
+is_dockerized() bool
}
class FilesHelper {
+get_abs_path(*paths: str) str
}
class PrintStyle {
+error(message: str) void
}
FileBrowser --> RuntimeHelper : uses
FileBrowser --> FilesHelper : uses
FileBrowser --> PrintStyle : logs errors
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughIntroduces Docker base image build targets and multi-architecture support to Makefile; updates dependency versions in requirements.txt; adds conditional development-mode logging and configuration to run_ui.py; enhances plugin discovery with filesystem existence checks; refactors environment variables in supervisor configuration and application startup; normalizes code formatting across multiple Python files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 4 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
docker/run/fs/exe/self_update_manager.py::launch_ui_process, the UI is now launched with--dockerized=falseand--development=trueinside the Docker environment, which seems counter to the expected production/docker runtime behavior and may break path assumptions and logging defaults; consider keeping--dockerized=trueand making--developmentconfigurable instead of hardcoded. - The Supervisor config for
run_uinow hardcodesCTX0_DEBUG="true", CTX0_WS_DEBUG="true", LOGLEVEL="DEBUG", UVICORN_LOG_LEVEL="debug", which will force verbose logging in all environments; it would be safer to gate these on environment variables or a profile so production deployments can run with lower log levels.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `docker/run/fs/exe/self_update_manager.py::launch_ui_process`, the UI is now launched with `--dockerized=false` and `--development=true` inside the Docker environment, which seems counter to the expected production/docker runtime behavior and may break path assumptions and logging defaults; consider keeping `--dockerized=true` and making `--development` configurable instead of hardcoded.
- The Supervisor config for `run_ui` now hardcodes `CTX0_DEBUG="true", CTX0_WS_DEBUG="true", LOGLEVEL="DEBUG", UVICORN_LOG_LEVEL="debug"`, which will force verbose logging in all environments; it would be safer to gate these on environment variables or a profile so production deployments can run with lower log levels.
## Individual Comments
### Comment 1
<location path="helpers/file_browser.py" line_range="44" />
<code_context>
full_path = path.resolve()
if not str(full_path).startswith(str(self.base_dir)):
- raise ValueError(f"Access denied: path {full_path} is outside base directory {self.base_dir}")
+ raise ValueError(f"Access denied: path {full_path} is outside base directory {self.base_dir}")
return full_path
</code_context>
<issue_to_address>
**🚨 issue (security):** Strengthen the path validation to avoid false positives and potential path traversal issues.
Using `str(full_path).startswith(str(self.base_dir))` can both misclassify paths (e.g. `/ctx0_backup/foo` passes when `base_dir` is `/ctx0`) and leave room for traversal issues. Prefer comparing resolved paths, for example:
```python
full_path = full_path.resolve()
base_dir = self.base_dir.resolve()
if os.path.commonpath([full_path, base_dir]) != str(base_dir):
raise ValueError(...)
```
or `full_path.is_relative_to(base_dir)` on Python 3.9+ (with a fallback).
</issue_to_address>
### Comment 2
<location path="docker/run/fs/exe/self_update_manager.py" line_range="680-686" />
<code_context>
[
sys.executable,
str(repo_dir / "run_ui.py"),
- "--dockerized=true",
+ "--dockerized=false",
+ "--development=true",
"--port=80",
"--host=0.0.0.0",
],
</code_context>
<issue_to_address>
**issue (bug_risk):** Re-evaluate `--dockerized=false` in a Docker-managed UI process; it likely should stay dockerized.
Passing `--dockerized=false` here will make `runtime.is_dockerized()` return false for a process that’s actually running in a container, which can change paths and config (e.g., upload/base directories) and potentially break container-specific behavior. If the goal is just to enable dev logging, keep `--dockerized=true` and only add `--development=true` so the process still behaves as containerized code.
</issue_to_address>
### Comment 3
<location path="Makefile" line_range="169-173" />
<code_context>
+
+docker-base: ## Build base Docker image for current platform
+ @echo "$(BLUE)Building base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)"
+ docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --load .
+
+docker-base-push: ## Build and push multi-arch base Docker image
+ @echo "$(BLUE)Building and pushing multi-arch base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)"
+ docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --platform $(DOCKER_PLATFORMS) --push .
+
docker-build: ## Build Docker image
</code_context>
<issue_to_address>
**suggestion:** Avoid hard-coding the `buildx` builder name to reduce setup friction.
These targets currently depend on a preconfigured BuildKit builder named `mybuilder`, so they’ll fail on machines where that builder doesn’t exist.
Consider either:
- dropping the `--builder` flag and using the default builder, or
- making the builder name configurable (e.g. `DOCKER_BUILDER ?= mybuilder`) and documenting the requirement, or
- having the Makefile create/configure the builder if it’s missing.
That would make these targets more portable across different environments.
Suggested implementation:
```
docker-base: ## Build base Docker image for current platform
@echo "$(BLUE)Building base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)"
docker buildx build -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --load .
```
```
docker-base-push: ## Build and push multi-arch base Docker image
@echo "$(BLUE)Building and pushing multi-arch base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)"
docker buildx build -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --platform $(DOCKER_PLATFORMS) --push .
```
</issue_to_address>
### Comment 4
<location path="api/message.py" line_range="59" />
<code_context>
# call extension point, alow it to modify data
data = {"message": message, "attachment_paths": attachment_paths}
- await extension.call_extensions_async(
</code_context>
<issue_to_address>
**nitpick (typo):** Fix the typo in the comment for clarity.
Change `alow` to `allow` in the comment:
```python
# call extension point, allow it to modify data
```
```suggestion
# call extension point, allow it to modify data
```
</issue_to_address>
### Comment 5
<location path="docker/run/fs/exe/self_update_manager.py" line_range="680-687" />
<code_context>
return subprocess.Popen(
[
sys.executable,
str(repo_dir / "run_ui.py"),
"--dockerized=false",
"--development=true",
"--port=80",
"--host=0.0.0.0",
],
cwd=repo_dir,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -41,7 +42,7 @@ def _validate_path(self, path: Union[str, Path]) -> Path: | |||
| full_path = path.resolve() | |||
|
|
|||
| if not str(full_path).startswith(str(self.base_dir)): | |||
There was a problem hiding this comment.
🚨 issue (security): Strengthen the path validation to avoid false positives and potential path traversal issues.
Using str(full_path).startswith(str(self.base_dir)) can both misclassify paths (e.g. /ctx0_backup/foo passes when base_dir is /ctx0) and leave room for traversal issues. Prefer comparing resolved paths, for example:
full_path = full_path.resolve()
base_dir = self.base_dir.resolve()
if os.path.commonpath([full_path, base_dir]) != str(base_dir):
raise ValueError(...)or full_path.is_relative_to(base_dir) on Python 3.9+ (with a fallback).
Comment on lines
680
to
686
| [ | ||
| sys.executable, | ||
| str(repo_dir / "run_ui.py"), | ||
| "--dockerized=true", | ||
| "--dockerized=false", | ||
| "--development=true", | ||
| "--port=80", | ||
| "--host=0.0.0.0", |
There was a problem hiding this comment.
issue (bug_risk): Re-evaluate --dockerized=false in a Docker-managed UI process; it likely should stay dockerized.
Passing --dockerized=false here will make runtime.is_dockerized() return false for a process that’s actually running in a container, which can change paths and config (e.g., upload/base directories) and potentially break container-specific behavior. If the goal is just to enable dev logging, keep --dockerized=true and only add --development=true so the process still behaves as containerized code.
Comment on lines
+169
to
+173
| docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --load . | ||
|
|
||
| docker-base-push: ## Build and push multi-arch base Docker image | ||
| @echo "$(BLUE)Building and pushing multi-arch base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)" | ||
| docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --platform $(DOCKER_PLATFORMS) --push . |
There was a problem hiding this comment.
suggestion: Avoid hard-coding the buildx builder name to reduce setup friction.
These targets currently depend on a preconfigured BuildKit builder named mybuilder, so they’ll fail on machines where that builder doesn’t exist.
Consider either:
- dropping the
--builderflag and using the default builder, or - making the builder name configurable (e.g.
DOCKER_BUILDER ?= mybuilder) and documenting the requirement, or - having the Makefile create/configure the builder if it’s missing.
That would make these targets more portable across different environments.
Suggested implementation:
docker-base: ## Build base Docker image for current platform
@echo "$(BLUE)Building base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)"
docker buildx build -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --load .
docker-base-push: ## Build and push multi-arch base Docker image
@echo "$(BLUE)Building and pushing multi-arch base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)"
docker buildx build -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --platform $(DOCKER_PLATFORMS) --push .
| @@ -60,9 +58,7 @@ async def communicate(self, input: dict, request: Request): | |||
|
|
|||
| # call extension point, alow it to modify data | |||
There was a problem hiding this comment.
nitpick (typo): Fix the typo in the comment for clarity.
Change alow to allow in the comment:
# call extension point, allow it to modify data
Suggested change
| # call extension point, alow it to modify data | |
| # call extension point, allow it to modify data |
Comment on lines
680
to
687
| [ | ||
| sys.executable, | ||
| str(repo_dir / "run_ui.py"), | ||
| "--dockerized=true", | ||
| "--dockerized=false", | ||
| "--development=true", | ||
| "--port=80", | ||
| "--host=0.0.0.0", | ||
| ], |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/run/fs/exe/self_update_manager.py`:
- Around line 683-684: The UI launcher in self_update_manager.py currently
passes "--dockerized=false" and "--development=true"; change
"--dockerized=false" to "--dockerized=true" to reflect the container runtime
(matching the prepare.py invocation) and remove the redundant
"--development=true" flag because is_development() derives its value from
is_dockerized(); update the argument list where the UI is launched so it passes
"--dockerized=true" and omit "--development=true".
In `@helpers/file_browser.py`:
- Line 207: The subprocess.run call that executes ["ls", "-la", str(full_path)]
should use an absolute, validated path to the ls executable to avoid PATH-hijack
issues: resolve the executable with shutil.which("ls") (or hardcode a trusted
path like "/bin/ls"), check the result is not None and is an absolute path, then
call subprocess.run([resolved_ls, "-la", str(full_path)], capture_output=True,
text=True, timeout=30); update the code around the result = subprocess.run(...)
line in helpers/file_browser.py and add a clear error/exception path if the
resolver returns None.
- Around line 44-45: The current boundary check uses string startswith on
full_path and self.base_dir which is bypassable by prefix collisions; update the
check in the method that computes full_path (refer to variables full_path and
self.base_dir) to use resolved pathlib path comparison instead — resolve both
paths (e.g., full_path.resolve() and self.base_dir.resolve()) and either use
Path.relative_to(self.base_dir) in a try/except to ensure full_path is inside
the base directory or verify that self.base_dir is a parent of full_path by
checking self.base_dir in full_path.parents; raise the same ValueError if the
validation fails.
In `@helpers/plugins.py`:
- Around line 592-593: The code assumes find_plugin_dir(plugin_name) always
returns a path; guard that call in the functions using file_path/default_used
(where file_path = files.get_abs_path(find_plugin_dir(plugin_name),
CONFIG_DEFAULT_FILE_NAME) and the similar usage at the other occurrence) by
checking the result of find_plugin_dir(plugin_name) first, and if it is None
return None immediately for unknown plugins; ensure you only call
files.get_abs_path(...) when the plugin_dir is non-None and preserve setting
default_used = True only after confirming the plugin dir exists.
In `@Makefile`:
- Around line 169-173: The Makefile hardcodes "--builder mybuilder" in the
docker buildx invocations (see the top docker buildx build line and the
docker-base-push target), which breaks on clean CI/dev hosts; change these
invocations to use a configurable variable (e.g. DOCKER_BUILDX_BUILDER) or omit
the flag entirely so the default builder is used, and only pass "--builder
$(DOCKER_BUILDX_BUILDER)" when that variable is set (or document/ensure builder
creation separately); update the docker-base-push and the other build rule to
reference the variable instead of the literal "mybuilder".
In `@run_ui.py`:
- Around line 34-37: Move both logging and Socket.IO logger configuration out of
module import time and run them after runtime.initialize() is called: remove
calls to runtime.is_development() at top-level and instead invoke the existing
_configure_logging() (or create it if missing) from inside run() immediately
after calling runtime.initialize(), and similarly defer the Socket.IO logger
flag setup (the code currently at lines referencing Socket.IO config) until
after initialization; use runtime.is_development() only after
runtime.initialize() so the flags and levels reflect the initialized args.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74594a7a-40e5-46f9-9758-1db7648edecd
📒 Files selected for processing (9)
Makefileapi/message.pydocker/base/Dockerfiledocker/run/fs/etc/supervisor/conf.d/supervisord.confdocker/run/fs/exe/self_update_manager.pyhelpers/file_browser.pyhelpers/plugins.pyrequirements.txtrun_ui.py
Comment on lines
+683
to
+684
| "--dockerized=false", | ||
| "--development=true", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify how docker/development flags are interpreted and consumed
rg -n -C3 'def is_dockerized|def is_development|def initialize|get_arg\(' helpers/runtime.py
rg -n -C3 '--dockerized|--development|is_dockerized\(|is_development\(' run_ui.py docker/run/fs/exe/self_update_manager.pyRepository: FortiShield/CtxAI
Length of output: 1353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check exact lines 683-684 in self_update_manager.py
echo "=== Lines 680-690 in self_update_manager.py ==="
sed -n '680,690p' docker/run/fs/exe/self_update_manager.py | cat -n
# Search for flag usage patterns separately
echo -e "\n=== Flag usage in self_update_manager.py ==="
rg -n 'dockerized|development' docker/run/fs/exe/self_update_manager.py
# Search for how this is invoked from run_ui or other places
echo -e "\n=== References to self_update_manager ==="
rg -n 'self_update_manager' --type pyRepository: FortiShield/CtxAI
Length of output: 3658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get broader context around lines 674 and 683
echo "=== Context around line 674 (prepare_script) ==="
sed -n '670,680p' docker/run/fs/exe/self_update_manager.py | cat -n
echo -e "\n=== Context around line 683 (run_ui) ==="
sed -n '675,695p' docker/run/fs/exe/self_update_manager.py | cat -n
# Search for function definition containing these lines
echo -e "\n=== Function containing these lines ==="
rg -n -B20 'run_command.*prepare_script.*--dockerized=true' docker/run/fs/exe/self_update_manager.py | head -40
# Check run_ui.py to see how it uses these flags
echo -e "\n=== run_ui.py usage of dockerized and development ==="
rg -n 'dockerized|development' run_ui.py | head -20Repository: FortiShield/CtxAI
Length of output: 2651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check where launch_ui_process is called from
echo "=== Where is launch_ui_process called? ==="
rg -n 'launch_ui_process' docker/run/fs/exe/self_update_manager.py
# Check the broader context of the docker-run-ui command
echo -e "\n=== docker-run-ui command context ==="
rg -n -B5 -A10 'docker-run-ui' docker/run/fs/exe/self_update_manager.py | head -50
# Check if there are any other UI launches and how they set these flags
echo -e "\n=== Other run_ui.py invocations ==="
rg -n 'run_ui.py' --type py
# Check run_ui.py itself to see how it determines if it should run in dev mode
echo -e "\n=== run_ui.py initialization of runtime args ==="
head -50 run_ui.pyRepository: FortiShield/CtxAI
Length of output: 3509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full docker_run_ui function to understand the overall flow
echo "=== docker_run_ui function ==="
rg -n -A50 'def docker_run_ui\(' docker/run/fs/exe/self_update_manager.py | head -70
# Check if there are comments or documentation about these flags
echo -e "\n=== Documentation/comments about dockerized flag ==="
rg -n -B2 -A2 'dockerized' docker/run/fs/exe/self_update_manager.py
# Check if run_ui.py is ever launched with --dockerized=true
echo -e "\n=== All run_ui.py invocations with flags ==="
rg -n -A10 'run_ui.py' docker/run/fs/exe/self_update_manager.pyRepository: FortiShield/CtxAI
Length of output: 3381
Set --dockerized=true to match the actual runtime environment.
The self_update_manager runs inside a docker container (as indicated by its path in docker/run/fs/exe/). Setting --dockerized=false when launching the UI creates a runtime mismatch that can break environment-specific behavior, path handling, and feature detection. Additionally, line 674 correctly uses --dockerized=true for prepare.py, but line 683 contradicts this for the UI launcher.
Since is_development() in runtime.py returns not is_dockerized(), the explicit --development=true at line 684 is redundant once the dockerized flag is fixed.
🔧 Proposed fix
- "--dockerized=false",
- "--development=true",
+ "--dockerized=true",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "--dockerized=false", | |
| "--development=true", | |
| "--dockerized=true", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/run/fs/exe/self_update_manager.py` around lines 683 - 684, The UI
launcher in self_update_manager.py currently passes "--dockerized=false" and
"--development=true"; change "--dockerized=false" to "--dockerized=true" to
reflect the container runtime (matching the prepare.py invocation) and remove
the redundant "--development=true" flag because is_development() derives its
value from is_dockerized(); update the argument list where the UI is launched so
it passes "--dockerized=true" and omit "--development=true".
Comment on lines
44
to
+45
| if not str(full_path).startswith(str(self.base_dir)): | ||
| raise ValueError(f"Access denied: path {full_path} is outside base directory {self.base_dir}") | ||
| raise ValueError(f"Access denied: path {full_path} is outside base directory {self.base_dir}") |
There was a problem hiding this comment.
Path boundary check is bypassable with prefix collisions.
Line 44 uses a string startswith check, so paths like /ctx0_evil/... can pass when base dir is /ctx0. Use path-relative validation instead.
🔧 Proposed fix
- if not str(full_path).startswith(str(self.base_dir)):
+ try:
+ full_path.relative_to(self.base_dir)
+ except ValueError:
raise ValueError(f"Access denied: path {full_path} is outside base directory {self.base_dir}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not str(full_path).startswith(str(self.base_dir)): | |
| raise ValueError(f"Access denied: path {full_path} is outside base directory {self.base_dir}") | |
| raise ValueError(f"Access denied: path {full_path} is outside base directory {self.base_dir}") | |
| try: | |
| full_path.relative_to(self.base_dir) | |
| except ValueError: | |
| raise ValueError(f"Access denied: path {full_path} is outside base directory {self.base_dir}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helpers/file_browser.py` around lines 44 - 45, The current boundary check
uses string startswith on full_path and self.base_dir which is bypassable by
prefix collisions; update the check in the method that computes full_path (refer
to variables full_path and self.base_dir) to use resolved pathlib path
comparison instead — resolve both paths (e.g., full_path.resolve() and
self.base_dir.resolve()) and either use Path.relative_to(self.base_dir) in a
try/except to ensure full_path is inside the base directory or verify that
self.base_dir is a parent of full_path by checking self.base_dir in
full_path.parents; raise the same ValueError if the validation fails.
| text=True, | ||
| timeout=30 | ||
| ) | ||
| result = subprocess.run(["ls", "-la", str(full_path)], capture_output=True, text=True, timeout=30) |
There was a problem hiding this comment.
Harden subprocess executable resolution for ls.
Line 207 launches ls via PATH lookup. Use an absolute executable path to reduce PATH-hijack risk and satisfy S607.
🔧 Proposed fix
- result = subprocess.run(["ls", "-la", str(full_path)], capture_output=True, text=True, timeout=30)
+ result = subprocess.run(["/bin/ls", "-la", str(full_path)], capture_output=True, text=True, timeout=30)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| result = subprocess.run(["ls", "-la", str(full_path)], capture_output=True, text=True, timeout=30) | |
| result = subprocess.run(["/bin/ls", "-la", str(full_path)], capture_output=True, text=True, timeout=30) |
🧰 Tools
🪛 Ruff (0.15.7)
[error] 207-207: subprocess call: check for execution of untrusted input
(S603)
[error] 207-207: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helpers/file_browser.py` at line 207, The subprocess.run call that executes
["ls", "-la", str(full_path)] should use an absolute, validated path to the ls
executable to avoid PATH-hijack issues: resolve the executable with
shutil.which("ls") (or hardcode a trusted path like "/bin/ls"), check the result
is not None and is an absolute path, then call subprocess.run([resolved_ls,
"-la", str(full_path)], capture_output=True, text=True, timeout=30); update the
code around the result = subprocess.run(...) line in helpers/file_browser.py and
add a clear error/exception path if the resolver returns None.
Comment on lines
+592
to
593
| file_path = files.get_abs_path(find_plugin_dir(plugin_name), CONFIG_DEFAULT_FILE_NAME) | ||
| default_used = True |
There was a problem hiding this comment.
Guard find_plugin_dir(...) before building default-config paths.
If find_plugin_dir(plugin_name) returns None, these path builds can fail unexpectedly. Return None early for unknown plugins.
🔧 Proposed fix
def get_plugin_config(
@@
- if not file_path:
- file_path = files.get_abs_path(find_plugin_dir(plugin_name), CONFIG_DEFAULT_FILE_NAME)
+ if not file_path:
+ plugin_dir = find_plugin_dir(plugin_name)
+ if not plugin_dir:
+ return None
+ file_path = files.get_abs_path(plugin_dir, CONFIG_DEFAULT_FILE_NAME)
default_used = True
@@
def get_default_plugin_config(plugin_name: str):
- file_path = files.get_abs_path(find_plugin_dir(plugin_name), CONFIG_DEFAULT_FILE_NAME)
+ plugin_dir = find_plugin_dir(plugin_name)
+ if not plugin_dir:
+ return None
+ file_path = files.get_abs_path(plugin_dir, CONFIG_DEFAULT_FILE_NAME)Also applies to: 616-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helpers/plugins.py` around lines 592 - 593, The code assumes
find_plugin_dir(plugin_name) always returns a path; guard that call in the
functions using file_path/default_used (where file_path =
files.get_abs_path(find_plugin_dir(plugin_name), CONFIG_DEFAULT_FILE_NAME) and
the similar usage at the other occurrence) by checking the result of
find_plugin_dir(plugin_name) first, and if it is None return None immediately
for unknown plugins; ensure you only call files.get_abs_path(...) when the
plugin_dir is non-None and preserve setting default_used = True only after
confirming the plugin dir exists.
Comment on lines
+169
to
+173
| docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --load . | ||
|
|
||
| docker-base-push: ## Build and push multi-arch base Docker image | ||
| @echo "$(BLUE)Building and pushing multi-arch base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)" | ||
| docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --platform $(DOCKER_PLATFORMS) --push . |
There was a problem hiding this comment.
Avoid hardcoding buildx builder name.
Using --builder mybuilder assumes local pre-configuration and makes these targets fail in clean CI/dev environments.
🔧 Proposed fix
-docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --load .
+docker buildx build -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --load .
-docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --platform $(DOCKER_PLATFORMS) --push .
+docker buildx build -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --platform $(DOCKER_PLATFORMS) --push .📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --load . | |
| docker-base-push: ## Build and push multi-arch base Docker image | |
| @echo "$(BLUE)Building and pushing multi-arch base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)" | |
| docker buildx build --builder mybuilder -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --platform $(DOCKER_PLATFORMS) --push . | |
| docker buildx build -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --load . | |
| docker-base-push: ## Build and push multi-arch base Docker image | |
| `@echo` "$(BLUE)Building and pushing multi-arch base Docker image $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG)...$(NC)" | |
| docker buildx build -f docker/base/Dockerfile -t $(DOCKER_BASE_IMAGE):$(DOCKER_BASE_TAG) --platform $(DOCKER_PLATFORMS) --push . |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 169 - 173, The Makefile hardcodes "--builder
mybuilder" in the docker buildx invocations (see the top docker buildx build
line and the docker-base-push target), which breaks on clean CI/dev hosts;
change these invocations to use a configurable variable (e.g.
DOCKER_BUILDX_BUILDER) or omit the flag entirely so the default builder is used,
and only pass "--builder $(DOCKER_BUILDX_BUILDER)" when that variable is set (or
document/ensure builder creation separately); update the docker-base-push and
the other build rule to reference the variable instead of the literal
"mybuilder".
Comment on lines
+34
to
+37
| if runtime.is_development(): | ||
| logging.getLogger().setLevel(logging.DEBUG) | ||
| else: | ||
| logging.getLogger().setLevel(logging.WARNING) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'runtime\.is_development\(\)|runtime\.initialize\(' run_ui.py
rg -n -C3 'def initialize|def is_development' helpers/runtime.pyRepository: FortiShield/CtxAI
Length of output: 1707
Logging configuration happens before runtime initialization, causing incorrect development mode detection.
Lines 34–37 and 74–75 call runtime.is_development() at module import time, but runtime.initialize() (which sets the args global that is_development() depends on) isn't called until line 472. The logging level and Socket.IO logger flags will be determined using an uninitialized state.
Suggested fix
- if runtime.is_development():
- logging.getLogger().setLevel(logging.DEBUG)
- else:
- logging.getLogger().setLevel(logging.WARNING)
+ def _configure_logging() -> None:
+ logging.getLogger().setLevel(logging.DEBUG if runtime.is_development() else logging.WARNING)Call _configure_logging() after runtime.initialize() inside the run() function. Apply the same pattern to lines 74–75 by deferring Socket.IO configuration until after initialization is guaranteed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@run_ui.py` around lines 34 - 37, Move both logging and Socket.IO logger
configuration out of module import time and run them after runtime.initialize()
is called: remove calls to runtime.is_development() at top-level and instead
invoke the existing _configure_logging() (or create it if missing) from inside
run() immediately after calling runtime.initialize(), and similarly defer the
Socket.IO logger flag setup (the code currently at lines referencing Socket.IO
config) until after initialization; use runtime.is_development() only after
runtime.initialize() so the flags and levels reflect the initialized args.
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
1 participant
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.
Summary by Sourcery
Improve development-time observability and Docker workflows while hardening plugin, file browsing, and upload handling.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Chores