Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/devcontainer/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,16 @@ type DevContainerConfigBase struct {
// DEPRECATED: Use 'customizations/vscode/devPort' instead
// The port VS Code can use to connect to its backend.
DevPort int `json:"devPort,omitempty"`

// Secrets declared by the dev container. Keys are secret names; values describe the secret.
// At runtime, each key is looked up in the host environment and, if set, injected into lifecycle commands.
Secrets map[string]SecretConfig `json:"secrets,omitempty"`
}
Comment on lines +107 to +110

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm Secrets is not merged across ImageMetadata entries.
rg -nP -C3 '\bSecrets\b' --type=go

Repository: devsy-org/devsy

Length of output: 12220


🏁 Script executed:

cat -n pkg/devcontainer/config/merge.go

Repository: devsy-org/devsy

Length of output: 8867


🏁 Script executed:

# Search for ImageMetadata struct definition and its fields
rg -n "type ImageMetadata struct" -A 50 --type=go

Repository: devsy-org/devsy

Length of output: 1896


🏁 Script executed:

# Look for merge tests related to Secrets or features
fd -e "*_test.go" -x grep -l "MergeConfiguration\|merge.*[Ss]ecrets" {} \;

Repository: devsy-org/devsy

Length of output: 41


🏁 Script executed:

# Find all merge-related test files
fd -e "merge*test.go" --type=f

Repository: devsy-org/devsy

Length of output: 41


🏁 Script executed:

# List all test files in config package
fd -e "*test.go" pkg/devcontainer/config/

Repository: devsy-org/devsy

Length of output: 460


🏁 Script executed:

# List all test files in config package
fd . 'pkg/devcontainer/config/' -e '*test.go'

Repository: devsy-org/devsy

Length of output: 41


🏁 Script executed:

# Search for test files in the config directory
ls -la pkg/devcontainer/config/ | grep test

Repository: devsy-org/devsy

Length of output: 263


🏁 Script executed:

# Search for MergeConfiguration in test files
rg -n "MergeConfiguration" pkg/devcontainer/config/ --type=go

Repository: devsy-org/devsy

Length of output: 119


🏁 Script executed:

# Search for MergeConfiguration usage across the codebase
rg -n "MergeConfiguration" --type=go

Repository: devsy-org/devsy

Length of output: 490


🏁 Script executed:

# Search for feature-related content and secrets in the codebase
rg -n "feature" -i pkg/devcontainer/config/merge.go

Repository: devsy-org/devsy

Length of output: 41


🏁 Script executed:

# Search for devcontainer spec references or feature documentation
rg -n "feature.*secret|secret.*feature" -i --type=go

Repository: devsy-org/devsy

Length of output: 41


🏁 Script executed:

# Check for any comments in merge.go about missing functionality or TODOs
rg -n "TODO|FIXME|XXX|BUG" pkg/devcontainer/config/merge.go

Repository: devsy-org/devsy

Length of output: 96


🏁 Script executed:

# Check the devcontainer spec comment in config.go for Secrets context
rg -n -B5 "Secrets.*secret" pkg/devcontainer/config/config.go

Repository: devsy-org/devsy

Length of output: 511


🏁 Script executed:

# Check how imageMetadataEntries are populated/created to understand if features can declare secrets
rg -n "imageMetadata|ImageMetadata" pkg/devcontainer/ -A3 | head -80

Repository: devsy-org/devsy

Length of output: 5049


🏁 Script executed:

# Find FeatureConfigToImageMetadata function
rg -n "FeatureConfigToImageMetadata" -A 20 --type=go

Repository: devsy-org/devsy

Length of output: 3279


🏁 Script executed:

# Check the FeatureConfig struct definition
rg -n "type FeatureConfig struct" -A 30 --type=go

Repository: devsy-org/devsy

Length of output: 2881


Secrets from feature/image metadata are dropped during merge.

Per MergeConfiguration in pkg/devcontainer/config/merge.go, the DevContainerConfigBase (which includes Secrets) is copied from the user config once at initialization (line 31) and never re-merged from the imageMetadataEntries. Since ImageMetadata embeds DevContainerConfigBase, any secrets declared in feature metadata are silently ignored.

The codebase already uses mergeMaps() for other maps like RemoteEnv and ContainerEnv. Apply the same pattern to Secrets to ensure feature-contributed secrets are properly merged (e.g., using a last-wins or first-wins policy with clear documentation).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/devcontainer/config/config.go` around lines 107 - 110, MergeConfiguration
currently copies DevContainerConfigBase (which contains Secrets) from the user
config once and never re-merges imageMetadataEntries, causing feature-provided
secrets to be dropped; update MergeConfiguration in
pkg/devcontainer/config/merge.go to merge Secrets the same way
RemoteEnv/ContainerEnv are merged by calling mergeMaps for the Secrets map on
each imageMetadataEntries iteration (use the same last-wins merge policy as the
other env maps) and add a brief comment documenting the chosen policy so
feature-contributed secrets are not silently ignored.


// SecretConfig describes a secret declared in devcontainer.json.
type SecretConfig struct {
Description string `json:"description,omitempty"`
DocumentationUrl string `json:"documentationUrl,omitempty"`
}

type DevContainerActions struct {
Expand Down
80 changes: 80 additions & 0 deletions pkg/devcontainer/config/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,86 @@ import (
"testing"
)

//nolint:cyclop // test function with multiple assertions
func TestParseSecrets(t *testing.T) {
tmpDir := t.TempDir()
configPath := filepath.Join(tmpDir, "devcontainer.json")
raw := `{
"name": "test-secrets",
"secrets": {
"MY_TOKEN": {
"description": "API token",
"documentationUrl": "https://example.com/docs"
},
"EMPTY_SECRET": {}
}
}`
// #nosec G306 -- test file
if err := os.WriteFile(configPath, []byte(raw), 0o644); err != nil {
t.Fatal(err)
}

cfg, err := ParseDevContainerJSONFile(configPath)
if err != nil {
t.Fatal(err)
}

if len(cfg.Secrets) != 2 {
t.Fatalf("expected 2 secrets, got %d", len(cfg.Secrets))
}
myToken, ok := cfg.Secrets["MY_TOKEN"]
if !ok {
t.Fatal("expected MY_TOKEN secret")
}
if myToken.Description != "API token" {
t.Errorf("expected description 'API token', got %q", myToken.Description)
}
if myToken.DocumentationUrl != "https://example.com/docs" {
t.Errorf(
"expected documentationUrl 'https://example.com/docs', got %q",
myToken.DocumentationUrl,
)
}
empty, ok := cfg.Secrets["EMPTY_SECRET"]
if !ok {
t.Fatal("expected EMPTY_SECRET secret")
}
if empty.Description != "" || empty.DocumentationUrl != "" {
t.Errorf("expected empty SecretConfig, got %+v", empty)
}
}

func TestSecretsRoundTrip(t *testing.T) {
tmpDir := t.TempDir()
cfg := &DevContainerConfig{
DevContainerConfigBase: DevContainerConfigBase{
Name: "round-trip",
Secrets: map[string]SecretConfig{
"DB_PASSWORD": {Description: "database password"},
},
},
}
cfg.Origin = filepath.Join(tmpDir, "devcontainer.json")

if err := SaveDevContainerJSON(cfg); err != nil {
t.Fatal(err)
}

loaded, err := ParseDevContainerJSONFile(cfg.Origin)
if err != nil {
t.Fatal(err)
}
if len(loaded.Secrets) != 1 {
t.Fatalf("expected 1 secret after round-trip, got %d", len(loaded.Secrets))
}
if loaded.Secrets["DB_PASSWORD"].Description != "database password" {
t.Errorf(
"expected 'database password', got %q",
loaded.Secrets["DB_PASSWORD"].Description,
)
}
}

func TestSaveDevContainerJSON(t *testing.T) {
type args struct {
config *DevContainerConfig
Expand Down
11 changes: 10 additions & 1 deletion pkg/devcontainer/setup/lifecyclehooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,19 @@ func resolveLifecycleEnv(
)
}

env := mergeRemoteEnv(mergedConfig.RemoteEnv, probedEnv, remoteUser)

// Resolve declared secrets from the host environment.
for name := range mergedConfig.Secrets {
if val := os.Getenv(name); val != "" {
env[name] = val
}
}
Comment on lines +46 to +51

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Empty-string host values silently skip declared secrets; also consider logging unresolved secrets.

Two concerns on this block:

  1. if val := os.Getenv(name); val != "" conflates "unset" with "set to empty string". os.LookupEnv is the idiomatic way to distinguish the two; whether an intentionally-empty secret should be propagated is a policy decision worth making explicit.
  2. When a declared secret is missing from the host environment, nothing is logged. Given these are declared secrets users opted into, a debug/warn log (without the value) would aid troubleshooting when lifecycle commands fail due to missing env vars.
Suggested adjustment
-	// Resolve declared secrets from the host environment.
-	for name := range mergedConfig.Secrets {
-		if val := os.Getenv(name); val != "" {
-			env[name] = val
-		}
-	}
+	// Resolve declared secrets from the host environment.
+	for name := range mergedConfig.Secrets {
+		if val, ok := os.LookupEnv(name); ok {
+			env[name] = val
+		} else {
+			log.Debugf("declared secret %q is not set in host environment; skipping", name)
+		}
+	}

Note: the existing test asserts that an absent secret is not present in remoteEnv, which the suggested change preserves. The val != "" check test case isn't covered — worth adding if you keep that semantic.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/devcontainer/setup/lifecyclehooks.go` around lines 46 - 51, The current
loop over mergedConfig.Secrets uses os.Getenv and skips host-provided
empty-string values and logs nothing when a secret is missing; change it to use
os.LookupEnv to distinguish unset vs empty-string so intentionally-empty secrets
are propagated according to policy, and add a debug/warn log (without printing
secret values) when LookupEnv reports the variable is not present; update the
loop that writes into env (referencing mergedConfig.Secrets and env) to call
os.LookupEnv and call the package logger (or processLogger) to record unresolved
secret names only.


return lifecycleEnv{
remoteUser: remoteUser,
workspaceFolder: setupInfo.SubstitutionContext.ContainerWorkspaceFolder,
remoteEnv: mergeRemoteEnv(mergedConfig.RemoteEnv, probedEnv, remoteUser),
remoteEnv: env,
Comment on lines +44 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

devcontainer.json secrets spec precedence remoteEnv

💡 Result:

In devcontainer.json, there is a "secrets" property for declaring recommended secrets, primarily used in GitHub Codespaces to prompt users to set user-specific secrets. These are not stored in the file but reference secrets provided by the implementing tool/service (e.g., Codespaces secrets). Secrets are handled similarly to "remoteEnv": implementing tools inject them as environment variables for the runtime environment (VS Code server, terminals, tasks, etc.) after the container's ENTRYPOINT. The devcontainers CLI supports passing secrets via --secrets-file for commands like "up", applying them like remoteEnv. Precedence: devcontainer.json takes precedence over image/feature metadata for properties like remoteEnv and containerEnv (per-variable last value wins, devcontainer.json last when order matters). Secrets follow similar logic if contributed by features/images. No native Docker buildkit --secret support in devcontainer.json; use workarounds like docker-compose build.secrets or build.options. Example declaration: { "secrets": { "MY_SECRET": { "description": "Personal API key" } }, "remoteEnv": { "MY_SECRET": "${secret:MY_SECRET}" } }

Citations:


🏁 Script executed:

# First, let's look at the full function and surrounding context
cat -n pkg/devcontainer/setup/lifecyclehooks.go | head -80

Repository: devsy-org/devsy

Length of output: 2961


🏁 Script executed:

# Look for test cases related to secret handling
fd -t f lifecyclehooks_test.go

Repository: devsy-org/devsy

Length of output: 104


🏁 Script executed:

# Search for how secrets are used elsewhere in the codebase
rg -i "secrets" pkg/devcontainer/setup/ -A 3 -B 3 --max-count=50

Repository: devsy-org/devsy

Length of output: 1754


🏁 Script executed:

# Check for documentation or spec about secret precedence
fd -t f -i "spec\|readme\|doc" pkg/devcontainer/

Repository: devsy-org/devsy

Length of output: 41


🏁 Script executed:

# View the full test case for secret handling
cat -n pkg/devcontainer/setup/lifecyclehooks_test.go | grep -A 40 "TestResolveLifecycleEnvIncludesSecrets"

Repository: devsy-org/devsy

Length of output: 1350


🏁 Script executed:

# Find the mergeRemoteEnv function definition
rg "func mergeRemoteEnv" pkg/devcontainer/setup/ -A 20

Repository: devsy-org/devsy

Length of output: 1673


🏁 Script executed:

# Check the Secrets configuration in the config package
rg "Secrets\s*map\[string\]" pkg/devcontainer/config/ -B 2 -A 2

Repository: devsy-org/devsy

Length of output: 494


🏁 Script executed:

# Search for any comments or TODOs about secret precedence
rg -i "secret.*precedence\|precedence.*secret\|secret.*override\|override.*secret" pkg/devcontainer/

Repository: devsy-org/devsy

Length of output: 41


🏁 Script executed:

# Check if there are any related GitHub issues or discussions referenced
rg -i "secret.*remoteenv\|remoteenv.*secret" pkg/devcontainer/ -B 3 -A 3

Repository: devsy-org/devsy

Length of output: 41


🏁 Script executed:

# Look at git history or comments around the secret handling code
rg "Resolve declared secrets" pkg/devcontainer/ -B 5 -A 10

Repository: devsy-org/devsy

Length of output: 1111


Secrets applied after mergeRemoteEnv create inconsistent precedence — author-specified values may be unexpectedly overwritten.

Secrets from the host environment (lines 47–50) are injected after mergeRemoteEnv(), which means a secret name matching a key in mergedConfig.RemoteEnv will overwrite the author-explicitly-set value. However, mergeRemoteEnv() itself states: "remoteEnv should always override probedEnv as it has been specified explicitly by the devcontainer author." This creates conflicting precedence: author-set RemoteEnv is prioritized over probed env, but then overridden by secrets. Either the order should be reversed (secrets first, then RemoteEnv), or the behavior should be clarified with a warning when a collision occurs. The existing test (TestResolveLifecycleEnvIncludesSecrets) does not cover the collision scenario.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/devcontainer/setup/lifecyclehooks.go` around lines 44 - 56, The current
code applies mergedConfig.Secrets after calling mergeRemoteEnv, letting host
secrets override author-specified RemoteEnv; change the order so secrets are
merged into probedEnv before calling mergeRemoteEnv (i.e., apply host secrets to
the probedEnv map, then call mergeRemoteEnv(probedEnv,...)) so mergeRemoteEnv
can correctly let mergedConfig.RemoteEnv override secrets; update any test
(e.g., TestResolveLifecycleEnvIncludesSecrets) to include a collision case
asserting author RemoteEnv wins, and reference mergeRemoteEnv,
mergedConfig.Secrets, and lifecycleEnv when making the change.

}
}

Expand Down
30 changes: 30 additions & 0 deletions pkg/devcontainer/setup/lifecyclehooks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,36 @@ func (s *LifecycleHookTestSuite) TestLifecycleHooksNoOpWithEmptyConfig() {
assert.NoError(s.T(), err)
}

func (s *LifecycleHookTestSuite) TestResolveLifecycleEnvIncludesSecrets() {
t := s.T()

// Set one secret in the environment, leave another absent.
t.Setenv("SECRET_PRESENT", "s3cret")

result := &config.Result{
MergedConfig: &config.MergedDevContainerConfig{
DevContainerConfigBase: config.DevContainerConfigBase{
Secrets: map[string]config.SecretConfig{
"SECRET_PRESENT": {Description: "a present secret"},
"SECRET_ABSENT": {Description: "a missing secret"},
},
},
},
ContainerDetails: &config.ContainerDetails{
State: config.ContainerDetailsState{},
},
SubstitutionContext: &config.SubstitutionContext{
ContainerWorkspaceFolder: "/workspaces/test",
},
}

env := resolveLifecycleEnv(context.Background(), result)

assert.Equal(t, "s3cret", env.remoteEnv["SECRET_PRESENT"])
_, found := env.remoteEnv["SECRET_ABSENT"]
assert.False(t, found, "SECRET_ABSENT should not be in remoteEnv when not set in environment")
}

func TestLifecycleHookTestSuite(t *testing.T) {
suite.Run(t, new(LifecycleHookTestSuite))
}
Loading