task arguments #717
task arguments #717
Conversation
| @@ -151,7 +151,8 @@ class JobManagementResource @Inject()(val jobScheduler: JobScheduler, | |||
| require(jobGraph.lookupVertex(jobName).isDefined, "Job '%s' not found".format(jobName)) | |||
| val job = jobGraph.getJobForName(jobName).get | |||
| log.info("Manually triggering job:" + jobName) | |||
| jobScheduler.taskManager.enqueue(TaskUtils.getTaskId(job, DateTime.now(DateTimeZone.UTC), 0), job.highPriority) | |||
| jobScheduler.taskManager.enqueue(TaskUtils.getTaskId(job, DateTime.now(DateTimeZone.UTC), 0, Option(arguments).filter(_.trim.nonEmpty)) | |||
There was a problem hiding this comment.
I'm not sure how job.arguments is scrubbed or sanitized, but looking at this ... seems like someone could easily embed a semi-colon and then append some other command after it - getting two for the price of one. You know, a little shell script injection. Are we worried about those kinds of things here? Or can we assume that someone else already scrubbed (or will scrub )the arguments query parameter?
There was a problem hiding this comment.
its a very good point. It seems that is missed in other parts of the code as well. This was suppose to be just a bug fix. That and the fact we are looking to move the whole thing to metronome I'm not sure if makes sense to spend too much time on it but I will have a look. At least to get rid of the semi-colon.
There was a problem hiding this comment.
If it's a PITA and exists in other parts of the codebase as well, maybe
just file a GH issue for it (because it's a problem in general) so as to
not block this PR
On Wed, Aug 24, 2016 at 8:27 AM, Ken Sipe notifications@github.com wrote:
In src/main/scala/org/apache/mesos/chronos/scheduler/api/
JobManagementResource.scala
#717 (comment):@@ -151,7 +151,8 @@ class JobManagementResource @Inject()(val jobScheduler: JobScheduler,
require(jobGraph.lookupVertex(jobName).isDefined, "Job '%s' not found".format(jobName))
val job = jobGraph.getJobForName(jobName).get
log.info("Manually triggering job:" + jobName)
jobScheduler.taskManager.enqueue(TaskUtils.getTaskId(job, DateTime.now(DateTimeZone.UTC), 0), job.highPriority)jobScheduler.taskManager.enqueue(TaskUtils.getTaskId(job, DateTime.now(DateTimeZone.UTC), 0, Option(arguments).filter(_.trim.nonEmpty))its a very good point. It seems that is missed in other parts of the code
as well. This was suppose to be just a bug fix. That and the fact we are
looking to move the whole thing to metronome I'm not sure if makes sense to
spend too much time on it but I will have a look. At least to get rid of
the semi-colon.—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/mesos/chronos/pull/717/files/40a7c0b42f1da0b3c60ae7989cf1d44e18d3cb3c#r76046360,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACPVLIlU0O1VzssA_dXMU0SEy9YdkgB2ks5qjDiggaJpZM4Jrgou
.
|
added protection against the evils of the world |
|
@jdef thanks! |
* using arguments if provided otherwise using the job arguments * fixing tests * adding additional tests for argument override * command injection fixed for this feature
enabling the ability to have the REST API override or add arguments to a job.