From 7f52ea535788aec49b217366d580f26525d8560f Mon Sep 17 00:00:00 2001 From: Matthew Blythe Date: Tue, 26 Nov 2024 14:31:28 -0700 Subject: [PATCH] Prevent modification of attribute defaults Either freeze them, in the case of Strings or empty Hash/Array, or call a proc to return a unique instance of a mutable object (mostly relevant for the 'principals' attribute of PermissionTarget). Receiving a FrozenError in this case may be unexpected for users, but it will force them into the preferred method of setting attributes (i.e. using the "#{name}=" setter). Fixes issue #174. Signed-off-by: Matthew Blythe --- lib/artifactory/resources/base.rb | 10 ++++ lib/artifactory/resources/build.rb | 18 ++++---- lib/artifactory/resources/ldap_setting.rb | 2 +- .../resources/permission_target.rb | 6 +-- lib/artifactory/resources/repository.rb | 22 ++++----- lib/artifactory/resources/user.rb | 2 +- spec/unit/resources/build_spec.rb | 38 +++++++++++++++ spec/unit/resources/ldap_setting_spec.rb | 5 ++ spec/unit/resources/permission_target_spec.rb | 19 ++++++++ spec/unit/resources/repository_spec.rb | 46 +++++++++++++++++++ spec/unit/resources/user_spec.rb | 6 +++ 11 files changed, 149 insertions(+), 25 deletions(-) diff --git a/lib/artifactory/resources/base.rb b/lib/artifactory/resources/base.rb index e6b013b..7b437b1 100644 --- a/lib/artifactory/resources/base.rb +++ b/lib/artifactory/resources/base.rb @@ -44,6 +44,16 @@ class << self # def attribute(key, default = nil) key = key.to_sym unless key.is_a?(Symbol) + case default + when NilClass, Integer, Float, Symbol, TrueClass, FalseClass, Proc + # Immutable primitive data types OK + when String + raise "Mutable String must not be used as a default attribute value. (Frozen OK)" unless default.frozen? + when Hash, Array + raise "Mutable or non-empty #{default.class} must not be used as a default attribute value. (Frozen empty #{default.class} OK)" unless default.frozen? && default.empty? + else + raise "Mutable type #{default.class} must not be used as a default attribute value" + end # Set this attribute in the top-level hash attributes[key] = nil diff --git a/lib/artifactory/resources/build.rb b/lib/artifactory/resources/build.rb index 49e78f3..a30f223 100644 --- a/lib/artifactory/resources/build.rb +++ b/lib/artifactory/resources/build.rb @@ -95,24 +95,24 @@ def from_hash(hash, options = {}) end # Based on https://github.com/JFrogDev/build-info/blob/master/README.md#build-info-json-format - attribute :properties, {} + attribute :properties, {}.freeze attribute :version, BUILD_SCHEMA_VERSION attribute :name, -> { raise "Build component missing!" } attribute :number, -> { raise "Build number missing!" } - attribute :type, "GENERIC" - attribute :build_agent, {} - attribute :agent, {} - attribute :started, Time.now.utc.iso8601(3) + attribute :type, "GENERIC".freeze + attribute :build_agent, {}.freeze + attribute :agent, {}.freeze + attribute :started, Time.now.utc.iso8601(3).freeze attribute :duration_millis attribute :artifactory_principal attribute :url attribute :vcs_revision attribute :vcs_url - attribute :license_control, {} - attribute :build_retention, {} - attribute :modules, [] + attribute :license_control, {}.freeze + attribute :build_retention, {}.freeze + attribute :modules, [].freeze attribute :governance - attribute :statuses, [] + attribute :statuses, [].freeze # # Compare a build artifacts/dependencies/environment with an older diff --git a/lib/artifactory/resources/ldap_setting.rb b/lib/artifactory/resources/ldap_setting.rb index 67e1dbb..6704ffa 100644 --- a/lib/artifactory/resources/ldap_setting.rb +++ b/lib/artifactory/resources/ldap_setting.rb @@ -119,6 +119,6 @@ def find_from_config(xpath, config, options = {}) attribute :manager_dn attribute :manager_password attribute :auto_create_user - attribute :email_attribute, "mail" + attribute :email_attribute, "mail".freeze end end diff --git a/lib/artifactory/resources/permission_target.rb b/lib/artifactory/resources/permission_target.rb index 3a3fba1..98f88c3 100644 --- a/lib/artifactory/resources/permission_target.rb +++ b/lib/artifactory/resources/permission_target.rb @@ -134,10 +134,10 @@ def abbreviate_principal(principal_hash) end attribute :name, -> { raise "Name missing!" } - attribute :includes_pattern, "**" - attribute :excludes_pattern, "" + attribute :includes_pattern, "**".freeze + attribute :excludes_pattern, "".freeze attribute :repositories - attribute :principals, { "users" => {}, "groups" => {} } + attribute :principals, -> { { "users" => {}, "groups" => {} } } def client_principal @client_principal ||= Principal.new(principals["users"], principals["groups"]) diff --git a/lib/artifactory/resources/repository.rb b/lib/artifactory/resources/repository.rb index 8dec3cd..1aa6334 100644 --- a/lib/artifactory/resources/repository.rb +++ b/lib/artifactory/resources/repository.rb @@ -68,26 +68,26 @@ def find(name, options = {}) attribute :blacked_out, false attribute :description - attribute :checksum_policy_type, "client-checksums" - attribute :excludes_pattern, "" + attribute :checksum_policy_type, "client-checksums".freeze + attribute :excludes_pattern, "".freeze attribute :handle_releases, true attribute :handle_snapshots, true - attribute :includes_pattern, "**/*" + attribute :includes_pattern, "**/*".freeze attribute :key, -> { raise "Key is missing!" } attribute :max_unique_snapshots, 0 attribute :notes - attribute :package_type, "generic" - attribute :property_sets, [] - attribute :repo_layout_ref, "simple-default" - attribute :rclass, "local" - attribute :snapshot_version_behavior, "non-unique" + attribute :package_type, "generic".freeze + attribute :property_sets, [].freeze + attribute :repo_layout_ref, "simple-default".freeze + attribute :rclass, "local".freeze + attribute :snapshot_version_behavior, "non-unique".freeze attribute :suppress_pom_consistency_checks, false - attribute :url, "" + attribute :url, "".freeze attribute :yum_root_depth, 0 attribute :calculate_yum_metadata, false - attribute :repositories, [] + attribute :repositories, [].freeze attribute :external_dependencies_enabled, false - attribute :client_tls_certificate, "" + attribute :client_tls_certificate, "".freeze # # Creates or updates a repository configuration depending on if the diff --git a/lib/artifactory/resources/user.rb b/lib/artifactory/resources/user.rb index 40deaa5..98e11c2 100644 --- a/lib/artifactory/resources/user.rb +++ b/lib/artifactory/resources/user.rb @@ -68,7 +68,7 @@ def find(name, options = {}) attribute :admin, false attribute :email - attribute :groups, [] + attribute :groups, [].freeze attribute :internal_password_disabled, false attribute :last_logged_in attribute :name, -> { raise "Name missing" } diff --git a/spec/unit/resources/build_spec.rb b/spec/unit/resources/build_spec.rb index 69de8a4..d3e05a3 100644 --- a/spec/unit/resources/build_spec.rb +++ b/spec/unit/resources/build_spec.rb @@ -343,5 +343,43 @@ module Artifactory end end end + describe "attribute defaults" do + it "#properties modifying the default return value produces an error" do + expect { subject.properties["key"] = "value" }.to raise_error(FrozenError) + end + + it "#type modifying the default return value produces an error" do + expect { subject.type.downcase! }.to raise_error(FrozenError) + end + + it "#build_agent modifying the default return value produces an error" do + expect { subject.build_agent["key"] = "value" }.to raise_error(FrozenError) + end + + it "#agent modifying the default return value produces an error" do + expect { subject.agent["key"] = "value" }.to raise_error(FrozenError) + end + + it "#started modifying the default return value produces an error" do + expect { subject.started.downcase! }.to raise_error(FrozenError) + end + + it "#license_control modifying the default return value produces an error" do + expect { subject.license_control["key"] = "value" }.to raise_error(FrozenError) + end + + it "#build_retention modifying the default return value produces an error" do + expect { subject.build_retention["key"] = "value" }.to raise_error(FrozenError) + end + + it "#modules modifying the default return value produces an error" do + expect { subject.modules << "value" }.to raise_error(FrozenError) + end + + it "#statuses modifying the default return value produces an error" do + expect { subject.statuses << "value" }.to raise_error(FrozenError) + end + + end end end diff --git a/spec/unit/resources/ldap_setting_spec.rb b/spec/unit/resources/ldap_setting_spec.rb index b1115b1..ebc683d 100644 --- a/spec/unit/resources/ldap_setting_spec.rb +++ b/spec/unit/resources/ldap_setting_spec.rb @@ -61,5 +61,10 @@ module Artifactory expect(described_class.find("viridian-ldap").key).to eq("viridian-ldap") end end + describe "attribute defaults" do + it "#email_attribute modifying the default return value produces an error" do + expect { subject.email_attribute.downcase! }.to raise_error(FrozenError) + end + end end end diff --git a/spec/unit/resources/permission_target_spec.rb b/spec/unit/resources/permission_target_spec.rb index e3e2609..aa7ec5c 100644 --- a/spec/unit/resources/permission_target_spec.rb +++ b/spec/unit/resources/permission_target_spec.rb @@ -185,5 +185,24 @@ module Artifactory expect(subject.groups).to eq({ "beatles" => %w{delete deploy} }) end end + describe "attribute defaults" do + it "#includes_pattern modifying the default return value produces an error" do + expect { subject.includes_pattern.gsub!(/^\*\*$/, "path/**") }.to raise_error(FrozenError) + end + + it "#excludes_pattern modifying the default return value produces an error" do + expect { subject.excludes_pattern.replace("path/**") }.to raise_error(FrozenError) + end + + it "#users modifying the return value doesn't modify the default" do + subject.users["a_user"] = ["read"] + expect(described_class.new.users).to eq({}) + end + + it "#groups modifying the return value doesn't modify the default" do + subject.groups["a_group"] = ["read"] + expect(described_class.new.groups).to eq({}) + end + end end end diff --git a/spec/unit/resources/repository_spec.rb b/spec/unit/resources/repository_spec.rb index cdc32ae..4719266 100644 --- a/spec/unit/resources/repository_spec.rb +++ b/spec/unit/resources/repository_spec.rb @@ -215,5 +215,51 @@ module Artifactory subject.upload_from_archive("/local/path", "/remote/path", { properties: :foobar }) end end + + describe "attribute defaults" do + it "#checksum_policy_type modifying the default return value produces an error" do + expect { subject.checksum_policy_type.downcase! }.to raise_error(FrozenError) + end + + it "#excludes_pattern modifying the default return value produces an error" do + expect { subject.excludes_pattern.downcase! }.to raise_error(FrozenError) + end + + it "#includes_pattern modifying the default return value produces an error" do + expect { subject.includes_pattern.downcase! }.to raise_error(FrozenError) + end + + it "#package_type modifying the default return value produces an error" do + expect { subject.package_type.downcase! }.to raise_error(FrozenError) + end + + it "#property_sets modifying the default return value produces an error" do + expect { subject.property_sets << "value" }.to raise_error(FrozenError) + end + + it "#repo_layout_ref modifying the default return value produces an error" do + expect { subject.repo_layout_ref.downcase! }.to raise_error(FrozenError) + end + + it "#rclass modifying the default return value produces an error" do + expect { subject.rclass.downcase! }.to raise_error(FrozenError) + end + + it "#snapshot_version_behavior modifying the default return value produces an error" do + expect { subject.snapshot_version_behavior.downcase! }.to raise_error(FrozenError) + end + + it "#url modifying the default return value produces an error" do + expect { subject.url.downcase! }.to raise_error(FrozenError) + end + + it "#repositories modifying the default return value produces an error" do + expect { subject.repositories << "value" }.to raise_error(FrozenError) + end + + it "#client_tls_certificate modifying the default return value produces an error" do + expect { subject.client_tls_certificate.downcase! }.to raise_error(FrozenError) + end + end end end diff --git a/spec/unit/resources/user_spec.rb b/spec/unit/resources/user_spec.rb index 0bf474b..1a27910 100644 --- a/spec/unit/resources/user_spec.rb +++ b/spec/unit/resources/user_spec.rb @@ -111,5 +111,11 @@ module Artifactory end end end + + describe "attribute defaults" do + it "#groups modifying the default return value produces an error" do + expect { subject.groups << "value" }.to raise_error(FrozenError) + end + end end end