Skip to content

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

Merged
pboling merged 1 commit into
omniauth:masterfrom
btalbot:reg-path-script-name
Sep 18, 2024
Merged

Fix: broken registration_path when SCRIPT_NAME is not empty string#120
pboling merged 1 commit into
omniauth:masterfrom
btalbot:reg-path-script-name

Conversation

@btalbot

@btalbot btalbot commented Jun 15, 2024

Copy link
Copy Markdown
Contributor

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.

@pboling pboling left a comment

Copy link
Copy Markdown
Member

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