Skip to content

fix: harden security — PBKDF2 iterations, JWT validation, health endpoint, A2A auth, notification limit - #182

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1776014363-security-fixes
Open

fix: harden security — PBKDF2 iterations, JWT validation, health endpoint, A2A auth, notification limit#182
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1776014363-security-fixes

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

Security hardening across six files, addressing findings from a codebase security audit:

  • PBKDF2 iterations (password.py): Increased from 10,000 → 600,000 (OWASP minimum for SHA-256). compare_password tries the new count first, then falls back to legacy 10,000 so existing password hashes continue to work.
  • JWT secret key validation (jwt_service.py): New _get_secret_key() rejects empty or known-placeholder values (e.g. "your-random-secret-key-here-change-this") with an error log + 401 response instead of silently signing/verifying with None.
  • Health endpoint (app_handler.py): Public /health response no longer exposes internal components (database/Redis/Weaviate/Celery status) or metrics. Only status and service are returned. Component probes still run internally for alerting.
  • A2A endpoint auth (public_app_handler.py): Added @login_required to send_public_app_a2a_message, get_public_app_a2a_conversation_messages, and get_latest_public_app_a2a_conversation. The read-only agent card endpoint remains public.
  • Notification limit cap (notification_handler.py): limit query param clamped to max 100 to prevent unbounded DB reads.
  • Bare except:except Exception: in three places in public_app_handler.py.
  • TODO comment added to oauth_service.py flagging that encrypted_token is stored in plaintext.

Review & Testing Checklist for Human

  • A2A auth change may be incorrect: The A2A protocol is designed for machine-to-machine communication and typically uses API keys, not user login sessions. Verify that @login_required is the intended auth mechanism for these endpoints — if A2A clients don't carry a user session token, this will break them. Consider whether API-key auth via the openapi blueprint would be more appropriate.
  • Health endpoint is a breaking API change: Any external monitoring (Prometheus, uptime checkers, dashboards) that parses components or metrics from /health will stop receiving those fields. Confirm no monitoring depends on them, or plan a migration.
  • PBKDF2 backward compatibility has no re-hash path: Existing users will incur two PBKDF2 computations on every login (600k iterations attempt → fail → 10k fallback) indefinitely, since there is no logic to re-hash with the new count on successful login. Consider adding re-hashing on successful legacy verification if performance matters at scale.
  • JWT placeholder blocklist coverage: Review whether _INSECURE_PLACEHOLDER_KEYS covers all placeholder values actually used across deployment environments (e.g. Docker Compose defaults, CI fixtures).
  • Run the full test suite (pytest) — tests could not be fully executed locally due to deep dependency chain; CI results should be verified.

Notes

  • The get_public_app_a2a_card endpoint was intentionally left without @login_required since agent cards are typically public metadata in the A2A protocol.
  • No password migration script is included. Existing users' passwords will be re-hashed to 600k iterations only when they call change-password.

Link to Devin session: https://app.devin.ai/sessions/66b5b2fccab74dbf862f5c4e1321fe35
Requested by: @Haohao-end

…oint, A2A auth, notification limit

- Increase PBKDF2-SHA256 iterations from 10,000 to 600,000 (OWASP recommendation)
  with backward-compatible password verification for existing hashes
- Reject empty/placeholder JWT_SECRET_KEY values at runtime
- Remove internal component details from public /health endpoint
- Add @login_required to A2A message-sending and conversation endpoints
- Cap notification list limit to 100 to prevent large query DoS
- Replace bare except: clauses with except Exception:
- Add TODO for encrypting OAuth tokens at rest

Co-Authored-By: Haohao <2227625024@qq.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

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