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

Protect against combine argument being broken via Array.slice #801

Merged
merged 7 commits into from
Oct 22, 2022

Conversation

AlexandrHoroshih
Copy link
Member

@AlexandrHoroshih AlexandrHoroshih commented Oct 9, 2022

Effector uses Array.slice internally to prepare argument object for a user, there is a case, where it was broken

Changelog:

  • Added tests against breaking combine argument in userland via .slice = undefined or similiar code
  • Changed arr.slice() to [...arr]

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2022

🚛 size-compare report

Comparing e835447b...8a5a32ee

File +/- Base Current +/- gzip Base gzip Current gzip
npm/effector/compat.js +0.01% 27.7 kB 27.7 kB +0.01% 11 kB 11 kB
npm/effector/effector.cjs.js -0.01% 24.2 kB 24.2 kB = 10.4 kB 10.4 kB
npm/effector/effector.mjs -0.01% 24.2 kB 24.2 kB = 10.4 kB 10.4 kB
npm/effector/effector.umd.js -0.01% 24.1 kB 24.1 kB -0.01% 10.4 kB 10.4 kB
Files wasn't changed
File +/- Base Current +/- gzip Base gzip Current gzip
npm/effector/babel-plugin-react.js = 6.14 kB 6.14 kB = 1.57 kB 1.57 kB
npm/effector/babel-plugin.js = 27.1 kB 27.1 kB = 5.12 kB 5.12 kB
npm/effector/fork.js = 377 B 377 B = 233 B 233 B
npm/effector/fork.mjs = 197 B 197 B = 167 B 167 B
npm/effector-react/compat.js = 8.57 kB 8.57 kB = 3.33 kB 3.33 kB
npm/effector-react/effector-react.cjs.js = 7.88 kB 7.88 kB = 3.21 kB 3.21 kB
npm/effector-react/effector-react.mjs = 7.52 kB 7.52 kB = 3.2 kB 3.2 kB
npm/effector-react/effector-react.umd.js = 8.44 kB 8.44 kB = 3.3 kB 3.3 kB
npm/effector-react/scope.js = 7.45 kB 7.45 kB = 3.09 kB 3.09 kB
npm/effector-react/scope.mjs = 7.12 kB 7.12 kB = 3.1 kB 3.1 kB
npm/effector-react/ssr.js = 571 B 571 B = 263 B 263 B
npm/effector-react/ssr.mjs = 230 B 230 B = 171 B 171 B
npm/effector-solid/effector-solid.cjs.js = 4.04 kB 4.04 kB = 1.9 kB 1.9 kB
npm/effector-solid/effector-solid.mjs = 3.83 kB 3.83 kB = 1.92 kB 1.92 kB
npm/effector-solid/effector-solid.umd.js = 4.28 kB 4.28 kB = 1.97 kB 1.97 kB
npm/effector-solid/scope.js = 3.75 kB 3.75 kB = 1.74 kB 1.74 kB
npm/effector-solid/scope.mjs = 3.5 kB 3.5 kB = 1.74 kB 1.74 kB
npm/effector-vue/compat.js = 1.79 kB 1.79 kB = 864 B 864 B
npm/effector-vue/composition.cjs.js = 4.19 kB 4.19 kB = 1.88 kB 1.88 kB
npm/effector-vue/composition.mjs = 4 kB 4 kB = 1.91 kB 1.91 kB
npm/effector-vue/effector-vue.cjs.js = 1.71 kB 1.71 kB = 832 B 832 B
npm/effector-vue/effector-vue.mjs = 1.51 kB 1.51 kB = 800 B 800 B
npm/effector-vue/effector-vue.umd.js = 1.98 kB 1.98 kB = 938 B 938 B
npm/effector-vue/ssr.cjs.js = 561 B 561 B = 346 B 346 B
npm/effector-vue/ssr.mjs = 540 B 540 B = 331 B 331 B
npm/forest/forest.cjs.js = 34.3 kB 34.3 kB = 10.6 kB 10.6 kB
npm/forest/forest.mjs = 33.6 kB 33.6 kB = 10.6 kB 10.6 kB
npm/forest/forest.umd.js = 34.5 kB 34.5 kB = 10.6 kB 10.6 kB
npm/forest/server.js = 5.79 kB 5.79 kB = 2.06 kB 2.06 kB
npm/forest/server.mjs = 5.76 kB 5.76 kB = 2.03 kB 2.03 kB

@sergeysova sergeysova deleted the fix-combine-leaking-internals branch October 11, 2022 09:53
@AlexandrHoroshih AlexandrHoroshih restored the fix-combine-leaking-internals branch October 22, 2022 10:22
@AlexandrHoroshih AlexandrHoroshih force-pushed the fix-combine-leaking-internals branch from 47a506d to 154b73d Compare October 22, 2022 10:47
@AlexandrHoroshih AlexandrHoroshih marked this pull request as ready for review October 22, 2022 10:47
@AlexandrHoroshih AlexandrHoroshih changed the title Protect against combine argument mutations Protect against combine argument breakage via Array.slice Oct 22, 2022
@AlexandrHoroshih AlexandrHoroshih changed the title Protect against combine argument breakage via Array.slice Protect against combine argument being broken via Array.slice Oct 22, 2022
Copy link
Member

@zerobias zerobias left a comment

Choose a reason for hiding this comment

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

Great! Thanks 👍

@zerobias zerobias merged commit 5a3b308 into master Oct 22, 2022
@zerobias zerobias deleted the fix-combine-leaking-internals branch October 22, 2022 11:20
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