Skip to content

Skip Key Vault secret refs in the container app until Stripe keys are set - #416

Merged
gerardrecinto merged 1 commit into
masterfrom
fix-containerapp-optional-stripe-secrets
Oct 2, 2026
Merged

gerardrecinto merged 1 commit into
masterfrom
fix-containerapp-optional-stripe-secrets

Conversation

@gerardrecinto

Copy link
Copy Markdown
Collaborator

The Azure deploy has never completed. Infra provisions, but the Container App revision fails with "unable to fetch secret 'stripe-secret-key' using Managed identity" because it always references the three Stripe secrets in Key Vault, even when no real Stripe keys were supplied (Key Vault just holds the placeholder 'unset').

This adds a stripeEnabled flag to the container app module. main.bicep sets it to true only when both stripeSecretKey and stripeWebhookSecret are non-empty. While it is false, the app gets no Key Vault secret references and no STRIPE_* env vars, so it starts in simulation mode (the existing behavior when no keys are set). Once the real keys are added as repo secrets and the deploy re-runs, the references come back.

Checked: az bicep build passes. The identity, role assignment, and secrets in Key Vault all look correct, so I could not find why the Key Vault read fails; this change takes it out of the first-deploy path. If it still fails after real keys are set, that needs a look at the vault's diagnostic logs.

Not changed: Key Vault module, workflow, parameters.

Thanks, Gerard Recinto

@gerardrecinto gerardrecinto self-assigned this Oct 2, 2026
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Gemini PR Review

Reviewed commit: 03c73366619dc5dad48005f41079d93aaac3fe17
Verdict: PASS

  • Correctness: The infra/azure/modules/container-app.bicep module assumes the application code correctly handles the absence of Stripe-related environment variables and Key Vault secret references when the stripeEnabled parameter is false. The description While false the app gets no Key Vault secret references and runs in simulation mode implies this is the case, but without seeing the application code, this is an implicit dependency. If the application would crash or malfunction without these variables, even in "simulation mode", this could be a bug. However, based on the provided context, the Bicep logic itself correctly implements the conditional deployment.

@gerardrecinto
gerardrecinto force-pushed the fix-containerapp-optional-stripe-secrets branch from 03c7336 to b0f712b Compare October 2, 2026 05:33
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Gemini PR Review

Reviewed commit: b0f712b29f0ae9d10e17f5190cb9d747efe408d0
Verdict: PASS

  • No actionable findings.

@gerardrecinto
gerardrecinto force-pushed the fix-containerapp-optional-stripe-secrets branch from b0f712b to 362bfdf Compare October 2, 2026 06:04
@gerardrecinto

Copy link
Copy Markdown
Collaborator Author

/gemini review

2 similar comments
@gerardrecinto

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gerardrecinto

Copy link
Copy Markdown
Collaborator Author

/gemini review

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Gemini PR Review

Reviewed commit: 362bfdfd3a97b1ee9944cddc26ab9d35a6398eb7
Verdict: FAIL

  • Correctness Bug: In infra/azure/main.bicep, the stripeEnabled flag is calculated based on !empty(stripeSecretKey) && !empty(stripeWebhookSecret). If stripePublishableKey (which is also conditionally deployed as a secret) were provided but either of the other two secrets were missing, stripeEnabled would be false, and stripePublishableKey would not be mounted to the container app. This assumes that stripePublishableKey is always treated as a "real Stripe secret" that is only needed when full Stripe integration is enabled. If the application requires STRIPE_PUBLISHABLE_KEY even when running in "simulation mode" (e.g., for front-end usage with dummy data), then its conditional deployment is incorrect, leading to a missing environment variable. The diff, however, does not provide enough context on the application's behavior in "simulation mode" to confirm this as a bug versus intended behavior. Given the description in container-app.bicep ("While false the app gets no Key Vault secret references and runs in simulation mode."), it implies all Stripe-related secrets are absent in simulation mode.

This result blocks merge. Push a fix and comment /gemini review. To contest a finding, see the Gemini dispute process in CONTRIBUTING.md.

@gerardrecinto

Copy link
Copy Markdown
Collaborator Author

/gemini dispute

  1. Finding, quoted: "If stripePublishableKey ... were provided but either of the other two secrets were missing, stripeEnabled would be false, and stripePublishableKey would not be mounted ... If the application requires STRIPE_PUBLISHABLE_KEY even when running in 'simulation mode' ... then its conditional deployment is incorrect." The review itself says the diff does not give enough context to confirm this as a bug.

  2. Location: infra/azure/main.bicep, stripeEnabled: !empty(stripeSecretKey) && !empty(stripeWebhookSecret), and the conditional secrets and env blocks in infra/azure/modules/container-app.bicep.

  3. Claim type: Inferred, and the application code shows it does not apply.

  4. Repository evidence, tools/httpserver/billing_handler.go on master:

    • getBillingService: simulate := simulateStr == "true" || simulateStr == "1" || secretKey == "". Simulation mode is defined by the secret key being empty. A publishable key alone never enables real billing.
    • STRIPE_PUBLISHABLE_KEY is read through firstNonEmpty(...), so an unset value is the empty string with no error or panic.
    • The only use of the publishable key is the publishable_key field in the billing status JSON response, next to simulate_mode. A search for publishable_key and PUBLISHABLE across the repo's Go, JS, TS and HTML finds no frontend consumer. Checkout is created server-side through the Checkout Session code path.
  5. Docs: Stripe states publishable keys are only needed client-side for Stripe.js, which this app does not load: https://docs.stripe.com/keys

  6. Reachability: the case described needs a publishable key without a secret key or webhook secret. In that case the app is in simulation mode by the code above, and an empty publishable_key in the status JSON is correct and harmless. With both required secrets present, stripeEnabled is true and all three secrets and env vars are mounted, as before.

  7. Reproduction: az bicep build --file infra/azure/main.bicep passes. The previous reviews of this same diff on b0f712b2 and 03c73366 returned PASS. The current commit is a rebase of that diff onto master with no content change.

  8. Compensating control: the Key Vault still stores all three values. Only the Container App mounting is conditional, and re-running the deploy after real keys are set mounts them.

  9. Resolution requested: treat as not applicable and return PASS on the current commit.

@gerardrecinto

Copy link
Copy Markdown
Collaborator Author

/gemini review

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Gemini PR Review

Reviewed commit: 362bfdfd3a97b1ee9944cddc26ab9d35a6398eb7
Verdict: PASS

  • The dispute regarding the conditional deployment of stripePublishableKey has been verified against the provided diff and repository evidence. The application's billing_handler.go code demonstrates that the application enters simulation mode when STRIPE_SECRET_KEY is empty, and in this mode, STRIPE_PUBLISHABLE_KEY is not required client-side (as Stripe.js is not loaded) and is handled benignly as an empty string server-side if unset or unmounted. Thus, the scenario described in the original finding does not result in a bug. The stripeEnabled condition correctly aligns with the application's operational requirements. The prior finding is therefore withdrawn.

@gerardrecinto
gerardrecinto merged commit e6fe608 into master Oct 2, 2026
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant