fix(middleware): use world-readable permissions for payload files#773
fix(middleware): use world-readable permissions for payload files#773
Conversation
lpcox
commented
Feb 7, 2026
lpcox
commented
- Change directory permissions from 0700 to 0755
- Change file permissions from 0600 to 0644
- Allows agents running in containers to read payload files
- Change directory permissions from 0700 to 0755 - Change file permissions from 0600 to 0644 - Allows agents running in containers to read payload files
There was a problem hiding this comment.
Pull request overview
Updates the MCP gateway’s payload storage permissions so payload directories/files are readable by agents running in separate containers/users.
Changes:
- Change session and payload directory permissions from
0700to0755. - Change payload file permissions from
0600to0644. - Update unit tests to assert the new permission modes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/server/unified.go | Creates session payload directories with 0755 instead of 0700. |
| internal/server/unified_test.go | Updates expected session directory permissions to 0755. |
| internal/middleware/jqschema.go | Creates payload query directories with 0755 and writes payload.json with 0644; updates permission log strings. |
| internal/middleware/jqschema_test.go | Updates expected payload directory/file permissions to 0755/0644. |
Comments suppressed due to low confidence (2)
internal/middleware/jqschema.go:140
- The comment above
payload.jsonstill says the file is written with “restrictive permissions (owner read/write only)”, but the code now writes it as 0644. Please update the comment to match the new behavior to avoid misleading future changes and reviews.
// Save payload to file with restrictive permissions (owner read/write only)
filePath := filepath.Join(dir, "payload.json")
internal/middleware/jqschema.go:147
- Changing payload directories/files to 0755/0644 makes full tool responses world-readable on the host (default payload dir is under /tmp), which can expose secrets to any local user/process. Consider making this configurable (e.g., default to 0750/0640 with a shared group) or at least documenting the security tradeoff prominently and scoping access via a dedicated directory with tighter parent permissions.
if err := os.MkdirAll(dir, 0755); err != nil {
logger.LogError("payload", "Failed to create payload directory: path=%s, error=%v", dir, err)
return "", fmt.Errorf("failed to create payload directory: %w", err)
}
logger.LogDebug("payload", "Successfully created payload directory: path=%s, permissions=0755", dir)
// Save payload to file with restrictive permissions (owner read/write only)
filePath := filepath.Join(dir, "payload.json")
payloadSize := len(payload)
logger.LogInfo("payload", "Writing large payload to filesystem: path=%s, size=%d bytes (%.2f KB, %.2f MB)",
filePath, payloadSize, float64(payloadSize)/1024, float64(payloadSize)/(1024*1024))
if err := os.WriteFile(filePath, payload, 0644); err != nil {
logger.LogError("payload", "Failed to write payload file: path=%s, size=%d bytes, error=%v",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TestPayloadStorage_FilePermissions verifies that payload directories and files have secure permissions | ||
| func TestPayloadStorage_FilePermissions(t *testing.T) { | ||
| baseDir := t.TempDir() | ||
| sessionID := "test-permissions" | ||
| queryID := "test-query-perms" | ||
| payload := []byte(`{"secure": "data"}`) | ||
|
|
||
| filePath, err := savePayload(baseDir, sessionID, queryID, payload) | ||
| require.NoError(t, err) | ||
|
|
||
| // Check directory permissions | ||
| dirPath := filepath.Dir(filePath) | ||
| dirInfo, err := os.Stat(dirPath) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, os.FileMode(0700), dirInfo.Mode().Perm(), "Directory should have 0700 permissions") | ||
| assert.Equal(t, os.FileMode(0755), dirInfo.Mode().Perm(), "Directory should have 0755 permissions") | ||
|
|
||
| // Check file permissions | ||
| fileInfo, err := os.Stat(filePath) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, os.FileMode(0600), fileInfo.Mode().Perm(), "File should have 0600 permissions") | ||
| assert.Equal(t, os.FileMode(0644), fileInfo.Mode().Perm(), "File should have 0644 permissions") |
There was a problem hiding this comment.
This test (and its assertion messages) still describes the permissions as “secure”, but the expected modes were changed to 0755/0644 (world-readable). Please update the test description/message to reflect the new intent (agent-readable vs secure) so future failures are easier to interpret.
| if err := os.MkdirAll(dir, 0755); err != nil { | ||
| logger.LogError("payload", "Failed to create payload directory: path=%s, error=%v", dir, err) |
There was a problem hiding this comment.
os.MkdirAll only applies the provided mode when the directory is created; if the directory already exists its permissions are not updated. If this change is intended to make existing payload dirs readable by agents after an upgrade, add an explicit os.Chmod (and potentially for parent/session dirs) to ensure the effective permissions become 0755.
This issue also appears on line 132 of the same file.
| filePath, payloadSize, err) | ||
| return "", fmt.Errorf("failed to write payload file: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
os.WriteFile does not change permissions if payload.json already exists (it truncates but retains the existing mode). To guarantee agents can read old files created as 0600, follow the write with an explicit os.Chmod(filePath, 0644) (or open with Chmod on existing files) and handle any error.
This issue also appears on line 139 of the same file.
| // Ensure file has the expected permissions, even if it already existed | |
| if err := os.Chmod(filePath, 0644); err != nil { | |
| logger.LogError("payload", "Failed to set payload file permissions: path=%s, permissions=%04o, error=%v", | |
| filePath, 0644, err) | |
| return "", fmt.Errorf("failed to set payload file permissions: %w", err) | |
| } |
| // Check if directory already exists | ||
| if _, err := os.Stat(sessionDir); err == nil { | ||
| // Directory already exists | ||
| logUnified.Printf("Session directory already exists: %s", sessionDir) | ||
| return nil | ||
| } else if !os.IsNotExist(err) { | ||
| // Some other error occurred while checking | ||
| return fmt.Errorf("failed to check session directory: %w", err) | ||
| } | ||
|
|
||
| // Directory doesn't exist, create it with restrictive permissions (owner-only access) | ||
| if err := os.MkdirAll(sessionDir, 0700); err != nil { | ||
| // Directory doesn't exist, create it with world-readable permissions (for agent access) | ||
| if err := os.MkdirAll(sessionDir, 0755); err != nil { | ||
| return fmt.Errorf("failed to create session directory: %w", err) | ||
| } |
There was a problem hiding this comment.
ensureSessionDirectory returns early when the session directory exists, so sessions created before this change (0700) will remain unreadable to agents even after upgrading. To make this fix effective in real deployments, stat the existing directory and os.Chmod it to the desired mode (or at least add world/group read+exec) before returning.
1 participant
