chore: hasValidRelativePathSegments to align with s3 object-keys - #1387
staaldraad wants to merge 2 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // only allow s3 safe characters and characters which require special handling for now | ||
| // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html | ||
| return key.length > 0 && VALID_OBJECT_KEY.test(key) | ||
| return key.length > 0 && VALID_OBJECT_KEY.test(key) && hasValidRelativePathSegments(key) |
There was a problem hiding this comment.
🔴 isValidKey's new relative-path check is applied retroactively via mustBeValidKey in object.ts's findObject/copyObject/moveObject, so any object key already stored under the old (more permissive) validation with excess '..' segments (e.g. 'videos/../../video1.wmv', previously accepted) now throws InvalidKey on lookup, making pre-existing objects permanently inaccessible even though the underlying data is untouched. Fix: only enforce the stricter relative-path rule on write paths (upload/rename/copy destination), or grandfather already-existing keys, so GET/find on legacy keys does not start failing after this deploy.
Extended reasoning...
Before this change, isValidKey only checked the character-class regex, so a caller could successfully upload an object with objectName 'videos/../../video1.wmv' (or any key with excess '..') and it would be stored and later retrievable via ObjectStorage.findObject, which also calls mustBeValidKey on the same key. After this diff, hasValidRelativePathSegments rejects that same key (depth goes negative), so isValidKey now returns false and mustBeValidKey throws ERRORS.InvalidKey for that exact stored key. Any tenant with such a pre-existing object will get GET/HEAD/copy/move failures for it post-deploy, even though nothing about the object itself changed, since findObject (src/storage/object.ts:124/282) applies the new stricter rule uniformly to reads as well as writes.
Verification: normal (backward-compat regression on read paths). On the base branch, isValidKey only checked the char-class regex VALID_OBJECT_KEY = /^[A-Za-z0-9_/!.*'() &$=@;:+,?-]*$/ (limits.ts:88), which permits both . and /, so a key like videos/../../video1.wmv passed validation and could be uploaded/stored (createObject at object.ts:124 uses the same mustBeValidKey→isValidKey). After this diff,…
There was a problem hiding this comment.
this is important to handle
Coverage Report for CI Build 34834080191Coverage increased (+0.008%) to 82.811%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
| */ | ||
| function hasValidRelativePathSegments(key: string): boolean { | ||
| let depth = 0 | ||
| for (const segment of key.split('/')) { |
There was a problem hiding this comment.
nit: maybe too much but we could have !contains('..') fast path since easy/small
|
@staaldraad do you want us to handle the inline comment for legacy keys? |
This branch has not been deployed
3 participants
What kind of change does this PR introduce?
Bug fix/prevent
What is the current behavior?
'isValidKey` applies the allowed character set, without accounting for character semantics such as path normalisation.
What is the new behavior?
Adds
hasValidRelativePathSegmentswhich is evaluated as part ofisValidKey. This applies the same prefix limitations as what AWS s3 does, namely;Additional context
https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html