fix(security): SECRET_KEY, CORS wildcard, SQL injection, command injection (#193 #195 #196 #197 #200) - #207
Merged
Conversation
…#196 #197 #200 Resolves conflicts manually — the conflicting hunks were all in tests/e2e/pipeline.test.ts and .github/workflows/e2e-tests.yml which were already fixed better in #206. The security-relevant changes are applied cleanly: - code_generator.py (#193): replace hardcoded SECRET_KEY with os.getenv/secrets.token_urlsafe - code_generator.py (#195): restrict CORS allow_origins from ["*"] to localhost origins - real_api_endpoints.py (#196): read ALLOWED_ORIGINS from env; default to localhost origins - database_cleanup_service.py (#197): validate table name with regex before SQL use; quote safe_table_name with double quotes for PRAGMA and DELETE statements - deployment_manager.py (#200): add path traversal guard (resolve + is_dir check); add --ignore-scripts to npm install; use resolved_path for all cwd args - tests/unit/test_database_cleanup_security.py: new unit tests for SQL injection prevention Closes #193 #195 #196 #197 #200 https://claude.ai/code/session_01AgA9F82EwazbdB5R2f9nsd
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR hardens backend security across four dimensions: environment-driven secret management and CORS allowlisting in generated FastAPI apps, production API CORS restriction, safe deployment path resolution, and SQL injection prevention in database cleanup operations. All changes are backed by security tests. ChangesSecurity Hardening for FastAPI Backend and Database Operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🔴 E2E Test Results: FAILURE DETECTED
Test Output |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces several security enhancements, including dynamic SECRET_KEY generation, restricted CORS origins, path traversal validation during project verification, and SQL injection prevention in the database cleanup service. The feedback suggests ensuring that dynamically generated SECRET_KEYs do not disrupt multi-worker environments, verifying that resolved paths are constrained to allowed directories to fully mitigate path traversal, and stripping whitespace from the ALLOWED_ORIGINS environment variable to prevent CORS preflight failures.
| auth_code = ''' | ||
| # Authentication setup | ||
| SECRET_KEY = "your-secret-key-here" | ||
| SECRET_KEY = os.getenv("SECRET_KEY", secrets.token_urlsafe(32)) |
Contributor
There was a problem hiding this comment.
Generating a random SECRET_KEY on startup using secrets.token_urlsafe(32) is a great security improvement over a hardcoded key. However, in multi-worker production environments (e.g., running Uvicorn with --workers), each worker process will generate a different key. This will cause intermittent authentication failures as requests are load-balanced across workers.\n\nConsider adding a startup warning or documenting that SECRET_KEY must be explicitly set in the environment for multi-worker or multi-instance deployments.
Suggested change
| SECRET_KEY = os.getenv("SECRET_KEY", secrets.token_urlsafe(32)) | |
| SECRET_KEY = os.getenv("SECRET_KEY")\nif not SECRET_KEY:\n import logging\n logging.warning("SECRET_KEY env var is missing! Generating a random key. This will break sessions/JWTs across restarts and multi-worker deployments.")\n SECRET_KEY = secrets.token_urlsafe(32) |
Comment on lines
+113
to
+120
| try: | ||
| resolved_path = project_dir.resolve() | ||
| if not resolved_path.is_dir(): | ||
| result["summary"] = "Invalid project path: not a directory" | ||
| return result | ||
| except Exception as e: | ||
| result["summary"] = f"Invalid project path: {e}" | ||
| return result |
Contributor
There was a problem hiding this comment.
While calling resolve() resolves relative path segments (like ..), it does not prevent path traversal on its own unless you verify that the resolved path remains within an allowed base directory. An attacker could still pass a path like /etc or ../../../../etc which resolves to a valid directory outside the workspace, allowing arbitrary directory access or command execution (via npm install in arbitrary directories).\n\nTo prevent path traversal, verify that the resolved path is relative to either the project root (uvai_root) or the system temporary directory.
try:\n resolved_path = project_dir.resolve()\n if not resolved_path.is_dir():\n result["summary"] = "Invalid project path: not a directory"\n return result\n import tempfile\n temp_path = Path(tempfile.gettempdir()).resolve()\n if not (resolved_path.is_relative_to(uvai_root.resolve()) or resolved_path.is_relative_to(temp_path)):\n result["summary"] = "Access denied: path is outside allowed directories"\n return result\n except Exception as e:\n result["summary"] = f"Invalid project path: {e}"\n return result
Comment on lines
+457
to
+460
| allowed_origins = os.getenv( | ||
| "ALLOWED_ORIGINS", | ||
| "http://localhost:3000,http://localhost:5173,http://localhost:8080,http://localhost:3001" | ||
| ).split(",") |
Contributor
There was a problem hiding this comment.
When splitting ALLOWED_ORIGINS by comma, any leading or trailing whitespace in the environment variable (e.g., http://localhost:3000, http://localhost:5173) will result in invalid origin strings with spaces. This will cause CORS preflight requests to fail in the browser.\n\nConsider stripping whitespace from each origin after splitting.
allowed_origins = [\n origin.strip()\n for origin in os.getenv(\n "ALLOWED_ORIGINS",\n "http://localhost:3000,http://localhost:5173,http://localhost:8080,http://localhost:3001"\n ).split(",")\n if origin.strip()\n ]![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+8
to
+12
| from youtube_extension.backend.services.database_cleanup_service import ( | ||
| CleanupResult, | ||
| DatabaseCleanupService, | ||
| RetentionPolicy, | ||
| ) |
groupthinking
added a commit
that referenced
this pull request
May 27, 2026
![v0[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
* chore: trigger uvai.io production deploy * feat: UVAI UI/UX full refactor — features page, pricing, Nav, LandingNav (#205) Zero emoji, real product mockups per feature section, SVG icons everywhere, LandingNav with proper cross-page routing and active states. Co-authored-by: v0[bot] <v0[bot]@users.noreply.github.com> * test: improve test coverage, fix Vercel build, dashboard auto-select, E2E resilience (#206) - 155 new unit tests (middleware, API models, error handling) - Fix Vercel ERESOLVE build failure via .npmrc legacy-peer-deps - Fix dashboard ?video= URL param auto-select (issue #159) - Fix next.config.js duplicate redirects/headers - Fix CI test dependency installation for Python 3.12 - Make SSE stream always emit terminal pipeline_status event - Make E2E tests resilient to live server degraded mode - Replace banned dQw4w9WgXcQ video ID with auJzb1D-fag throughout * fix: remove hardcoded Grok API key (#194) 🎯 What: Removed the hardcoded fallback value for the GROK_API_KEY in TriModelConsensusTool.⚠️ Risk: Hardcoded API keys in source code can be exploited if the codebase is exposed or leaked, leading to unauthorized API access, quota exhaustion, and potential financial loss. 🛡️ Solution: Removed the hardcoded string so the tool relies strictly on the environment variable, aligning with secure configuration management practices. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> * feat(web): add privacy/terms/api-docs/app/login routes; noindex prototype pages (#204) Resolves 404s on /privacy, /terms, /api/docs, /app, /login and ensures /prototype carries a noindex robots tag. Scope is intentionally narrow and does not overlap PR #202 (assets, robots, sitemap, JSON-LD, a11y). - /privacy, /terms: server-rendered legal placeholder pages with proper metadata, canonical URLs, and footer links. Plain-language, startup- friendly; will be replaced before enterprise contracts. - /api/docs: human-readable reference matching the documentation pointer returned by /api JSON. Lists actual /api/* routes that exist in code. - /app, /login: server redirects to /dashboard, marked noindex. UVAI has no auth gate today, so this matches actual product behavior. - /prototype: adds a route layout with robots.index=false because the underlying page is an internal prototype spec, not a public surface. - LandingFooter: surfaces Privacy and Terms links now that the pages exist. Build: next build succeeds, 25 routes generated. Type-check and ESLint clean. Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> * test: add unit tests for DatabaseOptimizer._calculate_performance_grade (#182) Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> * fix(security): SECRET_KEY, CORS wildcard, SQL injection, command injection (#207) Resolves conflicts manually — the conflicting hunks were all in tests/e2e/pipeline.test.ts and .github/workflows/e2e-tests.yml which were already fixed better in #206. The security-relevant changes are applied cleanly: - code_generator.py (#193): replace hardcoded SECRET_KEY with os.getenv/secrets.token_urlsafe - code_generator.py (#195): restrict CORS allow_origins from ["*"] to localhost origins - real_api_endpoints.py (#196): read ALLOWED_ORIGINS from env; default to localhost origins - database_cleanup_service.py (#197): validate table name with regex before SQL use; quote safe_table_name with double quotes for PRAGMA and DELETE statements - deployment_manager.py (#200): add path traversal guard (resolve + is_dir check); add --ignore-scripts to npm install; use resolved_path for all cwd args - tests/unit/test_database_cleanup_security.py: new unit tests for SQL injection prevention Closes #193 #195 #196 #197 #200 https://claude.ai/code/session_01AgA9F82EwazbdB5R2f9nsd Co-authored-by: Claude <noreply@anthropic.com> * chore: move legacy .agent content under .github (#162) * chore: move legacy agent files into .github Agent-Logs-Url: https://github.com/groupthinking/EventRelay/sessions/8e79c6e5-9755-40a2-b8b0-73b9ff75249e Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com> * docs: fix relocated agent references Agent-Logs-Url: https://github.com/groupthinking/EventRelay/sessions/8e79c6e5-9755-40a2-b8b0-73b9ff75249e Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com> * docs: remove vague agent rule reference Agent-Logs-Url: https://github.com/groupthinking/EventRelay/sessions/8e79c6e5-9755-40a2-b8b0-73b9ff75249e Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com> * chore(web): UVAI Phase 1 — SEO, a11y, missing static assets Live uvai.io referenced /favicon.ico, /icon.svg, /apple-touch-icon.png but apps/web/public/ did not exist in the repo, producing 404s. Layout metadata also pointed metadataBase and og.url at the legacy v0-uvai.vercel.app host rather than the canonical uvai.io domain. Changes: - Add apps/web/public with favicon.ico (multi-res), icon.svg, apple-touch-icon.png, og-image.png (1200x630), manifest.json, robots.txt. - Add apps/web/src/app/sitemap.ts (Next.js Metadata Route sitemap). - layout.tsx: metadataBase + og.url -> https://uvai.io, add alternates.canonical, inject Organization/WebSite/SoftwareApplication JSON-LD, add skip-to-main link. - page.tsx: <main id=\"main\"> as skip-link target. - LandingNav.tsx: aria-label=\"Primary\" on nav, aria-labels on brand + GitHub external link, visible focus rings on all interactive elements. - HeroSection.tsx: focus rings on CTAs, honor prefers-reduced-motion for marquee. - Add CHANGELOG.md with timestamped entry. Local verification: - npx eslint on touched files: clean - npm run build (apps/web): success, /sitemap.xml route generated, TypeScript clean No production / deploy / DNS / secret changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(web): PR #202 review fixes — no dangerouslySetInnerHTML, MD022, skip-link in layout, central SITE_URL Addresses code review feedback on PR #202. 1. layout.tsx: replace dangerouslySetInnerHTML JSON-LD with React <script>{jsonLdString}</script> children. jsonLdString escapes `<` -> `<` to prevent any nested `</script>` breakout. Build verified: rendered HTML contains exactly one valid JSON-LD block. Complies with repo policy that forbids dangerouslySetInnerHTML. 2. CHANGELOG.md: markdownlint MD022 — blank lines after `#### Added`, `### Changed`, and `### Notes / known follow-ups (not in this change)`. 3. Skip-to-main-content target moved from `apps/web/src/app/page.tsx`'s <main> to the root layout's content wrapper. The link now works on every route (dashboard, pricing, features, playground, prototype, not-found), not just the homepage. Duplicate `id="main"` removed from page.tsx — rendered HTML on `/` now contains exactly one `id="main"`. 4. Add apps/web/src/lib/site.ts exporting SITE_URL = 'https://uvai.io'. layout.tsx (metadataBase, alternates.canonical, og.url, JSON-LD URLs) and sitemap.ts both consume it. Pricing/playground references kept as-is — those are mailto: addresses and api.uvai.io examples in code samples, not the same axis as the site origin. Failing CI check (E2E Pipeline Tests) is unrelated to this PR: - E2E runs vitest against BASE_URL=https://uvai.io (the live deployment) - Live root returns 200 but is stale (title still "UVAI — Video to Software") - This PR touches zero files under tests/e2e/, src/youtube_extension/, or apps/web/src/app/api/ - Resolution requires a redeploy of the current main, which is outside this PR's scope per the original instructions Local verification: - npx eslint on touched files: clean (exit 0) - npm run build (apps/web): ✓ Compiled, TypeScript clean, 21 pages - Rendered HTML inspection: JSON-LD block present and well-formed; id="main" present on /, /pricing, /features, /dashboard, /playground; exactly one id="main" on each prerendered page (no duplicates) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: v0[bot] <v0[bot]@users.noreply.github.com> Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Claude Code <claude-code@anthropic.com>
This was referenced Jul 30, 2026
This was referenced Aug 2, 2026
Closed
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applies five security fixes from PRs that had merge conflicts with #206. The conflicting hunks were all in
tests/e2e/pipeline.test.tswhich was already fixed better in #206; only the security-relevant changes are applied here.Changes
code_generator.pySECRET_KEY = \"your-secret-key-here\"withos.getenv(\"SECRET_KEY\", secrets.token_urlsafe(32))code_generator.pyallow_originsfrom[\"*\"]to explicit localhost originsreal_api_endpoints.pyALLOWED_ORIGINSfrom env var; default to localhost origins instead of[\"*\"]database_cleanup_service.py^[a-zA-Z0-9_]+$regex before SQL use; quote with double quotes for PRAGMA/DELETEdeployment_manager.py--ignore-scriptsto npm install; useresolved_pathfor allcwdargstests/unit/test_database_cleanup_security.pyCloses
Closes #193, #195, #196, #197, #200
https://claude.ai/code/session_01AgA9F82EwazbdB5R2f9nsd
Generated by Claude Code