From 381ff8164f6df9d01f9d3c7e38990b9813922cd9 Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 15 Aug 2022 11:41:47 +0200 Subject: [PATCH 1/4] chore: set as undefined if there's an error in the grabbing of the user entity Signed-off-by: blam --- plugins/scaffolder-backend/src/service/router.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 2f72004afe..1a1077cccf 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -192,7 +192,12 @@ export async function createRouter( ); const userEntity = userEntityRef - ? await catalogClient.getEntityByRef(userEntityRef, { token }) + ? await catalogClient + .getEntityByRef(userEntityRef, { token }) + .catch(e => { + logger.error(`Failed to get user entity: ${stringifyError(e)}`); + return undefined; + }) : undefined; let auditLog = `Scaffolding task for ${templateRef}`; From 471dc5699f5b06185ef1d2789acfe8e893fe2b9d Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 15 Aug 2022 11:43:27 +0200 Subject: [PATCH 2/4] chore: a little more refactoring here if it's undefined Signed-off-by: blam --- plugins/scaffolder-backend/src/service/router.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 1a1077cccf..beaa0388dc 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -241,10 +241,12 @@ export async function createRouter( })), output: template.spec.output ?? {}, parameters: values, - user: { - entity: userEntity as UserEntity, - ref: userEntityRef, - }, + user: userEntity + ? { + entity: userEntity as UserEntity, + ref: userEntityRef, + } + : undefined, templateInfo: { entityRef: stringifyEntityRef({ kind, From dad0f65494036639ba487be0d2cbe67f13817830 Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 15 Aug 2022 11:44:38 +0200 Subject: [PATCH 3/4] chore: add changeset Signed-off-by: blam --- .changeset/smooth-planes-itch.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/smooth-planes-itch.md diff --git a/.changeset/smooth-planes-itch.md b/.changeset/smooth-planes-itch.md new file mode 100644 index 0000000000..638a2e0bf6 --- /dev/null +++ b/.changeset/smooth-planes-itch.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Fail gracefully if an invalid `Authorization` header is passed to `POST /v2/tasks` From 0ba7c724054c8871036ced048d2aacefdd377d15 Mon Sep 17 00:00:00 2001 From: blam Date: Mon, 15 Aug 2022 12:58:10 +0200 Subject: [PATCH 4/4] chore: reworking the parsing so that we verify it's a valid entity ref before calling out anywhere Signed-off-by: blam --- .../src/service/router.test.ts | 51 +++++++++++++++++++ .../scaffolder-backend/src/service/router.ts | 48 ++++++++++------- 2 files changed, 80 insertions(+), 19 deletions(-) diff --git a/plugins/scaffolder-backend/src/service/router.test.ts b/plugins/scaffolder-backend/src/service/router.test.ts index f545e57f85..f769038369 100644 --- a/plugins/scaffolder-backend/src/service/router.test.ts +++ b/plugins/scaffolder-backend/src/service/router.test.ts @@ -267,6 +267,57 @@ describe('createRouter', () => { ); }); + it('should not throw when an invalid authorization header is passed', async () => { + const broker = taskBroker.dispatch as jest.Mocked['dispatch']; + const mockToken = 'blob.eyJzdWIiOiIiLCJuYW1lIjoiSm9obiBEb2UifQ.blob'; + + await request(app) + .post('/v2/tasks') + .set('Authorization', `Bearer ${mockToken}`) + .send({ + templateRef: stringifyEntityRef({ + kind: 'template', + name: 'create-react-app-template', + }), + values: { + required: 'required-value', + }, + }); + expect(broker).toHaveBeenCalledWith( + expect.objectContaining({ + createdBy: undefined, + secrets: { + backstageToken: undefined, + }, + + spec: { + apiVersion: mockTemplate.apiVersion, + steps: mockTemplate.spec.steps.map((step, index) => ({ + ...step, + id: step.id ?? `step-${index + 1}`, + name: step.name ?? step.action, + })), + output: mockTemplate.spec.output ?? {}, + parameters: { + required: 'required-value', + }, + user: { + entity: undefined, + ref: undefined, + }, + templateInfo: { + entityRef: stringifyEntityRef({ + kind: 'Template', + namespace: 'Default', + name: mockTemplate.metadata?.name, + }), + baseUrl: 'https://dev.azure.com', + }, + }, + }), + ); + }); + it('should not decorate a user when no backstage auth is passed', async () => { const broker = taskBroker.dispatch as jest.Mocked['dispatch']; diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index beaa0388dc..2a8f15d967 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -146,7 +146,10 @@ export async function createRouter( '/v2/templates/:namespace/:kind/:name/parameter-schema', async (req, res) => { const { namespace, kind, name } = req.params; - const { token } = parseBearerToken(req.headers.authorization); + const { token } = parseBearerToken({ + header: req.headers.authorization, + logger, + }); const template = await findTemplate({ catalogApi: catalogClient, entityRef: { kind, namespace, name }, @@ -187,17 +190,13 @@ export async function createRouter( const { kind, namespace, name } = parseEntityRef(templateRef, { defaultKind: 'template', }); - const { token, entityRef: userEntityRef } = parseBearerToken( - req.headers.authorization, - ); + const { token, entityRef: userEntityRef } = parseBearerToken({ + header: req.headers.authorization, + logger, + }); const userEntity = userEntityRef - ? await catalogClient - .getEntityByRef(userEntityRef, { token }) - .catch(e => { - logger.error(`Failed to get user entity: ${stringifyError(e)}`); - return undefined; - }) + ? await catalogClient.getEntityByRef(userEntityRef, { token }) : undefined; let auditLog = `Scaffolding task for ${templateRef}`; @@ -241,12 +240,10 @@ export async function createRouter( })), output: template.spec.output ?? {}, parameters: values, - user: userEntity - ? { - entity: userEntity as UserEntity, - ref: userEntityRef, - } - : undefined, + user: { + entity: userEntity as UserEntity, + ref: userEntityRef, + }, templateInfo: { entityRef: stringifyEntityRef({ kind, @@ -396,7 +393,10 @@ export async function createRouter( throw new InputError('Input template is not a template'); } - const { token } = parseBearerToken(req.headers.authorization); + const { token } = parseBearerToken({ + header: req.headers.authorization, + logger, + }); for (const parameters of [template.spec.parameters ?? []].flat()) { const result = validate(body.values, parameters); @@ -447,7 +447,13 @@ export async function createRouter( return app; } -function parseBearerToken(header?: string): { +function parseBearerToken({ + header, + logger, +}: { + header?: string; + logger: Logger; +}): { token?: string; entityRef?: string; } { @@ -479,8 +485,12 @@ function parseBearerToken(header?: string): { throw new TypeError('Expected string sub claim'); } + // Check that it's a valid ref, otherwise this will throw. + parseEntityRef(sub); + return { entityRef: sub, token }; } catch (e) { - throw new InputError(`Invalid authorization header: ${stringifyError(e)}`); + logger.error(`Invalid authorization header: ${stringifyError(e)}`); + return {}; } }