Broken Link - Githubissues

jawman1 commented 5 years ago

Describe the problem

I just installed firefox. I opened a new tab and moved to the bookmarks selection. I tapped the Firefox: customize with addons and it took me to a page that does not exist. This seems to be on the default home page.

What happened?

The page it directed me to seemed to be where maybe a soecific add-on had been. I dont know where it was supposed to take me.

What did you expect to happen?

Anything else we should know?

https://addons.mozilla.org/android?utm_source=inproduct&utm_medium=default-bookmarks&utm_campaign=mobileandroid

jvillalobos commented 5 years ago

It looks like there should be an / after android and before the params. @muffinresearch, is this necessary for the link to work?

I just installed firefox.

@jawman1 This is on a mobile device, right?

jawman1 commented 5 years ago

On mobile yes, sorry. Android: Galaxy S10

On Wed, May 22, 2019 at 14:56 Jorge Villalobos notifications@github.com wrote:

It looks like there should be an / after android and before the params. @muffinresearch https://github.com/muffinresearch, is this necessary for the link to work?

I just installed firefox.

@jawman1 https://github.com/jawman1 This is on a mobile device, right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla/addons/issues/13207?email_source=notifications&email_token=AMEG5CN5QKO4EE26KAUPTCTPWWXRTA5CNFSM4HOXFJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWAKHZA#issuecomment-494969828, or mute the thread https://github.com/notifications/unsubscribe-auth/AMEG5CO5A6KLAPGNL7XOXJ3PWWXRTANCNFSM4HOXFJFA .

aplaice commented 5 years ago

Some further information:

The above-mentioned link is (has been) a default bookmark in all versions of Firefox for Android ("normal", beta and nightly). According to archive.is it had worked in the past.

Interestingly, the link is a default bookmark even with a newly reinstalled nightly, and indeed it's present in the android source. Just in case, I've filed a bug on bugzilla.

willdurand commented 5 years ago

It looks like there should be an / after android and before the params. @muffinresearch, is this necessary for the link to work?

With a trailing slash, the prefix middleware builds a correct URL but without it, it redirects to /<locale>/firefox/android.

muffinresearch commented 5 years ago

It looks like there should be an / after android and before the params. @muffinresearch, is this necessary for the link to work?

This does seem like it's a case that should just work, though IIRC our entire URL scheme typically does have trailing slashes.

willdurand commented 5 years ago

With a trailing slash, the prefix middleware builds a correct URL but without it, it redirects to /<locale>/firefox/android.

I also noticed that /android is redirected correctly, but /android?something=here is not. There is a test case for that, so I don't know how to change the behavior of the prefix middleware.

muffinresearch commented 5 years ago

I also noticed that /android is redirected correctly, but /android?something=here is not. There is a test case for that, so I don't know how to change the behavior of the prefix middleware.

I suspect if you add a test similar to that with a clientApp in the part before the ? it would fail.

willdurand commented 5 years ago

I added this test case to reproduce this bug, and it failed:

  it('should not mangle a query string for a redirect', () => {
    const fakeReq = {
      originalUrl: '/android?test=1&bar=2',
      headers: {},
    };
    prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig });
    sinon.assert.calledWith(
      fakeRes.redirect,
      301,
      '/en-US/android/?test=1&bar=2',
    );
  });

● /Users/williamdurand/projects/mozilla/addons-frontend/tests/unit/core/middleware/test_prefixMiddleware.js › should not mangle a query string for a redirect

AssertError: expected redirect to be called with arguments
301
/en-US/firefox/android?test=1&bar=2 /en-US/android/?test=1&bar=2

  249 |     };
  250 |     prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig });
> 251 |     sinon.assert.calledWith(
      |                  ^
  252 |       fakeRes.redirect,
  253 |       301,
  254 |       '/en-US/android/?test=1&bar=2',

  at Object.fail (node_modules/sinon/lib/sinon/assert.js:106:21)
  at failAssertion (node_modules/sinon/lib/sinon/assert.js:65:16)
  at Object.assert.(anonymous function) [as calledWith] (node_modules/sinon/lib/sinon/assert.js:91:13)
  at Object.calledWith (tests/unit/core/middleware/test_prefixMiddleware.js:251:18)

muffinresearch commented 5 years ago

I would imagine a fix for this would actually be for this url to not result in a redirect but to be passed straight-through as the slashes middleware should add the slash for it and that middleware follows this one.

See also https://addons.mozilla.org/firefox?foo=1 which would have the same issue.

willdurand commented 5 years ago

How could we detect that? If URL contains /${clientApp} (only?), then we early return so that the slashes middleware handles it?

muffinresearch commented 5 years ago

I think the core of the problem is that the places checking for clientApp are including the query params because they're not split out independently.

Redirection only occurs in the prefix middleware if the url is different https://github.com/mozilla/addons-frontend/blob/32c493bfc2290206df827bbee2c5b1b9ce8082a5/src/core/middleware/prefixMiddleware.js#L118 so if the resulting url is unchanged the slash middleware should just do its thing.

willdurand commented 5 years ago

I think the core of the problem is that the places checking for clientApp are including the query params because they're not split out independently.

Yep, I noticed that too.

willdurand commented 5 years ago

so if the resulting url is unchanged the slash middleware should just do its thing.

👌

muffinresearch commented 5 years ago

Actually in these cases it would still be a redir because the locale would be added. But something like /en-US/firefox?foo=1 would not be a redir from the prefix middleware.

ioanarusiczki commented 5 years ago

@willdurand I tested this on FF67 like this:

on Android - if I go to a new tab to Bookmarks and tap Firefox:Customize with add-ons I get this link https://addons.mozilla.org/en-US/android/android/?utm_source=inproduct&utm_medium=default-bookmarks&utm_campaign=mobileandroid it's a 404 - page not found same if I paste this link https://addons.mozilla.org/android?utm_source=inproduct&utm_medium=default-bookmarks&utm_campaign=mobileandroid (without /)in a new tab I'm redirected to https://addons.mozilla.org/en-US/android/android/?utm_source=inproduct&utm_medium=default-bookmarks&utm_campaign=mobileandroid (it adds an extra /android/) and which results in a 404 - page not found.
if I paste the link without / https://addons.mozilla.org/android?utm_source=inproduct&utm_medium=default-bookmarks&utm_campaign=mobileandroid in a tab on Windows it redirects to https://addons.mozilla.org/en-US/firefox/android/?utm_source=inproduct&utm_medium=default-bookmarks&utm_campaign=mobileandroid - it won't add the extra /android but it still results in a 404.

willdurand commented 5 years ago

@ioanarusiczki I am not sure to follow, those links are for prod, but the fix has not been pushed to prod yet.

ioanarusiczki commented 5 years ago

@willdurand Sorry, I got confused thinking that links with utm parameters only have to deal with addons.mozilla (more complicated to explain why) . So I only described above the actual issue.

https://addons-dev.allizom.org/android?utm_source=inproduct&utm_medium=default-bookmarks&utm_campaign=mobileandroid redirects correctly https://addons.allizom.org/android?utm_source=inproduct&utm_medium=default-bookmarks&utm_campaign=mobileandroid redirects correctly Tested on both Win and Android - FF67

mozilla / addons

Broken Link #13207

Describe the problem

What happened?

What did you expect to happen?

Anything else we should know?