Skip to content

(PA-8346) Preserve sensitive parameters #388

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

Merged
david22swan merged 2 commits into from
Jun 30, 2026
+28 −0
Merged

(PA-8346) Preserve sensitive parameters #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7623e71
(PA-8346) Preserve sensitive parameters
joshcooper May 12, 2026
481bb25
(maint) Add in rubocop fixes for 1.9.x cherry picked commits
tvpartytonight Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/puppet/resource_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ def initialize(attributes)
attributes[:title] = @title if attributes[:title].nil? && !type_definition.namevars.empty?
end

attributes[:sensitive_parameters] = sensitives unless sensitives.empty?

@gavindidrichsen gavindidrichsen Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding 1 (Medium): Fix couples to hash2resource undocumented internals

This line works because Puppet::Type#hash2resource explicitly deletes :sensitive_parameters from the hash before processing remaining keys (puppet/type.rb:1213). That deletion behavior is an undocumented internal — if Puppet core changes how hash2resource handles this key, this fix silently breaks.

A more robust alternative would set sensitive_parameters explicitly after super completes, using Puppet's public API:

super(attributes)
set_sensitive_parameters(sensitives) unless sensitives.empty?

set_sensitive_parameters is a public method on Puppet::Type that iterates the list and marks each parameter as sensitive. This removes the dependency on hash2resource's internal key-deletion behavior.

Note: you can't remove set_sensitive_parameters from hash2resource itself — that's Puppet core code shared by all type implementations (native types, defined types, etc.), not just Resource API types. But the Resource API can safely add its own post-super call. The double-call (once inside hash2resource, once explicitly) is idempotent and harmless.

Trade-off is minimal — robustness over a negligible duplicate call.

super
end

Expand Down
27 changes: 27 additions & 0 deletions spec/puppet/resource_api_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,33 @@ def set(_context, _changes); end
instance[:secret] = Puppet::Pops::Types::PSensitiveType::Sensitive.new('a new password')
instance.flush
end

it 'marks the sensitive property with sensitive=true so it is excluded from the transaction store' do
expect(instance.parameter(:secret).sensitive).to be true
end
end

context 'when loading from a Puppet::Resource with no sensitive parameters' do
let(:params) { instance_double('Puppet::Resource', 'resource') }
let(:provider_instance) { instance_double(provider_class, 'provider_instance') }
let(:catalog) { instance_double('Unknown', 'catalog') }

before do
allow(provider_class).to receive(:new).with(no_args).and_return(provider_instance)
allow(provider_instance).to receive(:get).and_return([])
allow(params).to receive(:is_a?).with(Puppet::Resource).and_return(true)
allow(params).to receive(:title).with(no_args).and_return('a title')
allow(params).to receive(:catalog).with(no_args).and_return(catalog)
allow(params).to receive(:sensitive_parameters).with(no_args).and_return([])
allow(params).to receive(:to_hash).with(no_args)
.and_return(title: 'test',
secret: Puppet::Pops::Types::PSensitiveType::Sensitive.new('a password value'))
allow(catalog).to receive(:host_config?).and_return(true)

@gavindidrichsen gavindidrichsen Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding 3 (Low): This test stubs an impossible state

This test creates a Puppet::Resource stub where sensitive_parameters returns [] but to_hash returns { secret: Puppet::Pops::Types::PSensitiveType::Sensitive.new("a password value") }. In real Puppet, if a value is wrapped in Sensitive in the catalog, its parameter name always appears in sensitive_parameters. The empty list + wrapped value combination can't happen.

The test passes but doesn't verify meaningful behavior — it's testing a code path that can't be reached in production. Consider either:

  • Removing the Sensitive wrapper from the stub (plain string for :secret) to represent a genuinely non-sensitive parameter, or
  • Adding the parameter name to sensitive_parameters to represent the realistic case (which then becomes a duplicate of the existing test above)

end

it 'does not mark the property as sensitive' do
expect(instance.parameter(:secret).sensitive).not_to be true
end
end
end
end
Expand Down
Loading