fix: Use Path.is_relative_to() for path traversal validation#20
fix: Use Path.is_relative_to() for path traversal validation#20KHAEntertainment merged 3 commits intomasterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRefactors restore-serving logic to remove global server state and introduce explicit URL generation; hardens path resolution by replacing a relative_to-based guard with explicit Path.is_relative_to checks; cleans up imports; and adjusts pyproject optional-dependency format for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/ocbs/serve.py (4)
100-335:⚠️ Potential issue | 🔴 CriticalMultiple orphaned methods reference
selfoutside any class.Methods like
_get_checkpoint_info,_validate_token,_mark_proceeded,_write_proceed_notification,_send_webhook_notification,get_pending_proceed_notifications,clear_proceed_notification,_mark_used,_mark_restored,get_active_serves,serve_checkpoint,get_restore_url,_get_html_page,do_GET,do_POSTall referenceselfbut aren't inside a class definition.Additionally, these methods reference undefined names:
secrets(line 61)datetime,timedelta(lines 66-67)sqlite3(multiple locations)json(line 212)os(line 220)html_module(line 357)server_instance(lines 736, 744, 747)This entire section appears to be remnants of a class that was incompletely removed during refactoring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/serve.py` around lines 100 - 335, These functions were left as instance methods but are not inside any class and reference undefined names; restore them into the original request/serve class (e.g., place _get_checkpoint_info, _validate_token, _mark_proceeded, _write_proceed_notification, _send_webhook_notification, get_pending_proceed_notifications, clear_proceed_notification, _mark_used, _mark_restored, get_active_serves, serve_checkpoint, get_restore_url, _get_html_page, do_GET, do_POST back into the OCBS request/serve class so `self` is valid, or alternatively convert them to module-level functions and remove `self` usages; also add the missing imports (import secrets, from datetime import datetime,timedelta, import sqlite3, import json, import os, and ensure html_module is defined or replaced with the correct html library) and either define/replace the undefined server_instance references or remove them; update function signatures and any references accordingly so the code compiles and runtime names are defined.
838-841:⚠️ Potential issue | 🔴 CriticalCritical:
detect_connection_typeis called but never defined.The
__main__block callsdetect_connection_type()which doesn't exist in this file or any visible import. Running this file directly will fail withNameError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/serve.py` around lines 838 - 841, The __main__ block calls detect_connection_type() which is undefined; either add a proper implementation of the function or import it from the module that provides it. Implement a function named detect_connection_type that returns a tuple (conn_type, host) and handles errors (or raise a clear exception), or add an import like "from <module> import detect_connection_type" and ensure the symbol exists; update the main block to catch exceptions from detect_connection_type and print a helpful error if detection fails.
815-835:⚠️ Potential issue | 🔴 CriticalAdd missing imports for
threadingand define or importRestoreHandler.The
start_restore_serverfunction usesRestoreHandler(line 832) andthreading(line 833), but neither is imported or defined in this file. When called, this function will immediately crash with aNameError—like trying to drive a car with parts missing from the engine.You'll need to:
- Import the
threadingmodule at the top of the file- Either define
RestoreHandlerin this file or import it from wherever it's defined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/serve.py` around lines 815 - 835, The start_restore_server function references threading and RestoreHandler but neither is defined or imported, causing a NameError; add "import threading" near the other imports and either define RestoreHandler in this module or import it from its source (e.g., from mymodule.handlers import RestoreHandler) so start_restore_server can construct HTTPServer((bind_host, port), RestoreHandler) and spawn the thread; ensure the import name matches the RestoreHandler class used in start_restore_server.
59-98:⚠️ Potential issue | 🔴 CriticalMultiple critical runtime errors: undefined references and incorrectly nested methods.
The code has severe structural problems that will cause
NameErrorandNameErrorexceptions:
Lines 59-72: Methods
_generate_tokenand_create_serve_recordare indented inside theget_tailscale_ip()function, making them inaccessible local functions that referenceself—which doesn't exist in that scope. This looks like leftover code from a refactoring that removed aRestorePageServerclass.Line 832: References
RestoreHandlerclass that's never defined or imported anywhere in the codebase.Line 840: Calls
detect_connection_type()function that doesn't exist.Missing imports: The code uses
secrets,datetime/timedelta,sqlite3,threading, andhtmlmodules without importing them, causingNameErrorat runtime.Format mismatch:
format_restore_message()returns a placeholder message without actually generating or including a URL, breaking the user-facing output that the CLI and skill modules expect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/serve.py` around lines 59 - 98, The two methods _generate_token and _create_serve_record are accidentally nested inside get_tailscale_ip and reference self; move them out to be methods of the appropriate class (restore server class) or top-level functions as originally intended so they can access self and _active_tokens correctly, and ensure they use datetime/timedelta, secrets, and sqlite3 which you must import; define or import RestoreHandler and implement or import detect_connection_type where they are referenced (lines around 832/840) so calls resolve, and update format_restore_message to produce and return the actual restore URL (include html escaping via the html module if needed) instead of the placeholder text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ocbs/serve.py`:
- Line 9: The file currently only imports HTTPServer but is missing several
required imports used elsewhere (add imports at the top alongside HTTPServer):
secrets; from datetime import datetime, timedelta; sqlite3; json; os; import
html as html_module; and threading; update the import section so symbols like
HTTPServer and html_module and datetime/timedelta are available to the rest of
serve.py.
- Around line 844-856: The formatted restore message currently omits the actual
URL; change format_restore_message to call generate_restore_url(checkpoint_id,
port, host) and include the returned URL in the multi-line message (keeping the
original reason and explanatory text), so callers (skill.py and cli.py) that use
format_restore_message get a usable restore link; ensure signature and default
port/host are preserved so imports of generate_restore_url remain consistent.
---
Outside diff comments:
In `@src/ocbs/serve.py`:
- Around line 100-335: These functions were left as instance methods but are not
inside any class and reference undefined names; restore them into the original
request/serve class (e.g., place _get_checkpoint_info, _validate_token,
_mark_proceeded, _write_proceed_notification, _send_webhook_notification,
get_pending_proceed_notifications, clear_proceed_notification, _mark_used,
_mark_restored, get_active_serves, serve_checkpoint, get_restore_url,
_get_html_page, do_GET, do_POST back into the OCBS request/serve class so `self`
is valid, or alternatively convert them to module-level functions and remove
`self` usages; also add the missing imports (import secrets, from datetime
import datetime,timedelta, import sqlite3, import json, import os, and ensure
html_module is defined or replaced with the correct html library) and either
define/replace the undefined server_instance references or remove them; update
function signatures and any references accordingly so the code compiles and
runtime names are defined.
- Around line 838-841: The __main__ block calls detect_connection_type() which
is undefined; either add a proper implementation of the function or import it
from the module that provides it. Implement a function named
detect_connection_type that returns a tuple (conn_type, host) and handles errors
(or raise a clear exception), or add an import like "from <module> import
detect_connection_type" and ensure the symbol exists; update the main block to
catch exceptions from detect_connection_type and print a helpful error if
detection fails.
- Around line 815-835: The start_restore_server function references threading
and RestoreHandler but neither is defined or imported, causing a NameError; add
"import threading" near the other imports and either define RestoreHandler in
this module or import it from its source (e.g., from mymodule.handlers import
RestoreHandler) so start_restore_server can construct HTTPServer((bind_host,
port), RestoreHandler) and spawn the thread; ensure the import name matches the
RestoreHandler class used in start_restore_server.
- Around line 59-98: The two methods _generate_token and _create_serve_record
are accidentally nested inside get_tailscale_ip and reference self; move them
out to be methods of the appropriate class (restore server class) or top-level
functions as originally intended so they can access self and _active_tokens
correctly, and ensure they use datetime/timedelta, secrets, and sqlite3 which
you must import; define or import RestoreHandler and implement or import
detect_connection_type where they are referenced (lines around 832/840) so calls
resolve, and update format_restore_message to produce and return the actual
restore URL (include html escaping via the html module if needed) instead of the
placeholder text.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2658512-d032-4974-98cd-fdd4e6f8fe9c
📒 Files selected for processing (5)
pyproject.tomlsrc/ocbs/core.pysrc/ocbs/serve.pysrc/ocbs/skill.pytests/test_core.py
💤 Files with no reviewable changes (1)
- src/ocbs/skill.py
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Summary
try/except ValueErrorpattern withPath.is_relative_to()for cleaner path validation in_resolve_restore_path()Changes
is_relative_to()instead ofrelative_to()with exception handlingTestResolveRestorePathclass with 7 test cases covering normal paths,.openclawprefix stripping, absolute path rejection, empty path rejection, path traversal attempts, and symlink escape preventionSecurity Fix
Fixes path traversal vulnerability where a malicious database could supply paths like
../etc/passwdto escape the target directory during restore operations.Closes #12
Summary by CodeRabbit
New Features
Chores
Tests