Closed
Bug 838332
Opened 11 years ago
Closed 10 years ago
t.co links use JS redirection instead of a HTTP 301
Categories
(Firefox for Android Graveyard :: General, defect, P5)
Tracking
(fennecNightly+)
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
Assignee | ||
Comment 1•11 years ago
|
||
We could consider not saving the shortner in history
Comment 2•11 years ago
|
||
We shouldn't be saving redirects already. I wonder how they're doing this...
Assignee | ||
Comment 3•11 years ago
|
||
IIRC, When Brad was playing with the t.co redirector in Java, we saw the redirector using client-side JS to do the redirect.
Reporter | ||
Comment 4•11 years ago
|
||
$ 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>
Assignee | ||
Comment 5•11 years ago
|
||
But... curl -LA "Mozilla/5.0 (Android; Mobile; rv:21.0)" http://t.co/ud7Zrk5U will redirect
Comment 6•11 years ago
|
||
FWIW, Desktop adds these history entries too.
Assignee | ||
Comment 7•11 years ago
|
||
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)
Reporter | ||
Comment 8•11 years ago
|
||
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 9•11 years ago
|
||
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-
Comment 10•11 years ago
|
||
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.
Comment 11•11 years ago
|
||
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
Assignee | ||
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
Here is a simple 1-level redirect. Not the target page has the referrer.
Assignee | ||
Comment 15•11 years ago
|
||
Showing two redirecting shorteners in action
Assignee | ||
Comment 16•11 years ago
|
||
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.
Comment 17•11 years ago
|
||
(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?
Assignee | ||
Comment 18•11 years ago
|
||
Comment on attachment 733741 [details] [diff] [review] patch v2 I hate the fact we need a patch like this.
Attachment #733741 -
Flags: review?(bnicholson)
Comment 19•11 years ago
|
||
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?
Reporter | ||
Comment 20•11 years ago
|
||
Is this something we want to consider worth tracking for new-about-home's top-sites additions now?
tracking-fennec: --- → ?
Reporter | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Comment 21•11 years ago
|
||
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?
Updated•11 years ago
|
tracking-fennec: ? → +
Assignee | ||
Comment 22•11 years ago
|
||
I want to WONTFIX this
Updated•10 years ago
|
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
Assignee | ||
Comment 25•10 years ago
|
||
Besides being a little faster, this patch also keeps t.co redirects from appearing in history.
Comment 26•10 years ago
|
||
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+
Assignee | ||
Comment 27•10 years ago
|
||
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.
Assignee | ||
Comment 28•10 years ago
|
||
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
Comment 29•10 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1223476&repo=fx-team
Comment 30•10 years ago
|
||
Dropping the dependency after morph.
Assignee | ||
Comment 31•10 years ago
|
||
(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.
Assignee | ||
Comment 32•10 years ago
|
||
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
Assignee | ||
Comment 33•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/db10aad51ebc
https://hg.mozilla.org/mozilla-central/rev/db10aad51ebc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment 35•9 years ago
|
||
this seems to be nightly only https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3136
tracking-fennec: + → Nightly+
Keywords: feature
Comment 36•5 years ago
|
||
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".
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•