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

Authentication error: hash fragment= not a valid redirect URL #60

Closed
josephguillaume opened this issue Mar 15, 2023 · 6 comments · Fixed by #61
Closed

Authentication error: hash fragment= not a valid redirect URL #60

josephguillaume opened this issue Mar 15, 2023 · 6 comments · Fixed by #61
Assignees
Labels
bug Something isn't working

Comments

@josephguillaume
Copy link

This may or may not be related to SolidOS/solidos#200, but clearly appears to be an error in how solidos is interacting with recent changes in the authentication library.

  1. Go to a URL with a hash fragment, e.g. https://josephguillaume.solidcommunity.net/public/index.ttl#this

  2. Try to login

  3. Page shows an alert with error message: "https://josephguillaume.solidcommunity.net/public/index.ttl#this is not a valid redirect URL, it is either a malformed IRI or it includes a hash fragment"

It appears this message originates from here:
https://github.com/inrupt/solid-client-authn-js/blob/5bc1113604576dcff61b9284f8b736472bc42f4d/packages/node/src/ClientAuthentication.ts#L66

And is a result of a design change by which the authentication library no longer normalises redirect urls
inrupt/solid-client-authn-js#2643

I would guess that solid-logic needs to be updated to normalise the url before passing it in, but I haven't confirmed (apologies if this issue is in the wrong repo)

The possible link to SolidOS/solidos#200 is that the example in question ends up with a hash fragment for "state". I'm not sure where that's coming from, but it seems possible that error could be related to the same change in the authentication library.

@bourgeoa bourgeoa added the bug Something isn't working label Mar 15, 2023
@bourgeoa
Copy link
Contributor

@josephguillaume Thanks for the issue you can make a PR in the redirectUrl branch.

@josephguillaume
Copy link
Author

I wish I could, but I'll see if I can at least work on what the solution should be.

removeOidcQueryParam is now no longer applied to redirectUrl when it is passed in by the client
inrupt/solid-client-authn-js@baac030#diff-84bdced58167dbe812f9b691108262a1590dff1c5df9263f6cd5a9dfa4b6d77aR80
I don't think this is the issue

The only other normalisation I could find that might have been removed in that PR is

redirectUrl: options.redirectUrl
        ? new URL(options.redirectUrl).href
        : undefined

inrupt/solid-client-authn-js@baac030#diff-5e0ce0c5dff0fa7fb28da12e83474b4bc88c930ca57525bea273411789596f6fL64

To reinstate the previous behaviour and close this issue, that's probably sufficient, but given that solid-logic is now responsible for normalising and this is now an explicit design choice, it feels like redirecting to the current page without a hash is rather arbitrary.
At the very least we'd want some tests to make the expected behaviour explicit.

Shouldn't mashlib just consistently redirect to it's homepage as is common with SPAs (i.e. the redirectUrl is always the same), and then use existing functionality to reinstate the correct URI?

@josephguillaume
Copy link
Author

Turns out the specification of the redirectUrl actually occurs in solid-ui
https://github.com/SolidOS/solid-ui/blob/6a9639ed3ed2cd0a96d3c6cd7446c3147bfd3582/src/login/login.ts#L517

The handling of restoration of state also appears to be spread across both solid-ui and solid-logic.

I'm not touching this, sorry.

@bourgeoa
Copy link
Contributor

bourgeoa commented Mar 16, 2023

@timea-solid I'm not sure the above PR's are correctly resolving the problem. Could you check ?

@bourgeoa bourgeoa self-assigned this Mar 16, 2023
@bourgeoa
Copy link
Contributor

NSS tested locally with Watch on both PR's
Issues seemed resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

2 participants