Fix page tree node slug validation to prevent URL encoded characters

[fichtnerma]

---

Improve the Slug validation to filter out url escaped special characters to prevent path traversal attacks

PR Checklist

• Verify if the change requires a changeset. See CONTRIBUTING.md
• Link to the respective task if one exists: COM-788
• Provide screenshots/screencasts if the change contains visual changes

[dkarnutsch]

This would be very suitable for a unit test

[nsams]

Please write a description for your PR

[johnnyomair]

This validation should also be adapted in the API: https://github.com/vivid-planet/comet/blob/main/packages/api/cms-api/src/common/validators/is-slug.ts

[nsams]

• you are removing url encoded special characters (%ab)

But you are also:

• you are removing ~ and . from the allowed chars
• you are not allowing uppercase letters anymore
• not allowing - or _ a at the beginning (eg. -abc is not allowed)
• not allowing more than one - or _ (eg. abc--123 is not allowed)
• not allowing - or _ a the and (eg. abc- is not allowed)

(at least that is what I observed by looking at your regex)

That should all have been explained in the description, together with reasons for the change.

---

And as support for encoded special chars was explicitly implemented, are we really sure we can remove it? What was it originally inteded for?

[johnnyomair]

And as support for encoded special chars was explicitly implemented, are we really sure we can remove it? What was it originally inteded for?

Was this explicitly implemented?

Background: In a recent pen test it was concerned that certain characters in the slug input aren't allowed, but that the url-encoded variants of those characters are allowed.

[johnnyomair]

Was this explicitly implemented?

@fichtnerma could you please check when and why this was originally implemented?

[johnnyomair]

We still need to check why this was added and if it can be removed.

[fichtnerma]

Was this explicitly implemented?

@fichtnerma could you please check when and why this was originally implemented?

This validation was added before this repository was moved over to github and as far as i can tell this was not implemented for a specific reason other than the regex supporting all allowed chars in a URI according to https://tools.ietf.org/html/rfc3986#section-2.1

[johnnyomair]

@fichtnerma please address this concern: #2229 (comment). IMO we should only remove the URL encoded characters here.

[fichtnerma]

• you are removing url encoded special characters (%ab)

But you are also:

• you are removing ~ and . from the allowed chars
• you are not allowing uppercase letters anymore
• not allowing - or _ a at the beginning (eg. -abc is not allowed)
• not allowing more than one - or _ (eg. abc--123 is not allowed)
• not allowing - or _ a the and (eg. abc- is not allowed)

(at least that is what I observed by looking at your regex)

That should all have been explained in the description, together with reasons for the change.

And as support for encoded special chars was explicitly implemented, are we really sure we can remove it? What was it originally inteded for?

I have adjusted the validator to only fail if a slash was provided.
The rest of your examples do now work again and were added as tests.

[johnnyomair]

In the linked task it also mentions ".". Wouldn't it be better to remove URL encoding altogether?

[johnnyomair]

@fichtnerma we discussed this in last week's meeting: Please remove support for URL encoded characters in general (as it's supposedly a security issue).

[johnnyomair]

LGTM. @dkarnutsch @nsams please review.

[dkarnutsch]

LGTM

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud