Merge pull request #2441 from manandbytes/defcustom-secure-protocols

Make list of secure protocols customizable

el-get-custom.el

@@ -614,8 +614,17 @@ platforms where this recipe should apply"
 
 ;; TODO: this should be nil; change at the next major version bump
 (defcustom el-get-allow-insecure t
-  "Allow packages to be installed over insecure connections."
+  "Allow packages to be installed over insecure connections.
+
+See `el-get-insecure-check'."
   :group 'el-get
   :type 'boolean)
 
+(defcustom el-get-secure-protocols '("https" "ssh" "git+ssh" "bzr+ssh" "sftp")
+  "List of secure protocols.
+
+See `el-get-insecure-check'."
+  :group 'el-get
+  :type '(repeat string))
+
 (provide 'el-get-custom)

el-get-methods.el

@@ -12,6 +12,8 @@
 ;; Install
 ;;     Please see the README.md file from the same distribution
 (require 'el-get-core)
+(unless (version< emacs-version "24.4")
+  (require 'subr-x))
 
 ;;
 ;; NOTE: this will probably benefit from some autoloading magic, later.
@@ -22,17 +24,39 @@
                "methods"
                (file-name-directory (or load-file-name byte-compile-current-file buffer-file-name)))))
 
-(defun el-get-insecure-check (package url)
-  (when (and (not el-get-allow-insecure)
-             (not (string-match "^https://" url))
-             (not (string-match "^[-_\.A-Za-z0-9]+@" url))
-             (not (string-match "^ssh" url)))
-    ;; If we have :checksum, we can rely on `el-get-post-install' for
-    ;; security.
-    (unless (plist-get (el-get-package-def package) :checksum)
-      (error (concat "Attempting to install insecure package "
-                     (el-get-as-string package)
-                     " without `el-get-allow-insecure'.")))))
+(defun el-get-insecure-check (PACKAGE URL)
+  "Raise an error if it's not safe to install PACKAGE from URL.
+
+When `el-get-allow-insecure' is non-nil, check if either of the
+following is true and retun nil:
+
+- URL's protocol is in `el-get-secure-protocols'
+
+- URL starts with 'file:///' (without hostname), so it points to the
+  local file
+
+- URL starts with username, i.e. 'username@example.com', also known as
+  SCP-like syntax
+
+- PACKAGE definition has a non-empty :checksum"
+  (let* ((checksum (plist-get (el-get-package-def PACKAGE) :checksum))
+         (checksum-empty (or (not (stringp checksum))
+                             (if (fboundp 'string-blank-p)
+                                 (string-blank-p checksum)
+                               (string-match-p "\\`[ \t\n\r]*\\'" checksum)))))
+    (when (and (not el-get-allow-insecure)
+               (not (string-match "\\`file:///" URL))
+               (not (car (member 0 (mapcar (lambda (secure-proto)
+                                             (let ((proto-rx (concat "\\`" (regexp-quote secure-proto) "://")))
+                                               (string-match-p proto-rx URL))) el-get-secure-protocols))))
+               (not (string-match "\\`[-_\.A-Za-z0-9]+@" URL)))
+      ;; With not empty :checksum, we can rely on `el-get-post-install' calling
+      ;; `el-get-verify-checksum' for security.
+      (unless (not checksum-empty)
+        (error (concat "Attempting to install PACKAGE "
+                       (el-get-as-string PACKAGE)
+                       " from insecure URL " URL
+                       " without `el-get-allow-insecure'."))))))
 
 (require 'el-get-apt-get)
 (require 'el-get-builtin)

test/el-get-tests.el

@@ -125,3 +125,55 @@ Following variables are bound to temporal values:
      (should-not (featurep pkg))
      (el-get 'sync (mapcar 'el-get-source-name el-get-sources))
      (should (featurep pkg)))))
+
+(defconst insecure-urls '("http://example.com"
+                          "ftp://example.com"
+                          "file://example.com/home/user"
+                          ":pserver:anonymous@example.com"
+                        "
+https://example.com"
+                        "
+file:///home/user"
+                        "
+John.Doe-123_@example.com"))
+
+(ert-deftest el-get-insecure-check-insecure ()
+  "Insecure URL for a package without :checksum"
+  (dolist (url insecure-urls)
+    (let ((el-get-allow-insecure nil)
+          (el-get-sources '((:name "dummy" :type github))))
+      ;; TODO check for error message?
+      (should-error (el-get-insecure-check "dummy" url) :type 'error))))
+
+(defconst secure-urls '("https://example.com"
+                        "ssh://example.com"
+                        "git+ssh://example.com/"
+                        "bzr+ssh://example.com/"
+                        "sftp://example.com/"
+                        "file:///home/user"
+                        "file:///c|/WINDOWS/clock.avi"
+                        "file:///c:/WINDOWS/clock.avi"
+                        "John.Doe-123_@example.com"))
+
+(ert-deftest el-get-insecure-check-secure ()
+  "Secure URL for a package without :checksum doesn't matter"
+  (dolist (url secure-urls)
+    (let ((el-get-allow-insecure nil)
+          (el-get-sources '((:name "dummy" :type github))))
+      (should-not (el-get-insecure-check "dummy" url)))))
+
+(ert-deftest el-get-insecure-check-checksum ()
+  "Either secure or insecure URL for a package with :checksum"
+  (dolist (url (append insecure-urls secure-urls))
+    (let ((el-get-allow-insecure nil)
+          (el-get-sources '((:name "dummy" :type github :checksum "checksum"))))
+      (should-not (el-get-insecure-check "dummy" url)))))
+
+(ert-deftest el-get-insecure-check-checksum-empty ()
+  "Insecure URL for a package with empty :checksum"
+  (dolist (url insecure-urls)
+    (dolist (checksum '("" "   "))
+      (let ((el-get-allow-insecure nil)
+            (el-get-sources '((:name "dummy" :type github :checksum checksum))))
+        ;; TODO check for error message?
+        (should-error (el-get-insecure-check "dummy" url) :type 'error)))))