[drgrice1]

This method was only needed for a context that extends the SignificantFigures context (for example the Units context via context::Units::extending('SignificantFigures')) because the contextSignificantFigures.pl macro calls the methods of its context::SignificantFigures::common package as $self->STRING, $self->COPY, etc. When another context extends the SignificantFigures context, those calls are made with $self blessed into the extension context's classes, and those classes do not inherit from that package. So those methods were not found, and the AUTOLOAD method was used to find them in the extended class. Instead the contextSignificantFigures.pl macro now calls those methods with their fully qualified names, so that they are found regardless of the class of $self. Instead inherited parent methods are looked up with the approach of temporarily modifying the @ISA array that @dpvc suggested (see #1564 (comment) and ensuing comments).

The updated WWSafe.pm module based on Safe.pm version 2.47 (added in #1520) deletes the AUTOLOAD method (as well as the DELETE method and any overloads) in its _clean_stash call, and so the current AUTOLOAD method no longer works, and problems using the Units context extending the SignificantFigures context were broken.

Allowing code that runs in the safe compartment to define a DESTROY or AUTOLOAD method or to defined custom overloads is a security vulnerability that cannot be allowed.

[pstaabp]

Problems still work. I think this was mainly for getting the tests to run clean.

[dpvc]

Allowing code that runs in the safe compartment to define a DESTROY or AUTOLOAD method or to defined custom overloads is a security vulnerability that cannot be allowed.

Can you explain what the vulnerability is?

I think this was mainly for getting the tests to run clean.

No, this was actually an important mechanism for making inheritance work in extended contexts. The solution use here (using fully qualified names) is not a general solution, as it requires context authors to take the possibility that they may be extended into account. There are other contexts that use inheritance from a common package like this (e.g., the units context does). Will it be required to be modified in order to allow it to be extended by some other context in the future?

If AUTOLOAD is to be removed, then the context extension mechanism should be adjusted to handle inheritance in a different way. That will probably require creating individual packages for each overridden class that will inherit the overridden class (and its @ISA values), rather than trying to use a common super class for all inheritance. My original prototype for the extension context did that, but it seemed like a lot of extra work to create them and tie them in when a single class seemed to do the trick. It turns out that (without AUTOLOAD) that is no longer the case.

Unfortunately, I don't currently have the time to work out the new approach, and am not sure when I would be able to get to it. But I would encourage you not to make fully qualified method calls when proper inheritance is the natural way to proceed, as it is in the significant-figures context. If this doesn't work with extensions, it is better to fix the extension mechanism rather than change every context that might be extended someday.

[drgrice1]

The updated WWSafe.pm package is based on version 2.47 of the upstream Safe.pm package included in core Perl. It strips the DESTROY and AUTOLOAD methods from packages because the DESTROY method will execute in clean up after the safe compartment is ended, and execute in the main:: namespace. That means they can execute methods that are available from outside of the safe compartment and change variable values in the main:: namespace. If the DESTROY method is not defined, then the AUTOLOAD method will be called if it is defined, so it can then be used to do the same thing. Those are security vulnerabilities that allow the problem author to provide code that will break out of the safe compartment and do things they should not be allowed to do. I can provide problems that do this sort of thing with the main branch that does not have the updated WWSafe.pm.

[dpvc]

I can provide problems that do this sort of thing

No need, your description is sufficient. I didn't realize DESTROY works outside the safe compartment. That is the key detail I was missing, and I probably wouldn't have thought about the lack of DESTROY falling back to AUTOLOAD.

[dpvc]

I will think more about the extension mechanism and how it can be made to work without fully qualified method names. If I remember correctly, the issue arose in the significant-figures context because its implicit multiplication function is complicated and uses a method that doesn't get subclassed during an extension. It may be that the usual overridden classes work correctly, but this special one didn't. It will take some investigation to figure out a new approach.

[drgrice1]

I agree that for the contextExtensions.pl macro, the AUTOLOAD method approach was a nice way of handling passing calls to the super class, and that using the fully qualified name space is not. I am open to suggestions, if any one has a better way of handing this.

[drgrice1]

I guess I should give full disclosure here. I have overinflated the security issue slightly for this. At this point all production renderings (for webwork2 or the standalone renderer) occur inside of a Mojo::IOLoop::Subprocess. The Mojo::IOLoop::Subprocess calls POSIX::_exit(0) on the child process when it exits after it serializes the return value. The POSIX::_exit(0) system call terminates the process immediately and skips Perl's usual destruction phase. This means that if a problem is rendered by webwork2 or the standalone renderer, this is not an actual security vulnerability, and leaving the DESTROY and AUTOLOAD methods in place will not be an issue.

The issue does occur when rendering a problem directly via a script (such as is done in the unit tests). So as long as we ensure that we only execute trusted problem code in the unit tests, and perhaps give strong warnings to those that use PG in this way that extra precautions should be taking to ensure that untrusted problem code is not executed, then the stripping of the DESTROY and AUTOLOAD methods could be removed from the updated WWSafe.pm package (I already have a branch with this alternate approach).

[dpvc]

for the contextExtensions.pl macro, the AUTOLOAD method approach was a nice way of handling passing calls to the super class

Yes, that is why I was sad to see it go.

I have taken a quick look at the current contextExtension.pl implementation and I think I see how it might be handled without too much trouble. I haven't tried it yet, but that might make it possible to remove AUTOLOAD and still have the extension contexts work without needing to modify the context being extended (which, after all, was the original point of the extension context). I will see if I can get a few minutes to try it out.

[dpvc]

OK, I think I have a work-around for the AUTOLOAD problem. Replace the super function in the context::Extensions::Super package with

our @ISA = ();

sub super {
	my ($self, $method, $context) = @_;
	my $selfclass = ref($self);
	my $class = $self->superClass($context);
	my $code = $class->can($method);
	die "Can't locate object method \"$method\" via package \"" .
		($selfclass || $self) . '"' . (Value::getCaller() // '') unless $code;
	return !$selfclass ? $code : sub {
		unshift(@ISA, $class);
		my @result = wantarray ? Parser::Eval($code, @_) : (Parser::Eval($code, @_));
		shift(@ISA);
		die $@ if $@;
		return wantarray ? @result : $result[0];
	};
}

and remove the AUTOLOAD method and I think that will do the trick. This temporarily adds the original class to the @ISA array when calling the super-class method so that it inherits from the super class, and removes it afterward. It also adds an error message for when the super-class doesn't have the required method.

[dpvc]

Just to be clear, my proposed change to the super method above would mean that you don't need to make the changes to the significant-figures context that use fully qualified method names.

[drgrice1]

Interesting approach. I will try it out.

[dpvc]

I've changed the push() and pop() to unshift() and shift() just in case super gets called during the processing of $code (in the case where a context extends another extended context). That way, the inner call's class will be the first one in @ISA. But because extension contexts are supposed to subclass the context::Extension::Super class, they should each have their own @ISA, so I think this might alter the subclass @ISA array. If not, we can certainly arrange for that if it becomes a problem.

An alternative would be to use bless $self, $class in place of the push or unshift, and bless $self, $selfclass in place of the pop/shift in order to temporarily change the class so that it will inherit properly. That seems more aggressive, somehow, so I went with this approach. I'm not sure which is better.

[drgrice1]

I am seeing an issue with the t/math_objects/differentiation.t "differentiation with units" test with your code. Although the t/contexts/significant_figures_units.t tests pass.

All tests pass if I instead use the bless $self, $class approach as follows:

sub super {
	my ($self, $method, $context) = @_;
	my $selfclass = ref($self);
	my $class     = $self->superClass($context);
	my $code      = $class->can($method);
	die qq{Can't locate object method "$method" via package "}
		. ($selfclass || $self) . '"'
		. (Value::getCaller() // '')
		unless $code;
	return !$selfclass ? $code : sub {
		bless $self, $class;
		my @result = wantarray ? Parser::Eval($code, @_) : (Parser::Eval($code, @_));
		bless $self, $selfclass;
		die $@ if $@;
		return wantarray ? @result : $result[0];
	};
}

When using the unshift(@ISA, $class) approach the mentioned test gives Caught exception in subtest: Can't call method "copy" on an undefined value at /opt/webwork/pg/lib/Parser.pm line 87. It seems that adding $class to @ISA changes method resolution during the call, and so Parser::copy is resolved to and called because Parser::Formula gets added to @ISA, instead of Parser::BOP::copy.

The unit tests are passing with the bless $self, $class approach, but there could still be unintended consequences. With that approach nested calls could get redirected to the parent class methods instead of an override in the extension.

[dpvc]

Try removing the new function from the context::Extensions::Super package. It was there to handle the new calls before I had the AUTOLOAD function, and before the changes to super. It should no longer be needed, I think, and removing it causes all the math_object tests to pass.

[drgrice1]

Yeah, that seems to work. I will change this branch to that code.