diff --git a/lib/vector_mcp/image_util.rb b/lib/vector_mcp/image_util.rb index 838ab02..8df8881 100644 --- a/lib/vector_mcp/image_util.rb +++ b/lib/vector_mcp/image_util.rb @@ -231,29 +231,52 @@ def determine_final_mime_type(explicit_mime_type, detected_mime_type) # base_directory: "/app/uploads" # ) def file_to_mcp_image_content(file_path, validate: true, max_size: DEFAULT_MAX_SIZE, base_directory: nil) - validate_path_safety!(file_path, base_directory) if base_directory + validated_path = validate_path_safety!(file_path, base_directory) - raise ArgumentError, "Image file not found: #{file_path}" unless File.exist?(file_path) + raise ArgumentError, "Image file not found: #{validated_path}" unless File.exist?(validated_path) - raise ArgumentError, "Image file not readable: #{file_path}" unless File.readable?(file_path) + raise ArgumentError, "Image file not readable: #{validated_path}" unless File.readable?(validated_path) - binary_data = File.binread(file_path) + binary_data = File.binread(validated_path) to_mcp_image_content(binary_data, validate: validate, max_size: max_size) end - # Validates that a file path does not escape the given base directory. + # Validates that a file path is safe to access. + # + # When +base_directory+ is provided, the resolved path must reside within it. + # When +base_directory+ is omitted, the path is still canonicalized and + # rejected if it contains path traversal sequences (+..+) that resolve + # outside the current working directory. # # @param file_path [String] The file path to validate. - # @param base_directory [String] The base directory boundary. - # @raise [ArgumentError] If the resolved path is outside base_directory. + # @param base_directory [String, nil] Optional base directory boundary. + # @return [String] The canonicalized, validated path. + # @raise [ArgumentError] If path traversal is detected. # @api private def validate_path_safety!(file_path, base_directory) - resolved_base = File.expand_path(base_directory) - resolved_path = File.expand_path(file_path, resolved_base) + if base_directory + resolved_base = File.expand_path(base_directory) + resolved_path = File.expand_path(file_path, resolved_base) - return if resolved_path.start_with?("#{resolved_base}/") || resolved_path == resolved_base + unless resolved_path.start_with?("#{resolved_base}/") || resolved_path == resolved_base + raise ArgumentError, "Path traversal detected: resolved path is outside the allowed base directory" + end + else + resolved_path = File.expand_path(file_path) + + # Reject paths that use traversal sequences to escape upward, even without + # an explicit base directory. This catches the common case of + # user-supplied input like "../../etc/passwd". + if file_path.to_s.include?("..") + canonical_base = File.expand_path(".") + unless resolved_path.start_with?("#{canonical_base}/") || resolved_path == canonical_base + raise ArgumentError, + "Path traversal detected: '#{file_path}' resolves outside the working directory" + end + end + end - raise ArgumentError, "Path traversal detected: resolved path is outside the allowed base directory" + resolved_path end # Extracts image metadata from binary data. diff --git a/lib/vector_mcp/transport/http_stream.rb b/lib/vector_mcp/transport/http_stream.rb index e665ccb..d98ca6e 100644 --- a/lib/vector_mcp/transport/http_stream.rb +++ b/lib/vector_mcp/transport/http_stream.rb @@ -53,6 +53,16 @@ class HttpStream DEFAULT_EVENT_RETENTION = 100 # Keep last 100 events for resumability DEFAULT_REQUEST_TIMEOUT = 30 # Default timeout for server-initiated requests + # Default allowed origins — restrict to localhost by default for security. + DEFAULT_ALLOWED_ORIGINS = %w[ + http://localhost + https://localhost + http://127.0.0.1 + https://127.0.0.1 + http://[::1] + https://[::1] + ].freeze + # Initializes a new HTTP Stream transport. # # @param server [VectorMCP::Server] The server instance that will handle messages @@ -62,7 +72,8 @@ class HttpStream # @option options [String] :path_prefix ("/mcp") The base path for HTTP endpoints # @option options [Integer] :session_timeout (300) Session timeout in seconds # @option options [Integer] :event_retention (100) Number of events to retain for resumability - # @option options [Array] :allowed_origins (["*"]) List of allowed origins for CORS. Use ["*"] to allow all origins. + # @option options [Array] :allowed_origins Allowed origins for CORS validation. + # Defaults to localhost origins only. Pass ["*"] to allow all origins (NOT recommended for production). def initialize(server, options = {}) @server = server @logger = server.logger @@ -668,7 +679,13 @@ def valid_get_accept?(env) accept.include?("text/event-stream") || accept.include?("*/*") end - # Validates the Origin header for security + # Validates the Origin header for security. + # + # Matches are checked both exactly and as prefix (so that + # +http://localhost+ in the allowed list matches +http://localhost:3000+). + # + # Requests without an Origin header are allowed through because they + # originate from non-browser contexts (curl, server-to-server, etc.). # # @param env [Hash] The Rack environment # @return [Boolean] True if origin is allowed, false otherwise @@ -678,7 +695,9 @@ def valid_origin?(env) origin = env["HTTP_ORIGIN"] return true if origin.nil? # Allow requests without Origin header (e.g., server-to-server) - @allowed_origins.include?(origin) + @allowed_origins.any? do |allowed| + origin == allowed || origin.start_with?("#{allowed}:") + end end # Logging and error handling @@ -874,7 +893,17 @@ def initialize_configuration(options) @path_prefix = normalize_path_prefix(options[:path_prefix] || DEFAULT_PATH_PREFIX) @session_timeout = options[:session_timeout] || DEFAULT_SESSION_TIMEOUT @event_retention = options[:event_retention] || DEFAULT_EVENT_RETENTION - @allowed_origins = options[:allowed_origins] || ["*"] + @allowed_origins = options[:allowed_origins] || DEFAULT_ALLOWED_ORIGINS + + warn_on_permissive_origins if @allowed_origins.include?("*") + end + + # Logs a security warning when wildcard origin is configured. + def warn_on_permissive_origins + logger.warn do + "[SECURITY] allowed_origins includes '*', which permits cross-origin requests from any website. " \ + "This is not recommended for production. Specify explicit origins instead." + end end # Initialize core HTTP stream components diff --git a/spec/vector_mcp/image_util_spec.rb b/spec/vector_mcp/image_util_spec.rb index 2c239f5..d7d8421 100644 --- a/spec/vector_mcp/image_util_spec.rb +++ b/spec/vector_mcp/image_util_spec.rb @@ -354,7 +354,20 @@ end.to raise_error(ArgumentError, /Path traversal detected/) end - it "allows access without base_directory (backward compatible)" do + it "allows access without base_directory for non-traversal paths" do + result = described_class.file_to_mcp_image_content(temp_file.path) + expect(result[:type]).to eq("image") + end + end + + context "without base_directory (enforced traversal rejection)" do + it "rejects relative paths with .. that escape the working directory" do + expect do + described_class.file_to_mcp_image_content("../../etc/passwd") + end.to raise_error(ArgumentError, /Path traversal detected/) + end + + it "allows absolute paths that do not contain .." do result = described_class.file_to_mcp_image_content(temp_file.path) expect(result[:type]).to eq("image") end diff --git a/spec/vector_mcp/transport/http_stream_spec.rb b/spec/vector_mcp/transport/http_stream_spec.rb index 1c02c7b..35ffa0a 100644 --- a/spec/vector_mcp/transport/http_stream_spec.rb +++ b/spec/vector_mcp/transport/http_stream_spec.rb @@ -560,7 +560,7 @@ let(:restricted_transport) { described_class.new(mock_mcp_server, allowed_origins: ["https://example.com"]) } let(:restricted_app) { restricted_transport } - context "with unrestricted origins (default)" do + context "with default origins (localhost only)" do it "allows requests without Origin header" do post "/mcp", { "jsonrpc" => "2.0", "method" => "ping" }.to_json, valid_headers.merge("CONTENT_TYPE" => "application/json") @@ -568,10 +568,44 @@ expect(last_response.status).to eq(200) end - it "allows requests with any Origin header" do + it "allows requests from localhost origins" do + post "/mcp", { "jsonrpc" => "2.0", "method" => "ping" }.to_json, + valid_headers.merge("CONTENT_TYPE" => "application/json", "HTTP_ORIGIN" => "http://localhost:3000") + + expect(last_response.status).to eq(200) + end + + it "allows requests from 127.0.0.1 origins" do + post "/mcp", { "jsonrpc" => "2.0", "method" => "ping" }.to_json, + valid_headers.merge("CONTENT_TYPE" => "application/json", "HTTP_ORIGIN" => "http://127.0.0.1:8080") + + expect(last_response.status).to eq(200) + end + + it "rejects requests from non-localhost origins" do post "/mcp", { "jsonrpc" => "2.0", "method" => "ping" }.to_json, valid_headers.merge("CONTENT_TYPE" => "application/json", "HTTP_ORIGIN" => "https://malicious.com") + expect(last_response.status).to eq(403) + end + end + + context "with wildcard origins (opt-in)" do + let(:wildcard_transport) do + described_class.new(mock_mcp_server, allowed_origins: ["*"]) + end + let(:app) { wildcard_transport } + + before do + allow(wildcard_transport.session_manager).to receive(:get_or_create_session).with(session_id, anything).and_return(mock_session) + allow(wildcard_transport.session_manager).to receive(:get_session).with(session_id).and_return(mock_session) + allow(mock_session_context).to receive(:request_context=) + end + + it "allows requests from any origin" do + post "/mcp", { "jsonrpc" => "2.0", "method" => "ping" }.to_json, + valid_headers.merge("CONTENT_TYPE" => "application/json", "HTTP_ORIGIN" => "https://anywhere.com") + expect(last_response.status).to eq(200) end end