From 4eec0499f8658e117c718e10c0b4d8ddd5bbe45f Mon Sep 17 00:00:00 2001 From: Joe Cardoso Date: Thu, 12 Mar 2026 17:59:46 -0300 Subject: [PATCH] Fix announce/publish rejecting uppercase owner names Normalise package and skill names to lowercase before validation so that owners like @JoeCardoso13 can publish without hitting a 400. This also prevents @Foo/bar and @foo/bar from being treated as separate packages. Add regression tests for bundles and skills announce endpoints. --- apps/registry/src/routes/packages.ts | 7 +- apps/registry/src/routes/v1/bundles.ts | 7 +- apps/registry/src/routes/v1/skills.ts | 7 +- apps/registry/tests/bundles.test.ts | 45 +++++ apps/registry/tests/skills.test.ts | 234 +++++++++++++++++++++++++ 5 files changed, 294 insertions(+), 6 deletions(-) create mode 100644 apps/registry/tests/skills.test.ts diff --git a/apps/registry/src/routes/packages.ts b/apps/registry/src/routes/packages.ts index 0d01b90..073409c 100644 --- a/apps/registry/src/routes/packages.ts +++ b/apps/registry/src/routes/packages.ts @@ -107,13 +107,16 @@ export const packageRoutes: FastifyPluginAsync = async (fastify) => { } // Extract name and version from manifest - const packageName = manifest.name; + const rawPackageName = manifest.name; const version = manifest.version; - if (!packageName || !version) { + if (!rawPackageName || !version) { throw new BadRequestError('Manifest must contain name and version'); } + // Normalise to lowercase so @Foo/bar and @foo/bar are the same package + const packageName = rawPackageName.toLowerCase(); + // Validate package name format if (!isValidPackageName(packageName)) { throw new BadRequestError( diff --git a/apps/registry/src/routes/v1/bundles.ts b/apps/registry/src/routes/v1/bundles.ts index 30aa5b3..c2b9094 100644 --- a/apps/registry/src/routes/v1/bundles.ts +++ b/apps/registry/src/routes/v1/bundles.ts @@ -724,7 +724,7 @@ export const bundleRoutes: FastifyPluginAsync = async (fastify) => { // Extract body const { - name, + name: rawName, version, manifest, release_tag, @@ -745,6 +745,9 @@ export const bundleRoutes: FastifyPluginAsync = async (fastify) => { }; }; + // Normalise to lowercase so @Foo/bar and @foo/bar are the same package + const name = rawName.toLowerCase(); + // Validate artifact platform values const VALID_OS = ['darwin', 'linux', 'win32', 'any']; const VALID_ARCH = ['x64', 'arm64', 'any']; @@ -769,7 +772,7 @@ export const bundleRoutes: FastifyPluginAsync = async (fastify) => { // Validate package name if (!isValidScopedPackageName(name)) { throw new BadRequestError( - `Invalid package name: "${name}". Must be scoped (@scope/name) with lowercase alphanumeric characters and hyphens.` + `Invalid package name: "${rawName}". Must be scoped (@scope/name) with alphanumeric characters and hyphens.` ); } diff --git a/apps/registry/src/routes/v1/skills.ts b/apps/registry/src/routes/v1/skills.ts index d6aec1e..4891eb9 100644 --- a/apps/registry/src/routes/v1/skills.ts +++ b/apps/registry/src/routes/v1/skills.ts @@ -488,7 +488,7 @@ export const skillRoutes: FastifyPluginAsync = async (fastify) => { throw new UnauthorizedError(`Invalid OIDC token: ${message}`); } - const { name, version, skill: frontmatter, release_tag, prerelease = false, artifact } = + const { name: rawName, version, skill: frontmatter, release_tag, prerelease = false, artifact } = request.body as { name: string; version: string; @@ -502,10 +502,13 @@ export const skillRoutes: FastifyPluginAsync = async (fastify) => { }; }; + // Normalise to lowercase so @Foo/bar and @foo/bar are the same skill + const name = rawName.toLowerCase(); + // Validate name if (!isValidScopedName(name)) { throw new BadRequestError( - `Invalid skill name: "${name}". Must be scoped (@scope/name) with lowercase alphanumeric characters and hyphens.` + `Invalid skill name: "${rawName}". Must be scoped (@scope/name) with alphanumeric characters and hyphens.` ); } diff --git a/apps/registry/tests/bundles.test.ts b/apps/registry/tests/bundles.test.ts index a2b02c9..1a3967d 100644 --- a/apps/registry/tests/bundles.test.ts +++ b/apps/registry/tests/bundles.test.ts @@ -503,6 +503,51 @@ describe('Bundle Routes', () => { expect(res.statusCode).toBe(400); }); + it('normalises uppercase package name to lowercase before validation', async () => { + (verifyGitHubOIDC as Mock).mockResolvedValue({ + ...validOIDCClaims, + repository_owner: 'TestOrg', + repository: 'TestOrg/mcp-server', + }); + + const res = await app.inject({ + method: 'POST', + url: '/announce', + headers: { authorization: 'Bearer valid-token' }, + payload: { + ...validPayload, + name: '@TestOrg/mcp-server', + }, + }); + + // Should NOT fail with "Invalid package name" — the name is normalised + // to lowercase before the regex check. It will fail later (GitHub fetch), + // but the validation gate must pass. + const body = JSON.parse(res.payload); + expect(body.error?.message ?? '').not.toContain('Invalid package name'); + }); + + it('normalises mixed-case scope for OIDC owner matching', async () => { + (verifyGitHubOIDC as Mock).mockResolvedValue({ + ...validOIDCClaims, + repository_owner: 'MyOrg', + repository: 'MyOrg/cool-server', + }); + + const res = await app.inject({ + method: 'POST', + url: '/announce', + headers: { authorization: 'Bearer valid-token' }, + payload: { + ...validPayload, + name: '@MyOrg/cool-server', + }, + }); + + const body = JSON.parse(res.payload); + expect(body.error?.message ?? '').not.toContain('Scope mismatch'); + }); + it('rejects manifest without server type', async () => { (verifyGitHubOIDC as Mock).mockResolvedValue(validOIDCClaims); diff --git a/apps/registry/tests/skills.test.ts b/apps/registry/tests/skills.test.ts new file mode 100644 index 0000000..76bc5eb --- /dev/null +++ b/apps/registry/tests/skills.test.ts @@ -0,0 +1,234 @@ +/** + * Skill API route tests. + * + * Covers announce name-normalisation to prevent uppercase-scope regressions. + * External dependencies (config, db, oidc, discord) are mocked so tests run + * without a database or network access. + */ + +import { describe, it, expect, vi, beforeEach, beforeAll, afterAll } from 'vitest'; +import type { Mock } from 'vitest'; +import Fastify, { type FastifyInstance } from 'fastify'; +import sensible from '@fastify/sensible'; + +// --------------------------------------------------------------------------- +// Module mocks (hoisted before all imports) +// --------------------------------------------------------------------------- + +vi.mock('../src/config.js', () => ({ + config: { + storage: { + type: 'local', + path: '/tmp/test-storage', + cloudfront: { urlExpirationSeconds: 900, domain: '', keyPairId: '' }, + s3: { bucket: '', region: '', accessKeyId: '', secretAccessKey: '' }, + }, + scanner: { enabled: false, callbackSecret: 'test-secret' }, + server: { nodeEnv: 'test', port: 3000, host: '0.0.0.0', corsOrigins: [] }, + clerk: { secretKey: '' }, + limits: { maxBundleSizeMB: 50 }, + }, + validateConfig: vi.fn(), +})); + +vi.mock('../src/db/index.js', () => ({ + runInTransaction: vi.fn(async (fn: (tx: unknown) => unknown) => fn({})), + getPrismaClient: vi.fn(), + disconnectDatabase: vi.fn(), +})); + +vi.mock('../src/lib/oidc.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + verifyGitHubOIDC: vi.fn(), + }; +}); + +vi.mock('../src/utils/discord.js', () => ({ + notifyDiscordAnnounce: vi.fn(), +})); + +vi.mock('../src/utils/badge.js', () => ({ + generateBadge: vi.fn().mockReturnValue('badge'), +})); + +vi.mock('../src/utils/skill-content.js', () => ({ + extractSkillContent: vi.fn().mockReturnValue('# Skill content'), +})); + +// --------------------------------------------------------------------------- +// Imports (after mocks) +// --------------------------------------------------------------------------- + +import { createMockSkillRepo, createMockStorage, createMockPrisma } from './helpers.js'; +import { verifyGitHubOIDC } from '../src/lib/oidc.js'; + +// --------------------------------------------------------------------------- +// Test setup +// --------------------------------------------------------------------------- + +describe('Skill Routes', () => { + let app: FastifyInstance; + let skillRepo: ReturnType; + let storage: ReturnType; + let prisma: ReturnType; + + beforeAll(async () => { + skillRepo = createMockSkillRepo(); + storage = createMockStorage(); + prisma = createMockPrisma(); + + app = Fastify({ logger: false }); + app.setReplySerializer((payload) => JSON.stringify(payload)); + await app.register(sensible); + + // Decorate with mocks + app.decorate('repositories', { + packages: {}, + users: {}, + skills: skillRepo, + }); + app.decorate('storage', storage); + app.decorate('prisma', prisma); + + const { skillRoutes } = await import('../src/routes/v1/skills.js'); + await app.register(skillRoutes); + await app.ready(); + }); + + afterAll(async () => { + await app.close(); + }); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + // ========================================================================= + // POST /announce (validation / normalisation tests) + // ========================================================================= + + describe('POST /announce', () => { + const validOIDCClaims = { + repository: 'test-org/my-skill', + repository_owner: 'test-org', + repository_owner_id: '12345', + workflow: '.github/workflows/publish.yml', + workflow_ref: 'ref', + ref: 'refs/tags/v1.0.0', + ref_type: 'tag', + sha: 'abc123', + actor: 'bot', + actor_id: '1', + run_id: '1', + run_number: '1', + run_attempt: '1', + event_name: 'release', + job_workflow_ref: 'ref', + }; + + const validPayload = { + name: '@test-org/my-skill', + version: '1.0.0', + skill: { name: 'my-skill', description: 'A test skill', metadata: {} }, + release_tag: 'v1.0.0', + artifact: { + filename: 'my-skill-1.0.0.skill', + sha256: 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef', + size: 512, + }, + }; + + it('rejects requests without authorization header', async () => { + const res = await app.inject({ + method: 'POST', + url: '/announce', + payload: validPayload, + }); + + // Returns 400 (schema validation) or 401 — either way, the request is rejected + expect([400, 401]).toContain(res.statusCode); + }); + + it('rejects invalid (non-scoped) skill name', async () => { + (verifyGitHubOIDC as Mock).mockResolvedValue(validOIDCClaims); + + const res = await app.inject({ + method: 'POST', + url: '/announce', + headers: { authorization: 'Bearer valid-token' }, + payload: { + ...validPayload, + name: 'no-scope-skill', + }, + }); + + expect(res.statusCode).toBe(400); + }); + + it('normalises uppercase skill name to lowercase before validation', async () => { + (verifyGitHubOIDC as Mock).mockResolvedValue({ + ...validOIDCClaims, + repository_owner: 'TestOrg', + repository: 'TestOrg/my-skill', + }); + + const res = await app.inject({ + method: 'POST', + url: '/announce', + headers: { authorization: 'Bearer valid-token' }, + payload: { + ...validPayload, + name: '@TestOrg/my-skill', + }, + }); + + // Should NOT fail with "Invalid skill name" — the name is normalised + // to lowercase before the regex check. It will fail later (GitHub fetch), + // but the validation gate must pass. + const body = JSON.parse(res.payload); + expect(body.error?.message ?? '').not.toContain('Invalid skill name'); + }); + + it('normalises mixed-case scope for OIDC owner matching', async () => { + (verifyGitHubOIDC as Mock).mockResolvedValue({ + ...validOIDCClaims, + repository_owner: 'MyOrg', + repository: 'MyOrg/my-skill', + }); + + const res = await app.inject({ + method: 'POST', + url: '/announce', + headers: { authorization: 'Bearer valid-token' }, + payload: { + ...validPayload, + name: '@MyOrg/my-skill', + }, + }); + + const body = JSON.parse(res.payload); + expect(body.error?.message ?? '').not.toContain('Scope mismatch'); + }); + + it('rejects scope mismatch between skill name and OIDC owner', async () => { + (verifyGitHubOIDC as Mock).mockResolvedValue(validOIDCClaims); // owner = test-org + + const res = await app.inject({ + method: 'POST', + url: '/announce', + headers: { authorization: 'Bearer valid-token' }, + payload: { + ...validPayload, + name: '@other-org/my-skill', + }, + }); + + // Handler returns the error as JSON via handleError + const body = JSON.parse(res.payload); + expect(body.error.message).toContain('Scope mismatch'); + expect(res.statusCode).toBe(401); + }); + }); +});