Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feature: eject controllers #3377

Merged
merged 4 commits into from
Nov 4, 2024
Merged

feature: eject controllers #3377

merged 4 commits into from
Nov 4, 2024

Conversation

adrianthedev
Copy link
Collaborator

@adrianthedev adrianthedev commented Nov 1, 2024 •
edited
Loading

Description

This PR adds the ability to eject controllers from Avo and more specifically the application controller.
THis enables more granular customizations to the whole request cycle.

Related to #3376

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Manual review steps

  1. run bin/rails generate avo:eject --controller application
  2. see the application_controller.rb file being generated in your app.

Manual reviewer: please leave a comment with output from the test if that's the case.

@codeclimate CodeClimate
Copy link

codeclimate bot commented Nov 1, 2024 •
edited
Loading

Code Climate has analyzed commit 2650bca and detected 0 issues on this pull request.

View more on Code Climate.

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 1, 2024
View reviewed changes
app/controllers/avo/base_application_controller.rb
def set_related_record
association_name = BaseResource.valid_association_name(@record, params[:related_name])
@related_record = if @field.is_a? Avo::Fields::HasOneField
@record.send association_name

Check failure

Code scanning / CodeQL

Code injection Critical

This code execution depends on a
user-provided value
Loading
.

Copilot Autofix AI 4 months ago

To fix the problem, we need to ensure that the value of association_name is safe before using it in the send method. This can be achieved by explicitly validating that association_name is one of the expected method names. This way, we can prevent any arbitrary method execution based on user input.

We will modify the set_related_record method to include a whitelist of valid association names and check if association_name is included in this list before calling send.

Suggested changeset 1
app/controllers/avo/base_application_controller.rb

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/controllers/avo/base_application_controller.rb b/app/controllers/avo/base_application_controller.rb
--- a/app/controllers/avo/base_application_controller.rb
+++ b/app/controllers/avo/base_application_controller.rb
@@ -157,8 +157,13 @@
       association_name = BaseResource.valid_association_name(@record, params[:related_name])
-      @related_record = if @field.is_a? Avo::Fields::HasOneField
-        @record.send association_name
+      valid_associations = @record.class.reflect_on_all_associations.map(&:name).map(&:to_s)
+      if valid_associations.include?(association_name)
+        @related_record = if @field.is_a? Avo::Fields::HasOneField
+          @record.send association_name
+        else
+          @related_resource.find_record params[:related_id], query: @record.send(association_name), params: params
+        end
+        @related_resource.hydrate(record: @related_record)
       else
-        @related_resource.find_record params[:related_id], query: @record.send(association_name), params: params
+        raise Avo::InvalidAssociationNameError.new(association_name)
       end
-      @related_resource.hydrate(record: @related_record)
     end
EOF
@@ -157,8 +157,13 @@
association_name = BaseResource.valid_association_name(@record, params[:related_name])
@related_record = if @field.is_a? Avo::Fields::HasOneField
@record.send association_name
valid_associations = @record.class.reflect_on_all_associations.map(&:name).map(&:to_s)
if valid_associations.include?(association_name)
@related_record = if @field.is_a? Avo::Fields::HasOneField
@record.send association_name
else
@related_resource.find_record params[:related_id], query: @record.send(association_name), params: params
end
@related_resource.hydrate(record: @related_record)
else
@related_resource.find_record params[:related_id], query: @record.send(association_name), params: params
raise Avo::InvalidAssociationNameError.new(association_name)
end
@related_resource.hydrate(record: @related_record)
end
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
app/controllers/avo/base_application_controller.rb
@related_record = if @field.is_a? Avo::Fields::HasOneField
@record.send association_name
else
@related_resource.find_record params[:related_id], query: @record.send(association_name), params: params

Check failure

Code scanning / CodeQL

Code injection Critical

This code execution depends on a
user-provided value
Loading
.

Copilot Autofix AI 4 months ago

To fix the problem, we need to ensure that the association_name derived from params[:related_name] is safe to use with the send method. This can be achieved by explicitly validating or sanitizing the association_name to ensure it only contains expected and safe values. One approach is to use a whitelist of allowed association names or to ensure that the association_name is a valid method name on the @record object.

Suggested changeset 1
app/controllers/avo/base_application_controller.rb

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/controllers/avo/base_application_controller.rb b/app/controllers/avo/base_application_controller.rb
--- a/app/controllers/avo/base_application_controller.rb
+++ b/app/controllers/avo/base_application_controller.rb
@@ -157,8 +157,12 @@
       association_name = BaseResource.valid_association_name(@record, params[:related_name])
-      @related_record = if @field.is_a? Avo::Fields::HasOneField
-        @record.send association_name
+      if @record.respond_to?(association_name)
+        @related_record = if @field.is_a? Avo::Fields::HasOneField
+          @record.send association_name
+        else
+          @related_resource.find_record params[:related_id], query: @record.send(association_name), params: params
+        end
+        @related_resource.hydrate(record: @related_record)
       else
-        @related_resource.find_record params[:related_id], query: @record.send(association_name), params: params
+        raise Avo::InvalidAssociationNameError.new(association_name)
       end
-      @related_resource.hydrate(record: @related_record)
     end
EOF
@@ -157,8 +157,12 @@
association_name = BaseResource.valid_association_name(@record, params[:related_name])
@related_record = if @field.is_a? Avo::Fields::HasOneField
@record.send association_name
if @record.respond_to?(association_name)
@related_record = if @field.is_a? Avo::Fields::HasOneField
@record.send association_name
else
@related_resource.find_record params[:related_id], query: @record.send(association_name), params: params
end
@related_resource.hydrate(record: @related_record)
else
@related_resource.find_record params[:related_id], query: @record.send(association_name), params: params
raise Avo::InvalidAssociationNameError.new(association_name)
end
@related_resource.hydrate(record: @related_record)
end
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@adrianthedev adrianthedev requested a review from Paul-Bob November 1, 2024 19:15
@adrianthedev adrianthedev mentioned this pull request Nov 1, 2024
Closed
11 tasks
@Paul-Bob Paul-Bob merged commit 1f790ba into main Nov 4, 2024
19 of 21 checks passed
@Paul-Bob Paul-Bob deleted the feature/eject-application-controller branch November 4, 2024 10:32
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2024

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Paul-Bob Paul-Bob Paul-Bob left review comments

Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adrianthedev @Paul-Bob