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

Nonce on JS files can cause vendor.js to load twice - breaking the sitetree #8346

Closed
1 task done
blueo opened this issue Aug 29, 2018 · 10 comments · Fixed by silverstripe/silverstripe-admin#649
Closed
1 task done

Comments

@blueo
Copy link
Contributor

blueo commented Aug 29, 2018

Affected Version

tested on 4.2.1, probably exists on all 4.x

Description

TLDR: Nonce on JS files can cause vendor.js to load twice.

Theory:
When loading the CMS, jquery ondemand checks the X-Include-JS headers for JS files to load. If it sees that a file is missing (script isn't present on the page with a src matching that from the header) it will load it. In a multi-server environment (eg with a load balancer) it is possible that you will get two requests on the same page served from different server (eg initial page load vs the treeview request). In this case, the filemtime may be different and the resulting X-Include-JS has a different nonce. Jquery ondemand then sees this as a separate file and proceeds to load the file again. Vendor.js is the first on the list, it loads again and causes the site tree loading to fail.

Steps to Reproduce

Good question, it is hard to reproduce but we have an environment where it is happening currently and I have a HAR archive of a time when we got two vendor.js files loaded with different nonce values - so this could be useful?

Notes

PR's

@emteknetnz
Copy link
Member

I had this same issue in SS3, seemed to fix issue by using a custom Requirements_Backend that replaced ?m=123456789 on js + css with a ?b=234567890 that's based on the last dev/build time. You might want to consider that approach when fixing this. SS3 module here https://github.com/emteknetnz/silverstripe-build-suffix for reference

@chillu
Copy link
Member

chillu commented Sep 5, 2018

We've chased this issue since the dawn of time, and one of the complexities involved is shared networked filesystems (first NFS, now AWS EFS on SSP at least). So it's entirely possible that this can't be reproduced locally (with a local filesystem), or not consistently within SSP either - because it's an edge case. I think @emteknetnz is on the right track with having a different base for the nonce value, but that also invalidates caches way too often. Most static asset files handled through Requirements don't change on each deployment. Changing to this approach will have a tangible impact on the average network payload of many sites, so it's not ideal.

@blueo
Copy link
Contributor Author

blueo commented Sep 5, 2018

Could hash on file contents? That is more intensive on memory/cpu but would change less often. Another approach could be linking to package.json versions?

@emteknetnz
Copy link
Member

https://github.com/brettt89/silverstripe-multi-server also solves this issue by doing an md5 on javascript files, so that's another way to go about fixing this

@chillu
Copy link
Member

chillu commented Sep 5, 2018

My view is that "multi server" (distributed filesystem) isn't an edge case that should require module workarounds. It's good to have those as PoCs, but we need to fix this in core. It would've consumed hundreds of debugging hours across the community already, let's not rely on everyone of those devs finding random modules ;)

@chillu
Copy link
Member

chillu commented Sep 16, 2018

Note that on SilverStripe Platform, we've released an infrastructure-level fix for this. It's not clear if that's a complete fix though. https://docs.platform.silverstripe.com/changelog/#3-22-2-12-september-2018

Ensure source code files have the same modified time on all servers to prevent disappearing buttons on TinyMCE content editor in SilverStripe version 3.X

@emteknz Do you know if this issue occurred outside of SSP, and if the above fixed it?

@blueo
Copy link
Contributor Author

blueo commented Sep 16, 2018

Great to see SSP is handling it but I'd rather see a infrastructure independent solution as other deployments might not be able to do this. Eg a sha/md5 hash of the file contents.

@chillu
Copy link
Member

chillu commented Sep 16, 2018

In general, infrastructure with distributed filesystems should keep stable file modification times. The fact that it's not is an infrastructure bug, not a CMS bug. But we can do better at resilience in the CMS layer for it. I'm tending towards solving this through Option 3 (already the case in SSP), and using Option 2 as a fallback that still needs to be implemented.

Option 1a: Dynamic file contents hash

Compute every time the file is required via SS Requirements API (so all CMS requests).

Pro: Resilient to file modification dates
Con: Needs to read file in PHP, potentially slower. API change for frontend sites, unless we opt-in for the CMS only.

Option 1b: Precomputed file contents hash

Compute file contents hash on dev/build, store in database.

Pro: Same as Option 1a
Con: Faster than Option 1a. API change. More closely filesystem state to database state (can get out of sync on database or filesystem restores). Requires a list of files to hash, only a tiny fraction of files goes through the Requirements API. Makes the SilverStripe API harder to use (bug might reoccur when new files are required, but not added to this list)

Option 2: Ignore file timestamp nonces in JavaScript

Create a whitelist of GET params which are stripped from the filename before its used for comparison in jquery.ondemand.js.

Pro: Fast timestamp hashing.
Con: Would apply for all users of jquery.ondemand.js, even outside of the CMS (although we're not exactly promoting this as a public API).

Option 3: Ensure infrastructure has stable timestamps

Sic

@blueo
Copy link
Contributor Author

blueo commented Sep 16, 2018

Could I suggest also 1a but with caching of the dynamic hash. That way the hash is only computed once per flush? This wouldn't have to be the default - leaving it with the current hash function and timestamps is OK so long as your infrastructure is correct.

I get the logic but relying on 3 could lead to a potentially undesirable situation. Say someone is deploying to somewhere where they can't control the platform we're left telling them to take it up with their provider rather than use a alternative hash? Option 2 could also work - it appears the currently included file is a modified version anyway.

@bergice bergice self-assigned this Sep 16, 2018
@chillu
Copy link
Member

chillu commented Sep 17, 2018

@blueo Thanks for the feedback! We're trying to implement Option 2 now. Doesn't preclude us from doing Option 1a as well. One thing I haven't considered before: Outside of the CMS, this is a small issue as well. It reduces cache hit ratios by the number of webservers with out-of-sync timestamps. Much less noticeable, and wouldn't break anything. But it also won't be fixed through Option 2.

@bergice bergice removed their assignment Sep 18, 2018
@lukereative lukereative self-assigned this Sep 19, 2018
bergice pushed a commit to open-sausages/silverstripe-framework that referenced this issue Nov 18, 2018
This will fix load balancer issue where multiple instances serve the same file but with different modified timestamps

fixes silverstripe#8346
smarcet added a commit to OpenStackweb/silverstripe-framework that referenced this issue May 10, 2019
Whitelist nonce parameters from JS resources to be loaded.

This will fix load balancer issue where multiple instances serve the same file but with different modified timestamps

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

Successfully merging a pull request may close this issue.

5 participants