testserver: fail on unexpanded shell/test variables in API requests#5378
Open
pietern wants to merge 1 commit into
Open
testserver: fail on unexpanded shell/test variables in API requests#5378pietern wants to merge 1 commit into
pietern wants to merge 1 commit into
Conversation
The test server now inspects each request's URL and body and fails the
test if it finds an unexpanded uppercase variable reference (${FOO} or
$FOO). Acceptance scripts inject test/env variables by that convention
(UNIQUE_NAME, JOB_ID, ...), while DABs interpolation is lowercase and
namespaced (${resources.jobs.foo.id}), so it is ignored with no allowlist
upkeep. Endpoints carrying variable-shaped content are exempt (file uploads
and /telemetry-ext), and a global allowedDollarTokens escape hatch covers
intentional cases.
This catches the class of bug fixed in #5376, where a ${UNIQUE_NAME} left
inside single quotes was sent to the API literally: locally the fake server
accepted it, so it only failed intermittently on cloud and never showed up
in output.txt.
One fixture that used $VAR-shaped placeholder values
(resources/jobs/instance_pool_and_node_type) now uses plain literals.
Co-authored-by: Isaac
Contributor
Approval status: pending
|
Collaborator
|
Commit: 123301b |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
The test server now inspects each request's URL and body and fails the test if it finds an unexpanded uppercase variable reference (
${FOO}or$FOO). Uppercase-only is deliberate: scripts inject test/env variables by that convention (UNIQUE_NAME,JOB_ID, …), while DABs interpolation is lowercase and namespaced (${resources.jobs.foo.id}), so it's ignored with no allowlist upkeep. Endpoints carrying variable-shaped content are exempt (file uploads and/telemetry-ext), and a globalallowedDollarTokensescape hatch covers intentional cases.One fixture that used
$VAR-shaped placeholder values (resources/jobs/instance_pool_and_node_type) now uses plain literals; its assertion is unchanged.Why
In #5376, a
${UNIQUE_NAME}left inside single quotes in an acceptance script was never expanded, so the literaltest-pipeline-${UNIQUE_NAME}was sent to the API. The fake server accepted it, so the test passed locally and only failed intermittently on cloud — and it never appeared inoutput.txt. This guard turns that class of bug into a deterministic local failure on every PR. The check sits at the existingserve()chokepoint next toINJECT_ERROR, so it runs for every request regardless of engine orRecordRequests.Tests
New table-driven unit tests for the matcher. Full acceptance suite passes in both direct and terraform modes with zero violations.
This pull request and its description were written by Isaac.