fix: Prevent lm-eval command escape #503
Open
Conversation
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
Contributor
Reviewer's GuideThis PR refactors command construction to eliminate shell invocation, integrates comprehensive input validation in the controller and CRD schemas to block unsafe patterns, and updates tests to reflect these changes. Sequence diagram for LMEvalJob input validation in controllersequenceDiagram
participant Controller
participant Validation
participant StatusUpdate
Controller->>Validation: ValidateUserInput(job)
alt Validation fails
Validation-->>Controller: error
Controller->>StatusUpdate: Update job.Status (State=Complete, Reason=Failed, Message)
Controller-->>Controller: Log error
Controller-->>Controller: Return error
else Validation passes
Validation-->>Controller: nil
Controller->>Controller: Continue with job processing
end
Class diagram for new validation logic and patternsclassDiagram
class LMEvalJob {
+LMEvalJobSpec Spec
+LMEvalJobStatus Status
}
class LMEvalJobSpec {
+string Model
+[]Arg ModelArgs
+TaskList TaskList
+string Limit
+[]Arg GenArgs
+*OfflineSpec Offline
+*int BatchSize
+string SystemInstruction
+*ChatTemplate ChatTemplate
}
class TaskList {
+[]string TaskNames
+[]TaskRecipe TaskRecipes
+*CustomArtifacts CustomArtifacts
+*CustomTasks CustomTasks
}
class Validation {
+ValidateUserInput(job)
+ValidateModelName(model)
+ValidateArgs(args, argType)
+ValidateTaskNames(taskNames)
+ValidateTaskRecipes(args, argType)
+ValidateCustomArtifactValue(input)
+ValidateLimit(limit)
+ValidateS3Path(path)
+ValidateBatchSizeInput(batchSize)
+ValidateSystemInstruction(instruction)
+ValidateChatTemplateName(name)
+ValidateGitURL(url)
+ValidateGitPath(path)
+ValidateGitBranch(branch)
+ValidateGitCommit(commit)
+ContainsShellMetacharacters(input)
}
class Patterns {
+PatternModelName
+PatternTemplateName
+PatternArgName
+PatternArgValue
+PatternLimit
+PatternTaskName
+AllowedModels
}
LMEvalJobSpec o-- TaskList
TaskList o-- CustomArtifacts
TaskList o-- CustomTasks
LMEvalJob o-- LMEvalJobSpec
LMEvalJob o-- LMEvalJobStatus
Validation ..> Patterns : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @ruivieira - I've reviewed your changes - here's some feedback:
- The PR mixes shell‐escaping fixes and extensive input validation/CRD changes — consider splitting it into smaller, focused PRs (one for command generation and one for validation/schema updates) for easier review and maintainability.
- The validation logic and kubebuilder regex tags are duplicated across patterns.go, validation.go and the CRD YAML — consider centralizing these patterns to avoid drift and ensure consistency.
- Injecting ValidateUserInput in the controller is helpful, but it might be cleaner to move input validation into a webhook (or a dedicated admission step) so invalid CRs are rejected before reconciliation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PR mixes shell‐escaping fixes and extensive input validation/CRD changes — consider splitting it into smaller, focused PRs (one for command generation and one for validation/schema updates) for easier review and maintainability.
- The validation logic and kubebuilder regex tags are duplicated across patterns.go, validation.go and the CRD YAML — consider centralizing these patterns to avoid drift and ensure consistency.
- Injecting ValidateUserInput in the controller is helpful, but it might be cleaner to move input validation into a webhook (or a dedicated admission step) so invalid CRs are rejected before reconciliation.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
PR image build and manifest generation completed successfully! 📦 PR image: 📦 LMES driver image: 📦 LMES job image: 📦 Guardrails orchestrator image: 🗂️ CI manifests |
christinaexyou
approved these changes
Jun 20, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: christinaexyou The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
|
@ruivieira: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Collaborator
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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 by Sourcery
Implement strict input validation and secure command generation for LMEvalJob: introduce ValidateUserInput, integrate it into the reconciler to reject malicious CRs, harden CRD schema with patterns/enums, refactor CLI construction to use argument slices, and expand test coverage accordingly.
New Features:
Bug Fixes:
sh -ecand sanitizing all CLI inputsEnhancements:
Tests: