more validation for containerName and blobKey to avoid access escape #203
Conversation
| @@ -38,6 +38,8 @@ public void validate(String name) throws IllegalArgumentException { | |||
| //blobkey cannot start with / (or \ in Windows) character | |||
| if (name.startsWith("\\") || name.startsWith("/")) | |||
| throw new IllegalArgumentException("Blob key '" + name + "' cannot start with \\ or /"); | |||
| if (name.contains("../")) | |||
| throw new IllegalArgumentException("Blob key '" + name + "' cannot contains ../"); | |||
There was a problem hiding this comment.
super nit
| throw new IllegalArgumentException("Blob key '" + name + "' cannot contains ../"); | |
| throw new IllegalArgumentException("Blob key '" + name + "' cannot contain ../"); |
| @@ -38,6 +38,8 @@ public void validate(String name) throws IllegalArgumentException { | |||
| //blobkey cannot start with / (or \ in Windows) character | |||
| if (name.startsWith("\\") || name.startsWith("/")) | |||
| throw new IllegalArgumentException("Blob key '" + name + "' cannot start with \\ or /"); | |||
| if (name.contains("../")) | |||
| throw new IllegalArgumentException("Blob key '" + name + "' cannot contain ../"); | |||
There was a problem hiding this comment.
Why is this different than the container name validator? For robustness should this tokenize the path via / then check each component to see if one contains . or ..? This would allow keys like ..foo to work.
There was a problem hiding this comment.
I think ..foo should be allowed. I tried several object storage services, such as gcloud and aliyun oss, they all allow ..foo as object key.
| @@ -691,6 +698,8 @@ public BlobAccess getBlobAccess(String containerName, String blobName) { | |||
|
|
|||
| @Override | |||
| public void setBlobAccess(String container, String name, BlobAccess access) { | |||
| filesystemContainerNameValidator.validate(container); | |||
| filesystemBlobKeyValidator.validate(name); | |||
There was a problem hiding this comment.
Should this also apply to getBlobAccess, putBlob, and removeBlob? getBlobKeysInsideContainer too I guess.
There was a problem hiding this comment.
These methods have been applied by the validators. I just added some missing ones.
|
@gaul Could you please review the new commits? |
|
Thank you for your contribution @jixinchi! |
No description provided.