feat: add CSRF protection middleware by deacon-mp · Pull Request #3332 · mitre/caldera

deacon-mp · 2026-03-16T03:56:33Z

Summary

Adds csrf_protect_middleware_factory to app/api/v2/security.py that protects all state-modifying HTTP methods (POST/PUT/PATCH/DELETE) against CSRF attacks for session-authenticated users
Safe methods (GET/HEAD/OPTIONS) and API-key-authenticated requests are exempt from CSRF checks
On successful login, a per-session csrf_token is generated and stored in the server-side session; the token is also exposed as a readable XSRF-TOKEN cookie so client-side JavaScript can read it for double-submit validation
Cookie security hardened in auth_svc.py: EncryptedCookieStorage now sets secure=True, httponly=True, max_age=86400, and samesite='Strict'
Middleware wired into app/api/v2/__init__.py after authentication middleware

Notes

The base_world fixture in tests/api/v2/test_security.py does not yet include the apply_hash=True parameter added to master in hash passwords and API keys in main config #3257 — a minor rebase/fixup will be needed before this merges
Test file includes a TODO comment acknowledging that the API key skip check should use a valid API key (not just any KEY header) — tracked for follow-up

Test plan

Review CSRF middleware logic in app/api/v2/security.py
Run pytest tests/api/v2/test_security.py after rebasing onto current master (to pick up apply_hash=True fixture change)
Verify that POST requests without X-CSRF-Token header return 403 for session-authenticated users
Verify that POST requests with a valid X-CSRF-Token header return 200
Verify that API-key-authenticated requests bypass CSRF checks
Confirm cookie attributes (secure, httponly, samesite=Strict) are set correctly

Copilot

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

gitguardian · 2026-03-16T17:34:21Z

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
22305803	Triggered	Generic Password	`83ffce0`	tests/api/v2/test_security.py	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

fionamccrae · 2026-03-16T19:31:21Z

Want to remove plugins and conf from this PR - not relevant to changes

- Bump cryptography 44.0.1 → 46.0.5 (CVE-2026-26007) - Bump Markdown 3.4.4 → 3.8.1 (CVE-2025-69534) - Add Python 3.13 to quality and security CI matrices - Add bandit static analysis to security workflow and tox - Run security checks on pull_request events (not just push) - Fix SonarQube condition: only run on push or non-fork pull_request - Remove untrusted fork code execution from sonar_fork_pr job - Prevent duplicate CI runs via pull_request_target - Fix stale bot messages and align bandit args with pre-commit

Bumps [minimatch](https://github.com/isaacs/minimatch) from 3.1.2 to 3.1.5. - [Changelog](https://github.com/isaacs/minimatch/blob/main/changelog.md) - [Commits](isaacs/minimatch@v3.1.2...v3.1.5) --- updated-dependencies: - dependency-name: minimatch dependency-version: 3.1.5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add architecture field to agent deployment cmds * fix: add architecture field to AgentConfigUpdateSchema; add happy-path test - architecture field was missing from AgentConfigUpdateSchema, causing API requests with architecture to fail marshmallow validation - adds test asserting architecture value is stored in agents config --------- Co-authored-by: Chris Lenk <402940+clenk@users.noreply.github.com> Co-authored-by: deacon <marksoccerman1@aol.com>

* hash passwords and API keys in main config * style fixes * remove superfluous line * move hash checks to utility function * simplify code * add unit tests for config util * fix: guard _is_hashed against non-string config values * test: verify make_secure_config logs plaintext once then returns hashes * style: remove unused logging import from test_config_util.py (F401) * fix: guard verify_hash against None inputs; use yaml.safe_dump for config overwrite - verify_hash() now returns False for non-string inputs instead of raising TypeError (prevents 500 errors when API key header is absent and None is passed to verify) - base_world.py overwrite now uses yaml.safe_dump for safe, consistent output - test: rename 'hash' variable to 'hash_val' to avoid shadowing built-in - test: add None-input assertions to test_verify_hash - test: use side_effect=deepcopy to prevent SENSITIVE_CONF module-level mutation --------- Co-authored-by: deacon <marksoccerman1@aol.com>

* fix: replace create_subprocess_shell with safe exec in start_vue_dev_server Avoid shell injection risk by using create_subprocess_exec instead. * fix: address Copilot review feedback on subprocess PR - Capture proc from create_subprocess_exec, log PID, schedule proc.wait() to avoid zombie processes on Vue dev server exit - Rewrite test to use pytest style, ast-based function extraction, and Path-relative server.py resolution Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: guard against None return from ast.get_source_segment ast.get_source_segment() returns None when source offsets are unavailable; assert it is not None before using it in string checks. * fix: remove untracked create_task in start_vue_dev_server to avoid zombie subprocess * test: accept FunctionDef and AsyncFunctionDef in start_vue_dev_server check * test: use explicit utf-8 encoding in read_text() --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

#3266) * fix: replace deprecated asyncio.get_event_loop() with new_event_loop() Use asyncio.new_event_loop() in run_tasks() and --fresh block. * fix: close event loop in finally block; use try/finally in fresh block; fix tests to use ast+pytest * fix: remove unused variable in asyncio test (flake8 F841) * test: rewrite asyncio event loop tests to use pure AST inspection - Replace brittle substring matching with AST Call node inspection via a shared _is_asyncio_call() helper - Remove incorrect startswith('#') guard (AST never includes comments) - Eliminate ast.get_source_segment() to avoid potential None return * fix: ensure event loop is always closed and cleared on all exit paths * fix: cancel pending tasks before closing event loop in run_tasks * test: relax event loop assertion to allow non-new_event_loop refactors * test: use explicit utf-8 encoding in read_text()

* fix: reduce global client_max_size and add configurable setting Reduce default from ~26MB to 1MB with configurable client_max_size_mb key. * fix: add separate upload size limit for authenticated API routes Global client_max_size stays at 1MB for unauthenticated surfaces. Introduces api_upload_max_size_mb (default 100MB) applied to the /api/v2 sub-app, which is entirely behind authentication, allowing large payload uploads and exfil files without exposing the DoS vector to unauthenticated routes. * fix: restore default plugins list and remove mcp Restores atomic, compass, fieldmanual, and response which were accidentally dropped. Removes automation and mcp which should not be in the default plugin set. * fix: coerce client_max_size config to int; remove unused rate_limit config; test actual runtime behavior * fix: flake8 style fixes * test: rewrite client_max_size tests to call real make_app with mock services Replace the patched duplicate of make_app with calls to the real function using MagicMock services. The last two constant-comparison tests now also assert against the actual configured app value. * test: fix misleading variable name and add root/subapp limit integration test * style: remove unused patch import in test_client_max_size.py * test: actually mount v2 as subapp in root_app to validate real runtime behavior

* fix: degrade gracefully when plugins/magma/dist is absent (#3227) Previously, AppService.load_plugins() unconditionally appended 'plugins/magma/dist' to the jinja2 template search path. When the Magma plugin's built assets are absent (e.g. cloned without --recursive, or --build not yet run), any request reaching RestApi.landing() or handle_catch() would raise a TemplateNotFound exception instead of starting cleanly. - Guard the 'plugins/magma/dist' template-path append behind an os.path.exists() check; emit a WARNING log when the path is missing so the operator knows the web UI will be unavailable. - Apply the same guard in tests/conftest.py so the test suite can run without a built Magma dist. - Add tests/services/test_magma_graceful_degradation.py with four tests that verify: no crash on load_plugins, no /assets static route registered, dist excluded from templates when absent, and included when present. * style: remove unused pytest import in test_magma_graceful_degradation.py * test: create event loop before RestApi.__init__ to avoid RuntimeError on Python 3.11+

…3276) * fix: guard against missing/None agent in operations summary endpoint (#3181) `get_agents()` and `get_hosts()` in OperationApiManager would raise KeyError when a link had a falsy paw, and AttributeError when `find_object()` returned None for an agent that no longer exists in RAM (e.g. after deletion). Both conditions caused the /operations/summary endpoint to return HTTP 500. Fix: skip links with no paw; guard `find_object()` return value with an explicit None check before calling `.display`. Adds regression tests for the summary endpoint. * style: fix E127 continuation line indentation in test_operations_api.py * test: inject null-paw link into operation to exercise issue #3181 fix

…peration init (#3278) * fix: resolve trait-only relationship facts from source fact list on operation init When a fact source defines relationships where the source/target facts reference only a trait (no value) -- as happens when relationships are created via the Caldera UI -- _init_source() was seeding those relationships into the knowledge service with null fact values. This made the relationships functionally useless because they could never match any real seeded facts during planning. The fix introduces Operation._resolve_fact(), which replaces a trait-only fact stub with the first matching fact (by trait) found in the source's fact list before the relationship is added to the knowledge service. If the fact already carries a value, or no match exists, the original fact is returned unchanged. Fixes #2988. * fix: remove always-truthy if r.target guard; Relationship.target is never None

#3048) (#3279) * fix: prevent operation report from returning null when a link paw is absent (#3048) Three related KeyErrors in c_operation.py could cause Operation.report() to silently return None, which the API then serialised as JSON null and the UI rendered as "Null": 1. `agents_steps[step.paw]` in report() raised KeyError when a link's paw was not in the set of operation agents built at call time (e.g. the agent was removed between operation run and report download). Fixed with `agents_steps.setdefault(step.paw, {'steps': []})`. 2. `abilities_by_agent[link.paw]` in _get_all_possible_abilities_by_agent() had the same pattern — orphan paw not guarded. Fixed with an explicit membership check before the extend. 3. The `except Exception` block in report() logged the error but fell off the end of the function, returning None implicitly. The caller then returned None to web.json_response(), producing the "Null" download. Fixed by re-raising so the framework returns a proper 500 with an error body instead of a silent null payload. Adds a regression test that constructs an operation with a chain link whose paw is not present in operation.agents and asserts report() returns a non-None dict that includes the orphan paw's steps. * style: fix E303 too many blank lines in test_operation.py * refactor: simplify double dict lookup using abilities_by_agent.get()

…3280) * fix: correct exfil operation filter and patch path traversal bypass in download_exfil - _get_operation_exfil_folders now returns paw-only keys matching the directory naming convention used at exfil upload time - download_exfil path containment check appends os.sep to prevent startswith bypass via sibling directories (e.g. /tmp/caldera2/) Fixes #3155 * style: fix E306 blank line + remove unused imports in test_rest_svc.py * test: exercise production download_exfil_file to catch regressions in is_in_exfil_dir

* fix: validate upload filename character set in file_svc Reject filenames with path traversal, null bytes, or special characters. * fix: validate field.filename before os.path.split() to prevent traversal bypass; precompile regex; convert tests to pytest * fix: flake8 style fixes * fix: reject '.' as a filename and add test coverage for dot-only names A single '.' passes the safe-character regex but is not a valid upload filename. Add an explicit check and a parametrized test case. * fix: consume rejected multipart part before continue to prevent reader stall * fix: return 400 Bad Request on invalid upload filename instead of silently skipping * fix: re-raise HTTPException so HTTPBadRequest propagates; add HTTP-level test for invalid filename

… shutdown (#3018) (#3277) * fix: register SIGTERM handler in run_tasks() to ensure teardown on service shutdown When Caldera runs as a systemd service (or backgrounded via & / nohup), shutdown sends SIGTERM rather than SIGINT/KeyboardInterrupt. Without a handler, the existing 'except KeyboardInterrupt' teardown block is never reached, so operations and other in-memory state are not saved to disk (issue #3018). Register a SIGTERM handler at the start of run_tasks() that converts the signal into KeyboardInterrupt, reusing the established teardown path without duplicating logic. Tests added to verify structure (AST) and runtime behaviour. * style: remove unused imports in test_server_sigterm.py * fix: wrap full startup in try/except so teardown runs on SIGTERM during startup; add AST test

…icated_endpoint_accepts_session_cookie to match updated auth_svc code where EncryptedCookieStorage is configured with secure=True

…n will fail (opposite of previous commit fix)

…y against timing attacks, CSRF token operates as required, skips depending on whether authentication is required, and a test of a cross-site operation attempt

…ssion is authentication exempt. This may need more security refinement

…ates to master branch

Fiona McCrae added 2 commits March 2, 2026 16:27

initial commit with CSRF mitigations

cc101f8

added some tests for CSRF middleware

2851b30

deacon-mp requested a review from Copilot March 16, 2026 03:56

Copilot AI reviewed Mar 16, 2026

View reviewed changes

Copilot started reviewing on behalf of deacon-mp March 16, 2026 04:25 View session

Fiona McCrae added 2 commits March 16, 2026 10:47

added additional CSRF tests

f1d4330

separated regular functioning security tests from CSRF tests

74b2efe

deacon-mp and others added 21 commits March 16, 2026 15:49

fix: reuse existing fact source on operation close (#3261)

f1dbc0a

added modification to test_authentication_required_middleware_authent…

1c5d5ba

…icated_endpoint_accepts_session_cookie to match updated auth_svc code where EncryptedCookieStorage is configured with secure=True

added test to show that without cookie, authenticated endpoint sessio…

83ffce0

…n will fail (opposite of previous commit fix)

CSRF operation tests are passing, including tests to verify resilienc…

21d065c

…y against timing attacks, CSRF token operates as required, skips depending on whether authentication is required, and a test of a cross-site operation attempt

security.py change allowing CSRF protections to be bypassed if the se…

cf6c1a2

…ssion is authentication exempt. This may need more security refinement

CSRF operation creation test now passes - small change reflecting upd…

5951697

…ates to master branch

linting reformatting

86528b5

fionamccrae force-pushed the VIRTS-1953 branch from 78731ec to 86528b5 Compare March 16, 2026 19:52

Merge branch 'master' into VIRTS-1953

73e7850

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add CSRF protection middleware#3332

feat: add CSRF protection middleware#3332
deacon-mp wants to merge 26 commits intomasterfrom
VIRTS-1953

deacon-mp commented Mar 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

gitguardian bot commented Mar 16, 2026 •

edited

Loading

Uh oh!

fionamccrae commented Mar 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

deacon-mp commented Mar 16, 2026

Summary

Notes

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

gitguardian bot commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Uh oh!

fionamccrae commented Mar 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gitguardian bot commented Mar 16, 2026 •

edited

Loading