Skip to content

fix: apply upload guards and rate limiting to /api/analyze_stream (#778)#780

Open
Suyash2527 wants to merge 3 commits into
neeru24:mainfrom
Suyash2527:fix/api-analyze-stream-guards-778
Open

fix: apply upload guards and rate limiting to /api/analyze_stream (#778)#780
Suyash2527 wants to merge 3 commits into
neeru24:mainfrom
Suyash2527:fix/api-analyze-stream-guards-778

Conversation

@Suyash2527

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR secures the POST /api/analyze_stream endpoint by applying the same upload guards and rate limiting protections that are currently in place for /api/analyze. Previously, this streaming endpoint was missing these protections, allowing attackers to bypass rate limits, size enforcement, and magic-byte validation while triggering full ML inference.

Related Issue

Closes #778

Changes made

  • Added @limiter.limit decorator to enforce the API_UPLOAD_RATE_LIMIT (e.g. 10 requests per minute).
  • Called enforce_request_size(get_upload_max_bytes()) to strictly cap the uploaded file size.
  • Replaced direct cv2.imdecode with read_validated_upload_image() to enforce magic-byte file validation and securely read the file into memory.
  • Added a try/except/finally block to ensure cleanup_temp_upload() is called, preventing temporary files from lingering in memory/disk after streaming completes.

Checklist

  • I have tested my changes
  • My code follows project guidelines
  • No new errors introduced

@github-actions

Copy link
Copy Markdown
Contributor

Thanks for creating a PR for your Issue! ☺️

We'll review it as soon as possible.
In the meantime, please double-check the file changes and ensure that all commits are accurate.

If there are any unresolved review comments, feel free to resolve them. 🙌🏼

@Suyash2527

Copy link
Copy Markdown
Contributor Author

Hi @neeru24 👋

I've pushed the fixes to patch the security bypass on the /api/analyze_stream endpoint. The missing upload guards, file validation, and rate limiting have now been implemented, and all the CI tests are passing cleanly! ✅

Since this addresses a critical vulnerability, could you please take a look when you have a moment? Also, as you are the project owner, could you kindly help assign the appropriate labels to this PR (such as security, bug, high-priority, and any relevant gssoc t level labels) so it gets tracked correctly?

Thank you! Let me know if you need any changes made to the code.

@Suyash2527

Copy link
Copy Markdown
Contributor Author

Hi @neeru24 👋

We noticed that the CI Run Unit Tests & Coverage action is failing after ~2 minutes. After investigating, we found that this is not caused by the new security patch in /api/analyze_stream, but rather by pre-existing issues in the test suite that were previously hidden.

Here is what's happening:

  1. The Hidden Crash on main: Before our PR, app.py was importing flask_talisman, but it was missing from requirements.txt. This caused the test suite to crash instantly (within seconds) with a ModuleNotFoundError, preventing any actual tests from running.
  2. Uncovering the True Failures: Our PR fixed this by adding Flask-Talisman to requirements.txt. Now that the tests can finally boot up, the test suite runs for its normal duration (~2 minutes to load the ResNet/YOLO ML models) and exposes underlying tests that were already broken on the main branch.
  3. Why the tests are failing now: By default, Flask-Talisman enforces strict security headers, including session_cookie_secure=True. This forces session cookies to only work over HTTPS. Since the unit tests use a Flask test client over http://localhost, tests that require login (like test_admin_auth.py and test_account_lockout.py) drop the session cookies and fail authentication.

We pushed a commit to disable session_cookie_secure during testing to help mitigate this, but any remaining failures are likely other pre-existing bugs in the main branch tests that we just uncovered.

Since the /api/analyze_stream endpoint doesn't currently have test coverage, our security patch there isn't the root cause of the red CI. Let me know how you'd like to proceed with the remaining main branch test failures!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

POST /api/analyze_stream Bypasses All Upload Guards — No Rate Limit, No File Validation, No Size Enforcement

1 participant