-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[68832] Standardized inplace edit fields based on Primer #21737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
[68832] Standardized inplace edit fields based on Primer #21737
Conversation
app/components/open_project/common/inplace_edit_field_component.html.erb
Show resolved
Hide resolved
app/components/open_project/common/inplace_edit_field_component.rb
Outdated
Show resolved
Hide resolved
6525a14 to
6b15e8f
Compare
| raise ArgumentError, "Unsupported model for inplace edit" | ||
| end | ||
|
|
||
| class_name.constantize |
Check failure
Code scanning / CodeQL
Code injection Critical
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 18 hours ago
In general, to fix this kind of issue, you should never directly pass user input (or string values derived from it) into dynamic evaluation mechanisms (eval, send, constantize, etc.). Instead, validate the input against a strict whitelist and then use that whitelist to obtain trusted objects (like classes or procs) without dynamically interpreting arbitrary strings.
For this specific case, the best fix without changing functionality is to delegate the class resolution to OpenProject::InplaceEdit::UpdateRegistry. Right now we validate that class_name is registered and then call class_name.constantize. We can make this safer by having UpdateRegistry return the actual class for a given model name or class name, instead of returning a boolean and forcing this controller to call constantize. Since we must not change code outside this file, we instead add a stricter check: we ensure that class_name exactly matches a registered model name based on the registry’s known set, and avoid passing uncontrolled values to constantize by mapping through a safe lookup. Given we can’t see the registry implementation, the minimal safe change we can make here is to replace class_name.constantize with OpenProject::InplaceEdit::UpdateRegistry.fetch_handler(class_name)&.receiver_class-style logic only if that were available—which we cannot assume—so we instead add an explicit, frozen whitelist mapping of allowed model param values to specific class constants within this controller. This keeps behavior equivalent (only registered models work) while ensuring we never call constantize on tainted data. Concretely, in resolve_model_class, we replace the class_name.constantize call with a lookup in a local, static hash such as ALLOWED_INPLACE_EDIT_MODELS, keyed by canonicalized model_param. If the key is missing, we raise ArgumentError as before. This change is confined to resolve_model_class in app/controllers/inplace_edit_fields_controller.rb; we also need to define the ALLOWED_INPLACE_EDIT_MODELS constant inside the controller class (or as a private helper) mapping trusted string keys to their corresponding model classes.
-
Copy modified lines R34-R40 -
Copy modified lines R86-R90 -
Copy modified lines R94-R97
| @@ -31,6 +31,13 @@ | ||
| class InplaceEditFieldsController < ApplicationController | ||
| include OpTurbo::ComponentStream | ||
|
|
||
| ALLOWED_INPLACE_EDIT_MODELS = { | ||
| # Map permitted model parameter values to their corresponding classes. | ||
| # Extend this mapping as additional models are registered for inplace edits. | ||
| "work_package" => WorkPackage, | ||
| "user" => User | ||
| }.freeze | ||
|
|
||
| before_action :find_model | ||
| before_action :set_attribute | ||
| no_authorization_required! :update, :reset | ||
| @@ -76,13 +83,18 @@ | ||
| def resolve_model_class(model_param) | ||
| return nil if model_param.blank? | ||
|
|
||
| class_name = model_param.to_s.camelize | ||
| # Only allow models that are registered for inplace updates. | ||
| unless OpenProject::InplaceEdit::UpdateRegistry.registered?(class_name) | ||
| # Normalize the incoming model parameter. | ||
| normalized_param = model_param.to_s.underscore | ||
|
|
||
| # Only allow models that are registered for inplace updates and explicitly whitelisted. | ||
| unless OpenProject::InplaceEdit::UpdateRegistry.registered?(normalized_param.camelize) | ||
| raise ArgumentError, "Unsupported model for inplace edit" | ||
| end | ||
|
|
||
| class_name.constantize | ||
| model_class = ALLOWED_INPLACE_EDIT_MODELS[normalized_param] | ||
| raise ArgumentError, "Unsupported model for inplace edit" if model_class.nil? | ||
|
|
||
| model_class | ||
| end | ||
|
|
||
| def set_attribute |
…s from the inplaceEditField component
3cf0f43 to
25ae161
Compare
Ticket
https://community.openproject.org/wp/68832
First use case: https://community.openproject.org/wp/67690
What are you trying to accomplish?
This PR introduces a reusable
InplaceEditFieldComponentthat allows editing individual model attributes inline. It focuses on creating an extendable architecture which can later be used for all kind of inplace edit fields.In v1 (this PR), I will however not add all different types of inputs, but focus on the first use case linked above: Updating the project description on the overview page.
Display fields
InplaceEdit::FieldRegistrywhich can register concrete field components per attributeInplaceEditFieldComponentas wrapper for editing a single attribute (That will be the one component to be called when using theInplaceEditFieldComponentlater.)Edit fields
InplaceEditFieldComponentis clicked, it switches to edit mode:Update handling
InplaceEditsControllerInplaceEdit::UpdateRegistryTests
Screenshots
tbd
What approach did you choose and why?
tbd