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: prevent adding keys with undefined values to the yar object in lazy mode #160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

orakili
Copy link

@orakili orakili commented Apr 1, 2022

This PR adds a check before copying stored keys to the yar object in lazy mode to prevent adding undefined values.

Problem

In some cases the yar.onPreAuth() can be called several times in a row without any yar.onPreResponse() call in between.

In lazy mode, this causes the properties moved from the internal _store to the yar object to be overridden with undefined values.

Example with a "authorize" lazy key:

  1. first call to onPreAuth: "authorize" property is moved from the _store to the yar object in the _initialize().
  2. second call to onPreAuth: the "authorize" key is still the _store._lazyKeys so the code tries to move the "authorize" value from the store to the yar object. However this was already moved in (1), so this results in the "authorize" property on the yar object to be overridden with an "undefined" value.

Solution

Only move the properties if they exist in the _store.

Alternative solution

Empty the _lazyKeys after moving the values in _initialize(). Note: deleting the _lazyKeys would result in the lazy mode to be reseted to false, so _lazyKeys needs to be set to [] instead of being deleted.

@devinivy
Copy link
Member

It's possible I've missed something here, but I do not believe yar's onPreAuth could be called multiple times for the same request. So it seems like you might have a situation where _store state is being shared across multiple requests, and I am trying to imagine how this could happen. Do you use the catbox-object cache engine, by chance?

If I've misunderstood here, would you be able to write a failing test case to illustrate? I believe that in order land a fix like this we will need to add a test to ensure this case is covered in the test suite.

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