Skip to content

encodeURI vs. encodeURIComponent #75

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yomybaby opened this issue Oct 7, 2019 · 7 comments
Closed

encodeURI vs. encodeURIComponent #75

yomybaby opened this issue Oct 7, 2019 · 7 comments

Comments

@yomybaby
Copy link

yomybaby commented Oct 7, 2019

Hi,
I have a question.
Is there any reason to use encodeURIComponent instead of encodeURI in getFileLocation()?

https://github.com/parse-community/parse-server-s3-adapter/blob/master/index.js#L129

This encodes objectKey. objectKey can have / character. (eg. w200/myImg.png).
encodeURIComponent encodes / to %2F.
encodeURI does NOT encode /.

@dplewis
Copy link
Member

dplewis commented Oct 14, 2019

If I had to take a guess its because encodeURIComponent handles reserve characters ;,/?:@&=+$#.

This is encouraged by AWS best practices (see the “Characters That Might Require Special Handling” section.

Do you have a specific file / uri that is causing an issue?

@davimacedo
Copy link
Member

Take a look at this stack overflow question:

encodeURI assumes that the input is a complete URI that might have some characters which need encoding in it.
encodeURIComponent will encode everything with special meaning, so you use it for components of URIs such as

In this case, we are encoding an URI's component, so encodeURIComponent should be more appropriate as a file with '/' in it's name should have this character encoded as well. Otherwise it would be understood as another path.

@yomybaby
Copy link
Author

yomybaby commented Oct 28, 2019

Please look ..

  1. S3 object key can have / characters (my/file/is/here.png, s3 bucket / make a folder )
  2. Default filename validator doesn't allow / in the file name. So it could uplod a file which has '/' character in file name.
  3. If some file my/file/is/here.png is already existed in S3 bucket, it can be pointed by file fields.
curl -X "POST" "http://localhost:1337/parse/classes/Test" \
     -H 'X-Parse-Application-Id: my.app.id' \
     -H 'Content-Type: text/plain' \
     -d $'{
  "myFile": {
    "name": "my/file/is/here.png",
    "url": "my/file/is/here.png",
    "__type": "File"
  }
}'

It can fetched :

curl "http://localhost:1337/parse/classes/Test" \
     -H 'X-Parse-Application-Id: my.app.id' \

# result
{
  "results": [
    {
      "objectId": "X7ylThRwrT",
      "createdAt": "2019-10-28T09:56:03.240Z",
      "updatedAt": "2019-10-28T09:56:03.240Z",
      "myFile": {
        "__type": "File",
        "name": "my/file/is/here.png",
        "url": "https://s3.ap-northeast-2.amazonaws.com/dev-image.mojitok.com/my%2Ffile%2Fis%2Fhere.png"
      }
    }
  ]
}

Look this part my%2Ffile%2Fis%2Fhere.png. my/file/is/here.png is better than my%2Ffile%2Fis%2Fhere.png. / should not be encoded.
There is no reason to escape / to %2F.

https://github.com/parse-community/parse-server-s3-adapter/blob/master/index.js#L129

// current
const fileName = encodeURIComponent(filename);

// should be
const fileName = filename.split('/').map(encodeURIComponent).join('/');

What do you think about this issue?

@davimacedo
Copy link
Member

You just need to add my/file/is/ to your bucketPrefix option.

@yomybaby
Copy link
Author

yomybaby commented Oct 28, 2019

I know bucketPrefix option. :) I explained when prefix is not fixed.

For example,

  • Upload “my.png” using File Api. S3 object key is “o27gs83sgdbgj_my.png”
  • S3 event triggers AWS Lambda function to make thumbnails. It generates files to S3 directly.
    “200x200/o27gs83sgdbgj_my.png”
    “100x100/o27gs83sgdbgj_my.png”

There is no way to handle this.

I found similar issue and PR.

@mpatnode
Copy link

My vote would be to just use encodeURL, but I'm adding @yomybaby's suggestion to the PR that's now handling directories.

mpatnode pushed a commit to mpatnode/parse-server-s3-adapter that referenced this issue Oct 29, 2019
@mpatnode
Copy link

TFW: Amazon ignores their own recommendations: https://images-na.ssl-images-amazon.com/images/I/41l1%2BOPTfKL.jpg

mpatnode pushed a commit to mpatnode/parse-server-s3-adapter that referenced this issue Nov 27, 2019
dplewis pushed a commit that referenced this issue Dec 11, 2019
…erveFileName Support (#76)

* Add support for the validateFilename method

* Add AWS preserveFileName support with options for "directories"

* Don't extend FilesAdapter (pre-emptive PR request)

* Minor test improvements

* Remove all dependencies.  Bump version number

* speling is hard

* Don't encode / when returning urls.  closes #75

* Make % safe

* Allow filename validation and key generation to be methods

* Parse.Error return type no longer required
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants