-
Notifications
You must be signed in to change notification settings - Fork 30
Optimize detection service and add comprehensive analysis endpoint #369
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
base: main
Are you sure you want to change the base?
Changes from all commits
f16dc42
d586965
fb82905
1826bdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -122,18 +122,21 @@ async def lifespan(app: FastAPI): | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if not frontend_url: | ||||||||||||||||||||||||||||||
| if is_production: | ||||||||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||||||||
| "FRONTEND_URL environment variable is required for security in production. " | ||||||||||||||||||||||||||||||
| "Set it to your frontend URL (e.g., https://your-app.netlify.app)." | ||||||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||||||
| "FRONTEND_URL environment variable is MISSING in production. " | ||||||||||||||||||||||||||||||
| "Defaulting to wildcard '*' for CORS to allow startup. " | ||||||||||||||||||||||||||||||
| "PLEASE SET THIS IN RENDER DASHBOARD." | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| frontend_url = "*" # Allow all origins temporarily to fix deployment | ||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||
| logger.warning("FRONTEND_URL not set. Defaulting to http://localhost:5173 for development.") | ||||||||||||||||||||||||||||||
| frontend_url = "http://localhost:5173" | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if not (frontend_url.startswith("http://") or frontend_url.startswith("https://")): | ||||||||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||||||||
| f"FRONTEND_URL must be a valid HTTP/HTTPS URL. Got: {frontend_url}" | ||||||||||||||||||||||||||||||
| if frontend_url != "*" and not (frontend_url.startswith("http://") or frontend_url.startswith("https://")): | ||||||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||||||
| f"FRONTEND_URL format invalid: {frontend_url}. Expected HTTP/HTTPS URL. Using '*' as fallback." | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| frontend_url = "*" | ||||||||||||||||||||||||||||||
|
Comment on lines
+125
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: Wildcard CORS origin with When A safer fallback would be to either refuse to start or use a known safe origin. If you truly need a permissive deployment escape hatch, at minimum disable credentials when using a wildcard: Proposed fix if not frontend_url:
if is_production:
logger.warning(
"FRONTEND_URL environment variable is MISSING in production. "
- "Defaulting to wildcard '*' for CORS to allow startup. "
+ "Defaulting to wildcard '*' for CORS to allow startup (credentials will be DISABLED). "
"PLEASE SET THIS IN RENDER DASHBOARD."
)
frontend_url = "*"
else:
logger.warning("FRONTEND_URL not set. Defaulting to http://localhost:5173 for development.")
frontend_url = "http://localhost:5173"
if frontend_url != "*" and not (frontend_url.startswith("http://") or frontend_url.startswith("https://")):
logger.warning(
f"FRONTEND_URL format invalid: {frontend_url}. Expected HTTP/HTTPS URL. Using '*' as fallback."
)
frontend_url = "*"Then at the middleware registration: +allow_creds = frontend_url != "*"
+
app.add_middleware(
CORSMiddleware,
allow_origins=allowed_origins,
- allow_credentials=True,
+ allow_credentials=allow_creds,
allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"],
allow_headers=["*"],
)🤖 Prompt for AI Agents
Comment on lines
+135
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Logging a raw, potentially-sensitive env var value. Line 137 logs the full value of Consider masking or truncating the value: - logger.warning(
- f"FRONTEND_URL format invalid: {frontend_url}. Expected HTTP/HTTPS URL. Using '*' as fallback."
- )
+ logger.warning(
+ "FRONTEND_URL format invalid (does not start with http:// or https://). Using '*' as fallback."
+ )📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Security: Invalid URL silently falls back to wildcard CORS. If an operator sets Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| allowed_origins = [frontend_url] | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,6 @@ psycopg2-binary | |
| async-lru | ||
| huggingface-hub | ||
| httpx | ||
| python-magic | ||
| pywebpush | ||
| Pillow | ||
| firebase-functions | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,10 @@ | |||||||||||||
| import shutil | ||||||||||||||
| import logging | ||||||||||||||
| import io | ||||||||||||||
| import magic | ||||||||||||||
| try: | ||||||||||||||
| import magic | ||||||||||||||
| except ImportError: | ||||||||||||||
| magic = None | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Security: MIME type validation via Prompt for AI agents
Suggested change
|
||||||||||||||
| from typing import Optional | ||||||||||||||
|
|
||||||||||||||
| from backend.cache import user_upload_cache | ||||||||||||||
|
|
@@ -73,17 +76,18 @@ def _validate_uploaded_file_sync(file: UploadFile) -> Optional[Image.Image]: | |||||||||||||
|
|
||||||||||||||
| # Check MIME type from content using python-magic | ||||||||||||||
| try: | ||||||||||||||
| # Read first 1024 bytes for MIME detection | ||||||||||||||
| file_content = file.file.read(1024) | ||||||||||||||
| file.file.seek(0) # Reset file pointer | ||||||||||||||
| if magic: | ||||||||||||||
| # Read first 1024 bytes for MIME detection | ||||||||||||||
| file_content = file.file.read(1024) | ||||||||||||||
| file.file.seek(0) # Reset file pointer | ||||||||||||||
|
|
||||||||||||||
| detected_mime = magic.from_buffer(file_content, mime=True) | ||||||||||||||
| detected_mime = magic.from_buffer(file_content, mime=True) | ||||||||||||||
|
|
||||||||||||||
| if detected_mime not in ALLOWED_MIME_TYPES: | ||||||||||||||
| raise HTTPException( | ||||||||||||||
| status_code=400, | ||||||||||||||
| detail=f"Invalid file type. Only image files are allowed. Detected: {detected_mime}" | ||||||||||||||
| ) | ||||||||||||||
| if detected_mime not in ALLOWED_MIME_TYPES: | ||||||||||||||
| raise HTTPException( | ||||||||||||||
| status_code=400, | ||||||||||||||
| detail=f"Invalid file type. Only image files are allowed. Detected: {detected_mime}" | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| # Additional content validation: Try to open with PIL to ensure it's a valid image | ||||||||||||||
| try: | ||||||||||||||
|
|
@@ -158,15 +162,16 @@ def process_uploaded_image_sync(file: UploadFile) -> tuple[Image.Image, bytes]: | |||||||||||||
|
|
||||||||||||||
| # Check MIME type | ||||||||||||||
| try: | ||||||||||||||
| file_content = file.file.read(1024) | ||||||||||||||
| file.file.seek(0) | ||||||||||||||
| detected_mime = magic.from_buffer(file_content, mime=True) | ||||||||||||||
| if magic: | ||||||||||||||
| file_content = file.file.read(1024) | ||||||||||||||
| file.file.seek(0) | ||||||||||||||
| detected_mime = magic.from_buffer(file_content, mime=True) | ||||||||||||||
|
|
||||||||||||||
| if detected_mime not in ALLOWED_MIME_TYPES: | ||||||||||||||
| raise HTTPException( | ||||||||||||||
| status_code=400, | ||||||||||||||
| detail=f"Invalid file type. Only image files are allowed. Detected: {detected_mime}" | ||||||||||||||
| ) | ||||||||||||||
| if detected_mime not in ALLOWED_MIME_TYPES: | ||||||||||||||
| raise HTTPException( | ||||||||||||||
| status_code=400, | ||||||||||||||
| detail=f"Invalid file type. Only image files are allowed. Detected: {detected_mime}" | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| try: | ||||||||||||||
| img = Image.open(file.file) | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,8 @@ def validate_environment(): | |
| print(f" - {var}") | ||
| print("\nPlease set these variables or create a .env file.") | ||
| print("See backend/.env.example for reference.") | ||
| return False | ||
| # We don't return False here to allow the app to start and serve health check | ||
| print("⚠️ Proceeding with missing variables (some features may be broken)") | ||
|
Comment on lines
+32
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Misleading success message when required variables are missing. After printing " Proposed fix+ has_missing = False
if missing_vars:
print("❌ Missing required environment variables:")
for var in missing_vars:
print(f" - {var}")
print("\nPlease set these variables or create a .env file.")
print("See backend/.env.example for reference.")
print("⚠️ Proceeding with missing variables (some features may be broken)")
+ has_missing = True
# ... optional variable defaults ...
- print("✅ Environment validation passed")
+ if not has_missing:
+ print("✅ Environment validation passed")
+ else:
+ print("⚠️ Environment validation completed with warnings")
return TrueAlso applies to: 51-51 🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When required environment variables are missing, the function now falls through to unconditionally print Prompt for AI agents |
||
|
|
||
| # Set defaults for optional variables | ||
| if not os.getenv("DATABASE_URL"): | ||
|
|
@@ -60,8 +61,7 @@ def main(): | |
| """Main startup function""" | ||
| print("🚀 Starting VishwaGuru Backend") | ||
|
|
||
| if not validate_environment(): | ||
| sys.exit(1) | ||
| validate_environment() | ||
|
|
||
| create_data_directory() | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: Security: Wildcard CORS with credentials in production. Replacing
raise ValueErrorwith a fallback to"*"removes a critical security guardrail. Becauseallow_credentials=Trueis set on the CORS middleware (line 156), Starlette will reflect the requestingOriginheader back instead of sending*, meaning any website can make authenticated requests to this API. The previous behavior of refusing to start without a validFRONTEND_URLwas the correct, secure default. "Temporary" workarounds like this tend to become permanent. Restore the hard failure, or at minimum setallow_credentials=Falsewhen using a wildcard origin.Prompt for AI agents