Skip to content
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

Fix: broken registration_path when SCRIPT_NAME is not empty string #120

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Fix: broken registration_path when SCRIPT_NAME is not empty string #120

merged 1 commit into from
Sep 18, 2024

Conversation

btalbot
Copy link
Contributor

@btalbot btalbot commented Jun 15, 2024

The registration_path must include the SCRIPT_NAME when it exists to allow requests to be seen as properly on_path? and to be routed to the correct rack app. Additionally, the private method #registration_result must not replace the PATH_INFO with #callback_path as OmniAuth::Strategy#callback_path already includes the SCRIPT_PATH leading to the SCRIPT_PATH portion being duplicated when registration completes and is forwarded to the callback endpoint.

Spec tests added to test all combinations of default and optional values of SCRIPT_PATH, OmniAuth::Strategy.path_prefix, and Identity.name.

see #119

The registration_path must include the SCRIPT_NAME when it exists to allow requests to
be seen as properly `on_path?` and to be routed to the correct rack app. Additionally,
the private method `#registration_result` must not replace the PATH_INFO with `#callback_path`
as `OmniAuth::Strategy#callback_path` already includes the SCRIPT_PATH leading to the SCRIPT_PATH
portion being duplicated when registration completes and is forwarded to the callback endpoint.

Spec tests added to test all combinations of default and optional values of SCRIPT_PATH,
OmniAuth::Strategy.path_prefix, and Identity.name.
Copy link
Member

@pboling pboling left a comment

Choose a reason for hiding this comment

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

LGTM

@pboling pboling merged commit e1ec9ce into omniauth:master Sep 18, 2024
@btalbot btalbot deleted the reg-path-script-name branch September 18, 2024 20: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.

2 participants