Skip to content

Conversation

@ericclemmons
Copy link
Contributor

Issue #, if available: Based on #6146, this solves the security concerns with DataStore & PubSub.subscribe in Node environments.

  • PubSub.subscribe throws in Node. This behavior wasn't explicitly supported and shouldn't be for security reasons. (Multi-tenant apps may share sensitive information across a single, accessible socket)

  • DataStore doesn't start the sync engine in Node.

  • withSSRContext includes DataStore as a default module (along with API and Auth).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

WebSockets aren't supported in Node and this can cause memory leaks
and security issues with multi-tenant customer data being shared
@sammartinez sammartinez added needs-review SSR Issues related to Server Side Rendering labels Sep 4, 2020
Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

Soft approval as I want weigh in from @manueliglesias and @elorzafe on this before we merge

Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Nice work @ericclemmons 🌮 🎉 🥇 !

Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

Lets add the unit tests needed to resolve the unit test failure and Ill be happy to do a full approval

Copy link
Contributor

@manueliglesias manueliglesias left a comment

Choose a reason for hiding this comment

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

Minor comment, LGTM! 🎉

@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

Merging #6726 into main will decrease coverage by 0.01%.
The diff coverage is 29.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6726      +/-   ##
==========================================
- Coverage   73.15%   73.13%   -0.02%     
==========================================
  Files         211      211              
  Lines       13056    13065       +9     
  Branches     2542     2450      -92     
==========================================
+ Hits         9551     9555       +4     
- Misses       3312     3346      +34     
+ Partials      193      164      -29     
Impacted Files Coverage Δ
packages/datastore/src/sync/index.ts 15.41% <9.52%> (+0.02%) ⬆️
packages/aws-amplify/src/withSSRContext.ts 100.00% <100.00%> (ø)
packages/pubsub/src/PubSub.ts 93.84% <100.00%> (+0.29%) ⬆️
packages/auth/src/OAuth/OAuth.ts 56.11% <0.00%> (ø)
packages/core/src/Credentials.ts 30.80% <0.00%> (ø)
packages/analytics/src/Analytics.ts 63.58% <0.00%> (ø)
packages/datastore/src/sync/outbox.ts 24.48% <0.00%> (ø)
packages/datastore/src/storage/storage.ts 71.66% <0.00%> (ø)
packages/core/src/OAuthHelper/GoogleOAuth.ts 33.33% <0.00%> (ø)
... and 11 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 0d24cd5...a6768f4. Read the comment docs.

Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

LGTM 🌮

@ericclemmons ericclemmons force-pushed the ssr-datastore-subscribe branch from 9ed3af3 to 92599ee Compare September 4, 2020 20:09
@ericclemmons ericclemmons merged commit e56aba6 into aws-amplify:main Sep 4, 2020
@ericclemmons ericclemmons deleted the ssr-datastore-subscribe branch September 4, 2020 20:31
@adamup928
Copy link

I see this change now prevents subscriptions from being established in a Node environment, and I understand the justification above regarding multi-tenant apps sharing sockets. In our implementation, however, we are using the PubSub module in a client application running in a desktop environment and built with Electron. This pull request breaks the functionality of the application entirely, and we can not use the latest update.

Is there a recommendation for this scenario? Could we introduce a mode that allows subscriptions to be made when explicitly set?

@ericclemmons
Copy link
Contributor Author

@adamup928 Oh no! Can you open a new issue as a feature request?

You're absolutely right: the restriction in Node does not apply to Electron apps, so we can do better detection to unblock you.

Let's discuss more in that issue, but here are 2 solutions I'm aware of:

  • Remove the isNode check from this PR in favor of a if (config.ssr). In this scenario, we're blocking those connections in SSR scenarios specifically. If Amplify is being used in an Electron app, SSR wouldn't be applicable. (Unless you say otherwise :D)
  • Expand our browserOrNode utility to have other checks like isElectron, isReactNative so we're less generic.

@adamup928
Copy link

Thanks, @ericclemmons. Done! Here it is: #6873

@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

SSR Issues related to Server Side Rendering

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants