Bug #16259

Redirection on /donate drop the query parameter

Added by sajolida 2018-12-30 18:20:10 . Updated 2019-02-22 08:15:41 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Fundraising
Target version:
Start date:
2018-12-30
Due date:
% Done:

100%

Feature Branch:
web/16259-slash-on-donate
Type of work:
Sysadmin
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

Until recently we’ve used URLs like https://tails.boum.org/donate?r=plans2019. See git grep '?r=' -- "*.*m*". And they worked.

Today I noticed that I now have to add the canonical slash to make them work: https://tails.boum.org/donate/?r=progress. Otherwise the query parameter is dropped.

Which is harder:

  • Fixing the redirections (sysadmins)?
  • Fixing all previous such URLs on our website (I can do that)?

I’m also afraid of doing this same mistake again the future by habit…


Subtasks


Related issues

Related to Tails - Bug #16220: Website's "add trailing slash" trick is partly broken Resolved 2018-12-11

History

#1 Updated by intrigeri 2019-01-02 13:20:30

  • related to Bug #16220: Website's "add trailing slash" trick is partly broken added

#2 Updated by intrigeri 2019-01-02 13:21:50

  • Target version set to Tails_3.12

#3 Updated by intrigeri 2019-01-02 13:22:06

  • Assignee changed from intrigeri to sajolida
  • QA Check set to Info Needed

> Until recently we’ve used URLs like https://tails.boum.org/donate?r=plans2019. See git grep '?r=' -- "*.*m*". And they worked.

> Today I noticed that I now have to add the canonical slash to make them work: https://tails.boum.org/donate/?r=progress. Otherwise the query parameter is dropped.

In order to better understand the problem and evaluate solutions, I’d like to ask: what’s the problem with the query parameter being dropped upon redirection, exactly? Does it break functionality for users, or does it hide useful info in our web access logs, or anything else?

Thanks in advance!

Once I understand the problem better, I’ll handle this together with Bug #16220.

#4 Updated by sajolida 2019-01-04 17:00:14

It hides information from the web access logs but it doesn’t break functionality for users.

I guess it’s been the case since we moved the website so some information for this year’s campaign is lost already and there’s no hurry :)

#5 Updated by sajolida 2019-01-04 17:00:36

  • Assignee changed from sajolida to intrigeri
  • QA Check deleted (Info Needed)

#6 Updated by intrigeri 2019-01-27 11:23:44

  • Target version changed from Tails_3.12 to Tails_3.13

#7 Updated by intrigeri 2019-02-15 13:53:04

  • Assignee changed from intrigeri to sajolida
  • QA Check set to Info Needed

First, allow me take a step back. I think there are two things here:

Query parameter in the browser’s URL bar

My understanding is that displaying the query parameter in the visitor’s URL bar is not a particularly desirable feature: they’re merely an implementation detail of home-made website metrics. If we have to display it (as we always did before), fine; if we don’t have to, I would say UX only gets better. So I don’t think there’s anything to fix on this front: the new setup actually improved things.

Are we in agreement?

Query parameter in web server logs

Requesting https://tails.boum.org/donate?r=plans2019 with curl, which does not follow redirections by default, gives me a redirect to https://tails.boum.org/donate/ and adds a GET /donate?r=plans2019 HTTP/2.0 line in the access log. If I add the --location option to follow redirections, then I get the content of the /donate page, and two lines are added to the access log: GET /donate?r=plans2019 HTTP/2.0 and GET /donate/ HTTP/2.0.

Requesting https://tails.boum.org/donate/?r=plans2019 with curl gives me the content of the /donate page directly, without any redirection involved, and adds a GET /donate/?r=plans2019 HTTP/2.0 line in the access log.

So in both cases, the query parameter is logged => I don’t understand how it “hides information from the web access logs”. But perhaps having redirects involved makes it harder to analyze the logs?

I’m happy to work on this again once I better understand what’s the problem. Currently, my next scheduled sysadmin work session is March 6.

#8 Updated by sajolida 2019-02-18 19:31:21

> implementation detail of home-made website metrics.

For info, all companies doing email marketing use the same technique.
But yes, we’re implementing manually what’s usually done automatically
by mailing list systems.

> If we have to display it (as we always did before), fine; if we don’t have to, I would say UX only gets better.

I’m not sure that displaying a parameter in the URL is an UX issue…

> Are we in agreement?

We also pass this query parameter over to PayPal so that we get it back
on /donate/thanks and /donate/cancel when people return.

That’s the data I wanted to use to confirm or infirm your assumption
that people were not donating through PayPal from Tails.

See 38730560b3.

I understand that if we drop the parameter in a redirection, then it
won’t be passed over to PayPal.

But I’ll have data until November 11 from the old server to check your
assumption.

> So in both cases, the query parameter is logged => I don’t understand how it “hides information from the web access logs”. But perhaps having redirects involved makes it harder to analyze the logs?

Indeed, I see the redirection in the logs:

“GET /donate?r=sidebar HTTP/2.0” 301 393 “https://tails.boum.org/”
“GET /donate/ HTTP/2.0” 200 5520 “https://tails.boum.org/”

#9 Updated by sajolida 2019-02-18 21:01:31

  • Assignee changed from sajolida to intrigeri

#10 Updated by intrigeri 2019-02-19 19:10:57

  • QA Check changed from Info Needed to Dev Needed

> We also pass this query parameter over to PayPal so that we get it back on /donate/thanks and /donate/cancel when people return.
> I understand that if we drop the parameter in a redirection, then it won’t be passed over to PayPal.

Thanks for the clarification, this is a symptom I understand and can work with :) So this change does break functionality for users.

I’ll do my best to fix this because even if we changed all our existing links to add a trailing slash, it makes sense that you’re “afraid of doing this same mistake again the future by habit”. I don’t know yet how easy it’ll be to fix, nor whether the fix will add constraints on your side.

Still, please consider changing such links that are still in use, so we avoid one unnecessary redirection and HTTP round-trip.

>> So in both cases, the query parameter is logged => I don’t understand how it “hides information from the web access logs”. But perhaps having redirects involved makes it harder to analyze the logs?

> Indeed, I see the redirection in the logs:

> “GET /donate?r=sidebar HTTP/2.0” 301 393 “https://tails.boum.org/”
> “GET /donate/ HTTP/2.0” 200 5520 “https://tails.boum.org/”

I understand you’re confirming that the logs have all the info we need (and some more) and none is hidden. Good.

Regarding logs analysis, I assume that we don’t want to consider queries that got answered with a 301 HTTP code and they should be easy to filter out, right?

Note that I think we’ll need to keep the redirection, as long as we point to URLs such as /donate?r= without a trailing slash, otherwise issues like Bug #16220, Bug #16124 and Bug #16219 will come back.

#11 Updated by sajolida 2019-02-21 19:12:06

Here is a branch that fixes all these links. Please have a look.

Sorry it took me some time to understand that it wouldn’t be easy for
you to make our new web server behave like the old one.

> I’ll do my best to fix this because even if we changed all our existing links to add a trailing slash, it makes sense that you’re “afraid of doing this same mistake again the future by habit”. I don’t know yet how easy it’ll be to fix, nor whether the fix will add constraints on your side.

Maybe let’s wait until the need arises again: either because I mess up
on some of such links or because we need this to work better for other
reasons.

> Still, please consider changing such links that are still in use, so we avoid one unnecessary redirection and HTTP round-trip.

Done in web/16259-slash-on-donate.

> Regarding logs analysis, I assume that we don’t want to consider queries that got answered with a 301 HTTP code and they should be easy to filter out, right?

Now yes with internal.git:0de8c30 :)

#12 Updated by sajolida 2019-02-21 19:12:39

  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch set to web/16259-slash-on-donate

#13 Updated by intrigeri 2019-02-22 07:56:07

  • QA Check changed from Ready for QA to Dev Needed

> Here is a branch that fixes all these links. Please have a look.

Looks good, merging! (Will take a while as this will fully rebuild our website.)

> Sorry it took me some time to understand that it wouldn’t be easy for you to make our new web server behave like the old one.

Well, I had not even checked if implementing a solution would be easy until today: until then I was trying to understand what was the problem, which was hard given the original ticket description did not tell me much (“work” vs. “does not work”) and the mistaken info provided on Bug #16259#note-4 got me lost on wrong leads. Instead, I certainly could have skipped trying to understand at all and blindly “[made] our new web server behave like the old one”. I’m afraid I’m very bad at that… which arguably is both a feature and a bug ;) One reason why I wanted to understand the problem is that I was hoping there could be a solution that’s more nginx-native-speak than “try to emulate Apache”: when switching tools, sometimes the best solution with the new tool is not the same as the best solution with the old one (e.g. when we migrate our issues to GitLab, we’ll need to reinvent some workflows instead of trying to make it behave like Redmine).

>> I’ll do my best to fix this because even if we changed all our existing links to add a trailing slash, it makes sense that you’re “afraid of doing this same mistake again the future by habit”. I don’t know yet how easy it’ll be to fix, nor whether the fix will add constraints on your side.

> Maybe let’s wait until the need arises again: either because I mess up on some of such links or because we need this to work better for other reasons.

Today I took a look and it seems it’s actually easy to pass through the query parameters when redirecting. So I’ll give it a try.

#14 Updated by intrigeri 2019-02-22 08:14:40

  • Status changed from Confirmed to Resolved

> Today I took a look and it seems it’s actually easy to pass through the query parameters when redirecting. So I’ll give it a try.

Done, works fine except for browsers with history enabled that had already followed the link (the 301 redirection is a permanent ones, that browsers remember and apply blindly without querying the web server again).

#15 Updated by intrigeri 2019-02-22 08:15:41

  • Assignee deleted (intrigeri)
  • % Done changed from 0 to 100
  • QA Check deleted (Dev Needed)