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

Transport: When 'Accept' header is stripped - the migration process silently stops. #627

Open
JorindeUsMedia opened this issue Mar 29, 2023 · 3 comments
Assignees
Labels
[Extension Manager] [Type] Environmental bug A corner case bug that's invoked in other software, and it should be resolved there.

Comments

@JorindeUsMedia
Copy link

When 'Accept' header is stripped - the migration process silently stops.

On our server we stripped the 'Accept' header from requests which caused the migration via the transport plugin to silently stop.

The server.class.php (in the transport-extention) determines it can't stream, because of the missing header, but the front-end still expects a stream-response. The backend sends a json response which the front-end can't handle which causes an error in the console: 'EventSource's response has a MIME type ("application/json") that is not "text/event-stream". Aborting the connection.'

Possible things to 'fix' this:

  • Check before migration starts if server support streaming: if not show a clear error on this.
  • Add better debugging so the front-end can get more information on why migration failed.
@sybrew
Copy link
Owner

sybrew commented Mar 29, 2023

Why are Accept headers stripped? They are a standard way for browsers and servers to communicate intent and support: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept. With it omitted, applications can behave unexpectedly, sometimes not as obvious as what you've experienced with Transport.

The server can handle any response; but, if the browser supports the newer one, yet the server is unaware, the server will try the old model while the browser expects the new model.

Two things:

  1. We may not want to add fallbacks if there is a standard method that's broken without good reason.
  2. If the return type is the legacy version, we may want to try unpack that and send the message to the Logger. Still, I'm not sure if browsers allow unpacking of unexpected MIME type responses in streams.

That said, I did want to test if the server supports streaming beforehand, but then to determine the chunk buffer size -- however, that returned false numbers too often, so I left it out (didn't even commit the code).

@usm-joost
Copy link

Fair enough
We are using a 'whitelist' approach on which headers are forwarded to the webserver, and intentional or not but the 'Accept' header wasn't whitelisted in this case ~ it is now.

This issue was mostly just a heads-up, ie. 'hey we noticed this'.
As it feels like weird behaviour when the plugin fallsback to json-communication, even though the frontend doesn't work with it.
Maybe simply throw a hard exception then? But up to you ofcourse as it is a freak-occurance

@sybrew sybrew transferred this issue from sybrew/The-SEO-Framework-Extension-Manager Mar 29, 2023
@sybrew sybrew added [Type] Environmental bug A corner case bug that's invoked in other software, and it should be resolved there. [Extension Manager] labels Mar 29, 2023
@sybrew sybrew self-assigned this Mar 29, 2023
@sybrew
Copy link
Owner

sybrew commented Mar 31, 2023

I'll probably drop the legacy handler since practically all browsers support the event streams: https://caniuse.com/eventsource.
That'll remove the negotiation between the browser and the server, so it'll always return a stream whether the Accept header is read or not.

That'd make a fix for #626 obsolete, but I'll still push that through for now. I may want to use the datastore in the future when we gracefully handle connection drops.

About the error you saw in the console: I cannot move that to the logger because it is unreachable; it is invoked by the browser, which restricts further access:

image

If I'd log event from https://github.com/sybrew/The-SEO-Framework-Extension-Manager/blob/2.6.1/extensions/free/transport/trunk/lib/js/sse.worker.js#L84-L88.

I'll get the following, which contains nothing useful:

image

The issue remains a corner case, but I'll leave this open for when I find new information (or remove legacy support altogether).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Extension Manager] [Type] Environmental bug A corner case bug that's invoked in other software, and it should be resolved there.
Projects
None yet
Development

No branches or pull requests

3 participants