Add check for base_prefix indicating Radicale running at site root (/)

[fasterit]

Alternative to PR #1310 (which in turn documents the issue from #1210)

This small code amendment makes it more easy for users than having to do the right thing™ from documentation which is a bit non-intuitive

[pbiering]

Please check format, according to "Files changed" unexpected indent is inserted?

[fasterit]

That is intentional to get the block under the if base_prefix stanza.
Alternatively it could stay one indent less and the if base_prefix could be repeated.

[pbiering]

Have you tested reachability of your code? I think it will never be reached as it is after the return response... statement...

before:

        base_prefix = environ.get(base_prefix_src, "")
        if base_prefix and base_prefix[0] != "/":
            logger.error("Base prefix (from %s) must start with '/': %r",
                         base_prefix_src, base_prefix)
            if base_prefix_src == "HTTP_X_SCRIPT_NAME":
                return response(*httputils.BAD_REQUEST)
            return response(*httputils.INTERNAL_SERVER_ERROR)
        if base_prefix.endswith("/"):
            logger.warning("Base prefix (from %s) must not end with '/': %r",
                           base_prefix_src, base_prefix)
            base_prefix = base_prefix.rstrip("/")

after:

        base_prefix = environ.get(base_prefix_src, "")
        if base_prefix and base_prefix[0] != "/":
            logger.error("Base prefix (from %s) must start with '/': %r",
                         base_prefix_src, base_prefix)
            if base_prefix_src == "HTTP_X_SCRIPT_NAME":
                return response(*httputils.BAD_REQUEST)
            return response(*httputils.INTERNAL_SERVER_ERROR)
            if base_prefix != "/" and base_prefix.endswith("/"):
                logger.warning("Base prefix (from %s) must not end with '/': %r",
                           base_prefix_src, base_prefix)
                base_prefix = base_prefix.rstrip("/")

[fasterit]

hm, let me check that again. We changed a few more things in the course of getting the base_prefix = "/" warning fixed.
/DLange

[fasterit]

We reverted back to 3.0.6 in prod for reasons unrelated to this one, so sorry this update took so long.
Hopefully the newly pushed change is better.
/DLange

[pbiering]

I'm still wondering why this was set at all, what is the sense behind?

RequestHeader    set X-Script-Name /

Patch would not be required if this RequestHeader is not set

[fasterit]

Because the docs say to set that and it is common on other web applications that are proxied. #1310 wants to fix the docs. Which is - imho - less likely to work than just doing the right thing in the code.
/DLange

[pbiering]

current doc says RequestHeader set X-Script-Name /radicale and not RequestHeader set X-Script-Name /, also latest version is 3.1.8 and I'm running radicale since long behind Apache reverse proxy, not having set the header at all.

I would not change the code if it's only about documentation.

[fasterit]

The doc is an example if people run their radicale proxy-forwarded from server.domain.tld/radicale/. Most people will have other pathinfo so X-Script-Name needs to be adapted from the doc example. When they run it on a server root directory, they can set X-Script-Name / (which is technically correct) or omit the variable.
You should merge this and #1310. This is the most user-friendly option and reduces irritation / support requests.
Have a merry Christmas!
/DLange

[pbiering]

#1310 was merged today, is this PR then still valid?

[fasterit]

Sure, it still saves people that have not spotted the newly added info in the DOCUMENTATION.md. But do as you please, if you don't want the added convenience in code as well, feel free to close.

[pbiering]

this PR breaks checks

[fasterit]

Yeah, somebody wrote an explicit test for the "/" -> force user to unset X-Script-Name case. Let's keep it as is.