mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-10-07 14:58:39 +08:00
* feat: Add OAuth 2.0 Device Flow authentication for OpenHands Cloud backends Implements one-click login for cloud backends using the OAuth 2.0 Device Authorization Grant (RFC 8628). When adding a cloud backend with a known OpenHands Cloud host (*.all-hands.dev, *.openhands.dev), users can click 'Login with OpenHands' to authenticate via browser instead of manually copying their API key. Changes: - Add device-flow-client.ts with startDeviceFlow() and pollForToken() - Add useDeviceFlow React hook for managing auth state in components - Add DeviceFlowAuth component with auth UI states (idle, starting, awaiting_authorization, success, error) - Update BackendForm to show device flow auth for cloud backends - Add i18n translations for all device flow UI strings Closes #379 * feat: Show device flow login for all cloud backends with clearer UI - Remove restriction to only known OpenHands Cloud hosts - device flow is now available for all cloud backends (including self-hosted) - Add clear 'OR' divider between login button and manual API key entry - Add link to API key documentation for manual key generation - Add new i18n keys: LOGIN_OR, KEY_DOCS_HINT, KEY_DOCS_LINK * feat: Always show login button for cloud backends, disable when no host - Login button, OR divider, and manual API key input are now always visible when cloud backend type is selected - Login button is disabled until a valid host URL is entered - Improves UX by showing the full auth options upfront * fix: Keep login button visible when typing custom cloud host URL The kind inference was incorrectly downgrading from 'cloud' to 'local' when typing a host URL that didn't match known OpenHands Cloud patterns. Now the inference only upgrades to 'cloud' when a known pattern is detected, but never downgrades - allowing users to type any custom cloud host URL while keeping the login button visible. * fix: Use cloud proxy for device flow to avoid CORS issues For known OpenHands Cloud hosts (*.all-hands.dev, *.openhands.dev), device flow requests are now routed through the local agent-server's cloud-proxy endpoint. This avoids CORS errors when the browser tries to make direct cross-origin requests to the cloud backend. Self-hosted instances still use direct requests, assuming they have CORS properly configured. * fix: Always use proxy for device flow and fix kind inference regression 1. Device flow now always uses proxy for all hosts (not just known cloud hosts). This avoids CORS issues for any custom backend that supports device flow. 2. Fix regression where typing a local address (e.g., 127.0.0.1) would not switch from cloud to local type. The kind inference now: - Auto-infers kind from host in add mode (initial behavior) - Only prevents downgrade when user explicitly clicked the cloud radio button (not when cloud is just the default) - Tracks explicit user selection separately from initial default * fix: Address PR review feedback for device flow security and RFC compliance Security fixes: - Fix isOpenHandsCloudHost() to use URL hostname extraction instead of substring matching, preventing attacks like all-hands.dev.evil.com - Add URL validation in handleStartAuth to check for credential injection - Sanitize error messages to avoid exposing server error details RFC 8628 compliance: - Add required grant_type parameter to token requests - Make verification_uri_complete optional per RFC Section 3.2 - Build verification_uri_complete if not provided by server Robustness improvements: - Validate polling interval to at least 1 second - Cap slow_down interval to MAX_INTERVAL_MS to prevent DoS - Open popup on user click to avoid popup blockers Accessibility: - Add role='status' and aria-live='polite' to status containers - Add role='alert' to error container * fix: Pass abort signal to makeProxiedRequest fetch call Address review feedback: the abort signal is now properly passed through makeProxiedRequest to the underlying fetch call, allowing in-flight proxied requests to be cancelled immediately when the user cancels. Co-authored-by: openhands <openhands@all-hands.dev> * fix: Address comprehensive review feedback for device flow auth Security fixes: - Add URL validation to prevent XSS via javascript: URLs - Validate verification URLs have https: protocol before use in popup and links - Add type validation for slow_down interval to prevent NaN tight loops RFC 8628 compliance: - Fix slow_down to increment by 5 seconds per Section 3.5 (not double) - Validate interval is number, finite, and positive before using server value Robustness: - Network errors now continue polling instead of failing immediately - Wrap sleep in try-catch for consistent abort handling - Add cleanup effect to close popup on unmount Code cleanup: - Remove dead userSelectedCloud state (was unreachable) - Add defensive programming comment for cancellation check - Fix onSuccess effect to include deviceFlow.reset in deps Tests: - Add DoS protection test (caps interval at 30s) - Add type confusion test (rejects non-numeric interval) - Add RFC 8628 +5s increment test - Add network error retry test - Add unmount cleanup test Co-authored-by: openhands <openhands@all-hands.dev> * chore: Remove PR-only artifacts * fix: Remove noopener from popup to maintain window reference The 'noopener' option causes window.open() to return null, which means we lose the reference to the popup and can't update its location when the verification URL becomes available. This was causing a blank page to appear instead of the device flow auth page. Removed 'noopener' from the initial popup open call so we can maintain the reference and update popupRef.current.location.href when the verification URL arrives from the device flow. Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: openhands <openhands@all-hands.dev> Co-authored-by: allhands-bot <allhands-bot@users.noreply.github.com>
507 lines
14 KiB
TypeScript
507 lines
14 KiB
TypeScript
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
|
import {
|
|
startDeviceFlow,
|
|
pollForToken,
|
|
isOpenHandsCloudHost,
|
|
DeviceFlowError,
|
|
} from "../../src/api/device-flow-client";
|
|
|
|
// Mock the backend registry to avoid import issues
|
|
vi.mock("../../src/api/backend-registry/active-store", () => ({
|
|
getEffectiveLocalBackend: () => ({
|
|
host: "http://localhost:18000",
|
|
apiKey: "test-api-key",
|
|
}),
|
|
}));
|
|
|
|
vi.mock("../../src/api/backend-registry/auth", () => ({
|
|
buildAuthHeaders: () => ({ "X-Session-API-Key": "test-api-key" }),
|
|
}));
|
|
|
|
const TEST_HOST_URL = "https://app.all-hands.dev";
|
|
|
|
describe("device-flow-client", () => {
|
|
beforeEach(() => {
|
|
vi.useFakeTimers();
|
|
});
|
|
|
|
afterEach(() => {
|
|
vi.useRealTimers();
|
|
vi.restoreAllMocks();
|
|
});
|
|
|
|
describe("isOpenHandsCloudHost", () => {
|
|
it("returns true for all-hands.dev domains", () => {
|
|
expect(isOpenHandsCloudHost("https://app.all-hands.dev")).toBe(true);
|
|
expect(isOpenHandsCloudHost("https://staging.all-hands.dev")).toBe(true);
|
|
expect(isOpenHandsCloudHost("app.all-hands.dev")).toBe(true);
|
|
expect(isOpenHandsCloudHost("ALL-HANDS.DEV")).toBe(true);
|
|
expect(isOpenHandsCloudHost("all-hands.dev")).toBe(true);
|
|
});
|
|
|
|
it("returns true for openhands.dev domains", () => {
|
|
expect(isOpenHandsCloudHost("https://app.openhands.dev")).toBe(true);
|
|
expect(isOpenHandsCloudHost("openhands.dev")).toBe(true);
|
|
});
|
|
|
|
it("returns false for other domains", () => {
|
|
expect(isOpenHandsCloudHost("https://localhost:8000")).toBe(false);
|
|
expect(isOpenHandsCloudHost("http://127.0.0.1")).toBe(false);
|
|
expect(isOpenHandsCloudHost("https://example.com")).toBe(false);
|
|
expect(isOpenHandsCloudHost("https://my-openhands-server.com")).toBe(
|
|
false,
|
|
);
|
|
});
|
|
|
|
it("prevents substring matching attacks", () => {
|
|
// These should NOT be treated as trusted hosts
|
|
expect(isOpenHandsCloudHost("https://all-hands.dev.evil.com")).toBe(
|
|
false,
|
|
);
|
|
expect(isOpenHandsCloudHost("https://malicious-all-hands.dev")).toBe(
|
|
false,
|
|
);
|
|
expect(isOpenHandsCloudHost("https://evil.com/all-hands.dev")).toBe(
|
|
false,
|
|
);
|
|
});
|
|
|
|
it("returns false for invalid URLs", () => {
|
|
expect(isOpenHandsCloudHost("")).toBe(false);
|
|
expect(isOpenHandsCloudHost("not-a-url")).toBe(false);
|
|
});
|
|
});
|
|
|
|
describe("startDeviceFlow", () => {
|
|
it("returns device authorization response on success", async () => {
|
|
const mockResponse = {
|
|
device_code: "device123",
|
|
user_code: "USER-1234",
|
|
verification_uri: `${TEST_HOST_URL}/device`,
|
|
verification_uri_complete: `${TEST_HOST_URL}/device?user_code=USER-1234`,
|
|
expires_in: 600,
|
|
interval: 5,
|
|
};
|
|
|
|
global.fetch = vi.fn().mockResolvedValue({
|
|
ok: true,
|
|
json: () => Promise.resolve(mockResponse),
|
|
});
|
|
|
|
const result = await startDeviceFlow(TEST_HOST_URL);
|
|
|
|
expect(result).toEqual(mockResponse);
|
|
// Should call the local proxy endpoint
|
|
expect(fetch).toHaveBeenCalledWith(
|
|
"http://localhost:18000/api/cloud-proxy",
|
|
expect.objectContaining({
|
|
method: "POST",
|
|
headers: expect.objectContaining({
|
|
"Content-Type": "application/json",
|
|
"X-Session-API-Key": "test-api-key",
|
|
}),
|
|
}),
|
|
);
|
|
});
|
|
|
|
it("normalizes host URL by removing trailing slashes", async () => {
|
|
global.fetch = vi.fn().mockResolvedValue({
|
|
ok: true,
|
|
json: () =>
|
|
Promise.resolve({
|
|
device_code: "dc",
|
|
user_code: "uc",
|
|
verification_uri: "v",
|
|
verification_uri_complete: "vc",
|
|
expires_in: 600,
|
|
interval: 5,
|
|
}),
|
|
});
|
|
|
|
await startDeviceFlow(`${TEST_HOST_URL}///`);
|
|
|
|
// Verify the proxy body contains the normalized host
|
|
const fetchCall = (fetch as ReturnType<typeof vi.fn>).mock.calls[0];
|
|
const body = JSON.parse(fetchCall[1].body);
|
|
expect(body.host).toBe(TEST_HOST_URL);
|
|
});
|
|
|
|
it("throws DeviceFlowError on HTTP error", async () => {
|
|
global.fetch = vi.fn().mockResolvedValue({
|
|
ok: false,
|
|
status: 500,
|
|
text: () => Promise.resolve("Internal Server Error"),
|
|
});
|
|
|
|
await expect(startDeviceFlow(TEST_HOST_URL)).rejects.toThrow(
|
|
DeviceFlowError,
|
|
);
|
|
await expect(startDeviceFlow(TEST_HOST_URL)).rejects.toThrow(
|
|
/Failed to start device flow.*500/,
|
|
);
|
|
});
|
|
|
|
it("throws DeviceFlowError on missing required fields", async () => {
|
|
global.fetch = vi.fn().mockResolvedValue({
|
|
ok: true,
|
|
json: () =>
|
|
Promise.resolve({
|
|
device_code: "dc",
|
|
// Missing other required fields
|
|
}),
|
|
});
|
|
|
|
await expect(startDeviceFlow(TEST_HOST_URL)).rejects.toThrow(
|
|
DeviceFlowError,
|
|
);
|
|
await expect(startDeviceFlow(TEST_HOST_URL)).rejects.toThrow(
|
|
/missing required fields/,
|
|
);
|
|
});
|
|
|
|
it("throws DeviceFlowError on network error", async () => {
|
|
global.fetch = vi.fn().mockRejectedValue(new Error("Network failed"));
|
|
|
|
await expect(startDeviceFlow(TEST_HOST_URL)).rejects.toThrow(
|
|
DeviceFlowError,
|
|
);
|
|
await expect(startDeviceFlow(TEST_HOST_URL)).rejects.toThrow(
|
|
/Network failed/,
|
|
);
|
|
});
|
|
});
|
|
|
|
describe("pollForToken", () => {
|
|
it("returns token response on immediate success", async () => {
|
|
const mockTokenResponse = {
|
|
access_token: "api-key-123",
|
|
token_type: "Bearer",
|
|
};
|
|
|
|
global.fetch = vi.fn().mockResolvedValue({
|
|
ok: true,
|
|
status: 200,
|
|
json: () => Promise.resolve(mockTokenResponse),
|
|
});
|
|
|
|
const result = await pollForToken(TEST_HOST_URL, "device123", {
|
|
interval: 5,
|
|
});
|
|
|
|
expect(result).toEqual(mockTokenResponse);
|
|
// Should call the local proxy endpoint
|
|
expect(fetch).toHaveBeenCalledWith(
|
|
"http://localhost:18000/api/cloud-proxy",
|
|
expect.objectContaining({
|
|
method: "POST",
|
|
headers: expect.objectContaining({
|
|
"Content-Type": "application/json",
|
|
"X-Session-API-Key": "test-api-key",
|
|
}),
|
|
}),
|
|
);
|
|
});
|
|
|
|
it("polls until authorization is complete", async () => {
|
|
const pendingResponse = {
|
|
ok: false,
|
|
status: 400,
|
|
json: () =>
|
|
Promise.resolve({
|
|
error: "authorization_pending",
|
|
error_description: "User hasn't authorized yet",
|
|
}),
|
|
};
|
|
const successResponse = {
|
|
ok: true,
|
|
status: 200,
|
|
json: () =>
|
|
Promise.resolve({
|
|
access_token: "api-key-123",
|
|
token_type: "Bearer",
|
|
}),
|
|
};
|
|
|
|
global.fetch = vi
|
|
.fn()
|
|
.mockResolvedValueOnce(pendingResponse)
|
|
.mockResolvedValueOnce(successResponse);
|
|
|
|
const pollPromise = pollForToken(TEST_HOST_URL, "device123", {
|
|
interval: 1,
|
|
});
|
|
|
|
// Advance past the first poll interval
|
|
await vi.advanceTimersByTimeAsync(1000);
|
|
|
|
const result = await pollPromise;
|
|
expect(result.access_token).toBe("api-key-123");
|
|
expect(fetch).toHaveBeenCalledTimes(2);
|
|
});
|
|
|
|
it("increases interval on slow_down error", async () => {
|
|
const slowDownResponse = {
|
|
ok: false,
|
|
status: 400,
|
|
json: () =>
|
|
Promise.resolve({
|
|
error: "slow_down",
|
|
interval: 10,
|
|
}),
|
|
};
|
|
const successResponse = {
|
|
ok: true,
|
|
status: 200,
|
|
json: () =>
|
|
Promise.resolve({
|
|
access_token: "api-key-123",
|
|
token_type: "Bearer",
|
|
}),
|
|
};
|
|
|
|
global.fetch = vi
|
|
.fn()
|
|
.mockResolvedValueOnce(slowDownResponse)
|
|
.mockResolvedValueOnce(successResponse);
|
|
|
|
const pollPromise = pollForToken(TEST_HOST_URL, "device123", {
|
|
interval: 5,
|
|
});
|
|
|
|
// Advance by new interval (10 seconds)
|
|
await vi.advanceTimersByTimeAsync(10000);
|
|
|
|
const result = await pollPromise;
|
|
expect(result.access_token).toBe("api-key-123");
|
|
});
|
|
|
|
it("throws on expired_token error", async () => {
|
|
global.fetch = vi.fn().mockResolvedValue({
|
|
ok: false,
|
|
status: 400,
|
|
json: () =>
|
|
Promise.resolve({
|
|
error: "expired_token",
|
|
}),
|
|
});
|
|
|
|
await expect(
|
|
pollForToken(TEST_HOST_URL, "device123", { interval: 1 }),
|
|
).rejects.toThrow(DeviceFlowError);
|
|
await expect(
|
|
pollForToken(TEST_HOST_URL, "device123", { interval: 1 }),
|
|
).rejects.toThrow(/expired/i);
|
|
});
|
|
|
|
it("throws on access_denied error", async () => {
|
|
global.fetch = vi.fn().mockResolvedValue({
|
|
ok: false,
|
|
status: 400,
|
|
json: () =>
|
|
Promise.resolve({
|
|
error: "access_denied",
|
|
}),
|
|
});
|
|
|
|
await expect(
|
|
pollForToken(TEST_HOST_URL, "device123", { interval: 1 }),
|
|
).rejects.toThrow(DeviceFlowError);
|
|
await expect(
|
|
pollForToken(TEST_HOST_URL, "device123", { interval: 1 }),
|
|
).rejects.toThrow(/denied/i);
|
|
});
|
|
|
|
it("respects abort signal", async () => {
|
|
vi.useRealTimers(); // Use real timers for this test
|
|
const controller = new AbortController();
|
|
|
|
global.fetch = vi.fn().mockResolvedValue({
|
|
ok: false,
|
|
status: 400,
|
|
json: () =>
|
|
Promise.resolve({
|
|
error: "authorization_pending",
|
|
}),
|
|
});
|
|
|
|
// Pre-abort the controller
|
|
controller.abort();
|
|
|
|
// Now the promise should reject immediately with cancelled
|
|
await expect(
|
|
pollForToken(TEST_HOST_URL, "device123", {
|
|
interval: 1,
|
|
signal: controller.signal,
|
|
}),
|
|
).rejects.toThrow(/cancelled/i);
|
|
});
|
|
|
|
it("times out after specified duration", async () => {
|
|
vi.useRealTimers(); // Use real timers for this test
|
|
global.fetch = vi.fn().mockResolvedValue({
|
|
ok: false,
|
|
status: 400,
|
|
json: () =>
|
|
Promise.resolve({
|
|
error: "authorization_pending",
|
|
}),
|
|
});
|
|
|
|
// Use very short timeout
|
|
await expect(
|
|
pollForToken(TEST_HOST_URL, "device123", {
|
|
interval: 0.01, // 10ms interval
|
|
timeout: 50, // 50ms timeout
|
|
}),
|
|
).rejects.toThrow(/timeout/i);
|
|
}, 10000);
|
|
|
|
it("caps slow_down interval at 30 seconds (DoS protection)", async () => {
|
|
const slowDownResponse = {
|
|
ok: false,
|
|
status: 400,
|
|
json: () =>
|
|
Promise.resolve({
|
|
error: "slow_down",
|
|
interval: 999999, // Malicious server tries to DoS
|
|
}),
|
|
};
|
|
const successResponse = {
|
|
ok: true,
|
|
status: 200,
|
|
json: () =>
|
|
Promise.resolve({
|
|
access_token: "api-key-123",
|
|
token_type: "Bearer",
|
|
}),
|
|
};
|
|
|
|
global.fetch = vi
|
|
.fn()
|
|
.mockResolvedValueOnce(slowDownResponse)
|
|
.mockResolvedValueOnce(successResponse);
|
|
|
|
const pollPromise = pollForToken(TEST_HOST_URL, "device123", {
|
|
interval: 5,
|
|
});
|
|
|
|
// Should use 30s max, not 999999s
|
|
await vi.advanceTimersByTimeAsync(30000);
|
|
|
|
const result = await pollPromise;
|
|
expect(result.access_token).toBe("api-key-123");
|
|
expect(fetch).toHaveBeenCalledTimes(2);
|
|
});
|
|
|
|
it("rejects non-numeric slow_down interval (type confusion protection)", async () => {
|
|
const slowDownResponse = {
|
|
ok: false,
|
|
status: 400,
|
|
json: () =>
|
|
Promise.resolve({
|
|
error: "slow_down",
|
|
interval: "pwned", // Non-numeric value
|
|
}),
|
|
};
|
|
const successResponse = {
|
|
ok: true,
|
|
status: 200,
|
|
json: () =>
|
|
Promise.resolve({
|
|
access_token: "api-key-123",
|
|
token_type: "Bearer",
|
|
}),
|
|
};
|
|
|
|
global.fetch = vi
|
|
.fn()
|
|
.mockResolvedValueOnce(slowDownResponse)
|
|
.mockResolvedValueOnce(successResponse);
|
|
|
|
const pollPromise = pollForToken(TEST_HOST_URL, "device123", {
|
|
interval: 5,
|
|
});
|
|
|
|
// With invalid interval, should use RFC 8628 default: current + 5s
|
|
// Starting interval is 5s, so next should be 10s (5000 + 5000 = 10000ms)
|
|
await vi.advanceTimersByTimeAsync(10000);
|
|
|
|
const result = await pollPromise;
|
|
expect(result.access_token).toBe("api-key-123");
|
|
});
|
|
|
|
it("increments interval by 5 seconds per RFC 8628 when slow_down has no interval", async () => {
|
|
const slowDownResponse = {
|
|
ok: false,
|
|
status: 400,
|
|
json: () =>
|
|
Promise.resolve({
|
|
error: "slow_down",
|
|
// No interval field - RFC 8628 mandates +5s increment
|
|
}),
|
|
};
|
|
const successResponse = {
|
|
ok: true,
|
|
status: 200,
|
|
json: () =>
|
|
Promise.resolve({
|
|
access_token: "api-key-123",
|
|
token_type: "Bearer",
|
|
}),
|
|
};
|
|
|
|
global.fetch = vi
|
|
.fn()
|
|
.mockResolvedValueOnce(slowDownResponse)
|
|
.mockResolvedValueOnce(successResponse);
|
|
|
|
const pollPromise = pollForToken(TEST_HOST_URL, "device123", {
|
|
interval: 5, // 5 seconds initial
|
|
});
|
|
|
|
// RFC 8628: must increment by 5 seconds, so 5s -> 10s
|
|
await vi.advanceTimersByTimeAsync(10000);
|
|
|
|
const result = await pollPromise;
|
|
expect(result.access_token).toBe("api-key-123");
|
|
});
|
|
|
|
it("continues polling on network errors instead of failing immediately", async () => {
|
|
const networkError = new Error("Network failed");
|
|
const successResponse = {
|
|
ok: true,
|
|
status: 200,
|
|
json: () =>
|
|
Promise.resolve({
|
|
access_token: "api-key-123",
|
|
token_type: "Bearer",
|
|
}),
|
|
};
|
|
|
|
// First call fails with network error, second succeeds
|
|
global.fetch = vi
|
|
.fn()
|
|
.mockRejectedValueOnce(networkError)
|
|
.mockResolvedValueOnce(successResponse);
|
|
|
|
const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
|
|
|
const pollPromise = pollForToken(TEST_HOST_URL, "device123", {
|
|
interval: 1,
|
|
});
|
|
|
|
// Advance past the retry interval
|
|
await vi.advanceTimersByTimeAsync(1000);
|
|
|
|
const result = await pollPromise;
|
|
expect(result.access_token).toBe("api-key-123");
|
|
expect(consoleSpy).toHaveBeenCalledWith(
|
|
"Network error during polling, retrying:",
|
|
networkError,
|
|
);
|
|
|
|
consoleSpy.mockRestore();
|
|
});
|
|
});
|
|
});
|