Skip to content

[FEATURE][UI]: Add gateway credential reveal endpoint with admin UI support#3504

Draft
gandhipratik203 wants to merge 7 commits intomainfrom
feat/gateway-credential-reveal-endpoint
Draft

[FEATURE][UI]: Add gateway credential reveal endpoint with admin UI support#3504
gandhipratik203 wants to merge 7 commits intomainfrom
feat/gateway-credential-reveal-endpoint

Conversation

@gandhipratik203
Copy link
Copy Markdown
Collaborator

@gandhipratik203 gandhipratik203 commented Mar 5, 2026


🔗 Related Issue

Closes #3346, closes #2968 — see also #3201 for an alternative approach


📝 Summary

  • Adds GatewayCredentialRevealResponse schema to mcpgateway/schemas.py
  • Adds get_gateway_with_credentials() service method to gateway_service.py — fetches and decrypts gateway
    credentials without calling .masked()
  • Adds POST /admin/gateways/{gateway_id}/reveal endpoint to admin.py, gated by gateways.read permission
    with mandatory audit logging on every call
  • Wires up the Show button in the Edit Gateway modal (admin.js) to transparently call the reveal endpoint
    on first click, then toggle show/hide on subsequent clicks
  • Adds 5 unit tests covering all acceptance criteria

Changes are purely additive — masked(), get_gateway(), and the existing GET endpoint are untouched.

Functionality PR #3201 This PR
Reveal gateway credentials via API
Audit log on every credential reveal
Dedicated reveal endpoint

🏷️ 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 ✅ 13,776 passed
Coverage ≥ 80% make coverage

🔬 Steps to Test

Prerequisites

Test 1 — UI flow (end-to-end)

  1. Navigate to MCP Servers → scroll to Add New MCP Server or Gateway
  2. Fill in: Name = test-reveal, URL = any reachable SSE URL, Authentication Type = Bearer Token, Token =
    supersecrettoken123
  3. Click Add Gateway
  4. Find test-reveal in the list → click Edit
  5. In the Token field, click Show
  6. Expected: button briefly shows Loading…, then reveals supersecrettoken123
  7. Click Hide → token is masked again
  8. Click Show again → token revealed instantly (no network call needed, cached in data-real-value)

Optional for reviewers:
Test 2 — API (Swagger UI)

  1. Open http://localhost:8000/docs
  2. Authorize with a Bearer JWT token
  3. Call POST /admin/gateways/{gateway_id}/reveal with the ID of test-reveal
  4. Expected: 200 response with authToken:

Test 3 — Unauthenticated request

  1. Call POST /admin/gateways/{gateway_id}/reveal without Authorization header
  2. Expected: 401 Unauthorized

Test 4 — Non-existent gateway

  1. Call POST /admin/gateways/does-not-exist/reveal
  2. Expected: 404 Not Found

Test 5 — Audit trail

  1. After calling the reveal endpoint, check audit trail via GET /admin/audit-trail
  2. Expected: entry with action: READ, resource_type: gateway, resource_id: <gateway_id>

📸 Observed Output

Edit Gateway modal — before clicking Show
Edit Gateway modal before clicking Show
Edit Gateway modal — after clicking Show
Edit Gateway modal after clicking Show

✅ Checklist

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

📓 Notes

This implementation uses a dedicated POST endpoint rather than modifying the existing GET endpoint (as
proposed in #3201). Benefits: explicit user intent, mandatory audit logging on every reveal, no risk of
accidentally exposing credentials in GET responses or logs.

@gandhipratik203 gandhipratik203 changed the title feat: add POST /admin/gateways/{id}/reveal endpoint for credential re… feat: add gateway credential reveal endpoint with admin UI support Mar 5, 2026
@gandhipratik203 gandhipratik203 marked this pull request as ready for review March 5, 2026 23:13
Comment thread mcpgateway/admin.py Outdated
Comment thread mcpgateway/static/admin.js Outdated
@crivetimihai crivetimihai changed the title feat: add gateway credential reveal endpoint with admin UI support [FEATURE][UI]: Add gateway credential reveal endpoint with admin UI support Mar 7, 2026
@crivetimihai crivetimihai added enhancement New feature or request ui User Interface security Improves security SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release labels Mar 7, 2026
@crivetimihai crivetimihai added this to the Release 1.0.0-RC2 milestone Mar 7, 2026
@crivetimihai
Copy link
Copy Markdown
Member

Thanks @gandhipratik203. Well-designed approach — the dedicated POST endpoint with mandatory audit logging is the right pattern for credential reveal. Good comparison with #3201. A few items: (1) CI checks are failing — please investigate, (2) this will conflict with #3508 which removes the same Show buttons — we'll coordinate merge order, (3) the allow_admin_bypass=False on @require_permission('gateways.read') is a good security choice for credential access.

@crivetimihai crivetimihai self-assigned this Mar 7, 2026
@gandhipratik203 gandhipratik203 requested a review from jonpspri March 8, 2026 22:59
@gandhipratik203 gandhipratik203 force-pushed the feat/gateway-credential-reveal-endpoint branch from 5be3b38 to d6036e9 Compare March 9, 2026 11:06
@gandhipratik203 gandhipratik203 added release-fix Critical bugfix required for the release bug Something isn't working and removed release-fix Critical bugfix required for the release labels Mar 10, 2026
@gandhipratik203 gandhipratik203 force-pushed the feat/gateway-credential-reveal-endpoint branch from d6036e9 to 2b924ca Compare March 10, 2026 08:50
@gandhipratik203 gandhipratik203 marked this pull request as draft March 10, 2026 09:58
@gandhipratik203 gandhipratik203 marked this pull request as ready for review March 10, 2026 11:26
@gandhipratik203
Copy link
Copy Markdown
Collaborator Author

gandhipratik203 commented Mar 10, 2026

Thanks @gandhipratik203. Well-designed approach — the dedicated POST endpoint with mandatory audit logging is the right pattern for credential reveal. Good comparison with #3201. A few items: (1) CI checks are failing — please investigate, (2) this will conflict with #3508 which removes the same Show buttons — we'll coordinate merge order, (3) the allow_admin_bypass=False on @require_permission('gateways.read') is a good security choice for credential access.

Thanks for the review @crivetimihai

(1) CI is now passing.

(2) Since #3508 has merged, I've rebased on main and re-added the Show/Hide buttons to the Edit Gateway form only.
These are now wired to the working toggleInputMask() function.

I've intentionally kept the scope limited to the Edit form, since revealing stored credentials is the primary use case.
If Show/Hide support is also needed on the Add form, I can track that as a separate issue.

(3) Glad the allow_admin_bypass = False choice makes sense.
Credential access felt like it warranted an explicit permission check regardless of admin status.

@gandhipratik203 gandhipratik203 force-pushed the feat/gateway-credential-reveal-endpoint branch from 2827dd7 to b4df80e Compare March 10, 2026 15:16
Comment thread mcpgateway/static/admin.js
…veal

Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
…dential reveal tests

Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
…ct resolution

Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
…base

PR #3508 removed non-functional Show/Hide toggle buttons from admin.html.
After rebasing on main, re-add the buttons to the Edit Gateway form only,
now wired to the working async toggleInputMask() that calls the
POST /admin/gateways/{id}/reveal-credentials endpoint.

Also fix button state reset in editGateway() so reopening the Edit dialog
always shows the correct initial "Show" label with token masked.

Add form intentionally excluded — no stored secret exists at creation time.

Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Add inline comments explaining that data-real-value acts as a per-session
cache (backend called at most once per reveal) and that button.disabled
prevents duplicate in-flight requests on rapid clicks.

Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
@gandhipratik203 gandhipratik203 force-pushed the feat/gateway-credential-reveal-endpoint branch from 87ce09d to 0b83ae5 Compare March 12, 2026 22:07
…ict in get_gateway_with_credentials

_prepare_gateway_for_read was removed in #3570. Inline the equivalent
dict construction from convert_gateway_to_read, skipping .masked() so
that _populate_auth() leaves plaintext values in the _unmasked fields.
Update the test to drop the now-unnecessary monkeypatch of the removed method.

Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
gcgoncalves

This comment was marked as outdated.

Copy link
Copy Markdown
Collaborator

@gcgoncalves gcgoncalves left a comment

Choose a reason for hiding this comment

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

Disregard my last review, the problem was on my end.

@gandhipratik203 gandhipratik203 added release-fix Critical bugfix required for the release ready Validated, ready-to-work-on items labels Mar 13, 2026
@gandhipratik203 gandhipratik203 self-assigned this Mar 23, 2026
@marekdano
Copy link
Copy Markdown
Collaborator

@gandhipratik203 - can you please resolve conflicts?

@jonpspri
Copy link
Copy Markdown
Collaborator

Updated scope: this PR now also resolves the underlying bug in #2968 (admin UI can no longer reveal gateway tokens after security hardening). PR #3201 has been closed in favor of this approach.

Please update the PR description to include Closes #2968 alongside Closes #3346.

@crivetimihai crivetimihai removed the release-fix Critical bugfix required for the release label Apr 3, 2026
@jonpspri jonpspri self-assigned this Apr 12, 2026
@jonpspri jonpspri marked this pull request as draft April 12, 2026 19:41
@jonpspri
Copy link
Copy Markdown
Collaborator

Security Review — Converting to Draft

Security review identified issues that need to be addressed before this PR is merge-ready. Summary below, roughly prioritized.

Critical

  1. Viewer-level access to plaintext secrets (admin.py:11914) — The endpoint uses gateways.read, which is granted to viewer (the lowest-privilege role). Any viewer can reveal plaintext credentials. Needs a dedicated permission (e.g. gateways.reveal_credentials) restricted to platform_admin/team_admin, or at minimum gateways.update.

  2. No team-scoping — cross-tenant credential access (gateway_service.py:2779) — get_gateway_with_credentials queries by gateway_id alone with no team filter. A user on Team A who knows a gateway ID on Team B can reveal Team B's credentials.

High

  1. __repr__ leaks plaintext credentials (schemas.py:3633) — Pydantic's default __repr__ renders all field values. Any log, traceback, or debug statement touching a GatewayCredentialRevealResponse will emit plaintext secrets. Needs a __repr__ override or SecretStr with custom serializer.

  2. Unhandled decryption/validation errors (admin.py:11942) — Only GatewayNotFoundError is caught. Malformed auth data or encryption failures become opaque 500s with no logging context.

  3. Audit service failure blocks credential return (admin.py:11947) — If log_action() raises, the response is never returned. No explicit policy on whether audit failure should block reveal.

  4. Fetch errors only shown as tooltip (auth.js:91) — When the reveal fetch fails, the error is communicated only via button.title (hover tooltip). No showErrorMessage() or console.error().

Medium

  1. gatewayId not URL-encoded (auth.js:83) — Should use encodeURIComponent() as defense-in-depth.

  2. All credential fields populated at once (auth.js:30-46) — _populateRevealedCredentials caches all credentials in DOM data-real-value even when the user only clicked Show on one field.

  3. Missing deny-path regression tests — CLAUDE.md requires tests for unauthenticated, wrong-team, and insufficient-permission paths. None exist for this endpoint.

@jonpspri
Copy link
Copy Markdown
Collaborator

@gandhipratik203 This one makes me nervous -- let's revisit it after we've gone through the re-engineering of the security architecture.

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

Labels

bug Something isn't working enhancement New feature or request ready Validated, ready-to-work-on items security Improves security SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release ui User Interface

Projects

None yet

5 participants