Skip to content

feat: implement text and file resource types#3522

Draft
dishaprakash wants to merge 1 commit into
rename-resource-managerfrom
mcp-resources-core
Draft

feat: implement text and file resource types#3522
dishaprakash wants to merge 1 commit into
rename-resource-managerfrom
mcp-resources-core

Conversation

@dishaprakash

Copy link
Copy Markdown
Contributor

feat: implement text and file resource types with path sandboxing, hidden file blocking, and boot-phase URI/collision validation

…dden file blocking, and boot-phase URI/collision validation

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for managing and validating Model Context Protocol (MCP) resources and resource templates (specifically file and text types) within the toolbox. It updates configuration parsing, command-line options, and server initialization to handle these new configurations, and implements boot-phase security checks (such as name/URI collision prevention and scheme validation). Feedback on the implementation highlights two security-related improvements in internal/resources/file.go: first, resolving symlinks during sandbox root initialization to prevent false-positive sandbox escape errors on systems with symlinked directories; second, performing hidden-file checks relative to the matched sandbox root to avoid incorrectly blocking access when the application is deployed inside a hidden directory.

Comment on lines +72 to +87
if len(c.AllowedPaths) > 0 {
for _, p := range c.AllowedPaths {
abs, err := filepath.Abs(p)
if err != nil {
return nil, fmt.Errorf("invalid allowed path %q for resource %q: %w", p, c.Name, err)
}
sandboxRoots = append(sandboxRoots, abs)
}
} else {
// Default sandbox is the directory where the configuration file lives
abs, err := filepath.Abs(configDir)
if err != nil {
return nil, fmt.Errorf("invalid config directory: %w", err)
}
sandboxRoots = append(sandboxRoots, abs)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The sandbox roots are initialized using filepath.Abs, which does not resolve symlinks. However, during a read operation, the requested path is resolved using filepath.EvalSymlinks. This mismatch can cause false-positive sandbox escape errors on systems where common directories are symlinks (for example, /var is a symlink to /private/var on macOS). Resolving symlinks for the sandbox roots during initialization prevents these false positives.

	if len(c.AllowedPaths) > 0 {
		for _, p := range c.AllowedPaths {
			abs, err := filepath.Abs(p)
			if err != nil {
				return nil, fmt.Errorf("invalid allowed path %q for resource %q: %w", p, c.Name, err)
			}
			if resolved, err := filepath.EvalSymlinks(abs); err == nil {
				abs = resolved
			}
			sandboxRoots = append(sandboxRoots, abs)
		}
	} else {
		// Default sandbox is the directory where the configuration file lives
		abs, err := filepath.Abs(configDir)
		if err != nil {
			return nil, fmt.Errorf("invalid config directory: %w", err)
		}
		if resolved, err := filepath.EvalSymlinks(abs); err == nil {
			abs = resolved
		}
		sandboxRoots = append(sandboxRoots, abs)
	}

Comment on lines +174 to +192
// 3. Sandbox Enforcement: verify path is strictly inside one of the sandbox roots
isInside := false
for _, root := range r.sandboxRoots {
if isPathInside(resolvedPath, root) {
isInside = true
break
}
}
if !isInside {
return nil, fmt.Errorf("permission denied: path %q escapes the allowed sandbox roots", filePath)
}

// 4. Hidden Files Check: block reading hidden files or directories (starting with '.')
segments := strings.Split(resolvedPath, string(filepath.Separator))
for _, seg := range segments {
if strings.HasPrefix(seg, ".") && seg != "." && seg != ".." {
return nil, fmt.Errorf("permission denied: reading hidden files or directories is not allowed")
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The hidden files check splits the absolute resolvedPath and blocks access if any segment starts with a dot (.). If the toolbox or its configuration is deployed inside a hidden directory (such as /home/user/.config/toolbox/), this check will incorrectly block reading any files, even if they are completely safe and within the allowed sandbox. Checking only the segments of the path relative to the matched sandbox root solves this issue while still preventing access to hidden files/directories inside the sandbox.

	// 3. Sandbox Enforcement: verify path is strictly inside one of the sandbox roots
	isInside := false
	var matchedRoot string
	for _, root := range r.sandboxRoots {
		if isPathInside(resolvedPath, root) {
			isInside = true
			matchedRoot = root
			break
		}
	}
	if !isInside {
		return nil, fmt.Errorf("permission denied: path %q escapes the allowed sandbox roots", filePath)
	}

	// 4. Hidden Files Check: block reading hidden files or directories (starting with '.')
	// Only check segments relative to the matched sandbox root to allow running from hidden directories.
	relPath, err := filepath.Rel(matchedRoot, resolvedPath)
	if err != nil {
		return nil, fmt.Errorf("failed to get relative path: %w", err)
	}
	segments := strings.Split(relPath, string(filepath.Separator))
	for _, seg := range segments {
		if strings.HasPrefix(seg, ".") && seg != "." && seg != ".." {
			return nil, fmt.Errorf("permission denied: reading hidden files or directories is not allowed")
		}
	}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant