diff --git a/.github/actions/publish-plugin/action.yml b/.github/actions/publish-plugin/action.yml index ee054a6..2415a4c 100644 --- a/.github/actions/publish-plugin/action.yml +++ b/.github/actions/publish-plugin/action.yml @@ -53,6 +53,10 @@ runs: REF_SUFFIX_OVERRIDE: ${{ inputs.ref-suffix }} run: | set -euo pipefail + # Strip any whitespace/newline picked up when the key was pasted into + # the CI secret — a raw line break in the x-api-key header corrupts + # the HTTP request (the registry then sees no Content-Type header). + REGISTRY_API_KEY=$(printf '%s' "$REGISTRY_API_KEY" | tr -d '[:space:]') PACKAGE_NAME=$(node -p "require('./package.json').name") PACKAGE_VERSION=$(node -p "require('./package.json').version") diff --git a/api/nodemon.json b/api/nodemon.json index 91717bb..216f200 100644 --- a/api/nodemon.json +++ b/api/nodemon.json @@ -1,6 +1,6 @@ { "ignoreRoot": [], - "ignore": ["node_modules/", "../dev/", "data/"], + "ignore": ["node_modules/", "../dev/", "data/", "data-upstream/"], "execMap": { "ts": "node" } diff --git a/api/src/artefacts/router.ts b/api/src/artefacts/router.ts index 8c91ec7..2dbffab 100644 --- a/api/src/artefacts/router.ts +++ b/api/src/artefacts/router.ts @@ -400,6 +400,35 @@ router.get('/:id/download', async (req, res, next) => { } catch (err) { next(err) } }) +// Helper: build the busboy parser for an upload request, or settle with a 400 +// when it can't be built. Busboy's constructor throws synchronously on a +// missing Content-Type header, a non-multipart type, or a multipart type +// without a boundary — plain Errors that would otherwise surface as 500s +// (seen in production as "Missing Content-Type" from clients POSTing a raw +// body without the multipart header). +const createBusboy = (req: import('express').Request, settle: (err: Error | null) => void): ReturnType | null => { + const contentType = req.headers['content-type'] + if (!contentType || !contentType.toLowerCase().startsWith('multipart/form-data')) { + settle(httpError(400, 'request Content-Type must be multipart/form-data')) + return null + } + try { + return Busboy({ + headers: req.headers, + limits: { + fileSize: MAX_UPLOAD_BYTES, + files: 1, + fields: 20, + fieldSize: 64 * 1024, + fieldNameSize: 200 + } + }) + } catch (err) { + settle(httpError(400, err instanceof Error ? err.message : 'invalid multipart request')) + return null + } +} + // Helper: stream a multipart upload containing a tarball to a caller-provided // sink (typically the configured files-storage backend), collecting the // `category` field if present. Enforces MAX_UPLOAD_BYTES at the busboy layer. @@ -419,16 +448,8 @@ function streamTarballUpload (req: import('express').Request, writer: StreamWrit let fileSeen = false let pendingWrite: Promise | null = null - const busboy = Busboy({ - headers: req.headers, - limits: { - fileSize: MAX_UPLOAD_BYTES, - files: 1, - fields: 20, - fieldSize: 64 * 1024, - fieldNameSize: 200 - } - }) + const busboy = createBusboy(req, settle) + if (!busboy) return busboy.on('field', (name, val) => { // The artefact category comes solely from this multipart field; the @@ -488,16 +509,8 @@ function streamFileUpload (req: import('express').Request, writer: StreamWriter) let fileSeen = false let pendingWrite: Promise | null = null - const busboy = Busboy({ - headers: req.headers, - limits: { - fileSize: MAX_UPLOAD_BYTES, - files: 1, - fields: 20, - fieldSize: 64 * 1024, - fieldNameSize: 200 - } - }) + const busboy = createBusboy(req, settle) + if (!busboy) return busboy.on('field', (name, val) => { fields[name] = val diff --git a/docs/ci-integration.md b/docs/ci-integration.md index 10f4fc9..fbcb05c 100644 --- a/docs/ci-integration.md +++ b/docs/ci-integration.md @@ -13,6 +13,8 @@ Upload API keys are created by a superadmin in the registry UI ("Admin → API k The raw key is displayed **once** at creation time — copy it immediately and store it as a CI secret. It is never retrievable again (only a SHA-512 hash is stored server-side). +> **Trailing newline gotcha.** A key pasted into a CI secret with a trailing line break (easy to pick up when copying, or via `echo "$KEY" | gh secret set ...`) ends up verbatim in the `x-api-key` header and corrupts the HTTP request: depending on the curl/server versions the upload fails with an opaque connection error, or the registry sees the request without its `Content-Type` header and rejects it. The recipes below strip whitespace from the key before use — keep that line when adapting them. + You need **one key per registry environment**. A key issued by `koumoul.com/registry` will not authenticate against `staging-koumoul.com/registry` and vice-versa. ## Authentication @@ -136,6 +138,10 @@ jobs: REGISTRY_API_KEY: ${{ secrets.REGISTRY_API_KEY }} run: | set -euo pipefail + # Strip any whitespace/newline picked up when the key was pasted into + # the secret — a raw line break in the x-api-key header corrupts the + # HTTP request. + REGISTRY_API_KEY=$(printf '%s' "$REGISTRY_API_KEY" | tr -d '[:space:]') PACKAGE_NAME=$(node -p "require('./package.json').name") PACKAGE_MAJOR=$(node -p "require('./package.json').version.split('.')[0]") # Artefact id: package name with '/' flattened to '-', plus '-'. @@ -193,6 +199,8 @@ jobs: env: REGISTRY_API_KEY: ${{ secrets.REGISTRY_API_KEY }} run: | + # Strip any whitespace/newline a copy-paste may have added to the key. + REGISTRY_API_KEY=$(printf '%s' "$REGISTRY_API_KEY" | tr -d '[:space:]') curl -sS --fail-with-body -X POST \ "https://registry.example.com/api/v1/artefacts/file/my-tileset" \ -H "x-api-key: ${REGISTRY_API_KEY}" \ @@ -285,6 +293,8 @@ jobs: REGISTRY_API_KEY: ${{ secrets.REGISTRY_API_KEY }} run: | set -euo pipefail + # Strip any whitespace/newline a copy-paste may have added to the key. + REGISTRY_API_KEY=$(printf '%s' "$REGISTRY_API_KEY" | tr -d '[:space:]') PACKAGE_NAME=$(node -p "require('./package.json').name") # Artefact id: package name with '/' flattened to '-', plus '-'. ARTEFACT_ID="${PACKAGE_NAME//\//-}-${GITHUB_REF_NAME}" @@ -341,6 +351,8 @@ publish: - npm ci - npm pack - | + # Strip any whitespace/newline a copy-paste may have added to the key. + REGISTRY_API_KEY=$(printf '%s' "$REGISTRY_API_KEY" | tr -d '[:space:]') TARBALL=$(ls *.tgz) PACKAGE_NAME=$(node -p "require('./package.json').name") PACKAGE_MAJOR=$(node -p "require('./package.json').version.split('.')[0]") @@ -367,6 +379,8 @@ publish-tileset: script: - ./build-tileset.sh - | + # Strip any whitespace/newline a copy-paste may have added to the key. + REGISTRY_API_KEY=$(printf '%s' "$REGISTRY_API_KEY" | tr -d '[:space:]') curl -sS --fail-with-body -X POST \ "${REGISTRY_URL}/api/v1/artefacts/file/my-tileset" \ -H "x-api-key: ${REGISTRY_API_KEY}" \ @@ -469,4 +483,5 @@ This is inherently more secure than GitHub's default model — the protection is - [ ] `architecture` form field set on upload (matches `process.arch` of the consumer) - [ ] `category` form field set on npm uploads (matches the artefact kind and any `allowedCategory` on the key) - [ ] Upload uses `curl --fail-with-body` so the registry's response is printed (no silent `curl -f`) +- [ ] Upload step strips whitespace from the key before the curl call (guards against a secret stored with a trailing newline) - [ ] Key rotation process documented for your team diff --git a/tests/artefacts.api.spec.ts b/tests/artefacts.api.spec.ts index e072a80..4dd7cdf 100644 --- a/tests/artefacts.api.spec.ts +++ b/tests/artefacts.api.spec.ts @@ -97,6 +97,41 @@ test.describe('Artefacts', () => { expect(second.data.artefact.dataUpdatedAt).not.toBe(firstDataAt) }) + test('upload without a Content-Type header returns 400, not 500', async () => { + const ax = axiosWithApiKey(uploadApiKey) + try { + // No body at all → axios sends no Content-Type header + await ax.post('/api/v1/artefacts/npm/' + encodeURIComponent('@test/pkg@1')) + expect(true).toBe(false) + } catch (err: any) { + expect(err.status).toBe(400) + } + }) + + test('upload with a non-multipart Content-Type returns 400', async () => { + const ax = axiosWithApiKey(uploadApiKey) + try { + await ax.post( + '/api/v1/artefacts/npm/' + encodeURIComponent('@test/pkg@1'), + Buffer.from('not a multipart body'), + { headers: { 'Content-Type': 'application/octet-stream' } } + ) + expect(true).toBe(false) + } catch (err: any) { + expect(err.status).toBe(400) + } + }) + + test('file upload without a Content-Type header returns 400, not 500', async () => { + const ax = axiosWithApiKey(uploadApiKey) + try { + await ax.post('/api/v1/artefacts/file/some-file') + expect(true).toBe(false) + } catch (err: any) { + expect(err.status).toBe(400) + } + }) + test('re-upload with different manifest name on the same artefact id returns 409', async () => { const ax = axiosWithApiKey(uploadApiKey) const form1 = new FormData() diff --git a/ui/dts/auto-imports.d.ts b/ui/dts/auto-imports.d.ts index aef0c19..8c4de5d 100644 --- a/ui/dts/auto-imports.d.ts +++ b/ui/dts/auto-imports.d.ts @@ -129,6 +129,7 @@ declare module 'vue' { readonly $sitePath: UnwrapRef readonly $uiConfig: UnwrapRef readonly EffectScope: UnwrapRef + readonly SEVERITY_ORDER: UnwrapRef readonly categoryColor: UnwrapRef readonly categoryColors: UnwrapRef readonly categoryItems: UnwrapRef @@ -175,6 +176,7 @@ declare module 'vue' { readonly readonly: UnwrapRef readonly ref: UnwrapRef readonly resolveComponent: UnwrapRef + readonly severityColor: UnwrapRef readonly shallowReactive: UnwrapRef readonly shallowReadonly: UnwrapRef readonly shallowRef: UnwrapRef @@ -224,5 +226,6 @@ declare module 'vue' { readonly watchPostEffect: UnwrapRef readonly watchSyncEffect: UnwrapRef readonly withUiNotif: UnwrapRef + readonly worstSeverity: UnwrapRef } } \ No newline at end of file