Skip to content

fix(ui): replace tojson with single-quoted literals in Fetch Tools onclick#3616

Closed
omorros wants to merge 2 commits intoIBM:mainfrom
omorros:fix/fetch-tools-button-tojson-escaping
Closed

fix(ui): replace tojson with single-quoted literals in Fetch Tools onclick#3616
omorros wants to merge 2 commits intoIBM:mainfrom
omorros:fix/fetch-tools-button-tojson-escaping

Conversation

@omorros
Copy link
Copy Markdown
Contributor

@omorros omorros commented Mar 11, 2026

🔗 Related Issue Closes #3082

Supersedes #3179

📝 Summary

Replaced tojson_attr with single-quoted JS literals in the Fetch Tools button onclick handler in
gateways_partial.html, fixing broken HTML parsing when double-quoted JSON strings appeared inside a double-quoted onclick attribute. This aligns the HTMX partial with the working pattern used elsewhere in the templates.


🏷️ Type of Change

  • Bug fix
  • Feature / Enhancement
  • Documentation - [ ] Refactor
  • Chore (deps, CI, tooling)
  • Other (describe below)

🧪 Verification

Check Command Status
Lint suite make lint
Unit tests make test

✅ Checklist

  • Code formatted (make black isort pre-commit)
  • Tests added/updated for changes - [x] Documentation updated (if applicable)
  • No secrets or credentials committed

📓 Notes (optional)

Single-line change in gateways_partial.html:42. The same button works correctly in admin.html (initial page load)
which already uses single quotes — this fix aligns the HTMX partial to match.

@omorros omorros changed the base branch from fix/fetch-tools-button-tojson-escaping to main March 11, 2026 12:07
@marekdano
Copy link
Copy Markdown
Collaborator

@omorros - thanks for your PR. Can you please solve the DCO check? You need

To avoid having PRs blocked in the future, always include 
Signed-off-by: Author Name <authoremail@example.com> in every commit message. 
You can also do this automatically by using the -s flag (i.e., git commit -s).

…k handler

Signed-off-by: Oriol Morros Vilaseca <OM368@student.aru.ac.uk>
@omorros omorros force-pushed the fix/fetch-tools-button-tojson-escaping branch from 1db790b to 07b0fbd Compare March 12, 2026 11:08
@omorros
Copy link
Copy Markdown
Contributor Author

omorros commented Mar 12, 2026

Sorry about that @marekdano Everything should work now with no issues!

@marekdano marekdano added ui User Interface bug Something isn't working SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release labels Mar 12, 2026
Copy link
Copy Markdown
Collaborator

@marekdano marekdano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@omorros - thanks for the PR!

The PR replaces tojson_attr with single-quoted JS literals, which is the correct fix for the HTML attribute parsing issue.

Root Cause Analysis

The tojson_attr filter outputs JSON with double quotes (e.g., "my-gateway"), which when placed inside a double-quoted onclick attribute creates:
onclick="fetchToolsForGateway("abc123", "my-gateway")"

This breaks HTML parsing because the browser sees the attribute ending at the first inner ", resulting in malformed HTML.

Why the PR Fix is Safe

Gateway names are strictly validated (mcpgateway/common/validators.py):

  • Pattern: ^[a-zA-Z0-9_.- ]+$ (letters, numbers, underscore, dot, hyphen, space only)
  • Explicitly rejects HTML special characters including single quotes (') and double quotes (")
  • Maximum length: 255 characters

The PR's approach:
onclick="fetchToolsForGateway('{{ gateway.id }}', '{{ gateway.name }}')"

This is safe because:

  1. Gateway names cannot contain single quotes (validation prevents it)
  2. Gateway IDs are UUIDs (hex strings, no special characters)
  3. This matches the working pattern already used in admin.html line 5296

LGTM 🚀

@marekdano marekdano added release-fix Critical bugfix required for the release merge-queue Rebased and ready to merge labels Mar 12, 2026
@marekdano marekdano added this to the Release 1.0.0 milestone Mar 12, 2026
@crivetimihai
Copy link
Copy Markdown
Member

Thanks for the contribution @omorros — the original bug report (#3082) was a real issue and your diagnosis of the |tojson double-quote collision was correct.

However, this fix has been superseded by the introduction of the tojson_attr custom Jinja2 filter in #3350 (merged 2026-03-01), which properly addresses the root cause. Specifically:

  • tojson_attr (mcpgateway/main.py:3000) JSON-encodes the value, Unicode-escapes dangerous characters (&, <, >, '), and returns a plain str so Jinja2 autoescape HTML-encodes the remaining " — keeping double-quoted HTML attributes intact.
  • Both gateways_partial.html:42 and admin.html:5297 already use |tojson_attr for the fetchToolsForGateway call on current main.
  • Every other Jinja-templated event handler across all partials consistently uses |tojson_attr.

Switching to raw '{{ gateway.name }}' would weaken defense-in-depth: while input validation currently blocks quotes in gateway names, tojson_attr ensures template-level safety independent of validation rules. Merging this would also introduce an inconsistency against the established pattern.

Closing as superseded by #3350. Thanks again for reporting the issue and putting together the fix — it helped drive the proper solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working merge-queue Rebased and ready to merge release-fix Critical bugfix required for the release SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release ui User Interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][UI]: Fetch Tools button broken due to Jinja tojson quote escaping in gateways_partial.html

3 participants