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

Display containers even if trailing slash is missing from the URL #4

Closed
michielbdejong opened this issue Nov 14, 2023 · 16 comments · Fixed by #19
Closed

Display containers even if trailing slash is missing from the URL #4

michielbdejong opened this issue Nov 14, 2023 · 16 comments · Fixed by #19

Comments

@michielbdejong
Copy link
Collaborator

No description provided.

@thhck
Copy link
Collaborator

thhck commented Nov 24, 2023

CSS' issue on this topic, seems that there is no conflict ( neither obligation ) from the spec perspective, and it could be merged upstream
CommunitySolidServer/CommunitySolidServer#1208
CommunitySolidServer/CommunitySolidServer#1208 (comment)

@bourgeoa
Copy link
Member

bourgeoa commented Nov 15, 2024

The patch only works when logged-in, else fails with 401 unauthorized
Example below (I removed manually the /) .
But do not fail with https://bourgeoa.solidcommunitypublic/test the / is always added back

image

@jeff-zucker
Copy link
Member

Also note - I tested /public and it redirected me to /public/ correctly BUT it did not add the trailing slash to the browser location bar URI. NSS does add the slash to the URI.

@jeff-zucker
Copy link
Member

jeff-zucker commented Nov 15, 2024

The patch only works when logged-in, else fails with 401 unauthorized

I found this behavior

         logged-in  /public/  - view container
  not logged-in  /public/  - view container
          logged-in  /public  - view container
     not-logged-in /public - 401

@michielbdejong
Copy link
Collaborator Author

only works when logged-in, else fails with 401 unauthorized

Ah, good catch! I guess in that case our code is not reached. I'll fix that.

the browser location bar URI. NSS does add the slash to the URI.

I'll look into that as well! Maybe that's a subtle difference between a 301 and a 302 or something.

@michielbdejong
Copy link
Collaborator Author

I have to look at CSS architecture and see where we can do the check for public resources.

@michielbdejong
Copy link
Collaborator Author

michielbdejong added a commit that referenced this issue Nov 21, 2024
Due to #29 I'm disabling the code from #19 and using the upstream code.

This means that #4 is now fully unsupported again (both for public and for non-public resources)
@michielbdejong
Copy link
Collaborator Author

As noted in #29 it's probably better to find a different point in the architecture to address this issue, and then fix it once and for all for both publish and non-public resources, and without confusing the DPoP checker and the locking mechanism.

@michielbdejong
Copy link
Collaborator Author

michielbdejong commented Nov 21, 2024

Maybe the best place to do this is actually in mashlib:
When the Outline.expand: Unable to fetch ... error occurs, try an asynchronous fetch of the current URL but with a slash added/removed, and if that succeeds, redirect to it.

@michielbdejong
Copy link
Collaborator Author

After discussion in Solid OS Matrix chat, we decided that the idea of handling it client-side would be less than ideal, so I'll have another stab at it, server-side. There must be a way to catch a 404/401/403 on the server side, just before it's sent in the http response, and then decide whether it needs to be rewritten to a redirect response.

@michielbdejong
Copy link
Collaborator Author

The trouble is that we don't want to send a redirect if the request would still fail after redirection.
For that, we can't use the whole request handling code path, because the DPoP headers would mis-match on the URL.
We also don't want to expose if the URL exists in the absence of authorization.
Maybe pass an option 'ignore DPoP URL mismatch', and then give the client an opportunity to produce the correct DPoP headers after redirection. This might be a small leap of faith but I think it's safe and doesn't open any attack vectors.
It's not that the client would be able to use ANY DPoP headers, only one that is off by a slash.

@michielbdejong
Copy link
Collaborator Author

Digging into this with tubsproject/devlog#8 - I should probably overwrite the HandlerServerConfigurator.

@bourgeoa
Copy link
Member

bourgeoa commented Dec 4, 2024

It is also the only place in CSS where a 404 status is set.

@michielbdejong
Copy link
Collaborator Author

I'll create a PivotHttpHandler that wraps around the CSS HttpHandler.

@michielbdejong
Copy link
Collaborator Author

Working on this in #39

@michielbdejong
Copy link
Collaborator Author

Done.

It adds a slash when missing, it doesn't remove a slash when it's extra.

It acts when status code is 401, 403 or 404, and uses a 301 status code to do the redirect.

It's not checking whether the resource it's redirecting to actually exists nor whether the presented WebId and Origin would have access to it. So it's also not leaking that information.

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 a pull request may close this issue.

4 participants