-
Notifications
You must be signed in to change notification settings - Fork 45
picod: initialize and validate workspace when --workspace is set #196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
968e3d2
ecda6bc
05b4d49
ab2e0e9
94bf10d
61f8cf2
32499f3
c836c9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,6 +102,16 @@ func (s *Server) ExecuteHandler(c *gin.Context) { | |
| }) | ||
| return | ||
| } | ||
|
|
||
| // Ensure working directory exists | ||
| if err := os.MkdirAll(safeWorkingDir, 0755); err != nil { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still leaves the default path uncovered: when |
||
| c.JSON(http.StatusInternalServerError, gin.H{ | ||
| "error": fmt.Sprintf("failed to create working directory: %v", err), | ||
| "code": http.StatusInternalServerError, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| cmd.Dir = safeWorkingDir | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -389,14 +389,44 @@ func parseFileMode(modeStr string) os.FileMode { | |||||||||||||||||||
| // setWorkspace sets the global workspace directory | ||||||||||||||||||||
| func (s *Server) setWorkspace(dir string) { | ||||||||||||||||||||
| klog.Infof("setWorkspace called with dir: %q", dir) | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
|
Comment on lines
+392
to
+393
|
||||||||||||||||||||
| // Save original working directory before changing it (only once) | ||||||||||||||||||||
| if s.originalWorkingDir == "" { | ||||||||||||||||||||
| cwd, err := os.Getwd() | ||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||
| klog.Warningf("failed to get current working directory: %v", err) | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| s.originalWorkingDir = cwd | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+394
to
+402
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| // Resolve to absolute path | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
Comment on lines
+404
to
+405
|
||||||||||||||||||||
| absDir, err := filepath.Abs(dir) | ||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||
| klog.Warningf("Failed to resolve absolute path for workspace '%s': %v", dir, err) | ||||||||||||||||||||
| s.workspaceDir = dir // Fallback to provided path | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| s.workspaceDir = absDir | ||||||||||||||||||||
| klog.Infof("Resolved workspace to absolute path: %q", s.workspaceDir) | ||||||||||||||||||||
| klog.Fatalf("failed to resolve absolute path for workspace %q: %v", dir, err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Create directory if it doesn't exist | ||||||||||||||||||||
| if err := os.MkdirAll(absDir, 0755); err != nil { | ||||||||||||||||||||
| klog.Fatalf("failed to create workspace directory %q: %v", absDir, err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
|
Comment on lines
+392
to
+416
|
||||||||||||||||||||
| // Verify path exists and is a directory | ||||||||||||||||||||
| stat, err := os.Stat(absDir) | ||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||
| klog.Fatalf("failed to stat workspace directory %q: %v", absDir, err) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| if !stat.IsDir() { | ||||||||||||||||||||
| klog.Fatalf("workspace path %q is not a directory", absDir) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
Comment on lines
+416
to
+425
|
||||||||||||||||||||
| // Verify path exists and is a directory | |
| stat, err := os.Stat(absDir) | |
| if err != nil { | |
| klog.Fatalf("failed to stat workspace directory %q: %v", absDir, err) | |
| } | |
| if !stat.IsDir() { | |
| klog.Fatalf("workspace path %q is not a directory", absDir) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please respect this
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,15 @@ func setupTestServer(t *testing.T, pubPEM string) (*Server, *httptest.Server, st | |
| } | ||
|
|
||
| func TestPicoD_EndToEnd(t *testing.T) { | ||
| // Capture current working directory and restore it in cleanup | ||
| originalWd, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| if err := os.Chdir(originalWd); err != nil { | ||
| t.Logf("failed to restore working directory: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| // 1. Setup Keys - single key pair for Router-style auth | ||
| routerPriv, routerPubStr := generateRSAKeys(t) | ||
|
|
||
|
|
@@ -92,11 +101,8 @@ func TestPicoD_EndToEnd(t *testing.T) { | |
| defer os.Unsetenv(PublicKeyEnvVar) | ||
|
|
||
| // Switch to temp dir for relative path tests | ||
| originalWd, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| err = os.Chdir(tmpDir) | ||
| require.NoError(t, err) | ||
| defer func() { require.NoError(t, os.Chdir(originalWd)) }() | ||
|
|
||
| client := ts.Client() | ||
|
|
||
|
|
@@ -172,25 +178,55 @@ func TestPicoD_EndToEnd(t *testing.T) { | |
| assert.Equal(t, 124, resp.ExitCode) | ||
| assert.Contains(t, resp.Stderr, "Command timed out") | ||
|
|
||
| // 5. Working Directory Escape (Should Fail) | ||
| // 5. Non-existent Working Directory (Should Create It) | ||
| nonExistReq := ExecuteRequest{ | ||
| Command: []string{"sh", "-c", "pwd"}, | ||
| WorkingDir: "subdir/nested", | ||
| } | ||
| nonExistBody, _ := json.Marshal(nonExistReq) | ||
| nonExistClaims := jwt.MapClaims{ | ||
| "iat": time.Now().Unix(), | ||
| "exp": time.Now().Add(time.Hour * 6).Unix(), | ||
| } | ||
| nonExistToken := createToken(t, routerPriv, nonExistClaims) | ||
|
|
||
| nonExistReqHTTP, _ := http.NewRequest("POST", ts.URL+"/api/execute", bytes.NewBuffer(nonExistBody)) | ||
| nonExistReqHTTP.Header.Set("Authorization", "Bearer "+nonExistToken) | ||
| nonExistReqHTTP.Header.Set("Content-Type", "application/json") | ||
|
|
||
| httpResp, err := client.Do(nonExistReqHTTP) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, http.StatusOK, httpResp.StatusCode) | ||
|
|
||
| var nonExistResp ExecuteResponse | ||
| err = json.NewDecoder(httpResp.Body).Decode(&nonExistResp) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, 0, nonExistResp.ExitCode) | ||
| assert.NotEmpty(t, nonExistResp.Stdout) | ||
|
|
||
| // Verify directory was created | ||
| _, err = os.Stat("subdir/nested") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should remove this dir after test |
||
| assert.NoError(t, err) | ||
|
|
||
| // 6. Working Directory Escape (Should Fail) | ||
| escapeReq := ExecuteRequest{ | ||
| Command: []string{"ls"}, | ||
| WorkingDir: "../", | ||
| } | ||
| escapeBody, _ := json.Marshal(escapeReq) | ||
| claims := jwt.MapClaims{ | ||
| escapeClaims := jwt.MapClaims{ | ||
| "iat": time.Now().Unix(), | ||
| "exp": time.Now().Add(time.Hour * 6).Unix(), | ||
| } | ||
| token := createToken(t, routerPriv, claims) | ||
| escapeToken := createToken(t, routerPriv, escapeClaims) | ||
|
|
||
| req, _ := http.NewRequest("POST", ts.URL+"/api/execute", bytes.NewBuffer(escapeBody)) | ||
| req.Header.Set("Authorization", "Bearer "+token) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| escapeReqHTTP, _ := http.NewRequest("POST", ts.URL+"/api/execute", bytes.NewBuffer(escapeBody)) | ||
| escapeReqHTTP.Header.Set("Authorization", "Bearer "+escapeToken) | ||
| escapeReqHTTP.Header.Set("Content-Type", "application/json") | ||
|
|
||
| httpResp, err := client.Do(req) | ||
| escapeResp, err := client.Do(escapeReqHTTP) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, http.StatusBadRequest, httpResp.StatusCode) | ||
| assert.Equal(t, http.StatusBadRequest, escapeResp.StatusCode) | ||
| }) | ||
|
|
||
| t.Run("File Operations", func(t *testing.T) { | ||
|
|
@@ -328,17 +364,23 @@ func TestPicoD_EndToEnd(t *testing.T) { | |
| // requires public key at startup. Without it, PicoD will fail to start. | ||
|
|
||
| func TestPicoD_DefaultWorkspace(t *testing.T) { | ||
| // Capture current working directory and restore it in cleanup | ||
| originalWd, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| if err := os.Chdir(originalWd); err != nil { | ||
| t.Logf("failed to restore working directory: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| // Setup temporary directory for test | ||
| tmpDir, err := os.MkdirTemp("", "picod_default_workspace_test") | ||
| require.NoError(t, err) | ||
| defer os.RemoveAll(tmpDir) | ||
|
|
||
| // Switch to temp dir | ||
| originalWd, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| err = os.Chdir(tmpDir) | ||
| require.NoError(t, err) | ||
| defer func() { require.NoError(t, os.Chdir(originalWd)) }() | ||
|
|
||
| // Set public key env | ||
| _, pubStr := generateRSAKeys(t) | ||
|
|
@@ -352,6 +394,7 @@ func TestPicoD_DefaultWorkspace(t *testing.T) { | |
| } | ||
|
|
||
| server := NewServer(config) | ||
| defer server.RestoreWorkingDirectory() | ||
|
|
||
| // Verify workspaceDir is set to current working directory | ||
| cwd, err := os.Getwd() | ||
|
|
@@ -364,6 +407,15 @@ func TestPicoD_DefaultWorkspace(t *testing.T) { | |
| } | ||
|
|
||
| func TestPicoD_SetWorkspace(t *testing.T) { | ||
| // Capture current working directory and restore it in cleanup | ||
| originalWd, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| if err := os.Chdir(originalWd); err != nil { | ||
| t.Logf("failed to restore working directory: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| // Setup temp dir | ||
| tmpDir, err := os.MkdirTemp("", "picod_setworkspace_test") | ||
| require.NoError(t, err) | ||
|
|
@@ -401,11 +453,8 @@ func TestPicoD_SetWorkspace(t *testing.T) { | |
| assert.Equal(t, resolve(absPath), resolve(server.workspaceDir)) | ||
|
|
||
| // Case 2: Relative Path | ||
| originalWd, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| err = os.Chdir(tmpDir) | ||
| require.NoError(t, err) | ||
| defer func() { require.NoError(t, os.Chdir(originalWd)) }() | ||
|
|
||
| server.setWorkspace("real") | ||
| assert.Equal(t, resolve(absPath), resolve(server.workspaceDir)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,11 +34,12 @@ type Config struct { | |
|
|
||
| // Server defines the PicoD HTTP server | ||
| type Server struct { | ||
| engine *gin.Engine | ||
| config Config | ||
| authManager *AuthManager | ||
| startTime time.Time | ||
| workspaceDir string | ||
| engine *gin.Engine | ||
| config Config | ||
| authManager *AuthManager | ||
| startTime time.Time | ||
| workspaceDir string | ||
| originalWorkingDir string | ||
| } | ||
|
|
||
| // NewServer creates a new PicoD server instance | ||
|
|
@@ -110,6 +111,17 @@ func (s *Server) Run() error { | |
| return server.ListenAndServe() | ||
| } | ||
|
|
||
| // RestoreWorkingDirectory restores the process working directory to its original state | ||
| func (s *Server) RestoreWorkingDirectory() { | ||
| if s.originalWorkingDir != "" { | ||
| if err := os.Chdir(s.originalWorkingDir); err != nil { | ||
| klog.Warningf("failed to restore working directory to %q: %v", s.originalWorkingDir, err) | ||
| } else { | ||
| klog.Infof("restored working directory to: %q", s.originalWorkingDir) | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+114
to
+123
|
||
|
|
||
| // HealthCheckHandler handles health check requests | ||
| func (s *Server) HealthCheckHandler(c *gin.Context) { | ||
| c.JSON(http.StatusOK, gin.H{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation incorrectly states that "The process working directory is also changed to this workspace". However, the implementation in setWorkspace() does not call os.Chdir() to change the process working directory. The code only stores the workspace path in the workspaceDir field and uses it for path sanitization. Either the documentation should be corrected to remove this claim, or the implementation should be updated to match the documentation.