Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Odoo module Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base_dav/controllers/main.py`:
- Around line 68-70: The nested start_response function is not WSGI-compliant
because it lacks the optional third exc_info parameter; change its signature in
main.py to accept exc_info (e.g., def start_response(status, headers,
exc_info=None):) and record exc_info into the existing status_headers mapping
(e.g., status_headers["exc_info"] = exc_info) so calls like
start_response(status, headers, exc_info) won’t raise a TypeError and the error
info is preserved.
- Line 51: The comment in main.py that reads "Radical 3.x requires wsgi.errors
and wsgi.input" contains a typo; change "Radical" to "Radicale" so the comment
reads "Radicale 3.x requires wsgi.errors and wsgi.input" — update the inline
comment near the wsgi.errors/wsgi.input note in the module (look around the
comment in base_dav.controllers.main).
- Around line 34-46: The Radicale Application is being recreated on every
request; instead create and cache a single Application instance (e.g.,
module-level RADICALE_APP) after building the configuration returned by
radicale_config.load() and calling configuration.update(...), then reuse that
cached Application for incoming requests in the controller; ensure the
configuration is built once and passed to Application only during the cached
creation (referencing Application, radicale_config.load, and
configuration.update) so subsequent requests use the same long-lived instance.
In `@base_dav/models/dav_collection_field_mapping.py`:
- Around line 120-123: The _from_vobject_code function currently returns
context.get("result", {}) which yields an empty dict by default and can be
passed into record.write()/record.create(), causing type errors; change the
return to use context.get("result", None) (matching _to_vobject_code) so missing
results yield None instead of {}. Locate the return after
safe_eval_mod.safe_eval in _from_vobject_code and replace the default {} with
None to ensure consistent behavior.
- Around line 128-139: The converter loop in DavCollectionFieldMapping builds
conversion_funcs and calls methods like _from_vobject_{self.field_id.ttype} via
getattr but uses "if value:" to decide success, which wrongly drops legitimate
falsy results (0, False, ""). Change the success check to test for None (e.g.,
"if value is not None") or use a distinct sentinel to detect "no conversion" so
that falsy but valid values returned by _from_vobject_* methods are preserved;
update the loop that assigns value and the final fallback to child.value
accordingly.
In `@base_dav/models/dav_collection.py`:
- Around line 243-248: The current code appends the new uuid to the original
href (href = f"{href}/{uuid}"), producing multi-segment refs; instead rebuild
the href by replacing the final path component with the new uuid (i.e.,
reconstruct href from the existing split components plus the new uuid) so the
created record's href is the collection path with the new id; modify the block
that uses components, uuid, href, self.field_uuid and collection_model.create to
set href = "/".join(components[:-1] + [uuid]) (or otherwise derive the
collection base path then append uuid) rather than appending onto the original
href.
- Around line 92-94: The method named eval in the class (defined in
dav_collection.py) shadows the Python built-in eval; rename the method to a
clearer name such as evaluate or eval_domain_records (e.g., def evaluate(self):
self.ensure_one(); return
self.env[self.model_id.model].search(self._eval_domain())); update all internal
references and imports to call the new name, and if you need backward
compatibility add a thin wrapper def eval(self): return self.evaluate() that
emits a deprecation warning before delegating to the new method; ensure you
update tests and any callers of eval to use the new symbol.
- Around line 170-172: Replace the platform-dependent os.path.normpath usage in
the _split_path method with posixpath.normpath so URL paths always use forward
slashes; update the imports to include posixpath, and keep the rest of
_split_path logic (strip("/"), split("/"), filter) intact, ensuring the symbol
references are _split_path and posixpath.normpath instead of os.path.normpath.
- Around line 116-124: The code currently drops legitimate falsy values by using
truthiness checks; update the checks in the methods that map fields so they only
exclude None. Specifically, in the method that builds result from vobject (the
loop using self.field_mapping_ids and mapping.from_vobject(child)) replace "if
value:" with "if value is not None" and make the same change in to_vobject (the
corresponding loop around line 141) so empty string, 0, and False are preserved
but None is still treated as missing.
In `@base_dav/radicale/auth.py`:
- Around line 5-67: The _login_ext method does not match Radicale 3.x and will
never be called; rename/replace it with def _login(self, login, password) ->
str, remove the unused context parameter, and ensure it returns the
authenticated username or "" on failure (keep the existing logic that calls res
= request.env["res.users"]._login(...) and _set_request_user(base_env, uid) but
adapt the _login call to only pass login/password and any needed DB info from
request.env). If you need HTTP headers (user agent / remote) extract them in a
new get_external_login(environ) implementation that uses the provided WSGI
environ and _user_agent_env(environ)-style logic, and do not rely on a context
arg inside _login; keep references to _user_agent_env and _set_request_user for
reuse.
In `@base_dav/radicale/collection.py`:
- Around line 88-97: The collection resolver currently accepts any principal
segment and calls env["dav.collection"].browse(...) unconditionally; change
__init__ so you extract the principal segment from self.path_components (e.g.,
principal = self.path_components[0] or similar) and compare it to the
authenticated principal in request.env (use env.get("uid") or env.get("user") as
available), and only call env["dav.collection"].browse(int(...)) and assign
self._record when the principal matches the authenticated principal; if it does
not match, skip the browse/list call and leave self._record as None (apply the
same guard where collection resolution is repeated in the later block referenced
around methods that call env["dav.collection"].browse in lines 117-139).
- Around line 20-45: Sanitize the incoming href before composing paths to
prevent path traversal/aliasing: in both _abs_href and _rel_href replace the
current h = (href or "").lstrip("/") (or h = href.lstrip("/") ) with h =
pathutils.strip_path(pathutils.sanitize_path(href or "")) so href is
normalized/cleaned before you compute prefix/pref and build the returned path;
keep the early empty-href behavior unchanged.
- Around line 160-166: The delete method currently returns silently when href is
None, violating the Radicale BaseCollection contract which expects deleting the
entire collection; change the behavior in Collection.delete (the def
delete(self, href: str | None = None) method) to raise NotImplementedError for
href is None (consistent with existing move() and create_collection() behavior)
instead of returning, and keep the existing path handling that computes abs_href
with _abs_href(self.path, href) and calls self._record.dav_delete(self,
abs_href) for non-None hrefs.
- Around line 195-227: The discover method accepts child_context_manager but
never uses it, which violates Radicale's contract; update discover to call
child_context_manager(...) as a context manager around each yielded child (both
for items returned by coll.get_all() and for children created from coll.list())
so callers can perform per-child locking/cleanup, e.g. wrap the creation/yield
of each child Collection or item in with child_context_manager(child_path,
href): before appending/yielding it; reference the discover function, the
Collection usage (coll, Collection(...)), and the iterations over coll.get_all()
and coll.list() to find where to add these context-manager wrappers, or
alternatively add a clear comment in discover explaining why per-child context
management is intentionally unnecessary.
In `@base_dav/radicale/rights.py`:
- Around line 6-20: The current try/except sets BaseRights = None which causes
class Rights(BaseRights) to raise TypeError on import; fix by either removing
the try/except and importing radicale.rights.BaseRights as required (if radicale
is mandatory), or provide a real fallback stub BaseRights when ImportError
occurs (e.g., define a minimal class BaseRights: pass with any methods Rights
relies on) so class Rights(BaseRights) can be defined; update load and _split
usages if needed to work with the stub.
In `@requirements.txt`:
- Line 5: Pin the radicale dependency in requirements.txt to a 3.x constraint to
avoid installing incompatible major releases; update requirements.txt to require
radicale==3.* (or >=3.0,<4.0) so the code in base_dav/radicale/ that extends
BaseAuth, BaseRights, BaseCollection and BaseStorage continues to match the
Radicale 3.x plugin APIs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
base_dav/security/ir.model.access.csvis excluded by!**/*.csvbase_dav/static/description/icon.pngis excluded by!**/*.png
📒 Files selected for processing (25)
base_dav/README.rstbase_dav/__init__.pybase_dav/__manifest__.pybase_dav/controllers/__init__.pybase_dav/controllers/main.pybase_dav/demo/dav_collection.xmlbase_dav/i18n/base_dav.potbase_dav/models/__init__.pybase_dav/models/dav_collection.pybase_dav/models/dav_collection_field_mapping.pybase_dav/pyproject.tomlbase_dav/radicale/__init__.pybase_dav/radicale/auth.pybase_dav/radicale/collection.pybase_dav/radicale/rights.pybase_dav/readme/CONFIGURE.mdbase_dav/readme/CONTRIBUTORS.mdbase_dav/readme/DESCRIPTION.mdbase_dav/readme/ROADMAP.mdbase_dav/static/description/index.htmlbase_dav/tests/__init__.pybase_dav/tests/test_base_dav.pybase_dav/tests/test_collection.pybase_dav/views/dav_collection.xmlrequirements.txt
tendil
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base_dav/controllers/main.py`:
- Around line 68-73: The WSGI response iterator returned by app(environ,
start_response) (bound to result_iter) may implement a close() method that must
be called to avoid resource leaks; wrap consumption of result_iter (used to
build response_body) in a try/finally: iterate/join into response_body in the
try and in the finally call result_iter.close() if hasattr(result_iter,
"close"), ensuring start_response and app semantics are preserved (references:
app, start_response, result_iter, response_body).
In `@base_dav/models/dav_collection_field_mapping.py`:
- Around line 221-223: The _to_vobject_binary method currently calls
value.decode("ascii") directly on a base64-encoded string; update it to handle
both str and bytes and to base64-decode before ASCII-decoding: if value is falsy
return None; if value is a str, base64.b64decode(value) then decode("ascii"); if
value is bytes, attempt base64.b64decode(value) and fall back to treating it as
raw bytes if decoding fails, then decode("ascii"); ensure base64 is imported and
reference the paired method _from_vobject_binary to confirm symmetry.
In `@base_dav/models/dav_collection.py`:
- Around line 296-305: The code is passing raw attachment.datas into FileItem
which expects an object with a .datas attribute, causing empty content; change
the return to pass the attachment record itself (the attachment variable) into
FileItem along with collection, href and last_modified (keep the existing
last_modified call _odoo_to_http_datetime(record.write_date)) so FileItem can
call getattr(attachment, "datas", None) and retrieve the actual data.
In `@base_dav/radicale/collection.py`:
- Around line 58-76: FileItem.__init__ currently assumes an attachment record
with a .datas attribute but callers pass raw base64 data (e.g. attachment.datas
or b""), causing text to be empty; fix by making FileItem.__init__ accept either
an attachment record or raw base64 data: check if the passed attachment has
attribute "datas" (getattr(attachment, "datas", None)) and use that, otherwise
treat attachment itself as the base64 payload (bytes/str), then base64.b64decode
that payload (handle str->bytes) and decode to utf-8; update FileItem.__init__
logic accordingly so callers in dav_collection.py can continue passing
attachment.datas or raw data without losing content.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
base_dav/controllers/main.pybase_dav/models/dav_collection.pybase_dav/models/dav_collection_field_mapping.pybase_dav/radicale/collection.pybase_dav/radicale/rights.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base_dav/models/dav_collection.py`:
- Around line 267-272: The DavCollection branch that constructs a Radicale
Collection (DavFolder = Collection(href)) erroneously overwrites the
Collection's own logger by setting folder.logger = getattr(self, "logger",
None); remove that assignment line so the Collection instance retains the logger
initialized in Collection.__init__, i.e., delete the folder.logger =
getattr(self, "logger", None) statement in the block that creates DavFolder
after detecting three path components.
ivs-cetmix
requested changes
Feb 26, 2026
ivs-cetmix
left a comment
ivs-cetmix
left a comment
There was a problem hiding this comment.
ALL functions MUST have COMPLETE docstrings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
base_dav/models/dav_collection.py (1)
411-413:⚠️ Potential issue | 🟠 MajorDo not overwrite
DavFolder.loggerwithNone.Line [412] can null out the logger because
DavCollectionhas nologgerattribute. That can crash later when metadata code callsself.logger.warning(...).💡 Suggested fix
if len(components) == 3: from ..radicale.collection import Collection as DavFolder folder = DavFolder(href) - folder.logger = getattr(self, "logger", None) return folder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base_dav/models/dav_collection.py` around lines 411 - 413, DavCollection is assigning DavFolder.logger to None when DavCollection has no logger, which can later cause AttributeError; change the assignment in the DavCollection code that creates DavFolder (the block creating folder = DavFolder(href)) to only set folder.logger if self has a non-None logger (e.g., check hasattr(self, "logger") or if getattr(self, "logger", None) is not None) and then assign folder.logger = self.logger so you never overwrite folder.logger with None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base_dav/models/dav_collection.py`:
- Around line 281-288: The folder-name lookup is using encoded names
(quote_plus) so name_search misses records; before calling
collection_model.name_search (where path_components[2] or other path parts are
passed) decode the token with unquote_plus so the search uses the original name;
update the occurrences that construct the search term (e.g., the code around the
path_components[2] browse/name_search and the similar blocks noted at lines
~304–307 and ~416–423) to call unquote_plus(...) on the incoming
folder/attachment name prior to name_search, keeping the existing use of
unquote_plus for attachment decoding (line ~426) intact.
---
Duplicate comments:
In `@base_dav/models/dav_collection.py`:
- Around line 411-413: DavCollection is assigning DavFolder.logger to None when
DavCollection has no logger, which can later cause AttributeError; change the
assignment in the DavCollection code that creates DavFolder (the block creating
folder = DavFolder(href)) to only set folder.logger if self has a non-None
logger (e.g., check hasattr(self, "logger") or if getattr(self, "logger", None)
is not None) and then assign folder.logger = self.logger so you never overwrite
folder.logger with None.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
base_dav/controllers/main.pybase_dav/models/dav_collection.pybase_dav/models/dav_collection_field_mapping.pybase_dav/radicale/auth.pybase_dav/radicale/collection.pybase_dav/radicale/rights.pybase_dav/tests/test_base_dav.pybase_dav/tests/test_collection.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base_dav/radicale/rights.py`:
- Around line 74-81: The current owner check sets is_owner = bool(user) and
(user == parts[0]) which trusts the URL principal segment (parts[0]) and can be
spoofed; change the logic in the rights calculation (the code that computes
is_owner and the branches for mode == "owner_only" and "owner_write_only") to
fetch the actual collection owner from authoritative metadata/storage (e.g., via
the collection lookup function or a get_collection_owner/get_owner helper) and
compare that owner to the authenticated user, falling back to denying write
access if the owner cannot be determined; update the branches that return "rw"
to use this verified is_owner and keep read-only behavior for authenticated
users as before.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
base_dav/controllers/main.pybase_dav/radicale/auth.pybase_dav/radicale/rights.py
ivs-cetmix
requested changes
Mar 6, 2026
ivs-cetmix
left a comment
ivs-cetmix
left a comment
There was a problem hiding this comment.
Besides that, here are some AI-assisted comments:
-
High — Non-admin users can alter DAV mappings and execute mapping code paths
Where: base_dav/security/ir.model.access.csv:4, base_dav/models/dav_collection_field_mapping.py:167-169, :323-325
base.group_user has write/create on dav.collection.field_mapping, and mapping code is executed via safe_eval (import_code / export_code).
Even with safe_eval, this is a dangerous capability for regular internal users because they can alter data transformation behavior for existing collections (and potentially abuse object access through record / item context).
Recommendation: Restrict mapping write/create to base.group_system (or a dedicated admin group), and optionally block mapping_type='code' for non-admins at model level. -
High — File attachments are lossy/corrupted for binary content
Where: base_dav/radicale/collection.py:117-125
FileItem base64-decodes attachment bytes, then does raw.decode("utf-8", errors="ignore") and passes text to RadicaleItem.
This silently drops/changes bytes for non-UTF8 files (images, PDFs, etc.), so downloaded content can be corrupted.
Recommendation: Preserve binary payload end-to-end (no UTF-8 coercion), and return bytes with correct content type semantics. -
Medium — Collection domain is not enforced on DAV create
Where: base_dav/models/dav_collection.py:397-416
On upload, create(data) happens without validating that the created record matches self._eval_domain().
This can create “invisible” records (created via DAV but never listed in that DAV collection), causing sync inconsistency and data surprises.
Recommendation: After create/write, verify domain membership; reject with a DAV error if out of scope. -
Medium — Files mode uses non-unique display names as folder keys
Where: base_dav/models/dav_collection.py:315-325, :456-466
Folder resolution relies on name_search(..., operator="=", limit=1) against display name.
Duplicate names can resolve to the wrong record, causing wrong attachment listing/get behavior.
Recommendation: Use stable identifiers in the DAV path (record id/uuid), not display names. -
Medium (potential) — Failed login may surface as 500 instead of auth failure
Where: base_dav/radicale/auth.py:101-108
_login is called without exception handling; if backend auth raises (e.g., bad creds path), this may propagate as server error instead of clean auth denial.
Recommendation: Catch auth exceptions and return empty auth result to let Radicale return proper 401/challenge behavior. -
Low — Principal-level authorization is too broad
Where: base_dav/radicale/rights.py:65-67, base_dav/radicale/collection.py:226-227
Any authenticated user gets access to any principal root path (/<any_login>), and principal listing iterates all collections.
May allow unnecessary metadata/discovery exposure.
Recommendation: Restrict principal access to user == principal unless explicitly intended.
ivs-cetmix
requested changes
Mar 6, 2026
tendil
force-pushed
the
18.0-t5250-base_dav-standard_migrate
branch
from
4b51cbe to
4eb3494
Compare
|
This PR has the |
tendil
force-pushed
the
18.0-t5250-base_dav-standard_migrate
branch
3 times, most recently
from
973fc83 to
a902e02
Compare
tendil
force-pushed
the
18.0-t5250-base_dav-standard_migrate
branch
2 times, most recently
from
9b6a760 to
bed8559
Compare
Co-authored-by: Yannick Vaucher <yannick.vaucher@camptocamp.com>
Task: 5250
Task: 5250
Task: 5250
Task: 5250
tendil
force-pushed
the
18.0-t5250-base_dav-standard_migrate
branch
from
6e6d5f2 to
14f10be
Compare
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
6 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.
Task: 5250
Summary by CodeRabbit
New Features
Documentation
Demo
Localization
Tests