fix: guard against null provider in useUrlSearch hook (#341)

* fix: guard against null provider in useUrlSearch hook

Prevent unnecessary cloud proxy requests when the provider is null/undefined
(e.g., before providers have loaded from settings).

The useUrlSearch hook was calling GitService.searchGitRepositories()
without validating that the provider was truthy first. When the parent
component passed undefined (from providers[0] when array is empty),
the request would be sent to the cloud proxy with an invalid provider.

Changes:
- Update type signature to accept Provider | null | undefined
- Add early return guard when provider is falsy
- Clear results when provider becomes null

* fix: add defensive guards in GitService for invalid providers

Add a second layer of defense at the GitService level to prevent
cloud proxy requests with invalid providers (null, undefined, empty
string, or stringified 'undefined'/'null').

This fixes the installations search API being called with
'provider=undefined' even when hooks have enabled guards.

Changes:
- Add isInvalidProvider() guard function
- Add guards to all GitService methods that take a provider param
- Return empty results instead of making invalid API requests
- Add comprehensive tests for the guards

---------

Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
Rohit Malhotra
2026-05-11 17:14:33 -04:00
committed by GitHub
co-authored by openhands
parent f4dbfcdf8f
commit 24da834da3
4 changed files with 546 additions and 2 deletions
+260
View File
@@ -0,0 +1,260 @@
import { describe, expect, vi, beforeEach, it, afterEach } from "vitest";
import GitService from "#/api/git-service/git-service.api";
import * as cloudGitService from "#/api/cloud/git-service.api";
import { ProviderHandler } from "#/api/git-providers/provider-handler";
import * as activeStore from "#/api/backend-registry/active-store";
// Mock the cloud git service
vi.mock("#/api/cloud/git-service.api", () => ({
searchCloudRepositories: vi.fn(),
getCloudInstallations: vi.fn(),
getCloudRepositoryBranches: vi.fn(),
}));
// Mock the provider handler
vi.mock("#/api/git-providers/provider-handler", () => ({
ProviderHandler: {
searchRepositories: vi.fn(),
getInstallations: vi.fn(),
getBranches: vi.fn(),
},
}));
// Mock the active backend store
vi.mock("#/api/backend-registry/active-store", () => ({
getActiveBackend: vi.fn(),
}));
const mockSearchCloudRepositories = vi.mocked(
cloudGitService.searchCloudRepositories,
);
const mockGetCloudInstallations = vi.mocked(
cloudGitService.getCloudInstallations,
);
const mockGetCloudRepositoryBranches = vi.mocked(
cloudGitService.getCloudRepositoryBranches,
);
const mockProviderSearchRepositories = vi.mocked(
ProviderHandler.searchRepositories,
);
const mockProviderGetInstallations = vi.mocked(
ProviderHandler.getInstallations,
);
const mockProviderGetBranches = vi.mocked(ProviderHandler.getBranches);
const mockGetActiveBackend = vi.mocked(activeStore.getActiveBackend);
describe("GitService", () => {
beforeEach(() => {
vi.clearAllMocks();
});
afterEach(() => {
vi.clearAllMocks();
});
describe("invalid provider guards", () => {
const invalidProviders = [
{ value: null, name: "null" },
{ value: undefined, name: "undefined" },
{ value: "", name: "empty string" },
{ value: "undefined", name: '"undefined" string' },
{ value: "null", name: '"null" string' },
];
describe("searchGitRepositories", () => {
it.each(invalidProviders)(
"should return empty results when provider is $name (cloud mode)",
async ({ value }) => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "cloud", id: "test", name: "Test", host: "https://example.com", apiKey: "test-key" },
orgId: "org-1",
});
const result = await GitService.searchGitRepositories(
"test query",
value as string,
);
expect(result).toEqual({ items: [], next_page_id: null });
expect(mockSearchCloudRepositories).not.toHaveBeenCalled();
},
);
it.each(invalidProviders)(
"should return empty results when provider is $name (local mode)",
async ({ value }) => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "local", id: "test", name: "Test", host: "http://localhost", apiKey: "test-key" },
orgId: null,
});
const result = await GitService.searchGitRepositories(
"test query",
value as string,
);
expect(result).toEqual({ items: [], next_page_id: null });
expect(mockProviderSearchRepositories).not.toHaveBeenCalled();
},
);
});
describe("retrieveUserGitRepositories", () => {
it.each(invalidProviders)(
"should return empty results when provider is $name",
async ({ value }) => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "cloud", id: "test", name: "Test", host: "https://example.com", apiKey: "test-key" },
orgId: "org-1",
});
const result = await GitService.retrieveUserGitRepositories(
value as string,
);
expect(result).toEqual({ items: [], next_page_id: null });
expect(mockSearchCloudRepositories).not.toHaveBeenCalled();
},
);
});
describe("retrieveInstallationRepositories", () => {
it.each(invalidProviders)(
"should return empty results when provider is $name",
async ({ value }) => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "cloud", id: "test", name: "Test", host: "https://example.com", apiKey: "test-key" },
orgId: "org-1",
});
const result = await GitService.retrieveInstallationRepositories(
value as string,
0,
["installation-1"],
);
expect(result).toEqual({ items: [], next_page_id: null });
expect(mockSearchCloudRepositories).not.toHaveBeenCalled();
},
);
});
describe("getUserInstallations", () => {
it.each(invalidProviders)(
"should return empty results when provider is $name (cloud mode)",
async ({ value }) => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "cloud", id: "test", name: "Test", host: "https://example.com", apiKey: "test-key" },
orgId: "org-1",
});
const result = await GitService.getUserInstallations(value as string);
expect(result).toEqual({ items: [], next_page_id: null });
expect(mockGetCloudInstallations).not.toHaveBeenCalled();
},
);
it.each(invalidProviders)(
"should return empty results when provider is $name (local mode)",
async ({ value }) => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "local", id: "test", name: "Test", host: "http://localhost", apiKey: "test-key" },
orgId: null,
});
const result = await GitService.getUserInstallations(value as string);
expect(result).toEqual({ items: [], next_page_id: null });
expect(mockProviderGetInstallations).not.toHaveBeenCalled();
},
);
});
describe("getRepositoryBranches", () => {
it.each(invalidProviders)(
"should return empty results when provider is $name",
async ({ value }) => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "cloud", id: "test", name: "Test", host: "https://example.com", apiKey: "test-key" },
orgId: "org-1",
});
const result = await GitService.getRepositoryBranches(
"owner/repo",
value as string,
);
expect(result).toEqual({ items: [], next_page_id: null });
expect(mockGetCloudRepositoryBranches).not.toHaveBeenCalled();
},
);
});
describe("searchRepositoryBranches", () => {
it.each(invalidProviders)(
"should return empty results when provider is $name",
async ({ value }) => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "cloud", id: "test", name: "Test", host: "https://example.com", apiKey: "test-key" },
orgId: "org-1",
});
const result = await GitService.searchRepositoryBranches(
"owner/repo",
value as string,
"main",
);
expect(result).toEqual({ items: [], next_page_id: null });
expect(mockGetCloudRepositoryBranches).not.toHaveBeenCalled();
},
);
});
});
describe("valid provider behavior", () => {
it("should call cloud API when provider is valid and cloud is active", async () => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "cloud", id: "test", name: "Test", host: "https://example.com", apiKey: "test-key" },
orgId: "org-1",
});
mockSearchCloudRepositories.mockResolvedValue({
items: [{ id: "1", full_name: "owner/repo", git_provider: "github", is_public: true }],
next_page_id: null,
});
const result = await GitService.searchGitRepositories("test", "github");
expect(mockSearchCloudRepositories).toHaveBeenCalledWith({
provider: "github",
query: "test",
limit: 100,
pageId: undefined,
installationId: undefined,
});
expect(result.items).toHaveLength(1);
});
it("should call provider handler when provider is valid and local is active", async () => {
mockGetActiveBackend.mockReturnValue({
backend: { kind: "local", id: "test", name: "Test", host: "http://localhost", apiKey: "test-key" },
orgId: null,
});
mockProviderSearchRepositories.mockResolvedValue({
items: [{ id: "1", full_name: "owner/repo", git_provider: "github", is_public: true }],
next_page_id: null,
});
const result = await GitService.searchGitRepositories("test", "github");
expect(mockProviderSearchRepositories).toHaveBeenCalledWith("github", {
query: "test",
installationId: undefined,
pageId: undefined,
limit: 100,
});
expect(result.items).toHaveLength(1);
});
});
});
@@ -0,0 +1,241 @@
import { renderHook, waitFor, act } from "@testing-library/react";
import { describe, expect, vi, beforeEach, it, afterEach } from "vitest";
import { useUrlSearch } from "#/components/features/home/git-repo-dropdown/use-url-search";
import GitService from "#/api/git-service/git-service.api";
vi.mock("#/api/git-service/git-service.api", () => ({
default: {
searchGitRepositories: vi.fn(),
},
}));
const mockSearchGitRepositories = vi.mocked(GitService.searchGitRepositories);
describe("useUrlSearch", () => {
beforeEach(() => {
vi.clearAllMocks();
});
afterEach(() => {
vi.clearAllMocks();
});
describe("null/undefined provider guard", () => {
it("should not call GitService when provider is null", async () => {
const { result } = renderHook(() =>
useUrlSearch("https://github.com/owner/repo", null),
);
// Wait a tick for the effect to run
await act(async () => {
await new Promise((resolve) => {
setTimeout(resolve, 10);
});
});
expect(mockSearchGitRepositories).not.toHaveBeenCalled();
expect(result.current.urlSearchResults).toEqual([]);
expect(result.current.isUrlSearchLoading).toBe(false);
});
it("should not call GitService when provider is undefined", async () => {
const { result } = renderHook(() =>
useUrlSearch("https://github.com/owner/repo", undefined),
);
// Wait a tick for the effect to run
await act(async () => {
await new Promise((resolve) => {
setTimeout(resolve, 10);
});
});
expect(mockSearchGitRepositories).not.toHaveBeenCalled();
expect(result.current.urlSearchResults).toEqual([]);
expect(result.current.isUrlSearchLoading).toBe(false);
});
it("should clear results when provider becomes null", async () => {
mockSearchGitRepositories.mockResolvedValue({
items: [
{ id: "1", full_name: "owner/repo", git_provider: "github", is_public: true },
],
next_page_id: null,
});
type TestProps = { inputValue: string; provider: "github" | null };
const { result, rerender } = renderHook(
({ inputValue, provider }: TestProps) => useUrlSearch(inputValue, provider),
{
initialProps: {
inputValue: "https://github.com/owner/repo",
provider: "github",
} as TestProps,
},
);
// Wait for initial search to complete
await waitFor(() => {
expect(result.current.urlSearchResults).toHaveLength(1);
});
// Change provider to null
rerender({
inputValue: "https://github.com/owner/repo",
provider: null,
});
// Results should be cleared
await waitFor(() => {
expect(result.current.urlSearchResults).toEqual([]);
});
});
});
describe("URL search behavior", () => {
it("should call GitService when input is a valid URL and provider is set", async () => {
mockSearchGitRepositories.mockResolvedValue({
items: [
{ id: "1", full_name: "owner/repo", git_provider: "github", is_public: true },
],
next_page_id: null,
});
const { result } = renderHook(() =>
useUrlSearch("https://github.com/owner/repo", "github"),
);
await waitFor(() => {
expect(mockSearchGitRepositories).toHaveBeenCalledWith(
"owner/repo",
"github",
3,
);
});
await waitFor(() => {
expect(result.current.urlSearchResults).toHaveLength(1);
expect(result.current.urlSearchResults[0].full_name).toBe("owner/repo");
});
});
it("should not call GitService when input is not a URL", async () => {
const { result } = renderHook(() =>
useUrlSearch("some search query", "github"),
);
// Wait a tick for the effect to run
await act(async () => {
await new Promise((resolve) => {
setTimeout(resolve, 10);
});
});
expect(mockSearchGitRepositories).not.toHaveBeenCalled();
expect(result.current.urlSearchResults).toEqual([]);
});
it("should not call GitService when URL does not match repo pattern", async () => {
const { result } = renderHook(() =>
useUrlSearch("https://github.com/", "github"),
);
// Wait a tick for the effect to run
await act(async () => {
await new Promise((resolve) => {
setTimeout(resolve, 10);
});
});
expect(mockSearchGitRepositories).not.toHaveBeenCalled();
expect(result.current.urlSearchResults).toEqual([]);
});
it("should handle API errors gracefully", async () => {
mockSearchGitRepositories.mockRejectedValue(new Error("API Error"));
const { result } = renderHook(() =>
useUrlSearch("https://github.com/owner/repo", "github"),
);
await waitFor(() => {
expect(mockSearchGitRepositories).toHaveBeenCalled();
});
// Should return empty results on error
await waitFor(() => {
expect(result.current.urlSearchResults).toEqual([]);
expect(result.current.isUrlSearchLoading).toBe(false);
});
});
it("should set loading state correctly during search", async () => {
let resolveSearch: (value: unknown) => void;
const searchPromise = new Promise((resolve) => {
resolveSearch = resolve;
});
mockSearchGitRepositories.mockReturnValue(searchPromise as Promise<{
items: [];
next_page_id: null;
}>);
const { result } = renderHook(() =>
useUrlSearch("https://github.com/owner/repo", "github"),
);
// Should be loading
await waitFor(() => {
expect(result.current.isUrlSearchLoading).toBe(true);
});
// Resolve the search
await act(async () => {
resolveSearch!({ items: [], next_page_id: null });
});
// Should no longer be loading
await waitFor(() => {
expect(result.current.isUrlSearchLoading).toBe(false);
});
});
});
describe("clear results on non-URL input", () => {
it("should clear results when input changes from URL to non-URL", async () => {
mockSearchGitRepositories.mockResolvedValue({
items: [
{ id: "1", full_name: "owner/repo", git_provider: "github", is_public: true },
],
next_page_id: null,
});
const { result, rerender } = renderHook(
({ inputValue, provider }) => useUrlSearch(inputValue, provider),
{
initialProps: {
inputValue: "https://github.com/owner/repo",
provider: "github" as const,
},
},
);
// Wait for initial search to complete
await waitFor(() => {
expect(result.current.urlSearchResults).toHaveLength(1);
});
// Change to non-URL input
rerender({
inputValue: "some search",
provider: "github" as const,
});
// Results should be cleared
await waitFor(() => {
expect(result.current.urlSearchResults).toEqual([]);
});
});
});
});
+34 -1
View File
@@ -16,6 +16,21 @@ const safeProvider = (value: string): Provider => value as Provider;
const isCloudActive = () => getActiveBackend().backend.kind === "cloud";
/**
* Guard against null/undefined provider values that would result in
* invalid API requests (e.g., "?provider=undefined"). Returns true
* if the provider is falsy and the request should be skipped.
*/
const isInvalidProvider = (provider: string | null | undefined): boolean =>
!provider || provider === "undefined" || provider === "null";
const EMPTY_REPOSITORY_PAGE: RepositoryPage = { items: [], next_page_id: null };
const EMPTY_BRANCH_PAGE: BranchPage = { items: [], next_page_id: null };
const EMPTY_INSTALLATION_PAGE: InstallationPage = {
items: [],
next_page_id: null,
};
class GitService {
static async searchGitRepositories(
query: string,
@@ -24,6 +39,9 @@ class GitService {
pageId?: string,
installationId?: string,
): Promise<RepositoryPage> {
if (isInvalidProvider(provider)) {
return EMPTY_REPOSITORY_PAGE;
}
if (isCloudActive()) {
return searchCloudRepositories({
provider: safeProvider(provider),
@@ -47,6 +65,9 @@ class GitService {
limit = 30,
installationId?: string,
): Promise<RepositoryPage> {
if (isInvalidProvider(provider)) {
return EMPTY_REPOSITORY_PAGE;
}
if (isCloudActive()) {
return searchCloudRepositories({
provider: safeProvider(provider),
@@ -69,8 +90,11 @@ class GitService {
pageId?: string,
limit = 30,
): Promise<RepositoryPage> {
if (isInvalidProvider(provider)) {
return EMPTY_REPOSITORY_PAGE;
}
const installationId = installations[installationIndex];
if (!installationId) return { items: [], next_page_id: null };
if (!installationId) return EMPTY_REPOSITORY_PAGE;
if (isCloudActive()) {
return searchCloudRepositories({
provider: safeProvider(provider),
@@ -93,6 +117,9 @@ class GitService {
pageId?: string,
limit = 30,
): Promise<BranchPage> {
if (isInvalidProvider(provider)) {
return EMPTY_BRANCH_PAGE;
}
if (isCloudActive()) {
return getCloudRepositoryBranches({
provider: safeProvider(provider),
@@ -117,6 +144,9 @@ class GitService {
pageId?: string,
limit = 30,
): Promise<BranchPage> {
if (isInvalidProvider(provider)) {
return EMPTY_BRANCH_PAGE;
}
if (isCloudActive()) {
return getCloudRepositoryBranches({
provider: safeProvider(provider),
@@ -139,6 +169,9 @@ class GitService {
pageId?: string,
limit = 100,
): Promise<InstallationPage> {
if (isInvalidProvider(provider)) {
return EMPTY_INSTALLATION_PAGE;
}
if (isCloudActive()) {
return getCloudInstallations({
provider: safeProvider(provider),
@@ -3,12 +3,22 @@ import { Provider } from "#/types/settings";
import { GitRepository } from "#/types/git";
import GitService from "#/api/git-service/git-service.api";
export function useUrlSearch(inputValue: string, provider: Provider) {
export function useUrlSearch(
inputValue: string,
provider: Provider | null | undefined,
) {
const [urlSearchResults, setUrlSearchResults] = useState<GitRepository[]>([]);
const [isUrlSearchLoading, setIsUrlSearchLoading] = useState(false);
useEffect(() => {
const handleUrlSearch = async () => {
// Guard against null/undefined provider to prevent sending
// requests via the cloud proxy before providers have loaded
if (!provider) {
setUrlSearchResults([]);
return;
}
if (inputValue.startsWith("https://")) {
const match = inputValue.match(/https:\/\/[^/]+\/([^/]+\/[^/]+)/);
if (match) {