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

Assert "mockServiceWorker" integrity #81

Closed
kettanaito opened this issue Mar 24, 2020 · 18 comments · Fixed by #82
Closed

Assert "mockServiceWorker" integrity #81

kettanaito opened this issue Mar 24, 2020 · 18 comments · Fixed by #82
Assignees

Comments

@kettanaito
Copy link
Member

Motivation

The library must assert the integrity of the mockServiceWorker.js file for multiple reasons:

  • Ensure the file is up-to-date, as it lives in the user's public directory and is not updated during new installations of msw package
  • Ensure the file is actually a valid MSW file, to prevent malware masking as MSW

Implementation

Publication

  1. Minimize
  2. Remove comments
  3. Generate a checksum out of the mockServiceWorker.js file content

Client-side

  1. Signal to Service Worker upon start() invocation to get the integrity information.
  2. Assert the integrity on the client-side.
  3. Display an error in case integrity fails. Suggest to run npx init <PUBLIC_DIR> to resolve the issue.
@kettanaito
Copy link
Member Author

Starting from 0.11.0, MSW will validate the Service Worker’s integrity on runtime to ensure that mockServiceWorker.js file stays up-to-date.

This is necessary since the user is responsible for serving the Service Worker file, and it can get out of sync with the library’s logic, if the file is stale. To prevent extra actions from the user's side, we've introduced an algorithm that would invalidate the Service Worker only when its code has changed.

Please, if you are using MSW prior to 0.11.0, when updated to that version your Service Worker’s integrity check will fail. You will see the error message in the browser’s console with the instructions on how to resolve it. Keep in mind that MSW does not enable mocking with Service Workers that don’t pass the integrity check.

@kentcdodds, I know you are an active user of MSW so I wanted to ping you directly regarding these changes. Sorry for possible inconvenience, I hope the update goes smoothly for you, as this is a crucial change for the library’s future. Thanks.

@kentcdodds
Copy link
Contributor

I just updated and I haven't gotten any console warnings yet. But I tried running npx msw init public anyway and here's what I got:

$ npx msw init public
Cannot find module '../config/constants'
Require stack:
- /Users/kentcdodds/code/bookshelf/node_modules/msw/cli/init.js
- /Users/kentcdodds/code/bookshelf/node_modules/msw/cli/msw.js

@kentcdodds
Copy link
Contributor

Also, I think there may be a bug with 0.11.0. I don't think it's initializing the msw service worker anymore.

@kettanaito
Copy link
Member Author

@kentcdodds, Thanks for getting back with this. Now I see I haven't whitelisted the config/ directory to be published, while client-side code relies on it. The fix should be published now, could you please update and let me know?

@kentcdodds
Copy link
Contributor

I'm getting an integrity check failure even though I did re-init...

@kentcdodds
Copy link
Contributor

Honestly, I see this integrity check as adding unnecessary complexity. I'd rather opt-into it.

@kentcdodds
Copy link
Contributor

I just verified that the checksum that was created for me was different from the one that's inside msw... Not sure how that happened.

Correct me if I'm wrong, but this checksum basically means that every time I update msw, I'll need to update the service worker? That would be a pretty annoying workflow...

@kettanaito
Copy link
Member Author

kettanaito commented Mar 27, 2020

@kentcdodds sorry to hear that. It mat look like a redundant step, but once we get it right it’ll be worth it. Your feedback is helpful and I’ll look into the issue you’re experiencing.

The integrity checksum is generated upon build before publishing the package. No matter what you do with MSW workflow, it must never change. You can see it in the mockServiceWorker.js file source, but you shouldn’t edit the Service Worker directly (regardless, the checksum is not re-generated during the usage).

The latest release should contain the integrity 5f3682de8784af08f53b896cdfc3fb42. Could you please verify this is the checksum you have in the mockServiceWorker.js when it's copied via npx msw init? I'll do so myself on a clean project to check.

@kentcdodds
Copy link
Contributor

I understand that, but I really don't think it's worth the impact to everyone's workflow. Doing this would basically mean every time you upgrade packages you have to rerun the init. That would be super annoying. Totally not worth it. Please consider an opt-out at least.

@kentcdodds
Copy link
Contributor

This basically protects me from myself by making me do extra work. But if I have the discipline to not change that file myself then I don't want to do the extra work.

@kettanaito
Copy link
Member Author

kettanaito commented Mar 27, 2020

@kentcdodds the way checksum is generated is so it only updates if mockServiceWorker.js changes on the library's side. So if between release A and release B there were no changes introduced by the library, you can safely keep using the Service Worker you have registered. For that, the checksum is derived from the minified and comment-free Service Worker file, to prevent false integrity bumps.

I'm not pushing for this kind of check and I'm open to discuss an alternative to keep the Service Worker and the library's logic in sync. I think what you are experiencing right now is a hiccup to get this check rolled out. I plan to make the process as painless as possible for the developer, so no extra actions are necessary, if there is no reason for it. Your feedback also helps a lot, and I'm once more sorry for the painful initial experience. Hope to polish this functionality to its best.

I've bumped the version and see the integrity check pass on 0.11.2. The checksum published in the previous release was corrupted. I hope this won't happen again.

@kettanaito
Copy link
Member Author

Perhaps, as a middle-ground the integrity assertion failure could still allow SW to mock responses. I'd consider such behavior as opt-in.

@kentcdodds
Copy link
Contributor

Oh, I was confused. So it's not likely that there would be a problem because that file doesn't get updates very often.

I think that's ok. Sorry for my misunderstanding, thank you for taking the time to explain it to me.

@kettanaito
Copy link
Member Author

It’s perfectly fine. I’m glad I was able to explain it better :)

Please, does 0.11.2 fixes the integrity issue for you?

@kentcdodds
Copy link
Contributor

It does fix the integrity warning, but the service worker is no longer handling requests. I'm afraid I don't have time to investigate why, but if you want to try it out feel free: https://github.com/kentcdodds/bookshelf

@kettanaito
Copy link
Member Author

@kentcdodds, thanks for reporting the status. I'll take a look in the nearest time.

@kettanaito
Copy link
Member Author

kettanaito commented Mar 30, 2020

@kentcdodds, I've addressed the issue in kentcdodds/bookshelf#26. It turned out that your application didn't have the latest msw version installed, which caused the integrity check failure. I'm not entirely sure why 0.10.0 prompted you to perform an integrity check in the first place, as the integrity check has been introduced in version 0.11.0. I still suspect the upper caret range quirks that took place and resulted in that issue.

Could you please take a look at the pull request? Thanks.

@kentcdodds
Copy link
Contributor

Excellent. Thank you!

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 a pull request may close this issue.

2 participants