diff --git a/application/single_app/config.py b/application/single_app/config.py index 11e7b300d..5a2d077a7 100644 --- a/application/single_app/config.py +++ b/application/single_app/config.py @@ -98,7 +98,7 @@ EXECUTOR_TYPE = 'thread' EXECUTOR_MAX_WORKERS = 30 SESSION_TYPE = 'filesystem' -VERSION = "0.261.049" +VERSION = "0.261.050" IS_DEVELOPMENT = is_development_env_enabled() # Opt-out for deployments where App Service Easy Auth is active but the platform diff --git a/application/single_app/semantic_kernel_plugins/plugin_invocation_logger.py b/application/single_app/semantic_kernel_plugins/plugin_invocation_logger.py index f973f2a3d..a4ce23b3d 100644 --- a/application/single_app/semantic_kernel_plugins/plugin_invocation_logger.py +++ b/application/single_app/semantic_kernel_plugins/plugin_invocation_logger.py @@ -62,6 +62,8 @@ r"(?i)\b(api[-_]?key|access[-_]?token|client[-_]?secret|connection[-_]?string|password|secret|subscription[-_]?key|token)=([^&\s,;]+)" ) AUTHORIZATION_VALUE_RE = re.compile(r"(?i)\b(Bearer|Basic)\s+[A-Za-z0-9._~+/=-]+") +# Azure Maps tile templates carry an encrypted, expiring proxy token the browser needs for tiles. +AZURE_MAPS_TILE_PROXY_QUERY_PREFIX = "/api/azure-maps/tile?" class PluginInvocationResult(str): @@ -109,9 +111,19 @@ def _redact_url(value: str) -> str: return urlunsplit((parts.scheme, netloc, parts.path, urlencode(query_pairs, doseq=True, safe="*"), parts.fragment)) +def _redact_secret_assignment(match: "re.Match[str]") -> str: + is_tile_proxy_token = ( + match.group(1).lower() == "token" + and match.string.endswith(AZURE_MAPS_TILE_PROXY_QUERY_PREFIX, 0, match.start()) + ) + if is_tile_proxy_token: + return match.group(0) + return f"{match.group(1)}={REDACTED_INVOCATION_VALUE}" + + def _sanitize_invocation_string(value: str, max_string_length: Optional[int]) -> str: sanitized = _redact_url(value) - sanitized = SECRET_ASSIGNMENT_RE.sub(lambda match: f"{match.group(1)}={REDACTED_INVOCATION_VALUE}", sanitized) + sanitized = SECRET_ASSIGNMENT_RE.sub(_redact_secret_assignment, sanitized) sanitized = AUTHORIZATION_VALUE_RE.sub(lambda match: f"{match.group(1)} {REDACTED_INVOCATION_VALUE}", sanitized) if max_string_length and len(sanitized) > max_string_length: return f"{sanitized[:max_string_length]}... [truncated]" diff --git a/docs/explanation/fixes/AZURE_MAPS_TILE_TOKEN_REDACTION_FIX.md b/docs/explanation/fixes/AZURE_MAPS_TILE_TOKEN_REDACTION_FIX.md new file mode 100644 index 000000000..f93ccc404 --- /dev/null +++ b/docs/explanation/fixes/AZURE_MAPS_TILE_TOKEN_REDACTION_FIX.md @@ -0,0 +1,68 @@ +# Azure Maps Tile Token Redaction Fix + +Fixed/Implemented in version: **0.261.050** + +## Issue Description + +Maps that an agent drew with the Azure Maps action showed their markers, paths and areas on a +blank background, in chat and in workflow results. Every tile request returned 400, "Invalid or +expired Azure Maps tile token." + +## Root Cause Analysis + +- Before a tool result is stored as a reply's agent citation, it passes through + `sanitize_plugin_invocation_value` in `semantic_kernel_plugins/plugin_invocation_logger.py`. + Chat (`_build_plugin_invocation_agent_citation`) and the workflow runner both do this. +- Its `SECRET_ASSIGNMENT_RE` replaces the value of any `token=` assignment with + `***REDACTED***`. That includes the map payload's `tile_url_template`, + `/api/azure-maps/tile?token=&...`. +- The stored template therefore carried `token=***REDACTED***`. The tile proxy could not decrypt + it, and the token refresh that runs when a conversation is opened could not reissue it. + +## Version Implemented + +- **0.261.050** +- Application version updated in `application/single_app/config.py` to `0.261.050`. + +## Files Modified + +- `application/single_app/semantic_kernel_plugins/plugin_invocation_logger.py` +- `application/single_app/config.py` +- `functional_tests/test_azure_maps_tile_token_redaction_fix.py` +- `docs/explanation/release_notes.md` + +## Code Changes Summary + +- New `AZURE_MAPS_TILE_PROXY_QUERY_PREFIX` (`/api/azure-maps/tile?`), matching + `AZURE_MAPS_TILE_PROXY_ROUTE` in `functions_azure_maps.py`. +- `_redact_secret_assignment` keeps a `token=` value only when it directly follows that prefix, + which is where `build_tile_proxy_url_template` writes it. Every other match is still redacted: + another key after the prefix, a token later in the query, the prefix on another path, and + absolute URLs, which `_redact_url` handles as before. +- The kept token is not a credential. It is encrypted with the app's secret key, expires after + 240 minutes, only works on SimpleChat's signed-in tile proxy, and the browser needs it to load + tiles. The Azure Maps subscription key never leaves the server. + +## Testing Approach + +`functional_tests/test_azure_maps_tile_token_redaction_fix.py` loads the real invocation logger +and Azure Maps helpers. It checks that: + +- A map result keeps its tile template, as an object and as JSON text, and the kept token still + decodes. +- Other `token=` values, other keys after the prefix, a token later in the query, `access_token`, + absolute tile URLs and a password next to a kept token are still redacted. +- The prefix matches `AZURE_MAPS_TILE_PROXY_ROUTE`. + +## Impact Analysis + +- New maps load their tiles, and the existing refresh keeps them loading after 240 minutes. +- Maps stored before this fix keep `***REDACTED***` in place of the token. That cannot be + recovered, so those maps still show no tiles. Running the request again produces a working map. + +## Validation + +- Before: a stored map's tile template read `token=***REDACTED***`, and every tile request + returned 400. +- After: the stored template keeps its proxy token and tiles load. Every other secret in a tool + result is redacted as before. diff --git a/docs/explanation/release_notes.md b/docs/explanation/release_notes.md index 100f71891..fb5ce3f8a 100644 --- a/docs/explanation/release_notes.md +++ b/docs/explanation/release_notes.md @@ -2,6 +2,16 @@ For feature-focused and fix-focused drill-downs by version, see [Features by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/features) and [Fixes by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/fixes). +### **(v0.261.050)** + +#### Bug Fixes + +* **Maps From Agents Load Their Tiles Again** + * Tool results are redacted before they are stored with a reply, and the redaction treated the Azure Maps tile proxy token as a secret. Every stored map lost its token, so maps drew their markers and paths on blank tiles. + * The token in SimpleChat's own tile template is now kept. It is encrypted with the app's secret key, expires, and only works on SimpleChat's signed-in tile proxy. Every other `token=` value and every other secret in a tool result is still redacted. + * Maps stored before this fix keep the redacted token and still show no tiles. Run the request again to get a map with tiles. + * (Ref: `plugin_invocation_logger.py`, [Azure Maps Tile Token Redaction Fix](fixes/AZURE_MAPS_TILE_TOKEN_REDACTION_FIX.md)) + ### **(v0.261.049)** #### Bug Fixes diff --git a/functional_tests/test_azure_maps_tile_token_redaction_fix.py b/functional_tests/test_azure_maps_tile_token_redaction_fix.py new file mode 100644 index 000000000..05e2f089f --- /dev/null +++ b/functional_tests/test_azure_maps_tile_token_redaction_fix.py @@ -0,0 +1,139 @@ +#!/usr/bin/env python3 +# test_azure_maps_tile_token_redaction_fix.py +""" +Functional test for the Azure Maps tile token redaction fix. +Version: 0.261.050 +Implemented in: 0.261.050 + +This test ensures that redacting a tool result before it is stored keeps the Azure Maps tile +proxy token, and only that token, so a stored map can load its tiles. +""" + +import contextlib +import importlib.util +import json +import sys +import types +from pathlib import Path +from urllib.parse import parse_qs, urlparse + +ROOT = Path(__file__).resolve().parents[1] +APP_DIR = ROOT / "application" / "single_app" + +sys.path.insert(0, str(ROOT / "functional_tests")) + +from test_support.versioning import assert_app_version_at_least # noqa: E402 + +IMPLEMENTED_IN = "0.261.050" + + +@contextlib.contextmanager +def _isolated_modules(**stubs): + missing = object() + saved = {name: sys.modules.get(name, missing) for name in stubs} + sys.modules.update(stubs) + try: + yield + finally: + for name, module in saved.items(): + if module is missing: + sys.modules.pop(name, None) + else: + sys.modules[name] = module + + +def _stub(name, **attributes): + module = types.ModuleType(name) + for attribute, value in attributes.items(): + setattr(module, attribute, value) + return module + + +def _load(module_name, path, **stubs): + spec = importlib.util.spec_from_file_location(module_name, path) + module = importlib.util.module_from_spec(spec) + with _isolated_modules(**stubs): + spec.loader.exec_module(module) + return module + + +def _load_modules(): + no_op = lambda *args, **kwargs: None # noqa: E731 + helpers = _load( + "functions_azure_maps_under_test", + APP_DIR / "functions_azure_maps.py", + config=_stub("config", SECRET_KEY="functional-test-secret"), + functions_appinsights=_stub("functions_appinsights", log_event=no_op), + ) + logger = _load( + "plugin_invocation_logger_under_test", + APP_DIR / "semantic_kernel_plugins" / "plugin_invocation_logger.py", + agent_execution_context=_stub("agent_execution_context", current_agent_execution=lambda: None), + functions_appinsights=_stub("functions_appinsights", log_event=no_op, get_appinsights_logger=lambda: None), + functions_authentication=_stub("functions_authentication", get_current_user_id=lambda: None), + functions_debug=_stub("functions_debug", debug_print=no_op), + ) + return helpers, logger + + +def test_map_results_keep_their_tile_proxy_token(): + """A stored map keeps a usable tile token, as an object and as JSON text.""" + print("Testing map tile templates through tool result redaction...") + helpers, logger = _load_modules() + sanitize = logger.sanitize_plugin_invocation_value + assert logger.AZURE_MAPS_TILE_PROXY_QUERY_PREFIX == f"{helpers.AZURE_MAPS_TILE_PROXY_ROUTE}?" + + template = helpers.build_tile_proxy_url_template(helpers.create_tile_proxy_token("maps-test-key")) + result = { + "success": True, + "render_type": helpers.AZURE_MAPS_RENDER_TYPE, + "map_payload": {"title": "Route", "tile_url_template": template}, + } + assert sanitize(result)["map_payload"]["tile_url_template"] == template + assert json.loads(sanitize(json.dumps(result)))["map_payload"]["tile_url_template"] == template + + token = parse_qs(urlparse(template).query)["token"][0] + assert helpers.decode_tile_proxy_token(token)["subscription_key"] == "maps-test-key" + print("Map tile token kept!") + + +def test_every_other_secret_is_still_redacted(): + """Only the token directly after the tile proxy prefix is kept.""" + print("Testing that other secrets are still redacted...") + _, logger = _load_modules() + sanitize = logger.sanitize_plugin_invocation_value + redacted = logger.REDACTED_INVOCATION_VALUE + + cases = { + "token=abc": f"token={redacted}", + "/api/other?token=abc": f"/api/other?token={redacted}", + "/api/azure-maps/tile?api_key=abc": f"/api/azure-maps/tile?api_key={redacted}", + "/api/azure-maps/tile?zoom=1&token=abc": f"/api/azure-maps/tile?zoom=1&token={redacted}", + "x/api/azure-maps/tile?access_token=abc": f"x/api/azure-maps/tile?access_token={redacted}", + "/api/azure-maps/tile?token=keep and password=hunter2": f"/api/azure-maps/tile?token=keep and password={redacted}", + } + for value, expected in cases.items(): + assert sanitize(value) == expected, value + assert "token=abc" not in sanitize("https://tiles.example.test/api/azure-maps/tile?token=abc") + print("Other secrets still redacted!") + + +def test_version_was_incremented(): + assert_app_version_at_least(IMPLEMENTED_IN) + + +if __name__ == "__main__": + tests = [ + test_map_results_keep_their_tile_proxy_token, + test_every_other_secret_is_still_redacted, + test_version_was_incremented, + ] + failures = 0 + for test in tests: + try: + test() + except Exception as exc: # noqa: BLE001 - report every failing check + failures += 1 + print(f"FAILED {test.__name__}: {exc}") + print(f"\n{len(tests) - failures}/{len(tests)} tests passed") + sys.exit(1 if failures else 0)