Save incoming returnToUrl from query string in session #19
Conversation
dmitrizagidulin
changed the title
|
Addresses issue #17 |
|
Ready for review. |
|
Thanks so much, will look at this ASAP (but have some backlog after a week of Solid). |
| static extractReturnToUrl (session) { | ||
| const returnToUrl = session.returnToUrl | ||
|
|
||
| return returnToUrl ? decodeURIComponent(returnToUrl) : null |
There was a problem hiding this comment.
If I understand your question correctly -- if this function returns null, then it'll be set to the default / in the AuthCallbackRequest constructor.
test/unit/auth-callback-request.js
Outdated
| let host = { serverUri: 'https://example.com' } | ||
| let session = { returnToUrl: 'https://example.com/resource' } | ||
| let returnToUrl = 'https://example.com/resource#hash' | ||
| let session = { returnToUrl: encodeURIComponent(returnToUrl) } |
There was a problem hiding this comment.
I am assuming (based on what I saw the 401 error handler doing in solid-server) that the returnToUrl is being url-encoded before being stuffed in a query parameter. (So, I'm simulating that in this unit test). Is that not the right assumption?
There was a problem hiding this comment.
Yeah, because it needs to be URL-encoded as an URL parameter. However, decoding is part of URL query string parsing, so it should be fine (and in any case, the session should have the unencoded value).
test/unit/auth-callback-request.js
Outdated
|
|
||
| it('should return a url-decoded returnToUrl from session', () => { | ||
| let returnToUrl = 'https://example.com/resource#hash' | ||
| let session = { returnToUrl: encodeURIComponent(returnToUrl) } |
| webId, | ||
| oidcManager, | ||
| serverUri, | ||
| returnToUrl: query.returnToUrl, |
There was a problem hiding this comment.
This one should normally already be decoded.
There was a problem hiding this comment.
Is it automatically decoded by Express?
There was a problem hiding this comment.
Yes, query is the parsed query string, which includes decoding.
|
@RubenVerborgh - I removed the encoding & decoding of Let me know if there's anything else. |
|
Thanks, retested and works! Could you do a release after you merge? |
|
@RubenVerborgh sure thing, re release. (Are you talking about the |
|
No, lots of vulnerabilities in random npm packages. Mostly an update to package-lock.json. Pull request coming in a couple of minutes. |
|
@RubenVerborgh ok cool! :) What should I do with this PR - merge it, or wait for yours? |
|
You can merge this one, just wait for mine to release please :-) |
|
Thanks, got it. |
|
PR in #21 |
No description provided.