FIX: Ensure SAML follows after-login redirects (#49)

SAML flows end in a cross-site POST back to Discourse. We have the SameSite=lax attributes on our session cookies so this cross-site POST request has no cookies, and therefore we are unable to check any values in the `session`. This commit makes the browser re-submit the POST request in a SameSite context (i.e. with cookies). Upon receiving a cross-site POST, it renders a simple HTML form with some auto-submit JS. This form submits exactly the same data to the same URL, but this time the request will include the cookies, and authentication can complete properly

FIX: Ensure SAML follows after-login redirects (#49)
SAML flows end in a cross-site POST back to Discourse. We have the SameSite=lax attributes on our session cookies so this cross-site POST request has no cookies, and therefore we are unable to check any values in the `session`. This commit makes the browser re-submit the POST request in a SameSite context (i.e. with cookies). Upon receiving a cross-site POST, it renders a simple HTML form with some auto-submit JS. This form submits exactly the same data to the same URL, but this time the request will include the cookies, and authentication can complete properly
d137e981 · David Taylor · GitHub · 6209fc90 · d137e981 · d137e981
Unverified Commit d137e981 authored 3 years ago by David Taylor Committed by GitHub 3 years ago
--- a/lib/discourse_saml/saml_omniauth_strategy.rb
+++ b/lib/discourse_saml/saml_omniauth_strategy.rb
@@ -5,7 +5,31 @@ class ::DiscourseSaml::SamlOmniauthStrategy < OmniAuth::Strategies::SAML

  def request_phase
    if options[:request_method] == "POST"
-      render_auto_submitted_form
+      with_settings do |settings|
+        authn_request = OneLogin::RubySaml::Authrequest.new
+        params = authn_request.create_params(settings, additional_params_for_authn_request)
+        destination = settings.idp_sso_service_url
+        render_auto_submitted_form(
+          destination: destination,
+          params: params
+        )
+      end
+    else
+      super
+    end
+  end
+
+  def callback_phase
+    if request.request_method.downcase.to_sym == :post && !request.params["SameSite"] && request.params["SAMLResponse"]
+      # Make browser re-issue the request in a same-site context so we get cookies
+      # For this particular action, we explicitely **want** cross-site requests to include session cookies
+      render_auto_submitted_form(
+        destination: callback_url,
+        params: {
+          "SAMLResponse" => request.params["SAMLResponse"],
+          "SameSite" => "1"
+        }
+      )
    else
      super
    end
@@ -13,41 +37,41 @@ class ::DiscourseSaml::SamlOmniauthStrategy < OmniAuth::Strategies::SAML

  private

-  def render_auto_submitted_form
-    authn_request = OneLogin::RubySaml::Authrequest.new
-    with_settings do |settings|
-      saml_req = authn_request.create_params(settings, additional_params_for_authn_request)["SAMLRequest"]
-      destination_url = settings.idp_sso_service_url
+  def render_auto_submitted_form(destination:, params:)
+    submit_script_url = UrlHelper.absolute('/plugins/discourse-saml/javascripts/submit-form-on-load.js', GlobalSetting.cdn_url)

-      script_url = UrlHelper.absolute('/plugins/discourse-saml/javascripts/submit-form-on-load.js', GlobalSetting.cdn_url)
+    inputs = params.map do |key, value|
+      <<~HTML
+        <input type="hidden" name="#{CGI.escapeHTML(key)}" value="#{CGI.escapeHTML(value)}"/>
+      HTML
+    end.join("\n")

-      html = <<~HTML
-        <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
-          <body>
+    html = <<~HTML
+      <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
+        <body>
+          <noscript>
+            <p>
+              <strong>Note:</strong> Since your browser does not support JavaScript,
+              you must press the Continue button once to proceed.
+            </p>
+          </noscript>
+          <form action="#{CGI.escapeHTML(destination)}" method="post">
+            <div>
+              #{inputs}
+            </div>
            <noscript>
-              <p>
-                <strong>Note:</strong> Since your browser does not support JavaScript,
-                you must press the Continue button once to proceed.
-              </p>
-            </noscript>
-            <form action="#{destination_url}" method="post">
              <div>
-                <input type="hidden" name="SAMLRequest" value="#{saml_req}"/>
+                <input type="submit" value="Continue"/>
              </div>
-              <noscript>
-                <div>
-                  <input type="submit" value="Continue"/>
-                </div>
-              </noscript>
-            </form>
-            <script src="#{script_url}"></script>
-          </body>
-        </html>
-      HTML
+            </noscript>
+          </form>
+          <script src="#{CGI.escapeHTML(submit_script_url)}"></script>
+        </body>
+      </html>
+    HTML

-      r = Rack::Response.new
-      r.write(html)
-      r.finish
-    end
+    r = Rack::Response.new
+    r.write(html)
+    r.finish
  end
 end
--- a/spec/integration/saml_cross_site_spec.rb
+++ b/spec/integration/saml_cross_site_spec.rb
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe "SAML cross-site with same-site cookie", type: :request do
+  before do
+    OmniAuth.config.test_mode = false
+    global_setting :saml_target_url, "https://example.com/samlidp"
+  end
+
+  it "serves an auto-submitting POST form" do
+    post "/auth/saml/callback", params: { "SAMLResponse" => "somesamldata" }
+    expect(response.status).to eq(200)
+    expect(response.body).to have_tag(
+      "form",
+      with: {
+        "action" => "http://test.localhost/auth/saml/callback",
+        "method" => "post",
+      }
+    )
+
+    expect(response.body).to have_tag(
+      "form input",
+      with: {
+        "name" => "SAMLResponse",
+        "value" => "somesamldata",
+        "type" => "hidden",
+      }
+    )
+
+    expect(response.body).to have_tag(
+      "form input",
+      with: {
+        "name" => "SameSite",
+        "value" => "1",
+        "type" => "hidden",
+      }
+    )
+
+    expect(response.body).to have_tag("script")
+  end
+
+  it "continues once the samesite form has been submitted" do
+    post "/auth/saml/callback", params: { "SAMLResponse" => "somesamldata", "SameSite" => "1" }
+    expect(response.status).to eq(302)
+    expect(response.location).to eq("/auth/failure?message=invalid_ticket&strategy=saml")
+  end
+end