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

Pass token into the path variable #537

Closed
wants to merge 2 commits into from
Closed

Pass token into the path variable #537

wants to merge 2 commits into from

Conversation

miguelxpn
Copy link

Fixes #536

@DirectXMan12
Copy link
Member

@miguelxpn : I think this could have issues if someone already has ?abc=xyz on the end of path -- you should check to see if we're already in a query string and then append &token=blah instead of ?token=blah

@miguelxpn
Copy link
Author

@DirectXMan12 You are absolutely right, I changed my commit and it should be ok now.

If token is already present in path the new variable is ignored.
@miguelxpn
Copy link
Author

Made a new commit adding a separate token variable in vnc.html and ui.js, if the token is already present in the path then this new variable is ignored so it doesn't break anything for people using it in the old way.

var path = $D('noVNC_path').value;

//if token is in path then ignore the new token variable
if (token && !(path.indexOf("token") > -1)){
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this cause the token insertion not to trigger if I have
path = "/token", in which case a path of token?token=[foo] is perfectly valid.

Copy link
Member

Choose a reason for hiding this comment

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

We should be able to fix this by using the an <a> element in memory, and using that to manipulate the path.

@DirectXMan12
Copy link
Member

Besides the issue I noted, this LGTM 👍. I'll make the change, rebase, and merge.

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