Strip path from root #10

Open
f wants to merge 3 commits from f/twins:path-strip into master
f commented 5 months ago

If there is a path (the URL path) specified under paths in the
configuration, which points to a root (a physical folder on the
filesystem), this path should not be attached to the root. This fix will
do that. It strips the path from the root.

Example configuration:

hosts:
  localhost:
    paths:
      -
        path: /a/
        root: public/aroot
      -
        path: /
        root: public
Filesystem:
+ public
  + index.gmi
  + aroot
    + index.gmi
If there is a path (the URL path) specified under paths in the configuration, which points to a root (a physical folder on the filesystem), this path should not be attached to the root. This fix will do that. It strips the path from the root. Example configuration: ``` hosts: localhost: paths: - path: /a/ root: public/aroot - path: / root: public ``` ``` Filesystem: + public + index.gmi + aroot + index.gmi ```
f added 1 commit 5 months ago
bd0555069e Strip path from root
Owner

Thanks. From CONFIGURATION.md:

Any path not matching a specific page, file name or file extension should end
in a trailing slash, signifying that it is a directory. Visitors are
automatically redirected when accessing a directory path without including a
trailing slash.

Paths which are folders should always be specified with a trailing slash. If you are encountering issues when specifying paths this way, please open an issue.

Thanks. From CONFIGURATION.md: ``` Any path not matching a specific page, file name or file extension should end in a trailing slash, signifying that it is a directory. Visitors are automatically redirected when accessing a directory path without including a trailing slash. ``` Paths which are folders should always be specified with a trailing slash. If you are encountering issues when specifying paths this way, please open an issue.
tslocum closed this pull request 5 months ago
f commented 4 months ago
Poster

I am not sure if I made my point clear. This works partially for the gemini protocol, but not for https. I double checked it with the latest HEAD and the result is the same (with and without the trailing slash). / works, all other paths result in a 404 error.

My commit bd0555069e fixes this by removing the path from the root, because it will added twice.

I am not sure if I made my point clear. This works partially for the ```gemini``` protocol, but not for ```https```. I double checked it with the latest ```HEAD``` and the result is the same (with and without the trailing slash). ```/``` works, all other paths result in a 404 error. My commit bd0555069ede209f482a524a9a18fdc9a3f35183 fixes this by removing the path from the root, because it will added twice.
f reopened this pull request 4 months ago
f added 1 commit 4 months ago
f commented 4 months ago
Poster

@tslocum Can you please verify this?

@tslocum Can you please verify this?
tslocum requested changes 3 months ago
tslocum left a comment

Thanks for submitting these changes. When using the following path configurations:

`
hosts:
localhost:
...

paths:
path: /sitez1
lang: en
list: true
root: /home/trevor/programming
-
path: /sitez2`
lang: en
list: true
root: /home/trevor/programming/index.gmi
...
`

I encounter a 404 error even though index.gmi exists. This seems to be an issue even before applying your changes. Would you mind reproducing this locally and let me know what you find? If you don't have time to look into this I should be able to at some point over the next few weeks.

Owner

Not sure why that was formatted as a table, but you get the idea.

Not sure why that was formatted as a table, but you get the idea.
f commented 3 months ago
Poster

I've reproduced your configuration like this:

listen: :443

hosts:
  localhost:
    cert: certs/localhost.crt
    key: certs/localhost.key
    paths:
      -
        path: /sitez1/
        root: public_gmi/a
        lang: en
        list: true
      -
        path: /sitez2/
        root: public_gmi/b
        lang: en
        list: true

With current HEAD, I've got 404 errors for both paths (HTTPS, with and without a trailing slash). Gemini works as expected (with AND without trailing slash).

My PR fixes this, so both paths open the correct index.gmi. See the comment in my PR for the root cause.

One minor problem remains: For gemini, there is a redirect (30) if the trailing slash is missing. This redirect is not executed for HTTPS. But this is a separate issue.

It would be great if you double check my PR in order to solve the problem. 👍

I've reproduced your configuration like this: ``` listen: :443 hosts: localhost: cert: certs/localhost.crt key: certs/localhost.key paths: - path: /sitez1/ root: public_gmi/a lang: en list: true - path: /sitez2/ root: public_gmi/b lang: en list: true ``` With current HEAD, I've got 404 errors for both paths (HTTPS, with and without a trailing slash). Gemini works as expected (with AND without trailing slash). My PR fixes this, so both paths open the correct index.gmi. See the comment in my PR for the root cause. One minor problem remains: For gemini, there is a redirect (30) if the trailing slash is missing. This redirect is not executed for HTTPS. But this is a separate issue. It would be great if you double check my PR in order to solve the problem. 👍
f added 1 commit 3 months ago
f commented 3 months ago
Poster

To get this working on my side, I've added my PR as a patch to the Dockerfile and is part of this docker image:
https://hub.docker.com/repository/docker/f00860/twins

Sourcecode for this is located here:
https://git.okoyono.de/f/twins-docker

To get this working on my side, I've added my PR as a patch to the ```Dockerfile``` and is part of this docker image: https://hub.docker.com/repository/docker/f00860/twins Sourcecode for this is located here: https://git.okoyono.de/f/twins-docker

Reviewers

tslocum requested changes 3 months ago
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.