Skip to content

Commit b99b63d

Browse files
dylanhuff-at-coderdylanhuff-at-coder
committed
fix: tighten coder_secret validation, add duplicate detection, and address other pr feedback
1 parent 184188d commit b99b63d

5 files changed

Lines changed: 212 additions & 17 deletions

File tree

‎extract/secret.go‎

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,21 +35,43 @@ func SecretFromBlock(block *terraform.Block) (req *types.SecretRequirement, diag
3535
diags = diags.Append(helpDiag)
3636
}
3737

38-
env := optionalString(block, "env")
39-
file := optionalString(block, "file")
38+
// Check presence separately from value so we can distinguish "attribute
39+
// absent" from "attribute present but wrong type"; the latter must produce
40+
// a type diagnostic instead of being silently treated as unset.
41+
envAttr := block.GetAttribute("env")
42+
fileAttr := block.GetAttribute("file")
43+
envSet := envAttr != nil && !envAttr.IsNil()
44+
fileSet := fileAttr != nil && !fileAttr.IsNil()
4045

41-
// Mutual exclusivity: exactly one of env/file must be set.
46+
var env, file string
47+
if envSet {
48+
v, d := requiredString(block, "env")
49+
if d != nil {
50+
diags = diags.Append(d)
51+
}
52+
env = v
53+
}
54+
if fileSet {
55+
v, d := requiredString(block, "file")
56+
if d != nil {
57+
diags = diags.Append(d)
58+
}
59+
file = v
60+
}
61+
62+
// Mutual exclusivity is based on presence, not parsed value, so a
63+
// wrong-type attribute still counts as "set" here.
4264
switch {
43-
case env == "" && file == "":
44-
r := block.HCLBlock().Body.MissingItemRange()
65+
case !envSet && !fileSet:
66+
r := block.HCLBlock().DefRange
4567
diags = diags.Append(&hcl.Diagnostic{
4668
Severity: hcl.DiagError,
4769
Summary: `Invalid "coder_secret" block`,
4870
Detail: `Exactly one of "env" or "file" must be set, neither were set`,
4971
Subject: &r,
5072
})
51-
case env != "" && file != "":
52-
r := block.HCLBlock().Body.MissingItemRange()
73+
case envSet && fileSet:
74+
r := block.HCLBlock().DefRange
5375
diags = diags.Append(&hcl.Diagnostic{
5476
Severity: hcl.DiagError,
5577
Summary: `Invalid "coder_secret" block`,

‎preview_test.go‎

Lines changed: 100 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,13 @@ func Test_Extract(t *testing.T) {
4242
failPreview bool
4343
input preview.Input
4444

45-
expTags map[string]string
46-
unknownTags []string
47-
params map[string]assertParam
48-
variables map[string]assertVariable
49-
presetsFuncs func(t *testing.T, presets []types.Preset)
50-
presets map[string]assertPreset
51-
warnings []*regexp.Regexp
45+
expTags map[string]string
46+
unknownTags []string
47+
params map[string]assertParam
48+
variables map[string]assertVariable
49+
presetsFuncs func(t *testing.T, presets []types.Preset)
50+
presets map[string]assertPreset
51+
warnings []*regexp.Regexp
5252
secretRequirements []types.SecretRequirement
5353
}{
5454
{
@@ -1200,6 +1200,60 @@ data "coder_secret" "x" {
12001200
`,
12011201
wantDiag: `Exactly one of "env" or "file" must be set`,
12021202
},
1203+
{
1204+
name: "env wrong type (number)",
1205+
tf: `
1206+
data "coder_secret" "x" {
1207+
env = 42
1208+
help_message = "ok"
1209+
}
1210+
`,
1211+
wantDiag: `Expected a string`,
1212+
},
1213+
{
1214+
name: "file wrong type (bool)",
1215+
tf: `
1216+
data "coder_secret" "x" {
1217+
file = true
1218+
help_message = "ok"
1219+
}
1220+
`,
1221+
wantDiag: `Expected a string`,
1222+
},
1223+
{
1224+
name: "env null",
1225+
tf: `
1226+
data "coder_secret" "x" {
1227+
env = null
1228+
help_message = "ok"
1229+
}
1230+
`,
1231+
wantDiag: `Expected a string`,
1232+
},
1233+
{
1234+
name: "file null",
1235+
tf: `
1236+
data "coder_secret" "x" {
1237+
file = null
1238+
help_message = "ok"
1239+
}
1240+
`,
1241+
wantDiag: `Expected a string`,
1242+
},
1243+
{
1244+
name: "duplicate block labels",
1245+
tf: `
1246+
data "coder_secret" "x" {
1247+
env = "A"
1248+
help_message = "first"
1249+
}
1250+
data "coder_secret" "x" {
1251+
env = "B"
1252+
help_message = "second"
1253+
}
1254+
`,
1255+
wantDiag: `duplicate coder_secret blocks with name "x"`,
1256+
},
12031257
}
12041258
for _, tc := range tests {
12051259
t.Run(tc.name, func(t *testing.T) {
@@ -1219,3 +1273,42 @@ data "coder_secret" "x" {
12191273
})
12201274
}
12211275
}
1276+
1277+
// Test_SecretRequirement_DuplicateDetectionRequiresExtractionSuccess pins the
1278+
// current behavior: duplicate-label detection only fires for blocks that
1279+
// successfully extract. When one of two same-labeled blocks has an extraction
1280+
// error (e.g. wrong attribute type), the dedup bookkeeping skips it, so the
1281+
// "duplicate coder_secret blocks" diagnostic does not appear — only the
1282+
// per-block extraction error does. This matches parameter.go's precedent.
1283+
// If this behavior is ever changed to "track duplicates regardless of
1284+
// extraction success," update this test.
1285+
func Test_SecretRequirement_DuplicateDetectionRequiresExtractionSuccess(t *testing.T) {
1286+
t.Parallel()
1287+
tf := `
1288+
data "coder_secret" "x" {
1289+
env = 42
1290+
help_message = "first"
1291+
}
1292+
data "coder_secret" "x" {
1293+
env = "B"
1294+
help_message = "second"
1295+
}
1296+
`
1297+
fsys := fstest.MapFS{"main.tf": &fstest.MapFile{Data: []byte(tf)}}
1298+
_, diags := preview.Preview(context.Background(), preview.Input{}, fsys)
1299+
require.True(t, diags.HasErrors(), "expected errors; got %v", diags)
1300+
1301+
var hasTypeErr, hasDupeErr bool
1302+
for _, d := range diags {
1303+
msg := d.Summary + " " + d.Detail
1304+
if strings.Contains(msg, "Expected a string") {
1305+
hasTypeErr = true
1306+
}
1307+
if strings.Contains(msg, "duplicate coder_secret blocks") {
1308+
hasDupeErr = true
1309+
}
1310+
}
1311+
require.True(t, hasTypeErr, "expected type error for env=42; got: %v", diags)
1312+
require.False(t, hasDupeErr,
1313+
"duplicate diagnostic unexpectedly fired; dedup should only track successful extractions. diags: %v", diags)
1314+
}

‎secret.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package preview
22

33
import (
4+
"fmt"
5+
"strings"
6+
47
"github.com/aquasecurity/trivy/pkg/iac/terraform"
58
"github.com/hashicorp/hcl/v2"
69

@@ -11,6 +14,10 @@ import (
1114
func secrets(modules terraform.Modules) ([]types.SecretRequirement, hcl.Diagnostics) {
1215
diags := make(hcl.Diagnostics, 0)
1316
reqs := make([]types.SecretRequirement, 0)
17+
// Track blocks by label (e.g. "x" in `data "coder_secret" "x"`) so we can
18+
// emit a single duplicate diagnostic per colliding label. Mirrors the
19+
// parameter dedup pattern in parameter.go.
20+
exists := make(map[string][]*terraform.Block)
1421

1522
for _, mod := range modules {
1623
blocks := mod.GetDatasByType(types.BlockTypeSecret)
@@ -21,10 +28,29 @@ func secrets(modules terraform.Modules) ([]types.SecretRequirement, hcl.Diagnost
2128
}
2229
if req != nil {
2330
reqs = append(reqs, *req)
31+
name := block.NameLabel()
32+
exists[name] = append(exists[name], block)
2433
}
2534
}
2635
}
2736

37+
for name, blocks := range exists {
38+
if len(blocks) <= 1 {
39+
continue
40+
}
41+
var detail strings.Builder
42+
for _, b := range blocks {
43+
_, _ = detail.WriteString(fmt.Sprintf("block %q at %s\n",
44+
b.Type()+"."+strings.Join(b.Labels(), "."),
45+
b.HCLBlock().TypeRange))
46+
}
47+
diags = diags.Append(&hcl.Diagnostic{
48+
Severity: hcl.DiagError,
49+
Summary: fmt.Sprintf("Found %d duplicate coder_secret blocks with name %q, this is not allowed", len(blocks), name),
50+
Detail: detail.String(),
51+
})
52+
}
53+
2854
types.SortSecretRequirements(reqs)
2955
return reqs, diags
3056
}

‎types/secret.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,9 @@ const BlockTypeSecret = "coder_secret"
1212
// template. Exactly one of Env or File will be non-empty; validation of that
1313
// invariant happens during extraction.
1414
type SecretRequirement struct {
15-
Env string `json:"env"`
16-
File string `json:"file"`
17-
HelpMessage string `json:"help_message"`
15+
Env string `json:"env,omitempty"`
16+
File string `json:"file,omitempty"`
17+
HelpMessage string `json:"help_message,omitempty"`
1818
}
1919

2020
// SortSecretRequirements orders requirements first by Env then by File so

‎types/secret_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package types_test
2+
3+
import (
4+
"encoding/json"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
9+
"github.com/coder/preview/types"
10+
)
11+
12+
// Test_SecretRequirement_JSON_Omitempty verifies that empty Env, File, and
13+
// HelpMessage fields are omitted from the marshaled JSON. Since extraction
14+
// enforces exactly one of Env/File is non-empty, omitempty makes the on-the-
15+
// wire shape explicit about which field applies.
16+
func Test_SecretRequirement_JSON_Omitempty(t *testing.T) {
17+
t.Parallel()
18+
19+
tests := []struct {
20+
name string
21+
req types.SecretRequirement
22+
want string
23+
}{
24+
{
25+
name: "env only",
26+
req: types.SecretRequirement{Env: "FOO", HelpMessage: "set FOO"},
27+
want: `{"env":"FOO","help_message":"set FOO"}`,
28+
},
29+
{
30+
name: "file only",
31+
req: types.SecretRequirement{File: "~/bar", HelpMessage: "set bar"},
32+
want: `{"file":"~/bar","help_message":"set bar"}`,
33+
},
34+
{
35+
name: "empty help_message is omitted",
36+
req: types.SecretRequirement{Env: "FOO"},
37+
want: `{"env":"FOO"}`,
38+
},
39+
{
40+
name: "all empty produces empty object",
41+
req: types.SecretRequirement{},
42+
want: `{}`,
43+
},
44+
}
45+
46+
for _, tc := range tests {
47+
t.Run(tc.name, func(t *testing.T) {
48+
t.Parallel()
49+
got, err := json.Marshal(tc.req)
50+
require.NoError(t, err)
51+
require.JSONEq(t, tc.want, string(got))
52+
})
53+
}
54+
}

0 commit comments

Comments
 (0)