feat: env var secret delivery — stop inlining credentials in YAML#23
feat: env var secret delivery — stop inlining credentials in YAML#23TerrifiedBug merged 5 commits intomainfrom
Conversation
Greptile SummaryThis PR replaces plaintext credential inlining in pipeline YAML with Three correctness gaps remain:
The core refactor is sound and represents a clear security improvement. All loop optimization and ordering determinism issues from earlier reviews have been addressed. Confidence Score: 3/5
Last reviewed commit: 8fae4a1 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function convertSecretRefsToEnvVars( | ||
| config: Record<string, unknown>, | ||
| ): Record<string, unknown> { | ||
| return walkConvertSecretRefs(config); | ||
| } | ||
|
|
||
| function walkConvertSecretRefs( | ||
| obj: Record<string, unknown>, | ||
| ): Record<string, unknown> { | ||
| const result: Record<string, unknown> = {}; | ||
|
|
||
| for (const [key, value] of Object.entries(obj)) { | ||
| if (typeof value === "string") { | ||
| const match = value.match(SECRET_REF_PATTERN); | ||
| if (match) { | ||
| result[key] = `\${VF_SECRET_${match[1]}}`; | ||
| } else { | ||
| result[key] = value; | ||
| } | ||
| } else if (typeof value === "object" && value !== null && !Array.isArray(value)) { | ||
| result[key] = walkConvertSecretRefs(value as Record<string, unknown>); | ||
| } else { | ||
| result[key] = value; | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Referenced secrets are no longer validated at delivery time
The old resolveSecretRefs explicitly verified that every SECRET[name] reference in the config had a corresponding secret in the environment, throwing a descriptive error if one was missing:
for (const ref of refs) {
if (!secretMap.has(ref)) {
throw new Error(`Secret "${ref}" not found in environment`);
}
}The new convertSecretRefsToEnvVars is a pure string transformation — it blindly converts any SECRET[name] reference to an env var placeholder regardless of whether that secret exists in the database. If a pipeline config references a secret that doesn't exist (e.g., a typo, or a secret that was deleted), the YAML will silently contain ${VF_SECRET_MISSING_NAME}. Vector will resolve this to an empty string or fail to start, producing a runtime error on the agent that's difficult to trace back to a missing secret.
Since the secrets dict is built from all environment secrets anyway (in route.ts lines 91–96), it would be straightforward to check that every SECRET[...] reference found in the config has a matching key in the secrets before returning the config response, and return an error if any are missing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/services/secret-resolver.ts
Line: 101-128
Comment:
**Referenced secrets are no longer validated at delivery time**
The old `resolveSecretRefs` explicitly verified that every `SECRET[name]` reference in the config had a corresponding secret in the environment, throwing a descriptive error if one was missing:
```ts
for (const ref of refs) {
if (!secretMap.has(ref)) {
throw new Error(`Secret "${ref}" not found in environment`);
}
}
```
The new `convertSecretRefsToEnvVars` is a pure string transformation — it blindly converts any `SECRET[name]` reference to an env var placeholder regardless of whether that secret exists in the database. If a pipeline config references a secret that doesn't exist (e.g., a typo, or a secret that was deleted), the YAML will silently contain `${VF_SECRET_MISSING_NAME}`. Vector will resolve this to an empty string or fail to start, producing a runtime error on the agent that's difficult to trace back to a missing secret.
Since the `secrets` dict is built from all environment secrets anyway (in `route.ts` lines 91–96), it would be straightforward to check that every `SECRET[...]` reference found in the config has a matching key in the secrets before returning the config response, and return an error if any are missing.
How can I resolve this? If you propose a fix, please make it concise.Secret names with hyphens or dots (e.g. "db-password") produced
invalid env var names like ${VF_SECRET_db-password}. Now normalized
to uppercase with underscores: ${VF_SECRET_DB_PASSWORD}.
Both the YAML placeholder and secrets dict key use the same
secretNameToEnvVar function to ensure they always match.
| export function secretNameToEnvVar(name: string): string { | ||
| return `VF_SECRET_${name.replace(/[^a-zA-Z0-9]/g, "_").toUpperCase()}`; | ||
| } |
There was a problem hiding this comment.
Name-collision risk after normalization
secretNameToEnvVar maps multiple distinct secret names to the same env var key. For example, db-password and db_password both normalize to VF_SECRET_DB_PASSWORD. If both secrets exist in the same environment, the secrets dict in route.ts (built by iterating envSecrets in an unspecified order) will silently overwrite one value with the other. At the same time, both SECRET[db-password] and SECRET[db_password] in the pipeline YAML would emit the identical placeholder ${VF_SECRET_DB_PASSWORD}, so the wrong credential would be injected at runtime with no error surfaced.
Since the YAML placeholder and the delivered env var key must agree, the normalization itself is correct — but it creates an implicit constraint that DB-level uniqueness (environmentId + name) no longer guarantees uniqueness of the resolved key. Consider adding a uniqueness check at secret-creation time (or during config delivery) that rejects a new secret whose normalized name collides with an existing one.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/services/secret-resolver.ts
Line: 101-103
Comment:
**Name-collision risk after normalization**
`secretNameToEnvVar` maps multiple distinct secret names to the same env var key. For example, `db-password` and `db_password` both normalize to `VF_SECRET_DB_PASSWORD`. If both secrets exist in the same environment, the `secrets` dict in `route.ts` (built by iterating `envSecrets` in an unspecified order) will silently overwrite one value with the other. At the same time, both `SECRET[db-password]` and `SECRET[db_password]` in the pipeline YAML would emit the identical placeholder `${VF_SECRET_DB_PASSWORD}`, so the wrong credential would be injected at runtime with no error surfaced.
Since the YAML placeholder and the delivered env var key must agree, the normalization itself is correct — but it creates an implicit constraint that DB-level uniqueness (`environmentId + name`) no longer guarantees uniqueness of the resolved key. Consider adding a uniqueness check at secret-creation time (or during config delivery) that rejects a new secret whose normalized name collides with an existing one.
How can I resolve this? If you propose a fix, please make it concise.- Include secrets dict in checksum computation so secret rotation triggers agent restart (previously, YAML-only checksum wouldn't change when env var placeholders stayed the same) - Log warning when secret name normalization produces a collision (e.g. "db-password" and "db_password" both → VF_SECRET_DB_PASSWORD)
| export function convertSecretRefsToEnvVars( | ||
| config: Record<string, unknown>, | ||
| ): Record<string, unknown> { | ||
| return walkConvertSecretRefs(config); |
There was a problem hiding this comment.
The new convertSecretRefsToEnvVars function is a pure string transformation that blindly converts any SECRET[name] reference to an env var placeholder regardless of whether that secret actually exists in the database.
If a pipeline config references a secret that doesn't exist (e.g., a typo or deleted secret), the YAML will silently contain ${VF_SECRET_MISSING_NAME}. Vector will then resolve this to an empty string or fail to start with an unclear error message.
Unlike the old resolveSecretRefs which validated references at delivery time (throwing "Secret not found" errors), there is now no fail-fast validation. Consider either:
- Adding validation in
route.tsto check that all referenced secrets exist before converting to placeholders, or - Including a validation step in
convertSecretRefsToEnvVarsthat collects and validates references
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/services/secret-resolver.ts
Line: 110-113
Comment:
The new `convertSecretRefsToEnvVars` function is a pure string transformation that blindly converts any `SECRET[name]` reference to an env var placeholder regardless of whether that secret actually exists in the database.
If a pipeline config references a secret that doesn't exist (e.g., a typo or deleted secret), the YAML will silently contain `${VF_SECRET_MISSING_NAME}`. Vector will then resolve this to an empty string or fail to start with an unclear error message.
Unlike the old `resolveSecretRefs` which validated references at delivery time (throwing "Secret not found" errors), there is now no fail-fast validation. Consider either:
1. Adding validation in `route.ts` to check that all referenced secrets exist before converting to placeholders, or
2. Including a validation step in `convertSecretRefsToEnvVars` that collects and validates references
How can I resolve this? If you propose a fix, please make it concise.…ordering Move the secrets findMany + decrypt loop above the pipeline iteration so it runs once per request instead of once per pipeline. Add orderBy name asc to ensure deterministic collision resolution when multiple secret names normalize to the same env var key.
| export function convertSecretRefsToEnvVars( | ||
| config: Record<string, unknown>, | ||
| ): Record<string, unknown> { | ||
| return walkConvertSecretRefs(config); | ||
| } |
There was a problem hiding this comment.
convertSecretRefsToEnvVars performs a pure string transformation without validating that referenced secrets exist. If a pipeline config contains SECRET[missing-name] but that secret doesn't exist in the database, the YAML will be emitted with the literal placeholder ${VF_SECRET_MISSING_NAME}. Vector will fail to interpolate it (resulting in an empty string or startup failure), but the error won't be traced back to a missing secret — it will appear as a runtime failure on the agent instead of a validation error at deploy time.
Consider adding validation during config delivery (in route.ts) to ensure all SECRET[name] references in the config have matching secrets before converting to placeholders.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/services/secret-resolver.ts
Line: 110-114
Comment:
`convertSecretRefsToEnvVars` performs a pure string transformation without validating that referenced secrets exist. If a pipeline config contains `SECRET[missing-name]` but that secret doesn't exist in the database, the YAML will be emitted with the literal placeholder `${VF_SECRET_MISSING_NAME}`. Vector will fail to interpolate it (resulting in an empty string or startup failure), but the error won't be traced back to a missing secret — it will appear as a runtime failure on the agent instead of a validation error at deploy time.
Consider adding validation during config delivery (in `route.ts`) to ensure all `SECRET[name]` references in the config have matching secrets before converting to placeholders.
How can I resolve this? If you propose a fix, please make it concise.| const envSecrets = await prisma.secret.findMany({ | ||
| where: { environmentId: agent.environmentId }, | ||
| orderBy: { name: "asc" }, | ||
| }); | ||
| for (const s of envSecrets) { | ||
| const envKey = secretNameToEnvVar(s.name); | ||
| if (secrets[envKey] !== undefined) { | ||
| console.warn(`[agent-config] Secret name collision: "${s.name}" normalizes to "${envKey}" which is already set`); | ||
| } | ||
| secrets[envKey] = decrypt(s.encryptedValue); | ||
| } | ||
| } |
There was a problem hiding this comment.
Secret names can normalize to colliding env var keys — for example, both db-password and db_password normalize to VF_SECRET_DB_PASSWORD. While the deterministic orderBy: { name: "asc" } makes the collision resolution predictable, one secret's decrypted value will silently overwrite the other in the secrets dict, and both SECRET[db-password] and SECRET[db_password] references in the config will emit the same placeholder. The agent will set the wrong credential without any error.
Consider validating at secret-creation time that no two secrets in the same environment normalize to the same env var key, rejecting new secrets that would create collisions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/agent/config/route.ts
Line: 62-73
Comment:
Secret names can normalize to colliding env var keys — for example, both `db-password` and `db_password` normalize to `VF_SECRET_DB_PASSWORD`. While the deterministic `orderBy: { name: "asc" }` makes the collision resolution predictable, one secret's decrypted value will silently overwrite the other in the `secrets` dict, and both `SECRET[db-password]` and `SECRET[db_password]` references in the config will emit the same placeholder. The agent will set the wrong credential without any error.
Consider validating at secret-creation time that no two secrets in the same environment normalize to the same env var key, rejecting new secrets that would create collisions.
How can I resolve this? If you propose a fix, please make it concise.| } else if (typeof value === "object" && value !== null && !Array.isArray(value)) { | ||
| result[key] = walkConvertSecretRefs(value as Record<string, unknown>); | ||
| } else { | ||
| result[key] = value; |
There was a problem hiding this comment.
walkConvertSecretRefs skips arrays (due to the !Array.isArray(value) guard) and passes them through unchanged. If a Vector config field contains an array with a string element matching SECRET[name], that reference will not be converted to the ${VF_SECRET_NAME} placeholder — it will be emitted verbatim as SECRET[name]. Vector will then treat it as a literal string value rather than a reference to interpolate from an env var, causing the feature relying on that secret to malfunction.
Most Vector credential fields are object properties (not arrays), so real-world impact is limited. However, the conversion logic should be complete:
} else if (Array.isArray(value)) {
result[key] = value.map((item) =>
typeof item === "string"
? (item.match(SECRET_REF_PATTERN)
? `\${${secretNameToEnvVar(item.match(SECRET_REF_PATTERN)![1])}}`
: item)
: typeof item === "object" && item !== null
? walkConvertSecretRefs(item as Record<string, unknown>)
: item
);
} else if (typeof value === "object" && value !== null) {Prompt To Fix With AI
This is a comment left during a code review.
Path: src/server/services/secret-resolver.ts
Line: 129-132
Comment:
`walkConvertSecretRefs` skips arrays (due to the `!Array.isArray(value)` guard) and passes them through unchanged. If a Vector config field contains an array with a string element matching `SECRET[name]`, that reference will not be converted to the `${VF_SECRET_NAME}` placeholder — it will be emitted verbatim as `SECRET[name]`. Vector will then treat it as a literal string value rather than a reference to interpolate from an env var, causing the feature relying on that secret to malfunction.
Most Vector credential fields are object properties (not arrays), so real-world impact is limited. However, the conversion logic should be complete:
```ts
} else if (Array.isArray(value)) {
result[key] = value.map((item) =>
typeof item === "string"
? (item.match(SECRET_REF_PATTERN)
? `\${${secretNameToEnvVar(item.match(SECRET_REF_PATTERN)![1])}}`
: item)
: typeof item === "object" && item !== null
? walkConvertSecretRefs(item as Record<string, unknown>)
: item
);
} else if (typeof value === "object" && value !== null) {
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
SECRET[name]references to${VF_SECRET_NAME}env var placeholderssecretsdict (already delivered in the API response) provides the actual valuesBefore: YAML contained
password: actualpassword123(plaintext in config files on disk)After: YAML contains
password: ${VF_SECRET_MY_DB_PASSWORD}(resolved from env var at runtime)Test Plan
SECRET[name]reference in a sensitive field${VF_SECRET_*}placeholders (not real values)secretsdict in response still contains decrypted values