Fixed addons being unable to read project settings #1180 - #1181
Conversation
WalkthroughA guard was added to ConfigFile::_parse to set Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related issues
🚥 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. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/io/config_file.cpp (1)
276-282: Scope object allowance to the actual project settings file only.This enables object deserialization for any “.godot” file, which widens the surface area beyond the intended
project.godotand risks reintroducing the earlier RCE vector if arbitrary .godot configs are loaded. Prefer restricting to the canonical project settings path (or move this exception to the ProjectSettings load site where the file is known to be trusted).🔧 Possible tightening (filename-only)
- if (p_path.to_lower().ends_with(".godot")) { + if (p_path.get_file().to_lower() == "project.godot") { p_allow_objects = true; }
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6700 🏁 Script executed: Length of output: 5450 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 7188 🏁 Script executed: Length of output: 8411 🏁 Script executed: Length of output: 26076 🏁 Script executed: Length of output: 3787 🏁 Script executed: Length of output: 1982 🏁 Script executed: Length of output: 710 🏁 Script executed: Length of output: 3193 🏁 Script executed: Length of output: 4321 🏁 Script executed: Length of output: 7302 🏁 Script executed: Length of output: 15554
However, based on my examination of the codebase, I found that the project file name is actually hardcoded to "project.godot" throughout the engine. For example:
While command-line paths can reference the file by its full path, the actual filename must be "project.godot" (not a custom name). This means you could potentially restrict the check to That said, I respect your decision on the security/compatibility tradeoff. Your point about Godot projects and extensions not having this anti-RCE measure is valid context. If you're comfortable with the slightly broader scope, the current implementation achieves your compatibility goals. ✏️ Learnings added
|
PR Redot-Engine#1142 fixes a remote code execution vulnerability by disallowing deserialization of objects by default. The PR correctly bypasses the security measure when loading project settings from `project.godot`, but some addons also store their configuration settings in there as well, and are not aware of this security feature and thus the call will generate an error. This commit adds a check to determine if we are trying to access a project file, and if so, to allow for deserialization of raw objects which is needed for reading project settings.
89d6499 to
a02eca5
Compare
PR #1142 fixes a remote code execution vulnerability by disallowing deserialization of objects by default. The PR correctly bypasses the security measure when loading project settings from
project.godot, but some addons also store their configuration settings in there as well, and are not aware of this security feature and thus the call will generate an error. This commit adds a check to determine if we are trying to access a project file, and if so, to allow for deserialization of raw objects which is needed for reading project settings.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.