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

Make the session cookie work with the terminal #258

Merged
merged 1 commit into from
Mar 27, 2020
Merged

Make the session cookie work with the terminal #258

merged 1 commit into from
Mar 27, 2020

Conversation

Senyoret1
Copy link
Contributor

@Senyoret1 Senyoret1 commented Mar 26, 2020

Did you run make format && make check?
Yes

Fixes #248

Changes:

  • Now the Path param, set in the hypervisor configuration file, is added to the session cookie. This makes the web browser start sending the session cookie to the terminal window while using the default config, as previously web browsers were sending the cookie only to the /api paths (default browser behavior if the Path param is not set). This makes the terminal work with https connections while auth is activated.

How to test this PR:
Open the full terminal using the manager while auth and tls are active. See note below.

@Senyoret1
Copy link
Contributor Author

There is a small issue with this: if you use the manager without the change, the hypervisor will send a cookie without the path and the browser will save it; if you then add the changes, the hypervisor will send the new cookie with the path and the browser will send both cookies with every request, which will make the authentication process to fail. To solve this, erase the cookies on the browser or wait until the old cookie expires (I think it last something like 2 days)

Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intended change works as expected. I can successfully open the dmsgpty terminal with https when TLS is enabled. However, the overview page cannot be loaded because the uptime request fails:

2020/03/27 11:00:16 "GET https://localhost:8080/api/visors/02980b3933c56b68193efed1bb2f6438d20c381e0d34a29829ac91040d5e6f4a23/uptime HTTP/2.0" from 127.0.0.1:64574 - 500 59B in 877.671997ms

We recently changed the config name of the uptime tracker. Maybe merging latest changes from develop into your branch will solve this issue.

@jdknives jdknives merged commit 1a9be99 into skycoin:develop Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants