[FIX][API]: store authheadersauth_value as dict to prevent JSON null on persist#3510
Conversation
|
Thanks @shoummu1. Excellent root cause analysis and thorough fix — the type mismatch between |
Signed-off-by: Shoumi <shoumimukherjee@gmail.com>
Signed-off-by: Shoumi <shoumimukherjee@gmail.com>
Signed-off-by: Shoumi <shoumimukherjee@gmail.com>
Signed-off-by: Shoumi <shoumimukherjee@gmail.com>
Fix import ordering in export_service.py (isort), apply black formatting to test assertions, and add two regression tests covering the dict→encode_auth branch in both export gateway paths. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
encode_auth() uses os.urandom(12) for AES-GCM nonce, so each call produces different ciphertext even for identical plaintext. Comparing encoded values in _update_or_create_tools() caused every health-check refresh to detect a false auth change and rewrite all tools. Fix: decode existing tool auth_value and compare against plaintext gateway dict. Only encode on the actual write path when auth content truly changed. Also strengthen the test to assert byte-for-byte identity of existing auth_value (no spurious re-encryption). Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
f4bf2d0 to
7c29197
Compare
Review & ChangesThanks @shoummu1 — excellent root cause analysis and thorough fix. I've rebased onto main, reviewed all code paths, tested end-to-end (curl + Playwright against a live deployment), and added a few commits on top: Commits added
Pre-existing bugs found (not in this PR's scope)Filed two related issues discovered during testing:
Testing performed
|
crivetimihai
left a comment
There was a problem hiding this comment.
LGTM — rebased, fixed the spurious update comparison bug (random nonce), added missing export tests, and verified end-to-end. All 279 tests pass.
The test previously relied on "Invalid tool type" exit without verifying the dict→string encoding actually happened. Add spy on encode_auth to assert it is called with the expected dict during cache-miss hydration. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
7c29197 to
580df8b
Compare
…l on persist (#3510) * store authheaders auth_value as dict instead of encoded string Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * additional fixes for tool Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * encode DbGateway.auth_value dict for DbTool/export/tool-invoke paths Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * fix coverage Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * fix: isort import order and add authheaders dict export tests Fix import ordering in export_service.py (isort), apply black formatting to test assertions, and add two regression tests covering the dict→encode_auth branch in both export gateway paths. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: compare plaintext auth values to avoid spurious tool updates encode_auth() uses os.urandom(12) for AES-GCM nonce, so each call produces different ciphertext even for identical plaintext. Comparing encoded values in _update_or_create_tools() caused every health-check refresh to detect a false auth change and rewrite all tools. Fix: decode existing tool auth_value and compare against plaintext gateway dict. Only encode on the actual write path when auth content truly changed. Also strengthen the test to assert byte-for-byte identity of existing auth_value (no spurious re-encryption). Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test: assert encode_auth is called in hydration path The test previously relied on "Invalid tool type" exit without verifying the dict→string encoding actually happened. Add spy on encode_auth to assert it is called with the expected dict during cache-miss hydration. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Shoumi <shoumimukherjee@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
…l on persist (#3510) * store authheaders auth_value as dict instead of encoded string Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * additional fixes for tool Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * encode DbGateway.auth_value dict for DbTool/export/tool-invoke paths Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * fix coverage Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * fix: isort import order and add authheaders dict export tests Fix import ordering in export_service.py (isort), apply black formatting to test assertions, and add two regression tests covering the dict→encode_auth branch in both export gateway paths. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: compare plaintext auth values to avoid spurious tool updates encode_auth() uses os.urandom(12) for AES-GCM nonce, so each call produces different ciphertext even for identical plaintext. Comparing encoded values in _update_or_create_tools() caused every health-check refresh to detect a false auth change and rewrite all tools. Fix: decode existing tool auth_value and compare against plaintext gateway dict. Only encode on the actual write path when auth content truly changed. Also strengthen the test to assert byte-for-byte identity of existing auth_value (no spurious re-encryption). Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test: assert encode_auth is called in hydration path The test previously relied on "Invalid tool type" exit without verifying the dict→string encoding actually happened. Add spy on encode_auth to assert it is called with the expected dict during cache-miss hydration. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Shoumi <shoumimukherjee@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>
Bug-fix PR
📌 Summary
When registering a gateway with
auth_type=authheaders, the custom HTTP headers were silently discarded after the first write to the database. Any subsequent health check, auto-refresh, or tool-invocation attempt would find no authentication credentials and fail. The fix corrects the root cause and all downstream code paths where the mismatched type would have produced similar silent failures.🔗 Related issue
Closes: #3480
🔁 Reproduction Steps
auth_type: authheadersand one or moreauth_headersentries (e.g.X-Custom-Auth-Header: my-token).GET /gateways/{id};auth_valueisnull.🐞 Root Cause
Primary bug —
register_gateway()creation pathIn
mcpgateway/services/gateway_service.py, after buildingheader_dictfromgateway.auth_headers, the creation path called:encode_auth()returns astr. HoweverDbGateway.auth_valueis declared as:SQLAlchemy serialises a bare Python
strstored in aDict-typed JSON column as JSONnull; the auth headers were lost on everyINSERT. The update path (update_gateway()) already stored the plaindictdirectly — the creation path was inconsistent.Secondary bug — type mismatch propagation
DbTool.auth_valueis the opposite type —Mapped[Optional[str]](Text). After fixingDbGateway, the same plaindictwould have been passed toDbToolunchanged, storing a raw Pythondictin aTextcolumn. Additionally, every downstream code path that readsDbGateway.auth_valueexpecting a string had the same latent mismatch.💡 Fix Description
gateway_service.py—register_gateway()(2 changes)gateway_service.py—_update_or_create_tools()(2 changes)The comparison
existing_tool.auth_value != gateway.auth_valuewas comparing astr(DbTool Text column) against adict(DbGateway JSON column) — alwaysTrue, causing a spurious full-tool overwrite on every health-check refresh cycle. The assignmentexisting_tool.auth_value = gateway.auth_valuewould then store a raw dict into a Text column.tool_service.py(3 changes)gateway_payload["auth_value"]was set directly fromgateway.auth_value(a dict); downstream header-building code expects an encoded string.isinstance(runtime_gateway_auth_value, str)guard silently dropped dict values with no fallback.isinstance(..., str)guard silently dropped dict values.All three are fixed by adding an
elif isinstance(..., dict): gateway_auth_value = encode_auth(...)branch ahead of the existingstrpath.export_service.py(2 changes)Both the batch-query export path (
export_gateways()) and the raw-query export path (_export_selected_gateways()) were writing the rawdictinto the export payload. An import round-trip would fail becausedecode_auth()expects an encoded string.🧪 Tests
tests/unit/mcpgateway/services/test_gateway_service_helpers.py— 2 regression tests added:test_authheaders_auth_value_stored_as_dict(creation path):encode_auth()returnsstr(why storing it in a JSON dict column producesnull).register_gateway()call with two custom auth headers.db.add()is called, before_prepare_gateway_for_read()mutates the object for the API response.DbGateway.auth_valueis a plaindictmatching the original headers.DbTool.auth_valueis an encodedstrthat round-trips viadecode_auth()back to the original headers dict.test_update_or_create_tools_authheaders_encodes_for_dbtool(update/refresh path):auth_value, no spurious update is triggered (comparison correctly resolves to no change).DbTool.auth_valueremains an encoded string after the path runs.🧪 Verification
make testmake lintmake coverageauthheaders, confirmauth_valuepersisted as JSON object in DB, confirm health check sends correct headers📐 MCP Compliance
✅ Checklist
make black isort pre-commit)