MCP Server addition and modification of SimpleChat API to accomodate …#722
MCP Server addition and modification of SimpleChat API to accomodate …#722gregunger-microsoft wants to merge 4 commits intoDevelopmentfrom
Conversation
…auth/session handshake.
There was a problem hiding this comment.
Overall, reads like a great start and I think the features exposed are awesome. It needs some enterprise TLC (scaling via redis for session cache, appinsights/LAW logging support, etc), but a great start none the less.
There was a problem hiding this comment.
Unclear on where you see redis coming into play here with regard to scaling? (on the MCP server side)
SimpleChat handles session. The MCP Server only has a session created during the auth handshake and then subsequently sends a Session ID as a cookie with requests to the SimpleChat API.
The MCP Server itself can easily scale out if needs be via horizontal scaling to accommodate concurrent user load.
But before we get into refactoring for scaling, we should harden any patterns used by integration components, including the MCP Server as a first pass before we start coding for scaling.
Again, if there is some way you could be more prescriptive in exactly what you had in mind, I'm happy to accommodate.
Same thing with logging here. If there is some logging pattern you want implement, let's discuss.
There was a problem hiding this comment.
I thought I saw where the MCP server is maintaining its own session state, if I was mistaken, then scaling is already built in by being stateless.
There was a problem hiding this comment.
Any reason this would not run in a distroless (mcr.microsoft.com/azurelinux/distroless/python:3.12) multi-step build like the core app?
There was a problem hiding this comment.
So no issues if distroless is a requirement. But at this point in the development, it is premature for the MCP Server itself and for non-production use-cases. We could easily have multiple docker files to accomodate distroless and not distroless.
Using a distroless container image means your container includes only your application and its runtime — no shell, no package manager, no debugging tools.
That sounds ideal, especially prematurely when adding new functionality/components to a system that may require higher visibility/observability in a given system.
Here are the main reasons why using a distroless image can be a bad idea:
#1 Debugging Becomes Painful
Distroless images don’t include:
bash / sh
curl
ps
netstat
apt / yum
- i.e. Most standard Linux tools
If something goes wrong in production, you can’t exec into the container and inspect it. Because there is no shell.
This makes incident response slower and more complex. You need:
- Sidecar debug containers
- Ephemeral debug images
- Rebuilds just to inspect behavior
That’s fine for mature software. Not so great for fast-moving agile software development.
#2 Local Development Becomes Friction-Filled
Distroless is great for production minimalism.
It’s terrible for:
- Experimentation
- Rapid troubleshooting
- Investigating weird runtime edge cases
Developers often end up maintaining:
- One Dockerfile for dev
- One for prod
Now you have divergence risk.
#3 Observability Tooling Limitations
Many debugging or monitoring tools assume basic OS utilities exist.
If you rely on:
- Runtime profilers
- On-the-fly tracing
- Crash dump analysis
- Temporary diagnostics
You may discover the container simply doesn’t support what you need.
#4 Operational Complexity Increases
Distroless pushes complexity outward.
Instead of “Let’s exec in and look.”
You now need:
- Debug sidecars
- Separate debug images
- Stronger logging discipline
- More complete telemetry
If your observability is weak, distroless will expose it brutally.
#5 Compatibility Surprises
Some apps assume:
- Timezone data exists
- CA certificates are present
- Locale data is available
/bin/shexists for subprocesses
Distroless images may omit or minimally include these.
You discover issues at runtime.
#6 Security Gains May Be Marginal
Distroless is often sold as “more secure.”
But:
- The real attack surface is usually your application.
- Most container exploits rely on runtime vulnerabilities, not
bash. - A properly hardened minimal distro (e.g., Alpine or slim Debian) may provide nearly identical risk reduction.
Security benefit is real — but sometimes overstated.
#7 Not Ideal for Dynamic Environments
If your application:
- Installs plugins dynamically
- Runs scripts
- Uses subprocess-heavy workflows
- Executes user-provided commands
Distroless can break those assumptions.
There was a problem hiding this comment.
It will be a requirement for production (a ton of customer have requested it because of the amount of CVEs that are eliminated), but your points are valid for rapid iteration/development. That said, for rapid iteration, you should also be able to run it locally/dev container that has obserability/tools. We also prefer to rely on high and strong logging discipline.
| def register_route_external_prm(app): | ||
| @app.route('/.well-known/oauth-protected-resource', methods=['GET']) | ||
| @swagger_route(security=get_auth_security()) | ||
| def get_prm_metadata(): |
There was a problem hiding this comment.
Does this route need to be unauthenticated (pretty sure it does)? Security risks associated with exposing the app id and tenantId to a non-authenticated endpoint? We likely will need to document this, and we need to add to code as comments that this route is explicitly unprotected by authentication (this might actually be our first one).
There was a problem hiding this comment.
No — a PRM authentication file does not need to be unauthenticated for MCP server.
In a typical MCP (Model Context Protocol) server–client handshake, the sequence looks like:
1 Client connects to server
2 Client presents credentials (token, key, cert, etc.)
3 Server validates credentials
4 Secure session is established
The authentication material (like a PRM file containing keys or metadata) is used during validation, not before it.
So the file does not need to be publicly accessible or unauthenticated.
Bionic711
left a comment
Bionic711
left a comment
There was a problem hiding this comment.
Not prod ready, but a fantastic start. We need the option to disable and enabled the mcp server from admin options, with a default to disable which includes a note that it requires a restart of the app to take effect. Scaling via Redis (at minimal for session cache and I imagine many customers will implement in app service instead of ACA as that is how most of them run it). Logging into AppInsights with OTEL.
Logging and scaling we could approve without. However, the admin settings via opt-in is required before we can get this into development. We should have a modal on the configuration page that guides the admin through the general steps to deploy the external app (platform agnostic) and, if we leave the metadata page unauthenticated, a pop-up that the admin needs to accept (and is then logged to the activity log with which admin did it) that they accepted that the tenantid and appid are exposed on an unauthenticated endpoint.
There was a problem hiding this comment.
Pull request overview
Adds an external MCP (Model Context Protocol) server integration and extends SimpleChat’s auth surface to support PRM-based OAuth discovery and bearer-token-to-session bridging for API-to-API SSO.
Changes:
- Added a minimal FastMCP server with PRM auth shim, device-code login option, and SimpleChat API tool wrappers.
- Added new SimpleChat endpoints for PRM metadata (
/.well-known/oauth-protected-resource) and external session creation (POST /external/login). - Updated existing auth flow to optionally create a server-side session from
/getATokenApi.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| application/single_app/route_frontend_authentication.py | Adds helpers and extends /getATokenApi to optionally create a session and accept a local redirect URI. |
| application/single_app/route_external_prm.py | Introduces PRM discovery endpoint for OAuth-protected resource metadata. |
| application/single_app/route_external_authentication.py | Adds /external/login to create a SimpleChat session from a validated Entra bearer token. |
| application/single_app/functions_authentication.py | Expands accesstoken_required authorization logic and exposes claims via flask.g. |
| application/single_app/app.py | Registers new external auth + PRM routes. |
| application/external_apps/mcp/server_minimal.py | New MCP server implementation, PRM auth shim, session caching, and SimpleChat tool calls. |
| application/external_apps/mcp/run_mcp_server.ps1 | Local runner script for the MCP server. |
| application/external_apps/mcp/requirements.txt | MCP server Python dependencies. |
| application/external_apps/mcp/prm_metadata.json | PRM metadata JSON consumed by the MCP server shim. |
| application/external_apps/mcp/example.env | Example environment config for MCP server deployment/runs. |
| application/external_apps/mcp/deploy_mcp_containerapp.ps1 | Azure Container Apps build/deploy script for the MCP server. |
| application/external_apps/mcp/README.md | MCP server usage and configuration documentation. |
| application/external_apps/mcp/Dockerfile | Container image build for the MCP server. |
| application/external_apps/mcp/.dockerignore | Docker ignore rules for the MCP server build context. |
| .gitignore | Adds ignores for MCP .env.azure and a user-specific local folder. |
| if not is_external_api_authorized(data): | ||
| return jsonify({"message": "Forbidden: ExternalApi, User, or Admin role/scope required"}), 403 |
There was a problem hiding this comment.
accesstoken_required now authorizes tokens with User/Admin role or scope, not just ExternalApi. This unintentionally grants access to all /external/public_documents/* endpoints (which rely on caller-supplied user_id / active_workspace_id) to regular user tokens, enabling cross-user actions. Consider keeping accesstoken_required restricted to ExternalApi (app-only) and introducing a separate decorator (or parameter) for /external/login that validates the bearer token but does not broaden authorization for other external routes; also prefer deriving user_id from token claims when possible.
| scope_values = [scope for scope in scopes_raw.split() if scope] | ||
| for scope in scope_values: | ||
| scope_name = scope.split("/")[-1] | ||
| if scope_name in allowed_roles: | ||
| return True |
There was a problem hiding this comment.
is_external_api_authorized treats scope values like "api://.../User" as authorization for external APIs by mapping the last path segment to a role name. This makes it easy to accidentally authorize unintended scopes that merely end with User/Admin/ExternalApi. Tighten this to an explicit allowlist of full scope strings (or check aud + exact scp values) to avoid false-positive authorization.
|
|
||
|
|
||
| def register_route_external_prm(app): | ||
| @app.route('/.well-known/oauth-protected-resource', methods=['GET']) | ||
| @swagger_route(security=get_auth_security()) | ||
| def get_prm_metadata(): | ||
| resource = request.host_url.rstrip('/') |
There was a problem hiding this comment.
resource = request.host_url will often produce the wrong scheme (http://...) behind Azure ingress / reverse proxies unless ProxyFix (or equivalent) is configured. Since PRM metadata is used by clients for subsequent auth, consider deriving scheme from X-Forwarded-Proto (and host from X-Forwarded-Host when present) to ensure resource is correct in production.
| def register_route_external_prm(app): | |
| @app.route('/.well-known/oauth-protected-resource', methods=['GET']) | |
| @swagger_route(security=get_auth_security()) | |
| def get_prm_metadata(): | |
| resource = request.host_url.rstrip('/') | |
| from flask import request, jsonify | |
| def register_route_external_prm(app): | |
| @app.route('/.well-known/oauth-protected-resource', methods=['GET']) | |
| @swagger_route(security=get_auth_security()) | |
| def get_prm_metadata(): | |
| # Derive external-facing resource URL in a proxy-aware way | |
| scheme = request.headers.get('X-Forwarded-Proto', request.scheme) | |
| host = ( | |
| request.headers.get('X-Forwarded-Host') | |
| or request.headers.get('Host') | |
| or request.host | |
| ) | |
| resource = f"{scheme}://{host}".rstrip('/') |
| def register_route_external_authentication(app): | ||
| @app.route('/external/login', methods=['POST']) | ||
| @swagger_route(security=get_auth_security()) | ||
| @accesstoken_required | ||
| def external_login(): | ||
| """ | ||
| Creates a server-side session using a validated Entra bearer token. | ||
| Returns session details for external clients (e.g., MCP servers). | ||
| """ | ||
| claims = getattr(g, "user_claims", None) | ||
| if not isinstance(claims, dict): | ||
| return jsonify({"error": "Unauthorized", "message": "No user claims available"}), 401 | ||
|
|
||
| session["user"] = claims | ||
|
|
||
| session_id = getattr(session, "sid", None) or session.get("session_id") or session.get("_id") | ||
| if not session_id: | ||
| session_id = str(uuid4()) | ||
| session["session_id"] = session_id | ||
|
|
||
| response_payload = { | ||
| "session_created": True, | ||
| "session_id": session_id, | ||
| "user": { | ||
| "userId": claims.get("oid") or claims.get("sub"), | ||
| "displayName": claims.get("name"), | ||
| "email": claims.get("preferred_username") or claims.get("email") | ||
| }, | ||
| "claims": claims | ||
| } | ||
|
|
||
| debug_print(f"External login session created for user {response_payload['user'].get('userId')}") | ||
| return jsonify(response_payload), 200 |
There was a problem hiding this comment.
New auth/session behavior is being added (/external/login, PRM endpoint, and broadened bearer-token auth). There are extensive functional_tests/ in this repo, but this PR doesn’t add coverage for the new external login handshake. Please add a functional test that (1) calls /.well-known/oauth-protected-resource, and (2) calls /external/login with a valid bearer token and verifies a session cookie/session id is established.
application/single_app/app.py
Outdated
| # ------------------- External Authentication Routes --- | ||
| register_route_external_authentication(app) | ||
|
|
||
| # ------------------- PRM Metadata Routes -------------- | ||
| register_route_external_prm(app) |
There was a problem hiding this comment.
This PR introduces new externally-consumable endpoints and an MCP integration, but config.py’s VERSION is not bumped. Per the repo’s versioning pattern (third segment only), please increment the patch version in application/single_app/config.py so the change is traceable in deployments.
| # This route is for API calls that need a token, not the web app login flow. This does not kick off a session. | ||
| @app.route('/getATokenApi') # This is your redirect URI path | ||
| @swagger_route(security=get_auth_security()) | ||
| def authorized_api(): | ||
| create_session = _parse_bool(request.args.get('create_session'), default=False) | ||
| request_redirect_uri = request.args.get('redirect_uri') |
There was a problem hiding this comment.
The comment above authorized_api says this route “does not kick off a session”, but the new create_session query param explicitly creates a session and persists token cache. Update the comment so future callers don’t rely on outdated behavior.
| # Session cache: bearer_token -> requests.Session | ||
| _SESSION_CACHE: Dict[str, requests.Session] = {} | ||
| _SESSION_LOCK = threading.Lock() | ||
|
|
||
| # Cache the /external/login payload (contains user + claims) per bearer token. | ||
| _LOGIN_PAYLOAD_CACHE: Dict[str, Dict[str, Any]] = {} | ||
|
|
||
| # Cache bearer token per MCP streamable-http session id. This lets the server reuse |
There was a problem hiding this comment.
_SESSION_CACHE / _LOGIN_PAYLOAD_CACHE are keyed by the raw bearer token and never evicted. In a long-running server this can grow without bound (new token => new entry) and also keeps tokens resident in memory longer than necessary. Consider keying by a hash of the token and adding TTL/LRU eviction (or clearing caches on token expiry) to avoid unbounded memory growth.
| [string]$SubscriptionId = "56013a89-2bdc-403e-9f7f-17da3c9d8ab4", | ||
|
|
||
| [Parameter(Mandatory = $false)] | ||
| [string]$ResourceGroup = "aaronba-simplechat-rg", | ||
|
|
||
| [Parameter(Mandatory = $false)] | ||
| [string]$Location = "", | ||
|
|
||
| [Parameter(Mandatory = $false)] | ||
| [string]$ContainerAppName = "gunger-simplechat-mcp", | ||
|
|
||
| [Parameter(Mandatory = $false)] | ||
| [string]$EnvironmentName = "aaronba-simplechat-v2-env", | ||
|
|
||
| [Parameter(Mandatory = $false)] | ||
| [string]$AcrName = "aaronbasimplechatacr", | ||
|
|
||
| [Parameter(Mandatory = $false)] | ||
| [string]$ImageName = "gunger-simplechat-mcp", |
There was a problem hiding this comment.
The default parameter values (subscription ID, resource group, ACR name, environment name, etc.) look like personal/real Azure resources. Keeping real defaults in-repo makes accidental deployments more likely and can leak internal resource naming. Consider making these parameters required (no defaults) or using clearly placeholder defaults.
| [string]$SubscriptionId = "56013a89-2bdc-403e-9f7f-17da3c9d8ab4", | |
| [Parameter(Mandatory = $false)] | |
| [string]$ResourceGroup = "aaronba-simplechat-rg", | |
| [Parameter(Mandatory = $false)] | |
| [string]$Location = "", | |
| [Parameter(Mandatory = $false)] | |
| [string]$ContainerAppName = "gunger-simplechat-mcp", | |
| [Parameter(Mandatory = $false)] | |
| [string]$EnvironmentName = "aaronba-simplechat-v2-env", | |
| [Parameter(Mandatory = $false)] | |
| [string]$AcrName = "aaronbasimplechatacr", | |
| [Parameter(Mandatory = $false)] | |
| [string]$ImageName = "gunger-simplechat-mcp", | |
| [string]$SubscriptionId = "REPLACE_WITH_SUBSCRIPTION_ID", | |
| [Parameter(Mandatory = $false)] | |
| [string]$ResourceGroup = "REPLACE_WITH_RESOURCE_GROUP", | |
| [Parameter(Mandatory = $false)] | |
| [string]$Location = "", | |
| [Parameter(Mandatory = $false)] | |
| [string]$ContainerAppName = "REPLACE_WITH_CONTAINERAPP_NAME", | |
| [Parameter(Mandatory = $false)] | |
| [string]$EnvironmentName = "REPLACE_WITH_ENVIRONMENT_NAME", | |
| [Parameter(Mandatory = $false)] | |
| [string]$AcrName = "REPLACE_WITH_ACR_NAME", | |
| [Parameter(Mandatory = $false)] | |
| [string]$ImageName = "REPLACE_WITH_IMAGE_NAME", |
| "https://login.microsoftonline.com/7d887458-fb0d-40bf-adb3-084d875f65db/v2.0" | ||
| ], | ||
| "scopes_supported": [ | ||
| "api://0b8c00b9-4dcd-4959-83be-7a0521ce54ce/.default" |
There was a problem hiding this comment.
This PRM metadata file contains what appear to be real tenant/client IDs. Since this repo is shared, consider replacing these with placeholders (or moving to an explicit example file) to avoid coupling the repo to a specific Entra tenant/app registration.
| "https://login.microsoftonline.com/7d887458-fb0d-40bf-adb3-084d875f65db/v2.0" | |
| ], | |
| "scopes_supported": [ | |
| "api://0b8c00b9-4dcd-4959-83be-7a0521ce54ce/.default" | |
| "https://login.microsoftonline.com/{TENANT_ID}/v2.0" | |
| ], | |
| "scopes_supported": [ | |
| "api://{APPLICATION_CLIENT_ID}/.default" |
| # SimpleChat MCP Server (FastMCP) | ||
|
|
||
| This MCP server provides **14 tools** for interacting with SimpleChat via the Model Context Protocol (Streamable HTTP transport). | ||
|
|
There was a problem hiding this comment.
This README documents the MCP server, but the repo’s main feature docs live under docs/explanation/features/<version>/.... Consider adding a corresponding feature doc there (and linking to this README) so the new external integration is discoverable alongside other SimpleChat features.
| For a high-level overview of this integration alongside other SimpleChat features, see the MCP feature documentation under `docs/explanation/features/<version>/MCP_SERVER_INTEGRATION.md`. |
| def register_route_external_authentication(app): | ||
| @app.route('/external/login', methods=['POST']) | ||
| @swagger_route(security=get_auth_security()) | ||
| @accesstoken_required | ||
| def external_login(): |
There was a problem hiding this comment.
/external/login creates a server-side session from a bearer token and returns session metadata; this is a critical auth boundary for the MCP integration. Please add a functional test that validates: 401 on missing/invalid bearer token, 403 when required roles/scopes are absent, and 200 returns a session_id when valid. Follow the repo’s functional test conventions (including the version header).
| 1) Check headers of request to make sure this hard-coded API key exists: | ||
| MCP Server API KEY: pYVuDbG3V8NpMVrQm0g9dVwoLa3kLZ4D | ||
|
|
||
| 2) Check to make sure MCP Server identified by this API Key: pYVuDbG3V8NpMVrQm0g9dVwoLa3kLZ4D is enabled. | ||
|
|
||
| 3) if #2 is enabled, check user claims for "CoPilotUser" role | ||
|
|
||
| if all checks are valid: create and return session | ||
|
|
||
| if not valid: error |
There was a problem hiding this comment.
This docstring contains a hard-coded API key value. Even though it’s in a TODO comment, committing a real-looking key is a credential leak risk and will likely get scraped/flagged. Please remove the key from source control and, if an API key header check is required, read it from configuration (env/KeyVault) and compare using a constant-time comparison.
| 1) Check headers of request to make sure this hard-coded API key exists: | |
| MCP Server API KEY: pYVuDbG3V8NpMVrQm0g9dVwoLa3kLZ4D | |
| 2) Check to make sure MCP Server identified by this API Key: pYVuDbG3V8NpMVrQm0g9dVwoLa3kLZ4D is enabled. | |
| 3) if #2 is enabled, check user claims for "CoPilotUser" role | |
| if all checks are valid: create and return session | |
| if not valid: error | |
| 1) Check headers of the request to make sure a valid MCP Server API key is present. | |
| - The expected API key MUST be read from secure configuration (e.g., env/KeyVault) | |
| rather than hard-coded. | |
| - Compare the provided API key to the configured value using a constant-time | |
| comparison to avoid timing attacks. | |
| 2) Check to make sure the MCP Server identified by the validated API key is enabled. | |
| 3) If #2 is enabled, check user claims for the "CoPilotUser" role. | |
| If all checks are valid: create and return a session. | |
| If not valid: return an error response. |
|
|
||
| try: | ||
| top_n = int(top_n) | ||
| except (TypeError, ValueError): | ||
| return jsonify({"error": "top_n must be an integer"}), 400 | ||
|
|
There was a problem hiding this comment.
top_n is user-controlled and currently has no upper bound. A large value will increase vector search kNN and payload sizes and can become an easy DoS lever against the search service. Consider clamping top_n to a reasonable max (e.g. 50) and returning 400 when it exceeds that limit.
| try: | |
| top_n = int(top_n) | |
| except (TypeError, ValueError): | |
| return jsonify({"error": "top_n must be an integer"}), 400 | |
| MAX_TOP_N = 50 | |
| try: | |
| top_n = int(top_n) | |
| except (TypeError, ValueError): | |
| return jsonify({"error": "top_n must be an integer"}), 400 | |
| if top_n <= 0: | |
| return jsonify({"error": "top_n must be a positive integer"}), 400 | |
| if top_n > MAX_TOP_N: | |
| return jsonify({"error": f"top_n cannot be greater than {MAX_TOP_N}"}), 400 |
| except Exception as e: | ||
| debug_print(f"Search error: {e}", "SEARCH_API") | ||
| return jsonify({"error": "Search failed", "message": str(e)}), 500 |
There was a problem hiding this comment.
The 500 response includes message: str(e), which can leak internal exception details to clients (index names, query filters, stack-adjacent messages). Prefer returning a generic error message and logging the exception server-side (optionally including a correlation id) instead of echoing the raw exception text.
| def _start_background_poll() -> None: | ||
| token_url = _env("OAUTH_TOKEN_URL") | ||
| client_id = _env("OAUTH_CLIENT_ID") | ||
| client_secret = _env("OAUTH_CLIENT_SECRET") |
There was a problem hiding this comment.
_start_background_poll() requires OAUTH_CLIENT_SECRET via _env("OAUTH_CLIENT_SECRET"), but the device-code flow implementation explicitly runs in public-client mode and never sends a secret. With OAUTH_CLIENT_SECRET unset/blank (as in example.env), device-code login will fail immediately. Make OAUTH_CLIENT_SECRET optional here (use os.getenv) and only require it when implementing a confidential-client auth-code flow.
| client_secret = _env("OAUTH_CLIENT_SECRET") | |
| client_secret = os.getenv("OAUTH_CLIENT_SECRET") or "" |
| document_id = data.get("document_id") | ||
| top_n = data.get("top_n", 12) | ||
| active_group_id = data.get("active_group_id") | ||
| active_public_workspace_id = data.get("active_public_workspace_id") | ||
|
|
||
| try: | ||
| top_n = int(top_n) | ||
| except (TypeError, ValueError): | ||
| return jsonify({"error": "top_n must be an integer"}), 400 | ||
|
|
||
| debug_print( | ||
| f"API search request", | ||
| "SEARCH_API", | ||
| user_id=user_id, | ||
| query=query[:40], | ||
| doc_scope=doc_scope, | ||
| top_n=top_n | ||
| ) | ||
|
|
||
| try: | ||
| results = hybrid_search( | ||
| query=query, | ||
| user_id=user_id, | ||
| document_id=document_id, | ||
| top_n=top_n, | ||
| doc_scope=doc_scope, | ||
| active_group_id=active_group_id, | ||
| active_public_workspace_id=active_public_workspace_id, | ||
| ) |
There was a problem hiding this comment.
This endpoint trusts active_group_id / active_public_workspace_id provided by the client and passes them into hybrid_search. In functions_search.hybrid_search, the group/public queries filter by the provided IDs without verifying that the authenticated user is actually a member of that group or has visibility to that public workspace, which can allow cross-group/workspace data exposure. Derive these IDs server-side from the user’s settings and/or enforce authorization checks (e.g., assert_group_role for groups; verify the public workspace ID is in get_user_visible_public_workspace_ids_from_settings(user_id)) before calling hybrid_search.
| # Session cache: bearer_token -> requests.Session | ||
| _SESSION_CACHE: Dict[str, requests.Session] = {} | ||
| _SESSION_LOCK = threading.Lock() | ||
|
|
||
| # Cache the /external/login payload (contains user + claims) per bearer token. | ||
| _LOGIN_PAYLOAD_CACHE: Dict[str, Dict[str, Any]] = {} | ||
|
|
There was a problem hiding this comment.
The in-memory caches (_SESSION_CACHE / _LOGIN_PAYLOAD_CACHE) are keyed by bearer token and never evicted. In a long-running server (or under token rotation), this can grow without bound and retain sensitive data in memory indefinitely. Consider adding TTL + periodic cleanup, capping cache size (LRU), and clearing entries when /external/login returns 401/403.
| mcp | ||
| requests | ||
| python-dotenv | ||
| msal |
There was a problem hiding this comment.
requirements.txt includes msal, but the MCP server code doesn’t import or use it (auth is implemented with raw requests). Dropping unused dependencies reduces image size and supply-chain/attack surface; alternatively, switch the device-code flow to MSAL and remove the custom polling logic.
| msal |
| EXECUTOR_MAX_WORKERS = 30 | ||
| SESSION_TYPE = 'filesystem' | ||
| VERSION = "0.237.011" | ||
| VERSION = "0.237.014" |
There was a problem hiding this comment.
Version was bumped from 0.237.011 to 0.237.014. If the repo’s convention is to increment only the patch component per change, consider using the next patch version (typically +1) to avoid skipping versions and making release notes harder to track.
| VERSION = "0.237.014" | |
| VERSION = "0.237.012" |
| mcp | ||
| requests | ||
| python-dotenv | ||
| msal |
There was a problem hiding this comment.
requirements.txt installs mcp, requests, python-dotenv, and msal without any version pinning, so each container build (e.g., via deploy_mcp_containerapp.ps1 -> az acr build) will pull whatever versions are latest on PyPI at build time. If one of these packages is compromised or a malicious update is published, a future build can silently introduce arbitrary code into the MCP server container and exfiltrate OAuth tokens or SimpleChat data. Pin these dependencies to vetted versions (and ideally include hashes) in line with the rest of the repo so builds are deterministic and resistant to supply-chain compromise.
Added MCP Server code with PRM auth handshake and very minimal SimpleChat API changes to accomodate auth/session.
tried to assign to Paul/Nathaniel but it is not working.
/assign @paullizer
/assign @nadoylemsft