Fix doubled base URL in survey "Go Back" navigation - #398
Merged
Conversation
Pressing "Go Back" on a survey step could navigate to a URL with the site origin repeated, for example: https://www.openstack.org + https://www.openstack.org/user-survey/... which the browser then resolves as a path, and the request fails. getPreviousStepUrl() built its URL as: Controller::join_links( Director::absoluteBaseURL(), Controller::curr()->Link(), ... ) Controller::join_links() concatenates its arguments and does not detect that a later argument is already an absolute URL, so this is only correct while Link() returns a relative path. Link() is built on Director::baseURL(), which returns Director.alternate_base_url when one is configured -- as SS_BASE_URL does. Setting SS_BASE_URL to a full origin, which is reasonable when the site sits behind a reverse proxy, makes Link() absolute and the origin is then emitted twice. Use Director::absoluteURL() around the joined relative path instead. It returns a URL unchanged when it already begins with http and prepends the base otherwise, so the result is correct whether Link() is relative or absolute, and stays correct if that configuration changes again. Applied to all four call sites that prepended the base by hand: the three getPreviousStepUrl() implementations behind the "Go Back" button, and the server-side redirect in SurveyPage when a requested step is not allowed. EntitySurveyTeamMemberEmailSenderService also calls absoluteBaseURL(), but composes it with Director::makeRelative(), which strips any base first. It is unaffected and left unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uz3HpgshDPXb5GL7cdYZJh
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSurvey redirects and previous-step links now join route paths before resolving them with ChangesSurvey URL resolution
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to Survey back-navigation links and disallowed-step redirects now avoid duplicating the configured site origin when controller links are already absolute. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
smarcet
pushed a commit
that referenced
this pull request
Sep 8, 2026
Pressing "Go Back" on a survey step could navigate to a URL with the site origin repeated, for example: https://www.openstack.org + https://www.openstack.org/user-survey/... which the browser then resolves as a path, and the request fails. getPreviousStepUrl() built its URL as: Controller::join_links( Director::absoluteBaseURL(), Controller::curr()->Link(), ... ) Controller::join_links() concatenates its arguments and does not detect that a later argument is already an absolute URL, so this is only correct while Link() returns a relative path. Link() is built on Director::baseURL(), which returns Director.alternate_base_url when one is configured -- as SS_BASE_URL does. Setting SS_BASE_URL to a full origin, which is reasonable when the site sits behind a reverse proxy, makes Link() absolute and the origin is then emitted twice. Use Director::absoluteURL() around the joined relative path instead. It returns a URL unchanged when it already begins with http and prepends the base otherwise, so the result is correct whether Link() is relative or absolute, and stays correct if that configuration changes again. Applied to all four call sites that prepended the base by hand: the three getPreviousStepUrl() implementations behind the "Go Back" button, and the server-side redirect in SurveyPage when a requested step is not allowed. EntitySurveyTeamMemberEmailSenderService also calls absoluteBaseURL(), but composes it with Director::makeRelative(), which strips any base first. It is unaffected and left unchanged. Claude-Session: https://claude.ai/code/session_01Uz3HpgshDPXb5GL7cdYZJh Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://tipit.avaza.com/project/view/188885?view=List#!task=5037004
Problem
Reported on
/user-survey/survey-2026/your-openstack-deployments/edit/23400/deployment-decisions: pressing Go Back navigates to a URL with the origin repeated —— which the browser resolves as a path, and the request fails.
Cause
getPreviousStepUrl()built the URL as:Controller::join_links()concatenates its arguments and normalises slashes. It does not detect that a later argument is already an absolute URL, so this is correct only whileLink()returns a relative path.Behind the Netlify proxy,
Link()is absolute. The application's origin islegacy.openstack.org;www.openstack.orgis served by Netlify, which proxies the legacy path prefixes —/user-survey/*among them — to that origin. For those requests_ss_environment.phptakes this branch:BASE_URLis documented by the framework as a full URL to the webroot (framework/core/Constants.php:11), andDirector::baseURL()returns it verbatim with a trailing slash (framework/control/Director.php:559).SiteTree::Link()is built on top of it (cms/code/model/SiteTree.php:469), so in that mode every page link is absolute — not only the survey ones. Any caller prepending the base by hand then emits the origin twice.Confirmed live on
https://www.openstack.org/user-survey/survey-2026/your-organization:data-prev-urlon Go Backhttps://www.openstack.org/https://www.openstack.org/user-survey/survey-2026/about-you<form action>https://www.openstack.org/user-survey/survey-2026/SurveyStepFormThe absolute form action is the tell:
Controller::curr()->Link()is absolute, which is exactly the precondition the old code got wrong.A request that reaches the origin without the
X-From: netlifyheader takes the other branch —BASE_URLis auto-derived,Link()is relative, and the old code produced a correct URL. That is why the bug only shows up through the proxy.The result is written to the button's
data-prev-url, andsurvey_builder/js/survey.controller.jsassigns it straight towindow.location, so the bad URL is what the browser navigates to.Fix
Wrap the joined path in
Director::absoluteURL()instead of prependingDirector::absoluteBaseURL().Director::absoluteURL()returns a URL unchanged when it already begins withhttpand prepends the base otherwise (framework/control/Director.php:461), so it is idempotent — correct in both configurations above, and it stays correct if either changes.Applied to all four call sites that prepended the base by hand:
builders/EntitySurveyRegularStepTemplateUIBuilder.phpbuilders/SurveyAbstractStepTemplateUIBuilder.phpbuilders/SurveyReviewStepTemplateUIBuilder.phpSurveyPage.phpNote the bug is not limited to the page it was reported on — through the proxy, Go Back is affected on every survey step, not just entity sub-steps.
These four are the only affected sites. Every other caller of
absoluteBaseURL()composes it withDirector::makeRelative(),RelativeLink(),SS_HTTPRequest::getURL()or a string literal — all base-free, so none of them can double the origin.EntitySurveyTeamMemberEmailSenderServiceis in that group and is deliberately left unchanged; it needs an absolute URL for email.Verification
php -lpasses on all four files.The trigger is confirmed against production (table above) rather than inferred. Still to be checked by hand before merge:
SurveyPage.php, which is a server-side redirect rather than a JavaScript navigation and so exercises a different path.Summary by CodeRabbit