Always include cookies when communicating with signaling server

[aldenh-viam]

We're working on moving Signaling into a separate process from the rest of App: APP-8915

The above linked PR in App should work as-is in production.

In local development, until this PR is also merged, an App running on http[s]://localhost:8080 will be unable to send the session-id cookie required for authentication to a Signaling server running on http[s]://localhost:8081, because localhost:* are considered different origins. This PR changes the fetch request's credentials property from the default same-origin to include if the specified serviceHost different from the signalingAddress (otherwise you may be bypassing App and connecting directly to the robot's internal signaling server, like in the E2E test).

This should not have any effect on the current production, because *.viam.com:443/* is considered same-origin and cookies are already included with the fetch requests.

This approach seems simpler than setting up a reverse proxy like Envoy for local development or changing the RobotClient used by App (for the machine liveness dropdown & control tab) to authenticate using API keys instead of the session-id cookie, but let me know if you feel differently.

---

Current:

App address	Signaling address	Cookies
http[s]://localhost:8080	http[s]://localhost:8080	included (same-origin)
https://app.viam.com:443	https://app.viam.com:443	included (same-origin)

After APP-8915 + this PR:

App address	Signaling address	Cookies
http[s]://localhost:8080	http[s]://localhost:8081	included (via credentials: "include")
https://app.viam.com:443	https://app.viam.com:443	included (same-origin)

[aldenh-viam]

Test is failing because it's trying to connect to directly to viam-server's internal signaling server at http://localhost:9090 and the default Access-Control-Allow-Origin: * cannot be used with credentials: 'include'. It doesn't appear the SDK has a way to differentiate between when the specified signaling server is App (session-id cookie required) or a robot's internal signaling server. They both take this code path:

viam-typescript-sdk/src/rpc/dial.ts

Lines 132 to 137 in 32e1d63

	if (
	opts === undefined ||
	(opts.credentials === undefined && opts.accessToken === undefined)
	) {
	return createTransport(transportOpts);
	}

. I will think about the best way around this.

Update: I made it set credentials to include only if signalingAddress !== serviceHost. If we are bypassing App and connecting directly to a robot's internal signaling server like in the E2E test, its CORS settings are Access-Control-Allow-Origin: * rdk, which cannot be used with credentials: include [1].

[stuqdog]

The PR should work as-is in production, however for local development, an App running on http[s]://localhost:8080 will be unable to send the session-id cookie required for authentication to a Signaling server running on http[s]://localhost:8081, because localhost:* are considered different origins.

Help me understand this. Where does this leave people currently trying to develop against a local app? Will it break existing local-only workflows? Is this a permanent change, or only temporary while the rest of the project completes?

[aldenh-viam]

The PR should work as-is in production, however for local development, an App running on http[s]://localhost:8080 will be unable to send the session-id cookie required for authentication to a Signaling server running on http[s]://localhost:8081, because localhost:* are considered different origins.

Help me understand this. Where does this leave people currently trying to develop against a local app? Will it break existing local-only workflows? Is this a permanent change, or only temporary while the rest of the project completes?

I believe this can be merged now without impacting anyone because:

1. For the RobotClients used in App, the session-id cookie is already being included via the credentials: same-origin default.
2. For RobotClients authenticating with API key, this PR is irrelevant.

After this PR is merged, I need to bump the version in the Svelte SDK, then bump the Svelte SDK version in App. The correct TS SDK version that App's RobotClients need is embedded in App, so users can pull the latest App at their own convenience.

[stuqdog]

For the RobotClients used in App, the session-id cookie is already being included via the credentials: same-origin default.

I thought the same-origin default wouldn't work for an app running on localhost though? Or is that no longer the case.

[aldenh-viam]

For the RobotClients used in App, the session-id cookie is already being included via the credentials: same-origin default.

I thought the same-origin default wouldn't work for an app running on localhost though? Or is that no longer the case.

What do you mean by wouldn't work? Ignored? On the current Main branch in App (App & Signaling both on :8080) I'm able to see the RobotClient passing the cookie to the signaling service.

In my local dev branch (App on :8080 and Signaling on :8081), the RobotClient doesn't pass the cookie unless this change is active.

[stuqdog]

(spoke with @aldenh-viam in-person, got clarification!)

[atlassian]

🔗 Link your GitHub account to Atlassian

To enable Code Reviewer, please link your GitHub account to your Atlassian account.

Click here to connect your accounts

This is a one-time setup that takes less than a minute.