[server] Fix domain scope for session cookie#1903
Conversation
|
Failed with: We should really figure out how to configure the timeout for the "before each" hook in our tests, because at 2s they time out quite often. /werft run 👍 started the job as gitpod-build-at-session-fix.1 |
With this change slicing of hostname will only be applied for preview environments. ACK this still only works for preview environments deployed without a 2nd level TLD. OTOH it should quickly enable SH installations with 2nd level TLDs!
AlexTugarev
force-pushed
the
at/session-fix
branch
from
4566bce to
39c5080
Compare
|
/werft run 👍 started the job as gitpod-build-at-session-fix.3 |
|
I tried this, but not sure if relevant:
Result:
Note: Using Firefox Nightly as my browser. |
|
Update: Doing the same thing in Chrome works:
|
|
Maybe this is because we use insecure HTTP deployments while we should be using HTTPS instead? |
|
/werft run 👍 started the job as gitpod-build-at-session-fix.4 |
@jankeromnes does it also fail with regular FF? |
|
/werft run 👍 started the job as gitpod-build-at-session-fix.5 |
|
build failing? |
|
/werft run 👍 started the job as gitpod-build-at-session-fix.6 |
|
can i request for this to be bumped up? |
|
/werft run 👍 started the job as gitpod-build-at-session-fix.7 |
|
tested this
would that happen because my instance is behind a corporate firewall? |
|
@hm2075 thanks for watching this issue! what exactly did you test? did you deploy the server image from this branch?
I have no clue about the setup of your FW. The OAuth error OTOH looks unrelated to the underlying issue, though. This indeed indicates a broken config, most likely on the auth server side, e.g. wrong return URL or client id/password. |
hi i just added my local gitlab lab server to http://at-session-fix.staging.gitpod-dev.com/ probably not the best way to test this, however testing with gitlab.com worked fine I guess i would need to deploy this fix to my local instance to test fully |
|
|
||
| const hostParts = hostName.split('.'); | ||
| const baseDomain = hostParts.slice(hostParts.length - 2).join('.'); | ||
| domain = `.${baseDomain}`; |
There was a problem hiding this comment.
I think we need the dot prefixing in all cases, as we want tosend the cookie with requests to subdomains as well, no?
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
cf. Domain
Contrary to earlier specifications, leading dots in domain names (.example.com) are ignored.
There was a problem hiding this comment.
then let's remove the dot here as well.
| // - google: chromium not sending cookies set with redirect | ||
|
|
||
| const hostParts = hostName.split('.'); | ||
| const baseDomain = hostParts.slice(hostParts.length - 2).join('.'); |
There was a problem hiding this comment.
I wonder why the slicing and special handling is necessary here. Also in dev staging all the requests should go to the hostname URL. Is that not the case?
There was a problem hiding this comment.
exactly. I recall, why this is actually needed. in dev staging preview environments we have the ingress proxy which terminates https connections, which is a crucial part in the login process, right? the URL of the ingress proxy has no namespace prefix at all, that's why we've chosen the sliced version of it. this allows those cookies to be present on ingress proxy requests, which are then forwarded to the services.
5 participants
With this change slicing of hostname will only be applied for preview environments.
ACK this still only works for preview environments deployed without a 2nd level TLD.
OTOH it should quickly enable SH installations with 2nd level TLDs!
Fixes #1887