diff --git a/apps/runtime/src/config.c b/apps/runtime/src/config.c index 77de48a3..65f0d25a 100644 --- a/apps/runtime/src/config.c +++ b/apps/runtime/src/config.c @@ -31,8 +31,6 @@ * against any of them reads this one and nothing of ours. */ #define PORT_ALIAS "PORT" -/* Only a sigil followed by RUNTIME_PREFIX opens a reference, which is what leaves a - * secret's own '$' alone — see the format contract in config.h. */ #define REFERENCE_SIGIL '$' #define REFERENCE_OPEN '{' #define REFERENCE_CLOSE '}' @@ -316,50 +314,24 @@ struct reference { const char *after; }; -static bool at_reference(const char *text) { - if (*text != REFERENCE_SIGIL) { - return false; - } - const char *name = text + 1; - if (*name == REFERENCE_OPEN) { - name++; - } - return starts_with(name, RUNTIME_PREFIX); -} - -static const char *find_reference(const char *value) { +static const char *find_reference(const char *value, struct reference *out) { for (const char *cursor = strchr(value, REFERENCE_SIGIL); cursor != NULL; cursor = strchr(cursor + 1, REFERENCE_SIGIL)) { - if (at_reference(cursor)) { - return cursor; + if (cursor[1] != REFERENCE_OPEN || !starts_with(cursor + 2, RUNTIME_PREFIX)) { + continue; } - } - return NULL; -} - -/* `text` starts at a sigil at_reference has already accepted, so the only way this - * fails is a braced form nobody closed — a typo rather than a value meant literally. */ -static bool read_reference(const char *text, struct reference *out) { - const char *cursor = text + 1; - bool braced = *cursor == REFERENCE_OPEN; - if (braced) { - cursor++; - } - out->name = cursor; - while (is_name_character(*cursor)) { - cursor++; - } - out->length = (size_t)(cursor - out->name); - if (braced) { - if (*cursor != REFERENCE_CLOSE) { - log_line("a tenant variable names %.*s with no closing '%c'", (int)out->length, out->name, - REFERENCE_CLOSE); - return false; + const char *name = cursor + 2; + const char *end = name; + while (is_name_character(*end)) { + end++; + } + if (*end != REFERENCE_CLOSE) { + continue; } - cursor++; + *out = (struct reference){.name = name, .length = (size_t)(end - name), .after = end + 1}; + return cursor; } - out->after = cursor; - return true; + return NULL; } static bool names_key(const struct reference *reference, const char *key) { @@ -430,25 +402,19 @@ static void append(struct arena *arena, const char *text, size_t length) { * which is almost every one of them. */ static bool expand_entry(const struct instance_config *config, struct arena *arena, char **entry) { const char *value = strchr(*entry, '=') + 1; - if (find_reference(value) == NULL) { + struct reference reference; + const char *found = find_reference(value, &reference); + if (found == NULL) { return true; } char *expansion = arena->cursor; append(arena, *entry, (size_t)(value - *entry)); - for (const char *remaining = value;;) { - const char *found = find_reference(remaining); - if (found == NULL) { - append(arena, remaining, strlen(remaining)); - break; - } + const char *remaining = value; + while (found != NULL) { append(arena, remaining, (size_t)(found - remaining)); - struct reference reference; - if (!read_reference(found, &reference)) { - return false; - } /* uint32_t's whole range rather than the port's: anything narrower is a * truncation the compiler is right to refuse. */ char rendered[sizeof("4294967295")]; @@ -465,8 +431,9 @@ static bool expand_entry(const struct instance_config *config, struct arena *are } append(arena, substitution, strlen(substitution)); remaining = reference.after; + found = find_reference(remaining, &reference); } - append(arena, "", 1); + append(arena, remaining, strlen(remaining) + 1); if (arena->overflowed) { log_line("instance.env expands to more than %d bytes", CONFIG_MAX_EXPANDED_BYTES); diff --git a/apps/runtime/src/config.h b/apps/runtime/src/config.h index 6b47ddc5..0ab1f339 100644 --- a/apps/runtime/src/config.h +++ b/apps/runtime/src/config.h @@ -15,13 +15,11 @@ * called NIBRUN_HTTP_PORT arrives as ENV_NIBRUN_HTTP_PORT and is read as the * tenant's, which is what lets config_build_environment drop it on its own terms. * - * A tenant value may name a runtime one it is handed: `$NIBRUN_HTTP_PORT` and - * `${NIBRUN_HTTP_PORT}` both expand, and a name this runtime does not offer fails the - * boot rather than reaching the tenant as itself. Nothing else expands, so a secret - * holding `$`, `$$` or `$HOME` arrives byte for byte — the prefix is what keeps the - * substitution off values it was never meant for. The cost is that a value holding a - * literal `$NIBRUN_` has no representation, which is the bargain the format already - * makes for one holding a newline. + * Only complete `${NIBRUN_NAME}` references expand in tenant values. A complete + * reference to a name this runtime does not offer fails the boot. Bare names, + * unmatched braces and every other `$` remain literal, so secrets survive byte for + * byte unless they contain a complete runtime reference. There is no escape syntax + * for a literal complete runtime reference. * * The runtime carries no defaults for any of it. DEFAULT_RESTART_POLICY in * packages/protocol is the only place those values exist; the agent resolves them diff --git a/apps/runtime/tests/test-config.c b/apps/runtime/tests/test-config.c index 3a927688..f9fc789f 100644 --- a/apps/runtime/tests/test-config.c +++ b/apps/runtime/tests/test-config.c @@ -143,7 +143,7 @@ static void expands_the_public_address(void) { EXPECT(parse(&config, REQUIRED "NIBRUN_PUBLIC_IPV4=203.0.113.7\n" "NIBRUN_EXTRA_PUBLIC_PORT=22003\n" "ENV_ANNOUNCED=${NIBRUN_PUBLIC_IPV4}:${NIBRUN_EXTRA_PUBLIC_PORT}\n" - "ENV_RTC_PORT=$NIBRUN_EXTRA_PUBLIC_PORT\n")); + "ENV_RTC_PORT=${NIBRUN_EXTRA_PUBLIC_PORT}\n")); char *const *environment = config_build_environment(&config); EXPECT(strcmp(value_of(environment, "ANNOUNCED"), "203.0.113.7:22003") == 0); EXPECT(strcmp(value_of(environment, "RTC_PORT"), "22003") == 0); @@ -168,18 +168,17 @@ static void expands_a_runtime_reference(void) { EXPECT(parse(&config, REQUIRED "NIBRUN_HOSTNAME=my-app.nibrun.app\n" "ENV_BARE=$NIBRUN_HTTP_PORT\n" "ENV_BRACED=${NIBRUN_HTTP_PORT}\n" - "ENV_WITHIN=http://$NIBRUN_HOSTNAME:${NIBRUN_HTTP_PORT}/health\n" + "ENV_WITHIN=http://${NIBRUN_HOSTNAME}:${NIBRUN_HTTP_PORT}/health\n" "ENV_UNDER_THE_VOLUME=${NIBRUN_DATA_DIR}/state.db\n" - "ENV_TWICE=$NIBRUN_HTTP_PORT-$NIBRUN_HTTP_PORT\n" + "ENV_TWICE=${NIBRUN_HTTP_PORT}-${NIBRUN_HTTP_PORT}\n" "ENV_ADJACENT=${NIBRUN_HTTP_PORT}0\n")); char *const *environment = config_build_environment(&config); - EXPECT(strcmp(value_of(environment, "BARE"), "8080") == 0); + EXPECT(strcmp(value_of(environment, "BARE"), "$NIBRUN_HTTP_PORT") == 0); EXPECT(strcmp(value_of(environment, "BRACED"), "8080") == 0); EXPECT(strcmp(value_of(environment, "WITHIN"), "http://my-app.nibrun.app:8080/health") == 0); EXPECT(strcmp(value_of(environment, "UNDER_THE_VOLUME"), "/app/data/state.db") == 0); EXPECT(strcmp(value_of(environment, "TWICE"), "8080-8080") == 0); - /* Braces are the whole reason there are two forms: without them this names PORT0. */ EXPECT(strcmp(value_of(environment, "ADJACENT"), "80800") == 0); } @@ -199,19 +198,42 @@ static void leaves_every_other_dollar_alone(void) { EXPECT(strcmp(value_of(environment, "TRAILING"), "the cost is $") == 0); } +static void preserves_incomplete_references(void) { + const char *literals[] = { + "$", "{", "}", "${", "${}", "$NIBRUN_HTTP_PORT", "$NIBRUN_NOTHING", + "$NIBRUN_PUBLIC_IPV4", "$NIBRUN_EXTRA_PUBLIC_PORT", "${NIBRUN_HTTP_PORT", + "${NIBRUN_NOTHING", "{NIBRUN_HTTP_PORT}", "${NIBRUN_HTTP_PORT!}", + "${NIBRUN_HTTP_PORT with spaces}", "secret$NIBRUN_HTTP_PORT}suffix", + }; + for (size_t index = 0; index < sizeof(literals) / sizeof(literals[0]); index++) { + char text[CONFIG_MAX_BYTES]; + snprintf(text, sizeof(text), "%sENV_SECRET=%s\n", REQUIRED, literals[index]); + struct instance_config config; + EXPECT(parse(&config, text)); + EXPECT(strcmp(value_of(config_build_environment(&config), "SECRET"), literals[index]) == 0); + } +} + +static void expands_complete_references_among_literal_characters(void) { + struct instance_config config; + EXPECT(parse(&config, REQUIRED "ENV_MIXED=$${NIBRUN_HTTP_PORT}|{${NIBRUN_DATA_DIR}}|" + "${NIBRUN_BROKEN:${NIBRUN_HTTP_PORT}|$\n")); + EXPECT(strcmp(value_of(config_build_environment(&config), "MIXED"), + "$8080|{/app/data}|${NIBRUN_BROKEN:8080|$") == 0); +} + /* A reference nobody can answer fails the boot rather than reaching the tenant as * itself, where it would read as a value somebody meant to write. */ static void rejects_a_reference_it_cannot_answer(void) { - EXPECT(rejects(REQUIRED "ENV_A=$NIBRUN_NOTHING\n")); - EXPECT(rejects(REQUIRED "ENV_A=$NIBRUN_HTTP_PORT0\n")); - EXPECT(rejects(REQUIRED "ENV_A=${NIBRUN_HTTP_PORT\n")); + EXPECT(rejects(REQUIRED "ENV_A=${NIBRUN_NOTHING}\n")); + EXPECT(rejects(REQUIRED "ENV_A=${NIBRUN_HTTP_PORT0}\n")); /* Offered, but this instance was issued no hostname. */ - EXPECT(rejects(REQUIRED "ENV_A=$NIBRUN_HOSTNAME\n")); + EXPECT(rejects(REQUIRED "ENV_A=${NIBRUN_HOSTNAME}\n")); /* Likewise for an app that asked for no port. */ - EXPECT(rejects(REQUIRED "ENV_A=$NIBRUN_PUBLIC_IPV4\n")); - EXPECT(rejects(REQUIRED "ENV_A=$NIBRUN_EXTRA_PUBLIC_PORT\n")); + EXPECT(rejects(REQUIRED "ENV_A=${NIBRUN_PUBLIC_IPV4}\n")); + EXPECT(rejects(REQUIRED "ENV_A=${NIBRUN_EXTRA_PUBLIC_PORT}\n")); /* The supervisor's own, and never handed to a tenant. */ - EXPECT(rejects(REQUIRED "ENV_A=$NIBRUN_MAX_RESTARTS\n")); + EXPECT(rejects(REQUIRED "ENV_A=${NIBRUN_MAX_RESTARTS}\n")); } static void rejects_an_expansion_that_does_not_fit(void) { @@ -417,6 +439,8 @@ int main(void) { drops_a_tenant_public_address(); expands_a_runtime_reference(); leaves_every_other_dollar_alone(); + preserves_incomplete_references(); + expands_complete_references_among_literal_characters(); rejects_a_reference_it_cannot_answer(); rejects_an_expansion_that_does_not_fit(); rejects_a_broken_file(); diff --git a/packages/protocol/src/domain/app.ts b/packages/protocol/src/domain/app.ts index 4a28e1ec..6205fcad 100644 --- a/packages/protocol/src/domain/app.ts +++ b/packages/protocol/src/domain/app.ts @@ -87,15 +87,13 @@ const OFFERED = RUNTIME_VALUE_NAMES.join('|'); const NEEDS_A_PORT = EXTRA_PUBLIC_PORT_VALUES.map((value) => value.name).join('|'); const NAME_CHARACTER = '[A-Za-z0-9_]'; -// A value as the guest reads it: anything but a `$`, a `$` that opens no reference — which is what -// leaves a bcrypt hash and a literal `$HOME` alone — and the two forms that expand. A name the -// guest would refuse matches none of them, so it has no way through. +// Match the guest's complete reference syntax so secrets with bare names or unmatched +// braces remain literal. Complete references to unavailable names still fail validation. const TENANT_VALUE_PATTERN = [ '^(?:', '[^$]', - `|\\$(?!\\{?${RUNTIME_VALUE_PREFIX})`, + `|\\$(?!\\{${RUNTIME_VALUE_PREFIX}${NAME_CHARACTER}*\\})`, `|\\$\\{(?:${OFFERED})\\}`, - `|\\$(?:${OFFERED})(?!${NAME_CHARACTER})`, ')*$', ].join(''); @@ -115,12 +113,8 @@ export function interpolableRuntimeValue(name: string): string { return `\${${name}}`; } -// Both forms that expand, and only the names an app has to have asked for. Not a schema pattern -// like the one above: whether this is allowed depends on the config beside it, which is not -// something a value can be validated against on its own. -const NAMES_A_PORT = new RegExp( - `\\$(?:\\{(?:${NEEDS_A_PORT})\\}|(?:${NEEDS_A_PORT})(?!${NAME_CHARACTER}))`, -); +// Whether these references are allowed depends on the app's public-port config. +const NAMES_A_PORT = new RegExp(`\\$\\{(?:${NEEDS_A_PORT})\\}`); /** Whether `value` names a runtime value only an app with an extra public port is given. */ export function namesExtraPublicPortValues(value: string): boolean { diff --git a/packages/protocol/tests/protocol.test.ts b/packages/protocol/tests/protocol.test.ts index 6fee7b97..31073d84 100644 --- a/packages/protocol/tests/protocol.test.ts +++ b/packages/protocol/tests/protocol.test.ts @@ -378,10 +378,8 @@ describe('a variable named __proto__ is not one', () => { * the only end of this where whoever wrote it is still listening. */ describe('a value naming a runtime value', () => { - // biome-ignore lint/suspicious/noTemplateCurlyInString: the syntax being validated, not an interpolation - const OFFERED = '${NIBRUN_HOSTNAME}'; - // biome-ignore lint/suspicious/noTemplateCurlyInString: the syntax being validated, not an interpolation - const MISSPELLED = '${NIBRUN_HSOTNAME}'; + const OFFERED = `\${NIBRUN_HOSTNAME}`; + const MISSPELLED = `\${NIBRUN_HSOTNAME}`; function holding(value: string) { return { CALLBACK_URL: value }; @@ -391,28 +389,46 @@ describe('a value naming a runtime value', () => { return isValidMessage({ schema: TenantEnvironmentSchema, value: holding(value) }); } - test('both forms the guest expands are accepted', () => { + test('complete runtime references are accepted', () => { expect(accepts(`https://${OFFERED}/callback`)).toBe(true); - expect(accepts('$NIBRUN_HTTP_PORT')).toBe(true); + expect(accepts(`\${NIBRUN_HTTP_PORT}`)).toBe(true); }); test('a name the guest does not offer is refused', () => { expect(accepts(`https://${MISSPELLED}/callback`)).toBe(false); - expect(accepts('$NIBRUN_HTTP_PORTS')).toBe(false); + expect(accepts(`\${NIBRUN_HTTP_PORTS}`)).toBe(false); }); - // The guest reads a name to its last name character, so this one is NIBRUN_HOSTNAME with no - // closing brace rather than the value someone meant. - test('a brace nobody closed is refused', () => { - expect(accepts('https://${NIBRUN_HOSTNAME')).toBe(false); + test('a brace nobody closed is literal', () => { + expect(accepts(`https://\${NIBRUN_HOSTNAME`)).toBe(true); }); - // The prefix is the whole of what expands, which is what lets a secret hold a `$` at all: a - // bcrypt hash and a password that reads like a shell variable are values like any other. test('a $ that opens no reference is a $', () => { expect(accepts('$2y$10$K3JqBQ8Rt7uVwXyZaBcDeF')).toBe(true); expect(accepts('$HOME/bin')).toBe(true); - expect(accepts('$$')).toBe(true); + for (const literal of [ + '$', + '{', + '}', + `\${`, + `\${}`, + '$NIBRUN_HTTP_PORT', + '$NIBRUN_NOTHING', + '$NIBRUN_PUBLIC_IPV4', + '$NIBRUN_EXTRA_PUBLIC_PORT', + `\${NIBRUN_NOTHING`, + '{NIBRUN_HTTP_PORT}', + `\${NIBRUN_HTTP_PORT!}`, + `\${NIBRUN_HTTP_PORT with spaces}`, + 'secret$NIBRUN_HTTP_PORT}suffix', + ]) { + expect(accepts(literal)).toBe(true); + expect( + isValidMessage({ schema: TenantEnvironmentPatchSchema, value: holding(literal) }), + ).toBe(true); + } + expect(accepts(`$${OFFERED}|{${OFFERED}}|\${NIBRUN_BROKEN:${OFFERED}|$`)).toBe(true); + expect(accepts(`\${NIBRUN_BROKEN:${MISSPELLED}`)).toBe(false); }); test('an edit is held to the same rule', () => { @@ -430,30 +446,34 @@ describe('a value naming a runtime value', () => { * a value is allowed depends on the config beside it — so it is a question rather than a pattern. */ describe('a value naming a runtime value only some apps are given', () => { - test('either name, in either form', () => { - // biome-ignore lint/suspicious/noTemplateCurlyInString: the syntax being validated - expect(namesExtraPublicPortValues('${NIBRUN_PUBLIC_IPV4}')).toBe(true); - expect(namesExtraPublicPortValues('$NIBRUN_EXTRA_PUBLIC_PORT')).toBe(true); - expect(namesExtraPublicPortValues('udp://$NIBRUN_PUBLIC_IPV4:$NIBRUN_EXTRA_PUBLIC_PORT')).toBe( + test('either name in a complete reference', () => { + expect(namesExtraPublicPortValues(`\${NIBRUN_PUBLIC_IPV4}`)).toBe(true); + expect(namesExtraPublicPortValues(`\${NIBRUN_EXTRA_PUBLIC_PORT}`)).toBe(true); + expect(namesExtraPublicPortValues(`\${NIBRUN_PUBLIC_IPV4}:\${NIBRUN_EXTRA_PUBLIC_PORT}`)).toBe( true, ); }); test('a name every app is given is not one of them', () => { expect(namesExtraPublicPortValues('$NIBRUN_HOSTNAME')).toBe(false); - // biome-ignore lint/suspicious/noTemplateCurlyInString: the syntax being validated - expect(namesExtraPublicPortValues('${NIBRUN_HTTP_PORT}')).toBe(false); + expect(namesExtraPublicPortValues(`\${NIBRUN_HTTP_PORT}`)).toBe(false); }); - // The guest reads a name to its last name character, so this is a longer name it does not offer - // rather than one of these with something after it. test('a longer name is a different name', () => { - expect(namesExtraPublicPortValues('$NIBRUN_PUBLIC_IPV4X')).toBe(false); + expect(namesExtraPublicPortValues(`\${NIBRUN_PUBLIC_IPV4X}`)).toBe(false); }); test('a value naming nothing names none of them', () => { expect(namesExtraPublicPortValues('$2y$10$K3JqBQ8Rt7uVwXyZaBcDeF')).toBe(false); - expect(namesExtraPublicPortValues('NIBRUN_PUBLIC_IPV4')).toBe(false); + for (const literal of [ + 'NIBRUN_PUBLIC_IPV4', + '$NIBRUN_PUBLIC_IPV4', + '$NIBRUN_EXTRA_PUBLIC_PORT', + `\${NIBRUN_PUBLIC_IPV4`, + `\${NIBRUN_EXTRA_PUBLIC_PORT!}`, + ]) { + expect(namesExtraPublicPortValues(literal)).toBe(false); + } }); }); diff --git a/skills/deploy-to-nibrun/SKILL.md b/skills/deploy-to-nibrun/SKILL.md index c7d0e405..14bb82c0 100644 --- a/skills/deploy-to-nibrun/SKILL.md +++ b/skills/deploy-to-nibrun/SKILL.md @@ -247,9 +247,10 @@ it uses when it is not on nibrun. A binary that insists on a variable name of its own reaches the same values through it — `APP_BASE_URL=https://${NIBRUN_HOSTNAME}`, `DATABASE_URL=file:${NIBRUN_DATA_DIR}/app.db` — and the -guest expands it before exec. Only the `NIBRUN_` names above expand, and only those: a secret -holding a `$` arrives untouched, `${PORT}` is not one of them, and anything else is refused when -you deploy it. +guest expands it before exec. Only complete `${NIBRUN_NAME}` references to the names above +expand. Bare names such as `$NIBRUN_HTTP_PORT`, unmatched braces, and other dollar signs remain +literal. A complete reference to an unknown `NIBRUN_` name is refused when you deploy it; +`${PORT}` remains literal. ## A second public port