Allow OAuth redirect_uri to reference current port #12251
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
a65b59c to
f29a462
Compare
lib/streamlit/auth_util.py
Outdated
| def get_redirect_uri(auth_section: AttrDict) -> str: | ||
| """Get the redirect_uri from auth_section - filling in port number if needed.""" | ||
|
|
||
| if "redirect_uri" not in auth_section: | ||
| return None | ||
|
|
||
| redirect_uri = auth_section["redirect_uri"] | ||
|
|
||
| try: | ||
| redirect_uri_parsed = urlparse(redirect_uri) | ||
| except ValueError: | ||
| raise StreamlitAuthError( | ||
| f"Invalid redirect_uri: {redirect_uri}. Please check your configuration." | ||
| ) | ||
|
|
||
| # If redirect_uri hostname ends with ':' | ||
| # (indicating no port number), add the port number | ||
| if redirect_uri_parsed.netloc[-1] == ":": | ||
| redirect_uri_parsed = redirect_uri_parsed._replace( | ||
| netloc=redirect_uri_parsed.netloc + str(config.get_option("server.port")) | ||
| ) | ||
|
|
||
| return redirect_uri_parsed.geturl() |
There was a problem hiding this comment.
The get_redirect_uri function requires a complete Numpydoc-style docstring. The current single-line docstring does not comply with the Python Guide requirement that all user-facing modules have proper top-level docstrings in Numpydoc format. The docstring should include:
- Parameters section documenting the
auth_sectionparameter - Returns section describing the return value type and meaning
- Raises section documenting the potential
StreamlitAuthErrorexception
Replace the current docstring with a properly formatted multi-line Numpydoc docstring containing these required sections.
| def get_redirect_uri(auth_section: AttrDict) -> str: | |
| """Get the redirect_uri from auth_section - filling in port number if needed.""" | |
| if "redirect_uri" not in auth_section: | |
| return None | |
| redirect_uri = auth_section["redirect_uri"] | |
| try: | |
| redirect_uri_parsed = urlparse(redirect_uri) | |
| except ValueError: | |
| raise StreamlitAuthError( | |
| f"Invalid redirect_uri: {redirect_uri}. Please check your configuration." | |
| ) | |
| # If redirect_uri hostname ends with ':' | |
| # (indicating no port number), add the port number | |
| if redirect_uri_parsed.netloc[-1] == ":": | |
| redirect_uri_parsed = redirect_uri_parsed._replace( | |
| netloc=redirect_uri_parsed.netloc + str(config.get_option("server.port")) | |
| ) | |
| return redirect_uri_parsed.geturl() | |
| def get_redirect_uri(auth_section: AttrDict) -> str: | |
| """Get the redirect_uri from auth_section - filling in port number if needed. | |
| This function extracts the redirect_uri from the provided auth_section. | |
| If the redirect_uri's hostname ends with a colon (indicating no port number), | |
| the server's port number will be appended. | |
| Parameters | |
| ---------- | |
| auth_section : AttrDict | |
| The authentication configuration section containing redirect_uri. | |
| Returns | |
| ------- | |
| str or None | |
| The processed redirect URI with port number if needed, or None if | |
| no redirect_uri is found in auth_section. | |
| Raises | |
| ------ | |
| StreamlitAuthError | |
| If the redirect_uri is invalid and cannot be parsed. | |
| """ | |
| if "redirect_uri" not in auth_section: | |
| return None | |
| redirect_uri = auth_section["redirect_uri"] | |
| try: | |
| redirect_uri_parsed = urlparse(redirect_uri) | |
| except ValueError: | |
| raise StreamlitAuthError( | |
| f"Invalid redirect_uri: {redirect_uri}. Please check your configuration." | |
| ) | |
| # If redirect_uri hostname ends with ':' | |
| # (indicating no port number), add the port number | |
| if redirect_uri_parsed.netloc[-1] == ":": | |
| redirect_uri_parsed = redirect_uri_parsed._replace( | |
| netloc=redirect_uri_parsed.netloc + str(config.get_option("server.port")) | |
| ) | |
| return redirect_uri_parsed.geturl() | |
Spotted by Diamond (based on custom rule: Python Guide)
Is this helpful? React 👍 or 👎 to let us know.
lib/streamlit/auth_util.py
Outdated
| if secrets_singleton.load_if_toml_exists(): | ||
| auth_section = cast("AttrDict", secrets_singleton.get("auth")) | ||
|
|
||
| return auth_section | ||
|
|
||
|
|
||
| def get_redirect_uri(auth_section: AttrDict) -> str: |
There was a problem hiding this comment.
The return type annotation for get_redirect_uri is incorrect. The function returns None when 'redirect_uri' is not present in auth_section, but the return type is annotated as str. The return type should be str | None or Optional[str] to accurately reflect that the function can return either a string or None, as required by Python typing standards.
| def get_redirect_uri(auth_section: AttrDict) -> str: | |
| def get_redirect_uri(auth_section: AttrDict) -> Optional[str]: |
Spotted by Diamond (based on custom rule: Python Guide)
Is this helpful? React 👍 or 👎 to let us know.
lib/streamlit/auth_util.py
Outdated
|
|
||
| # If redirect_uri hostname ends with ':' | ||
| # (indicating no port number), add the port number | ||
| if redirect_uri_parsed.netloc[-1] == ":": |
There was a problem hiding this comment.
Consider adding a check to ensure netloc is not empty before accessing its last character. If netloc is empty, the expression netloc[-1] would raise an IndexError. A simple guard condition like if redirect_uri_parsed.netloc and redirect_uri_parsed.netloc[-1] == ":" would prevent this potential issue.
| if redirect_uri_parsed.netloc[-1] == ":": | |
| if redirect_uri_parsed.netloc and redirect_uri_parsed.netloc[-1] == ":": |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
My original idea was to just have a default redirect_uri value of http://localhost:port/oauth2Callback, but the current approach is more flexible, as it also enables more complex situations like using a virtual machine or with some other domain name being whitelisted for use at OAuth provider that is locally mapped to localhost. It should still be relatively safe for existing users, as ':' is a special symbol in URI reserved for specifying the port. This being said, it's not hard to change it to more explicit templating, like to expect ':port' instead of just ':' if you want to be more careful |
f29a462 to
aacea1a
Compare
| def test_get_redirect_uri_verbatim(self): | ||
| """Test get_redirect_uri returns the existing redirect_uri when present.""" | ||
| auth_section = AttrDict({"redirect_uri": "https://example.com/callback"}) | ||
| result = get_redirect_uri(auth_section) | ||
| assert result == "https://example.com/callback" | ||
|
|
||
| @patch( | ||
| "streamlit.auth_util.config", | ||
| MagicMock( | ||
| get_option=MagicMock(return_value=8502), | ||
| ), | ||
| ) | ||
| def test_get_redirect_uri_with_port_added(self): | ||
| """Test get_redirect_uri returns default value when redirect_uri is missing.""" | ||
| auth_section = AttrDict({"redirect_uri": "http://localhost:/oauth2callback"}) | ||
| result = get_redirect_uri(auth_section) | ||
| assert result == "http://localhost:8502/oauth2callback" |
There was a problem hiding this comment.
The test methods test_get_redirect_uri_verbatim and test_get_redirect_uri_with_port_added are missing return type annotations. According to the Python Unit Test Guide, new tests should be fully annotated with types. Add -> None return type annotations to both test methods.
| def test_get_redirect_uri_verbatim(self): | |
| """Test get_redirect_uri returns the existing redirect_uri when present.""" | |
| auth_section = AttrDict({"redirect_uri": "https://example.com/callback"}) | |
| result = get_redirect_uri(auth_section) | |
| assert result == "https://example.com/callback" | |
| @patch( | |
| "streamlit.auth_util.config", | |
| MagicMock( | |
| get_option=MagicMock(return_value=8502), | |
| ), | |
| ) | |
| def test_get_redirect_uri_with_port_added(self): | |
| """Test get_redirect_uri returns default value when redirect_uri is missing.""" | |
| auth_section = AttrDict({"redirect_uri": "http://localhost:/oauth2callback"}) | |
| result = get_redirect_uri(auth_section) | |
| assert result == "http://localhost:8502/oauth2callback" | |
| def test_get_redirect_uri_verbatim(self) -> None: | |
| """Test get_redirect_uri returns the existing redirect_uri when present.""" | |
| auth_section = AttrDict({"redirect_uri": "https://example.com/callback"}) | |
| result = get_redirect_uri(auth_section) | |
| assert result == "https://example.com/callback" | |
| @patch( | |
| "streamlit.auth_util.config", | |
| MagicMock( | |
| get_option=MagicMock(return_value=8502), | |
| ), | |
| ) | |
| def test_get_redirect_uri_with_port_added(self) -> None: | |
| """Test get_redirect_uri returns default value when redirect_uri is missing.""" | |
| auth_section = AttrDict({"redirect_uri": "http://localhost:/oauth2callback"}) | |
| result = get_redirect_uri(auth_section) | |
| assert result == "http://localhost:8502/oauth2callback" | |
Spotted by Diamond (based on custom rule: Python Unit Tests)
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Pull Request Overview
This PR implements functionality to allow OAuth redirect_uri configuration to dynamically reference the current port when the hostname ends with a colon. This enables multiple Streamlit applications to run on the same host using different ports while sharing the same secrets.toml configuration file.
- Added a new
get_redirect_urifunction that processes redirect URIs and appends the current port when needed - Updated OAuth client creation and origin detection to use the new function instead of direct configuration access
- Added comprehensive unit tests covering both standard redirect URIs and dynamic port insertion
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/streamlit/auth_util.py | Added get_redirect_uri function to handle dynamic port insertion for redirect URIs ending with colon |
| lib/streamlit/web/server/oauth_authlib_routes.py | Updated OAuth client creation and origin detection to use new get_redirect_uri function |
| lib/tests/streamlit/auth_util_test.py | Added unit tests for the new redirect URI functionality covering both standard and dynamic port scenarios |
| ), | ||
| ) | ||
| def test_get_redirect_uri_with_port_added(self): | ||
| """Test get_redirect_uri returns default value when redirect_uri is missing.""" |
There was a problem hiding this comment.
The docstring is incorrect. This test is checking port addition functionality, not handling missing redirect_uri. It should be: 'Test get_redirect_uri adds current port when hostname ends with colon.'
| """Test get_redirect_uri returns default value when redirect_uri is missing.""" | |
| """Test get_redirect_uri adds current port when hostname ends with colon.""" |
lib/streamlit/auth_util.py
Outdated
|
|
||
| # If redirect_uri hostname ends with ':' | ||
| # (indicating no port number), add the port number | ||
| if redirect_uri_parsed.netloc[-1] == ":": |
There was a problem hiding this comment.
This code will raise an IndexError if netloc is an empty string. Add a check to ensure netloc is not empty before accessing its last character.
| if redirect_uri_parsed.netloc[-1] == ":": | |
| if redirect_uri_parsed.netloc and redirect_uri_parsed.netloc[-1] == ":": |
| def get_redirect_uri(auth_section: AttrDict) -> str | None: | ||
| """Get the redirect_uri from auth_section - filling in port number if needed.""" | ||
|
|
There was a problem hiding this comment.
The get_redirect_uri function lacks a proper docstring. According to Python Guide docstring requirements, all functions must use Numpydoc style docstrings with triple quotes. The current single-line comment should be replaced with a comprehensive docstring including Parameters, Returns, and other relevant sections as specified by the Numpydoc format.
| def get_redirect_uri(auth_section: AttrDict) -> str | None: | |
| """Get the redirect_uri from auth_section - filling in port number if needed.""" | |
| def get_redirect_uri(auth_section: AttrDict) -> str | None: | |
| """Get the redirect_uri from auth_section, filling in port number if needed. | |
| Parameters | |
| ---------- | |
| auth_section : AttrDict | |
| The authentication configuration section containing redirect URI information. | |
| Returns | |
| ------- | |
| str or None | |
| The processed redirect URI with port number filled in if needed, or None if | |
| no redirect URI is configured. | |
| """ | |
Spotted by Diamond (based on custom rule: Python Guide)
Is this helpful? React 👍 or 👎 to let us know.
|
That's a great idea, agree that having this would be handy. But I feel like having a trailing colon is a bit too implicit. I think we should do this in a way where if you have [auth]
redirect_uri = "http://localhost:{port}/oauth2callback"I don't think we have any precedence for the curly bracket syntax in Streamlit, but I think it rhymes well with f-strings in Python. What do you think? |
429bd7e to
b8739cf
Compare
|
colon was the minimum viable option. Agree that something more explicit is probably better :) |
SummaryThis PR adds support for a Changes:
Related issue: #12249 Code QualityThe implementation is clean, focused, and follows existing patterns in the codebase: Positives:
Minor observations:
Test CoverageExisting tests:
Tests follow best practices:
Suggested additional test coverage:
def test_get_redirect_uri_returns_none_when_not_present(self):
"""Test get_redirect_uri returns None when redirect_uri is not in auth_section."""
auth_section = AttrDict({"some_other_key": "value"})
result = get_redirect_uri(auth_section)
assert result is None
Backwards CompatibilityFully backwards compatible ✅
Security & RiskNo security concerns identified ✅
Risk assessment: Low
Recommendations
VerdictAPPROVED: This is a well-implemented, backwards-compatible feature that solves a real use case for developers running multiple Streamlit dashboards locally. The code follows existing patterns, has reasonable test coverage, and introduces no security risks. The minor suggestions above are optional improvements and do not block merging. This is an automated AI review. Please verify the feedback and use your judgment. |
b8739cf to
31984ae
Compare
|
A small callout: we are currently in the process of migrating the server from Tornado to Starlette+Uvicorn. 1.53 will come with optional experimental Starlette support, and we likely will fully migrate to Starlette next quarter. The annoying part here is that for aspects impacting the server itself - |
This is something AI can likely do pretty well. Possibly you can have it do all the commits from a period in a batch instead of individually, as it might be faster to both ask for and to review (assuming the individual changes are not too big) |
|
Yep, I agree that this is easy to do with AI. I think one issue is that we don't have end-to-end (e2e) tests for these changes, which makes it hard to actually detect the relevant changes. I know this is quite hard to properly cover e2e, but is there any way to add some coverage similar to how it's done in https://github.com/streamlit/streamlit/blob/develop/e2e_playwright/auth.py and https://github.com/streamlit/streamlit/blob/develop/e2e_playwright/auth_test.py |
Describe your changes
Changed functionality so that if redirect_uri hostname part ends with a colon, it adds the current running port after it.
This allows multiple streamlit dashboards to run on the same host and share a secrets.toml file - which is especially useful in the local development environment, but might also have uses in some deployment situations
GitHub Issue Link (if applicable)
#12249
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.