Use DB_SECRET env var and deploy env IDs

[danielnaab]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: c77bd2e

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]

Code Review: Use DB_SECRET env var and deploy env IDs

Summary

This PR simplifies database secret handling by switching from AWS Secrets Manager ARN lookups to direct environment variable injection. It also updates deployment command naming for better clarity. Overall, this is a positive change that reduces complexity and improves maintainability.

---

✅ Strengths

1. Simplified Architecture: Removing the runtime AWS Secrets Manager API call reduces latency, eliminates a potential failure point, and removes an AWS SDK dependency from the runtime path.

2. Consistent Naming: The deployment command renaming (deploy:aws-* → deploy:flexion-sandbox-*) provides better clarity about which infrastructure is being deployed.

3. Clear Error Messages: The error handling in apps/sandbox/src/index.ts:22-26 properly validates the parsed JSON structure.

---

🔍 Issues & Concerns

1. Unused Import (apps/sandbox/src/index.ts:2)

import { getAWSSecretsManagerVault } from '@flexion/forms-infra-core';

This import is no longer used after switching to DB_SECRET env var. This should be removed to keep the codebase clean.

2. Security: Secrets in Environment Variables

While using environment variables for secrets is common in AWS App Runner, be aware that:

• Environment variables are visible in CloudWatch logs if logging configuration is verbose
• They're visible to anyone with access to the App Runner console
• Consider whether the existing IAM permissions appropriately restrict access to these values

AWS Secrets Manager provides additional audit logging and rotation capabilities that are now lost. Ensure this tradeoff is acceptable for your security requirements.

3. Error Handling: JSON Parse Safety (apps/sandbox/src/index.ts:21)

const dbSecret = JSON.parse(dbSecretStr);

This will throw an uncaught exception if DB_SECRET contains invalid JSON. Consider wrapping in a try-catch:

let dbSecret;
try {
  dbSecret = JSON.parse(dbSecretStr);
} catch (error) {
  console.error('Failed to parse DB_SECRET as JSON:', error);
  return;
}

4. Inconsistency: server-doj Still Uses Old Pattern (apps/server-doj/src/index.ts:24)

The server-doj app still uses DB_SECRET_ARN and makes runtime AWS Secrets Manager calls. This creates inconsistency:

• Different deployment patterns for different apps
• Maintained complexity in one place but not another
• Potential confusion for developers

Recommendation: Update server-doj to match the new pattern, or document why the two apps diverge.

5. Missing Test Coverage

The existing test (apps/sandbox/tests/integration.test.ts) uses in-memory database and doesn't exercise the getAppRunnerSecrets() function. Consider adding unit tests for:

• Valid secret parsing
• Invalid JSON handling
• Missing required fields in the secret object
• Missing environment variables

---

📝 Minor Issues

1. Naming Inconsistency (infra/cdktf/package.json:24-25)
   The script names use flexion-sandbox-* but the DEPLOY_ENV still uses aws-*. This could be confusing. Consider:

"deploy:flexion-sandbox-main": "DEPLOY_ENV=flexion-sandbox-main cdktf deploy",
"deploy:flexion-sandbox-demo": "DEPLOY_ENV=flexion-sandbox-demo cdktf deploy",

Or keep the current approach but document why they differ.

---

🎯 Recommendations

Before merging:

1. Remove the unused getAWSSecretsManagerVault import
2. Add try-catch around JSON.parse()
3. Add unit tests for the secret parsing logic

Follow-up (separate PRs):

1. Update apps/server-doj to use the same pattern or document the divergence
2. Consider documenting the security tradeoffs in a decision log

---

✨ Overall Assessment

Approved with minor changes requested. The architectural simplification is sound, but the code needs error handling improvements and cleanup before merging. The inconsistency with server-doj should be addressed in a follow-up.

---

Files Reviewed:

• .github/workflows/deploy.yml ✅
• apps/sandbox/src/index.ts ⚠️ (needs fixes)
• infra/cdktf/README.md ✅
• infra/cdktf/package.json ✅
• infra/cdktf/src/lib/aws/sandbox-stack.ts ✅