Broken Link

[jawman1]

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]

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]

On mobile yes, sorry. Android: Galaxy S10

…

On Wed, May 22, 2019 at 14:56 Jorge Villalobos ***@***.***> 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 <#13207?email_source=notifications&email_token=AMEG5CN5QKO4EE26KAUPTCTPWWXRTA5CNFSM4HOXFJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWAKHZA#issuecomment-494969828>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMEG5CO5A6KLAPGNL7XOXJ3PWWXRTANCNFSM4HOXFJFA> .

[aplaice]

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]

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]

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]

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]

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]

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]

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]

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

[muffinresearch]

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]

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]

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

👌

[muffinresearch]

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]

@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]

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

[ioanarusiczki]

@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