Conversation
Summary of ChangesHello @Zibbp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for integrating Kick as a supported streaming platform. It extends the application's data models, backend services, and frontend interface to recognize and interact with Kick, enabling future features like archiving Kick live streams and their associated chat. While marked as 'Work In Progress' and not yet fully functional, it establishes the necessary infrastructure for comprehensive Kick support. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // ConvertNDJSONToJSONArray converts a newline-delimited JSON file to a JSON array. | ||
| func ConvertNDJSONToJSONArrayInPlace(filePath string) error { | ||
| // Check if file starts with '[' | ||
| f, err := os.Open(filePath) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
General fix:
User-controlled file paths must be validated before being used in any filesystem operations, to avoid unauthorized file access or modification. This can be achieved by checking that the user-supplied file path is within a "safe" (whitelisted) directory.
Detailed fix:
- Clearly define a safe directory (e.g., a temp or uploads folder) in configuration, and ensure all file inputs must reside within that directory.
- Before using the path, combine it with the safe directory, normalize and resolve it using
filepath.Abs/filepath.Clean, and ensure the result actually remains within the safe root. - Reject any input not confined to the safe directory.
- Implement this check at the entry point—most robustly in
ConvertKickWebSocketChatToTDLChat—before passing the path along, but you could also defensively check inConvertNDJSONToJSONArrayInPlace. - You will need to import
path/filepathandstrings(if not already present in the target file).
Specific changes:
- Edit
internal/utils/tdl.go, in the functionConvertKickWebSocketChatToTDLChat(lines 345–352), to validate thepathargument before using it. You might add new parameters like a safe root or retrieve it from config. - Alternatively, do the validation at the start of
ConvertNDJSONToJSONArrayInPlaceininternal/utils/chat.go, but to match best practice, validate as early as possible.
| @@ -4,11 +4,12 @@ | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "regexp" | ||
| "sort" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
| //"time" | ||
|
|
||
| "github.com/google/uuid" | ||
| "github.com/rs/zerolog/log" | ||
| @@ -350,9 +347,23 @@ | ||
| channelID int, | ||
| chatStartTime time.Time, | ||
| ) error { | ||
| // Define a safe directory for chat files (update as appropriate) | ||
| const SafeChatDir = "/tmp/kickchats" // <-- CHANGE THIS to your actual safe upload dir (e.g., from config) | ||
| absSafeDir, err := filepath.Abs(SafeChatDir) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to resolve safe directory: %w", err) | ||
| } | ||
| absPath, err := filepath.Abs(path) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid chat file path: %w", err) | ||
| } | ||
| if !strings.HasPrefix(absPath, absSafeDir+string(os.PathSeparator)) && absPath != absSafeDir { | ||
| return fmt.Errorf("chat file path outside allowed directory") | ||
| } | ||
|
|
||
| log.Debug().Str("chat_file", path).Msg("Converting Kick WebSocket chat to TDL chat for rendering") | ||
| // Convert to JSON file | ||
| err := ConvertNDJSONToJSONArrayInPlace(path) | ||
| err = ConvertNDJSONToJSONArrayInPlace(absPath) | ||
| if err != nil { | ||
| return fmt.Errorf("converting kick chat to json: %w", err) | ||
| } |
|
|
||
| // Temp output file | ||
| tmpPath := filePath + ".tmp" | ||
| tmp, err := os.Create(tmpPath) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To safely use user-provided paths in filesystem operations, the code must strictly validate or restrict the user input. The most broadly-applicable method is to ensure that any path provided:
- Is a filename only (no slashes or ".." segments, if that is the intended use), or
- If it is a path, it must always be resolved within a safe directory.
In this context, since the input is coming untrusted from the HTTP API and used directly, the correct fix is:
- Restrict
filePathinConvertNDJSONToJSONArrayInPlaceto ensure it is always within a designated "safe directory" (such as a temp directory or application-data directory). - Accomplish this by resolving the absolute location of the candidate file, and checking its prefix against the safe directory's absolute path. If the check fails, return an error and abort.
- This check must be implemented at the entry-point function, before any sensitive file operations are performed.
- This will require knowledge of the intended safe directory (this can be injected via parameter, or can be hardcoded/configured as required).
Because of the function signature, the cleanest fix is to add such a check into ConvertNDJSONToJSONArrayInPlace itself, perhaps using an environment variable or config for the safe directory.
Changes required:
- Add package import(s) for
path/filepathand possiblyosif not present. - Define a safe base directory for all chat file operations. For demonstration, use
os.TempDir()or, if available via config, the proper temp/work directory. - In
ConvertNDJSONToJSONArrayInPlace, resolve and check thatfilePath(and thus alsotmpPath) both reside within the safe directory before further operations. - Return an error if the check fails.
No change to functionality is made for legitimate (well-formed) user input, but malicious paths will now be rejected.
| @@ -5,6 +5,7 @@ | ||
| "fmt" | ||
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "regexp" | ||
| "strings" | ||
| "unicode" | ||
| @@ -45,6 +46,20 @@ | ||
|
|
||
| // ConvertNDJSONToJSONArray converts a newline-delimited JSON file to a JSON array. | ||
| func ConvertNDJSONToJSONArrayInPlace(filePath string) error { | ||
| // Validate that filePath is within the allowed temp dir | ||
| safeDir := os.TempDir() // Ideally, replace with your actual safe directory if needed | ||
| absSafeDir, err := filepath.Abs(safeDir) | ||
| if err != nil { | ||
| return fmt.Errorf("could not resolve safe directory: %w", err) | ||
| } | ||
| absInput, err := filepath.Abs(filePath) | ||
| if err != nil { | ||
| return fmt.Errorf("could not resolve input file path: %w", err) | ||
| } | ||
| if len(absInput) < len(absSafeDir) || absInput[:len(absSafeDir)] != absSafeDir { | ||
| return fmt.Errorf("file path is outside allowed directory") | ||
| } | ||
|
|
||
| // Check if file starts with '[' | ||
| f, err := os.Open(filePath) | ||
| if err != nil { |
| f.Close() | ||
|
|
||
| // Replace original file | ||
| if err := os.Rename(tmpPath, filePath); err != nil { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
General approach:
User-controlled paths must never be used directly in filesystem operations without ensuring they cannot be manipulated to access or modify files outside of a safe directory. The best solution is to validate that the provided path is strictly within a designated temporary or upload folder (e.g., envConfig.TempDir), after resolving symlinks and normalizing the input. Reject any input that fails this check.
Detailed fix:
- In the handler (
ConvertKickChat), validate thatbody.LiveChatPathdoes not contain any absolute or parent directory navigation, and that the resolved absolute path (when joined with the intended base directory) starts with the safe base directory. If the input fails this check, return an error. - Only allow access to files inside a pre-configured directory such as
envConfig.TempDir. - No changes are needed in downstream functions (
ConvertKickWebSocketChatToTDLChat,ConvertNDJSONToJSONArrayInPlace) as long as the input is validated up front.
Files/Regions to change:
- Edit
internal/transport/http/archive.go, in theConvertKickChathandler, immediately after extracting thebody.LiveChatPathvalue, to validate and restrict it. - Add necessary
importlines if missing (e.g.,path/filepath,strings).
| @@ -14,6 +14,8 @@ | ||
| "github.com/zibbp/ganymede/internal/archive" | ||
| "github.com/zibbp/ganymede/internal/config" | ||
| "github.com/zibbp/ganymede/internal/utils" | ||
| "path/filepath" | ||
| "strings" | ||
| ) | ||
|
|
||
| type ArchiveService interface { | ||
| @@ -232,8 +234,21 @@ | ||
| envConfig := config.GetEnvConfig() | ||
| outPath := fmt.Sprintf("%s/%s-chat-convert.json", envConfig.TempDir, body.VideoID) | ||
|
|
||
| err = utils.ConvertKickWebSocketChatToTDLChat(body.LiveChatPath, outPath, body.ChannelName, body.VideoID, body.ChannelID, t) | ||
| // Validate that LiveChatPath is inside TempDir | ||
| absBase, err := filepath.Abs(envConfig.TempDir) | ||
| if err != nil { | ||
| return echo.NewHTTPError(http.StatusInternalServerError, "Invalid server configuration") | ||
| } | ||
| absPath, err := filepath.Abs(body.LiveChatPath) | ||
| if err != nil { | ||
| return echo.NewHTTPError(http.StatusBadRequest, "Invalid chat path") | ||
| } | ||
| if !strings.HasPrefix(absPath, absBase+string(filepath.Separator)) && absPath != absBase { | ||
| return echo.NewHTTPError(http.StatusBadRequest, "Chat path is outside allowed directory") | ||
| } | ||
|
|
||
| err = utils.ConvertKickWebSocketChatToTDLChat(absPath, outPath, body.ChannelName, body.VideoID, body.ChannelID, t) | ||
| if err != nil { | ||
| return echo.NewHTTPError(http.StatusInternalServerError, err.Error()) | ||
| } | ||
|
|
| f.Close() | ||
|
|
||
| // Replace original file | ||
| if err := os.Rename(tmpPath, filePath); err != nil { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix this issue, you must ensure that any path constructed from HTTP request data (here, body.LiveChatPath / path in subsequent calls) is validated before being used as a file path. The best way is to ensure the resolved absolute path is contained within an intended safe directory for chat files, using filepath.Abs and checking prefix with the safe directory’s absolute path (which must also be absolute). This check should be placed at the earliest opportunity before the path is passed to file APIs—in this case, within ConvertKickWebSocketChatToTDLChat just before any filesystem operations, since we’re limited to code in internal/utils/tdl.go and internal/utils/chat.go. Additionally, we should propagate the error up so the HTTP handler can report it as a bad request. You will need to:
- Choose (or determine from context) a safe base directory for chat files, and resolve paths relative to it.
- Import
"path/filepath"and"strings"ininternal/utils/tdl.goif needed. - Validate that the requested path’s absolute path is within the safe directory.
- Return an error if validation fails.
| @@ -9,6 +9,7 @@ | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
| "path/filepath" | ||
|
|
||
| "github.com/google/uuid" | ||
| "github.com/rs/zerolog/log" | ||
| @@ -350,9 +351,23 @@ | ||
| channelID int, | ||
| chatStartTime time.Time, | ||
| ) error { | ||
| // Validate that the input path is within the chat storage directory | ||
| const safeChatDir = "/tmp/kickchats" | ||
| absSafeDir, err := filepath.Abs(safeChatDir) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to resolve safe chat directory: %w", err) | ||
| } | ||
| absInputPath, err := filepath.Abs(path) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to resolve chat file path: %w", err) | ||
| } | ||
| if !strings.HasPrefix(absInputPath, absSafeDir+string(os.PathSeparator)) && absInputPath != absSafeDir { | ||
| return fmt.Errorf("invalid chat file path: must be within %s", absSafeDir) | ||
| } | ||
|
|
||
| log.Debug().Str("chat_file", path).Msg("Converting Kick WebSocket chat to TDL chat for rendering") | ||
| // Convert to JSON file | ||
| err := ConvertNDJSONToJSONArrayInPlace(path) | ||
| err = ConvertNDJSONToJSONArrayInPlace(path) | ||
| if err != nil { | ||
| return fmt.Errorf("converting kick chat to json: %w", err) | ||
| } |
| } | ||
|
|
||
| func OpenKickLiveChatFile(path string) ([]KickWebSocketChatMessage, error) { | ||
| liveChatJsonFile, err := os.Open(path) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, we must restrict the user-supplied path argument so that it cannot escape a "safe directory" intended for temporary chat files (as alluded to in other usages of envConfig.TempDir). The best way is to resolve the path with respect to the intended directory (e.g., the application's temp directory) and ensure the resulting path stays within that directory. This involves joining the provided relative path to the safe base directory, resolving the absolute path, and checking the prefix. If the base directory is not always absolute, resolve it first.
Edit regions:
internal/utils/live_chat.go: Edit theOpenKickLiveChatFilefunction to take abaseDirargument and validate the user input. The path is always constructed by joining this directory with the filename.internal/utils/tdl.go: UpdateConvertKickWebSocketChatToTDLChatand any callsites to pass the correct base directory toOpenKickLiveChatFile.- Possibly update the HTTP handler to ensure the base directory is given.
Required:
- Import
path/filepathandstringsinto files where used. - Validation code to ensure the path stays within the intended directory.
- Change usages accordingly.
| @@ -6,6 +6,8 @@ | ||
| "io" | ||
| "os" | ||
| "time" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "github.com/rs/zerolog/log" | ||
| ) | ||
| @@ -106,9 +108,23 @@ | ||
| return liveComments, nil | ||
| } | ||
|
|
||
| func OpenKickLiveChatFile(path string) ([]KickWebSocketChatMessage, error) { | ||
| liveChatJsonFile, err := os.Open(path) | ||
| func OpenKickLiveChatFile(baseDir, filename string) ([]KickWebSocketChatMessage, error) { | ||
| // Validate and clean filename | ||
| if strings.Contains(filename, "/") || strings.Contains(filename, "\\") || strings.Contains(filename, "..") { | ||
| return nil, fmt.Errorf("invalid filename") | ||
| } | ||
| safeDir, err := filepath.Abs(baseDir) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid base directory: %v", err) | ||
| } | ||
| fullPath := filepath.Join(safeDir, filename) | ||
| absPath, err := filepath.Abs(fullPath) | ||
| if err != nil || !strings.HasPrefix(absPath, safeDir) { | ||
| return nil, fmt.Errorf("invalid path: %v", err) | ||
| } | ||
|
|
||
| liveChatJsonFile, err := os.Open(absPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open chat file: %v", err) | ||
| } | ||
| defer func() { |
| @@ -357,7 +357,10 @@ | ||
| return fmt.Errorf("converting kick chat to json: %w", err) | ||
| } | ||
|
|
||
| liveComments, err := OpenKickLiveChatFile(path) | ||
| // Use a safe directory, e.g. from config/environment | ||
| envConfig := config.GetEnvConfig() | ||
| filename := filepath.Base(path) | ||
| liveComments, err := OpenKickLiveChatFile(envConfig.TempDir, filename) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open kick chat file: %v", err) | ||
| } |
2 participants
Publishing some ground work for Kick I worked on a few months ago. This is not functional at all. Maybe I'll pick this up again in the future.