BC5 OAuth: resource-first discovery, device-flow login, RFC 8707 resource echo by jeremy · Pull Request #582 · basecamp/basecamp-cli

jeremy · 2026-07-28T04:04:04Z

Adopts the SDK's BC5 OAuth stack end to end: resource-first discovery (RFC 9728 + 8414), RFC 8628 device-flow login as the pre-registered public basecamp-cli client, and the RFC 8707 multi-account refresh contract.

Why the resource echo matters: basecamp-cli is a trusted client in bc3's registry, so a device approval records an identity-wide grant and mints a multi-account refresh token. bc3's refresh grant hard-requires the RFC 8707 resource parameter for those (400 invalid_request without it) — without persisting and echoing it, every device login dies at its first token expiry (~1h). Credentials gain a resource field (auth + mirrored sdk store); device login persists token.Resource; refresh echoes it and preserves it when the refresh response omits it. An end-to-end test drives login → stored resource → forced refresh → form echo → binding surviving a resource-less rotation.

Gating: DRAFT until basecamp-sdk#478 (resource indicator + 429 poll contract, stacked on #376/#370) merges — the SDK pin is currently the #478 branch head (e583c994 pseudo-version + provenance); it moves to the merged SHA before this lands. No SDK release required first, per current pseudo-version practice.

Also restacked onto main (was 36 behind): reconciled the go.mod/go.sum/provenance conflict to main's newer pin, dropped stale go.sum entries the auto-merge left, and absorbed SDK API drift since the branch's old 7207f742 snapshot (VerificationURIComplete/RegistrationEndpoint → *string).

Summary by cubic

Adopts the BC5 OAuth stack end to end—resource‑first discovery, automatic device‑flow login as public basecamp-cli, and RFC 8707 resource echo—with stricter URL/port validation and provider‑neutral flags. Upgrades github.com/basecamp/basecamp-sdk/go to v0.9.1-0.20260728112802-e9469832f102 for hardened device‑flow handling and resource persistence.

New Features
- Resource‑first discovery (RFC 9728 → RFC 8414) from the API origin with canonicalized origins; only discovery/no‑issuer paths fall back to Launchpad.
- Device‑flow login as basecamp-cli with validated verification URLs (incl. 1–65535 port range), sanitized display, and early abort on bad data; --device-code forces headless remote flow; flag/help text is provider‑neutral.
- Persist and echo the RFC 8707 resource across refreshes for multi‑account tokens; mirrored in the SDK store.
- SDK bump to github.com/basecamp/basecamp-sdk/go@e9469832 for stricter protocol error gating, clamped timeouts, resilient Retry‑After parsing, cooperative cancellation, and exact‑200 device token acceptance.
Migration
- Removed the BC3 dev‑only DCR/PKCE flow; client.json is obsolete and safe to delete.
- Stored legacy "bc3" credentials will fail to refresh; re‑authenticate to migrate.

^{Written for commit 2414bc6. Summary will update on new commits.}

Copilot

Pull request overview

This PR updates Basecamp CLI authentication to adopt the SDK’s BC5 OAuth stack end-to-end: resource-first discovery (RFC 9728 → 8414), automatic device-flow login when available (RFC 8628), and persistence/echoing of the RFC 8707 resource indicator so multi-account refresh tokens can be refreshed reliably.

Changes:

Implement provider discovery + BC5 device-flow login path, including endpoint validation and sanitized device-code display, and persist/echo resource on refresh.
Update CLI help/docs to be provider-neutral and clarify --device-code semantics; document the now-obsolete client.json.
Bump github.com/basecamp/basecamp-sdk/go to the PR’s required pseudo-version + update provenance; expand unit and e2e coverage around auth/flags/refresh behavior.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
skills/basecamp/SKILL.md	Document `client.json` as obsolete; clarify `--scope` and `--device-code` help text.
README.md	Update authentication docs to describe device-flow-first behavior and provider-neutral flags; remove `client.json` from config tree and note it’s safe to delete.
internal/version/sdk-provenance.json	Update pinned SDK revision metadata to match the new pseudo-version.
internal/sdk/store.go	Add persisted `resource` field to SDK-facing credentials struct.
internal/completion/refresh_test.go	Ensure test client config includes `BaseURL` for refreshed completion behavior.
internal/commands/profile.go	Update `profile create` flag help strings to match provider-neutral semantics.
internal/commands/profile_test.go	Add regression coverage ensuring `--device-code` forces remote/paste-callback mode.
internal/commands/auth.go	Update `auth login` flag help strings to match provider-neutral semantics.
internal/commands/auth_login_test.go	Add regression coverage ensuring `--device-code` forces remote/paste-callback mode in `auth login`.
internal/auth/keyring.go	Persist `resource` in stored credentials (keyring/file fallback).
internal/auth/device_test.go	Add comprehensive unit coverage for discovery, device flow, endpoint hardening, and RFC 8707 resource echo on refresh.
internal/auth/auth.go	Implement resource-first discovery, device-flow login path, refresh `resource` echo/preservation, and shared endpoint validation.
internal/auth/auth_test.go	Update existing auth tests to match the new discovery and Launchpad-only authorization-code behavior.
go.mod	Bump basecamp-sdk dependency to required pseudo-version.
go.sum	Update module sums for the SDK bump.
e2e/auth.bats	Add e2e assertions for updated help text phrasing (provider-neutral flags).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cubic-dev-ai

1 issue found across 16 files

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/auth/auth.go">

<violation number="1" location="internal/auth/auth.go:762">
P1: Equivalent base-URL spellings with an explicit default port or mixed-case host will miss BC5 discovery and fall back to Launchpad. Canonicalize scheme, hostname, and default ports before returning the resource origin so it matches the protected-resource metadata identifier.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

Bump the SDK to the oauth-device-flow head (7207f742) and rework login around it: - Discovery is now resource-first (RFC 9728 -> RFC 8414) from the base URL's origin via DiscoverFromResource. Only the two soft pre-selection outcomes (resource_discovery_failed, no_as_advertised) fall back to Launchpad; once a BC5 issuer is selected every failure is loud and is never converted into a Launchpad attempt. The fallback warning is kept for resource_discovery_failed, so production behavior is unchanged while BC5 is dark. - A selected BC5 issuer runs the device authorization grant as the pre-registered public client "basecamp-cli" (oauth_type "bc5", no client secret). The device display callback is the trust boundary for the server-controlled response: verification URIs are validated with the same policy as other OAuth browser URLs before launching, printed copies are sanitized of ANSI/OSC/control sequences, and malformed display data aborts polling via context cancellation. - The Launchpad path is unchanged: authorization-code with loopback callback or remote paste flow, legacy token format. - The DCR/PKCE development flow against BC3 is removed (client registration, client.json load/save, PKCE challenge). Stored legacy "bc3" credentials refresh to a clear re-authenticate error; auth status still displays them. A stale client.json is left on disk. - resourceOrigin strictly reduces the base URL to an origin: the path is stripped, everything else (userinfo, query, fragment, bad scheme, out-of-range port) is rejected without echoing the raw URL. - Flag copy is provider-neutral: --device-code and --scope now describe both paths truthfully. Test configs that constructed SDK clients with an empty BaseURL now set one: the bumped SDK refuses to attach credentials when the request target cannot be same-origin with the base URL.

Behavior tests for the resource-first discovery and device flow adopted in the previous commit, against an httptest resource-server/AS pair with an injectable device-poll sleep: - Device happy path: bc5 credentials with token endpoint and scope, public client_id on both device and token forms, browser launched with the validated code-embedding URI, code and URI logged. - Flag matrix at the auth layer: Remote and NoBrowser print without launching (even with an injected BrowserLauncher); the default launches. - Display trust boundary: invalid verification_uri_complete falls back to the plain URI and is never launched or shown; both URIs invalid or a user code that sanitizes to empty aborts with an API-class error before any token poll; OSC/CSI/CRLF sequences are stripped from logged copies while the raw validated URL goes to the launcher; browser-launch failure logs and continues. - Scope wiring: explicit scope reaches the device-authorization form, unset scope is omitted; effective scope is token scope, then the requested scope, then "read". - Soft fallbacks: missing resource metadata warns and falls back to Launchpad; valid metadata with no non-Launchpad issuer falls back quietly. - Hard failures never fall back: ambiguous issuers, AS metadata fetch failure, and issuer binding mismatch surface their sentinels with zero requests to a recording Launchpad server; a selected issuer without device capability surfaces DeviceFlowUnavailable. - Poll outcomes: access_denied surfaces its reason; one authorization_pending cycle then success; parent-context cancellation mid-poll still matches errors.Is(err, context.Canceled). - bc5 refresh sends client_id=basecamp-cli with no client_secret key in the standard grant_type=refresh_token format; legacy "bc3" credentials refresh to a re-authenticate error without any network request. - A command-level regression test proves --device-code selects the remote paste-callback flow (observable on the Launchpad path, where remote and local modes differ). - Poisoned discovery endpoints (userinfo forms) are rejected before any POST; resourceOrigin table tests cover the strict origin reduction — including bare-"?" ForceQuery forms — asserting the raw URL is never echoed in failures.

README: login now describes the device flow (automatic when the server supports it, Launchpad authorization-code otherwise), scope help notes Launchpad ignores it, and the config tree drops client.json — a leftover copy from the removed development registration flow is documented as safe to delete. SKILL.md: provider-neutral wording for --device-code and --scope, matching the flag help.

…ping - Trim the sanitized user code and displayed URI: a code that reduces to whitespace is as unusable as an empty one and must abort before polling, not be displayed and polled until expiry. - Range-check ports (1-65535) centrally in isSecureEndpointURL — url.Parse accepts https://host:70000/ without complaint, and such a URL must never be dialed, displayed, or launched. One rule now covers the token, device-authorization, authorization, refresh, and verification-URL consumers; poisoned and out-of-range-port token/device endpoints are rejected with zero POSTs. - Regression-test the profile create --device-code → Remote mapping (the auth login copy was covered; the duplicated profile copy was not).

The rebase onto main auto-merged go.sum with both the old device-flow snapshot pin (7207f742) and main's bb363c84 pin present. go mod tidy keeps only the go.mod pin. The SDK will be re-pinned to the resource-indicator branch head next; the build is red until then because the device-flow API is not on SDK main yet.

BC5 device logins as basecamp-cli (a trusted client) mint MULTI-ACCOUNT refresh tokens: the token response carries an RFC 8707 resource indicator (urn:bc:account:<id>), and bc3's refresh grant rejects a multi-account refresh without that echo (400 invalid_request, per Oauth::RefreshToken#resolve_target_account!). Without this, every device login died at its first token expiry. Credentials (internal/auth + the mirrored internal/sdk store) gain a resource field; device login persists token.Resource; refresh echoes the stored value on the form (sent only when set) and preserves it when the refresh response omits it, so rotated credentials keep the binding. An end-to-end test drives login → stored resource → forced refresh → echo on the form → binding surviving a resource-less rotation. Pins the SDK at oauth-resource-indicator head e583c994 (basecamp-sdk#478, which stacks on #376/#370 and carries the device flow + resource support); absorbed its API drift since the old 7207f742 snapshot (VerificationURIComplete and RegistrationEndpoint are now *string). The pin moves to #478's merged SHA before this lands.

Copilot

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

internal/auth/device_test.go:563

After making countingServer return a synchronized accessor, hits is a function; dereferencing it (*hits) won’t compile and also bypasses synchronization. Call the accessor instead.

	assert.Zero(t, *hits, "capability failure on a selected issuer must not fall back")

internal/auth/device_test.go:137

countingServer returns a *int that is later read without taking the mutex, which will trigger a data race under -race when the handler increments count. Return a small accessor closure (or an atomic counter) so reads are synchronized too.

// countingServer records how many requests it receives and 404s all of them.
func countingServer(t *testing.T) (*httptest.Server, *int) {
	t.Helper()
	var mu sync.Mutex
	count := 0

resourceOrigin now lowercases the host (and the localhost carve-out checks the lowered host) and strips explicit default ports, normalizing leading zeros — the SDK binds the RFC 9728 protected-resource identifier to this string code-point exact, so an equivalent spelling (HTTPS://3.BasecampAPI.com, https://3.basecampapi.com:443) previously mismatched the advertised identifier and silently soft-fell back to Launchpad. IPv6 brackets are restored around the lowered hostname. Also restacked onto main 31c4936, preserving #581's bounded headless keyring probe alongside the credential resource field, and tidied the go.sum lines the auto-merge duplicated.

Carries the full review round: exact-200 device token acceptance, hardened Retry-After parsing, FileTokenStore resource persistence, and the AuthManager empty/non-string resource classification. Provenance updated to match; the pin moves to basecamp-sdk#478's merged SHA before this lands.

Copilot

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

The converged basecamp-sdk#478 head, carrying the full review tail since 819c0b86: exact-200 + 4xx-gated protocol errors, status-first terminal classification, request timeouts clamped to the code lifetime and the shared 3600s ceiling, ASCII-only zero-pad-tolerant Retry-After parsing, cooperative-cancellation coverage around every request, FileTokenStore resource persistence, empty-device-endpoint capability guard, truncated error interpolation, and the AuthManager no-expiry refresh guard. The pin moves to #478's merged SHA before landing.

github-actions · 2026-07-28T11:47:48Z

⚠️ Potential breaking changes detected:

Modified description for the '--scope' flag to indicate that it is now ignored by Launchpad, which could affect user scripts relying on its previous functionality.
Changed description for the '--device-code' flag, clarifying its functionality and removing the mention of being an alias for '--remote', which may impact user understanding or scripts that rely on this description.

Review carefully before merging. Consider a major version bump.

Copilot

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 28, 2026 04:04

github-actions Bot added commands CLI command implementations sdk SDK wrapper and provenance tests Tests (unit and e2e) skills Agent skills auth OAuth authentication docs deps enhancement New feature or request labels Jul 28, 2026

Copilot started reviewing on behalf of jeremy July 28, 2026 04:04 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

cubic-dev-ai Bot reviewed Jul 28, 2026

View reviewed changes

Comment thread internal/auth/auth.go Outdated

jeremy added 6 commits July 27, 2026 22:27

jeremy force-pushed the oauth-device branch from b0959fc to 05cb94c Compare July 28, 2026 05:36

Copilot AI review requested due to automatic review settings July 28, 2026 05:36

Copilot started reviewing on behalf of jeremy July 28, 2026 05:36 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Comment thread internal/auth/device_test.go

jeremy added 2 commits July 27, 2026 23:10

jeremy force-pushed the oauth-device branch from 05cb94c to 6656378 Compare July 28, 2026 06:12

Copilot AI review requested due to automatic review settings July 28, 2026 06:12

Copilot started reviewing on behalf of jeremy July 28, 2026 06:13 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 28, 2026 11:47

github-actions Bot added the breaking Breaking change label Jul 28, 2026

Copilot started reviewing on behalf of jeremy July 28, 2026 11:47 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

BC5 OAuth: resource-first discovery, device-flow login, RFC 8707 resource echo - #582

BC5 OAuth: resource-first discovery, device-flow login, RFC 8707 resource echo#582
jeremy wants to merge 9 commits into
mainfrom
oauth-device

jeremy commented Jul 28, 2026 •

edited by cubic-dev-ai Bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

cubic-dev-ai Bot left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jeremy commented Jul 28, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by cubic

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jeremy commented Jul 28, 2026 •

edited by cubic-dev-ai Bot

Loading