fix: reduce global client_max_size and add configurable setting#3268
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes aiohttp request body size limits configurable, tightening the global default while allowing a larger limit for API upload endpoints, and adds default config entries plus accompanying tests.
Changes:
- Replace the hard-coded global
client_max_sizewith a config-driven value (client_max_size_mb, default 1MB). - Add a separate API v2 upload request size limit (
api_upload_max_size_mb, default 100MB) viamake_app(..., upload_max_size_mb=...). - Update
conf/default.ymldefaults and add a new test file undertests/security/.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
server.py |
Reads request size limits from config and passes an upload limit into the API v2 subapp. |
app/api/v2/__init__.py |
Applies a per-subapp client_max_size derived from upload_max_size_mb. |
conf/default.yml |
Introduces defaults for the new config knobs (and adds rate-limit keys). |
tests/security/test_client_max_size.py |
Adds tests intended to cover the new defaults/logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_default_global_client_max_size(): | ||
| """Global default is 1MB.""" | ||
| assert (1) * 1024 * 1024 == 1048576 | ||
|
|
||
|
|
||
| def test_none_fallback_global(): | ||
| """When config returns None, global defaults to 1MB.""" | ||
| config_val = None | ||
| assert (config_val or 1) * 1024 * 1024 == 1048576 | ||
|
|
||
|
|
||
| def test_default_upload_max_size(): | ||
| """API upload default is 100MB to accommodate payloads and exfil files.""" | ||
| assert (100) * 1024 * 1024 == 104857600 | ||
|
|
||
|
|
||
| def test_none_fallback_upload(): | ||
| """When config returns None, upload defaults to 100MB.""" | ||
| config_val = None | ||
| assert (config_val or 100) * 1024 * 1024 == 104857600 | ||
|
|
||
|
|
||
| def test_upload_limit_exceeds_global_limit(): | ||
| """API upload limit must be larger than the global limit.""" | ||
| global_limit = 1 * 1024 * 1024 | ||
| upload_limit = 100 * 1024 * 1024 | ||
| assert upload_limit > global_limit | ||
|
|
||
|
|
||
| def test_old_value_was_larger_than_new_global(): | ||
| """Confirm old hardcoded value (5120**2 ~26MB) is replaced by a tighter global default.""" | ||
| old_value = 5120 ** 2 | ||
| new_global = 1 * 1024 * 1024 |
| def test_none_fallback_global(): | ||
| """When config returns None, global defaults to 1MB.""" | ||
| config_val = None | ||
| assert (config_val or 1) * 1024 * 1024 == 1048576 | ||
|
|
| application=web.Application( | ||
| client_max_size=5120**2, middlewares=[pass_option_middleware] | ||
| client_max_size=(BaseWorld.get_config('client_max_size_mb') or 1) * 1024 * 1024, | ||
| middlewares=[pass_option_middleware] |
| app_svc.register_subapp("/api/v2", app.api.v2.make_app( | ||
| app_svc.get_services(), | ||
| upload_max_size_mb=BaseWorld.get_config('api_upload_max_size_mb') or 100 | ||
| )) |
| app = web.Application( | ||
| client_max_size=upload_max_size_mb * 1024 * 1024, |
| rate_limit_requests: 360 | ||
| rate_limit_window: 60 |
|
dabb12b to
b939b34
Compare
There was a problem hiding this comment.
Pull request overview
Adds configurable request body size limits for the aiohttp root app vs. the /api/v2 sub-application, tightening the global default while keeping a larger API upload allowance.
Changes:
- Introduces
client_max_size_mb(global) andapi_upload_max_size_mb(API v2) configuration keys and wires them into server startup. - Extends
app.api.v2.make_appto accept anupload_max_size_mbparameter and applies it as the v2 app’sclient_max_size. - Adds tests intended to validate the new size-limit behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
server.py |
Reads config to set root client_max_size and passes API upload max size into the v2 subapp factory. |
app/api/v2/__init__.py |
Adds upload_max_size_mb parameter and sets client_max_size on the v2 aiohttp application. |
conf/default.yml |
Defines defaults for client_max_size_mb and api_upload_max_size_mb. |
tests/security/test_client_max_size.py |
Adds tests targeting size limit defaults/coercion behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Helper: return the client_max_size configured on the v2 sub-app.""" | ||
| services = {'auth_svc': None} | ||
| # make_app raises AttributeError when attaching routes without real services, | ||
| # so we monkeypatch the handler classes to do nothing. |
| def _make_app_max_size(upload_max_size_mb): | ||
| """Helper: return the client_max_size configured on the v2 sub-app.""" | ||
| services = {'auth_svc': None} | ||
| # make_app raises AttributeError when attaching routes without real services, | ||
| # so we monkeypatch the handler classes to do nothing. | ||
| import app.api.v2 as v2_module | ||
| original = v2_module.make_app | ||
|
|
||
| created_app = [None] | ||
|
|
||
| def patched(services, upload_max_size_mb=100): | ||
| from aiohttp import web | ||
| try: | ||
| max_size = int(upload_max_size_mb) | ||
| max_size = max_size if max_size > 0 else 100 | ||
| except (TypeError, ValueError): | ||
| max_size = 100 | ||
| created_app[0] = web.Application(client_max_size=max_size * 1024 * 1024) | ||
| return created_app[0] | ||
|
|
||
| v2_module.make_app = patched | ||
| try: | ||
| result = v2_module.make_app(services, upload_max_size_mb=upload_max_size_mb) | ||
| return result._client_max_size | ||
| finally: | ||
| v2_module.make_app = original | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('mb,expected_bytes', [ | ||
| (1, 1 * 1024 * 1024), | ||
| (100, 100 * 1024 * 1024), | ||
| (50, 50 * 1024 * 1024), | ||
| ]) | ||
| def test_valid_integer_config(mb, expected_bytes): | ||
| assert _make_app_max_size(mb) == expected_bytes | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('bad_val', [None, '', 'abc', '1MB', -1, 0]) | ||
| def test_invalid_config_falls_back_to_default(bad_val): | ||
| """None, strings, and non-positive values must fall back to 100MB default.""" | ||
| result = _make_app_max_size(bad_val) | ||
| assert result == 100 * 1024 * 1024 | ||
|
|
||
|
|
||
| def test_string_integer_coerces_correctly(): | ||
| """Config may return strings; '50' must coerce to 50MB, not repeat a string.""" | ||
| result = _make_app_max_size('50') |
| v2_module.make_app = patched | ||
| try: | ||
| result = v2_module.make_app(services, upload_max_size_mb=upload_max_size_mb) | ||
| return result._client_max_size |
|
|
||
| def test_upload_limit_exceeds_global_default(): | ||
| """API upload limit (100MB) must exceed the global client limit (1MB).""" | ||
| assert 100 * 1024 * 1024 > 1 * 1024 * 1024 |
| def test_new_global_tighter_than_old_hardcoded(): | ||
| """New 1MB global default is tighter than the old hardcoded 5120**2 (~26MB).""" | ||
| assert 1 * 1024 * 1024 < 5120 ** 2 |
0550d69 to
5cf97a2
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes request body size limits configurable, tightening the global aiohttp client_max_size default while allowing a larger limit specifically for /api/v2 uploads.
Changes:
- Add
client_max_size_mb(default 1MB) andapi_upload_max_size_mb(default 100MB) to configuration, and parse/validate them inserver.py. - Extend
app.api.v2.make_appto accept an upload size (MB) and apply it to the v2 app’sclient_max_size. - Add tests validating v2
client_max_sizecoercion/fallback behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
server.py |
Reads new config keys and sets root app client_max_size; passes v2 upload size into the v2 subapp. |
app/api/v2/__init__.py |
Adds upload_max_size_mb parameter, validates/coerces it, and sets v2 app client_max_size. |
conf/default.yml |
Introduces defaults for global and v2 upload size limits. |
tests/security/test_client_max_size.py |
Adds unit tests around v2 client_max_size value conversion and defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| application=web.Application( | ||
| client_max_size=5120**2, middlewares=[pass_option_middleware] | ||
| client_max_size=_get_size_mb('client_max_size_mb', 1) * 1024 * 1024, | ||
| middlewares=[pass_option_middleware] | ||
| ) |
| def test_api_upload_limit_larger_than_global_default(): | ||
| """API v2 upload limit (100MB default) must be larger than the global 1MB default.""" | ||
| api_app = _make_app(100) | ||
| assert api_app._client_max_size == 100 * 1024 * 1024 | ||
| assert api_app._client_max_size > 1 * 1024 * 1024 |
| new_default_mb = 1 * 1024 * 1024 | ||
| assert new_default_mb < old_hardcoded | ||
| # Confirm the app created with 1MB actually applies that limit | ||
| assert _make_app(1)._client_max_size == new_default_mb |
Reduce default from ~26MB to 1MB with configurable client_max_size_mb key.
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.
Restores atomic, compass, fieldmanual, and response which were accidentally dropped. Removes automation and mcp which should not be in the default plugin set.
…onfig; test actual runtime behavior
…ervices 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.
5cf97a2 to
5a938ea
Compare
There was a problem hiding this comment.
Pull request overview
This PR tightens and parameterizes aiohttp request body size limits by introducing configurable client_max_size_mb (root app) and api_upload_max_size_mb (API v2 subapp), with updated defaults and accompanying tests.
Changes:
- Replace the previous hardcoded root
client_max_sizewith a config-driven MB value (defaulting to 1MB). - Add an
upload_max_size_mbparameter toapp.api.v2.make_appto set API v2’sclient_max_size(defaulting to 100MB) with basic coercion/validation. - Add tests to validate byte conversion and fallback behavior for valid/invalid config inputs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
server.py |
Reads new config keys and applies per-app client_max_size limits (root vs v2 subapp). |
app/api/v2/__init__.py |
Extends make_app to accept/upload size (MB) and applies it to aiohttp client_max_size. |
conf/default.yml |
Introduces default values for the new size-limit config keys. |
tests/security/test_client_max_size.py |
Adds coverage around size coercion and defaults for the new behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,64 @@ | |||
| import pytest | |||
| from unittest.mock import MagicMock, patch | |||
|
|
||
| def test_root_and_subapp_limits(): | ||
| """Root app has 1MB limit; v2 subapp has 100MB limit; subapp limit exceeds root.""" | ||
| BaseWorld.apply_config('main', {'client_max_size_mb': 1, 'api_upload_max_size_mb': 100}) |
| def _get_size_mb(config_key, default): | ||
| try: | ||
| val = int(BaseWorld.get_config(config_key)) | ||
| return val if val > 0 else default | ||
| except (TypeError, ValueError): | ||
| return default |
| try: | ||
| max_size = int(upload_max_size_mb) | ||
| max_size = max_size if max_size > 0 else 100 | ||
| except (TypeError, ValueError): | ||
| max_size = 100 | ||
|
|
||
| app = web.Application( | ||
| client_max_size=max_size * 1024 * 1024, | ||
| middlewares=[ |
There was a problem hiding this comment.
Pull request overview
Introduces configurable request body size limits for the aiohttp root app and the /api/v2 subapp, tightening the global default while allowing larger API uploads.
Changes:
- Add
client_max_size_mb(default 1MB) andapi_upload_max_size_mb(default 100MB) toconf/default.yml. - Update
server.pyto read these config values and apply them to the root app and v2 subapp. - Update
app/api/v2.make_appto accept an upload size (MB) and setclient_max_sizeaccordingly; add tests validating coercion/defaulting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/security/test_client_max_size.py | Adds unit tests for upload max-size coercion and defaults |
| server.py | Applies configurable client_max_size to root app and passes upload limit to v2 subapp |
| conf/default.yml | Adds defaults for new max-size configuration keys |
| app/api/v2/init.py | Extends make_app to set client_max_size based on upload limit |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BaseWorld.apply_config('main', {'client_max_size_mb': 1, 'api_upload_max_size_mb': 100}) | ||
| root_app = web.Application( | ||
| client_max_size=1 * 1024 * 1024, | ||
| middlewares=[pass_option_middleware] | ||
| ) | ||
| v2_app = v2_module.make_app(MagicMock(), upload_max_size_mb=100) | ||
| assert root_app._client_max_size == 1 * 1024 * 1024 | ||
| assert v2_app._client_max_size == 100 * 1024 * 1024 | ||
| assert v2_app._client_max_size > root_app._client_max_size |
| client_max_size=1 * 1024 * 1024, | ||
| middlewares=[pass_option_middleware] | ||
| ) | ||
| v2_app = v2_module.make_app(MagicMock(), upload_max_size_mb=100) |
There was a problem hiding this comment.
Pull request overview
This PR reduces the default global client_max_size and makes both the root app and API v2 upload limits configurable via default.yml.
Changes:
- Add config keys for root (
client_max_size_mb) and API v2 (api_upload_max_size_mb) size limits with safer defaults. - Wire the new config into
server.pyand extend API v2make_app()to accept a configurable upload limit. - Add pytest coverage for size parsing/defaulting and for root vs subapp limit behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/security/test_client_max_size.py | Adds tests for upload size defaulting/coercion and root vs subapp limit behavior. |
| server.py | Introduces config-backed size limits and applies them to root app and API v2 subapp. |
| conf/default.yml | Adds the new size-limit configuration keys and reorders some existing keys. |
| app/api/v2/init.py | Makes API v2 app client_max_size configurable and validates the configured value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _get_size_mb(config_key, default): | ||
| try: | ||
| val = int(BaseWorld.get_config(config_key)) | ||
| return val if val > 0 else default | ||
| except (TypeError, ValueError): | ||
| return default |
| app_svc = AppService( | ||
| application=web.Application( | ||
| client_max_size=5120**2, middlewares=[pass_option_middleware] | ||
| client_max_size=_get_size_mb('client_max_size_mb', 1) * 1024 * 1024, |
| def _get_size_mb(config_key, default): | ||
| try: | ||
| val = int(BaseWorld.get_config(config_key)) | ||
| return val if val > 0 else default | ||
| except (TypeError, ValueError): | ||
| return default | ||
|
|
||
| app_svc = AppService( | ||
| application=web.Application( | ||
| client_max_size=5120**2, middlewares=[pass_option_middleware] | ||
| client_max_size=_get_size_mb('client_max_size_mb', 1) * 1024 * 1024, | ||
| middlewares=[pass_option_middleware] | ||
| ) | ||
| ) | ||
| app_svc.register_subapp("/api/v2", app.api.v2.make_app(app_svc.get_services())) | ||
| app_svc.register_subapp("/api/v2", app.api.v2.make_app( | ||
| app_svc.get_services(), | ||
| upload_max_size_mb=_get_size_mb('api_upload_max_size_mb', 100) | ||
| )) |
| BaseWorld.apply_config('main', {'client_max_size_mb': 1, 'api_upload_max_size_mb': 100}) | ||
| root_app = web.Application( | ||
| client_max_size=1 * 1024 * 1024, | ||
| middlewares=[pass_option_middleware] | ||
| ) | ||
| v2_app = v2_module.make_app(MagicMock(), upload_max_size_mb=100) | ||
| root_app.add_subapp('/api/v2', v2_app) |
|
* 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



Description
(insert summary)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: