Database secret handling fix

[danielnaab]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 65412ad

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

PR Review: Database Secret Handling Fix

Summary

This PR improves database secret handling by migrating from manually managed secrets to AWS RDS-managed master user passwords stored in Secrets Manager. This is a good security improvement that follows AWS best practices.

---

✅ Strengths

1. Better Security Model: Using RDS-managed passwords (manageMasterUserPassword: true) is an AWS best practice that provides automatic rotation capabilities and reduces secret sprawl.

2. Simplified Infrastructure: Removes manual secret creation/version management code from the CDK stack, reducing complexity and maintenance burden.

3. Consistent Pattern: The application code now properly fetches secrets from AWS Secrets Manager at runtime rather than having them passed directly as environment variables.

---

🔍 Issues & Concerns

1. Critical: Error Handling Gap in Secret Retrieval

Location: apps/sandbox/src/index.ts:23

The code doesn't differentiate between network failures and missing secrets:

const dbSecretStr = await vault.getSecret(dbSecretArn);

if (!dbSecretStr) {
  console.error('Failed to retrieve secret from Secrets Manager');
  return;
}

Issue: According to aws-secrets-manager.ts:56, getSecret() can return undefined for ResourceNotFoundException, but throws for other errors. The current code doesn't catch thrown errors, which could crash the application.

Recommendation: Add proper error handling:

try {
  const dbSecretStr = await vault.getSecret(dbSecretArn);
  if (!dbSecretStr) {
    console.error(`Secret not found in Secrets Manager: ${dbSecretArn}`);
    return;
  }
  // ... rest of the code
} catch (error) {
  console.error('Failed to retrieve secret from Secrets Manager:', error);
  return;
}

2. Inconsistency with server-doj Implementation

Location: apps/sandbox/src/index.ts:30 vs apps/server-doj/src/index.ts:36

The server-doj implementation parses the secret and extracts dbHost, dbPort, and dbName from within the secret itself (line 38), while sandbox passes these as separate environment variables.

RDS-managed secrets format: When using manageMasterUserPassword: true, AWS creates a secret with this structure:

{
  "engine": "postgres",
  "host": "...",
  "port": 5432,
  "dbname": "postgres",
  "username": "postgres",
  "password": "..."
}

Issue: The sandbox code expects only username and password in the secret, but RDS-managed secrets include ALL connection details. This mismatch could cause issues or require unnecessary duplication of configuration.

Recommendation: Update apps/sandbox/src/index.ts to use the full secret like server-doj does, eliminating the need for separate DB_HOST, DB_PORT, DB_NAME env vars:

const dbSecret = JSON.parse(dbSecretStr);
if (!dbSecret.username || !dbSecret.password || !dbSecret.host || !dbSecret.port || !dbSecret.dbname) {
  console.error('Secret from Secrets Manager is missing required fields');
  return;
}

return {
  dbUri: `postgresql://${dbSecret.username}:${dbSecret.password}@${dbSecret.host}:${dbSecret.port}/${dbSecret.dbname}`,
};

Then update the infrastructure code to only pass the secret ARN, not the individual connection parameters.

3. Commented-Out CI Steps

Location: .github/workflows/_validate.yml:69-84

Performance budget checks for Spotlight and Design packages have been commented out.

Question: Is this intentional for this PR, or should it be in a separate commit? The PR title doesn't mention this change, suggesting it might be unrelated.

Recommendation:

• If temporary: Add a TODO comment explaining when/why these will be re-enabled
• If permanent: Remove the code entirely rather than commenting it out
• If unrelated: Consider moving to a separate PR

---

🧪 Testing Concerns

No test coverage: There are no tests for getAppRunnerSecrets() in either implementation.

Recommendation: Add unit tests covering:

• Successful secret retrieval and parsing
• Missing environment variables
• Malformed secret JSON
• AWS Secrets Manager errors
• Missing fields in parsed secret

---

🔒 Security Assessment

✅ Good practices:

• Using AWS-managed password rotation
• Secrets stored in AWS Secrets Manager (not in code/env directly)
• Using ARNs to reference secrets

⚠️ Consider:

• Add IAM policy validation ensuring App Runner has minimal required permissions (secretsmanager:GetSecretValue only)
• Consider implementing secret caching to reduce AWS API calls (Secrets Manager charges per 10,000 API calls)

---

📝 Minor Suggestions

1. Variable naming consistency: The PR renamed dbSecretStr → more explicit, but consider using dbSecretJson to indicate it's JSON that needs parsing.

2. Error messages: Use structured logging instead of console.error for production code (consider adding a logger).

3. Type safety: Consider defining a TypeScript interface for the RDS secret structure:

interface RDSManagedSecret {
  engine: string;
  host: string;
  port: number;
  dbname: string;
  username: string;
  password: string;
}

---

📋 Summary

This is a solid improvement to secret management, but needs attention to:

1. Critical: Add try-catch error handling for secret retrieval
2. Important: Align with RDS-managed secret format (includes host/port/dbname)
3. Minor: Clarify commented-out CI steps

The infrastructure changes look good - using RDS-managed passwords is definitely the right approach!