-
Notifications
You must be signed in to change notification settings - Fork 37
update backend manifest with oauth route #475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update backend manifest with oauth route #475
Conversation
Signed-off-by: Michael Clifford <mcliffor@redhat.com>
Claude Code ReviewSummaryThis PR adds the BACKEND_URL environment variable to the backend deployment manifest, sourced from the google-workflow-app-secret Kubernetes Secret. This variable is used by the OAuth flow (introduced in PR #469 and #473) to construct redirect URIs for Google and GitHub OAuth callbacks. Overall Assessment: This is a straightforward configuration change that follows established patterns in the codebase. However, there are several critical issues that must be addressed before merging. Issues by SeverityBlocker Issues
Location: components/manifests/base/backend-deployment.yaml:97-102 Issue: The PR adds a reference to BACKEND_URL in the google-workflow-app-secret, but there is no documentation indicating what value should be set, how to create/configure this secret, or example values for different environments. Impact: Deployments will fail silently or use the fallback value (http://localhost:8080) which is incorrect for production environments. This breaks the OAuth flow. Evidence from code (components/backend/handlers/oauth.go:178-181): The backend falls back to localhost:8080 if BACKEND_URL is not set, which will break OAuth in production. Required Actions:
Location: components/manifests/base/backend-deployment.yaml vs components/manifests/base/operator-deployment.yaml Issue: The operator deployment also references google-workflow-app-secret for OAuth credentials (GOOGLE_OAUTH_CLIENT_ID, GOOGLE_OAUTH_CLIENT_SECRET), but does NOT include BACKEND_URL. This creates inconsistency. Required Actions:
Critical Issues
Location: components/backend/handlers/oauth.go:178-182 Issue: The code uses BACKEND_URL directly without validation. No check if URL is well-formed, no check if URL scheme is HTTPS (production requirement), no check if URL matches registered OAuth redirect URIs. Security Risk: Misconfigured BACKEND_URL could lead to OAuth redirect to incorrect URL, token leakage if URL is HTTP in production, or silent failures. Required Actions:
Location: OAuth flow in components/backend/handlers/oauth.go Issue: The BACKEND_URL must exactly match the redirect URI registered with OAuth providers (Google, GitHub). Mismatches cause OAuth failures. There is no validation that BACKEND_URL matches registered redirect URIs, no documentation of required OAuth configuration, and the fallback to localhost:8080 will break OAuth in all non-local environments. Required Actions:
Major Issues
Issue: No examples showing correct BACKEND_URL values for different environments (local development, minikube, production). Developers and operators will struggle to configure this correctly.
Location: components/manifests/base/backend-deployment.yaml:102 The secret key is marked as optional: true, which means backend will start even if BACKEND_URL is missing, OAuth flow will use fallback http://localhost:8080, and there will be no error logs or warnings until OAuth is attempted. Recommendation: Make BACKEND_URL required (optional: false) if OAuth is enabled, OR add startup validation. Minor Issues
Issue: The PR has an empty body. No context provided for why this change is needed, how to configure the new environment variable, what happens if it is not set, or links to related PRs. Positive Highlights
RecommendationsPriority 1 (Must Fix Before Merge):
Priority 2 (Should Fix Before Merge): Priority 3 (Future Improvements): ConclusionThis is a necessary configuration change to support the OAuth functionality introduced in recent PRs. The change itself is correct, but lacks critical documentation and validation that would prevent production misconfigurations. Recommendation: Request changes - address blocker issues #1-2 and critical issues #3-4 before merging. Once documentation and validation are added, this will be a solid, maintainable addition to the codebase. Review conducted using CLAUDE.md project standards, backend-development context, security-standards context, and error-handling patterns 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Signed-off-by: Michael Clifford <mcliffor@redhat.com>
No description provided.