fix: escape control characters when substituting multiline params - #2103
Open
pujitha24 wants to merge 1 commit into
Open
fix: escape control characters when substituting multiline params#2103pujitha24 wants to merge 1 commit into
pujitha24 wants to merge 1 commit into
Conversation
Motivation: TriggerBinding param values containing literal newlines (e.g. from a YAML `|` block scalar) broke resource creation with "couldn't unmarshal json from the TriggerTemplate: invalid character '\n' in string literal". applyParamToResourceTemplate substituted $(tt.params.NAME) tokens with the raw param value via a plain bytes.ReplaceAll, with no escaping of control characters. A literal newline (or tab, carriage return, etc.) landing inside a JSON string literal is invalid per RFC 8259, regardless of where the value came from. Approach: Replace the per-param bytes.ReplaceAll with a single linear scan over the resource template that tracks whether the current position is inside a JSON string literal (a minimal quote/backslash-escape tracker). When a $(tt.params.NAME) token is found inside a string, its value is substituted with control characters (\n, \r, \t, and other bytes < 0x20) escaped to valid JSON escape sequences via a new escapeJSONControlChars helper. Outside of a string (e.g. a raw JSON object substituted unquoted), the value is left untouched, preserving the behavior intentionally introduced by a prior PR that dropped automatic quote-escaping so raw JSON blobs could be embedded directly. Quote and backslash pass-through (both by default and under the triggers.tekton.dev/old-escape-quotes annotation) is unchanged. All params are now substituted together in one pass over the original template bytes (substituteParamsInResourceTemplate), rather than one pass per param feeding into the next. An earlier iteration of this fix ran one bytes.ReplaceAll-style pass per param; re-scanning the already-substituted output for each subsequent param let an earlier param's raw, unescaped quote (still pass-through by design) desync the string-boundary tracking used to decide whether a later param's control characters needed escaping, which could silently reproduce the original bug depending on param ordering. Scanning the original template exactly once for all params avoids this, since no substituted value is ever fed back through the tracker. This does not change the pre-existing behavior where a raw, unescaped quote in a param value can still produce structurally invalid JSON (default behavior since the quote-escaping removal); that is a separate, previously-accepted design tradeoff, not something this fix alters. Validation: - go build ./... - go test ./... (full repo; all packages pass, no failures) - gofmt -l pkg/template/resource.go pkg/template/resource_test.go (clean) - make golangci-lint (3 pre-existing issues remain in pkg/sink/sink.go and pkg/bootstrap/file_helper.go, confirmed present without this change via git stash; zero issues in the changed files) - Added unit tests in pkg/template/resource_test.go reproducing the exact multiline-value-in-a-script-field scenario from the report, an unquoted raw-JSON-substitution case confirming that behavior is unchanged, and a multi-param case confirming an earlier param's unescaped quote no longer affects a later param's control-character escaping. Report: tektoncd#1782 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
tekton-robot
added
the
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
tekton-robot
added
the
size/L
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2103 +/- ##
==========================================
+ Coverage 32.17% 32.40% +0.22%
==========================================
Files 260 260
Lines 11979 12030 +51
==========================================
+ Hits 3854 3898 +44
- Misses 7800 7806 +6
- Partials 325 326 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
/assign @khrm |
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
3 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
Fixes a bug where
TriggerBindingparam values containing literal control characters (e.g. newlines from a YAML|block scalar) broke resource creation with acouldn't unmarshal json from the TriggerTemplate: invalid character '\n' in string literalerror.applyParamToResourceTemplatesubstituted$(tt.params.NAME)tokens via a plainbytes.ReplaceAllwith no escaping of control characters, which is invalid inside a JSON string literal per RFC 8259.This replaces the per-param
bytes.ReplaceAllwith a single linear scan over the resource template (substituteParamsInResourceTemplate) that tracks whether the current position is inside a JSON string literal via a minimal quote/backslash-escape tracker. Substitutions inside a string literal now escape control characters (\n,\r,\t, and other bytes <0x20) to valid JSON escape sequences via a newescapeJSONControlCharshelper. Substitutions outside a string literal (e.g. raw JSON blobs) are left untouched, preserving prior intentional behavior. All params are now substituted in one pass over the original template bytes rather than one pass per param, since re-scanning already-substituted output per param could let an earlier param's unescaped quote desync the string-boundary tracking for a later param.Quote and backslash pass-through behavior (default, and under the
triggers.tekton.dev/old-escape-quotesannotation) is unchanged, as is the pre-existing tradeoff where a raw unescaped quote in a param value can still produce invalid JSON.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
AI assistance: this change was drafted with Claude Code.
Fixes #1782