Merge pull request #825 from clojars/tobias/security-headers

clojars · Apr 14, 2022 · dd3ef7e · dd3ef7e
2 parents 550fde4 + 0f5b7d3
commit dd3ef7e
Show file tree

Hide file tree

Showing 13 changed files with 129 additions and 120 deletions.
diff --git a/project.clj b/project.clj
@@ -49,7 +49,7 @@
                  [cheshire "5.10.1"]
                  [clj-stacktrace "0.2.8"]
                  [clj-time "0.15.2"]
-                 [ring/ring-anti-forgery "1.3.0"
+                 [ring/ring-defaults "0.3.3"
                   :exclusions [commons-codec
                                ;; newer version brought in by ring/ring-core
                                crypto-random]]

diff --git a/resources/public/js/selectText.js b/resources/public/js/selectText.js
@@ -0,0 +1,14 @@
+const selectText = (event) => {
+  if (window.getSelection) {
+    if (window.getSelection().type != 'Range') {
+      const range = document.createRange();
+      range.selectNodeContents(event.currentTarget);
+      window.getSelection().removeAllRanges();
+      window.getSelection().addRange(range);
+    }
+  }
+};
+
+$(() => {
+  $('.select-text').on('click', selectText);
+});
diff --git a/resources/public/stylesheets/screen.css b/resources/public/stylesheets/screen.css
@@ -1,5 +1,5 @@
 body {
-  font-family: "museo-sans-rounded", Arial, "Liberation Sans", FreeSans, sans-serif;
+  font-family: Arial, "Liberation Sans", FreeSans, sans-serif;
   background: #e2e4e3;
 }
 

diff --git a/src/clojars/http_utils.clj b/src/clojars/http_utils.clj
@@ -1,5 +1,6 @@
 (ns clojars.http-utils
   (:require
+   [clojure.string :as str]
    [ring.middleware.session :refer [wrap-session]]
    [ring.middleware.session.memory :as mem]))
 
@@ -8,11 +9,7 @@
     (let [response (handler req)]
       (if (= 200 (:status response))
                      (update-in response [:headers] assoc "Access-Control-Allow-Origin" "*")
-                     response
-                     ))))
-
-(defn wrap-x-frame-options [f]
-  (fn [req] (update-in (f req) [:headers] assoc "X-Frame-Options" "DENY")))
+                     response))))
 
 (defn https-request? [req]
   (or (= (:scheme req) :https)
@@ -37,3 +34,30 @@
       (if (https-request? req)
         (secure-session req)
         (regular-session req)))))
+
+(def ^:private content-security-policy
+  (str/join
+   ";"
+   ;; Load anything from the clojars domain
+   ["default-src 'self'"
+    ;; Load images from clojars domain along with dnsimple's logo
+    "img-src 'self' https://cdn.dnsimple.com"]))
+
+(def ^:private permissions-policy
+  ;; We need zero features
+  ;; Generated using https://www.permissionspolicy.com/
+  "accelerometer=(), ambient-light-sensor=(), autoplay=(), battery=(), camera=(), cross-origin-isolated=(), display-capture=(), document-domain=(), encrypted-media=(), execution-while-not-rendered=(), execution-while-out-of-viewport=(), fullscreen=(), geolocation=(), gyroscope=(), keyboard-map=(), magnetometer=(), microphone=(), midi=(), navigation-override=(), payment=(), picture-in-picture=(), publickey-credentials-get=(), screen-wake-lock=(), sync-xhr=(), usb=(), web-share=(), xr-spatial-tracking=(), clipboard-read=(), clipboard-write=(), gamepad=(), speaker-selection=(), conversion-measurement=(), focus-without-user-activation=(), hid=(), idle-detection=(), interest-cohort=(), serial=(), sync-script=(), trust-token-redemption=(), window-placement=(), vertical-scroll=()")
+
+(defn wrap-additional-security-headers [f]
+  (fn [req]
+    (-> (f req)
+        ;; Restrict content loading to help prevent xss
+        (assoc-in [:headers "Content-Security-Policy"] content-security-policy)
+        ;; Don't allow usage of any advanced js features
+        (assoc-in [:headers "Permissions-Policy"] permissions-policy)
+        ;; Clojars URLs don't have sensitive content, so we could get away with
+        ;; "unsafe-url" here. But that will give us a poorer score on
+        ;; https://securityheaders.com and lead to vulnerability reports from
+        ;; folks using automated tools, so we set this to just not share the
+        ;; referrer with non-secure sites.
+        (assoc-in [:headers "Referrer-Policy"] "no-referrer-when-downgrade"))))
diff --git a/src/clojars/web.clj b/src/clojars/web.clj
@@ -8,7 +8,7 @@
    [clojars.friend.oauth.github :as github]
    [clojars.friend.oauth.gitlab :as gitlab]
    [clojars.friend.registration :as registration]
-   [clojars.http-utils :refer [wrap-x-frame-options wrap-secure-session]]
+   [clojars.http-utils :refer [wrap-secure-session wrap-additional-security-headers]]
    [clojars.log :as log]
    [clojars.middleware :refer [wrap-ignore-trailing-slash]]
    [clojars.routes.api :as api]
@@ -27,14 +27,10 @@
    [clojure.java.io :as io]
    [compojure.core :refer [ANY context GET PUT routes]]
    [compojure.route :refer [not-found]]
-   [ring.middleware.anti-forgery :refer [wrap-anti-forgery]]
    [ring.middleware.content-type :refer [wrap-content-type]]
+   [ring.middleware.defaults :as ring-defaults]
    [ring.middleware.flash :refer [wrap-flash]]
-   [ring.middleware.keyword-params :refer [wrap-keyword-params]]
-   [ring.middleware.multipart-params :refer [wrap-multipart-params]]
-   [ring.middleware.not-modified :refer [wrap-not-modified]]
-   [ring.middleware.params :refer [wrap-params]]
-   [ring.middleware.resource :refer [wrap-resource]]))
+   [ring.middleware.not-modified :refer [wrap-not-modified]]))
 
 (defn try-parse-page
   "Will throw a targeted error if maybe-page doesn't parse as an integer."
@@ -94,6 +90,15 @@
                       [:h1 "Page not found"]
                       [:p "Thundering typhoons!  I think we lost it.  Sorry!"]]))))))
 
+(def ^:private defaults-config
+  (-> ring-defaults/secure-site-defaults
+      ;; Be more strict than the default; we never want to be frame-embedded
+      (assoc-in [:security :frame-options] :deny)
+      ;; we handle this in nginx
+      (update :security dissoc :ssl-redirect)
+      ;; We have our own session impl in http-utils
+      (dissoc :session)))
+
 (defn clojars-app
   [{:keys [db
            error-reporter
@@ -133,14 +138,12 @@
                        (gitlab/workflow gitlab http-client db)]})
          (wrap-exceptions error-reporter)
          (log/wrap-request-context)
-         (wrap-anti-forgery)
-         (wrap-x-frame-options)
-         (wrap-keyword-params)
-         (wrap-params)
-         (wrap-multipart-params)
+         ;; Use flash directly since we have custom session logic, so can't use
+         ;; ring-defaults' session support
          (wrap-flash)
+         (ring-defaults/wrap-defaults defaults-config)
+         (wrap-additional-security-headers)
          (wrap-secure-session)
-         (wrap-resource "public")
          (wrap-content-type)
          (wrap-not-modified)
          (wrap-ignore-trailing-slash)))))
diff --git a/src/clojars/web/common.clj b/src/clojars/web/common.clj
@@ -2,7 +2,7 @@
   (:require
    [clojars.maven :as maven]
    [clojars.web.helpers :as helpers]
-   [clojars.web.safe-hiccup :refer [html5 raw]]
+   [clojars.web.safe-hiccup :refer [form-to html5 raw]]
    [clojars.web.structured-data :as structured-data]
    [clojars.db :as db]
    [clojure.edn :as edn]
@@ -83,9 +83,6 @@
                         [:img {:src "/images/clojurists-together-logo.png" :alt "Clojurists Together Foundation" :height "40"}])]]]]]
     [:footer.row]))
 
-(defn typekit-js []
-  [:script "try{Typekit.load({async:true});}catch(e){}"])
-
 (defn html-doc [title ctx & body]
   (html5 {:lang "en"}
          [:head
@@ -109,10 +106,9 @@
          ;; (then the default colors were removed)
          ;; more info: http://getbootstrap.com/css/#grid
                ["reset.css" "vendor/bootstrap/bootstrap.css" "screen.css"])
-          (include-js "https://use.typekit.net/zhw0tse.js")
-          (typekit-js)
           (raw (when-ie (include-js "/js/html5.js")))
           (include-js "/js/jquery-3.6.0.min.js")
+          (include-js "/js/selectText.js")
           (for [path (:extra-js ctx)]
             (include-js path))]
          [:body.container-fluid
@@ -190,8 +186,6 @@
          ;; (then the default colors were removed)
          ;; more info: http://getbootstrap.com/css/#grid
                ["reset.css" "vendor/bootstrap/bootstrap.css" "screen.css"])
-          (include-js "https://use.typekit.net/zhw0tse.js")
-          (typekit-js)
           (raw (when-ie (include-js "/js/html5.js")))]
          [:body.container-fluid
           [:div.hero.row
@@ -435,3 +429,13 @@
         [:td (link-project audit)]
         [:td (linkify (:message audit))]
         [:td (:created audit)]])]])
+
+(defn form-table
+  [method-action label-input-pairs extra-inputs]
+  (form-to
+   method-action
+   [:div.form-table
+    [:table.form-table
+     (for [[label input] label-input-pairs]
+       [:tr [:td label] [:td input]])]
+    extra-inputs]))
diff --git a/src/clojars/web/dashboard.clj b/src/clojars/web/dashboard.clj
@@ -5,7 +5,6 @@
    [clojars.web.common :refer [audit-table html-doc html-doc-with-large-header
                                jar-link group-link tag
                                verified-group-badge verified-group-badge-small]]
-   [clojars.web.helpers :as helpers]
    [clojars.web.structured-data :as structured-data]
    [hiccup.element :refer [unordered-list link-to]]))
 
@@ -49,11 +48,10 @@
    structured-data/website
    structured-data/organisation
    [:article.row
-    (helpers/select-text-script)
     [:div.push-information.col-xs-12.col-sm-4
      [:h3.push-header "Push with "
       (link-to "http://leiningen.org/" "Leiningen")]
-     [:div#leiningen.push-example {:onClick "selectText('leiningen');"}
+     [:div#leiningen.push-example.select-text
       [:pre.push-example-leiningen
        (tag "$") " lein deploy clojars\n"]]]
     [:div.push-information.col-xs-12.col-sm-4
@@ -62,13 +60,13 @@
       " (using "
       (link-to "https://github.com/adzerk/bootlaces" "bootlaces")
       ")"]
-     [:div#boot.push-example {:onClick "selectText('boot');"}
+     [:div#boot.push-example.select-text
       [:pre.push-example-boot
        (tag "$") " boot build-jar push-snapshot\n"
        (tag "$") " boot build-jar push-release\n"]]]
     [:div.push-information.col-xs-12.col-sm-4
      [:h3.push-header "Maven Repository (running a " [:a {:href "https://github.com/clojars/clojars-web/wiki/Mirrors"} "mirror"] "?)"]
-     [:div#maven.push-example {:onClick "selectText('maven');"}
+     [:div#maven.push-example.select-text
       [:pre
        (tag "<repository>\n")
        (tag "  <id>") "clojars.org" (tag "</id>\n")

diff --git a/src/clojars/web/group.clj b/src/clojars/web/group.clj
@@ -2,7 +2,7 @@
   (:require
    [clojars.auth :refer [authorized-admin? authorized-member?]]
    [clojars.db :refer [find-group-verification jars-by-groupname]]
-   [clojars.web.common :refer [audit-table html-doc jar-link user-link error-list verified-group-badge-small]]
+   [clojars.web.common :refer [audit-table form-table html-doc jar-link user-link error-list verified-group-badge-small]]
    [clojars.web.safe-hiccup :refer [form-to]]
    [clojars.web.structured-data :as structured-data]
    [hiccup.element :refer [unordered-list]]
@@ -66,20 +66,16 @@
                (when admin?
                  [:div.add-member
                   [:h2 "Add member to group"]
-                  (form-to [:post (str "/groups/" groupname)]
-                           [:div
-                            [:label
-                             "Username "
-                             (text-field "username")]]
-                           [:div {:class "checkbox"}
-                            [:label
-                             "Admin? "
-                             [:input {:type "checkbox"
-                                      :name "admin"
-                                      :id "admin"
-                                      :value 1
-                                      :style "width:auto;margin-right:5px"
-                                      :checked false}]]]
-                           [:input.button {:type "submit" :value "Add Member"}])])
+                  (form-table
+                   [:post (str "/groups/" groupname)]
+                   [[[:label "Username "]
+                     (text-field "username")]
+                    [[:label "Admin? "]
+                     [:input {:type "checkbox"
+                              :name "admin"
+                              :id "admin"
+                              :value 1
+                              :checked false}]]]
+                   [:input.button {:type "submit" :value "Add Member"}])])
                (when show-membership-details?
                  (audit-table db groupname {:group-name groupname}))])))
diff --git a/src/clojars/web/helpers.clj b/src/clojars/web/helpers.clj
@@ -1,7 +1,6 @@
 (ns clojars.web.helpers
   (:require [clojure.java.io :as io]
-            [clojure.string :as str]
-            [clojars.web.safe-hiccup :as hiccup]))
+            [clojure.string :as str]))
 
 (defn public-resource-exists?
   "Takes a path and checks whether the resource exists under the public directory
@@ -37,19 +36,3 @@
            :srcset (->> (filter identity [(srcset-part base extension "2x")
                                           (srcset-part base extension "3x")])
                         (str/join ", "))}]))
-
-(defn select-text-script []
-  (hiccup/raw "<script>
-                     function selectText( containerid ) {
-                       var node = document.getElementById( containerid );
-
-                       if ( window.getSelection ) {
-                         if ( window.getSelection().type != \"Range\" ) {
-                           var range = document.createRange();
-                           range.selectNodeContents( node );
-                           window.getSelection().removeAllRanges();
-                           window.getSelection().addRange( range );
-                         }
-                       }
-                     }
-               </script>"))
diff --git a/src/clojars/web/jar.clj b/src/clojars/web/jar.clj
@@ -142,8 +142,7 @@
 (defn leiningen-coordinates [jar]
   (list
    [:h2 "Leiningen/Boot"]
-   [:div#leiningen-coordinates.package-config-example
-    {:onClick "selectText('leiningen-coordinates');"}
+   [:div#leiningen-coordinates.package-config-example.select-text
     [:pre
      (tag "[")
      (jar-name jar)
@@ -153,8 +152,7 @@
 (defn clojure-cli-coordinates [{:keys [group_name jar_name version]}]
   (list
    [:h2 "Clojure CLI/deps.edn"]
-   [:div#deps-coordinates.package-config-example
-    {:onClick "selectText('deps-coordinates');"}
+   [:div#deps-coordinates.package-config-example.select-text
     [:pre
      (str group_name "/" jar_name)
      \space
@@ -166,8 +164,7 @@
 (defn gradle-coordinates [{:keys [group_name jar_name version]}]
   (list
    [:h2 "Gradle"]
-   [:div#gradle-coordinates.package-config-example
-    {:onClick "selectText('gradle-coordinates');"}
+   [:div#gradle-coordinates.package-config-example.select-text
     [:pre
      "implementation("
      [:span.string \" group_name ":" jar_name ":" version \"]
@@ -176,8 +173,7 @@
 (defn maven-coordinates [{:keys [group_name jar_name version]}]
   (list
    [:h2 "Maven"]
-   [:div#maven-coordinates.package-config-example
-    {:onClick "selectText('maven-coordinates');"}
+   [:div#maven-coordinates.package-config-example.select-text
     [:pre
      (tag "<dependency>\n")
      (tag "  <groupId>") group_name (tag "</groupId>\n")
@@ -271,8 +267,8 @@
     "Want to display the "
     (link-to (version-badge-url jar) "latest version")
     " of your project on GitHub? Use the markdown code below!"]
-   [:textarea#version-badge
-    {:readonly "readonly" :rows 6 :onClick "selectText('version-badge')"}
+   [:textarea#version-badge.select-text
+    {:readonly "readonly" :rows 6}
     (badge-markdown jar)]))
 
 (defn show-jar [db stats account
@@ -296,7 +292,6 @@
       :data2       (format "[%s \"%s\"]" (jar-name jar) version)}
      [:div.light-article.row
       (breadcrumbs jar)
-      (helpers/select-text-script)
       [:div#jar-title.col-xs-12.col-sm-9
        [:div
         [:h1 (jar-link jar)]

diff --git a/src/clojars/web/safe_hiccup.clj b/src/clojars/web/safe_hiccup.clj
@@ -41,11 +41,12 @@ does some monkey patching to automatically escape strings."
   "Create a form that points to a particular method and route.
 e.g. (form-to [:put \"/post\"]
 ...)"
-  [[method action] & body]
-  `(if (contains? #{:head :get} ~method)
-     (form/form-to [~method ~action] ~@body)
-     (form/form-to [~method ~action]
-                   (conj
-                    (form/hidden-field "__anti-forgery-token"
-                                       *anti-forgery-token*)
-                    ~@body))))
+  [method-action & body]
+  `(let [[method# action#] ~method-action]
+     (if (contains? #{:head :get} method#)
+       (form/form-to [method# action#] ~@body)
+       (form/form-to [method# action#]
+                     (conj
+                      (form/hidden-field "__anti-forgery-token"
+                                         *anti-forgery-token*)
+                      ~@body)))))