Skip to content

Commit 976c79c

Browse files
fix: address code-review findings
- validate: run config warnings against RAW (pre-resolution) config so a correctly-externalized \ is not flagged as hardcoded; this fixes 'validate --strict' falsely failing CI when secrets are set in .env - validate: silence loadConfig's per-var warnings (already reported in aggregate) - validator: also detect hardcoded secrets in the env block, keyed precisely by the last key segment so benign keys like PASSWORD_RESET_URL don't false-positive - compose: generic services no longer auto-inherit a DB-type healthcheck by coincidental type name; they only get healthchecks explicitly declared in health_checks - logger: add getLevel(); tests for all of the above (225 passing)
1 parent 264daa2 commit 976c79c

7 files changed

Lines changed: 108 additions & 17 deletions

File tree

src/commands/validate.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,9 @@ import * as path from 'path';
22
import * as yaml from 'js-yaml';
33
import { findProjectRoot, loadConfig, loadEnvFile, findUnresolvedVars } from '../core/config/loader';
44
import { checkConfigWarnings } from '../core/config/validator';
5+
import type { DevEnvConfig } from '../types/config';
56
import { fileExists, readFile } from '../utils/file-ops';
6-
import { logger } from '../utils/logger';
7+
import { logger, LogLevel } from '../utils/logger';
78

89
export interface ValidateOptions {
910
/** When true, treat warnings and unresolved ${VAR} references as failures (non-zero exit). */
@@ -33,8 +34,16 @@ export async function runValidate(options: ValidateOptions = {}): Promise<void>
3334
const raw = yaml.load(await readFile(configPath), { schema: yaml.DEFAULT_SCHEMA });
3435
const unresolved = findUnresolvedVars(raw);
3536

36-
// Full schema validation (throws with readable issues on failure).
37-
const config = await loadConfig(projectRoot);
37+
// Full schema validation (throws with readable issues on failure). loadConfig
38+
// resolves ${VAR} and would re-warn for each unset one, which we already report
39+
// in aggregate below, so silence its per-occurrence warnings during this call.
40+
const prevLevel = logger.getLevel();
41+
logger.setLevel(LogLevel.ERROR);
42+
try {
43+
await loadConfig(projectRoot);
44+
} finally {
45+
logger.setLevel(prevLevel);
46+
}
3847

3948
if (unresolved.length > 0) {
4049
logger.warn(
@@ -44,7 +53,9 @@ export async function runValidate(options: ValidateOptions = {}): Promise<void>
4453
}
4554

4655
// Surface non-fatal config warnings (hardcoded secrets, port conflicts, ...).
47-
const warnings = checkConfigWarnings(config);
56+
// Check the RAW config so correctly-externalized secrets (still `${VAR}` here)
57+
// are not mistaken for hardcoded ones after resolution.
58+
const warnings = checkConfigWarnings((raw ?? {}) as DevEnvConfig);
4859

4960
if (options.strict && (unresolved.length > 0 || warnings.length > 0)) {
5061
throw new Error(

src/core/config/validator.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,16 @@ function isLiteralSecret(value: string | undefined): boolean {
2828
return !!value && !value.includes('${');
2929
}
3030

31+
const SECRET_KEY_WORDS = ['password', 'secret', 'token', 'key', 'pass', 'auth', 'apikey'];
32+
33+
// True only when the env key clearly NAMES a secret, judged by its last
34+
// underscore/hyphen segment — so DB_PASSWORD / API_KEY match, but benign keys
35+
// like PASSWORD_RESET_URL (ends in "url") do not.
36+
function isSecretEnvKey(key: string): boolean {
37+
const last = key.split(/[_-]/).pop()?.toLowerCase() ?? '';
38+
return SECRET_KEY_WORDS.includes(last);
39+
}
40+
3141
/**
3242
* Check for common configuration issues. Each issue is logged as a warning and
3343
* also returned, so callers (e.g. `validate --strict`) can act on them.
@@ -39,11 +49,17 @@ export function checkConfigWarnings(config: DevEnvConfig): string[] {
3949
logger.warn(msg);
4050
};
4151

42-
// Check for hardcoded passwords in known secret locations (should be in .env).
43-
// Only inspect real secret fields so keys like `password_reset_url` don't false-positive.
44-
if (config.databases?.some((db) => isLiteralSecret(db.password))) {
52+
// Check for hardcoded secrets in known secret locations (should be in .env).
53+
// Only inspect real secret fields so keys like `password_reset_url` don't false-positive:
54+
// - database passwords
55+
// - env values whose KEY looks secret-bearing (password/secret/token/key/auth)
56+
const hardcodedDbPassword = config.databases?.some((db) => isLiteralSecret(db.password));
57+
const hardcodedEnvSecret = Object.entries(config.env ?? {}).some(
58+
([key, value]) => isSecretEnvKey(key) && isLiteralSecret(value)
59+
);
60+
if (hardcodedDbPassword || hardcodedEnvSecret) {
4561
warn(
46-
'Warning: Passwords detected in configuration file.\n' +
62+
'Warning: Hardcoded secrets detected in configuration file.\n' +
4763
'Consider using environment variables (${VAR_NAME}) and .env file for secrets.'
4864
);
4965
}

src/core/docker/compose-generator.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,19 @@ function renderHealthcheckFragment(test: string[], hc?: HealthCheck): string {
4848
* Resolution order:
4949
* 1. A matching `health_checks` entry that specifies a raw `test` command (any service, incl. generic ones).
5050
* 2. A matching entry whose `type` is a known database type.
51-
* 3. Built-in defaults for known database `serviceType`s.
52-
* Returns undefined when no healthcheck can be determined (e.g. an unknown generic service with no `test`).
51+
* 3. (databases only) Built-in defaults for the known database `serviceType`.
52+
*
53+
* `allowTypeDefault` gates step 3: it is true for databases (a postgres container
54+
* should get pg_isready automatically) but false for generic services, so a
55+
* service that merely happens to be typed e.g. "redis" does not silently inherit
56+
* a `redis-cli` healthcheck its image may not support. Generic services only get
57+
* a healthcheck they explicitly declared via `health_checks`.
5358
*/
5459
function buildHealthcheckForService(
5560
serviceName: string,
5661
serviceType: string,
57-
healthChecks: HealthCheck[] | undefined
62+
healthChecks: HealthCheck[] | undefined,
63+
allowTypeDefault: boolean
5864
): string | undefined {
5965
const matched = healthChecks?.find(
6066
(h) => h.name === serviceName || h.type === serviceType
@@ -68,6 +74,8 @@ function buildHealthcheckForService(
6874
return renderHealthcheckFragment(DB_HEALTHCHECK_TESTS[matched.type], matched);
6975
}
7076

77+
if (!allowTypeDefault) return undefined;
78+
7179
const test = DB_HEALTHCHECK_TESTS[serviceType];
7280
if (!test) return undefined;
7381
return renderHealthcheckFragment(test, matched);
@@ -104,7 +112,7 @@ function databaseToComposeService(
104112
if (db.database) env.MONGO_INITDB_DATABASE = db.database;
105113
}
106114

107-
const healthcheck = buildHealthcheckForService(name, db.type, healthChecks);
115+
const healthcheck = buildHealthcheckForService(name, db.type, healthChecks, true);
108116

109117
return {
110118
name,
@@ -125,7 +133,7 @@ function serviceToComposeService(
125133
const ports: string[] = [];
126134
if (svc.port) ports.push(`${svc.port}:${svc.port}`);
127135
if (svc.management_port) ports.push(`${svc.management_port}:${svc.management_port}`);
128-
const healthcheck = buildHealthcheckForService(name, svc.type, healthChecks);
136+
const healthcheck = buildHealthcheckForService(name, svc.type, healthChecks, false);
129137
return {
130138
name,
131139
image: svc.type,

src/utils/logger.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@ class Logger {
2323
this.level = level;
2424
}
2525

26+
getLevel(): LogLevel {
27+
return this.level;
28+
}
29+
2630
private formatMessage(level: string, message: string, ...args: any[]): string {
2731
const timestamp = new Date().toISOString();
2832
const formattedArgs = args.map(arg =>

tests/commands/validate.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,27 @@ describe('validate command', () => {
6363
await expect(runValidate({ strict: true })).rejects.toThrow(/--strict|unresolved/);
6464
});
6565

66+
it('does not flag a correctly-externalized secret as hardcoded under --strict', async () => {
67+
// Regression: checkConfigWarnings must run on the raw config so that a
68+
// ${DB_PASSWORD} resolved from .env is not mistaken for a hardcoded secret.
69+
await fs.writeFile(
70+
path.join(tempDir, '.dev-env.yml'),
71+
[
72+
'name: app',
73+
'databases:',
74+
' - type: postgresql',
75+
' port: 5432',
76+
' user: ${DB_USER}',
77+
' password: ${DB_PASSWORD}',
78+
' database: app',
79+
].join('\n')
80+
);
81+
await fs.writeFile(path.join(tempDir, '.env'), 'DB_USER=u\nDB_PASSWORD=secret123\n');
82+
process.chdir(tempDir);
83+
84+
await expect(runValidate({ strict: true })).resolves.toBeUndefined();
85+
});
86+
6687
it('resolves a variable defined in the config env block', async () => {
6788
await fs.writeFile(
6889
path.join(tempDir, '.dev-env.yml'),

tests/core/config/validator.test.ts

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { validateConfig, checkConfigWarnings } from '../../../src/core/config/validator';
1+
import { validateConfig, checkConfigWarnings } from '../../../src/core/config/validator';
22
import type { DevEnvConfig } from '../../../src/types/config';
33
import { logger } from '../../../src/utils/logger';
44

@@ -87,7 +87,7 @@ describe('validator', () => {
8787
};
8888
checkConfigWarnings(config as DevEnvConfig);
8989
expect(warnSpy).toHaveBeenCalledWith(
90-
expect.stringContaining('Passwords detected')
90+
expect.stringContaining('secrets detected')
9191
);
9292
});
9393

@@ -98,7 +98,7 @@ describe('validator', () => {
9898
env: { PASSWORD_RESET_URL: 'https://example.com/reset' },
9999
};
100100
checkConfigWarnings(config as unknown as DevEnvConfig);
101-
expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining('Passwords detected'));
101+
expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining('secrets detected'));
102102
});
103103

104104
it('returns the list of warnings it emits', () => {
@@ -109,7 +109,27 @@ describe('validator', () => {
109109
],
110110
};
111111
const warnings = checkConfigWarnings(config as DevEnvConfig);
112-
expect(warnings.some((w) => w.includes('Passwords detected'))).toBe(true);
112+
expect(warnings.some((w) => w.includes('secrets detected'))).toBe(true);
113+
});
114+
115+
it('warns on a hardcoded secret in the env block (secret-like key)', () => {
116+
const config = {
117+
name: 'app',
118+
databases: [{ type: 'redis' as const, port: 6379, host: 'localhost' }],
119+
env: { API_KEY: 'sk-live-abc123' },
120+
};
121+
checkConfigWarnings(config as unknown as DevEnvConfig);
122+
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('secrets detected'));
123+
});
124+
125+
it('does not warn on an env placeholder secret value', () => {
126+
const config = {
127+
name: 'app',
128+
databases: [{ type: 'redis' as const, port: 6379, host: 'localhost' }],
129+
env: { API_KEY: '${API_KEY}' },
130+
};
131+
checkConfigWarnings(config as unknown as DevEnvConfig);
132+
expect(warnSpy).not.toHaveBeenCalled();
113133
});
114134

115135
it('warns when postgresql missing user or password', () => {

tests/core/docker/compose-generator.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,17 @@ describe('compose-generator', () => {
9595
expect(content).toContain('retries: 3');
9696
});
9797

98+
it('does not auto-apply a DB healthcheck to a generic service by coincidental type', () => {
99+
const config = {
100+
name: 'app',
101+
services: [{ type: 'redis', port: 6379, host: 'localhost' }],
102+
docker: {},
103+
} as DevEnvConfig;
104+
const content = generateComposeContent(config, templatesDir);
105+
expect(content).toContain('redis_1:');
106+
expect(content).not.toContain('healthcheck:');
107+
});
108+
98109
it('includes custom service with port and management_port', () => {
99110
const config = {
100111
name: 'app',

0 commit comments

Comments
 (0)