- {projectId ? (
-
- ) : null}
+
Setup and usage
diff --git a/client/www/components/dash/auth/GitHub.tsx b/client/www/components/dash/auth/GitHub.tsx
index f9d2fe09dd..5b375524d2 100644
--- a/client/www/components/dash/auth/GitHub.tsx
+++ b/client/www/components/dash/auth/GitHub.tsx
@@ -6,7 +6,6 @@ import {
Copyable,
Copytext,
Fence,
- SectionHeading,
SubsectionHeading,
TextInput,
} from '@/components/ui';
@@ -23,6 +22,7 @@ import {
RedirectUrlInput,
EditableRedirectUrl,
RedirectForwardingNote,
+ OAuthCredentialsEditor,
} from './shared';
import { errorToast } from '@/lib/toast';
import { messageFromInstantError } from '@/lib/errors';
@@ -207,7 +207,39 @@ export function GitHubClient({
return (
-
+
+ Client ID from{' '}
+
+ GitHub OAuth Apps
+
+
+ }
+ clientSecretLabel={
+ <>
+ Client secret from{' '}
+
+ GitHub OAuth Apps
+
+
+ }
+ />
-
+
+ Client ID from{' '}
+
+ LinkedIn developer portal
+
+
+ }
+ clientSecretLabel={
+ <>
+ Client secret from{' '}
+
+ LinkedIn developer portal
+
+
+ }
+ />
;
redirect_to?: string | null;
+ discovery_endpoint?: string;
use_shared_credentials?: boolean;
};
}): Promise<{ client: OAuthClient }> {
@@ -445,3 +446,124 @@ export function EditableRedirectUrl({
);
}
+
+// Editor for a client's ID + secret, used by providers whose credentials are a
+// plain client id / client secret pair (GitHub, LinkedIn). The existing secret
+// is never returned to the dashboard, so this only ever sets a new value:
+// leaving the secret field blank keeps the current secret unchanged.
+export function OAuthCredentialsEditor({
+ app,
+ client,
+ token,
+ onUpdateClient,
+ clientIdCopyLabel,
+ clientIdLabel,
+ clientSecretLabel,
+}: {
+ app: InstantApp;
+ client: OAuthClient;
+ token: string;
+ onUpdateClient: (client: OAuthClient) => void;
+ clientIdCopyLabel: string;
+ clientIdLabel: ReactNode;
+ clientSecretLabel: ReactNode;
+}) {
+ const [isEditing, setIsEditing] = useState(false);
+ const [clientId, setClientId] = useState(client.client_id || '');
+ const [clientSecret, setClientSecret] = useState('');
+ const [isSaving, setIsSaving] = useState(false);
+
+ const openEditor = () => {
+ setClientId(client.client_id || '');
+ setClientSecret('');
+ setIsEditing(true);
+ };
+
+ const cancel = () => {
+ setIsEditing(false);
+ setClientId(client.client_id || '');
+ setClientSecret('');
+ };
+
+ const handleSave = async () => {
+ if (!clientId) {
+ errorToast('Missing client id', { autoClose: 5000 });
+ return;
+ }
+ try {
+ setIsSaving(true);
+ const resp = await updateClient({
+ token,
+ appId: app.id,
+ oauthClientID: client.id,
+ body: {
+ client_id: clientId,
+ ...(clientSecret ? { client_secret: clientSecret } : {}),
+ },
+ });
+ onUpdateClient(resp.client);
+ cancel();
+ successToast('Credentials updated');
+ } catch (e) {
+ console.error(e);
+ const msg =
+ messageFromInstantError(e as InstantIssue) ||
+ 'Error updating credentials.';
+ errorToast(msg, { autoClose: 5000 });
+ } finally {
+ setIsSaving(false);
+ }
+ };
+
+ if (!isEditing) {
+ return (
+
+
+
+
+
+
+ );
+ }
+
+ return (
+
+ );
+}
diff --git a/server/src/instant/auth/jwt.clj b/server/src/instant/auth/jwt.clj
index 9a0c975e03..7beb146421 100644
--- a/server/src/instant/auth/jwt.clj
+++ b/server/src/instant/auth/jwt.clj
@@ -1,11 +1,12 @@
(ns instant.auth.jwt
(:require
[chime.core :as chime-core]
- [clj-http.client :as clj-http]
[instant.util.cache :as cache]
[instant.util.exception :as ex]
+ [instant.util.json :as json]
[instant.util.lang :as lang]
- [instant.util.tracer :as tracer])
+ [instant.util.tracer :as tracer]
+ [instant.webhook-sender :as webhook-sender])
(:import
(com.auth0.jwk Jwk SigningKeyNotFoundException)
(com.auth0.jwt JWT)
@@ -28,11 +29,17 @@
(defn- get-keys [jwks-uri]
(tracer/with-span! {:name "jwt/get-keys"
:jwks-uri jwks-uri}
- (let [resp (clj-http/get jwks-uri {:as :json-string-keys})
- expires (if-let [expires-header (get-in resp [:headers "expires"])]
+ ;; jwks-uri comes from the untrusted discovery doc; use the SSRF-guarded client.
+ (let [resp (webhook-sender/safe-get jwks-uri)
+ _ (when-not (:success? resp)
+ (throw (ex-info "Unable to fetch JWKS."
+ {:jwks-uri jwks-uri :status (:status resp)})))
+ headers (:headers resp)
+ body (json/<-json (:body resp))
+ expires (if-let [expires-header (get headers "expires")]
(parse-rfc822 expires-header)
- (if-let [max-age (some-> resp
- (get-in [:headers "cache-control"])
+ (if-let [max-age (some-> headers
+ (get "cache-control")
(#(re-find #"max-age=(\d+)" %))
second)]
(.plus (Instant/now)
@@ -41,7 +48,7 @@
:attributes {:jwks-uri jwks-uri}}
;; Just set it to one hour if there is no expires header
(.plus (Instant/now) 1 ChronoUnit/HOURS))))
- body-keys (let [keys (get-in resp [:body "keys"])]
+ body-keys (let [keys (get body "keys")]
(if (< 100 (count keys))
(tracer/with-span!
{:name "jwk/too-many-keys"
diff --git a/server/src/instant/auth/oauth.clj b/server/src/instant/auth/oauth.clj
index 0bbeb9f97d..5ec7f81978 100644
--- a/server/src/instant/auth/oauth.clj
+++ b/server/src/instant/auth/oauth.clj
@@ -10,7 +10,8 @@
[instant.util.lang :as lang]
[instant.util.json :as json]
[instant.util.tracer :as tracer]
- [instant.util.url :as url])
+ [instant.util.url :as url]
+ [instant.webhook-sender :as webhook-sender])
(:import
(clojure.lang PersistentHashSet)
(instant.util.crypt Secret)
@@ -147,21 +148,23 @@
#_else
(.value client-secret))
- resp (clj-http/post token-endpoint
- {:throw-exceptions false
- :as :json
- :coerce :always
- :form-params {:client_id client-id
- :client_secret secret
- :code code
- :grant_type "authorization_code"
- :redirect_uri redirect-url}})]
- (if-not (clj-http/success? resp)
- {:type :error :message (get-in resp [:body :error_description] "Error exchanging code for token.")}
+ ;; token-endpoint comes from the untrusted discovery doc; use the
+ ;; SSRF-guarded client (validated in assert-safe-discovery-endpoints!).
+ resp (webhook-sender/safe-post-form
+ token-endpoint
+ {:client_id client-id
+ :client_secret secret
+ :code code
+ :grant_type "authorization_code"
+ :redirect_uri redirect-url})
+ resp-body (try
+ (some-> (:body resp) (json/<-json true))
+ (catch Exception _ nil))]
+ (if-not (:success? resp)
+ {:type :error :message (get resp-body :error_description "Error exchanging code for token.")}
(let [id-token (try
;; extract the id token data that has the email and sub from the id_token JWT
- (some-> resp
- :body
+ (some-> resp-body
:id_token
(string/split #"\.")
^String (second)
@@ -170,19 +173,17 @@
(json/<-json true))
(catch IllegalArgumentException _e
(tracer/with-span! {:name "oauth/invalid-id_token"
- :attributes {:id_token (-> resp :body :id_token)}})))
- access-token (-> resp
- :body
- :access_token)
+ :attributes {:id_token (:id_token resp-body)}})))
+ access-token (:access_token resp-body)
id-token (or id-token
(when (and access-token userinfo-endpoint)
(try
- (-> (clj-http/get userinfo-endpoint
- {:headers {:Authorization (str "Bearer " access-token)}
- :as :json
- :coerce :always})
- :body)
+ (let [ui-resp (webhook-sender/safe-get
+ userinfo-endpoint
+ :headers {"Authorization" (str "Bearer " access-token)})]
+ (when (:success? ui-resp)
+ (some-> (:body ui-resp) (json/<-json true))))
(catch Exception e
(tracer/record-exception-span! e {:name "oauth/invalid-user-info-from-endpoint"})
nil))))]
@@ -293,13 +294,15 @@
:imageURL imageURL})))
(defn fetch-discovery [endpoint]
- (let [resp (clj-http/get endpoint {:throw-exceptions false
- :as :json
- ;; for https://account.apple.com/.well-known/openid-configuration
- :headers {"User-Agent" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.6 Safari/605.1.15"}})]
- (if (clj-http/success? resp)
+ ;; Uses the webhook-sender client so discovery fetches are SSRF-guarded: the
+ ;; endpoint is user-supplied, so a plain fetch could target internal hosts.
+ (let [resp (webhook-sender/safe-get
+ endpoint
+ ;; for https://account.apple.com/.well-known/openid-configuration
+ :headers {"User-Agent" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.6 Safari/605.1.15"})]
+ (if (:success? resp)
{:date (Instant/now)
- :data (:body resp)}
+ :data (json/<-json (:body resp) true)}
(do
(tracer/record-exception-span! (ex-info "Error fetching discovery"
{:status (:status resp)
@@ -317,6 +320,19 @@
(defn get-discovery [endpoint]
(:data (cache/get discovery-endpoint-cache endpoint)))
+(defn assert-safe-discovery-endpoints!
+ "Rejects a discovery document whose server-fetched endpoints (token, userinfo,
+ jwks) point at unsafe (SSRF) hosts. The discovery-endpoint URL is
+ user-supplied, so its document is untrusted."
+ [{:keys [token_endpoint userinfo_endpoint jwks_uri] :as discovery-data}]
+ (when token_endpoint
+ (webhook-sender/assert-safe-url! token_endpoint))
+ (when userinfo_endpoint
+ (webhook-sender/assert-safe-url! userinfo_endpoint))
+ (when jwks_uri
+ (webhook-sender/assert-safe-url! jwks_uri))
+ discovery-data)
+
(defn generic-oauth-client-from-discovery-url [{:keys [app-id
provider-id
client-id
@@ -329,7 +345,8 @@
issuer
id_token_signing_alg_values_supported
- userinfo_endpoint]} (get-discovery discovery-endpoint)]
+ userinfo_endpoint]} (assert-safe-discovery-endpoints!
+ (get-discovery discovery-endpoint))]
(map->GenericOAuthClient {:app-id app-id
:provider-id provider-id
:client-id client-id
diff --git a/server/src/instant/webhook_sender.clj b/server/src/instant/webhook_sender.clj
index ea9b636c19..7e6c70766f 100644
--- a/server/src/instant/webhook_sender.clj
+++ b/server/src/instant/webhook_sender.clj
@@ -17,7 +17,7 @@
(java.util.concurrent Callable ExecutorService TimeUnit)
(java.util.function Predicate)
(javax.net.ssl SSLException)
- (okhttp3 ConnectionPool Dispatcher Dns HttpUrl MediaType OkHttpClient OkHttpClient$Builder Request$Builder RequestBody)
+ (okhttp3 ConnectionPool Dispatcher Dns FormBody$Builder Headers HttpUrl MediaType OkHttpClient OkHttpClient$Builder Request$Builder RequestBody Response)
(okhttp3.dnsoverhttps DnsOverHttps DnsOverHttps$Builder)))
(def ^{:tag 'bytes} period-bytes (.getBytes "." StandardCharsets/UTF_8))
@@ -189,3 +189,65 @@
(throw (Exception. "Could not resolve URL.")))
(catch Exception _
(ex/throw-validation-err! :webhook {:url input-url} [{:message "Could not resolve URL."}])))))
+
+(defn assert-safe-url!
+ "Parses url and rejects it if unparseable or if its host is an unsafe (SSRF)
+ literal IP. Does not make a request. Returns the parsed HttpUrl."
+ ^HttpUrl [^String url]
+ (let [parsed-url (HttpUrl/parse url)]
+ (when (nil? parsed-url)
+ (ex/throw-validation-err! :url {:url url} [{:message "Invalid URL."}]))
+ (ensure-safe-host! parsed-url)
+ parsed-url))
+
+(defn- response-headers->map [^Headers hs]
+ (into {} (map (fn [^String n] [(.toLowerCase n) (.get hs n)])) (.names hs)))
+
+(def max-response-bytes
+ "Upper bound on bytes read from a guarded response body, to bound memory for
+ hostile endpoints that return unbounded/oversized responses."
+ (* 5 1024 1024))
+
+(defn- read-capped-body ^String [^Response response]
+ (when (.body response)
+ ;; peekBody buffers at most (inc limit) bytes without reading the rest, so
+ ;; an oversized/unbounded body is capped even with no Content-Length header.
+ (let [bytes (.. response
+ (peekBody (inc (long max-response-bytes)))
+ (bytes))]
+ (when (> (alength bytes) max-response-bytes)
+ (throw (ex-info "Response body exceeds size limit"
+ {:limit max-response-bytes})))
+ (String. bytes StandardCharsets/UTF_8))))
+
+(defn- execute-response [^Request$Builder builder headers]
+ (doseq [[k v] headers]
+ (.header builder ^String k ^String v))
+ (with-open [response (.. client
+ (newCall (.build builder))
+ (execute))]
+ {:success? (.isSuccessful response)
+ :status (.code response)
+ :headers (response-headers->map (.headers response))
+ :body (read-capped-body response)}))
+
+(defn safe-get
+ "SSRF-safe HTTP GET using the guarded client (SSRF-defending DNS resolver plus
+ literal-IP check, no redirects). Returns {:success? bool :status int :body
+ string}. Throws for an unparseable URL, an unsafe host, or a network error."
+ [^String url & {:keys [headers]}]
+ (let [parsed-url (assert-safe-url! url)]
+ (execute-response (doto (Request$Builder.) (.url parsed-url)) headers)))
+
+(defn safe-post-form
+ "SSRF-safe form-encoded HTTP POST using the guarded client. form-params is a
+ map of name -> value. Same return/throw contract as safe-get."
+ [^String url form-params & {:keys [headers]}]
+ (let [parsed-url (assert-safe-url! url)
+ form (FormBody$Builder.)]
+ (doseq [[k v] form-params]
+ (.add form (name k) (str v)))
+ (execute-response (doto (Request$Builder.)
+ (.url parsed-url)
+ (.post (.build form)))
+ headers)))
diff --git a/server/test/instant/auth/oauth_test.clj b/server/test/instant/auth/oauth_test.clj
new file mode 100644
index 0000000000..f553362f96
--- /dev/null
+++ b/server/test/instant/auth/oauth_test.clj
@@ -0,0 +1,39 @@
+(ns instant.auth.oauth-test
+ (:require
+ [clojure.test :refer [deftest is testing]]
+ [instant.auth.oauth :as oauth]))
+
+(deftest fetch-discovery-throws-on-invalid-urls
+ (testing "unparseable / non-http(s) urls are rejected before any request"
+ (is (thrown? Exception (oauth/fetch-discovery "not-a-url")))
+ (is (thrown? Exception (oauth/fetch-discovery "ftp://example.com/x")))
+ (is (thrown? Exception (oauth/fetch-discovery "file:///etc/passwd"))))
+ (testing "urls whose host is an unsafe (SSRF) literal ip are rejected"
+ (is (thrown? Exception
+ (oauth/fetch-discovery
+ "http://169.254.169.254/latest/meta-data/")))
+ (is (thrown? Exception
+ (oauth/fetch-discovery
+ "http://127.0.0.1/.well-known/openid-configuration")))))
+
+(deftest rejects-discovery-doc-with-unsafe-token-endpoint
+ (testing "a safe discovery endpoint but unsafe token_endpoint is rejected"
+ (is (thrown? Exception
+ (oauth/assert-safe-discovery-endpoints!
+ {:token_endpoint "http://169.254.169.254/token"
+ :userinfo_endpoint "https://safe.example.com/userinfo"}))))
+ (testing "an unsafe userinfo_endpoint is rejected"
+ (is (thrown? Exception
+ (oauth/assert-safe-discovery-endpoints!
+ {:token_endpoint "https://safe.example.com/token"
+ :userinfo_endpoint "http://127.0.0.1/userinfo"}))))
+ (testing "an unsafe jwks_uri is rejected"
+ (is (thrown? Exception
+ (oauth/assert-safe-discovery-endpoints!
+ {:token_endpoint "https://safe.example.com/token"
+ :jwks_uri "http://169.254.169.254/jwks"}))))
+ (testing "a fully safe discovery document is accepted"
+ (is (oauth/assert-safe-discovery-endpoints!
+ {:token_endpoint "https://safe.example.com/token"
+ :userinfo_endpoint "https://safe.example.com/userinfo"
+ :jwks_uri "https://safe.example.com/jwks"}))))
diff --git a/server/test/instant/webhook_sender_test.clj b/server/test/instant/webhook_sender_test.clj
index 6810cf17ab..f4eaa2c42a 100644
--- a/server/test/instant/webhook_sender_test.clj
+++ b/server/test/instant/webhook_sender_test.clj
@@ -212,3 +212,29 @@
(crypt-util/random-hex 16) "."
(crypt-util/random-hex 16))))
"non-resolvable host is rejected"))
+
+(deftest safe-get-caps-response-body
+ ;; Bypass smokescreen so we can route to a local MockWebServer via nip.io, and
+ ;; shrink the cap so we don't have to allocate a multi-MB body.
+ (with-redefs [smokescreen/bad-ip? (constantly false)
+ webhook-sender/max-response-bytes 16]
+ (let [server (doto (MockWebServer.) (.start))]
+ (try
+ (.enqueue server (.. (MockResponse$Builder.) (body "small body") (build)))
+ (let [url (str "http://127.0.0.1.nip.io:" (.getPort server) "/ok")
+ resp (webhook-sender/safe-get url)]
+ (is (= {:success? true :status 200 :body "small body"}
+ (select-keys resp [:success? :status :body]))
+ "a body within the cap is returned intact"))
+
+ ;; chunked transfer encoding => no Content-Length header
+ (.enqueue server (.. (MockResponse$Builder.)
+ (chunkedBody ^String (apply str (repeat 100 \a)) 8)
+ (build)))
+ (let [url (str "http://127.0.0.1.nip.io:" (.getPort server) "/big")]
+ (is (thrown-with-msg? clojure.lang.ExceptionInfo
+ #"exceeds size limit"
+ (webhook-sender/safe-get url))
+ "a body over the cap is rejected even without Content-Length"))
+ (finally
+ (.close server))))))