Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
It also adds tests to validate environment overrides and that extra parameters are safely ignored while preserving expected access behavior. Possible Issues• Ignoring extra params may mask configuration typos; ensure this is acceptable for This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Suggested change flags potential security risk from silently ignoring unknown settings keys.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- Ignoring unknown config keys may hide typos; consider safer filtering:
chromadb/config.py
Review Details
📁 2 files reviewed | 💬 1 comments
👍 / 👎 individual comments to help improve reviews for you
| return val | ||
|
|
||
| model_config = {"env_file": ".env", "env_file_encoding": "utf-8"} | ||
| model_config = {"env_file": ".env", "env_file_encoding": "utf-8", "extra": "ignore"} |
There was a problem hiding this comment.
[Security] Enabling extra='ignore' allows configuration typos to pass silently, which can be dangerous for security settings. For example, if a user misspells chroma_server_ssl_enabled=True as chroma_server_ssl_enabld=True, the setting will be ignored and the server will start without SSL (defaulting to False).
Ensure this trade-off is acceptable for your use case. If this is primarily to handle specific known extra parameters, filtering them before initialization might be safer than globally ignoring all unknown fields.
Context for Agents
Enabling `extra='ignore'` allows configuration typos to pass silently, which can be dangerous for security settings. For example, if a user misspells `chroma_server_ssl_enabled=True` as `chroma_server_ssl_enabld=True`, the setting will be ignored and the server will start without SSL (defaulting to False).
Ensure this trade-off is acceptable for your use case. If this is primarily to handle specific known extra parameters, filtering them before initialization might be safer than globally ignoring all unknown fields.
File: chromadb/config.py
Line: 320
This comment has been minimized.
This comment has been minimized.
2 participants
pydantic's default behavior should be to ignore extra params. In practice, we see it error if extra params are provided
https://docs.pydantic.dev/latest/concepts/models/#extra-data
This explicitly sets the "extra" param