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: Patch paho-mqtt to apply fix for bug when cookies are disabled #10841

Merged
merged 3 commits into from
Jan 11, 2023

Conversation

stocaaro
Copy link
Member

@stocaaro stocaaro commented Jan 5, 2023

Description of changes

paho-mqtt raises an error breaking amplify-js customer experiences when customers have cookies disabled (as detailed in this issue).

This change adopts the proposed fix as a patch directly in the PubSub category code to fix the experience.

Note
This change breaks the build watch developer convenience behavior for PubSub yarn build:esm:watch --scope @aws-amplify/pubsub. The workaround is worse than living with this for the time being so it is left broken.

Issue #, if available

#9185

Description of how you validated changes

The error is reproducible by:

  • Launching an PubSub enabled app
  • Turn browser cookies off in settings by checking the radio button "Block all cookies (not recommended)"
  • Navigate to the app
  • Observe the white page. Review the error in the console log.

I have verified that without this change, the result is the error:

Uncaught DOMException: Failed to read the 'localStorage' property from 'Window': Access is denied for this document.

After the change, my application works as normal allowing subscriptions to be created and published to, with the caveat that it will prompt for login on every reload (no cookies).

Checklist

  • PR description included
  • yarn test passes

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

@stocaaro stocaaro requested review from a team as code owners January 5, 2023 00:01
Comment on lines +1994 to +2001
var match = uri.match(
/^(wss?):\/\/((\[(.+)\])|([^\/]+?))(:(\d+))?(\/.*)$/
);

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [library input](2) may run slow on strings starting with 'ws://./' and with many repetitions of 'a\]/'.
Copy link
Member Author

Choose a reason for hiding this comment

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

Preferring to keep this consistent with the vendor library implementation.

elorzafe
elorzafe previously approved these changes Jan 5, 2023
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.

Looks good!

Thanks @stocaaro 🎖️

elorzafe
elorzafe previously approved these changes Jan 5, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #10841 (96a80e6) into main (8c797cd) will decrease coverage by 4.57%.
The diff coverage is 7.38%.

@@            Coverage Diff             @@
##             main   #10841      +/-   ##
==========================================
- Coverage   85.88%   81.30%   -4.58%     
==========================================
  Files         197      198       +1     
  Lines       18373    19510    +1137     
  Branches     3909     4208     +299     
==========================================
+ Hits        15779    15862      +83     
- Misses       2520     3358     +838     
- Partials       74      290     +216     
Impacted Files Coverage Δ
packages/pubsub/src/vendor/paho-mqtt.js 7.29% <7.29%> (ø)
...ackages/pubsub/src/Providers/MqttOverWSProvider.ts 91.53% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stocaaro stocaaro force-pushed the 9185-paho-mqtt-patch branch 4 times, most recently from db0657a to e8d34c5 Compare January 9, 2023 15:49
@stocaaro stocaaro force-pushed the 9185-paho-mqtt-patch branch from e8d34c5 to 8f74aea Compare January 9, 2023 15:53
@stocaaro stocaaro requested a review from hyandell January 11, 2023 16:00
@stocaaro stocaaro merged commit ef4beab into aws-amplify:main Jan 11, 2023
Copy link
Contributor

@hyandell hyandell left a comment

Choose a reason for hiding this comment

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

LGTM.

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.

6 participants