diff --git a/hub/app/controllers/concerns/fresh_authorization_required.rb b/hub/app/controllers/concerns/fresh_authorization_required.rb index 789bcd42..8314c30b 100644 --- a/hub/app/controllers/concerns/fresh_authorization_required.rb +++ b/hub/app/controllers/concerns/fresh_authorization_required.rb @@ -1,7 +1,12 @@ -# Halts sensitive mutations without saving their payload or replaying them after OAuth. +# Resumes confirmed browser actions through their original permission and mutation paths. module FreshAuthorizationRequired extend ActiveSupport::Concern + included do + before_action :restore_authorized_action + rescue_from FreshAuthorization::Continuation::Invalid, with: :continuation_unavailable + end + private # Supplies the same explicit challenge contract to browser and canvas consumers. @@ -10,8 +15,9 @@ def require_fresh_authorization(purpose:, return_to: root_path) return true if FreshAuthorization.fresh?(credential) record_security_audit(outcome: "denied") if security_audit_action + pending_action = FreshAuthorization::Continuation.capture(request, params, purpose:) if !Current.canvas_grant challenge = FreshAuthorization.start! \ - credential:, purpose:, return_to: Current.canvas_grant ? root_path : return_to + credential:, purpose:, return_to: Current.canvas_grant ? root_path : return_to, pending_action: pending_action url = "#{Rails.configuration.x.hub_origin.delete_suffix('/')}#{fresh_authorization_path(challenge.public_id)}" if Current.canvas_grant render \ @@ -33,4 +39,22 @@ def require_fresh_authorization(purpose:, return_to: root_path) end false end + + # An opaque identifier selects server-saved fields, never replacement client input. + def restore_authorized_action + return if !params.key?(:authorization_continuation_id) + + raise FreshAuthorization::Continuation::Invalid if !params[:authorization_continuation_id].is_a?(String) + + challenge = FreshAuthorization.find_by(public_id: params[:authorization_continuation_id]) + raise FreshAuthorization::Continuation::Invalid if challenge.nil? + + @continued_action_parameters = challenge.continuation.claim!(request, browser_session: Current.session) + self.params = ActionController::Parameters.new(request.path_parameters).merge(@continued_action_parameters) + end + + def continuation_unavailable + record_security_audit(outcome: "denied") if security_audit_action + redirect_to root_path, alert: I18n.t("fresh_authorization.continuation_failed"), status: :see_other + end end diff --git a/hub/app/controllers/fresh_authorizations_controller.rb b/hub/app/controllers/fresh_authorizations_controller.rb index 77973655..b98a71ac 100644 --- a/hub/app/controllers/fresh_authorizations_controller.rb +++ b/hub/app/controllers/fresh_authorizations_controller.rb @@ -1,4 +1,4 @@ -# Presents an explicit identity check without ever executing the original mutation. +# Verifies identity, then returns confirmed actions to their CSRF-protected routes. class FreshAuthorizationsController < SuperglueController allow_unauthenticated_access before_action :resume_session @@ -11,7 +11,8 @@ class FreshAuthorizationsController < SuperglueController # @route GET /authorization/:id (fresh_authorization) def show - raise FreshAuthorization::Invalid if !@challenge.available_to?(Current.session) + @continuing = @challenge.continuation.available_to?(Current.session) + raise FreshAuthorization::Invalid if !@continuing && !@challenge.available_to?(Current.session) end # @route POST /authorization/:id (fresh_authorization) diff --git a/hub/app/controllers/github_app_credentials_controller.rb b/hub/app/controllers/github_app_credentials_controller.rb index 011724e7..d3c5eb31 100644 --- a/hub/app/controllers/github_app_credentials_controller.rb +++ b/hub/app/controllers/github_app_credentials_controller.rb @@ -1,4 +1,4 @@ -# Accepts new credentials once without exporting or replaying saved secrets. +# Accepts replacement credentials without exporting saved secrets. class GithubAppCredentialsController < SuperglueController include HasGithubAppAdministration @@ -14,15 +14,21 @@ def edit # @route PATCH /github_app/credentials (github_app_credentials) def update - upload = params[:private_key] - limit = GithubAppConfiguration::Administration::PRIVATE_KEY_LIMIT - if !upload.is_a?(ActionDispatch::Http::UploadedFile) || upload.size > limit - raise GithubAppConfiguration::Administration::InvalidCredentials, "Invalid replacement credentials" + private_key = if @continued_action_parameters + @continued_action_parameters.fetch("private_key") + else + upload = params[:private_key] + limit = GithubAppConfiguration::Administration::PRIVATE_KEY_LIMIT + if !upload.is_a?(ActionDispatch::Http::UploadedFile) || upload.size > limit + raise GithubAppConfiguration::Administration::InvalidCredentials, "Invalid replacement credentials" + end + + upload.read(limit + 1) end @configuration.administration.replace_credentials! \ actor: Current.user, browser_session: Current.session, expected_version: @expected_version, - client_secret: params[:client_secret], private_key: upload.read(limit + 1) + client_secret: params[:client_secret], private_key: private_key @security_audit_recorded = true redirect_to github_app_path, notice: t("github_app.credentials.saved"), status: :see_other end diff --git a/hub/app/controllers/github_authentications_controller.rb b/hub/app/controllers/github_authentications_controller.rb index 51ab7036..b42ac398 100644 --- a/hub/app/controllers/github_authentications_controller.rb +++ b/hub/app/controllers/github_authentications_controller.rb @@ -82,16 +82,22 @@ def fresh_authorization_callback SessionRevocation.with_generation!(generation) do GithubAppConfiguration.with_credentials!(**snapshot) { challenge.verify!(profile) } end - redirect_to challenge.return_to, notice: t("fresh_authorization.succeeded"), status: :see_other + if challenge.pending_action.present? + redirect_to fresh_authorization_path(challenge.public_id), status: :see_other + else + redirect_to challenge.return_to, notice: t("fresh_authorization.succeeded"), status: :see_other + end rescue FreshAuthorization::Invalid, GithubAppConfiguration::Oauth::InvalidState, SessionRevocation::Superseded, GithubAppConfiguration::Superseded, ActiveRecord::RecordNotFound if challenge + challenge.continuation.discard! challenge.audit!("denied") else SecurityAuditEvent.record!(action: "fresh_authorization.verify", outcome: "denied") end redirect_to root_path, alert: t("fresh_authorization.failure"), status: :see_other rescue GithubClient::Error + challenge&.continuation&.discard! challenge&.audit!("failed") redirect_to root_path, alert: t("fresh_authorization.failure"), status: :see_other ensure diff --git a/hub/app/javascript/pages/copilot-connection-page/stories.tsx b/hub/app/javascript/pages/copilot-connection-page/stories.tsx index 8eabd1c0..ee206c31 100644 --- a/hub/app/javascript/pages/copilot-connection-page/stories.tsx +++ b/hub/app/javascript/pages/copilot-connection-page/stories.tsx @@ -25,7 +25,7 @@ const shared = { } const verification = - "This change requires GitHub verification within the last five minutes on this browser. If asked to verify, return and confirm again. Verification never submits this change." + "This change requires GitHub verification within the last five minutes on this browser. If verification is required, Hub continues your confirmed change after it succeeds." const connect = { label: "Connect Copilot", diff --git a/hub/app/javascript/pages/fresh-authorization-page/index.test.tsx b/hub/app/javascript/pages/fresh-authorization-page/index.test.tsx index 6b0cfcdd..7e113646 100644 --- a/hub/app/javascript/pages/fresh-authorization-page/index.test.tsx +++ b/hub/app/javascript/pages/fresh-authorization-page/index.test.tsx @@ -7,7 +7,7 @@ import { hubAdministrationWorkspaceData } from "@/storybook/fixtures/hub-adminis import { FreshAuthorizationPage } from "." describe("FreshAuthorizationPage()", () => { - it("keeps an administrator check in Hub scope without replaying its action", () => { + it("keeps an administrator check in Hub scope", () => { render( { productName: "Replica Hub", pageTitle: "Fresh authorization", heading: "Fresh authorization", - description: "Return and confirm again.", + description: "Verify your GitHub account.", submitLabel: "Verify with GitHub", submitPath: "/authorization/example", cancelLabel: "Cancel", @@ -50,7 +50,7 @@ describe("FreshAuthorizationPage()", () => { productName: "Replica Hub", pageTitle: "Fresh authorization", heading: "Fresh authorization", - description: "Verify the same GitHub account. No action is replayed.", + description: "Verify the same GitHub account.", submitLabel: "Verify with GitHub", submitPath: "/authorization/example", cancelLabel: "Cancel", diff --git a/hub/app/javascript/pages/fresh-authorization-page/index.tsx b/hub/app/javascript/pages/fresh-authorization-page/index.tsx index d7ba6adc..67af2557 100644 --- a/hub/app/javascript/pages/fresh-authorization-page/index.tsx +++ b/hub/app/javascript/pages/fresh-authorization-page/index.tsx @@ -1,4 +1,5 @@ import { Button, Stack } from "@primer/react-brand" +import type { Ref } from "react" import { HubContentPageHeader } from "@components/hub-content-page-header" import { GithubAppSetupHeader } from "@features/github-app-setup-header" @@ -8,12 +9,27 @@ import { HubAdministrationWorkspace } from "@features/hub-administration-workspa import type { FreshAuthorizationPageData } from "./types" import "@features/hub-administration-page/styles.css" -/** Requires explicit authorization and retry instead of retaining a sensitive payload. */ -export function FreshAuthorizationPage({ data }: { data: FreshAuthorizationPageData }) { +export function FreshAuthorizationPage({ + data, + continuationFormRef, +}: { + data: FreshAuthorizationPageData + continuationFormRef?: Ref +}) { const actions = ( -
+ + {data.continuation && ( + <> + + + + )} diff --git a/hub/app/javascript/pages/fresh-authorization-page/stories.tsx b/hub/app/javascript/pages/fresh-authorization-page/stories.tsx index c63fc5f7..e9a33af4 100644 --- a/hub/app/javascript/pages/fresh-authorization-page/stories.tsx +++ b/hub/app/javascript/pages/fresh-authorization-page/stories.tsx @@ -14,7 +14,7 @@ const meta = { pageTitle: "Fresh GitHub authorization required", heading: "Fresh GitHub authorization required", description: - "Verify the same GitHub account before changing sensitive settings. GitHub may reuse your signed-in browser; this is not a guaranteed password or MFA challenge.", + "Verify the same GitHub account to continue your confirmed action. GitHub may reuse your signed-in browser; this is not a guaranteed password or MFA challenge.", submitLabel: "Verify with GitHub", submitPath: "/authorization/example", cancelLabel: "Cancel", @@ -34,14 +34,22 @@ export const HubAdministratorChange: Story = { ...meta.args.data, administrationWorkspace: hubAdministrationWorkspaceData, description: - "Verify the same GitHub account before changing a Hub administrator role. Return to the confirmation page and submit again. No role change is saved or replayed.", + "Verify the same GitHub account to continue the Hub administrator role change you confirmed.", }, }, } -export const Narrow: Story = { - render: (args) => ( -
- -
- ), +export const Continuing: Story = { + args: { + data: { + ...meta.args.data, + administrationWorkspace: hubAdministrationWorkspaceData, + pageTitle: "Completing your action", + heading: "Completing your action", + description: + "GitHub verified your account. Hub is continuing the action you confirmed. If it does not continue automatically, choose Continue.", + submitLabel: "Continue", + submitPath: "/hub_admins", + continuation: { id: "example", method: "post" }, + }, + }, } diff --git a/hub/app/javascript/pages/fresh-authorization-page/types.ts b/hub/app/javascript/pages/fresh-authorization-page/types.ts index 375e8105..11324243 100644 --- a/hub/app/javascript/pages/fresh-authorization-page/types.ts +++ b/hub/app/javascript/pages/fresh-authorization-page/types.ts @@ -9,4 +9,5 @@ export interface FreshAuthorizationPageData extends GithubAppSetupHeaderData { authenticityToken: string cancelLabel?: string cancelPath?: string + continuation?: { id: string; method: string } } diff --git a/hub/app/javascript/pages/fresh-authorization-page/view.test.tsx b/hub/app/javascript/pages/fresh-authorization-page/view.test.tsx new file mode 100644 index 00000000..49b39f80 --- /dev/null +++ b/hub/app/javascript/pages/fresh-authorization-page/view.test.tsx @@ -0,0 +1,44 @@ +import { ThemeProvider } from "@primer/react-brand" +import { render } from "@testing-library/react" +import { StrictMode } from "react" +import { describe, expect, it, vi } from "vitest" + +import { usePageData } from "@hooks/use-page-data" + +import FreshAuthorizationsShowView from "../../../views/fresh_authorizations/show" + +vi.mock("@hooks/use-page-data", () => ({ usePageData: vi.fn() })) +vi.mock("@hooks/use-workspace-inbox", () => ({ + useWorkspaceInbox: (data: unknown) => data, +})) + +describe("FreshAuthorizationsShowView()", () => { + it("automatically submits the continuation once with its protected form fields", () => { + vi.mocked(usePageData).mockReturnValue({ + productName: "Replica Hub", + pageTitle: "Continue action", + heading: "GitHub account verified", + description: "Continuing your action.", + submitLabel: "Continue", + submitPath: "/github_app/credentials", + authenticityToken: "csrf-token", + continuation: { id: "opaque-authorization-id", method: "patch" }, + }) + const submit = vi.spyOn(HTMLFormElement.prototype, "requestSubmit").mockImplementation(() => {}) + render( + + + + + , + ) + expect(submit).toHaveBeenCalledTimes(1) + const form = submit.mock.instances[0] as HTMLFormElement + expect(form).toHaveAttribute("action", "/github_app/credentials") + expect(Object.fromEntries(new FormData(form).entries())).toEqual({ + authenticity_token: "csrf-token", + authorization_continuation_id: "opaque-authorization-id", + _method: "patch", + }) + }) +}) diff --git a/hub/app/javascript/pages/github-app-page/stories.tsx b/hub/app/javascript/pages/github-app-page/stories.tsx index 25743ee4..fdd541f0 100644 --- a/hub/app/javascript/pages/github-app-page/stories.tsx +++ b/hub/app/javascript/pages/github-app-page/stories.tsx @@ -19,7 +19,8 @@ const data: GithubAppPageData = { identity: "App 123 - github (Organization, identity 321)", installation: "Installation 456: active", busyLabel: null, - verification: "Verify this browser with GitHub, then confirm again. Nothing is replayed.", + verification: + "GitHub must have verified this browser's account within the last five minutes. If verification is required, Hub temporarily encrypts your submitted settings and continues your confirmed change after it succeeds.", paragraphs: ["Only the current webhook secret is accepted."], links: [ { label: "Replace App credentials", path: "/github_app/credentials" }, diff --git a/hub/app/javascript/pages/hub-admin-confirmation-page/stories.tsx b/hub/app/javascript/pages/hub-admin-confirmation-page/stories.tsx index 1f486b05..922ac53b 100644 --- a/hub/app/javascript/pages/hub-admin-confirmation-page/stories.tsx +++ b/hub/app/javascript/pages/hub-admin-confirmation-page/stories.tsx @@ -19,7 +19,7 @@ const data: HubAdminConfirmationPageData = { cancelLabel: "Cancel", cancelPath: "/hub_admins", verification: - "A role change requires GitHub to have verified this browser's account within the last five minutes. If verification is required, return here and confirm again. Hub never replays the change.", + "A role change requires GitHub to have verified this browser's account within the last five minutes. If verification is required, Hub continues your confirmed change after it succeeds.", blockedLabel: null, } @@ -44,15 +44,6 @@ export default meta type Story = StoryObj export const Designate: Story = {} -export const FreshVerificationComplete: Story = { - args: { - data: { - ...data, - notice: - "GitHub authorization succeeded for this connection only. Return to the action and submit it again within five minutes.", - }, - }, -} export const Remove: Story = { args: { data: removalData }, } diff --git a/hub/app/javascript/pages/session-revocation-page/stories.tsx b/hub/app/javascript/pages/session-revocation-page/stories.tsx index d432fd05..754392e4 100644 --- a/hub/app/javascript/pages/session-revocation-page/stories.tsx +++ b/hub/app/javascript/pages/session-revocation-page/stories.tsx @@ -27,7 +27,7 @@ export const sessionRevocationData: SessionRevocationPageData = { ownSession: "Your own session (@octocat) will end too. This browser will show the result without requiring another sign-in.", verification: - "GitHub must have verified this browser's account within the last five minutes. If verification is required, return here and confirm again. Hub never replays this action.", + "GitHub must have verified this browser's account within the last five minutes. If verification is required, Hub ends the sessions after it succeeds.", counts: "3 browser sessions, 2 canvas connections, and 1 pending approval. Counts can change before confirmation.", emptyLabel: null, @@ -60,6 +60,7 @@ const result: SessionRevocationPageData = { const meta = { title: "Hub/Pages/Security/End all sessions", component: SessionRevocationPage, + excludeStories: ["sessionRevocationData"], parameters: { layout: "fullscreen" }, args: { data: sessionRevocationData }, } satisfies Meta @@ -78,14 +79,6 @@ export const OnlyYourSession: Story = { }, }, } -export const Challenged: Story = { - args: { - data: { - ...sessionRevocationData, - alert: "Verify with GitHub, then return here and confirm again. No sessions were ended.", - }, - }, -} export const Denied: Story = { args: { data: { diff --git a/hub/app/javascript/pages/sharing-settings-page/stories.tsx b/hub/app/javascript/pages/sharing-settings-page/stories.tsx index 49be32b7..9df3977c 100644 --- a/hub/app/javascript/pages/sharing-settings-page/stories.tsx +++ b/hub/app/javascript/pages/sharing-settings-page/stories.tsx @@ -27,7 +27,7 @@ const data: SharingSettingsPageData = { "Anyone with a new share link can view its fixed Build without signing in. Previously revoked links stay revoked.", confirmLabel: "I understand and confirm this deployment-wide change.", verification: - "This change requires GitHub verification within the last five minutes on this browser. If asked to verify, return and confirm again. Verification never submits this change.", + "This change requires GitHub verification within the last five minutes on this browser. If verification is required, Hub continues your confirmed change after it succeeds.", } const meta = { diff --git a/hub/app/models/fresh_authorization.rb b/hub/app/models/fresh_authorization.rb index b918be78..e68a4c7c 100644 --- a/hub/app/models/fresh_authorization.rb +++ b/hub/app/models/fresh_authorization.rb @@ -5,9 +5,11 @@ # # id :integer not null, primary key # consumed_at :datetime +# continued_at :datetime # credential_type :string not null # expires_at :datetime not null # oauth_state_digest :string +# pending_action :text # purpose :string not null # return_to :string not null # verified_at :datetime @@ -34,6 +36,7 @@ # # Check Constraints # +# fresh_authorizations_continuation (continued_at IS NULL OR (verified_at IS NOT NULL AND pending_action IS NULL)) # fresh_authorizations_identity (credential_type IN ('Session', 'CanvasGrant') AND credential_id > 0 AND github_id > 0) # fresh_authorizations_purpose (purpose IN ('never_expiring_share', 'analytics_destination', 'hub_admin_management', 'sharing_settings', 'session_revocation', 'github_app_reset', 'github_app_credentials', 'github_app_webhook', 'github_app_finish_webhook', 'copilot_connection', 'copilot_disconnection')) # fresh_authorizations_verification (verified_at IS NULL OR consumed_at IS NOT NULL) @@ -56,6 +59,9 @@ class Invalid < StandardError; end belongs_to :user belongs_to :repository, optional: true + has_object :continuation + encrypts :pending_action + serialize :pending_action, coder: JSON attr_readonly :public_id, :user_id, :github_id, :credential_type, :credential_id, :purpose, :return_to, :expires_at, :repository_id validates :public_id, :github_id, :credential_id, :expires_at, presence: true @@ -74,7 +80,7 @@ def self.fresh?(credential) end # Requires an existing Session or active access grant before creating a challenge. - def self.start!(credential:, purpose:, return_to: "/") + def self.start!(credential:, purpose:, return_to: "/", pending_action: nil) transaction do live = live_credential(credential) raise Invalid, "The initiating credential is no longer active." if live.nil? || live.user.local? @@ -86,7 +92,7 @@ def self.start!(credential:, purpose:, return_to: "/") public_id: SecureRandom.uuid, user: live.user, github_id: live.user.github_id, repository: live.is_a?(CanvasGrant) ? live.repository : nil, credential_type: live.class.name, credential_id: live.id, purpose:, return_to:, - expires_at: LIFETIME.from_now + expires_at: LIFETIME.from_now, pending_action: pending_action challenge.audit!("succeeded", action: "fresh_authorization.request") challenge end @@ -163,9 +169,10 @@ def verify!(profile) # Ends a cancelled request without renewing any credential. def cancel!(browser_session:) with_lock do - raise Invalid, "This authorization request is no longer available." if !available_to?(browser_session) + raise Invalid, "This authorization request is no longer available." if + !available_to?(browser_session) && !continuation.available_to?(browser_session) - update!(consumed_at: Time.current) + update!(consumed_at: consumed_at || Time.current, pending_action: nil) audit!("denied") end end diff --git a/hub/app/models/fresh_authorization/continuation.rb b/hub/app/models/fresh_authorization/continuation.rb new file mode 100644 index 00000000..655168fa --- /dev/null +++ b/hub/app/models/fresh_authorization/continuation.rb @@ -0,0 +1,104 @@ +# Retains only known browser operations, never an arbitrary request or OAuth credential. +class FreshAuthorization::Continuation < ActiveRecord::AssociatedObject + LIMIT = 128.kilobytes + OPERATIONS = { + "hub_admins" => { "create" => "hub_admin_management", "update" => "hub_admin_management" }, + "analytics_destinations" => { + "create" => "analytics_destination", "update" => "analytics_destination", + "destroy" => "analytics_destination" + }, + "sharing_settings" => { "update" => "sharing_settings" }, + "session_revocations" => { "create" => "session_revocation" }, + "github_apps" => { "destroy" => "github_app_reset" }, + "github_app_credentials" => { "update" => "github_app_credentials" }, + "github_app_webhook_rotations" => { + "create" => "github_app_webhook", "destroy" => "github_app_finish_webhook" + }, + "copilot_connections" => { + "create" => "copilot_connection", "destroy" => "copilot_disconnection" + } + }.freeze + + class Invalid < StandardError; end + + # Saves a bounded selection of fields; files become encrypted contents, not paths. + def self.capture(request, params, purpose:) + controller, action = request.path_parameters.values_at(:controller, :action) + return if purpose.to_s == "never_expiring_share" + + raise Invalid if OPERATIONS.dig(controller, action) != purpose.to_s || + !request.path.match?(%r{\A/(?!/)[^\x00-\x20\\]*\z}) + + fields = case controller + when "hub_admins" then [ :confirmed, :operation, { user_ids: [] } ] + when "analytics_destinations" + [ :operation, { destination: [ + :provider, :region, :history, :name, :endpoint, :template, :headers, + :success_pointer, :success_value, :keep_pending, + { event_types: [], credentials: %i[write_key service_account_username service_account_secret project_id], + secrets: %i[name value] } + ] } ] + when "github_app_credentials" then %i[confirmed configuration_id expected_version client_secret] + when "github_apps" then %i[confirmed configuration_id expected_version confirmation] + when "github_app_webhook_rotations" then %i[confirmed configuration_id expected_version operation] + else %i[confirmed operation] + end + attributes = params.permit(*fields).to_h + if controller == "github_app_credentials" + upload = params[:private_key] + limit = GithubAppConfiguration::Administration::PRIVATE_KEY_LIMIT + raise Invalid if !upload.is_a?(ActionDispatch::Http::UploadedFile) || upload.size > limit + + contents = upload.read(limit + 1) + upload.rewind + raise Invalid if contents.bytesize > limit || !contents.dup.force_encoding(Encoding::UTF_8).valid_encoding? + + attributes["private_key"] = contents.force_encoding(Encoding::UTF_8) + end + payload = { + "controller" => controller, "action" => action, "path" => request.path, + "method" => request.request_method.downcase, "parameters" => attributes + } + raise Invalid if payload.to_json.bytesize > LIMIT + + payload + end + + # Only the original live browser can continue a verified, unexpired operation. + def available_to?(browser_session) + record.pending_action.present? && record.verified_at.present? && record.continued_at.nil? && + record.expires_at > Time.current && record.credential_type == "Session" && + browser_session&.id == record.credential_id && + browser_session.user.github_id == record.github_id && FreshAuthorization.fresh?(browser_session) + end + + # Claims one attempt before returning fields to the normal controller and its guards. + def claim!(request, browser_session:) + attributes = nil + record.with_lock do + payload = record.pending_action + controller, action = request.path_parameters.values_at(:controller, :action) + raise Invalid if record.credential_type != "Session" || browser_session&.id != record.credential_id + + if !available_to?(browser_session) + record.update!(pending_action: nil) + next + end + raise Invalid if + OPERATIONS.dig(controller, action) != record.purpose || + payload.values_at("controller", "action", "path", "method") != + [ controller, action, request.path, request.request_method.downcase ] + + record.update!(continued_at: Time.current, pending_action: nil) + attributes = payload.fetch("parameters") + end + raise Invalid if attributes.nil? + + attributes + end + + # Drops secrets as soon as verification fails or the person cancels. + def discard! + record.update!(pending_action: nil) if record.persisted? + end +end diff --git a/hub/app/views/fresh_authorizations/show.json.props b/hub/app/views/fresh_authorizations/show.json.props index 2556b012..4e53caed 100644 --- a/hub/app/views/fresh_authorizations/show.json.props +++ b/hub/app/views/fresh_authorizations/show.json.props @@ -1,12 +1,18 @@ json.productName t("application.name") -json.pageTitle t("fresh_authorization.heading") -json.heading t("fresh_authorization.heading") -json.description t("fresh_authorization.description") -json.submitLabel t("fresh_authorization.submit") +json.pageTitle t(@continuing ? "fresh_authorization.continuing_heading" : "fresh_authorization.heading") +json.heading t(@continuing ? "fresh_authorization.continuing_heading" : "fresh_authorization.heading") +json.description t(@continuing ? "fresh_authorization.continuing_description" : "fresh_authorization.description") +json.submitLabel t(@continuing ? "fresh_authorization.continue" : "fresh_authorization.submit") json.cancelLabel t("fresh_authorization.cancel") -json.submitPath fresh_authorization_path(@challenge.public_id) +json.submitPath(@continuing ? @challenge.pending_action.fetch("path") : fresh_authorization_path(@challenge.public_id)) json.cancelPath fresh_authorization_path(@challenge.public_id) json.authenticityToken form_authenticity_token +if @continuing + json.continuation do + json.id @challenge.public_id + json.set! :method, @challenge.pending_action.fetch("method") + end +end if FreshAuthorization::ADMIN_PURPOSES.include?(@challenge.purpose) section = case @challenge.purpose when "hub_admin_management" then "admins" diff --git a/hub/app/views/fresh_authorizations/show.tsx b/hub/app/views/fresh_authorizations/show.tsx index 90a7f642..e9486bd6 100644 --- a/hub/app/views/fresh_authorizations/show.tsx +++ b/hub/app/views/fresh_authorizations/show.tsx @@ -1,10 +1,21 @@ +import { useEffect, useRef } from "react" + import { usePageData } from "@hooks/use-page-data" import { useWorkspaceInbox } from "@hooks/use-workspace-inbox" import { FreshAuthorizationPage } from "@pages/fresh-authorization-page" import type { FreshAuthorizationPageData } from "@pages/fresh-authorization-page/types" export default function FreshAuthorizationsShowView() { - return ( - ())} /> - ) + const data = useWorkspaceInbox(usePageData()) + const form = useRef(null) + const submitted = useRef(false) + + useEffect(() => { + if (!data.continuation || submitted.current || !form.current) return + + submitted.current = true + form.current.requestSubmit() + }, [data.continuation]) + + return } diff --git a/hub/config/initializers/filter_parameter_logging.rb b/hub/config/initializers/filter_parameter_logging.rb index cda8856e..abe1a0f7 100644 --- a/hub/config/initializers/filter_parameter_logging.rb +++ b/hub/config/initializers/filter_parameter_logging.rb @@ -6,5 +6,5 @@ Rails.application.config.filter_parameters += [ :passw, :email, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn, :cvv, :cvc, :code, :pem, :state, :authorization, :signature, :grant, :credentials, :secrets, :proof, - :service_account_username, :headers, :endpoint, :template + :service_account_username, :headers, :endpoint, :template, :pending_action ] diff --git a/hub/config/locales/copilot_connection.en.yml b/hub/config/locales/copilot_connection.en.yml index e301196f..7b2714b6 100644 --- a/hub/config/locales/copilot_connection.en.yml +++ b/hub/config/locales/copilot_connection.en.yml @@ -13,7 +13,7 @@ en: refused: "Copilot refused the token on %{date}, so Hub stopped looking for learnings. Replace REPLICA_COPILOT_TOKEN and restart Hub, or check the token again." no_token: Hub has no Copilot token. The person who runs Hub sets REPLICA_COPILOT_TOKEN to a fine-grained personal access token whose only permission is Copilot requests, then restarts Hub. empty: Learnings stays off until a Hub administrator connects Copilot. - verification: This change requires GitHub verification within the last five minutes on this browser. If asked to verify, return and confirm again. Verification never submits this change. + verification: This change requires GitHub verification within the last five minutes on this browser. If verification is required, Hub continues your confirmed change after it succeeds. connect: label: Connect Copilot consequence: Hub will ask Copilot, with the configured token, to read new review comments from every repository on this Hub and suggest learnings, including while you are signed out. Copilot never sees who wrote a comment. The usage counts toward the Copilot plan of the token's owner. Hub checks the token with Copilot first, and never stores or shows it. diff --git a/hub/config/locales/fresh_authorization.en.yml b/hub/config/locales/fresh_authorization.en.yml index 466f2246..e7a49a4c 100644 --- a/hub/config/locales/fresh_authorization.en.yml +++ b/hub/config/locales/fresh_authorization.en.yml @@ -1,10 +1,14 @@ en: fresh_authorization: heading: Fresh GitHub authorization required - description: Verify the same GitHub account before changing sensitive settings. GitHub may reuse your signed-in browser; this is not a guaranteed password or MFA challenge. + description: Verify the same GitHub account to continue your confirmed action. GitHub may reuse your signed-in browser; this is not a guaranteed password or MFA challenge. required: Fresh GitHub authorization is required. No changes were made. Verify your account, then submit the action again. submit: Verify with GitHub cancel: Cancel cancelled: Authorization cancelled. No sensitive changes were made. failure: GitHub authorization failed, expired, or used a different account. No sensitive changes were made. Return to the original action to try again. succeeded: GitHub authorization succeeded for this connection only. Return to the action and submit it again within five minutes. + continuing_heading: Completing your action + continuing_description: GitHub verified your account. Hub is continuing the action you confirmed. If it does not continue automatically, choose Continue. + continue: Continue + continuation_failed: Hub could not continue this action. It may have expired or already been attempted. Review its current state before trying again. diff --git a/hub/config/locales/github_app.en.yml b/hub/config/locales/github_app.en.yml index 6cca5890..6323d0c1 100644 --- a/hub/config/locales/github_app.en.yml +++ b/hub/config/locales/github_app.en.yml @@ -6,7 +6,7 @@ en: admins: Hub administrators audit: Security audit cancel: Cancel - verification: GitHub must have verified this browser's account within the last five minutes. If asked to verify, return here and confirm again. Submitted credentials are discarded, never replayed. + verification: GitHub must have verified this browser's account within the last five minutes. If verification is required, Hub temporarily encrypts your submitted settings and continues your confirmed change after it succeeds. identity: "App %{app_id} - %{owner} (%{owner_type}, identity %{owner_id})" installation: "Installation %{installation_id}: %{state}" busy: A webhook update is in progress. Wait for it to finish before changing the App. diff --git a/hub/config/locales/hub_admins.en.yml b/hub/config/locales/hub_admins.en.yml index 973fc570..ad0090ca 100644 --- a/hub/config/locales/hub_admins.en.yml +++ b/hub/config/locales/hub_admins.en.yml @@ -32,7 +32,7 @@ en: changed: Hub administrator role changed. cancel: Cancel confirm: I confirm this Hub administrator role change. - verification: A role change requires GitHub to have verified this browser's account within the last five minutes. If verification is required, return here and confirm again. Hub never replays the change. + verification: A role change requires GitHub to have verified this browser's account within the last five minutes. If verification is required, Hub continues your confirmed change after it succeeds. identity: "GitHub identity %{github_id} - Hub user %{id}" confirmation: designate: diff --git a/hub/config/locales/session_revocation.en.yml b/hub/config/locales/session_revocation.en.yml index 33b556ef..bafeb4f1 100644 --- a/hub/config/locales/session_revocation.en.yml +++ b/hub/config/locales/session_revocation.en.yml @@ -4,7 +4,7 @@ en: description: Sign everyone out of this Hub and disconnect existing Copilot canvas connections. People can sign in and approve new connections afterward. scope: Existing browser sessions, canvas access and refresh credentials, pending canvas approvals, preview handoffs, and signed-in preview access will end. Public share links and public previews will not change. own_session: "Your own session (@%{login}) will end too. This browser will show the result without requiring another sign-in." - verification: GitHub must have verified this browser's account within the last five minutes. If verification is required, return here and confirm again. Hub never replays this action. + verification: GitHub must have verified this browser's account within the last five minutes. If verification is required, Hub ends the sessions after it succeeds. counts: "%{browsers} browser sessions, %{canvases} canvas connections, and %{pairings} pending approvals. Counts can change before confirmation." empty: No other browser sessions, canvas connections, or pending approvals are active. Your own session will still end. confirm: I understand this will end everyone's sessions, including my own. diff --git a/hub/config/locales/sharing_settings.en.yml b/hub/config/locales/sharing_settings.en.yml index 09924235..c4d7f454 100644 --- a/hub/config/locales/sharing_settings.en.yml +++ b/hub/config/locales/sharing_settings.en.yml @@ -12,7 +12,7 @@ en: label: Disable public sharing consequence: All existing share links will be permanently revoked, including open previews and pending handoffs. Enabling later permits new links only. confirm: I understand and confirm this deployment-wide change. - verification: This change requires GitHub verification within the last five minutes on this browser. If asked to verify, return and confirm again. Verification never submits this change. + verification: This change requires GitHub verification within the last five minutes on this browser. If verification is required, Hub continues your confirmed change after it succeeds. changed: Public sharing settings changed. no_access: Only Hub administrators can view or change deployment sharing settings. errors: diff --git a/hub/db/migrate/20261022160000_add_pending_actions_to_fresh_authorizations.rb b/hub/db/migrate/20261022160000_add_pending_actions_to_fresh_authorizations.rb new file mode 100644 index 00000000..cf9b63e5 --- /dev/null +++ b/hub/db/migrate/20261022160000_add_pending_actions_to_fresh_authorizations.rb @@ -0,0 +1,9 @@ +class AddPendingActionsToFreshAuthorizations < ActiveRecord::Migration[8.1] + def change + add_column :fresh_authorizations, :pending_action, :text + add_column :fresh_authorizations, :continued_at, :datetime + add_check_constraint :fresh_authorizations, + "continued_at IS NULL OR (verified_at IS NOT NULL AND pending_action IS NULL)", + name: "fresh_authorizations_continuation" + end +end diff --git a/hub/db/schema.rb b/hub/db/schema.rb index f73e261d..9ad82652 100644 --- a/hub/db/schema.rb +++ b/hub/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_10_19_140000) do +ActiveRecord::Schema[8.1].define(version: 2026_10_22_160000) do create_table "analytics_deliveries", force: :cascade do |t| t.integer "attempts", default: 0, null: false t.datetime "created_at", null: false @@ -233,11 +233,14 @@ t.datetime "verified_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.text "pending_action" + t.datetime "continued_at" t.index ["credential_type", "credential_id"], name: "idx_on_credential_type_credential_id_835b090260" t.index ["expires_at"], name: "index_fresh_authorizations_on_expires_at" t.index ["public_id"], name: "index_fresh_authorizations_on_public_id", unique: true t.index ["repository_id"], name: "index_fresh_authorizations_on_repository_id" t.index ["user_id"], name: "index_fresh_authorizations_on_user_id" + t.check_constraint "continued_at IS NULL OR (verified_at IS NOT NULL AND pending_action IS NULL)", name: "fresh_authorizations_continuation" t.check_constraint "credential_type IN ('Session', 'CanvasGrant') AND credential_id > 0 AND github_id > 0", name: "fresh_authorizations_identity" t.check_constraint "purpose IN ('never_expiring_share', 'analytics_destination', 'hub_admin_management', 'sharing_settings', 'session_revocation', 'github_app_reset', 'github_app_credentials', 'github_app_webhook', 'github_app_finish_webhook', 'copilot_connection', 'copilot_disconnection')", name: "fresh_authorizations_purpose" t.check_constraint "verified_at IS NULL OR consumed_at IS NOT NULL", name: "fresh_authorizations_verification" diff --git a/hub/docs/analytics.md b/hub/docs/analytics.md index dfdb2408..6c284408 100644 --- a/hub/docs/analytics.md +++ b/hub/docs/analytics.md @@ -8,7 +8,7 @@ Security decisions, including destination changes, use the separate [security au Open **Analytics** above the account avatar, or visit `/repositories/:id/analytics`. Hub checks current GitHub admin permission on reads and every change. If GitHub cannot confirm access, Hub denies it. There is no Hub-wide analytics administrator. -Every GitHub-backed destination mutation also requires [fresh GitHub authorization](fresh-authorization.md) on the same browser Session. When verification is needed, Hub makes no change and does not retain or replay submitted credentials. Verify the same account, then enter the settings and submit again within five minutes. +Every GitHub-backed destination mutation also requires [fresh GitHub authorization](fresh-authorization.md) on the same browser Session. When verification is needed, Hub temporarily encrypts the submitted settings and credentials without changing the destination. Verify the same account and Hub continues that specific action through its normal permission checks. Failed or cancelled verification discards the saved settings. The isolated local-development demo keeps its existing analytics editing without pretending to verify GitHub identity. This exception applies only to the local developer acting on a configured local repository with local mode enabled. Local mode requires development, an explicit flag, and a localhost Hub origin; production rejects it. No other sensitive operation receives this exception. diff --git a/hub/docs/copilot.md b/hub/docs/copilot.md index 4e574af7..1aac59de 100644 --- a/hub/docs/copilot.md +++ b/hub/docs/copilot.md @@ -25,7 +25,7 @@ Hub reads one environment variable, so it needs no code for any secret store. Hu ## Connect Copilot 1. Open **Copilot** in the sidebar's lower group, or go to `/copilot`. Without a token, the page says how to give Hub one. -2. Read what Hub will do with Copilot, check the confirmation, and choose **Connect Copilot**. Like Hub's other administrator settings, this needs a [fresh GitHub authorization](fresh-authorization.md) on the same browser Session within the last five minutes. If GitHub asks you to verify, return and confirm again; verification never submits the change. +2. Read what Hub will do with Copilot, check the confirmation, and choose **Connect Copilot**. Like Hub's other administrator settings, this needs a [fresh GitHub authorization](fresh-authorization.md) on the same browser Session within the last five minutes. If GitHub asks you to verify, Hub continues the confirmed connection change after verification succeeds. 3. Hub checks the token with a Copilot request that lists the models it may use. If Copilot answers, Hub connects Copilot and records who connected it. If another administrator disconnects or replaces the connection while Hub checks the token, Hub cancels the earlier check and leaves that change intact. diff --git a/hub/docs/fresh-authorization.md b/hub/docs/fresh-authorization.md index 76450323..92b4b483 100644 --- a/hub/docs/fresh-authorization.md +++ b/hub/docs/fresh-authorization.md @@ -6,7 +6,7 @@ Adding, editing, pausing, resuming, or deleting an analytics destination require Only the isolated local-development analytics demo differs: its local developer may edit configured local repositories without a GitHub proof. The exception lives in that analytics controller, not the reusable gate. Local mode requires development, an explicit opt-in flag, and a localhost Hub origin; production rejects it. Demo editing never creates freshness or exempts unrelated sensitive operations. -For example, adding an analytics destination after that window opens **Verify with GitHub** instead of saving it. Complete the check, return to analytics, and submit again. Hub does not keep or replay the original mutation. Analytics credentials must be entered again; they never enter OAuth state, challenge records, or a callback URL. +For example, adding an analytics destination after that window opens **Verify with GitHub** before saving it. Complete the check and Hub continues the action you already submitted, without asking you to enter the settings again. Hub temporarily encrypts the selected operation and its submitted settings, including credentials, in the challenge record. Settings never enter OAuth state, callback URLs, browser continuation forms, or security logs. ## Scope and proof @@ -16,11 +16,13 @@ GitHub may reuse its signed-in browser session. **Fresh authorization is not a g The window is strictly less than five minutes: at exactly five minutes, Hub requires another check. The challenge expires after 15 minutes. Hub binds it to a server-selected purpose, initiating credential, immutable GitHub identity, one-use state, and expiry. It extends the existing state/PKCE flow and registered `/auth/github/callback`; existing deployments need no App reset or callback change. -The callback rechecks that the Session exists or the grant still has active access. It checks challenge expiry again under the initiating credential's lock and records one verification time on both records. Removed, revoked, expired, mismatched, cancelled, or failed challenges cannot renew a credential. Success updates only the initiating credential and redirects to a safe server-selected page. That page confirms success and asks the person to submit the action again within five minutes. It never executes the sensitive action. +The callback rechecks that the Session exists or the grant still has active access. It checks challenge expiry again under the initiating credential's lock and records one verification time on both records. Removed, revoked, expired, mismatched, cancelled, or failed challenges cannot renew a credential. Success updates only the initiating credential. For a saved browser action, Hub then opens a continuation page that automatically submits an opaque, one-use reference to the original CSRF-protected route. A **Continue** button is available if automatic submission does not run. The original controller rechecks permissions, confirmation, target state and freshness before changing anything and reports the actual result. + +Continuation covers administrator role changes, analytics destination changes, deployment sharing settings, ending all sessions, GitHub App reset and credential replacement, webhook rotation and Copilot connection changes. Hub saves only explicitly allowed fields for these operations, not arbitrary requests. Each action is bound to its original browser Session, GitHub identity, purpose, route and HTTP method. Other input cannot replace the saved selection. The continuation expires with the 15-minute challenge and still needs fresh proof when submitted. An atomic claim removes the saved fields before one execution attempt; refreshes, repeated callbacks and duplicate submissions cannot execute it again. A failed or interrupted attempt requires checking the current state before explicitly trying again. ## Browser and canvas flow -Browser mutations without fresh identity redirect to the explicit challenge page. Both Verify and Cancel use CSRF-protected forms. Failure reports that no sensitive changes were made. +Browser mutations without fresh identity redirect to the explicit challenge page. Verify, Cancel and continuation use CSRF-protected forms. Cancelling or failing verification discards the saved action without executing it. Expired encrypted payloads are removed by hourly challenge cleanup. Submitted payloads are limited to 128 KiB; private keys retain their 16 KiB limit. Unsupported or oversized input reports a visible failure rather than silently dropping the action. Reading or cancelling an unrelated challenge does not interrupt a pending GitHub check. Starting another check replaces the browser's state only after Hub binds the new challenge. An invalid operator-enrollment secret also leaves the pending check intact. @@ -47,7 +49,7 @@ Controllers include `FreshAuthorizationRequired` and call `require_fresh_authori OAuth checks bind independently to the deployment session generation and the initiating App configuration and credential revision. Replacement credentials invalidate old in-flight checks and stored freshness without revoking live Sessions/connections or changing the session generation. A callback checks both snapshots before exchange and again before Session issuance, enrollment, or proof renewal. Reset removes the configuration and advances the session cutoff, so an old callback cannot restore credentials. -Challenge requests and successful, denied, or failed start, verification, and cancellation decisions use the separate security audit with the initiating actor and credential. Validation-only failures when reading unknown, expired, or foreign challenges do not create persistent security records; ordinary request logs and visible failure messages remain. Sensitive actions also retain their existing audit records. Challenges retain no credentials or raw mutation payloads and expire through hourly cleanup. They never enter analytics. +Challenge requests and successful, denied, or failed start, verification, and cancellation decisions use the separate security audit with the initiating actor and credential. Validation-only failures when reading unknown, expired, or foreign challenges do not create persistent security records; ordinary request logs and visible failure messages remain. Continued sensitive actions retain their existing audit records and rate limits. Challenges retain only the encrypted, allowed fields needed for continuation and expire through hourly cleanup. They never enter analytics. ## Setup and recovery commands diff --git a/hub/docs/github-app-setup.md b/hub/docs/github-app-setup.md index d69e0c72..f332278a 100644 --- a/hub/docs/github-app-setup.md +++ b/hub/docs/github-app-setup.md @@ -78,11 +78,11 @@ Production usually receives GitHub notifications for pushes, workflows, and inst ## Routine App administration -An existing Hub administrator opens **GitHub App** from the deployment security audit, or `/github_app`. For example, choose **Replace App credentials**, supply replacements from the same App's GitHub settings, confirm, and save. Every App mutation requires current persisted administrator permission, deliberate confirmation, and GitHub verification of the same browser Session within the last five minutes. If verification is needed, return and submit again; Hub never replays the operation. The role does not grant repository access. +An existing Hub administrator opens **GitHub App** from the deployment security audit, or `/github_app`. For example, choose **Replace App credentials**, supply replacements from the same App's GitHub settings, confirm, and save. Every App mutation requires current persisted administrator permission, deliberate confirmation, and GitHub verification of the same browser Session within the last five minutes. If verification is needed, Hub continues the confirmed operation after it succeeds, rechecking permissions and the App's saved version. The role does not grant repository access. An unchanged installation poll refreshes its checked time without invalidating an open confirmation. A change to installation, credentials, or webhook state still requires a new confirmation. -Hub encrypts the App's client secret, private key, and webhook secrets in the database. Credential fields start empty. Enter a new client secret (at most 4 KiB) and upload a PEM private key (at most 16 KiB) over HTTPS. Hub never prefills, exports, logs, or sends saved secrets to browsers. A challenge or validation error discards submitted credentials; enter them again. +Hub encrypts the App's client secret, private key, and webhook secrets in the database. Credential fields start empty. Enter a new client secret (at most 4 KiB) and upload a PEM private key (at most 16 KiB) over HTTPS. Hub never prefills, exports, logs, or sends saved secrets to browsers. During verification, submitted replacements are temporarily encrypted in the challenge record and removed before the continuation attempt. Continuation passes the saved key bytes directly to credential validation in memory without creating another upload file. A validation error requires entering replacements again. The candidate private key must authenticate as the saved App ID and owner ID/type. This check **does not verify the OAuth client secret**. Hub reports credentials saved, not sign-in repaired. Confirm a successful GitHub sign-in before retiring the old credentials at GitHub. Replacement invalidates pending OAuth/enrollment checks and fresh proof on Sessions and canvas grants, but retains live Sessions and canvas connections. diff --git a/hub/docs/security-audit.md b/hub/docs/security-audit.md index 874d55ee..fb9125b3 100644 --- a/hub/docs/security-audit.md +++ b/hub/docs/security-audit.md @@ -32,7 +32,7 @@ An existing Hub administrator opens **Hub administrators** from the sidebar, or Choose **Add administrator** to open a dialog and search existing GitHub-backed Hub users by login or name. Current administrators and local identities are excluded. Search returns up to 50 matches; refine the search to find other users. Selected accounts stay checked in the same list across searches, including when they do not match the current query. Select up to 50 users across searches, review the selected accounts and consequence, check the confirmation box, then choose **Add selected administrators**. The selection commits together with one success audit per user. If any account is missing, already an administrator, or cannot be saved, no selected roles change. -The same browser Session must have verified the same GitHub identity within the last five minutes. If Hub opens **Verify with GitHub**, complete that check, reopen the dialog, select users and confirm again. The callback never saves the selection or replays a role change. Removal uses its existing account confirmation page. See [Fresh GitHub authorization](fresh-authorization.md). +The same browser Session must have verified the same GitHub identity within the last five minutes. If Hub opens **Verify with GitHub**, complete that check and Hub continues the role change you already confirmed. The saved selection is bound to that browser and used once, after permission and target checks. Removal uses its existing account confirmation page. See [Fresh GitHub authorization](fresh-authorization.md). Removal takes effect immediately, including removal of your own role. Hub never removes the final administrator, even when requests overlap. It rereads the actor's role and initiating Session inside the same serialized transaction as the role change and success audit. A stale administrator, deleted Session, missing target, or already-changed role cannot change another user's role. A denial explains that no changes were made, including when the former administrator has no repository to return to. @@ -43,7 +43,7 @@ The role permits Hub administrator controls and global security records, not Git Add a replacement before removing a departing administrator. Hub will not remove its final administrator. 1. Have the replacement sign in with their own GitHub account so Hub can find them. If they cannot sign in through ordinary repository access, use the host-authorized [enrollment process](#enroll-an-operator) instead. Successful enrollment already grants the role; skip step 2 and verify access in step 3. -2. If the replacement is not already an administrator, open `/hub_admins`, choose **Add administrator**, select the existing account, review the selection and confirm. Complete **Verify with GitHub** and explicitly submit again if asked. +2. If the replacement is not already an administrator, open `/hub_admins`, choose **Add administrator**, select the existing account, review the selection and confirm. Complete **Verify with GitHub** if asked; Hub then continues the confirmed addition. 3. Have the replacement sign in in their own browser and open `/security_audit` and `/hub_admins`. Confirm that they can manage Hub, not just read a repository. 4. The replacement removes the departing account's Hub-admin role. Do this before ending sessions, and confirm that at least one authorised administrator remains. 5. Follow the [offboarding procedure](deployment.md#offboard-a-person-or-contain-a-reported-suspension) for GitHub access, public links and existing browser/canvas credentials. Removing an administrator role alone does not perform those actions. diff --git a/hub/docs/session-revocation.md b/hub/docs/session-revocation.md index 98409961..fca65740 100644 --- a/hub/docs/session-revocation.md +++ b/hub/docs/session-revocation.md @@ -8,7 +8,7 @@ This ends existing authentication, not the ability to sign in. People can comple Only a current, persisted, GitHub-backed Hub administrator may open `/session_revocation` or submit its CSRF-protected form. The same browser Session must have verified the same GitHub identity strictly less than five minutes ago. Another browser's proof or a canvas grant cannot authorize this action. -If Hub opens **Verify with GitHub**, complete that check, return to the confirmation page, and submit again. The callback never replays the original mutation. Cancel and missing confirmation leave all credentials unchanged and record a denial. See [Fresh GitHub authorization](fresh-authorization.md). +If Hub opens **Verify with GitHub**, complete that check and Hub continues the session revocation you already confirmed, including ending your own session. Cancel and missing confirmation leave all credentials unchanged and record a denial. See [Fresh GitHub authorization](fresh-authorization.md). The page explains that the operator's own Session will end. Counts are advisory: new sessions or approvals can arrive before confirmation. With only the operator's browser remaining, the action still ends that Session. diff --git a/hub/docs/share-links.md b/hub/docs/share-links.md index 53040485..1e651e12 100644 --- a/hub/docs/share-links.md +++ b/hub/docs/share-links.md @@ -6,7 +6,7 @@ Share links let prototype authors show one successful Build to people who do not Public sharing is **off by default for new and upgraded deployments**. For example, after an upgrade an old public URL returns not found even if its original expiration is still in the future. The upgrade permanently revokes every existing link and pending preview handoff. -A Hub administrator opens **Public sharing** from the deployment security audit, or `/sharing_settings`, to enable new links. Changes require deliberate confirmation and [fresh GitHub authorization](fresh-authorization.md) on that same browser Session. An OAuth callback never replays a toggle. Repository administrators and Build authors cannot change this deployment setting, and Hub administrators do not gain repository-content access. +A Hub administrator opens **Public sharing** from the deployment security audit, or `/sharing_settings`, to enable new links. Changes require deliberate confirmation and [fresh GitHub authorization](fresh-authorization.md) on that same browser Session. Successful verification continues the confirmed change through its normal permission checks. Repository administrators and Build authors cannot change this deployment setting, and Hub administrators do not gain repository-content access. Disabling permanently revokes all links and pending handoffs in the same transaction as the policy change and its security audit. It blocks anonymous share pages, preview authorization, cookies, files, browser routes and controls. Enabling later permits **new links only**; no revoked link returns. Signed-in previews, reviews and immutable Builds remain unchanged. diff --git a/hub/test/integration/copilot_connections_test.rb b/hub/test/integration/copilot_connections_test.rb index d21bdca6..b10cf892 100644 --- a/hub/test/integration/copilot_connections_test.rb +++ b/hub/test/integration/copilot_connections_test.rb @@ -1,7 +1,10 @@ require "open3" require "test_helper" +require_relative "../support/authorization_continuation_helpers" class CopilotConnectionsTest < ActionDispatch::IntegrationTest + include AuthorizationContinuationHelpers + CLI = Rails.root.join("test/fixtures/files/fake-replica-learning.mjs").to_s setup do @@ -131,20 +134,16 @@ class CopilotConnectionsTest < ActionDispatch::IntegrationTest CopilotConnection.private_class_method(:check!) end - test "connecting needs fresh GitHub proof and never replays" do + test "connecting continues after fresh GitHub proof without another confirmation" do @session.update!(fresh_authorized_at: nil) post copilot_connection_path, params: { confirmed: "1" } challenge = FreshAuthorization.last assert_equal "copilot_connection", challenge.purpose assert_redirected_to fresh_authorization_path(challenge.public_id) - post fresh_authorization_path(challenge.public_id) - state = Rack::Utils.parse_query(URI(response.location).query).fetch("state") - get github_authentication_callback_path, params: { code: "code", state: } - assert_redirected_to copilot_connection_path - assert_nil CopilotConnection.current, "verification never submits the change" - - post copilot_connection_path, params: { confirmed: "1" } + form = verify_pending_action(challenge, path: copilot_connection_path, method: "post") + assert_nil CopilotConnection.current + continue_pending_action(form) assert_equal I18n.t("copilot_connection.connected"), flash[:notice] assert CopilotConnection.current @@ -166,7 +165,7 @@ class CopilotConnectionsTest < ActionDispatch::IntegrationTest end end - test "disconnecting needs fresh GitHub proof, never replays, and leaves the token with the operator" do + test "disconnecting continues after fresh proof and leaves the token with the operator" do post copilot_connection_path, params: { confirmed: "1" } @session.update!(fresh_authorized_at: nil) @@ -174,13 +173,9 @@ class CopilotConnectionsTest < ActionDispatch::IntegrationTest challenge = FreshAuthorization.last assert_equal "copilot_disconnection", challenge.purpose assert_redirected_to fresh_authorization_path(challenge.public_id) - post fresh_authorization_path(challenge.public_id) - state = Rack::Utils.parse_query(URI(response.location).query).fetch("state") - get github_authentication_callback_path, params: { code: "code", state: } - assert_redirected_to copilot_connection_path - assert CopilotConnection.current, "verification never submits the change" - - delete copilot_connection_path, params: { confirmed: "1" } + form = verify_pending_action(challenge, path: copilot_connection_path, method: "delete") + assert CopilotConnection.current + continue_pending_action(form) assert_redirected_to copilot_connection_path assert_equal I18n.t("copilot_connection.disconnected"), flash[:notice] diff --git a/hub/test/integration/fresh_authorizations_test.rb b/hub/test/integration/fresh_authorizations_test.rb index 2899e6bf..0872f0f5 100644 --- a/hub/test/integration/fresh_authorizations_test.rb +++ b/hub/test/integration/fresh_authorizations_test.rb @@ -1,7 +1,10 @@ require "test_helper" +require_relative "../support/authorization_continuation_helpers" # Keeps legacy never-expiring authorization covered independently of the current cap. class FreshAuthorizationsTest < ActionDispatch::IntegrationTest + include AuthorizationContinuationHelpers + setup do @original_share_link_maximum = Rails.configuration.x.share_link_maximum_lifetime Rails.configuration.x.share_link_maximum_lifetime = nil @@ -58,40 +61,91 @@ class FreshAuthorizationsTest < ActionDispatch::IntegrationTest assert_nil ShareLink.last.expires_at end - test "analytics mutations never save or replay submitted credentials" do + test "analytics creation retains encrypted credentials and continues exactly once" do + assert_no_difference("Analytics::Destination.count") do + post @analytics_path, params: { destination: analytics_configuration } + end + challenge = FreshAuthorization.last + raw = FreshAuthorization.connection.select_value( + "SELECT pending_action FROM fresh_authorizations WHERE id = #{challenge.id}" + ) + assert_not_includes raw, "submitted-secret" + form = verify_pending_action(challenge, path: @analytics_path, method: "post") + assert_not_includes response.body, "submitted-secret" + assert_empty @repository.analytics_destinations + continue_pending_action(form, destination: analytics_configuration.merge(credentials: { write_key: "injected" })) + destination = @repository.analytics_destinations.sole + assert_equal "submitted-secret", destination.credentials.fetch("write_key") + assert_nil challenge.reload.pending_action + assert_no_difference("Analytics::Destination.count") do + continue_pending_action(form) + end + end + + test "analytics credential updates pause resume and deletion continue their original actions" do destination = @repository.analytics_destinations.create! \ provider: "segment", region: "us", credentials: { write_key: "existing" }, event_types: Analytics::Event::TYPES path = repository_analytics_destination_path(@repository.route_id, destination) - assert_no_difference("Analytics::Destination.count") do - post @analytics_path, params: { destination: analytics_configuration } + changes = [ + [ "patch", { destination: analytics_configuration }, -> { + assert_equal "submitted-secret", destination.reload.credentials.fetch("write_key") + } ], + [ "patch", { operation: "pause" }, -> { assert_not destination.reload.enabled? } ], + [ "patch", { operation: "resume" }, -> { assert destination.reload.enabled? } ], + [ "delete", {}, -> { assert_not Analytics::Destination.exists?(destination.id) } ] + ] + changes.each do |method, input, outcome| + @browser_session.reload.update!(fresh_authorized_at: nil) + travel 1.minute + public_send(method, path, params: input) + form = verify_pending_action(FreshAuthorization.last, path:, method:) + continue_pending_action(form) + assert_redirected_to repository_analytics_path(@repository.route_id) + outcome.call + end + end + + test "an expired verified action cannot continue" do + post @analytics_path, params: { destination: analytics_configuration } + challenge = FreshAuthorization.last + form = verify_pending_action(challenge, path: @analytics_path, method: "post") + travel 16.minutes do + continue_pending_action(form) + assert_empty @repository.analytics_destinations + assert_nil challenge.reload.pending_action end + end + + test "another signed in browser cannot execute a verified analytics action" do + post @analytics_path, params: { destination: analytics_configuration } challenge = FreshAuthorization.last - assert_equal "analytics_destination", challenge.purpose - assert_not_includes challenge.attributes.to_s, "submitted-secret" - patch path, params: { destination: { credentials: { write_key: "submitted-secret" } } } - assert_equal "existing", destination.reload.credentials.fetch("write_key") - patch path, params: { operation: "pause" } - assert destination.reload.enabled? - delete path - assert Analytics::Destination.exists?(destination.id) - callback(start_verification(FreshAuthorization.last)) - assert_equal "existing", destination.reload.credentials.fetch("write_key") - patch path, params: { operation: "pause" } - assert_not destination.reload.enabled? + form = verify_pending_action(challenge, path: @analytics_path, method: "post") + other = open_session + other.host! "replica.localhost:19500" + other.post github_authentication_path + state = Rack::Utils.parse_query(URI(other.response.location).query).fetch("state") + other.get github_authentication_callback_path, params: { code: "code", state: } + other.post form.fetch("submitPath"), params: { authorization_continuation_id: challenge.public_id } + assert_empty @repository.analytics_destinations + continue_pending_action(form) + assert_equal "submitted-secret", @repository.analytics_destinations.sole.credentials.fetch("write_key") end test "cancellation failure and identity mismatch leave sensitive requests unapplied" do - create_share + post @analytics_path, params: { destination: analytics_configuration } challenge = FreshAuthorization.last delete fresh_authorization_path(challenge.public_id) assert_equal I18n.t("fresh_authorization.cancelled"), flash[:alert] + assert_nil challenge.reload.pending_action assert_not FreshAuthorization.fresh?(@browser_session) - create_share - state = start_verification(FreshAuthorization.last) + post @analytics_path, params: { destination: analytics_configuration } + challenge = FreshAuthorization.last + state = start_verification(challenge) stub_github_oauth_user(github_id: 1002, login: "mona") callback(state) assert_equal I18n.t("fresh_authorization.failure"), flash[:alert] + assert_nil challenge.reload.pending_action assert_not FreshAuthorization.fresh?(@browser_session) assert_equal "denied", SecurityAuditEvent.where(action: "fresh_authorization.verify").last.outcome create_share @@ -101,6 +155,7 @@ class FreshAuthorizationsTest < ActionDispatch::IntegrationTest assert_equal "failed", SecurityAuditEvent.where(action: "fresh_authorization.verify").last.outcome assert_not FreshAuthorization.fresh?(@browser_session) assert_empty ShareLink.all + assert_empty @repository.analytics_destinations end test "expired and denied GitHub callbacks cannot renew or fall through to sign in" do @@ -137,13 +192,13 @@ class FreshAuthorizationsTest < ActionDispatch::IntegrationTest end test "fresh identity does not bypass current repository authorization" do - create_share - callback(start_verification(FreshAuthorization.last)) + post @analytics_path, params: { destination: analytics_configuration } + form = verify_pending_action(FreshAuthorization.last, path: @analytics_path, method: "post") stub_github_permissions([ @payload ], { 2001 => nil }) assert_no_difference("ShareLink.count") { create_share } assert_response :not_found assert_no_difference("Analytics::Destination.count") do - post @analytics_path, params: { destination: analytics_configuration } + continue_pending_action(form) end assert_response :not_found end diff --git a/hub/test/integration/github_app_administration_test.rb b/hub/test/integration/github_app_administration_test.rb index d1d136b9..3885bb1f 100644 --- a/hub/test/integration/github_app_administration_test.rb +++ b/hub/test/integration/github_app_administration_test.rb @@ -1,7 +1,10 @@ require "test_helper" require "tempfile" +require_relative "../support/authorization_continuation_helpers" class GithubAppAdministrationTest < ActionDispatch::IntegrationTest + include AuthorizationContinuationHelpers + setup do @configuration = github_app_configurations.create_installed @actor = users.create(hub_admin: true) @@ -47,25 +50,34 @@ class GithubAppAdministrationTest < ActionDispatch::IntegrationTest assert_not_requested :get, "#{GITHUB_API_URL}/app" end - test "stale proof challenges before parsing an upload and never replays credentials" do + test "stale proof continues the saved replacement credentials and upload" do @session.update!(fresh_authorized_at: 5.minutes.ago) - patch github_app_credentials_path, - params: confirmation.merge(client_secret: "never-store-this", private_key: "not-an-upload") + Tempfile.create([ "replacement", ".pem" ], Rails.root.join("storage")) do |key| + key.write(github_app_configurations.test_private_key) + key.flush + patch github_app_credentials_path, params: confirmation.merge( + client_secret: "replacement-client-secret", + private_key: Rack::Test::UploadedFile.new(key.path, "application/x-pem-file") + ) + end challenge = FreshAuthorization.last assert_redirected_to fresh_authorization_path(challenge.public_id) - assert_equal "github_app_credentials", challenge.purpose - assert_not_includes challenge.attributes.to_s, "never-store-this" - post fresh_authorization_path(challenge.public_id) - state = Rack::Utils.parse_query(URI(response.location).query).fetch("state") - get github_authentication_callback_path, params: { code: "code", state: } - assert_redirected_to edit_github_app_credentials_path + form = verify_pending_action(challenge, path: github_app_credentials_path, method: "patch") assert_equal "client-secret", @configuration.reload.client_secret - assert_not_requested :get, "#{GITHUB_API_URL}/app" + continue_pending_action(form) + assert_redirected_to github_app_path + assert_equal "replacement-client-secret", @configuration.reload.client_secret + assert_equal github_app_configurations.test_private_key, @configuration.private_key + assert_equal I18n.t("github_app.credentials.saved"), flash[:notice] + assert_not FreshAuthorization.exists?(challenge.id) end test "confirmed server-side webhook start and finish are separately authorized" do stub_request(:patch, "#{GITHUB_API_URL}/app/hook/config").to_return(status: 200, body: "{}") + @session.update!(fresh_authorized_at: nil) post github_app_webhook_rotation_path, params: confirmation.merge(operation: "start") + form = verify_pending_action(FreshAuthorization.last, path: github_app_webhook_rotation_path, method: "post") + continue_pending_action(form) assert_redirected_to github_app_webhook_rotation_path assert_equal "confirmed", @configuration.reload.webhook_rotation_state event = SecurityAuditEvent.where(action: "github_app.rotate_webhook_secret").last @@ -75,6 +87,31 @@ class GithubAppAdministrationTest < ActionDispatch::IntegrationTest delete github_app_webhook_rotation_path, params: confirmation assert_equal "github_app_finish_webhook", FreshAuthorization.last.purpose assert @configuration.reload.previous_webhook_secret + form = verify_pending_action(FreshAuthorization.last, path: github_app_webhook_rotation_path, method: "delete") + continue_pending_action(form) + assert_nil @configuration.reload.previous_webhook_secret + assert_equal "idle", @configuration.webhook_rotation_state + assert_equal I18n.t("github_app.webhook.finished"), flash[:notice] + end + + test "an App version conflict is reported after verification and consumes the saved reset" do + original = ENV["REPLICA_BOOTSTRAP_SECRET"] + ENV["REPLICA_BOOTSTRAP_SECRET"] = "manual" + @session.update!(fresh_authorized_at: nil) + delete github_app_path, params: confirmation.merge( + confirmation: GithubAppConfiguration::Administration::RESET_PHRASE + ) + challenge = FreshAuthorization.last + form = verify_pending_action(challenge, path: github_app_path, method: "delete") + @configuration.update!(webhook_secret: "changed-after-verification") + continue_pending_action(form, expected_version: @configuration.lock_version.to_s) + assert GithubAppConfiguration.exists?(@configuration.id) + assert Session.exists?(@session.id) + assert_equal I18n.t("github_app.errors.state_changed"), flash[:alert] + assert_nil challenge.reload.pending_action + assert challenge.continued_at + ensure + ENV["REPLICA_BOOTSTRAP_SECRET"] = original end test "replacement upload saves encrypted credentials and returns only a safe notice" do @@ -97,11 +134,15 @@ class GithubAppAdministrationTest < ActionDispatch::IntegrationTest test "App mutations retain browser forgery protection" do previous = ActionController::Base.allow_forgery_protection - ActionController::Base.allow_forgery_protection = true + @session.update!(fresh_authorized_at: nil) post github_app_webhook_rotation_path, params: confirmation.merge(operation: "start") + challenge = FreshAuthorization.last + form = verify_pending_action(challenge, path: github_app_webhook_rotation_path, method: "post") + ActionController::Base.allow_forgery_protection = true + continue_pending_action(form) assert_response :unprocessable_entity assert_equal "idle", @configuration.reload.webhook_rotation_state - assert_empty FreshAuthorization.all + assert challenge.reload.pending_action ensure ActionController::Base.allow_forgery_protection = previous end @@ -124,7 +165,7 @@ class GithubAppAdministrationTest < ActionDispatch::IntegrationTest patch github_app_credentials_path, params: { client_secret: "never-store-this" } assert_equal I18n.t("github_app.errors.confirmation_required"), flash[:alert] patch github_app_credentials_path, - params: confirmation.merge(client_secret: "never-store-this", private_key: "not-an-upload") + params: confirmation.merge(client_secret: "never-store-this", private_key: github_app_configurations.test_private_key) assert_equal I18n.t("github_app.errors.invalid_credentials"), flash[:alert] assert_not_includes flash.to_hash.to_s, "never-store-this" assert_not_requested :get, "#{GITHUB_API_URL}/app" @@ -133,9 +174,12 @@ class GithubAppAdministrationTest < ActionDispatch::IntegrationTest test "reset signs out users and retains roles without changing the remote App" do original = ENV["REPLICA_BOOTSTRAP_SECRET"] ENV["REPLICA_BOOTSTRAP_SECRET"] = "manual" + @session.update!(fresh_authorized_at: nil) delete github_app_path, params: confirmation.merge( confirmation: GithubAppConfiguration::Administration::RESET_PHRASE ) + form = verify_pending_action(FreshAuthorization.last, path: github_app_path, method: "delete") + continue_pending_action(form) assert_redirected_to root_path assert_empty Session.all assert @actor.reload.hub_admin? diff --git a/hub/test/integration/hub_admins_test.rb b/hub/test/integration/hub_admins_test.rb index a243de15..80f88a42 100644 --- a/hub/test/integration/hub_admins_test.rb +++ b/hub/test/integration/hub_admins_test.rb @@ -1,6 +1,9 @@ require "test_helper" +require_relative "../support/authorization_continuation_helpers" class HubAdminsTest < ActionDispatch::IntegrationTest + include AuthorizationContinuationHelpers + setup do github_app_configurations.create_installed @payload = repository_payload(2001, "github/prototype") @@ -66,7 +69,7 @@ class HubAdminsTest < ActionDispatch::IntegrationTest assert_includes response.body, I18n.t("hub_admins.errors.not_authorized") end - test "stale browser proof challenges without changing or replaying the role" do + test "stale browser proof continues the original role change" do @session.update!(fresh_authorized_at: 5.minutes.ago) assert_no_difference -> { User.where(hub_admin: true).count } do change("designate") @@ -77,17 +80,11 @@ class HubAdminsTest < ActionDispatch::IntegrationTest assert_equal @session.id, challenge.credential_id assert_equal edit_hub_admin_path(@target), challenge.return_to assert_equal @target.id, SecurityAuditEvent.where(action: "hub_admin.designate").last.context["target_user_id"] - post fresh_authorization_path(challenge.public_id) - state = Rack::Utils.parse_query(URI(response.location).query).fetch("state") - assert_no_difference -> { User.where(hub_admin: true).count } do - get github_authentication_callback_path, params: { code: "code", state: } - end - assert_redirected_to edit_hub_admin_path(@target) - follow_redirect! - assert_response :success + form = verify_pending_action(challenge, path: @path, method: "patch") assert_not @target.reload.hub_admin? - change("designate") + continue_pending_action(form, operation: "revoke", confirmed: "0", user_id: @actor.id) assert @target.reload.hub_admin? + assert_equal I18n.t("hub_admins.changed"), flash[:notice] end test "another Session's proof and submitted timestamps do not authorize this browser" do @@ -98,7 +95,7 @@ class HubAdminsTest < ActionDispatch::IntegrationTest assert_not @target.reload.hub_admin? end - test "administrator removal also halts at the fresh authorization gate" do + test "administrator removal continues after fresh authorization" do @target.update!(hub_admin: true) @session.update!(fresh_authorized_at: 5.minutes.ago) change("revoke") @@ -106,6 +103,9 @@ class HubAdminsTest < ActionDispatch::IntegrationTest assert @target.reload.hub_admin? assert_equal "hub_admin_management", FreshAuthorization.last.purpose assert_equal "denied", SecurityAuditEvent.where(action: "hub_admin.revoke").last.outcome + form = verify_pending_action(FreshAuthorization.last, path: @path, method: "patch") + continue_pending_action(form) + assert_not @target.reload.hub_admin? end test "management works without repository access and self removal still shows its result" do @@ -328,7 +328,7 @@ class HubAdminsTest < ActionDispatch::IntegrationTest assert_equal "denied", SecurityAuditEvent.where(action: "hub_admin.designate").last.outcome end - test "bulk addition requires this browser's fresh proof and never replays its selection" do + test "bulk addition continues only the saved selection after this browser verifies" do @actor.sessions.create!(fresh_authorized_at: Time.current) @session.update!(fresh_authorized_at: nil) post hub_admins_path, params: { @@ -339,13 +339,12 @@ class HubAdminsTest < ActionDispatch::IntegrationTest assert_equal hub_admins_path, challenge.return_to assert_equal "hub_admin_management", challenge.purpose assert_equal @session.id, challenge.credential_id - post fresh_authorization_path(challenge.public_id) - state = Rack::Utils.parse_query(URI(response.location).query).fetch("state") - get github_authentication_callback_path, params: { code: "code", state: } - assert_redirected_to hub_admins_path + form = verify_pending_action(challenge, path: hub_admins_path, method: "post") assert_not @target.reload.hub_admin? - post hub_admins_path, params: { user_ids: [ @target.id ], confirmed: "1" } + other = users.create(github_id: 1003, login: "other") + continue_pending_action(form, user_ids: [ other.id ], confirmed: "0") assert @target.reload.hub_admin? + assert_not other.reload.hub_admin? end test "revoked administrators cannot search users or submit bulk selections" do diff --git a/hub/test/integration/session_revocations_test.rb b/hub/test/integration/session_revocations_test.rb index b4170676..915a6b62 100644 --- a/hub/test/integration/session_revocations_test.rb +++ b/hub/test/integration/session_revocations_test.rb @@ -1,6 +1,9 @@ require "test_helper" +require_relative "../support/authorization_continuation_helpers" class SessionRevocationsTest < ActionDispatch::IntegrationTest + include AuthorizationContinuationHelpers + setup do @original_bootstrap_secret = ENV["REPLICA_BOOTSTRAP_SECRET"] ENV["REPLICA_BOOTSTRAP_SECRET"] = "session-cutoff-bootstrap" @@ -93,7 +96,7 @@ class SessionRevocationsTest < ActionDispatch::IntegrationTest assert_equal "denied", SecurityAuditEvent.where(action: "session_revocation.end_all").last.outcome end - test "exactly five-minute proof challenges and its callback never replays revocation" do + test "exactly five-minute proof continues the confirmed all-session revocation" do @session.update!(fresh_authorized_at: 5.minutes.ago) assert_no_difference("Session.count") do post session_revocation_path, params: { confirmed: "1" } @@ -103,14 +106,9 @@ class SessionRevocationsTest < ActionDispatch::IntegrationTest assert_equal "session_revocation", challenge.purpose assert_equal @session.id, challenge.credential_id assert_equal session_revocation_path, challenge.return_to - post fresh_authorization_path(challenge.public_id) - state = Rack::Utils.parse_query(URI(response.location).query).fetch("state") - assert_no_difference("Session.count") do - get github_authentication_callback_path, params: { code: "code", state: } - end - assert_redirected_to session_revocation_path + form = verify_pending_action(challenge, path: session_revocation_path, method: "post") assert FreshAuthorization.fresh?(@session) - post session_revocation_path, params: { confirmed: "1" } + continue_pending_action(form) assert_response :success assert_empty Session.all end diff --git a/hub/test/integration/sharing_settings_test.rb b/hub/test/integration/sharing_settings_test.rb index 25e89065..16084baa 100644 --- a/hub/test/integration/sharing_settings_test.rb +++ b/hub/test/integration/sharing_settings_test.rb @@ -1,6 +1,9 @@ require "test_helper" +require_relative "../support/authorization_continuation_helpers" class SharingSettingsTest < ActionDispatch::IntegrationTest + include AuthorizationContinuationHelpers + setup do github_app_configurations.create_installed @payload = repository_payload(2001, "github/prototype") @@ -66,7 +69,7 @@ class SharingSettingsTest < ActionDispatch::IntegrationTest assert_not SharingSetting.enabled? end - test "stale proof redirects without changes and verification never replays the toggle" do + test "stale proof continues enabling and disabling public sharing" do @session.update!(fresh_authorized_at: 5.minutes.ago) assert_no_difference("SharingSetting.count") { change("enable") } challenge = FreshAuthorization.last @@ -74,17 +77,29 @@ class SharingSettingsTest < ActionDispatch::IntegrationTest assert_equal @session.id, challenge.credential_id assert_equal sharing_settings_path, challenge.return_to assert_redirected_to fresh_authorization_path(challenge.public_id) - post fresh_authorization_path(challenge.public_id) - state = Rack::Utils.parse_query(URI(response.location).query).fetch("state") - get github_authentication_callback_path, params: { code: "code", state: } - assert_redirected_to sharing_settings_path + form = verify_pending_action(challenge, path: sharing_settings_path, method: "patch") assert_not SharingSetting.enabled? - change("enable") + continue_pending_action(form) assert SharingSetting.enabled? @session.update!(fresh_authorized_at: nil) change("disable") assert SharingSetting.enabled? assert_equal "sharing_settings", FreshAuthorization.last.purpose + form = verify_pending_action(FreshAuthorization.last, path: sharing_settings_path, method: "patch") + continue_pending_action(form) + assert_not SharingSetting.enabled? + end + + test "an administrator removed after verification cannot continue a saved toggle" do + @session.update!(fresh_authorized_at: nil) + change("enable") + challenge = FreshAuthorization.last + form = verify_pending_action(challenge, path: sharing_settings_path, method: "patch") + @actor.update!(hub_admin: false) + continue_pending_action(form) + assert_response :forbidden + assert_not SharingSetting.enabled? + assert_nil challenge.reload.pending_action end test "another browser Session and submitted freshness cannot authorize this browser" do diff --git a/hub/test/support/authorization_continuation_helpers.rb b/hub/test/support/authorization_continuation_helpers.rb new file mode 100644 index 00000000..55fcd096 --- /dev/null +++ b/hub/test/support/authorization_continuation_helpers.rb @@ -0,0 +1,27 @@ +module AuthorizationContinuationHelpers + private + + def verify_pending_action(challenge, path:, method:) + post fresh_authorization_path(challenge.public_id) + state = Rack::Utils.parse_query(URI(response.location).query).fetch("state") + get github_authentication_callback_path, params: { code: "code", state: } + assert_redirected_to fresh_authorization_path(challenge.public_id) + follow_redirect!(as: :json) + assert_response :success + form = JSON.parse(response.body).fetch("data") + assert_equal path, form.fetch("submitPath") + assert_equal({ "id" => challenge.public_id, "method" => method }, form.fetch("continuation")) + assert_equal "private, no-store", response.headers["Cache-Control"] + form + end + + def continue_pending_action(form, **extra) + continuation = form.fetch("continuation") + post \ + form.fetch("submitPath"), + params: extra.merge( + authorization_continuation_id: continuation.fetch("id"), + _method: continuation.fetch("method") + ) + end +end