From 24da834da33cc84c064803eccc11e2dc7199fd26 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Mon, 11 May 2026 17:14:33 -0400 Subject: [PATCH] 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 --- __tests__/api/git-service.test.ts | 260 ++++++++++++++++++ .../features/home/use-url-search.test.tsx | 241 ++++++++++++++++ src/api/git-service/git-service.api.ts | 35 ++- .../home/git-repo-dropdown/use-url-search.tsx | 12 +- 4 files changed, 546 insertions(+), 2 deletions(-) create mode 100644 __tests__/api/git-service.test.ts create mode 100644 __tests__/components/features/home/use-url-search.test.tsx diff --git a/__tests__/api/git-service.test.ts b/__tests__/api/git-service.test.ts new file mode 100644 index 0000000000..34d319c127 --- /dev/null +++ b/__tests__/api/git-service.test.ts @@ -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); + }); + }); +}); diff --git a/__tests__/components/features/home/use-url-search.test.tsx b/__tests__/components/features/home/use-url-search.test.tsx new file mode 100644 index 0000000000..70489f5d95 --- /dev/null +++ b/__tests__/components/features/home/use-url-search.test.tsx @@ -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([]); + }); + }); + }); +}); diff --git a/src/api/git-service/git-service.api.ts b/src/api/git-service/git-service.api.ts index 50ad2cab61..63d114531a 100644 --- a/src/api/git-service/git-service.api.ts +++ b/src/api/git-service/git-service.api.ts @@ -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 { + 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 { + 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 { + 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 { + 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 { + 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 { + if (isInvalidProvider(provider)) { + return EMPTY_INSTALLATION_PAGE; + } if (isCloudActive()) { return getCloudInstallations({ provider: safeProvider(provider), diff --git a/src/components/features/home/git-repo-dropdown/use-url-search.tsx b/src/components/features/home/git-repo-dropdown/use-url-search.tsx index e8ace43de1..c6d8181fa7 100644 --- a/src/components/features/home/git-repo-dropdown/use-url-search.tsx +++ b/src/components/features/home/git-repo-dropdown/use-url-search.tsx @@ -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([]); 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) {