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

The cache busting t query parameter does not work if multiple connections to the server are made #690

Closed
koush opened this issue Oct 10, 2023 · 3 comments
Labels
bug Something isn't working
Milestone

Comments

@koush
Copy link

koush commented Oct 10, 2023

This bug seems unique to Safari deduplicating identical concurrent network requests across embedded iframes.

The t query parameter is a timestamp that is intended for use with cache busting. This cache busting does not work if multiple iframe connections are made to the same server within the same page at the same time. I think this issue is specifically limited to limited to yeast generation in iframes, in Safari.

My use case creates a connection per iframe (for example, lets say 4). These 4 iframes all load at the same time and t value is the same for each iframe. Safari uses the same network executor for each iframe within the main page. Since the same request is for each iframe (including the t value), Safari decides to execute the request once and return the same sid response to all 4 frames. This causes three of the engine io connections to fail with a 400 error on subsequent xhr.

I am adding my own cache busting query parameter to fix this (as well as confirm the underlying pseudorandom seed issue in iframes).

Expected behavior
The cache busting parameter t should be actually random and not a time based deterministic value, since that fails in iframe scenarios.

Platform:

  • Safari on Mac and iOS
@koush koush added the bug Something isn't working label Oct 10, 2023
@darrachequesne
Copy link
Member

Hi!

That's an... interesting behavior, thanks for the detailed analysis. Do you have a suggestion for how to fix this?

@koush
Copy link
Author

koush commented Oct 11, 2023

I can't confidently suggest a fix without a deeper understanding of why engine.io uses yeast, which creates the sid based on the timestamp (or seeded by the timestamp). engine.io server and client do not seem to actually need to decode that timestamp and it's only for cache busting. I think yeast is unnecessary here.

Here's my understanding:
https://stackoverflow.com/questions/48633145/what-is-the-t-query-parameter-in-a-socket-io-handshake

Here's the yeast implementation:
https://github.com/unshiftio/yeast/blob/443a08ea08a10133b6f8a9db536b0d6e52c6bfe1/index.js#L51

My take is that this is not ideal. yeast will generate guaranteed unique values within a single js context, but not within multiple js contexts like my issue, multiple iframes loading all at once and timestamp lucking into the same sid. Addressing this means some sort of pseudorandom value should also be used.

I'd replace yeast altogether with something like:

// yeast-replacement.ts

// guarantee unique within a single js context, like yeast
let suffixUnique = 0;
// probably unique within multiple js contexts
const jsUnique = Math.random().toString(36).replace('.', '');

export function generateId() {
  // time based cache bust, like yeast
  const timeUnique = Date.now().toString(36);
  return timeUnique + jsUnique + (suffixUnique++).toString();
}

Or continue using yeast, and append a one time created jsUnique to every yeast generated id. That's probably the best approach.

darrachequesne added a commit to socketio/engine.io-client that referenced this issue Jun 5, 2024
The yeast() method could generate the same string twice when used in
two different iframes, which can cause Safari to only send one HTTP
request (deduplication) and trigger an HTTP 400 error afterwards since
the two iframes share the same session ID.

This new method, combining 5 chars from the timestamp and 3 chars from
Math.random() should be sufficient for our use case.

Related: socketio/engine.io#690

See also: 874484c
@darrachequesne
Copy link
Member

The format of the query parameter has been updated: socketio/engine.io-client@b624c50 (included in version 6.6.0).

It now uses:

export function randomString() {
  return (
    Date.now().toString(36).substring(3) +
    Math.random().toString(36).substring(2, 5)
  );
}

Thanks!

@darrachequesne darrachequesne added this to the 6.6.0 milestone Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants