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.

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

Fix path detection can crash app (if req.originalUrl is not set) #137

Merged
merged 1 commit into from
Oct 6, 2017

Conversation

iamjochem
Copy link
Contributor

hi there,

Background

the following line crashes if req.originalUrl is not set (it is technically possible even though unexpected in the context of express!):

let parts = req.originalUrl.split('/');

having encountered that I decided to try and set the lookupFromPathIndex option to undefined, but that is not possible due to the default value being 0 in combination with how the option defaults are applied (undefined values are always overwritten) i.e.

setting of option defaults happens here:

this.options = utils.defaults(options, this.options || {}, getDefaults());

logic for merging options with defaults is here: https://github.com/i18next/i18next-express-middleware
/blob/095a5a926a2ad9e6dcaebc2a22cbb42780b4003e/src/utils.js#L26

"Solution"

this PR :-) the changes are as follows:

  1. change the type check of lookupFromPathIndex so the "by index" lookup is only attempted if the value is a number - this allows people to turn it off by setting the option to false
  2. add a check for existence of req.originalUrl - to avoid potential TypeErrors
  3. add a check for existence of req.params - to avoid potential TypeErrors (because I'm paranoid)

number 3 in the list is not directly related to the issue I had but I figured "better safe than sorry"

@jamuhl
Copy link
Member

jamuhl commented Oct 6, 2017

LookupFromPath can be disabled by not having it in the lookups array: https://github.com/i18next/i18next-express-middleware#detector-options so that options does only set where to look it up...not if on or off

but anyway...the changes do not hurt at all ;)

thank you...merging and publishing

@jamuhl jamuhl merged commit d76786c into i18next:master Oct 6, 2017
@jamuhl
Copy link
Member

jamuhl commented Oct 6, 2017

i18next-express-middleware@1.0.7

@iamjochem
Copy link
Contributor Author

boy that was fast! thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants