Closed Bug 838332 Opened 7 years ago Closed 5 years ago

t.co links use JS redirection instead of a HTTP 301

Categories

(Firefox for Android :: General, defect, P5)

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
fennec Nightly+ ---

People

(Reporter: aaronmt, Assigned: mfinkle)

References

Details

(Keywords: feature)

Attachments

(4 files, 2 obsolete files)

I think this is an old issue, but now that we're all talking about thumbnails on about:home perhaps this can be addressed? If I open a URL via an intent (t.co/bi.tly/goo.gl), two thumbnail spots are taken; one for the shortener and the other for the actual link
We could consider not saving the shortner in history
We shouldn't be saving redirects already. I wonder how they're doing this...
IIRC, When Brad was playing with the t.co redirector in Java, we saw the redirector using client-side JS to do the redirect.
$ curl -LA "Mozilla/5.0 (Android; Mobile; rv:21.0) Gecko/21.0 Firefox/21.0" http://t.co/ud7Zrk5U

<noscript><META http-equiv="refresh" content="0;URL=http://bit.ly/WPpUlR"></noscript><script>location.replace("http:\/\/bit.ly\/WPpUlR")</script>

$ curl -LA "Mozilla/5.0 (Linux; Android 4.0.4; Galaxy Nexus Build/IMM76B) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.133 Mobile Safari/535.19" http://t.co/ud7Zrk5U

<noscript><META http-equiv="refresh" content="0;URL=http://bit.ly/WPpUlR"></noscript><script>location.replace("http:\/\/bit.ly\/WPpUlR")</script>

$ curl -LA "Mozilla/5.0 (iPhone; U; CPU like Mac OS X; en) AppleWebKit/420+ (KHTML, like Gecko) Version/3.0 Mobile/1A543 Safari/419.3" http://t.co/ud7Zrk5U

<noscript><META http-equiv="refresh" content="0;URL=http://bit.ly/WPpUlR"></noscript><script>location.replace("http:\/\/bit.ly\/WPpUlR")</script>
But...

curl -LA "Mozilla/5.0 (Android; Mobile; rv:21.0)" http://t.co/ud7Zrk5U

will redirect
FWIW, Desktop adds these history entries too.
Attached patch patch (obsolete) — Splinter Review
Not sure we want this approach. The patch changes the UA for t.co links. The modified UA causes t.co to do a real redirect.

I'm sad about this approach, but I am happy about how much faster t.co linked pages load.
Assignee: nobody → mark.finkle
Attachment #711473 - Flags: review?(bnicholson)
CC'ing Lawrence, this sounds like something that can be solved or reasoned through emails with Twitter instead of breaking their expected functionality for analytics or other purpose.
Comment on attachment 711473 [details] [diff] [review]
patch

I don't like doing this because this looks like the expected behavior. I think if a site is implemented to act a certain way, we're obligated as a web browser to respect that implementation, even if we don't like it. I'd be much more comfortable reaching out to Twitter so they can change it (or at least defend the behavior).
Attachment #711473 - Flags: review?(bnicholson) → review-
Let me summarize this bug to ensure that I'm clear on the state of affairs.

1. This redirect issue is present on Firefox Desktop, Firefox for Android, and the Nexus Stock Browser. So, this issue is not limited to Firefox. 
2. Dropping the trailing "Gecko/21.0 Firefox/21.0" from the Firefox for Android UA redirects as expected.
3. From the content of comment 4 it appears that the Twitter t.co redirects are actually managed by bitly.

If this is all correct, we'll need to find someone to speak with at bitly. Mozilla has a corporate account with bitly so we should have at least some point of contact.
Nevermind that last part about this being bitly specific. I tested with the Firefox for Android UA using the mzl.la and bit.ly domains and both redirect as expected.
Blocks: twitter.com
Update on this bug: Twitter is using the manual redirect to be able to send the referrer along to the target webpage. They want the credit for sending the traffic. A 301 redirect from HTTPS -> HTTP won't send the referrer because it's a security leak. You don't want to send that URL and potentially leak passwords or other data.

That's the general case. In the case of t.co there is no security information in the URL, but forcing a 301 would undermine twitter's traffic generation stats.
Attached patch patch v2 (obsolete) — Splinter Review
This patch forces the redirect to send a referrer along. I used the Remote Web Console to verify that the referrer was being sent to the target web page.

So this patch uses a 301 (faster and does not show up in history) but also sends a referrer (Twitter gets the traffic credit).

Technically this patch does not leak any HTTPS data to a HTTP request since the t.co link is HTTP. The only real downside to this patch is it a one-off hack in our code.
Attachment #711473 - Attachment is obsolete: true
Attachment #733741 - Flags: review?(bnicholson)
Attached image Simple 1-level redirect
Here is a simple 1-level redirect. Not the target page has the referrer.
Attached image 2-level redirect
Showing two redirecting shorteners in action
Notice that the initial t.co request has no referrer, but because it's a manual (normal HTML page) redirect, the referrer is added to the next request and passed to the target page.
(In reply to Mark Finkle (:mfinkle) from comment #12)
> Update on this bug: Twitter is using the manual redirect to be able to send
> the referrer along to the target webpage. They want the credit for sending
> the traffic. A 301 redirect from HTTPS -> HTTP won't send the referrer
> because it's a security leak. You don't want to send that URL and
> potentially leak passwords or other data.
> 
> That's the general case. In the case of t.co there is no security
> information in the URL, but forcing a 301 would undermine twitter's traffic
> generation stats.

Playing around with twitter, I'm trying to understand what the issue is. If they're shortening HTTP and HTTPS links, the obvious solution for them would be to use http://t.co and https://t.co, respectively. And in fact, they do just that: see https://twitter.com/thebnichTest. The cnn.com link, which goes to http://www.cnn.com, points to http://t.co/qqRI3V8hmW; the google.com link, which goes to https://www.google.com, points to https://t.co/ARrfUVl0cd. That means there's no HTTPS->HTTP protocol change, right? Is it just the case that HTTPS->X in general results in dropping the referer, even if X is also HTTPS?
Comment on attachment 733741 [details] [diff] [review]
patch v2

I hate the fact we need a patch like this.
Attachment #733741 - Flags: review?(bnicholson)
Scripts using location.replace() are supposed to not create a history entry for the page you're on. Can't we add some logic somewhere that also suppresses creating a thumbnail for pages that are location.replace()'d?
Is this something we want to consider worth tracking for new-about-home's top-sites additions now?
tracking-fennec: --- → ?
Status: NEW → ASSIGNED
Comment on attachment 733741 [details] [diff] [review]
patch v2

Review of attachment 733741 [details] [diff] [review]:
-----------------------------------------------------------------

Can we do something more generic, like only recording it in history if we spend more than 3s on the page?
tracking-fennec: ? → +
I want to WONTFIX this
filter on [mass-p5]
Priority: -- → P5
Depends on: 934702
Summary: URL shorteners alongside their associative URL's get placed as top-site thumbnails → URL shorteners alongside their associated URLs get placed as top-site thumbnails
Unbitrotted
Attachment #733741 - Attachment is obsolete: true
Besides being a little faster, this patch also keeps t.co redirects from appearing in history.
Comment on attachment 8524256 [details] [diff] [review]
tco-redirect-hack v0.3

Review of attachment 8524256 [details] [diff] [review]:
-----------------------------------------------------------------

This is gross but yay. If it works, ship it.

::: mobile/android/chrome/content/browser.js
@@ +2983,5 @@
> +      channel.referrer = channel.URI;
> +
> +      // Send a bot-like UA to t.co to get a real redirect. We strip off the
> +      // "Gecko/x.y Firefox/x.y" part
> +      defaultUA = defaultUA.replace(this.TCO_REPLACE, "");

Just return directly.

@@ +3039,5 @@
>    observe: function ua_observe(aSubject, aTopic, aData) {
>      if (aTopic === "DesktopMode:Change") {
>        let args = JSON.parse(aData);
>        let tab = BrowserApp.getTabForId(args.tabId);
> +      if (tab != null) {

just

  if (tab) {

?
Attachment #8524256 - Flags: review+
Note: We could just use an override preference to switch the UA for t.co, but that wouldn't set the channel.referrer correctly.

This should work for just switching the UA:
pref("general.useragent.override.t.co", " Gecko.*# ");

Since we still need to change the referrer, I don't think the override pref approach is better than the given patch.
This bug has morphed into a clone of bug 841388, so I am re-summarizing. We WONTFIXed bug 841388 because we didn't have a fix that kept the correct referrer. This patch looks to do that.

I'll land with a NIGHTLY_BUILD flag wrapped around the UA replacement. If we see good feedback, and no regressions, we can consider removing the flag. 

https://hg.mozilla.org/integration/fx-team/rev/bce2fc00ba0e

Idle thought: I wonder how much of any performance benefit (real or perceived) is due to not saving the t.co link in the database?
Summary: URL shorteners alongside their associated URLs get placed as top-site thumbnails → t.co links use JS redirection instead of a HTTP 301
Dropping the dependency after morph.
No longer depends on: 934702
Hardware: ARM → All
See Also: → 934702
(In reply to Carsten Book [:Tomcat] from comment #29)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1223476&repo=fx-team

Looks like the complex override handler (onRequest) should only return non-null if it's changing the UA. Otherwise, it should return null. Testing a patch now.
Try run looks good now:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1e45766b86cc

M8 on Android 4.0
M16 on Android 2.3
https://hg.mozilla.org/mozilla-central/rev/db10aad51ebc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Note that I intend to remove UserAgentOverrides.jsm - as this has been nightly only for 4 years now, I assume it's safe to remove, once bug 1513574 lands. If it is something we still wish to do, we can use the "Go faster webcompat addon".
You need to log in before you can comment on or make changes to this bug.