Skip to content

Harden module loading#57

Open
salva wants to merge 2 commits into
masterfrom
fix/module-loader-require-version
Open

Harden module loading#57
salva wants to merge 2 commits into
masterfrom
fix/module-loader-require-version

Conversation

@salva

@salva salva commented Jun 4, 2026

Copy link
Copy Markdown
Owner

Summary

Harden Net::OpenSSH::ModuleLoader module loading and version checks.

Changes

  • Validate module names before loading.
  • Replace string eval "require ..." with non-string require on a module path.
  • Use Perl's standard $module->VERSION($version) semantics for version requirements.
  • Add regression tests for unsafe module names and version checks.
  • Add the new test to MANIFEST.

Fixes #45.
Fixes #46.

Testing

  • perl -Ilib -c lib/Net/OpenSSH/ModuleLoader.pm
  • perl -Ilib t/module-loader.t

Copilot AI review requested due to automatic review settings June 4, 2026 11:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens Net::OpenSSH::ModuleLoader by preventing unsafe module loading patterns and aligning module version enforcement with standard Perl semantics, plus adds regression coverage to ensure these behaviors remain correct.

Changes:

  • Validate module names before attempting to load them.
  • Replace string eval "require ..." with a non-string require of the computed module path.
  • Use $module->VERSION($version) for version requirements and add a new regression test (and manifest entry).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
lib/Net/OpenSSH/ModuleLoader.pm Adds module-name validation, switches to path-based require, and uses standard VERSION checking.
t/module-loader.t Adds regression tests for unsafe module names and version requirement behavior.
MANIFEST Adds the new test file to the distribution manifest.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread t/module-loader.t
Comment on lines +19 to +22
{
package Test::Net::OpenSSH::ModuleLoader::Versioned;
our $VERSION = '1.02';
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This concern does not hold for the test as written. The package Test::Net::OpenSSH::ModuleLoader::Versioned; declaration is inside a bare block, and the later unqualified ok/like calls continue to resolve correctly. The test passes after the new undef-name coverage:\n\nperl -Ilib t/module-loader.t\n\nResult: 1..6 and all assertions pass.

Comment on lines 13 to +15
my ($module, $version) = @_;
$module =~ /\A[A-Za-z_]\w*(?:::\w+)*\z/
or croak "bad Perl module name $module";

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in commit 4e504a6 by handling undefined module names before applying the regex, with a regression assertion in t/module-loader.t.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ModuleLoader version checks use numeric comparison ModuleLoader should avoid unvalidated string eval for require

2 participants