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

Pass through the from parameter when connecting an already connected Jetpack #12351

Merged
merged 3 commits into from
May 15, 2019

Conversation

justinshreve
Copy link
Contributor

If a user tries to connect Jetpack when the are already connected, they are redirected back to the onboarding flow in Calypso. See #3788.

Currently, if a from parameter is supplied (like woocommerce-setup-wizard), this is lost during the redirect. We are going to start using this parameter in Calypso to show some different copy/design elements during the flow, so passing this along will help users who hit this edge case.

To Test:

@justinshreve justinshreve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. Connect Flow Connection banners, buttons, ... labels May 13, 2019
@justinshreve justinshreve requested a review from a team as a code owner May 13, 2019 18:37
@justinshreve justinshreve self-assigned this May 13, 2019
@jetpackbot
Copy link

jetpackbot commented May 13, 2019

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 20d3e9e

class.jetpack.php Outdated Show resolved Hide resolved
@kraftbj kraftbj added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 13, 2019
class.jetpack.php Outdated Show resolved Hide resolved
class.jetpack.php Outdated Show resolved Hide resolved
@justinshreve
Copy link
Contributor Author

Thanks for the reviews! I've moved the line inside the connect_url_redirect block, and added a @todo for a whitelist.

@justinshreve justinshreve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels May 14, 2019
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@tyxla tyxla added this to the 7.4 milestone May 14, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This should work. I only have a minor request until we have a whitelist of parameters.

class.jetpack.php Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 15, 2019
@jeherve
Copy link
Member

jeherve commented May 15, 2019

Unblocking for now; as @tyxla mentioned it can be addressed in a future PR.

@justinshreve You can merge whenever you're ready :)

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels May 15, 2019
jeherve added a commit that referenced this pull request May 15, 2019
Follow-up from #12351

Since one can currently pass any "from" parameter when building that URL, let's sanitize that value.
@justinshreve justinshreve merged commit ee71e31 into master May 15, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 15, 2019
@justinshreve justinshreve deleted the fix/connect-from-redirect branch May 15, 2019 14:33
jeherve added a commit that referenced this pull request May 21, 2019
…2380)

* Connect flow: sanitize "from" parameter when building connect url

Follow-up from #12351

Since one can currently pass any "from" parameter when building that URL, let's sanitize that value.

* Connect URL: allow the use of periods in from parameter

See #12380 (comment)

Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* Connect URL: escape full URL instead of sanitizing one parameter


Co-authored-by: Eric Binnion <ericbinnion@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Connect Flow Connection banners, buttons, ... [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants