-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: use dynamic port allocation for OAuth server #5019
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
Conversation
michaelneale
left a comment
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.
nice one - @frozendevil if you can update to main and push to trigger a build, we can get this in. really nice catch! we probably have other places we make this mistake too
Replace hardcoded port 8020 with OS-assigned dynamic port to prevent conflicts with other applications. The OAuth server now binds to port 0 by default (when no specific port is provided in redirect URL), allowing the OS to assign an available port automatically. Changes: - Modified OAuth flow to bind to dynamic port and use actual assigned port - Updated default redirect URL to not include hardcoded port - Added helper methods to handle dynamic redirect URLs - Maintains backward compatibility for explicit port specifications Resolves port conflicts during OAuth authentication flow. Signed-off-by: Izzy Fraimow <[email protected]>
4f0b3eb to
7f954cf
Compare
|
cool, thanks! The fork is up-to-date now, but I don't have permission to kickoff the workflows |
DOsinga
left a comment
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.
this is obviously a good fix! thanks for that.
that said, if you wanted to make it even sharper, I would the added LLM comments, the code should speak for itself. also I would put the url/port processing in get_authorization_url and not rename that thing.
|
I don't hate all the comments: is nice as I always forget you can do that! nice one! |
Signed-off-by: Izzy Fraimow <[email protected]>
|
Yeah, my reasoning for leaving the comments was that the port-0 behavior is something that lots of folks haven't encountered (or have forgotten). I've cleaned up the other comments though. Re: |
* main: (170 commits) Applied server side call to parse and save recipe (#5022) feat(prompt-library): add Code Documentation Migrator intermediate prompt (#4996) (#5051) Add Messy Column Fixer recipe (#5062) Cleanup temp files (#5081) add openmetadata recipe (#5076) Fix Hacktoberfest Leaderboard (#5080) adding brand guidelines to AGENTS.md (#4887) Fix: Prevent cross-contamination of cache data across analysis modes for `analyze` tool (#5075) fix: remove circular reference (#5018) Introduced a new prompt for content amplification that integrates multi-step workflows using official Goose extensions. Closes Issue #4998 (#5050) Add hint for focus mode when used on file paths for `analyze` tool (#5069) fix: use dynamic port allocation for OAuth server (#5019) Art vandelay: Import & Export (#5053) docs: misc updates for extensions directory (#5035) updating recipe scanner workflows for detecting recipes from forked repos (#5056) feat(prompt-library): add Smart Meeting Assistant advanced prompt (#4998) (#5031) Allow auto focus and typing while chat is initializing (#5043) docs(blog): Add blog for running Goose in containerized envs (#5052) fix: Add WINDOWS_CODESIGN_CERTIFICATE to nightly workflow (#5037) Developer `analyze` tool improvement (#5030) ...
* origin/main: Improve Rust analysis output for `analyze` tool (#5072) Remove duplicate prepare_reply_context call (#5063) install react dev tools in development (#4979) Doc: Added powershell installation link to the guide (#5012) draft of new blog post about automating more automation (#5038) Subagent extension selection behavior fix (#5093) Add dev and alpha environment indicator (#5092) docs: add content carousel (#5086) Applied server side call to parse and save recipe (#5022) feat(prompt-library): add Code Documentation Migrator intermediate prompt (#4996) (#5051) Add Messy Column Fixer recipe (#5062) Cleanup temp files (#5081) add openmetadata recipe (#5076) Fix Hacktoberfest Leaderboard (#5080) adding brand guidelines to AGENTS.md (#4887) Fix: Prevent cross-contamination of cache data across analysis modes for `analyze` tool (#5075) fix: remove circular reference (#5018) Introduced a new prompt for content amplification that integrates multi-step workflows using official Goose extensions. Closes Issue #4998 (#5050) Add hint for focus mode when used on file paths for `analyze` tool (#5069) fix: use dynamic port allocation for OAuth server (#5019)
Summary
Replace hardcoded port 8020 with OS-assigned dynamic port to prevent conflicts with other applications. The OAuth server now binds to port 0 by default (when no specific port is provided in redirect URL), allowing the OS to assign an available port automatically.
Changes:
Resolves port conflicts during OAuth authentication flow.
Type of Change
Testing
Unit tests pass.
Manual test:
python3 -m http.server 8020.The login redirect page opened with a URL of
http://localhost:61601/?code=dcod7de228f79cd0fecbe7b0955105959de8&state=yYAiqVQQl7AeDrlX, indicating it bound to port61601. We received a full response indicating that the subsequence communication with Databricks succeeded.