Skip to content

fix(oauth): apply manual-token + requiresClientSecret + app-not-found checks uniformly (#30251 follow-up) #30255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
noanflaherty merged 1 commit into from
May 11, 2026
Merged

fix(oauth): apply manual-token + requiresClientSecret + app-not-found checks uniformly (#30251 follow-up) #30255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
185 changes: 174 additions & 11 deletions assistant/src/__tests__/oauth-connect-routes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,49 @@ mock.module("../util/logger.js", () => ({
}),
}));

// ── Mock oauth-store so handleOAuthConnectStart can run without a seeded DB ──
type MockProviderRow = {
authorizeUrl?: string;
requiresClientSecret?: boolean;
[key: string]: unknown;
};

const DEFAULT_PROVIDER: MockProviderRow = {
authorizeUrl: "https://accounts.google.com/o/oauth2/auth",
requiresClientSecret: false,
};

let mockGetProvider: (service: string) => MockProviderRow | undefined = () =>
DEFAULT_PROVIDER;
let mockGetAppByProviderAndClientId: (
service: string,
clientId: string,
) => Record<string, unknown> | undefined = (_service, clientId) => ({
id: "test-app-id",
provider: "google",
clientId,
});
let mockGetMostRecentAppByProvider: (
service: string,
) => Record<string, unknown> | undefined = () => ({
id: "test-app-id",
provider: "google",
clientId: "default-client-id",
});
let mockGetAppClientSecret: (
row: Record<string, unknown>,
) => Promise<string | undefined> = async () => undefined;

mock.module("../oauth/oauth-store.js", () => ({
getProvider: (service: string) => mockGetProvider(service),
getAppByProviderAndClientId: (service: string, clientId: string) =>
mockGetAppByProviderAndClientId(service, clientId),
getMostRecentAppByProvider: (service: string) =>
mockGetMostRecentAppByProvider(service),
getAppClientSecret: (row: Record<string, unknown>) =>
mockGetAppClientSecret(row),
}));

// NOTE: Do NOT mock oauth-connect-state — use the real module so we can
// verify state transitions via getOAuthConnectState.

Expand All @@ -65,6 +108,23 @@ function findRoute(operationId: string) {
// ── Tests ──────────────────────────────────────────────────────────────────────

describe("oauth-connect-routes", () => {
beforeEach(() => {
// Reset oauth-store mocks to permissive defaults so every test starts
// from a clean baseline regardless of what the previous test set.
mockGetProvider = () => DEFAULT_PROVIDER;
mockGetAppByProviderAndClientId = (_service, clientId) => ({
id: "test-app-id",
provider: "google",
clientId,
});
mockGetMostRecentAppByProvider = () => ({
id: "test-app-id",
provider: "google",
clientId: "default-client-id",
});
mockGetAppClientSecret = async () => undefined;
});

describe("POST internal/oauth/connect/start", () => {
beforeEach(() => {
capturedOnDeferredComplete = undefined;
Expand Down Expand Up @@ -109,17 +169,6 @@ describe("oauth-connect-routes", () => {
).rejects.toBeInstanceOf(BadRequestError);
});

test("missing clientId throws BadRequestError", async () => {
await expect(
findRoute("internal_oauth_connect_start").handler({
body: {
service: "google",
callbackTransport: "gateway",
},
}),
).rejects.toBeInstanceOf(BadRequestError);
});

test("missing service throws BadRequestError", async () => {
await expect(
findRoute("internal_oauth_connect_start").handler({
Expand Down Expand Up @@ -183,6 +232,120 @@ describe("oauth-connect-routes", () => {
}),
).rejects.toBeInstanceOf(InternalError);
});

test("unknown provider throws NotFoundError", async () => {
mockGetProvider = () => undefined;
await expect(
findRoute("internal_oauth_connect_start").handler({
body: {
service: "made-up-service",
clientId: "my-client-id",
callbackTransport: "gateway",
},
}),
).rejects.toBeInstanceOf(NotFoundError);
});

test("manual-token provider rejected even when clientId is explicitly supplied", async () => {
// Codex/Devin finding on PR #30251: the manual-token check used to live
// inside `if (!clientId)`, so callers passing `--client-id` for a
// manual-token provider (e.g. slack_channel, telegram) bypassed it.
mockGetProvider = () => ({
authorizeUrl: "urn:manual-token",
requiresClientSecret: false,
});
await expect(
findRoute("internal_oauth_connect_start").handler({
body: {
service: "slack_channel",
clientId: "any-client-id",
callbackTransport: "gateway",
},
}),
).rejects.toBeInstanceOf(BadRequestError);
});

test("manual-token provider rejected when no clientId is supplied", async () => {
mockGetProvider = () => ({
authorizeUrl: "urn:manual-token",
requiresClientSecret: false,
});
await expect(
findRoute("internal_oauth_connect_start").handler({
body: {
service: "slack_channel",
callbackTransport: "gateway",
},
}),
).rejects.toBeInstanceOf(BadRequestError);
});

test("explicit clientId with no registered app throws NotFoundError", async () => {
// Devin finding on PR #30251: the explicit-clientId branch used to
// silently continue when getAppByProviderAndClientId returned null,
// letting orchestrateOAuthConnect fail later with a less helpful error.
mockGetAppByProviderAndClientId = () => undefined;
await expect(
findRoute("internal_oauth_connect_start").handler({
body: {
service: "google",
clientId: "unregistered-client-id",
callbackTransport: "gateway",
},
}),
).rejects.toBeInstanceOf(NotFoundError);
});

test("no clientId and no registered app throws BadRequestError", async () => {
mockGetMostRecentAppByProvider = () => undefined;
await expect(
findRoute("internal_oauth_connect_start").handler({
body: {
service: "google",
callbackTransport: "gateway",
},
}),
).rejects.toBeInstanceOf(BadRequestError);
});

test("missing client_secret when provider requiresClientSecret throws BadRequestError", async () => {
// Devin finding on PR #30251: requiresClientSecret was only checked
// inside the `!clientId` branch. Hoisting it ensures explicit-clientId
// callers also fail fast with an actionable error.
mockGetProvider = () => ({
authorizeUrl: "https://accounts.example.com/o/oauth2/auth",
requiresClientSecret: true,
});
mockGetAppClientSecret = async () => undefined;
await expect(
findRoute("internal_oauth_connect_start").handler({
body: {
service: "github",
clientId: "explicit-client-id",
callbackTransport: "gateway",
},
}),
).rejects.toBeInstanceOf(BadRequestError);
});

test("explicit clientId + stored secret succeeds without caller-supplied secret", async () => {
mockGetProvider = () => ({
authorizeUrl: "https://accounts.example.com/o/oauth2/auth",
requiresClientSecret: true,
});
mockGetAppClientSecret = async () => "stored-secret-from-keyring";
const result = await findRoute("internal_oauth_connect_start").handler({
body: {
service: "github",
clientId: "explicit-client-id",
callbackTransport: "gateway",
},
});
expect(result).toMatchObject({
auth_url: "https://accounts.google.com/o/oauth2/auth?client_id=test",
state: "test-state-uuid-abc123",
});
});
});

describe("GET internal/oauth/connect/status/:state", () => {
Expand Down
2 changes: 1 addition & 1 deletion assistant/src/cli/commands/oauth/connect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ Examples:
const providerRow = providerCheck.result?.provider as
| Record<string, unknown>
| undefined;
const authorizeUrl = providerRow?.auth_url as string | undefined;
const authorizeUrl = providerRow?.authUrl as string | undefined;

// ---------------------------------------------------------------
// 2. Detect mode via IPC
Expand Down
11 changes: 0 additions & 11 deletions assistant/src/cli/commands/oauth/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,6 @@ import type { Command } from "commander";
import { VellumPlatformClient } from "../../../platform/client.js";
import { writeOutput } from "../../output.js";

// ---------------------------------------------------------------------------
// Shared types
// ---------------------------------------------------------------------------

export interface PlatformConnectionEntry {
id: string;
account_label?: string;
scopes_granted?: string[];
status?: string;
}

// ---------------------------------------------------------------------------
// Platform connection helpers
// ---------------------------------------------------------------------------
Expand Down
59 changes: 33 additions & 26 deletions assistant/src/runtime/routes/oauth-connect-routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,26 +47,28 @@ async function handleOAuthConnectStart({

if (!service) throw new BadRequestError("service is required");

// When clientId is not provided, resolve from the DB
// Provider row drives validation that applies regardless of whether the
// caller supplied an explicit clientId: existence, manual-token rejection,
// and the requiresClientSecret check below.
const providerRow = getProvider(service);
if (!providerRow) {
throw new NotFoundError(
`Unknown provider "${service}". Run 'assistant oauth providers list' to see available providers.`,
);
}

// Manual-token providers don't use OAuth2 browser flows.
if (providerRow.authorizeUrl === "urn:manual-token") {
throw new BadRequestError(
`"${service}" uses manual token configuration, not an OAuth browser flow. ` +
`Set the token with: assistant credentials set <token_value> --service ${service} --field <field_name>`,
);
}

let clientId = rawClientId;
let clientSecret = rawClientSecret;

if (!clientId) {
const providerRow = getProvider(service);
if (!providerRow) {
throw new NotFoundError(
`Unknown provider "${service}". Run 'assistant oauth providers list' to see available providers.`,
);
}

// Manual-token providers don't use OAuth2 browser flows
if (providerRow.authorizeUrl === "urn:manual-token") {
throw new BadRequestError(
`"${service}" uses manual token configuration, not an OAuth browser flow. ` +
`Set the token with: assistant credentials set <token_value> --service ${service} --field <field_name>`,
);
}

const dbApp = getMostRecentAppByProvider(service);
if (!dbApp) {
throw new BadRequestError(
Expand All @@ -79,23 +81,28 @@ async function handleOAuthConnectStart({
const storedSecret = await getAppClientSecret(dbApp);
if (storedSecret) clientSecret = storedSecret;
}

// Check if client_secret is required but missing
if (clientSecret === undefined && providerRow.requiresClientSecret) {
throw new BadRequestError(
`client_secret is required for ${service} but not found. ` +
`Store it with 'assistant oauth apps upsert --client-secret'.`,
} else {
// clientId was explicitly provided — resolve its app.
const dbApp = getAppByProviderAndClientId(service, clientId);
if (!dbApp) {
throw new NotFoundError(
`No registered app found for "${service}" with client_id "${clientId}". ` +
`Register one with 'assistant oauth apps upsert'.`,
);
}
} else if (clientId) {
// clientId was explicitly provided — resolve its app
const dbApp = getAppByProviderAndClientId(service, clientId);
if (dbApp && clientSecret === undefined) {
if (clientSecret === undefined) {
const storedSecret = await getAppClientSecret(dbApp);
if (storedSecret) clientSecret = storedSecret;
}
}

if (clientSecret === undefined && providerRow.requiresClientSecret) {
throw new BadRequestError(
`client_secret is required for ${service} but not found. ` +
`Store it with 'assistant oauth apps upsert --client-secret'.`,
);
}

if (callbackTransport !== "loopback" && callbackTransport !== "gateway") {
throw new BadRequestError(
'callbackTransport must be "loopback" or "gateway"',
Expand Down
Loading