Skip to content

Fix allowlist plumbing through through TokenValidator#404

Merged
corinagum merged 2 commits intomainfrom
cg/fix-allowlist-plumbing
Apr 24, 2026
Merged

Fix allowlist plumbing through through TokenValidator#404
corinagum merged 2 commits intomainfrom
cg/fix-allowlist-plumbing

Conversation

@corinagum
Copy link
Copy Markdown
Contributor

App(additional_allowed_domains=[...]) was silently ignored at the token-validation layer, rejecting user-approved service URLs (e.g. canary.botapi.skype.com) before they ever reached the allowlist-aware check in HttpServer.handle_request.

Fix threads the allowlist through TokenValidator.__init__, both factory methods (for_service, for_entra), and the validate_token call site. Updates HttpServer.initialize and App.__init__ to forward the option.

Why unit tests missed it

All existing additional_allowed_domains tests used skip_auth=True, which bypasses the TokenValidator code path entirely. The helper function is_allowed_service_url had direct tests with 3 args, so the function itself was fine — just no test proved the code calling it in the auth-on path was also using 3 args. E2E validation was against default service url, which is in the built-in allowlist and doesn't need the knob. The intersection of "auth on" AND "non-default service URL" had zero coverage.

Tests

test_token_validator.py:

  • test_validate_token_honors_additional_allowed_domains — canary accepted when in allowlist
  • test_validate_token_rejects_when_domain_not_in_allowlist — baseline: canary rejected without the knob
  • test_validate_token_wildcard_allows_arbitrary_domain["*"] disables the check at the token-validation layer
  • test_for_service_stores_additional_allowed_domains / test_for_entra_stores_additional_allowed_domains — factory plumbing

test_http_server.py:

  • test_initialize_forwards_allowlist_to_token_validator — spies TokenValidator.for_service and asserts it's called with the allowlist (catches the exact regression
    class)

All 67 tests pass.

Test plan

  • Unit: uv run pytest packages/apps/tests/test_token_validator.py packages/apps/tests/test_http_server.py — 67/67
  • Lint/format: uv run poe check — clean
  • Type check: uv run pyright — no new errors
  • E2E on canary: verified activity from https://canary.botapi.skype.com/amer/... passes token validation and reaches onMessage (previously 401'd)
  • E2E regression: default smba.trafficmanager.net still works

Copilot AI review requested due to automatic review settings April 24, 2026 18:09
Copy link
Copy Markdown
Contributor

@singhk97 singhk97 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes a regression where additional_allowed_domains configured on App(...) was not applied during JWT token validation, causing valid non-default service URLs (e.g. canary) to be rejected before reaching the server-level allowlist check.

Changes:

  • Thread additional_allowed_domains through TokenValidator (__init__, for_service, for_entra) and enforce it in validate_token.
  • Forward the allowlist from HttpServer.initialize and App.__init__ into the token validator construction.
  • Add unit tests covering the “auth on + non-default service URL” path and verify HttpServer.initialize forwards the allowlist.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/apps/src/microsoft_teams/apps/auth/token_validator.py Plumbs allowlist into TokenValidator and uses it during validate_token service URL checks.
packages/apps/src/microsoft_teams/apps/http/http_server.py Forwards server-configured additional_allowed_domains into TokenValidator.for_service(...).
packages/apps/src/microsoft_teams/apps/app.py Forwards app option additional_allowed_domains into TokenValidator.for_entra(...).
packages/apps/tests/test_token_validator.py Adds coverage proving validate_token honors/rejects based on additional_allowed_domains, including wildcard behavior and factory plumbing.
packages/apps/tests/test_http_server.py Adds a regression test ensuring HttpServer.initialize passes the allowlist to TokenValidator.for_service.

Comment thread packages/apps/src/microsoft_teams/apps/auth/token_validator.py
Comment thread packages/apps/src/microsoft_teams/apps/auth/token_validator.py Outdated
Comment thread packages/apps/src/microsoft_teams/apps/auth/token_validator.py Outdated
@corinagum corinagum merged commit 01228e6 into main Apr 24, 2026
7 checks passed
@corinagum corinagum deleted the cg/fix-allowlist-plumbing branch April 24, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants