-
Notifications
You must be signed in to change notification settings - Fork 271
Fix getEffectiveProjectId to prioritize passed projectId over GITLAB_PROJECT_ID #320
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.
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
Changes from all commits
f4cc963
1739b4b
2279ec4
2ddb8fd
096169d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,8 @@ build | |
| .env.test | ||
| coverage/ | ||
| *.log | ||
| test-results-oauth.json | ||
|
|
||
| # ai | ||
| .opencode* | ||
| .aider* | ||
| .sisyphus | ||
| test-results*.json | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,279 @@ | ||
| /** | ||
| * Test suite for getEffectiveProjectId function | ||
| * Tests the behavior of project ID resolution with different environment configurations | ||
| */ | ||
|
|
||
| import { describe, test, before, after } from 'node:test'; | ||
| import assert from 'node:assert'; | ||
| import { | ||
| launchServer, | ||
| findAvailablePort, | ||
| cleanupServers, | ||
| ServerInstance, | ||
| TransportMode, | ||
| HOST | ||
| } from './utils/server-launcher.js'; | ||
| import { MockGitLabServer, findMockServerPort } from './utils/mock-gitlab-server.js'; | ||
| import { StreamableHTTPTestClient } from './clients/streamable-http-client.js'; | ||
|
|
||
| // Use the same token that will be passed via GITLAB_TOKEN_TEST environment variable | ||
| const MOCK_TOKEN = process.env.GITLAB_TOKEN_TEST || 'glpat-mock-token-12345'; | ||
| const DEFAULT_PROJECT_ID = '123'; | ||
| const OTHER_PROJECT_ID = '456'; | ||
|
|
||
| console.log('🔍 Testing getEffectiveProjectId functionality'); | ||
| console.log(''); | ||
|
|
||
| describe('getEffectiveProjectId - No GITLAB_ALLOWED_PROJECT_IDS', () => { | ||
| let mcpUrl: string; | ||
| let mockGitLab: MockGitLabServer; | ||
| let servers: ServerInstance[] = []; | ||
| let client: StreamableHTTPTestClient; | ||
|
|
||
| before(async () => { | ||
| // Start mock GitLab server | ||
| const mockPort = await findMockServerPort(9100); | ||
| mockGitLab = new MockGitLabServer({ | ||
| port: mockPort, | ||
| validTokens: [MOCK_TOKEN] | ||
| }); | ||
| await mockGitLab.start(); | ||
| const mockGitLabUrl = mockGitLab.getUrl(); | ||
|
|
||
| // Start MCP server WITHOUT GITLAB_ALLOWED_PROJECT_IDS | ||
| const mcpPort = await findAvailablePort(3100); | ||
| const server = await launchServer({ | ||
| mode: TransportMode.STREAMABLE_HTTP, | ||
| port: mcpPort, | ||
| timeout: 5000, | ||
| env: { | ||
| STREAMABLE_HTTP: 'true', | ||
| GITLAB_API_URL: `${mockGitLabUrl}/api/v4`, | ||
| GITLAB_PROJECT_ID: DEFAULT_PROJECT_ID, | ||
| GITLAB_READ_ONLY_MODE: 'true', | ||
| } | ||
|
Comment on lines
+45
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
These Useful? React with 👍 / 👎. |
||
| }); | ||
|
Comment on lines
+33
to
+55
|
||
| servers.push(server); | ||
| mcpUrl = `http://${HOST}:${mcpPort}/mcp`; | ||
|
|
||
| client = new StreamableHTTPTestClient(); | ||
| await client.connect(mcpUrl); | ||
|
|
||
| console.log(`Mock GitLab: ${mockGitLabUrl}`); | ||
| console.log(`MCP Server: ${mcpUrl}`); | ||
| console.log(`Default Project: ${DEFAULT_PROJECT_ID}`); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| if (client) { | ||
| await client.disconnect(); | ||
| } | ||
| cleanupServers(servers); | ||
| if (mockGitLab) { | ||
| await mockGitLab.stop(); | ||
| } | ||
| }); | ||
|
|
||
| test('should use GITLAB_PROJECT_ID when no project_id is provided', async () => { | ||
| // Call get_project without specifying project_id | ||
| const result = await client.callTool('get_project', { | ||
| project_id: '' | ||
| }); | ||
|
|
||
| assert.ok(result.content, 'Should have content'); | ||
| const content = result.content[0]; | ||
| assert.ok('text' in content, 'Content should have text'); | ||
| const project = JSON.parse(content.text); | ||
|
|
||
| // The mock server should receive a request for the default project | ||
| assert.strictEqual(project.id.toString(), DEFAULT_PROJECT_ID, 'Should use GITLAB_PROJECT_ID as default'); | ||
| console.log(` ✓ Used default project ${DEFAULT_PROJECT_ID} when no project_id provided`); | ||
| }); | ||
|
|
||
| test('should prioritize passed project_id over GITLAB_PROJECT_ID', async () => { | ||
| // Call get_project with a different project_id | ||
| const result = await client.callTool('get_project', { | ||
| project_id: OTHER_PROJECT_ID | ||
| }); | ||
|
|
||
| assert.ok(result.content, 'Should have content'); | ||
| const content = result.content[0]; | ||
| assert.ok('text' in content, 'Content should have text'); | ||
| const project = JSON.parse(content.text); | ||
|
|
||
| // Should use the passed project_id, not GITLAB_PROJECT_ID | ||
| assert.strictEqual(project.id.toString(), OTHER_PROJECT_ID, 'Should use passed project_id'); | ||
| console.log(` ✓ Used passed project_id ${OTHER_PROJECT_ID} instead of default ${DEFAULT_PROJECT_ID}`); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getEffectiveProjectId - With single GITLAB_ALLOWED_PROJECT_IDS', () => { | ||
| let mcpUrl: string; | ||
| let mockGitLab: MockGitLabServer; | ||
| let servers: ServerInstance[] = []; | ||
| let client: StreamableHTTPTestClient; | ||
|
|
||
| before(async () => { | ||
| // Start mock GitLab server | ||
| const mockPort = await findMockServerPort(9200); | ||
| mockGitLab = new MockGitLabServer({ | ||
| port: mockPort, | ||
| validTokens: [MOCK_TOKEN] | ||
| }); | ||
| await mockGitLab.start(); | ||
| const mockGitLabUrl = mockGitLab.getUrl(); | ||
|
|
||
| // Start MCP server WITH single GITLAB_ALLOWED_PROJECT_IDS | ||
| const mcpPort = await findAvailablePort(3200); | ||
| const server = await launchServer({ | ||
| mode: TransportMode.STREAMABLE_HTTP, | ||
| port: mcpPort, | ||
| timeout: 5000, | ||
| env: { | ||
| STREAMABLE_HTTP: 'true', | ||
| GITLAB_API_URL: `${mockGitLabUrl}/api/v4`, | ||
| GITLAB_PROJECT_ID: DEFAULT_PROJECT_ID, | ||
| GITLAB_ALLOWED_PROJECT_IDS: DEFAULT_PROJECT_ID, | ||
| GITLAB_READ_ONLY_MODE: 'true', | ||
| } | ||
| }); | ||
| servers.push(server); | ||
| mcpUrl = `http://${HOST}:${mcpPort}/mcp`; | ||
|
|
||
| client = new StreamableHTTPTestClient(); | ||
| await client.connect(mcpUrl); | ||
|
|
||
| console.log(`Mock GitLab: ${mockGitLabUrl}`); | ||
| console.log(`MCP Server: ${mcpUrl}`); | ||
| console.log(`Allowed Project: ${DEFAULT_PROJECT_ID}`); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| if (client) { | ||
| await client.disconnect(); | ||
| } | ||
| cleanupServers(servers); | ||
| if (mockGitLab) { | ||
| await mockGitLab.stop(); | ||
| } | ||
| }); | ||
|
|
||
| test('should use single allowed project as default', async () => { | ||
| const result = await client.callTool('get_project', { | ||
| project_id: '' | ||
| }); | ||
|
|
||
| assert.ok(result.content, 'Should have content'); | ||
| const content = result.content[0]; | ||
| assert.ok('text' in content, 'Content should have text'); | ||
| const project = JSON.parse(content.text); | ||
|
|
||
| assert.strictEqual(project.id.toString(), DEFAULT_PROJECT_ID, 'Should use allowed project as default'); | ||
| console.log(` ✓ Used allowed project ${DEFAULT_PROJECT_ID} as default`); | ||
| }); | ||
|
|
||
| test('should reject access to non-allowed project', async () => { | ||
| try { | ||
| await client.callTool('get_project', { | ||
| project_id: OTHER_PROJECT_ID | ||
| }); | ||
| assert.fail('Should have rejected access to non-allowed project'); | ||
| } catch (error) { | ||
| assert.ok(error instanceof Error); | ||
| assert.ok(error.message.includes('Access denied'), 'Should indicate access denied'); | ||
| console.log(' ✓ Correctly rejected access to non-allowed project'); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe('getEffectiveProjectId - With multiple GITLAB_ALLOWED_PROJECT_IDS', () => { | ||
| let mcpUrl: string; | ||
| let mockGitLab: MockGitLabServer; | ||
| let servers: ServerInstance[] = []; | ||
| let client: StreamableHTTPTestClient; | ||
|
|
||
| before(async () => { | ||
| // Start mock GitLab server | ||
| const mockPort = await findMockServerPort(9300); | ||
| mockGitLab = new MockGitLabServer({ | ||
| port: mockPort, | ||
| validTokens: [MOCK_TOKEN] | ||
| }); | ||
| await mockGitLab.start(); | ||
| const mockGitLabUrl = mockGitLab.getUrl(); | ||
|
|
||
| // Start MCP server WITH multiple GITLAB_ALLOWED_PROJECT_IDS | ||
| const mcpPort = await findAvailablePort(3300); | ||
| const server = await launchServer({ | ||
| mode: TransportMode.STREAMABLE_HTTP, | ||
| port: mcpPort, | ||
| timeout: 5000, | ||
| env: { | ||
| STREAMABLE_HTTP: 'true', | ||
| GITLAB_API_URL: `${mockGitLabUrl}/api/v4`, | ||
| GITLAB_ALLOWED_PROJECT_IDS: `${DEFAULT_PROJECT_ID},${OTHER_PROJECT_ID}`, | ||
| GITLAB_READ_ONLY_MODE: 'true', | ||
| } | ||
| }); | ||
| servers.push(server); | ||
| mcpUrl = `http://${HOST}:${mcpPort}/mcp`; | ||
|
|
||
| client = new StreamableHTTPTestClient(); | ||
| await client.connect(mcpUrl); | ||
|
|
||
| console.log(`Mock GitLab: ${mockGitLabUrl}`); | ||
| console.log(`MCP Server: ${mcpUrl}`); | ||
| console.log(`Allowed Projects: ${DEFAULT_PROJECT_ID},${OTHER_PROJECT_ID}`); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| if (client) { | ||
| await client.disconnect(); | ||
| } | ||
| cleanupServers(servers); | ||
| if (mockGitLab) { | ||
| await mockGitLab.stop(); | ||
| } | ||
| }); | ||
|
|
||
| test('should require explicit project_id when multiple projects allowed', async () => { | ||
| try { | ||
| await client.callTool('get_project', { | ||
| project_id: '' | ||
| }); | ||
| assert.fail('Should have required explicit project_id'); | ||
| } catch (error) { | ||
| assert.ok(error instanceof Error); | ||
| assert.ok(error.message.includes('Please specify a project ID'), 'Should require project ID'); | ||
| console.log(' ✓ Correctly required explicit project_id'); | ||
| } | ||
| }); | ||
|
|
||
| test('should allow access to first allowed project', async () => { | ||
| const result = await client.callTool('get_project', { | ||
| project_id: DEFAULT_PROJECT_ID | ||
| }); | ||
|
|
||
| assert.ok(result.content, 'Should have content'); | ||
| const content = result.content[0]; | ||
| assert.ok('text' in content, 'Content should have text'); | ||
| const project = JSON.parse(content.text); | ||
|
|
||
| assert.strictEqual(project.id.toString(), DEFAULT_PROJECT_ID, 'Should allow first project'); | ||
| console.log(` ✓ Allowed access to first project ${DEFAULT_PROJECT_ID}`); | ||
| }); | ||
|
|
||
| test('should allow access to second allowed project', async () => { | ||
| const result = await client.callTool('get_project', { | ||
| project_id: OTHER_PROJECT_ID | ||
| }); | ||
|
|
||
| assert.ok(result.content, 'Should have content'); | ||
| const content = result.content[0]; | ||
| assert.ok('text' in content, 'Content should have text'); | ||
| const project = JSON.parse(content.text); | ||
|
|
||
| assert.strictEqual(project.id.toString(), OTHER_PROJECT_ID, 'Should allow second project'); | ||
| console.log(` ✓ Allowed access to second project ${OTHER_PROJECT_ID}`); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper now makes
GITLAB_PROJECT_IDa fallback instead of a lock, but thefork_repositoryandcreate_repositoryhandlers still reject whenever that env var is present (index.ts:5316-5318andindex.ts:5388-5391). In a deployment that keepsGITLAB_PROJECT_IDonly to supply a default project, project-scoped tools can now target arbitraryproject_ids while create/fork always fail, so the new semantics are only applied to part of the API surface.Useful? React with 👍 / 👎.