Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions hub/app/controllers/concerns/fresh_authorization_required.rb
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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 \
Expand All @@ -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
5 changes: 3 additions & 2 deletions hub/app/controllers/fresh_authorizations_controller.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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)
Expand Down
18 changes: 12 additions & 6 deletions hub/app/controllers/github_app_credentials_controller.rb
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion hub/app/controllers/github_authentications_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ 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(
<ThemeProvider>
<FreshAuthorizationPage
data={{
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",
Expand Down Expand Up @@ -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",
Expand Down
22 changes: 19 additions & 3 deletions hub/app/javascript/pages/fresh-authorization-page/index.tsx
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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<HTMLFormElement>
}) {
const actions = (
<Stack direction="vertical" gap="normal">
<form action={data.submitPath} method="post">
<form action={data.submitPath} method="post" ref={continuationFormRef}>
<input type="hidden" name="authenticity_token" value={data.authenticityToken} />
{data.continuation && (
<>
<input
type="hidden"
name="authorization_continuation_id"
value={data.continuation.id}
/>
<input type="hidden" name="_method" value={data.continuation.method} />
</>
)}
<Button type="submit" variant="primary" block>
{data.submitLabel}
</Button>
Expand Down
24 changes: 16 additions & 8 deletions hub/app/javascript/pages/fresh-authorization-page/stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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) => (
<div style={{ width: "24rem", maxWidth: "100%" }}>
<FreshAuthorizationPage {...args} />
</div>
),
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" },
},
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ export interface FreshAuthorizationPageData extends GithubAppSetupHeaderData {
authenticityToken: string
cancelLabel?: string
cancelPath?: string
continuation?: { id: string; method: string }
}
44 changes: 44 additions & 0 deletions hub/app/javascript/pages/fresh-authorization-page/view.test.tsx
Original file line number Diff line number Diff line change
@@ -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(
<StrictMode>
<ThemeProvider>
<FreshAuthorizationsShowView />
</ThemeProvider>
</StrictMode>,
)
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",
})
})
})
3 changes: 2 additions & 1 deletion hub/app/javascript/pages/github-app-page/stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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" },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

Expand All @@ -44,15 +44,6 @@ export default meta
type Story = StoryObj<typeof meta>

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 },
}
Expand Down
11 changes: 2 additions & 9 deletions hub/app/javascript/pages/session-revocation-page/stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<typeof SessionRevocationPage>
Expand All @@ -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: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
Loading
Loading