Skip to content

Reject unauthenticated when no credentials#360

Open
heyitsaamir wants to merge 4 commits intomainfrom
aamirj/reject-unauthenticated-no-credentials
Open

Reject unauthenticated when no credentials#360
heyitsaamir wants to merge 4 commits intomainfrom
aamirj/reject-unauthenticated-no-credentials

Conversation

@heyitsaamir
Copy link
Copy Markdown
Collaborator

If a server is set up without any credentials, it currently will not be able to send messages, but it will currently accept all incoming requests.
This scenario is rare, but possible, and in this PR we reject any incoming requests if creds are not setup (and if skip_auth isn't explicitly set to True).

If a server is set up without any credentials, it currently will not be
able to send messages, but it will accept all incoming requests. This
scenario is rare, but possible, and in this PR we reject any incoming
requests if credentials are not configured (and if skip_auth isn't
explicitly set to True).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 5, 2026 06:17
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

Tightens the default security posture of the Apps HttpServer by rejecting incoming requests when no app credentials are configured (unless skip_auth=True), addressing the case where a server could previously accept unauthenticated inbound traffic.

Changes:

  • Reject incoming requests with 401 when skip_auth=False and no credentials/app ID are configured.
  • Add unit tests covering the “no credentials + skip_auth=False” rejection behavior.
  • Minor test cleanup (rename handler argument in FastAPI adapter test).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
packages/apps/src/microsoft_teams/apps/http/http_server.py Adds initialization warning for missing credentials and enforces 401 rejection when auth isn’t configured and auth isn’t skipped.
packages/apps/tests/test_http_server.py Adds test coverage ensuring requests are rejected when credentials are missing and skip_auth is false.

Comment on lines +131 to +133
except Exception as e:
logger.warning(f"JWT token validation failed: {e}")
return HttpResponse(status=401, body={"error": "Unauthorized"})
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

Avoid f-strings in logger calls; they eagerly format even when the log level is disabled. Use logger's %-style formatting (or pass exc_info) so formatting is lazy and exception details can be captured consistently.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@copilot can you address?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixed in 74d73f5 — changed logger.warning(f"JWT token validation failed: {e}") to logger.warning("JWT token validation failed: %s", e) so the message is only formatted when the warning level is actually enabled.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
heyitsaamir and others added 2 commits April 4, 2026 23:46
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 5, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • releases.astral.sh
    • Triggering command: /home/REDACTED/.local/bin/uv uv run pytest packages/apps/tests/test_http_server.py -v (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

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.

3 participants