-
Notifications
You must be signed in to change notification settings - Fork 3
Add native (React Native) OAuth sign-in #123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |||||||||||||
| from mcp.server.fastmcp import FastMCP | ||||||||||||||
| from pathlib import Path | ||||||||||||||
| from rbt.v1alpha1.application.application_pb2 import ExamplePrompt | ||||||||||||||
| from reboot.aio.auth.native_redirect_uris import validate_native_redirect_uri | ||||||||||||||
| from reboot.aio.auth.oauth_providers import OAuthProviderSelector | ||||||||||||||
| from reboot.aio.auth.oauth_server import OAuthServer | ||||||||||||||
| from reboot.aio.auth.token_verifiers import ( | ||||||||||||||
|
|
@@ -250,6 +251,7 @@ def __init__( | |||||||||||||
| token_verifier: Optional[TokenVerifier] = None, | ||||||||||||||
| oauth: Optional[OAuthProviderSelector] = None, | ||||||||||||||
| allowed_origins: Optional[list[str]] = None, | ||||||||||||||
| native_redirect_uris: Optional[list[str]] = None, | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're starting to grow quite a list of OAuth-specific parameters... Should we fold those into a single combined object?
Suggested change
Or prefix them all with
Suggested change
Or... [other idea]? What do you think? |
||||||||||||||
| title: Optional[str] = None, | ||||||||||||||
| description: Optional[str] = None, | ||||||||||||||
| example_prompts: Optional[list[ExamplePrompt]] = None, | ||||||||||||||
|
|
@@ -322,6 +324,49 @@ def __init__( | |||||||||||||
| default-None case almost always means "the developer | ||||||||||||||
| forgot", and we'd rather raise loudly than silently | ||||||||||||||
| CORS-block every sign-in attempt in production. | ||||||||||||||
| :param native_redirect_uris: exact-match list of the redirect | ||||||||||||||
| URIs belonging to this application's own first-party | ||||||||||||||
| native apps — a mobile app's custom scheme (e.g. | ||||||||||||||
| `"myapp://redirect"`), or an `https://` App Link / | ||||||||||||||
| Universal Link. A native app cannot use the browser | ||||||||||||||
| sign-in flow (there is no page to redirect and no cookie | ||||||||||||||
| jar to hold the session), so it registers itself | ||||||||||||||
| dynamically (RFC 7591) and completes an ordinary | ||||||||||||||
| authorization-code flow with PKCE instead. | ||||||||||||||
|
|
||||||||||||||
| Registration proves nothing about who is registering, so | ||||||||||||||
| by default such a client is treated as third-party: the | ||||||||||||||
| user is shown a consent screen naming it, and the flow | ||||||||||||||
| only continues once they approve. That screen is what | ||||||||||||||
| stands between a user and an attacker who registers a | ||||||||||||||
| client with *their own* `redirect_uri`, sends the user an | ||||||||||||||
| `/__/oauth/authorize` link on this trusted origin, and | ||||||||||||||
| collects an access token for the user's identity once | ||||||||||||||
| they sign in. PKCE is no help there, because in that | ||||||||||||||
| attack the attacker is the registered client. | ||||||||||||||
|
|
||||||||||||||
| Listing a redirect URI here says it is yours, so a client | ||||||||||||||
| that registers only such URIs signs the user in directly, | ||||||||||||||
| with no consent screen — the same treatment the browser | ||||||||||||||
| SPA gets. It is safe for exactly one reason: an | ||||||||||||||
| authorization code issued for one of these URIs is | ||||||||||||||
| delivered to *your* app, so an attacker registering the | ||||||||||||||
| same URI gains nothing. Entries are therefore compared for | ||||||||||||||
| exact equality, and wildcards are refused. | ||||||||||||||
|
|
||||||||||||||
| Under `rbt dev run`, Expo's `exp://<host>/--/...` | ||||||||||||||
| development URIs are trusted automatically, because they | ||||||||||||||
| carry the development machine's address and port and so | ||||||||||||||
| have no stable spelling to list here. | ||||||||||||||
|
|
||||||||||||||
| Note that a custom scheme is claimed on a first-come basis | ||||||||||||||
| on some platforms, so a hostile app on the same device can | ||||||||||||||
| register `myapp://` too. PKCE contains that: the code it | ||||||||||||||
| intercepts is useless without the verifier, which never | ||||||||||||||
| leaves your app. An `https://` App Link / Universal Link, | ||||||||||||||
| which the operating system verifies against your domain, | ||||||||||||||
| avoids the race entirely and is the stronger choice where | ||||||||||||||
| you can use one. | ||||||||||||||
| :param title: a human-readable name for the application. | ||||||||||||||
| Defaults to `application_name()` if unset. | ||||||||||||||
| :param description: a human-readable description of the | ||||||||||||||
|
|
@@ -525,6 +570,25 @@ def __init__( | |||||||||||||
| "never carry them, so an entry with a path would " | ||||||||||||||
| "never match." | ||||||||||||||
| ) | ||||||||||||||
| # Only meaningful alongside an OAuth server; without one there | ||||||||||||||
| # is no registration for the list to classify, so a lone | ||||||||||||||
| # `native_redirect_uris` is a config mistake worth surfacing | ||||||||||||||
| # rather than silently ignoring. | ||||||||||||||
| if native_redirect_uris is not None and oauth is None: | ||||||||||||||
| raise InputError( | ||||||||||||||
| reason=( | ||||||||||||||
| "`Application(native_redirect_uris=...)` requires " | ||||||||||||||
| "`oauth=...`: it marks which OAuth clients are " | ||||||||||||||
| "your own first-party native apps, and without an " | ||||||||||||||
| "OAuth provider this application has no OAuth " | ||||||||||||||
| "clients." | ||||||||||||||
| ), | ||||||||||||||
| ) | ||||||||||||||
| self._native_redirect_uris: list[str] = list( | ||||||||||||||
| native_redirect_uris or [] | ||||||||||||||
| ) | ||||||||||||||
| for redirect_uri in self._native_redirect_uris: | ||||||||||||||
| validate_native_redirect_uri(redirect_uri) | ||||||||||||||
| self._title = title or application_name() | ||||||||||||||
| self._description = description | ||||||||||||||
| self._example_prompts = example_prompts or [] | ||||||||||||||
|
|
@@ -780,6 +844,7 @@ def _mount_oauth( | |||||||||||||
| authenticated=self._authenticated, | ||||||||||||||
| claims_changed=self._set_claims_if_exists, | ||||||||||||||
| allowed_origins=self._allowed_origins, | ||||||||||||||
| native_redirect_uris=self._native_redirect_uris, | ||||||||||||||
| ) | ||||||||||||||
| self._oauth_server = oauth_server | ||||||||||||||
| if self._token_verifier is not None: | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| """The set of native redirect URIs an application claims as its own. | ||
|
|
||
| A mobile or desktop app signs in through the same OAuth authorization | ||
| server as an MCP client does, registering itself dynamically (RFC 7591) | ||
| and receiving the authorization code at a redirect URI of its own — | ||
| typically a custom scheme like `myapp://redirect`. Nothing about that | ||
| registration proves who registered, so by default the user is asked to | ||
| vouch for the client on the consent screen before the flow continues. | ||
|
|
||
| Listing a redirect URI here is the application developer stating that | ||
| it belongs to their own first-party app, which lets sign-in skip that | ||
| question. | ||
| """ | ||
|
|
||
| import re | ||
| from reboot.run_environments import running_rbt_dev | ||
| from typing import Sequence | ||
|
|
||
| # Full-string regexes for the native redirect URIs that are trusted | ||
| # automatically under `rbt dev run`. Expo (React Native's toolchain) | ||
| # serves a project from the development machine, so the redirect URI it | ||
| # hands the app carries that machine's address and a port — both of | ||
| # which change with the machine, the network, and the run. There is no | ||
| # stable string for a developer to put in | ||
| # `Application(native_redirect_uris=...)`, so we match the shape | ||
| # instead, and only in local development. | ||
| # | ||
| # Deliberately specific to Expo's scheme rather than covering localhost | ||
| # the way `allowed_origins` does in development. MCP clients register | ||
| # localhost redirect URIs, so trusting localhost here would stop them | ||
| # from showing the consent screen under `rbt dev run` — and a developer | ||
| # who never sees it locally is one who meets it for the first time in | ||
| # production. | ||
| DEV_REDIRECT_URI_REGEXES = (r"exp://[^/]+/--(/.*)?",) | ||
|
|
||
| # URI schemes never accepted, whatever the allow-list says: each one | ||
| # executes or reads local content rather than naming an app to hand an | ||
| # authorization code to. | ||
| _FORBIDDEN_SCHEMES = frozenset(["javascript", "data", "vbscript", "file"]) | ||
|
|
||
| # A URI scheme per RFC 3986: a letter followed by letters, digits, and | ||
| # `+`, `-`, or `.`. | ||
| _SCHEME_REGEX = r"[a-zA-Z][a-zA-Z0-9+.\-]*" | ||
|
|
||
|
|
||
| def validate_native_redirect_uri(redirect_uri: object) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| """Raise `ValueError` if `redirect_uri` is not usable as an entry of | ||
| `Application(native_redirect_uris=...)`.""" | ||
| if not isinstance(redirect_uri, str): | ||
| raise ValueError( | ||
| "`native_redirect_uris` must be a list of strings; got " | ||
| f"entry of type {type(redirect_uri).__name__}" | ||
| ) | ||
|
Comment on lines
+50
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What I'd suggest instead is this shape: def validate_redirect_uris(redirect_uris: list[str], field_name: str) -> None:
for redirect_uri in redirect_uris:
if not isinstance(redirect_uri, str):
raise ValueError(f"`{field_name}` is 💩! Do better!") |
||
| match = re.match(f"({_SCHEME_REGEX}):", redirect_uri) | ||
| if match is None: | ||
| raise ValueError( | ||
| f"`native_redirect_uris` entry {redirect_uri!r} must be a " | ||
| "full URI beginning with a scheme, e.g. " | ||
| "'myapp://redirect' for a custom-scheme app link or " | ||
| "'https://app.example.com/redirect' for a verified " | ||
| "App Link / Universal Link" | ||
| ) | ||
| scheme = match.group(1).lower() | ||
| if scheme in _FORBIDDEN_SCHEMES: | ||
| raise ValueError( | ||
| f"`native_redirect_uris` entry {redirect_uri!r} uses the " | ||
| f"forbidden '{scheme}' scheme" | ||
| ) | ||
| if "*" in redirect_uri: | ||
| raise ValueError( | ||
| f"`native_redirect_uris` entry {redirect_uri!r} must not " | ||
| "contain a wildcard: entries are compared for exact " | ||
| "equality against the `redirect_uri` a client registers, " | ||
| "because that URI is where an authorization code for one " | ||
| "of your users is delivered" | ||
| ) | ||
|
|
||
|
|
||
| def is_first_party_redirect_uri( | ||
| redirect_uri: str, | ||
| *, | ||
| native_redirect_uris: Sequence[str], | ||
| ) -> bool: | ||
| """Whether `redirect_uri` belongs to one of the application's own | ||
| first-party native apps: an exact match against the explicit | ||
| allow-list `native_redirect_uris`, or — under `rbt dev run` — a | ||
| development redirect URI whose shape only a local toolchain | ||
| produces.""" | ||
| if redirect_uri in native_redirect_uris: | ||
| return True | ||
| if running_rbt_dev(): | ||
| return any( | ||
| re.fullmatch(regex, redirect_uri) is not None | ||
| for regex in DEV_REDIRECT_URI_REGEXES | ||
| ) | ||
| return False | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what this parameter does is it suppresses the consent screen for the given URIs. Are we sure that isn't also a feature that's useful with MCP clients? E.g. I could imagine a developer wanting to say "you can skip consent for
chatgpt.com/[...]".If that makes sense, then I imagine we rename this parameter to say what it does rather than what we expect to have put in there, e.g.:
so that developers can write...
For bonus points, we could update the consent screen so that any non-
https://URI shows a subtle info-bubble with a message like...