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

feat: add support to get logged user as json #93

Merged
merged 1 commit into from
Dec 27, 2018

Conversation

perenecabuto
Copy link
Contributor

No description provided.

@magikstm
Copy link
Contributor

magikstm commented Dec 5, 2018

This PR should be turned down.

It would be an information exposure vulnerability.

@perenecabuto
Copy link
Contributor Author

perenecabuto commented Dec 5, 2018

This PR should be turned down.

It would be an information exposure vulnerability.

Hi, @magikstm.
The same route exposes the user data as text/html. I agree that parsing json is easier, but if someone have a stolen token it is possible to extract the the data using jwt.io, jwtlib or parsing html, even if it is a XSS vulnerability. I've checked and there is no jsonp support, so it is not possible to inject script tags to run malicious code.

I've implemented it because it's really useful to get verified logged user data without need to implement it on my APIs and use some basic user data on js web app.

@magikstm
Copy link
Contributor

magikstm commented Dec 6, 2018

@perenecabuto could you please specify the route that exposes it?

Default template does show it after logon:
https://github.com/tarent/loginsrv/blob/master/login/login_form.go#L56

But, it can be hidden with a template change using -template. Is it this one?

I'll review the PR more thoroughly.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 90.605% when pulling 746c3bd on perenecabuto:master into 23bd16e on tarent:master.

@magikstm
Copy link
Contributor

@perenecabuto would PR #96 related to issue #94 help?

Could PR #96 be used in your API to get logged in user?

@smancke smancke merged commit 746c3bd into qvest-digital:master Dec 27, 2018
@smancke
Copy link
Member

smancke commented Dec 27, 2018

@perenecabuto Thank you for the feature!
I overworked it a little bit and merged into master.

  1. I changed it to return the JSON if the Accept header is JSON. The Content-Type is not relevant for that.
  2. I have added some doku ..

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.

None yet

4 participants