[Bugfix] Reject empty keys in HTTP metadata server#2770
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request introduces validation to reject empty or whitespace-only metadata keys with a 400 Bad Request response in http_metadata_server.py, and adds corresponding unit tests in test_http_metadata_server.py. The reviewer identified two issues: first, the metadata key is not stripped for subsequent operations, meaning keys with leading or trailing whitespace are still processed; second, the error response returns plain text but incorrectly specifies content_type='application/json'. A code suggestion was provided to strip the key and update the content type to text/plain.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if not key.strip(): | ||
| return web.Response(text='metadata key is required', status=400, | ||
| content_type='application/json') |
There was a problem hiding this comment.
Issue 1: Key is not stripped for subsequent operations
While not key.strip() correctly rejects empty or whitespace-only keys, the original unstripped key (which may contain leading or trailing whitespaces, e.g., " valid ") is still passed to the subsequent handlers (_handle_get, _handle_put, _handle_delete). It is safer and more robust to strip the key before performing any operations on it.
Issue 2: Invalid JSON Content-Type
The response returns plain text ('metadata key is required') but specifies content_type='application/json'. This is technically incorrect and can cause JSON parsing errors on clients that strictly parse the response body based on the Content-Type header. It should be changed to 'text/plain'.
| if not key.strip(): | |
| return web.Response(text='metadata key is required', status=400, | |
| content_type='application/json') | |
| key = key.strip() | |
| if not key: | |
| return web.Response(text='metadata key is required', status=400, | |
| content_type='text/plain') |
There was a problem hiding this comment.
Thanks for the review. I addressed both points:
keyis now stripped before dispatching to GET/PUT/DELETE handlers, so values like" valid "are stored and looked up as"valid".- The 400 response now uses
content_type='text/plain'to match the plain-text error body.
I also added focused tests covering the text/plain error response and the whitespace-stripping behavior before metadata operations.
Validation run locally:
python -m pytest mooncake-wheel/tests/test_http_metadata_server.py -q
python -m py_compile mooncake-wheel/mooncake/http_metadata_server.py mooncake-wheel/tests/test_http_metadata_server.py
git diff --check|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
--------- Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
Description
What Problem This Solves
The Python HTTP metadata server currently treats a missing
keyquery parameter as an empty string. As a result, requests such asPUT /metadataorPUT /metadata?key=can operate onself.store[""]instead of being rejected.This can create accidental empty-string metadata entries when callers omit the query parameter, and later empty-key requests can then appear valid instead of surfacing the original bad request.
Changes
KVBootstrapServer._handle_metadata()before dispatching GET, PUT, or DELETE handling.Evidence
Possible call chain / impact
External metadata requests enter through
/metadata, thenKVBootstrapServer._handle_metadata()dispatches to_handle_get(),_handle_put(), or_handle_delete().This change affects only the Python HTTP metadata server key handling path: missing, empty, and whitespace-only
keyquery parameters are rejected, while valid non-empty keys are stripped before continuing through the existing handler path.Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Focused unit coverage validates the metadata handler directly:
keyis rejected for GET, PUT, and DELETEkeyis rejectedkeyis rejectedTest commands:
Test results:
Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks passAI Assistance Disclosure
AI assistance was used to identify the empty-key API contract issue, inspect the affected handler path, and draft focused validation coverage. The human submitter is responsible for reviewing and validating the final changes.