Parse array slices without eval(<user_input>)

[hyperrealist]

This PR addresses issue #751

[danielballan]

Echoing comments from #751 for visiblity:

Thanks for working on this @hyperrealist.

I want to be clear for anyone reading that there are no known exploitable vulnerabilities in the current codebase. This regex input validation:

tiled/tiled/server/dependencies.py

Line 163 in 9059fd0

slice: str = Query(None, pattern="^[-0-9,:]*$"),

rejects malicious input. For example:

$ http "https://tiled-demo.blueskyproject.io/api/v1/array/full/generated/small_image?slice=import os;os.system('rm -rf /')"
HTTP/1.1 422 Unprocessable Entity
Connection: keep-alive
Content-Length: 261
Content-Type: application/json
Date: Fri, 31 May 2024 20:02:40 GMT
Server: nginx/1.18.0 (Ubuntu)
Set-Cookie: tiled_csrf=GKH9tESfusL742-9G0XY5Rn8TZvqxLQyKg1gka7-42o; HttpOnly; Path=/; SameSite=lax
server-timing: app;dur=8.2
set-cookie: tiled_csrf=GKH9tESfusL742-9G0XY5Rn8TZvqxLQyKg1gka7-42o; HttpOnly; Path=/; SameSite=lax
x-tiled-request-id: b558539f3ba5d4fa

{
    "detail": [
        {
            "ctx": {
                "pattern": "^[-0-9,:]*$"
            },
            "input": "import os;os.system('rm -rf /')",
            "loc": [
                "query",
                "slice"
            ],
            "msg": "String should match pattern '^[-0-9,:]*$'",
            "type": "string_pattern_mismatch",
            "url": "https://errors.pydantic.dev/2.7/v/string_pattern_mismatch"
        }
    ]
}

However, I very much support refactoring to remove eval for defense in depth.

[danielballan]

I would like to retain a regex pattern. It no longer has a security function, with eval removed, but it does have a role in both documenting the acceptable inputs in the OpenAPI spec and providing a detailed error message. (See my example in PR Conversation tab.)

Additionally, it would be useful to test that all the failing examples (typo and malicious) will not match the regex, and therefore should get bounced by FastAPI before they parsed by our code. That is, the ValueError or TypeError should never actually get raised when used in integration with FastAPI.

Finally, the regex pattern I've used here is a little under-specified. It is sufficient to block malicious examples but not sufficient to block malformed (e.g. typo-ed) examples. A more precise regex was drafted in these lines of a dormant PR. Perhaps that would be reused here (with the mean aspect removed for now).

[hyperrealist]

Thanks for the feedback! I'll write a commit with updates.

[padraic-shafer]

I agree. The rest looks good to me.

[padraic-shafer]

Looks good. A couple of minor things are noted in the comments.

[danielballan]

I rebased on main to move this along.

While resolving conflicts in CHANGELOG.md, I refined the language to clarity that there was no known exploitable vulnerability, but that this change improves our defense in depth.