diff --git a/sources/src/develocity/short-lived-token.ts b/sources/src/develocity/short-lived-token.ts index d662fafd..f01e906a 100644 --- a/sources/src/develocity/short-lived-token.ts +++ b/sources/src/develocity/short-lived-token.ts @@ -147,7 +147,6 @@ type HostnameAccessKey = { } export class DevelocityAccessCredentials { - static readonly accessKeyRegexp = /^([^;=\s]+=\w+)(;[^;=\s]+=\w+)*$/ readonly keys: HostnameAccessKey[] private constructor(allKeys: HostnameAccessKey[]) { @@ -160,17 +159,63 @@ export class DevelocityAccessCredentials { private static readonly keyDelimiter = ';' private static readonly hostDelimiter = '=' + private static readonly whitespace = /\s/ + /** + * Parse a `host=key[;host=key]*` access key value. + * + * Only the structure needed to split the value is validated: entries are separated by `;`, and + * each entry is a hostname followed by `=` and a key, where the hostname contains no `=`, `;` or + * whitespace, and the key is non-empty and contains no `;` or whitespace. Nothing else is + * assumed about the key: it may be an OIDC token containing `.`, `-`, `_` and `=` padding, so + * each entry is split on its _first_ `=` only. + * + * Returns `null` if the value doesn't match, emitting a warning that describes what is wrong. + */ static parse(rawKey: string): DevelocityAccessCredentials | null { - if (!this.isValid(rawKey)) { + const trimmedKey = rawKey.trim() + if (!trimmedKey) { return null } - return new DevelocityAccessCredentials( - rawKey.split(this.keyDelimiter).map(hostKey => { - const pair = hostKey.split(this.hostDelimiter) - return {hostname: pair[0], key: pair[1]} - }) + const keys = new Array() + const entries = trimmedKey.split(this.keyDelimiter) + for (const [index, entry] of entries.entries()) { + const separatorIndex = entry.indexOf(this.hostDelimiter) + if (separatorIndex === -1) { + return this.warnBadlyFormed(index, entries.length, `no '${this.hostDelimiter}' separator`) + } + const hostname = entry.substring(0, separatorIndex) + const key = entry.substring(separatorIndex + 1) + if (!hostname) { + return this.warnBadlyFormed(index, entries.length, 'empty server name') + } + if (!key) { + return this.warnBadlyFormed(index, entries.length, 'empty key') + } + if (this.whitespace.test(hostname)) { + return this.warnBadlyFormed(index, entries.length, 'whitespace in the server name') + } + if (this.whitespace.test(key)) { + return this.warnBadlyFormed(index, entries.length, 'whitespace in the key') + } + keys.push({hostname, key}) + } + return new DevelocityAccessCredentials(keys) + } + + /** + * Warn that an access key value is badly formed and cannot be parsed. Reports only the position + * of the offending entry and the reason: the value is a secret, and is not yet registered for + * masking at this point, so no part of it is ever included in the message. + */ + private static warnBadlyFormed(index: number, entryCount: number, reason: string): null { + const location = entryCount > 1 ? `entry ${index + 1} of ${entryCount}` : 'the value' + core.warning( + `Ignoring badly formed Develocity access key: ${reason} in ${location}. ` + + `The expected format is 'server${this.hostDelimiter}key` + + `[${this.keyDelimiter}server${this.hostDelimiter}key]*'.` ) + return null } isEmpty(): boolean { @@ -182,10 +227,6 @@ export class DevelocityAccessCredentials { .map(k => `${k.hostname}${DevelocityAccessCredentials.hostDelimiter}${k.key}`) .join(DevelocityAccessCredentials.keyDelimiter) } - - private static isValid(allKeys: string): boolean { - return this.accessKeyRegexp.test(allKeys) - } } /** diff --git a/sources/test/jest/short-lived-token.test.ts b/sources/test/jest/short-lived-token.test.ts index faca95ac..a4f6af87 100644 --- a/sources/test/jest/short-lived-token.test.ts +++ b/sources/test/jest/short-lived-token.test.ts @@ -1,8 +1,65 @@ import nock from "nock"; -import {describe, expect, it} from '@jest/globals' +import {afterEach, beforeEach, describe, expect, it, jest} from '@jest/globals' import {DevelocityAccessCredentials, getToken, resolveTokenForServer} from "../../src/develocity/short-lived-token"; +describe('access key format warnings', () => { + // `core.warning` is an ESM export and cannot be spied on, so capture the workflow command it + // writes to stdout instead. + let stdout: jest.SpiedFunction + + const warnings = (): string[] => + stdout.mock.calls + .map(call => String(call[0])) + .filter(line => line.startsWith('::warning::')) + .map(line => line.substring('::warning::'.length).trim()) + + beforeEach(() => { + stdout = jest.spyOn(process.stdout, 'write').mockImplementation(() => true) + }) + + afterEach(() => { + stdout.mockRestore() + }) + + it.each([ + ['no separator', 'host1', "no '=' separator in the value"], + ['a trailing separator', 'host1=key1;', "no '=' separator in entry 2 of 2"], + ['a leading separator', ';host1=key1', "no '=' separator in entry 1 of 2"], + ['an empty hostname', '=key1', 'empty server name in the value'], + ['an empty key', 'host1=', 'empty key in the value'], + ['whitespace in the hostname', 'ho st1=key1', 'whitespace in the server name in the value'], + ['whitespace in the key', 'host1=ke y1', 'whitespace in the key in the value'], + ['whitespace around a separator', 'host1=key1; host2=key2', 'whitespace in the server name in entry 2 of 2'], + ])('warns about %s', (_description, rawKey, expectedReason) => { + expect(DevelocityAccessCredentials.parse(rawKey)).toBeNull() + + expect(warnings()).toEqual([ + `Ignoring badly formed Develocity access key: ${expectedReason}. The expected format is 'server=key[;server=key]*'.` + ]) + }) + + it('never includes any part of the access key value in the warning', () => { + expect(DevelocityAccessCredentials.parse('my-host=my sec ret')).toBeNull() + + const message = warnings()[0] + expect(message).not.toContain('my-host') + expect(message).not.toContain('sec') + }) + + it('does not warn for a valid access key', () => { + expect(DevelocityAccessCredentials.parse('host1=key1;host2=key2')).not.toBeNull() + + expect(warnings()).toEqual([]) + }) + + it('does not warn for an empty access key', () => { + expect(DevelocityAccessCredentials.parse(' ')).toBeNull() + + expect(warnings()).toEqual([]) + }) +}) + describe('short lived tokens', () => { it('parse valid access key should return an object', async () => { let develocityAccessCredentials = DevelocityAccessCredentials.parse('some-host.local=key1;host2=key2'); @@ -19,6 +76,53 @@ describe('short lived tokens', () => { expect(develocityAccessCredentials).toBeNull() }) + it('parse access key with an OIDC token value should return an object', async () => { + const oidcToken = 'eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJyZXBvOmZvby9iYXI_-x=.c2lnbmF0dXJl==' + let develocityAccessCredentials = DevelocityAccessCredentials.parse(`some-host.local=${oidcToken}`); + + expect(develocityAccessCredentials).toStrictEqual(DevelocityAccessCredentials.of([ + {hostname: 'some-host.local', key: oidcToken}]) + ) + }) + + it('parse access key splits each entry on the first separator only', async () => { + let develocityAccessCredentials = DevelocityAccessCredentials.parse('host1=a=b==;host2=c=d'); + + expect(develocityAccessCredentials).toStrictEqual(DevelocityAccessCredentials.of([ + {hostname: 'host1', key: 'a=b=='}, + {hostname: 'host2', key: 'c=d'}]) + ) + }) + + it('parse access key tolerates surrounding whitespace', async () => { + let develocityAccessCredentials = DevelocityAccessCredentials.parse(' host1=key1\n'); + + expect(develocityAccessCredentials).toStrictEqual(DevelocityAccessCredentials.of([ + {hostname: 'host1', key: 'key1'}]) + ) + }) + + it.each([ + ['no separator', 'host1'], + ['a trailing separator', 'host1=key1;'], + ['a leading separator', ';host1=key1'], + ['an empty hostname', '=key1'], + ['an empty key', 'host1='], + ['whitespace in the hostname', 'ho st1=key1'], + ['whitespace in the key', 'host1=ke y1'], + ['whitespace around a separator', 'host1=key1; host2=key2'], + ['one invalid entry', 'host1=key1;random'], + ])('parse access key with %s should return null', async (_description, rawKey) => { + expect(DevelocityAccessCredentials.parse(rawKey)).toBeNull() + }) + + it('access key with an OIDC token value as raw string', async () => { + const rawKey = 'host1=eyJhbGciOiJSUzI1NiJ9.payload.signature==;host2=key2' + let develocityAccessCredentials = DevelocityAccessCredentials.parse(rawKey); + + expect(develocityAccessCredentials?.raw()).toBe(rawKey) + }) + it('parse empty access key should return null', async () => { let develocityAccessCredentials = DevelocityAccessCredentials.parse('');