Skip to content

Allow instance_variable_get for gems than need low level acces #138

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

Open
chaadow wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Allow instance_variable_get for gems than need low level acces #138

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d50b4e7
Allow instance_variable_get for gems than need low level acces
chaadow Dec 31, 2025
a345449
Merge branch 'basecamp:master' into patch-1
chaadow Feb 18, 2026
0910b7e
Merge branch 'basecamp:master' into patch-1
chaadow May 1, 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
2 changes: 1 addition & 1 deletion lib/console1984/freezeable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def self.included(base)
end

module ClassMethods
SENSITIVE_INSTANCE_METHODS = %i[ instance_variable_get instance_variable_set ]
SENSITIVE_INSTANCE_METHODS = %i[ instance_variable_set ]

Comment on lines 30 to 32

Copilot AI May 1, 2026

Copy link

Choose a reason for hiding this comment

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

The surrounding documentation is now inaccurate: Freezeable no longer blocks instance_variable_get, but the header comment still says it prevents “accessing … instance variables via reflection”, and the flag comment mentions both instance_variable_get and instance_variable_set. Please update the comments (and/or rename prevent_instance_data_manipulation* if its meaning is now “prevent mutation”).

Copilot uses AI. Check for mistakes.
Comment on lines 30 to 32

Copilot AI May 1, 2026

Copy link

Choose a reason for hiding this comment

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

This change will flip existing tampering expectations: test/tampering_cases/flagged/config/override_with_instance_variable_get.rb currently asserts instance_variable_get is forbidden, but after this change it will be allowed (returning nil if the ivar doesn’t exist). Please update the tampering cases/tests to reflect the new policy (e.g., move that case to allowed/ and/or add an allowed example covering instance_variable_get).

Copilot uses AI. Check for mistakes.
Comment on lines 30 to 32

Copilot AI May 1, 2026

Copy link

Choose a reason for hiding this comment

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

Removing instance_variable_get from the protected list affects all classes/modules that include Console1984::Freezeable (e.g., Console1984::Config, Console1984::CommandExecutor, etc.), not just the ActiveRecord adapter extensions. If the intent is only to unblock frameworks like Rails on AR adapters while keeping Console1984 internals opaque, consider scoping the relaxation (e.g., make the sensitive method list configurable per including module/class, or disable instance-data protection only for Console1984::Ext::ActiveRecord::ProtectedAuditableTables).

Copilot uses AI. Check for mistakes.
def prevent_instance_data_manipulation
SENSITIVE_INSTANCE_METHODS.each do |method|
Expand Down
Loading