fix(hub): preserve requested page through sign-in

hub/app/controllers/concerns/authentication.rb

@@ -26,7 +26,10 @@ def resume_session
   def require_authentication
     resume_session
     if Current.user.nil?
-      session[:authentication_return_to] = request.path if request.path.match?(%r{\A/canvas/pair/[0-9A-F]{10}\z})
+      # Leave cookie space for OAuth state and the rest of the session.
+      if request.get? && request.fullpath.bytesize <= 1.kilobyte
+        session[:authentication_return_to] = request.fullpath
+      end
       redirect_to root_path
     end
     response.headers["Cache-Control"] = "private, no-store" if Current.user
@@ -37,6 +40,14 @@ def authenticated?
     Current.user.present?
   end
 
+  # Consumes the destination before sign-in rotates the browser session.
+  def authentication_return_to
+    value = session.delete(:authentication_return_to)
+    return if !value.is_a?(String) || !value.match?(%r{\A/(?!/)[^\x00-\x20\\]*\z})
+
+    url_from(value)
+  end
+
   # Rotates the encrypted cookie and records a revocable browser session.
   def start_new_session_for(user, fresh_authorized_at: nil,
     generation: SessionRevocation.current.generation, configuration_id: nil, credential_revision: nil)

hub/app/controllers/github_authentications_controller.rb

@@ -113,14 +113,6 @@ def discard_oauth_flow
     end
   end
 
-  # Returns the saved preview authorization path when it has the expected shape.
-  def authentication_return_to
-    value = session.delete(:authentication_return_to)
-    return if !value.to_s.match?(%r{\A(?:/preview/builds/[1-9]\d*/authorize(?:\?[^\r\n]*)?|/canvas/pair/[0-9A-F]{10})\z})
-
-    value
-  end
-
   # Returns one generic failure that keeps setup and repository state private.
   def authentication_failed
     redirect_to root_path, alert: I18n.t("authentication.failure")

hub/app/controllers/local_authentications_controller.rb

@@ -16,7 +16,7 @@ def create
         status: :see_other
     end
 
-    return_to = session.delete(:authentication_return_to)
+    return_to = authentication_return_to
     user = User.local_developer!
     start_new_session_for(user)
     show_welcome = user.consume_welcome_sign_in!

hub/docs/authentication-and-repository-authorization.md

@@ -12,7 +12,9 @@ Session -> Hub asks GitHub for current permission -> show repository or deny acc
 
 Hub admits repository users when they can read at least one installed repository. A `none` permission on another installed repository does not prevent sign-in. Explicitly enrolled Hub operators can also sign in to administer the deployment without repository access. Hub stores their GitHub ID and current profile, but not their OAuth token. The browser cookie points to a `Session` record that Hub deletes when the person signs out.
 
-Signed-in visits to `/` and successful sign-ins go to `/repositories`, regardless of Hub administrator status. Sign-in preserves a saved preview authorization or canvas pairing destination. The repository entry point opens the first readable repository or returns not found when none is available. Hub administrators can still open `/security_audit` directly.
+Signed-out visits to protected pages save the requested internal path and query for after sign-in, including Create, preview authorization, and canvas pairing. Hub consumes the saved destination before rotating the browser session and rejects external URLs, protocol-relative paths, backslashes, whitespace, and control characters. Only GET destinations are saved; sign-in never replays a mutation. The destination still requires its normal access checks.
+
+Without a saved destination, a successful sign-in opens the welcome page for a new user or `/repositories` for a returning user, regardless of Hub administrator status. Signed-in visits to `/` go to `/repositories`. The repository entry point opens the first readable repository or returns not found when none is available. Hub administrators can still open `/security_audit` directly.
 
 Before showing a repository, Hub confirms that its App is still installed there and asks GitHub whether the signed-in person can access it. Hub also confirms that GitHub's answer belongs to that person.
 

hub/test/integration/authentication_test.rb

@@ -18,14 +18,108 @@ class AuthenticationTest < ActionDispatch::IntegrationTest
     assert_includes response.body, I18n.t("authentication.failure")
   end
 
-  test "explains when GitHub sign-in succeeds without repository access" do
+  test "returns to Create with its query after signing in through the public home" do
+    github_app_configurations.create_installed
+    repository = repository_payload(1379291646, "github/prototype")
+    stub_github_sign_in(repositories: [ repository ], permissions: { 1379291646 => "pull" })
+    destination = "#{repository_new_prototype_path(1379291646)}?template=default&brief=hello%20world"
+
+    get destination
+
+    assert_redirected_to root_path
+    follow_redirect! headers: { "Accept" => "application/json" }
+    assert_response :success
+
+    post github_authentication_path
+    state = Rack::Utils.parse_query(URI(response.location).query).fetch("state")
+    get github_authentication_callback_path, params: { code: "code", state: }
+
+    assert_redirected_to destination
+    follow_redirect! headers: { "Accept" => "application/json" }
+    assert_response :success
+    assert_equal "new", response.parsed_body.fetch("data").fetch("selectedSection")
+
+    delete session_path
+    sign_in_to_hub
+    assert_redirected_to repositories_path
+  end
+
+  test "a saved Create destination still requires repository access after sign-in" do
+    github_app_configurations.create_installed
+    installed = [
+      repository_payload(1379291646, "github/private").merge("private" => true),
+      repository_payload(2002, "github/readable").merge("private" => true)
+    ]
+    stub_github_sign_in(repositories: installed, permissions: { 1379291646 => nil, 2002 => "pull" })
+    destination = repository_new_prototype_path(1379291646)
+
+    get destination
+    assert_redirected_to root_path
+    follow_redirect! headers: { "Accept" => "application/json" }
+    post github_authentication_path
+    state = Rack::Utils.parse_query(URI(response.location).query).fetch("state")
+    get github_authentication_callback_path, params: { code: "code", state: }
+
+    assert_redirected_to destination
+    follow_redirect!
+    assert_response :not_found
+    assert_not_includes response.body, "github/private"
+  end
+
+  [ "new", "returning" ].each do |visitor|
+    test "does not replay a signed-out POST and keeps the #{visitor} user's default destination" do
+      github_app_configurations.create_installed
+      users.create(welcome_sign_in_pending: false) if visitor == "returning"
+      repository = repository_payload(1379291646, "github/prototype")
+      stub_github_sign_in(repositories: [ repository ], permissions: { 1379291646 => "pull" })
+
+      post poll_repository_path(1379291646)
+
+      assert_redirected_to root_path
+      follow_redirect! headers: { "Accept" => "application/json" }
+      post github_authentication_path
+      state = Rack::Utils.parse_query(URI(response.location).query).fetch("state")
+      get github_authentication_callback_path, params: { code: "code", state: }
+
+      assert_redirected_to(visitor == "new" ? welcome_path : repositories_path)
+    end
+  end
+
+  {
+    "external URL" => "https://outside.example/repositories/1379291646/new",
+    "protocol-relative URL" => "//outside.example/repositories/1379291646/new",
+    "backslash" => "/\\outside.example/repositories/1379291646/new",
+    "control character" => "/repositories/1379291646/new\n",
+    "space" => "/repositories/1379291646/new?brief=hello world",
+    "relative path" => "repositories/1379291646/new",
+    "non-string value" => [ "/repositories/1379291646/new" ]
+  }.each do |description, destination|
+    test "rejects a saved authentication destination with #{description}" do
+      github_app_configurations.create_installed
+      repository = repository_payload(2001, "github/prototype")
+      stub_github_sign_in(repositories: [ repository ], permissions: { 2001 => "pull" })
+
+      with_saved_authentication_destination(destination) { post github_authentication_path }
+      state = Rack::Utils.parse_query(URI(response.location).query).fetch("state")
+      get github_authentication_callback_path, params: { code: "code", state: }
+
+      assert_redirected_to welcome_path
+    end
+  end
+
+  test "a saved destination does not bypass sign-in admission without repository access" do
     github_app_configurations.create_installed
     repository = repository_payload(2001, "github/private")
     stub_github_sign_in(repositories: [ repository ], permissions: { 2001 => nil })
 
+    get repository_new_prototype_path(2001)
+    assert_redirected_to root_path
+    follow_redirect!
     post github_authentication_path
     state = Rack::Utils.parse_query(URI(response.location).query).fetch("state")
-    get github_authentication_callback_path, params: { code: "code", state: }
+    assert_no_difference "Session.count" do
+      get github_authentication_callback_path, params: { code: "code", state: }
+    end
 
     assert_redirected_to root_path
     follow_redirect!
@@ -204,4 +298,18 @@ class AuthenticationTest < ActionDispatch::IntegrationTest
     assert_redirected_to root_path
     assert_not Session.exists?(browser_session.id)
   end
+
+  private
+
+  # Seeds forged server-side state without adding a public return-to parameter or bypassing OAuth.
+  def with_saved_authentication_destination(destination)
+    original = GithubAuthenticationsController.instance_method(:create)
+    GithubAuthenticationsController.define_method(:create) do
+      session[:authentication_return_to] = destination
+      original.bind_call(self)
+    end
+    yield
+  ensure
+    GithubAuthenticationsController.define_method(:create, original) if original
+  end
 end

hub/test/integration/local_development_test.rb

@@ -22,6 +22,27 @@ class LocalDevelopmentTest < ActionDispatch::IntegrationTest
     assert_raises(LocalDevelopment::Error) { Repository::LocalDiscovery.refresh_all! }
   end
 
+  test "local sign-in returns to the requested Create page" do
+    path = create_repository("sign-in-destination")
+
+    in_local_mode do
+      Repository::LocalDiscovery.register_repositories!
+      repository = Repository.find_by!(local_path: path.to_s)
+      destination = repository_new_prototype_path(repository.route_id, template: "default")
+
+      get destination
+      assert_redirected_to root_path
+      follow_redirect! headers: { "Accept" => "application/json" }
+      assert_response :success
+      post local_authentication_path
+
+      assert_redirected_to destination
+      follow_redirect! headers: { "Accept" => "application/json" }
+      assert_response :success
+      assert_equal "new", response.parsed_body.fetch("data").fetch("selectedSection")
+    end
+  end
+
   test "a GitHub browser cookie cannot resume a local session" do
     github_app_configurations.create_installed
     payload = repository_payload(2001, "github/prototype")

hub/test/integration/session_revocations_test.rb

@@ -68,7 +68,7 @@ class SessionRevocationsTest < ActionDispatch::IntegrationTest
     get session_revocation_path
     assert_redirected_to root_path
     sign_in
-    assert_redirected_to repositories_path
+    assert_redirected_to session_revocation_path
     get session_revocation_path
     assert_response :success
     replacement = pair