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

Gracefully handle redirect for DPoP #493

Merged
merged 10 commits into from
Oct 30, 2020
Merged

Conversation

NSeydoux
Copy link
Contributor

@NSeydoux NSeydoux commented Oct 27, 2020

Since a DPoP header is scoped to one target IRI, if the request is redirected, the DPoP header is invalid, which results in a 401 unauthenticated. To fix the issue, the fetch must capture the redirection, and replay the request to the actual target IRI.

This is particularly useful when requests target a container, but omit to include the trailing /.

Fixes #89.

Checklist

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4080b26:

Sandbox Source
solid-client-auth-browser-demo Configuration

@NSeydoux
Copy link
Contributor Author

Note that unlike it was initially planned for, this does not throw on redirect: the DPoP token is rebuilt, and the request re-sent to the target IRI.

@vercel vercel bot temporarily deployed to Preview October 27, 2020 14:49 Inactive
Copy link
Contributor

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

I'm not quite following how this works yet, so I left a question inline.

Also, it seems like this might be a good candidate to add and end-to-end test for? Since it's so particular to server behaviour.

@vercel vercel bot temporarily deployed to Preview October 28, 2020 17:28 Inactive
@NSeydoux
Copy link
Contributor Author

Good point, an end-to-end test would be a nice addition to this feature. i'll add one to the current PR.

@NSeydoux NSeydoux force-pushed the fix/handle-container-redirect branch from b9a14e5 to 85dc450 Compare October 30, 2020 09:04
@vercel vercel bot temporarily deployed to Preview October 30, 2020 09:04 Inactive
@vercel vercel bot temporarily deployed to Preview October 30, 2020 10:44 Inactive
@vercel vercel bot temporarily deployed to Preview October 30, 2020 14:04 Inactive
Copy link
Contributor

@pmcb55 pmcb55 left a comment

Choose a reason for hiding this comment

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

This does look a bit shaky, but I'll approve to get PB moving (I assume test coverage is good...!?)

@vercel vercel bot temporarily deployed to Preview October 30, 2020 14:42 Inactive
@NSeydoux
Copy link
Contributor Author

Coverage is 100% for the code impacted by this PR, and an e2e test is also added to verify that fetching https://my.pod/private ends up fetching https://my.pod/private/ successfully. The bits added in the fetch basically aim at not re-issuing any unnecessary request, meaning the initial request hadn't been redirected and had an auth issue, or it had a non-auth issue. 401 and 403 are the only errors that I can see making sense in this context: a request rejected because the provided credentials aren't accepted by the server because of the redirection. Actually typing this, it seems that only 403 makes sense: you are authenticated, but the credentials aren't accepted. If you aren't providing any authentication, retrying the request won't help.

@NSeydoux
Copy link
Contributor Author

NSeydoux commented Oct 30, 2020

Ah yeah right, after a quick test I verified that NSS does return 401 for an authenticated request that gets rejected for DPoP mismatch reasons. I'll log a bug ticket in NSS, and add a link in the code.

Well actually https://developer.mozilla.org/en-US/docs/Web/HTTP/Status and https://tools.ietf.org/html/rfc7235#section-3.1 contradict each other on this: the former specifically says that 401 means credentials are lacking, while the latter says it also applies when credentials are present but not accepted by the server, so I guess technically NSS's response makes sense as well.

@vercel vercel bot temporarily deployed to Preview October 30, 2020 15:13 Inactive
Since a DPoP header is scoped to one target IRI, if the request is redirected, the DPoP header is invalid, which results in a 401 unauthenticated. To fix the issue, the fetch must capture the redirection, and replay the request to the actual target IRI.
Tests showed that e.g. a 303 did not set response.redirected to true. In order for the redirection detection to be successful, the response's URl is compared to the original fetch parameter.
In order to avoid to issue an additional request in case a redirect leads to an error, the redirections are handled manually, and not internally by the browser. This means that each individual request has an appropriate DPoP header, matching the actual target resource.
Fetching with 'redirect: manual' doesn't allow to get the target IRI, which prevents from succesfully redirecting. This rolls back to the previous approach, but prevents from replaying a request if a non-auth error is reported.
@NSeydoux NSeydoux force-pushed the fix/handle-container-redirect branch from 8d27830 to 2ed5a4a Compare October 30, 2020 15:25
@vercel vercel bot temporarily deployed to Preview October 30, 2020 15:25 Inactive
@vercel vercel bot temporarily deployed to Preview October 30, 2020 15:31 Inactive
@NSeydoux NSeydoux merged commit 3db6002 into master Oct 30, 2020
@NSeydoux NSeydoux deleted the fix/handle-container-redirect branch October 30, 2020 15:59
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.

Does not handle redirects well
4 participants