Skip to content

Add specific message for double-encoded redirects#6704

Merged
kraftbj merged 4 commits intomasterfrom
add/double-encode-message
Mar 1, 2019
Merged

Add specific message for double-encoded redirects#6704
kraftbj merged 4 commits intomasterfrom
add/double-encode-message

Conversation

@johngodley
Copy link
Copy Markdown
Member

@johngodley johngodley commented Mar 21, 2017 •
edited
Loading

Some hosts seem to re-encode parameters in a http=>https redirect, which causes problems with Jetpack auth.

This checks the ‘redirect_to’ query param and if it detects a double-encoded value shows a specific error message, directing the user to a support page for further details.

This PR is an initial step to fixing the problem that is fully described in #6690:

  • Show a specific error related to the double-encoding problem
  • Link to a support page (needs writing) with further details and a list of possible solutions
  • Track stats / sites to see how wide-spread the problem is
  • If significant enough can think about adding Detect and remove double encoded redirects #6690

Note: would appreciate help regards the support page and tracking statistics!

@johngodley
Add specific message for double-encoded redirects
d541cbe 
Some hosts seem to re-encode parameters in a http=>https redirect, which causes problems with Jetpack auth.

This checks the ‘redirect_to’ query param and if it detects a double-encoded value shows a specific message with link to support guide
@johngodley johngodley mentioned this pull request Mar 21, 2017
Closed
@jeherve jeherve added General [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Mar 21, 2017
@samhotchkiss
Copy link
Copy Markdown
Contributor

samhotchkiss commented Mar 28, 2017

@jeherve -- are you able to help with creating this support page?

@samhotchkiss samhotchkiss added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Mar 28, 2017
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Mar 28, 2017

I'm happy to help, but I'm not sure what the list of possible solutions would be. @johngodley @georgeh I assume you already have some kind of predef you use for folks running into this issue with the Google Docs Add-on? Could you add the info you have to the draft I created here?

Thanks!

@johngodley
Copy link
Copy Markdown
Member Author

johngodley commented Mar 30, 2017

Thanks @jeherve - there's no predef as such, but I've filled in a few more details. It's mostly 'fix your server', but hopefully it helps. If it looks ok to you then I think the page is good to go.

If it's possible to track stats for things like this then that would be a good addition. Is this something Jetpack can do?

If not then the PR is ready.

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Mar 30, 2017

If it's possible to track stats for things like this then that would be a good addition. Is this something Jetpack can do?

Yes, it can be done, through Nosara Tracks. Jetpack includes a library making it easier to track actions all across the plugin:
https://github.com/Automattic/jetpack/blob/master/_inc/lib/tracks/class.tracks-client.php

@dereksmart could probably tell you more if needed.

@johngodley
Track double-encode error event
45c204d 
Will give an indication of how widespread this is, and hopefully help with support
@johngodley
Copy link
Copy Markdown
Member Author

johngodley commented Mar 31, 2017

That's perfect, added! Event should track under jetpack_error_double_encode

@johngodley johngodley added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Mar 31, 2017
@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Oct 25, 2017

Removing the Needs Review label as I can't get tests to run locally now. Please rebase it so it gets up to date. And forgive me if this was a mistake on my side.

@oskosk oskosk added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Oct 25, 2017
@stale
Copy link
Copy Markdown

stale bot commented Jun 29, 2018

This PR has been marked as stale. This happened because:

  • It has been inactive in the past 3 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@stale stale bot added the [Status] Stale label Jun 29, 2018
@kraftbj kraftbj dismissed a stale review via 8b6530a February 27, 2019 21:03
@kraftbj kraftbj requested a review from a team February 27, 2019 21:03
@stale stale bot removed the [Status] Stale label Feb 27, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Feb 27, 2019

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "record_user_event" function.

cc: @pesieminski

@kraftbj kraftbj added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 27, 2019
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Feb 27, 2019 •
edited
Loading

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 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 2d4e84d

zinigor
zinigor previously requested changes Mar 1, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Besides one minor spacing problem, and potential GDPR compliance issues I have no problems with this, thank you!

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 1, 2019
@kraftbj kraftbj merged commit 3cedf42 into master Mar 1, 2019
@kraftbj kraftbj deleted the add/double-encode-message branch March 1, 2019 18:36
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 1, 2019
@kraftbj kraftbj added this to the 7.2 milestone Mar 1, 2019
kraftbj added a commit that referenced this pull request Mar 26, 2019
@kraftbj
CL #6704
d26e0de
kraftbj added a commit that referenced this pull request Mar 27, 2019
@kraftbj
Add initial 7.2 changelog to master (#11690)
2227bfd 
* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Add CL for #11224

* Add CL for #11426

* Add CL for #11442

* Add testing instructions for #11224

* Add CL for #11451

* Reclassify CL item

* Add testing instructions for #11451

* Add CL for #11486

* Add CL for #11418

* Add CL for #11524

* Add CL and testing instructions for #11449

* Add CL for #11460

* Add CL for #11520 and #11582

* Add CL for #11531

* Add CL #11644

* Add testing instructions for #11644

* Add testing instructions for #11644

* Add CL for #11618

* Uniform changelog lines

* CL #11679

* CL #11661

* CL #11654

* CL #11645

* CL #11643

* CL #11636

* CL #11635 and for other PHPCS commits

* CL #11627

* CL #11626

* CL #11598

* CL #11596

* Remove nested items for shortcopy. I don't believe the detailed list is helpful

* CL #11570

* CL #11569

* CL #11560

* CL #11558

* CL #11555

* CL #6704

* CL #11298

* CL #11324

* CL #11443

* CL #11484

* CL #11516

* CL #11529

* Expand Ads block enhancement CL item
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kraftbj kraftbj kraftbj approved these changes

+1 more reviewer

@zinigor zinigor zinigor left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] WPCOM API General [Status] Needs GDPR Review

Projects

None yet

Milestone

7.2

Development

Successfully merging this pull request may close these issues.

8 participants

@johngodley @samhotchkiss @jeherve @oskosk @matticbot @jetpackbot @kraftbj @zinigor