Deal with codeQL alert: Uncontrolled data used in path expression#257
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
Addresses a CodeQL alert (“Uncontrolled data used in path expression”) by tightening path handling in the benchmark dashboard and improving integer parsing safety in a reconciler utility.
Changes:
- Updated the benchmark dashboard server constructor to normalize
dataDirto an absolute path and propagate errors. - Reworked result/image path construction to use a new
isSafeRelativePathhelper and added additional input checks. - Improved
GetPodComponentIDparsing by usingstrconv.ParseIntwith error handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| pkg/reconciler/roleinstance/utils/instance_utils.go | Switches component ID parsing to ParseInt and adds explicit parse error handling. |
| cmd/cli/cmd/llm/benchmark/dashboard/main.go | Updates server creation to handle the new NewServer error return. |
| cmd/cli/cmd/llm/benchmark/dashboard/handlers.go | Introduces absolute dataDir, adds isSafeRelativePath, and refactors result/image path handling and validation. |
| cmd/cli/cmd/llm/benchmark/dashboard/handlers_test.go | Replaces prior path-safety tests with new tests focused on isSafeRelativePath and NewServer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| componentId := pod.Labels[constants.ComponentIDLabelKey] | ||
| if len(componentId) != 0 { | ||
| id, _ := strconv.Atoi(componentId) | ||
| id, err := strconv.ParseInt(componentId, 10, 32) | ||
| if err != nil { | ||
| return 0 | ||
| } | ||
| return int32(id) | ||
| } | ||
| list := strings.Split(pod.Name, "-") | ||
| componentId = list[len(list)-1] | ||
| id, _ := strconv.Atoi(componentId) | ||
| id, err := strconv.ParseInt(componentId, 10, 32) | ||
| if err != nil { | ||
| return 0 | ||
| } | ||
| return int32(id) |
There was a problem hiding this comment.
GetPodComponentID returns 0 when parsing the component ID fails. Since 0 is a valid replica index, this can cause collisions in callers (e.g., sets of existing IDs) and lead to incorrect scale/delete decisions when labels/pod names are malformed. Consider returning a sentinel value (e.g., -1) and/or falling back to parsing from pod.Name when the label parse fails, so parse errors don't masquerade as component ID 0.
| if strings.Contains(experimentName, "/") || strings.Contains(experimentName, "\\") || strings.Contains(experimentName, "..") { | ||
| s.writeError(w, http.StatusBadRequest, fmt.Sprintf("Invalid file name: %s", experimentName)) | ||
| return | ||
| } | ||
| if strings.Contains(fileName, "/") || strings.Contains(fileName, "\\") || strings.Contains(fileName, "..") { |
There was a problem hiding this comment.
The manual strings.Contains(name, "..") checks reject any name containing consecutive dots (e.g., exp..v2), while isValidName only rejects ".." as the whole name and otherwise permits dots. This creates inconsistent validation behavior between endpoints and helpers. Consider centralizing validation in isValidName/isSafeRelativePath and making the rules consistent (either allow .. inside names or explicitly disallow it in isValidName and update tests/docs).
| if strings.Contains(experimentName, "/") || strings.Contains(experimentName, "\\") || strings.Contains(experimentName, "..") { | |
| s.writeError(w, http.StatusBadRequest, fmt.Sprintf("Invalid file name: %s", experimentName)) | |
| return | |
| } | |
| if strings.Contains(fileName, "/") || strings.Contains(fileName, "\\") || strings.Contains(fileName, "..") { | |
| // Reject explicit path separators in single path components and rely on | |
| // isSafeRelativePath for centralized traversal-safe validation. | |
| if strings.Contains(experimentName, "/") || strings.Contains(experimentName, "\\") { | |
| s.writeError(w, http.StatusBadRequest, fmt.Sprintf("Invalid file name: %s", experimentName)) | |
| return | |
| } | |
| if strings.Contains(fileName, "/") || strings.Contains(fileName, "\\") { |
There was a problem hiding this comment.
Addresses CodeQL warning: "Uncontrolled data used in path expression".
We should enforce validation to prevent users from creating files with .. in their names.
Deal with "Uncontrolled data used in path expression" error reported by CodeQL