Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle query params correctly for client app urls with no trailing slash #8120

Merged
merged 2 commits into from
Jun 4, 2019
Merged

Handle query params correctly for client app urls with no trailing slash #8120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97ff609
Handle query params correctly for client app urls with no trailing slash
muffinresearch Jun 3, 2019
15777fd
Add test for locale only starting url
muffinresearch Jun 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 26 additions & 17 deletions src/core/middleware/prefixMiddleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ import { getLanguage, isValidLang } from 'core/i18n/utils';
import log from 'core/logger';

export function prefixMiddleware(req, res, next, { _config = config } = {}) {
const URLParts = req.originalUrl.split('?');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we name the two different parts?

const [path, queryString] = req.originalUrl.split('?');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about doing that, but it assumes there's 2 parts, and that's not an unreasonable assumption, but if the url had multiple ? we'd lose the later ones using destructuring. Should we throwaway data if a url contains that mistake?

See below for a possible solution.


// Split on slashes after removing the leading slash.
const URLParts = req.originalUrl.replace(/^\//, '').split('/');
const URLPathParts = URLParts[0].replace(/^\//, '').split('/');

// Get lang and app parts from the URL. At this stage they may be incorrect
// or missing.
const [langFromURL, appFromURL] = URLParts;
const [langFromURL, appFromURL] = URLPathParts;

// Get language from URL or fall-back to detecting it from accept-language
// header.
Expand Down Expand Up @@ -45,14 +47,16 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {
let prependedOrMovedApplication = false;

if (hasValidLocaleException) {
log.info(oneLine`Second part of URL is a locale exception (${URLParts[1]});
log.info(oneLine`Second part of URL is a locale exception (${
URLPathParts[1]
});
make sure the clientApp is valid`);

// Normally we look for a clientApp in the second part of a URL, but URLs
// that match a locale exception don't have a locale so we look for the
// clientApp in the first part of the URL.
if (!isValidClientApp(langFromURL, { _config })) {
URLParts[0] = application;
URLPathParts[0] = application;
isApplicationFromHeader = true;
prependedOrMovedApplication = true;
}
Expand All @@ -65,25 +69,28 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {
// * It's valid and we've mapped it e.g: pt -> pt-PT.
// * The lang is invalid but we have a valid application
// e.g. /bogus/firefox/.
log.info(`Replacing lang in URL ${URLParts[0]} -> ${lang}`);
URLParts[0] = lang;
} else if (isValidLocaleUrlException(URLParts[0], { _config })) {
log.info(`Replacing lang in URL ${URLPathParts[0]} -> ${lang}`);
URLPathParts[0] = lang;
} else if (isValidLocaleUrlException(URLPathParts[0], { _config })) {
log.info(`Prepending clientApp to URL: ${application}`);
URLParts.splice(0, 0, application);
URLPathParts.splice(0, 0, application);
isApplicationFromHeader = true;
prependedOrMovedApplication = true;
} else if (!hasValidLang) {
// If lang wasn't valid or was missing prepend one.
log.info(`Prepending lang to URL: ${lang}`);
URLParts.splice(0, 0, lang);
URLPathParts.splice(0, 0, lang);
// If we've prepended the lang to the URL we need to re-check our
// URL exception and make sure it's valid.
hasValidClientAppUrlException = isValidClientAppUrlException(URLParts[1], {
_config,
});
hasValidClientAppUrlException = isValidClientAppUrlException(
URLPathParts[1],
{
_config,
},
);
}

if (!hasValidClientApp && isValidClientApp(URLParts[1], { _config })) {
if (!hasValidClientApp && isValidClientApp(URLPathParts[1], { _config })) {
// We skip prepending an app if we'd previously prepended a lang and the
// 2nd part of the URL is now a valid app.
log.info('Application in URL is valid following prepending a lang.');
Expand All @@ -93,7 +100,7 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {
);
} else if (hasValidLocaleException || hasValidClientAppUrlException) {
if (
clientAppRoutes.includes(URLParts[1]) === false &&
clientAppRoutes.includes(URLPathParts[1]) === false &&
(hasValidLang || hasValidLocaleException)
) {
log.info('Exception in URL found; we fallback to addons-server.');
Expand All @@ -107,14 +114,16 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {
} else if (!hasValidClientApp) {
// If the app supplied is not valid we need to prepend one.
log.info(`Prepending application to URL: ${application}`);
URLParts.splice(1, 0, application);
URLPathParts.splice(1, 0, application);
isApplicationFromHeader = true;
}

// Redirect to the new URL.
// For safety we'll deny a redirect to a URL starting with '//' since
// that will be treated as a protocol-free URL.
const newURL = `/${URLParts.join('/')}`;
URLParts[0] = `/${URLPathParts.join('/')}`;
const newURL = URLParts.join('?');
Copy link
Member

@willdurand willdurand Jun 4, 2019 •
edited by muffinresearch
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have named variables for each url part, then we could build the url as is (I think):

const newURL = [`/${URLParts.join('/')}`, queryString].join('?');

This reads a bit better, no?

Copy link
Contributor Author

@muffinresearch muffinresearch Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except that in the case of there being no queryString you need to check for it being undefined via the destructuring.

Both of the issues with destructuring are solved if the second parameter is a spread:

function splitURL(url) {
  const [path, ...queryString] = url.split('?');
  const URLParts = path.replace(/^\//, '').split('/');
  const newURL = [`/${URLParts.join('/')}`].concat(queryString).join('?');
  return newURL;
}

// Sane Query string                     
console.log(splitURL('/foo/bar?foo=1'));
// Bad query string
console.log(splitURL('/foo/bar?foo=1?foo=2'));
// No query string
console.log(splitURL('/foo/bar'));

Outputs:

  • /foo/bar?foo=1
  • /foo/bar?foo=1?foo=2
  • /foo/bar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if that's more readable though...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah well, maybe not...

Is a URL with more than one ? valid? I thought this was not possible.


if (newURL !== req.originalUrl && !newURL.startsWith('//')) {
// Collect vary headers to apply to the redirect
// so we can make it cacheable.
Expand All @@ -130,7 +139,7 @@ export function prefixMiddleware(req, res, next, { _config = config } = {}) {

// Add the data to res.locals to be utilised later.
/* eslint-disable no-param-reassign */
const [newLang, newApp] = URLParts;
const [newLang, newApp] = URLPathParts;
res.locals.lang = newLang;
// The newApp part of the URL might not be a client application
// so it's important to re-check that here before assuming it's good.
Expand Down
30 changes: 30 additions & 0 deletions tests/unit/core/middleware/test_prefixMiddleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,36 @@ describe(__filename, () => {
sinon.assert.called(fakeRes.redirect);
});

it('should not redirect for locale + app urls missing a trailing slash with query params', () => {
const fakeReq = {
originalUrl: '/en-US/android?foo=1',
headers: {},
};
prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig });
expect(fakeRes.locals.lang).toEqual('en-US');
expect(fakeRes.locals.clientApp).toEqual('android');
sinon.assert.notCalled(fakeRes.redirect);
sinon.assert.called(fakeNext);
});

it('should redirect for app url missing a trailing slash with query params', () => {
const fakeReq = {
originalUrl: '/android?foo=2',
headers: {},
};
prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig });
sinon.assert.calledWith(fakeRes.redirect, 301, '/en-US/android?foo=2');
});

it('should redirect for locale url missing a trailing slash with query params', () => {
const fakeReq = {
originalUrl: '/en-US?foo=3',
headers: {},
};
prefixMiddleware(fakeReq, fakeRes, fakeNext, { _config: fakeConfig });
sinon.assert.calledWith(fakeRes.redirect, 301, '/en-US/firefox?foo=3');
});

it('should not mangle a query string for a redirect', () => {
const fakeReq = {
originalUrl: '/foo/bar?test=1&bar=2',
Expand Down