fix[bug]: handle non-JSON responses and query params in REST tools (#3855, #3857)

[rakdutta]

Closes #3855 and #3857

This PR enhances REST tool robustness by improving error handling, query parameter processing, and input validation. It also streamlines the admin UI tool testing workflow.

Backend Improvements

Non-JSON Response Handling (#3855)

• New _handle_json_parse_error() helper function for graceful fallback when JSON parsing fails
• Handles multiple exception types: json.JSONDecodeError, orjson.JSONDecodeError, UnicodeDecodeError, AttributeError
• Supports REST APIs returning HTML error pages, plain text, XML, or responses with encoding issues
• Returns raw text as {"response_text": } instead of crashing

Response Truncation (Security Enhancement)

• New configurable setting: REST_RESPONSE_TEXT_MAX_LENGTH (default: 5000 chars, range: 1000-100000)
• Truncates non-JSON response text to prevent exposing excessive sensitive data in error responses
• Applies to both success and error responses
• Logs warning when truncation occurs with original and truncated lengths

Query Parameter Handling (#3857)

• GET requests: URL query params merged with input arguments (URL params take precedence on conflicts)
• POST/PUT/PATCH/DELETE: Query params preserved in URL to support signed URLs (Azure SAS, AWS presigned URLs, webhook signatures)
• Added conflict warning when URL query params override input arguments for better debugging

jq Filter Validation

• Added validation to detect simple email addresses mistakenly used as jq filters (e.g., user@example.com)
• Uses regex pattern ^[^.\[\]|]+@[^.\[\]|]+.[^.\[\]|]+$ to catch basic email patterns without false positives
• Logs warning and treats invalid filters as empty (returns unfiltered data)

Frontend Enhancements

Admin UI Tool Testing Workflow

• New invokeTool(toolName) function called by "Invoke" buttons in Tools table
• Fetches tool details via API (fetchToolDetails) and populates modal dynamically
• Dynamic form generation from tool input schema (createFormInput, generateToolFormFields)
• Refactored runToolTest() and runToolValidation() to use consistent tools/call method structure:
  {
  method: "tools/call",
  params: { name: toolName, arguments: {...} }
  }
• Enhanced error logging for better debugging

Test Coverage

Comprehensive test suite added:

• TestJqFilterEmailValidation: Email address detection and valid filter preservation
• TestRestToolQueryParamHandling: GET/POST/PUT/PATCH/DELETE param handling, conflict warnings
• TestRestToolNonJsonResponses: HTML, plain text, XML, encoding errors, empty responses
• Truncation tests: large responses, small responses, custom config values

[TS0713]

• Fixes the Admin UI issue where Test/Invoke buttons were not working — the missing invokeTool() and runToolTest() functions are now properly added, and the request format is corrected to tools/call. I verified this flow manually and it’s working end-to-end now.

• Fixes the REST POST query parameter issue — query params are now correctly preserved in the URL for POST/PUT requests. I tested this as well, and signed URL / query-based APIs are working as expected.

• Handles non-JSON responses (HTML, text, etc.) — prevents crashes and makes the system more robust.

• Manually tested on gateway admin UI by adding REST tools - 1 with query params and other returns HTML result as part of final response - Both working as expected

Overall, these are useful fixes across both UI and backend, and everything looks clean and well-tested.

I approve this PR.

[crivetimihai]

Nice work on this one — the core idea of handling non-JSON REST responses and fixing query param handling for signed URLs is solid and well-motivated.

What I changed during rebase

The branch had significant merge conflicts since admin.js was split into ES modules (#3137) after this PR was opened. I rebased onto current main, squashed the 22 commits into one clean commit, and made the following adjustments:

JS changes ported to modular structure

• All JS changes now target mcpgateway/admin_ui/tools.js and admin_ui/admin.js instead of the old monolithic admin.js
• Used fetchWithTimeout (already imported) instead of raw fetch + getCookie for consistency with the existing testTool() function
• Replaced innerHTML assignments with safe DOM methods (removeChild, createElement, textContent) to avoid XSS vectors

Fixed unreachable 4xx/5xx error handling (critical)

The original code placed the non-JSON error response handling after response.raise_for_status(), which throws httpx.HTTPStatusError for 4xx/5xx — making the error branch unreachable for the exact cases #3855 describes.

Fixed by catching HTTPStatusError and handling the error response in the except block:

• Parses body with the same JSON/non-JSON fallback
• Includes HTTP status code in the error message ("HTTP 500: <truncated html>")
• Sets structured_content={"status_code": N} so the retry plugin can distinguish retriable (503) from non-retriable (404) errors — matching the existing pattern at line 5040

The original elif status not in [200, 201, 202, 206] branch is retained for non-standard 2xx codes (203, 205, 207).

Test fix

Updated the HTML error response test to actually raise httpx.HTTPStatusError from raise_for_status() instead of mocking it to no-op — the original test passed but didn't exercise the real production code path.

Verification

• 290/290 tests in test_tool_service.py pass (18 new)
• 323/323 tests in test_tool_service_coverage.py pass
• Linters clean (black, isort, eslint)

[gcgoncalves]

Verified, working fine. 👍