Split up DownloadableURL() into it's individual components: SupportedURL(), DownloadableURL(), and ValidatedURL(). Updated all instances of DownloadableURL() to point to ValidatedURL(). Reverted the tests that are based on un-supported protocols.

builder/virtualbox/common/step_download_guest_additions.go

@@ -121,7 +121,7 @@ func (s *StepDownloadGuestAdditions) Run(state multistep.StateBag) multistep.Ste
 	}
 
 	// Convert the file/url to an actual URL for step_download to process.
-	url, err = common.DownloadableURL(url)
+	url, err = common.ValidatedURL(url)
 	if err != nil {
 		err := fmt.Errorf("Error preparing guest additions url: %s", err)
 		state.Put("error", err)

builder/virtualbox/ovf/config.go

@@ -97,7 +97,7 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) {
 	if c.SourcePath == "" {
 		errs = packer.MultiErrorAppend(errs, fmt.Errorf("source_path is required"))
 	} else {
-		c.SourcePath, err = common.DownloadableURL(c.SourcePath)
+		c.SourcePath, err = common.ValidatedURL(c.SourcePath)
 		if err != nil {
 			errs = packer.MultiErrorAppend(errs, fmt.Errorf("source_path is invalid: %s", err))
 		}

builder/virtualbox/ovf/config_test.go

@@ -76,6 +76,17 @@ func TestNewConfig_sourcePath(t *testing.T) {
 		t.Fatalf("Nonexistant file should throw a validation error!")
 	}
 
+	// Bad
+	c = testConfig(t)
+	c["source_path"] = "ftp://i/dont/exist"
+	_, warns, err = NewConfig(c)
+	if len(warns) > 0 {
+		t.Fatalf("bad: %#v", warns)
+	}
+	if err == nil {
+		t.Fatalf("should error")
+	}
+
 	// Good
 	tf := getTempFile(t)
 	defer os.Remove(tf.Name())

common/config.go

@@ -5,6 +5,7 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"time"
 )
@@ -42,58 +43,98 @@ func ChooseString(vals ...string) string {
 	return ""
 }
 
+// SupportedURL verifies that the url passed is actually supported or not
+// This will also validate that the protocol is one that's actually implemented.
+func SupportedURL(u *url.URL) bool {
+	// url.Parse shouldn't return nil except on error....but it can.
+	if u == nil {
+		return false
+	}
+
+	// build a dummy NewDownloadClient since this is the only place that valid
+	// protocols are actually exposed.
+	cli := NewDownloadClient(&DownloadConfig{})
+
+	// Iterate through each downloader to see if a protocol was found.
+	ok := false
+	for scheme, _ := range cli.config.DownloaderMap {
+		if strings.ToLower(u.Scheme) == strings.ToLower(scheme) {
+			ok = true
+		}
+	}
+	return ok
+}
+
 // DownloadableURL processes a URL that may also be a file path and returns
-// a completely valid URL. For example, the original URL might be "local/file.iso"
-// which isn't a valid URL. DownloadableURL will return "file:///local/file.iso"
+// a completely valid URL representing the requested file. For example,
+// the original URL might be "local/file.iso" which isn't a valid URL,
+// and so DownloadableURL will return "file://local/file.iso"
+// No other transformations are done to the path.
 func DownloadableURL(original string) (string, error) {
+	var result string
 
-	// Verify that the scheme is something we support in our common downloader.
-	supported := []string{"file", "http", "https", "smb"}
-	found := false
-	for _, s := range supported {
-		if strings.HasPrefix(strings.ToLower(original), s+"://") {
-			found = true
-			break
-		}
+	// Fix the url if it's using bad characters commonly mistaken with a path.
+	original = filepath.ToSlash(original)
+
+	// Check to see that this is a parseable URL with a scheme. If so, then just pass it through.
+	if u, err := url.Parse(original); err == nil && u.Scheme != "" && u.Host != "" {
+		return filepath.ToSlash(original), nil
 	}
 
-	// If it's properly prefixed with something we support, then we don't need
-	//	to make it a uri.
-	if found {
-		original = filepath.ToSlash(original)
-
-		// make sure that it can be parsed though..
-		uri, err := url.Parse(original)
+	// Since it's not a url, this might be a path. So, check that the file exists,
+	// then make it an absolute path so we can make a proper uri.
+	if _, err := os.Stat(original); err == nil {
+		result, err = filepath.Abs(filepath.FromSlash(original))
 		if err != nil {
 			return "", err
 		}
 
-		uri.Scheme = strings.ToLower(uri.Scheme)
-
-		return uri.String(), nil
-	}
-
-	// If the file exists, then make it an absolute path
-	_, err := os.Stat(original)
-	if err == nil {
-		original, err = filepath.Abs(filepath.FromSlash(original))
+		result, err = filepath.EvalSymlinks(result)
 		if err != nil {
 			return "", err
 		}
 
-		original, err = filepath.EvalSymlinks(original)
-		if err != nil {
-			return "", err
-		}
+		result = filepath.Clean(result)
+		result = filepath.ToSlash(result)
 
-		original = filepath.Clean(original)
-		original = filepath.ToSlash(original)
+		// We have no idea what this might be, so we'll leave it as is.
+	} else {
+		result = filepath.ToSlash(original)
 	}
 
-	// Since it wasn't properly prefixed, let's make it into a well-formed
-	//	file:// uri.
+	// We should have a path that can just turn into a file:// scheme'd url.
+	return fmt.Sprintf("file://%s", result), nil
+}
 
-	return "file://" + original, nil
+// Force the parameter into a url. This will transform the parameter into
+// a proper url, removing slashes, adding the proper prefix, etc.
+func ValidatedURL(original string) (string, error) {
+
+	// See if the user failed to give a url
+	if ok, _ := regexp.MatchString("(?m)^[^[:punct:]]+://", original); !ok {
+
+		// So since no magic was found, this must be a path.
+		result, err := DownloadableURL(original)
+		if err == nil {
+			return ValidatedURL(result)
+		}
+
+		return "", err
+	}
+
+	// Verify that the url is parseable...just in case.
+	u, err := url.Parse(original)
+	if err != nil {
+		return "", err
+	}
+
+	// We should now have a url, so verify that it's a protocol we support.
+	if !SupportedURL(u) {
+		return "", fmt.Errorf("Unsupported protocol scheme! (%#v)", u)
+	}
+
+	// We should now have a properly formatted and supported url
+	return u.String(), nil
 }
 
 // FileExistsLocally takes the URL output from DownloadableURL, and determines

common/config_test.go

@@ -36,15 +36,21 @@ func TestChooseString(t *testing.T) {
 	}
 }
 
-func TestDownloadableURL(t *testing.T) {
+func TestValidatedURL(t *testing.T) {
 	// Invalid URL: has hex code in host
-	_, err := DownloadableURL("http://what%20.com")
+	_, err := ValidatedURL("http://what%20.com")
+	if err == nil {
+		t.Fatalf("expected err : %s", err)
+	}
+
+	// Invalid: unsupported scheme
+	_, err = ValidatedURL("ftp://host.com/path")
 	if err == nil {
 		t.Fatalf("expected err : %s", err)
 	}
 
 	// Valid: http
-	u, err := DownloadableURL("HTTP://packer.io/path")
+	u, err := ValidatedURL("HTTP://packer.io/path")
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -69,7 +75,7 @@ func TestDownloadableURL(t *testing.T) {
 	}
 
 	for _, tc := range cases {
-		u, err := DownloadableURL(tc.InputString)
+		u, err := ValidatedURL(tc.InputString)
 		if u != tc.OutputURL {
 			t.Fatal(fmt.Sprintf("Error with URL %s: got %s but expected %s",
 				tc.InputString, tc.OutputURL, u))
@@ -77,11 +83,11 @@ func TestDownloadableURL(t *testing.T) {
 		if (err != nil) != tc.ErrExpected {
 			if tc.ErrExpected == true {
 				t.Fatal(fmt.Sprintf("Error with URL %s: we expected "+
-					"DownloadableURL to return an error but didn't get one.",
+					"ValidatedURL to return an error but didn't get one.",
 					tc.InputString))
 			} else {
 				t.Fatal(fmt.Sprintf("Error with URL %s: we did not expect an "+
-					" error from DownloadableURL but we got: %s",
+					" error from ValidatedURL but we got: %s",
 					tc.InputString, err))
 			}
 		}

common/download.go

@@ -86,8 +86,6 @@ func NewDownloadClient(c *DownloadConfig) *DownloadClient {
 
 	// Create downloader map if it hasn't been specified already.
 	if c.DownloaderMap == nil {
-		// XXX: Make sure you add any new protocols you implement
-		//      to the DownloadableURL implementation in config.go!
 		c.DownloaderMap = map[string]Downloader{
 			"file":  &FileDownloader{bufferSize: nil},
 			"http":  &HTTPDownloader{userAgent: c.UserAgent},

common/iso_config.go

@@ -111,7 +111,7 @@ func (c *ISOConfig) Prepare(ctx *interpolate.Context) (warnings []string, errs [
 	c.ISOChecksum = strings.ToLower(c.ISOChecksum)
 
 	for i, url := range c.ISOUrls {
-		url, err := DownloadableURL(url)
+		url, err := ValidatedURL(url)
 		if err != nil {
 			errs = append(
 				errs, fmt.Errorf("Failed to parse iso_url %d: %s", i+1, err))