Check Task type before instantiating scheduler task classes#6325
Open
dkayiwa wants to merge 2 commits into
Open
Check Task type before instantiating scheduler task classes#6325dkayiwa wants to merge 2 commits into
dkayiwa wants to merge 2 commits into
Conversation
LegacyTask.execute and SchedulerFormValidator.validate loaded the configured task class name and called newInstance() on it before checking whether it implements Task, so pointing a scheduler task at any class on the classpath ran that class's constructor and static initializer as a side effect. Check Task.class.isAssignableFrom(taskClass) first and only instantiate classes that implement Task. Reaching this requires the Manage Scheduler privilege (such a user can already run arbitrary Task code), so this is defense in depth rather than a privilege boundary. Adds a regression test proving a non-Task class is rejected before it is instantiated, and repoints the existing instantiation-failure test at an abstract Task subtype so that branch stays covered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6325 +/- ##
============================================
- Coverage 59.47% 59.41% -0.07%
+ Complexity 9541 9536 -5
============================================
Files 731 731
Lines 38323 38321 -2
Branches 5587 5587
============================================
- Hits 22794 22769 -25
- Misses 13489 13521 +32
+ Partials 2040 2031 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JobRunrSchedulerService.saveTaskDefinition/scheduleTask do not run SchedulerFormValidator, so a TaskDefinition naming a class that does not implement Task can be persisted and reach LegacyTask.execute unvalidated. LegacyTask's own Task.class.isAssignableFrom check is the runtime guard on that path and had no test coverage. This adds LegacyTaskTest, which reuses the existing ConstructorSideEffectNonTask probe to assert the class is rejected with a TaskException before it is ever instantiated (its constructor side effect never runs). The probe's flag is widened to public so the new test in org.openmrs.scheduler can read it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TpKNfxm3QjwUBDePEtFm2H
|
ibacher
reviewed
Jul 17, 2026
ibacher
left a comment
ibacher
left a comment
Member
There was a problem hiding this comment.
Definitely an improvement, but see my comment below. I don't think we've regarded side-effects of constructors as load-bearing, especially when we assume it's a no-args constructor.
| new Object[] { taskDefinition.getTaskClass(), Task.class.getName() }, | ||
| "Class does not implement Task interface"); | ||
| } else { | ||
| taskClass.newInstance(); |
Member
There was a problem hiding this comment.
Um... what's the point of calling newInstance() here when we do exactly nothing with the instance?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LegacyTask.executeandSchedulerFormValidator.validateload the configuredtask_class_namethroughOpenmrsClassLoaderand callnewInstance()on it before checking whether the result implementsTask:api/.../scheduler/LegacyTask.java—taskClass.getDeclaredConstructor().newInstance()thenif (instance instanceof Task)api/.../validator/SchedulerFormValidator.java—taskClass.newInstance()thenif (!(o instanceof Task))So configuring a scheduler task with any fully-qualified class name on the classpath runs that class's constructor and static initializer as a side effect, even when it isn't a
Task.Change
Check
Task.class.isAssignableFrom(taskClass)first, and only instantiate classes that implementTask. The validator still instantiates a genuineTaskafterward so the existing instantiation/access error reporting is preserved.Severity
Reaching either path requires the
Manage Schedulerprivilege, and such a user can already schedule a realTaskthat runs arbitrary code, so this is defense in depth (no new privilege boundary crossed) rather than a critical bug.Tests
validate_shouldRejectANonTaskClassBeforeInstantiatingItuses a sentinel non-Taskclass with a constructor side effect and asserts it is rejected without being instantiated.validate_shouldFailValidationIfClassCannotBeInstantiatednow points at an abstractTasksubtype so the instantiation-failure branch is still exercised after the type check.Addresses GHSA-6ggw-67pv-j87p.