Skip to content

fix(security) : Insecure defaults allow session hijacking via cookie sniffing and IP spoofing #499

Open
prince-shakyaa wants to merge 8 commits into
GenAI-Security-Project:mainfrom
prince-shakyaa:fix/insecure-config-defaults
Open

fix(security) : Insecure defaults allow session hijacking via cookie sniffing and IP spoofing #499
prince-shakyaa wants to merge 8 commits into
GenAI-Security-Project:mainfrom
prince-shakyaa:fix/insecure-config-defaults

Conversation

@prince-shakyaa
Copy link
Copy Markdown

@prince-shakyaa prince-shakyaa commented May 15, 2026

Summary

Fixes two critical insecure defaults in the application's configuration and middleware, ensuring the platform is secure when deployed to production.

Fixes #498


Changes

1. Secure Session Cookies by Default

The default SESSION_COOKIE_SECURE has been changed from False to True in finbot/config.py.

  • Session cookies will now only be transmitted over encrypted HTTPS connections by default, mitigating Man-in-the-Middle (MitM) cookie theft.
  • Local Dev Note: For local development over http://localhost, developers can opt-out by setting SESSION_COOKIE_SECURE=false in their .env file. This override is now commented out by default in .env.example to ensure raw copy-pastes to production servers remain secure.

2. Opt-in IP Hijack Heuristics for PaaS Compatibility

The ProxyHeadersMiddleware previously trusted ["*"] globally. In an earlier iteration of this PR, it was restricted to 127.0.0.1, which caused operational failure and false-positive hijack alarms on PaaS platforms (like Railway or Fly) that use churning internal proxy IPs.

Following reviewer feedback, the architecture has been revised:

  • Default (Unset): TRUSTED_PROXY_IPS defaults to unset. The app falls back to trusting ["*"] so PaaS deployments function correctly. The application emits a startup warning noting the degraded security state, and the SessionContext safely falls back to logging IP changes via a structured extra={"trusted_source": False} flag at the INFO level to maintain forensic queryability in downstream log shippers without triggering strict hijack alerts.
  • Opt-in (Set): Operators who configure their edge proxy to strip inbound X-Forwarded-For headers can set TRUSTED_PROXY_IPS (e.g., 10.0.0.0/8) in their environment. This explicitly enables the strong, trusted IP-spoofing hijack heuristics.

3. Test Coverage Added

  • tests/unit/core/test_proxy_middleware_startup.py: Mocks the uvicorn logger to verify the startup warning correctly fires when TRUSTED_PROXY_IPS is unset, and stays silent when configured.
  • tests/unit/auth/test_session_ip_logging.py: A parameterised test asserting that the structured log flag correctly downgrades to app.trusted_source=False when proxy IPs are untrusted.
  • tests/unit/core/test_proxy_middleware_behavior.py: Mathematically asserts uvicorn's right-to-left traversal logic, verifying that IP spoofing is successfully mitigated (and documenting the chained-header vulnerability that occurs if an edge proxy appends instead of strips).
  • tests/unit/core/test_config_defaults.py: A hermetic isolation test (chdir(tmp_path) + environment wipe) that explicitly pins the SESSION_COOKIE_SECURE=True code-level fallback.

Testing locally

  1. Try to sign in locally over HTTP (localhost) without .env configured. The session cookie will not be set on your browser (because Secure=True).
  2. Add SESSION_COOKIE_SECURE=false to .env and restart. The session cookie is now set.
  3. Check application startup logs: you will see the warning TRUSTED_PROXY_IPS is unset. Client IPs are untrusted and IP-based hijack detection is disabled.
  4. Set TRUSTED_PROXY_IPS=127.0.0.1 in your .env and restart. The warning disappears, and strict IP verification is active.
  5. Run the new tests: pytest tests/unit/core/test_proxy_middleware_startup.py tests/unit/auth/test_session_ip_logging.py

- Change SESSION_COOKIE_SECURE default from False to True so session
  cookies require HTTPS by default. Developers on local HTTP must set
  SESSION_COOKIE_SECURE=false in their .env (documented in .env.example).

- Replace ProxyHeadersMiddleware trusted_hosts=["*"] with an env-
  configurable list (TRUSTED_PROXY_IPS, defaults to 127.0.0.1) to
  prevent IP spoofing via forged X-Forwarded-For headers.

- Add Security section to .env.example documenting both settings.

Fix issue GenAI-Security-Project#498
@prince-shakyaa prince-shakyaa changed the title [Security] Insecure defaults allow session hijacking via cookie sniffing and IP spoofing (A05:2021) [Security] Insecure defaults allow session hijacking via cookie sniffing and IP spoofing May 15, 2026
@prince-shakyaa
Copy link
Copy Markdown
Author

prince-shakyaa commented May 15, 2026

Hii @saikishu @e2hln,
I've hardened our defaults by setting SESSION_COOKIE_SECURE=True and making TRUSTED_PROXY_IPS an opt-in env var for strict IP spoofing checks. To ensure PaaS compatibility (like Railway), the proxy falls back to ["*"] when unset while gracefully logging untrusted IP changes via app.trusted_source=False. I've also added comprehensive unit/integration tests to ensure this is completely bulletproof.
Let me know if you need any changes.
Thank You.

@prince-shakyaa prince-shakyaa changed the title [Security] Insecure defaults allow session hijacking via cookie sniffing and IP spoofing fix(security) : Insecure defaults allow session hijacking via cookie sniffing and IP spoofing May 18, 2026
Prince Shakya added 7 commits May 19, 2026 12:48
Reverts the ProxyHeadersMiddleware trusted_hosts configuration back to ["*"] to prevent logging failures and false positives on PaaS deployments (like Railway/Fly) that use dynamic internal proxies. Adds documentation clarifying that edge proxies MUST strip inbound X-Forwarded-For headers to prevent spoofing.
…ijack heuristics

Per reviewer feedback, re-introduced TRUSTED_PROXY_IPS as an unset default. When unset, ProxyHeadersMiddleware trusts ["*"] (ensuring PaaS compatibility) and IP-based session hijack logs are downgraded to debug since the IPs are untrusted. When set, strict IP spoofing checks and INFO-level hijack heuristics are enabled. Also added a startup warning when it is unset to clearly communicate the degraded security state.
Per reviewer feedback: 1) Downgraded untrusted IP changes back to INFO but flagged them as 'trusted_source=false' to maintain greppability without triggering incident alerts. 2) Commented out SESSION_COOKIE_SECURE in .env.example so production copy-pastes are secure-by-default.
…ion IP heuristics

Per reviewer feedback: 1) Updated session IP heuristic log to use a structured extra={'trusted_source': False} argument instead of a string tag for better downstream parsing. 2) Added integration tests for the uvicorn startup warning and the IP change fallback logic.
…ertions

Per reviewer feedback: 1) Added a custom ExtraFormatter to the standard library logging config so that `extra={}` fields actually survive in plain text standard output. 2) Added a test explicitly wiping the environment to prove SESSION_COOKIE_SECURE safely defaults to True in code. 3) Added a test asserting that ProxyHeadersMiddleware successfully mitigates IP spoofing from untrusted peers.
Per reviewer feedback: 1) Updated config default test to chdir(tmp_path) to strictly prevent Pydantic from resolving stray .env files. 2) Updated structured logging fields to use the 'app.' prefix (e.g. app.trusted_source) to avoid stdlib logging attribute collisions. 3) Added a test for the 'chained header' attack simulating an edge proxy appending to a forged X-Forwarded-For.
Per reviewer feedback: Added an explicit assertion for the completely bare Settings() class default to crystal-clear pin the code-level default alongside the isolated Pydantic instantiation.
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.

[Backend] Insecure defaults: SESSION_COOKIE_SECURE=False and ProxyHeadersMiddleware trusts all hosts

1 participant