← back to Cli Printing Press
fix(cli): close ResolveByName json_extract injection + dedupe cacheKey AuthHeader call (#1278)
e21feae6854f272d8a1d3c12d581da49ec739fb9 · 2026-05-13 14:08:38 -0700 · Trevin Chow
* fix(cli): close ResolveByName json_extract injection + dedupe cacheKey AuthHeader call
Validate matchFields against validIdentifierRE before splicing into the
json_extract path. SQLite's json_extract path components cannot be bound
as parameters, so the existing fmt.Sprintf splice is structurally
required - but the call site accepted any caller-supplied string. Fields
that fail validation are now skipped, matching ListField's defense
pattern so callers may pass untrusted strings without risk.
cacheKey() captured AuthHeader() into authHeader for the empty check
but then called AuthHeader() a second time to hash. Reuse the captured
value to avoid the duplicate call (and the second-call observation
surface).
Both findings flagged by Greptile on printing-press-library#416.
ListIDs splice already closed in #1000; this finishes the same review's
remaining template-level items.
Refs #1249
* test(cli): update goldens for ResolveByName guard and cacheKey dedupe
Three client.go fixtures pick up the authHeader-reuse change; one
store.go fixture picks up the validIdentifierRE guard and its comment
update.
* test(cli): regression tests for cacheKey reuse and ResolveByName field guard
Update TestClientCacheKeyScopesByBaseURLAndAuthIdentity to assert the
captured-authHeader reuse pattern (and forbid the duplicate-call
pattern). Add TestStoreResolveByNameValidatesField to pin the
validIdentifierRE guard on the json_extract path splice. Both tests
fail on the pre-fix templates.
Refs #1249
* docs(cli): trim ResolveByName doc-comment to the non-obvious why
Drop the second sentence ("Fields that fail validation are skipped so
callers may pass untrusted strings without risk") — it restates what
the three lines of code below it already say. Keep the first sentence;
it explains why fmt.Sprintf is structurally required (json_extract
path components cannot be bound).
Same simplification on the regression-test doc-comment for the same
reason.
* test(cli): update sync-walker golden for trimmed ResolveByName doc-comment
* test(cli): tighten ResolveByName guard assertion to the validIdentifierRE+continue pattern
The standalone \`require.Contains(body, "continue", ...)\` assertion
passed on the pre-fix template too — the function body already
contained \`continue\` inside the \`if err != nil { continue }\`
query-error branch. Switch to a regex that anchors \`continue\` to the
validIdentifierRE guard block specifically, so the assertion only
passes when the new security guard is present.
Addresses Greptile review comment on PR #1278.
Files touched
M internal/generator/client_cache_scope_test.goA internal/generator/store_resolve_by_name_guard_test.goM internal/generator/templates/client.go.tmplM internal/generator/templates/store.go.tmplM testdata/golden/expected/generate-golden-api-oauth2-cc/printing-press-oauth2-cc/internal/client/client.goM testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.goM testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.goM testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/client/client.go
Diff
commit e21feae6854f272d8a1d3c12d581da49ec739fb9
Author: Trevin Chow <trevin@trevinchow.com>
Date: Wed May 13 14:08:38 2026 -0700
fix(cli): close ResolveByName json_extract injection + dedupe cacheKey AuthHeader call (#1278)
* fix(cli): close ResolveByName json_extract injection + dedupe cacheKey AuthHeader call
Validate matchFields against validIdentifierRE before splicing into the
json_extract path. SQLite's json_extract path components cannot be bound
as parameters, so the existing fmt.Sprintf splice is structurally
required - but the call site accepted any caller-supplied string. Fields
that fail validation are now skipped, matching ListField's defense
pattern so callers may pass untrusted strings without risk.
cacheKey() captured AuthHeader() into authHeader for the empty check
but then called AuthHeader() a second time to hash. Reuse the captured
value to avoid the duplicate call (and the second-call observation
surface).
Both findings flagged by Greptile on printing-press-library#416.
ListIDs splice already closed in #1000; this finishes the same review's
remaining template-level items.
Refs #1249
* test(cli): update goldens for ResolveByName guard and cacheKey dedupe
Three client.go fixtures pick up the authHeader-reuse change; one
store.go fixture picks up the validIdentifierRE guard and its comment
update.
* test(cli): regression tests for cacheKey reuse and ResolveByName field guard
Update TestClientCacheKeyScopesByBaseURLAndAuthIdentity to assert the
captured-authHeader reuse pattern (and forbid the duplicate-call
pattern). Add TestStoreResolveByNameValidatesField to pin the
validIdentifierRE guard on the json_extract path splice. Both tests
fail on the pre-fix templates.
Refs #1249
* docs(cli): trim ResolveByName doc-comment to the non-obvious why
Drop the second sentence ("Fields that fail validation are skipped so
callers may pass untrusted strings without risk") — it restates what
the three lines of code below it already say. Keep the first sentence;
it explains why fmt.Sprintf is structurally required (json_extract
path components cannot be bound).
Same simplification on the regression-test doc-comment for the same
reason.
* test(cli): update sync-walker golden for trimmed ResolveByName doc-comment
* test(cli): tighten ResolveByName guard assertion to the validIdentifierRE+continue pattern
The standalone \`require.Contains(body, "continue", ...)\` assertion
passed on the pre-fix template too — the function body already
contained \`continue\` inside the \`if err != nil { continue }\`
query-error branch. Switch to a regex that anchors \`continue\` to the
validIdentifierRE guard block specifically, so the assertion only
passes when the new security guard is present.
Addresses Greptile review comment on PR #1278.
---
internal/generator/client_cache_scope_test.go | 4 ++-
.../generator/store_resolve_by_name_guard_test.go | 41 ++++++++++++++++++++++
internal/generator/templates/client.go.tmpl | 2 +-
internal/generator/templates/store.go.tmpl | 7 ++++
.../internal/client/client.go | 2 +-
.../internal/client/client.go | 2 +-
.../sync-walker-golden/internal/store/store.go | 7 ++++
.../tier-routing-golden/internal/client/client.go | 2 +-
8 files changed, 62 insertions(+), 5 deletions(-)
diff --git a/internal/generator/client_cache_scope_test.go b/internal/generator/client_cache_scope_test.go
index 817ad17d..012db905 100644
--- a/internal/generator/client_cache_scope_test.go
+++ b/internal/generator/client_cache_scope_test.go
@@ -30,7 +30,9 @@ func TestClientCacheKeyScopesByBaseURLAndAuthIdentity(t *testing.T) {
require.Contains(t, body, `"|base_url=" + c.BaseURL`, "cache keys must isolate staging/prod or per-tenant base URLs")
require.Contains(t, body, `"|auth_source=" + c.Config.AuthSource`, "cache keys should distinguish env/config/profile auth sources")
- require.Contains(t, body, `sha256.Sum256([]byte(c.Config.AuthHeader()))`, "cache keys should include an auth fingerprint without storing the raw token")
+ require.Contains(t, body, `authHeader := c.Config.AuthHeader()`, "cache keys should capture AuthHeader() once")
+ require.Contains(t, body, `sha256.Sum256([]byte(authHeader))`, "cache keys should include an auth fingerprint without storing the raw token")
+ require.NotContains(t, body, `sha256.Sum256([]byte(c.Config.AuthHeader()))`, "cache keys should reuse the captured authHeader, not call AuthHeader() twice")
require.Contains(t, body, `sort.Strings(paramKeys)`, "cache keys should be deterministic for map params")
}
diff --git a/internal/generator/store_resolve_by_name_guard_test.go b/internal/generator/store_resolve_by_name_guard_test.go
new file mode 100644
index 00000000..cfb32d70
--- /dev/null
+++ b/internal/generator/store_resolve_by_name_guard_test.go
@@ -0,0 +1,41 @@
+package generator
+
+import (
+ "os"
+ "path/filepath"
+ "regexp"
+ "strings"
+ "testing"
+
+ "github.com/stretchr/testify/require"
+)
+
+func TestStoreResolveByNameValidatesField(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("resolve-guard")
+ outputDir := filepath.Join(t.TempDir(), "resolve-guard-pp-cli")
+ require.NoError(t, New(apiSpec, outputDir).Generate())
+
+ storeSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "store", "store.go"))
+ require.NoError(t, err)
+ store := string(storeSrc)
+ body := resolveByNameBody(t, store)
+
+ require.Contains(t, body, `for _, field := range matchFields {`,
+ "ResolveByName must iterate matchFields")
+ guard := regexp.MustCompile(`if !validIdentifierRE\.MatchString\(field\) \{\s*continue\s*\}`)
+ require.Regexp(t, guard, body,
+ "ResolveByName must validate each field name and continue past invalid entries before splicing into the json_extract path; the continue must be inside the validIdentifierRE guard, not the pre-existing query-error continue")
+}
+
+func resolveByNameBody(t *testing.T, content string) string {
+ t.Helper()
+ start := strings.Index(content, "func (s *Store) ResolveByName(")
+ require.NotEqual(t, -1, start, "ResolveByName function must be emitted")
+ body := content[start:]
+ if next := strings.Index(body[1:], "\nfunc "); next != -1 {
+ body = body[:next+1]
+ }
+ return body
+}
diff --git a/internal/generator/templates/client.go.tmpl b/internal/generator/templates/client.go.tmpl
index 763de5a4..59023d2d 100644
--- a/internal/generator/templates/client.go.tmpl
+++ b/internal/generator/templates/client.go.tmpl
@@ -397,7 +397,7 @@ func (c *Client) cacheKey(path string, params map[string]string) string {
if c.Config != nil {
key += "|auth_source=" + c.Config.AuthSource
if authHeader := c.Config.AuthHeader(); authHeader != "" {
- authHash := sha256.Sum256([]byte(c.Config.AuthHeader()))
+ authHash := sha256.Sum256([]byte(authHeader))
key += "|auth=" + hex.EncodeToString(authHash[:8])
}
if c.Config.Path != "" {
diff --git a/internal/generator/templates/store.go.tmpl b/internal/generator/templates/store.go.tmpl
index 89afcce4..72e4d0ea 100644
--- a/internal/generator/templates/store.go.tmpl
+++ b/internal/generator/templates/store.go.tmpl
@@ -1221,6 +1221,10 @@ func (s *Store) Status() (map[string]int, error) {
// ResolveByName resolves a human-readable name to a UUID from synced data.
// If the input is already a UUID, it is returned as-is.
// matchFields are JSON field names to search against (e.g., "name", "key", "email").
+//
+// json_extract path components cannot be bound as SQL parameters, so each
+// field is validated against validIdentifierRE before being spliced into
+// the query.
func (s *Store) ResolveByName(resourceType string, input string, matchFields ...string) (string, error) {
if IsUUID(input) {
return input, nil
@@ -1228,6 +1232,9 @@ func (s *Store) ResolveByName(resourceType string, input string, matchFields ...
var matches []string
for _, field := range matchFields {
+ if !validIdentifierRE.MatchString(field) {
+ continue
+ }
query := fmt.Sprintf(
`SELECT id FROM resources WHERE resource_type = ? AND LOWER(json_extract(data, '$.%s')) = LOWER(?)`,
field,
diff --git a/testdata/golden/expected/generate-golden-api-oauth2-cc/printing-press-oauth2-cc/internal/client/client.go b/testdata/golden/expected/generate-golden-api-oauth2-cc/printing-press-oauth2-cc/internal/client/client.go
index 6ff99176..2ec802ce 100644
--- a/testdata/golden/expected/generate-golden-api-oauth2-cc/printing-press-oauth2-cc/internal/client/client.go
+++ b/testdata/golden/expected/generate-golden-api-oauth2-cc/printing-press-oauth2-cc/internal/client/client.go
@@ -101,7 +101,7 @@ func (c *Client) cacheKey(path string, params map[string]string) string {
if c.Config != nil {
key += "|auth_source=" + c.Config.AuthSource
if authHeader := c.Config.AuthHeader(); authHeader != "" {
- authHash := sha256.Sum256([]byte(c.Config.AuthHeader()))
+ authHash := sha256.Sum256([]byte(authHeader))
key += "|auth=" + hex.EncodeToString(authHash[:8])
}
if c.Config.Path != "" {
diff --git a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.go b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.go
index 15dd85d3..4ef66b85 100644
--- a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.go
+++ b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/client/client.go
@@ -95,7 +95,7 @@ func (c *Client) cacheKey(path string, params map[string]string) string {
if c.Config != nil {
key += "|auth_source=" + c.Config.AuthSource
if authHeader := c.Config.AuthHeader(); authHeader != "" {
- authHash := sha256.Sum256([]byte(c.Config.AuthHeader()))
+ authHash := sha256.Sum256([]byte(authHeader))
key += "|auth=" + hex.EncodeToString(authHash[:8])
}
if c.Config.Path != "" {
diff --git a/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.go b/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.go
index 585e79d5..993e94a6 100644
--- a/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.go
+++ b/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/internal/store/store.go
@@ -1109,6 +1109,10 @@ func (s *Store) Status() (map[string]int, error) {
// ResolveByName resolves a human-readable name to a UUID from synced data.
// If the input is already a UUID, it is returned as-is.
// matchFields are JSON field names to search against (e.g., "name", "key", "email").
+//
+// json_extract path components cannot be bound as SQL parameters, so each
+// field is validated against validIdentifierRE before being spliced into
+// the query.
func (s *Store) ResolveByName(resourceType string, input string, matchFields ...string) (string, error) {
if IsUUID(input) {
return input, nil
@@ -1116,6 +1120,9 @@ func (s *Store) ResolveByName(resourceType string, input string, matchFields ...
var matches []string
for _, field := range matchFields {
+ if !validIdentifierRE.MatchString(field) {
+ continue
+ }
query := fmt.Sprintf(
`SELECT id FROM resources WHERE resource_type = ? AND LOWER(json_extract(data, '$.%s')) = LOWER(?)`,
field,
diff --git a/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/client/client.go b/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/client/client.go
index c0ac2fa5..98425284 100644
--- a/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/client/client.go
+++ b/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/client/client.go
@@ -190,7 +190,7 @@ func (c *Client) cacheKey(path string, params map[string]string) string {
if c.Config != nil {
key += "|auth_source=" + c.Config.AuthSource
if authHeader := c.Config.AuthHeader(); authHeader != "" {
- authHash := sha256.Sum256([]byte(c.Config.AuthHeader()))
+ authHash := sha256.Sum256([]byte(authHeader))
key += "|auth=" + hex.EncodeToString(authHash[:8])
}
if c.Config.Path != "" {
← ea0cfd8d chore(cli): ignore local AI agent instruction files (#1326)
·
back to Cli Printing Press
·
feat(cli): emit fetchFull<X> companion for GET-embedded page 900b335c →