Skip to content

Add ability to use hx creds if api key not provided#72

Open
AHarmlessPyro wants to merge 7 commits intomainfrom
use_hx_creds
Open

Add ability to use hx creds if api key not provided#72
AHarmlessPyro wants to merge 7 commits intomainfrom
use_hx_creds

Conversation

@AHarmlessPyro
Copy link
Copy Markdown
Contributor

@AHarmlessPyro AHarmlessPyro commented Apr 14, 2026


Open with Devin

Note

Medium Risk
Introduces a new control-plane auth layer with OAuth session loading/refresh and request replay, which changes how all SDK HTTP requests are authorized and retried; failures could impact all API calls if edge cases aren’t handled.

Overview
Adds OAuth (hx) credential support when an API key isn’t provided. The client now resolves auth via resolveControlPlaneConfig, using HYPERBROWSER_API_KEY when present or falling back to an on-disk OAuth session profile (with base URL normalization and new config/env knobs).

Centralizes request authorization and token refresh. A new ControlPlaneAuthManager wraps fetch, injects x-api-key or Authorization: Bearer, refreshes OAuth tokens via /oauth/token with a filesystem rotation lock, and retries once on 401 when the request body is replayable.

Refactors services to use the auth manager and supports replayable request init factories. BaseService now accepts an auth manager, normalizes headers, maps auth errors into HyperbrowserError, and upload endpoints (sessions/extensions) build FormData via factories to allow safe retries; package version bumps to 0.90.0.

Reviewed by Cursor Bugbot for commit 16b69ee. Bugbot is set up for automated code reviews on this repo. Configure here.

@AHarmlessPyro
Copy link
Copy Markdown
Contributor Author

@claude - Can you review this PR

  • Focus on the iterative changes are made
  • Code quality and best practices
  • Potential bugs or issues
  • Performance considerations
  • Security concerns
  • Use the repository's CLAUDE.md for guidance on style and conventions. Be constructive and helpful in your feedback.

Look thoroughly, and see if these changes would break any existing functionality or any places you see something off. Please look thoroughly. Think really carefully about if you think something would potentially break.

Focus only on the changes made in this PR. If you see any other issues, mention them at the bottom as a one-liner. Otherwise, don't mention things beyond the changes made here and if these changes specifically lead to some sort of error/undefined/unusual behavior elsewhere

Think carefully about a suitable deployment order for this too. The services present here are two separate server instances, a single frontend service, a single instance of the telemetry service, and then multiple regional services for session-proxy and runners. It's hard to deploy all the services at once, so for example, the regional services can only be deployed one-by-one; same for the two server instances. Additionally, there's two other semi-independent services, the polling-jobs, and scheduled-jobs.

Consider how we could deploy things here properly in a phased manner without downtime.
Keeping the different api changes in mind, what's the appropriate path ? Inspect all changes made to the endpoints, and make sure that they're backwards/forwards compatible as needed for the phased deployment.

Additionally, if there's a previous review look carefully at that and how this code compares to the issues introduced in that review

Use gh pr comment with your Bash tool to leave your review as a comment on the PR.

cursor[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Comment thread src/control-auth.ts Outdated

function resolveOAuthTokenUrl(baseUrl: string): string {
return new URL("/oauth/token", `${baseUrl}/`).toString();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OAuth token URL drops path prefix from custom base URL

Medium Severity

resolveOAuthTokenUrl uses new URL("/oauth/token", ...) with a leading /, which causes the standard URL resolution algorithm to treat the path as absolute from the origin — completely discarding any path prefix in baseUrl. For a custom HYPERBROWSER_BASE_URL like https://proxy.example.com/hb, the resulting token URL would be https://proxy.example.com/oauth/token instead of https://proxy.example.com/hb/oauth/token. OAuth token refresh would silently fail for any user with a path-prefixed base URL. The relative URL should be "oauth/token" (no leading slash) to resolve correctly against the base.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bfb57e3. Configure here.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 16b69ee. Configure here.

normalizeText(typeof payload.token_type === "string" ? payload.token_type : "") ||
normalizeText(previous.token_type) ||
"Bearer";
const expiresAt = deriveOAuthExpiry(payload, "expires_in") || normalizeText(previous.expiry);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale expiry fallback causes repeated token refreshes

Medium Severity

When the OAuth refresh response doesn't include expires_in, deriveOAuthExpiry returns undefined and buildRefreshedOAuthSession falls back to normalizeText(previous.expiry) — the old session's expiry timestamp. Since the previous token was near-expired or already expired (that's why the refresh was triggered), the newly written session inherits this stale expiry. On the next API call, isAccessTokenUsable sees the stale expiry and considers the token unusable, triggering another refresh. This repeats on every request, causing an unnecessary token refresh for each API call.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 16b69ee. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant