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(vue-app): use mixin to provide this.$nuxt #8068

Merged
merged 4 commits into from
Sep 30, 2020

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Sep 13, 2020

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

avoids prototype pollution on the Vue prototype

closes #7696

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

avoids prototype pollution on Vue

closes nuxt#7696
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2020

Codecov Report

Merging #8068 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #8068   +/-   ##
=======================================
  Coverage   68.98%   68.98%           
=======================================
  Files          91       91           
  Lines        3847     3847           
  Branches     1047     1047           
=======================================
  Hits         2654     2654           
  Misses        969      969           
  Partials      224      224           
Flag Coverage Δ
#unittests 68.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45b7838...9944c5b. Read the comment docs.

@danielroe danielroe marked this pull request as ready for review September 13, 2020 12:27
Copy link

@Damianos02 Damianos02 left a comment

Choose a reason for hiding this comment

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

@pi0
Copy link
Member

pi0 commented Sep 15, 2020

@clarkdo @danielroe Can we add a SSR test to ensure this fix works?

@danielroe
Copy link
Member Author

Are we okay with prototype pollution outside of a component instance? (I'm also aware that injected objects currently also appear to be shared across requests but I haven't yet verified that...)

@clarkdo
Copy link
Member

clarkdo commented Sep 15, 2020

Are we okay with prototype pollution outside of a component instance?

Actually I don't think is a prototype pollution as it's a very normal way in Vue plugin, the issue is that different ssr requests are sharing same Vue reference.

@clarkdo
Copy link
Member

clarkdo commented Sep 30, 2020

Hi @danielroe , have you verified this fix in your project ? Are we OK to merge ?

@danielroe
Copy link
Member Author

@clarkdo Looks good to me - apologies for the delay. (Didn't realise you were waiting on me 🤦)

@clarkdo
Copy link
Member

clarkdo commented Sep 30, 2020

@danielroe No worries, thanks for the confirmation, I'll merge it now.

@clarkdo clarkdo merged commit 8abdc17 into nuxt:dev Sep 30, 2020
@danielroe danielroe deleted the 7696-shared-server-state branch September 30, 2020 10:51
@pi0
Copy link
Member

pi0 commented Sep 30, 2020

@clarkdo @danielroe I think we shouldn't merge this PR. Mixin method is probably replaceable with simple getter and SSR tests aren't added

@clarkdo
Copy link
Member

clarkdo commented Sep 30, 2020

SSR tests aren't added

I'll revert the pr.

Mixin method is probably replaceable with simple getter

Correct me if I misunderstood, I think we can't use getter like below, because it will still get new reference.

get nuxt() {
  return Vue.prototype.nuxt
}

@danielroe
Copy link
Member Author

Correct me if I misunderstood, I think we can't use getter like below, because it will still get new reference.

get nuxt() {
  return Vue.prototype.nuxt
}

This is correct - it results in the same original issue if we are using getters in the way that the plugins do.

clarkdo added a commit that referenced this pull request Sep 30, 2020
clarkdo added a commit that referenced this pull request Sep 30, 2020
@clarkdo
Copy link
Member

clarkdo commented Sep 30, 2020

Reverted and new pr is here #8132

@pi0
Copy link
Member

pi0 commented Sep 30, 2020

@danielroe Not directly returning same reference 🙈 But we can pass and read same reference from instance options (similar to plugins inject) this way we don't need a new mixin to be called for every component :)

@clarkdo
Copy link
Member

clarkdo commented Sep 30, 2020

@pi0 What about adding $root.... to Vue.prototyp.nuxt getter ?

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

Successfully merging this pull request may close these issues.

Nuxt server $nuxt.context changed by latter request
6 participants