diactric fix by ankur-dwivedi · Pull Request #63 · imagekit-developer/imagekit-python

ankur-dwivedi · 2024-02-26T09:57:15Z

https://www.loom.com/share/925021aeec754a9db3c66ab6ad6f0d62?sid=d5198d96-790c-4505-b804-0a5fcb004a6b

codecov · 2024-02-26T10:16:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.79%. Comparing base (0125fd2) to head (ae6dcb2).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   93.74%   93.79%   +0.05%     
==========================================
  Files          63       63              
  Lines        1630     1644      +14     
==========================================
+ Hits         1528     1542      +14     
  Misses        102      102

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aman051197 · 2024-02-28T10:41:54Z

imagekitio/url.py

+        encoded_path = Url.encode_string_if_required(path)
+        encoded_url = url[:last_slash_pos + 1] + encoded_path + url[question_mark_pos:] if question_mark_pos != -1 else url[:last_slash_pos + 1] + encoded_path
+        url = encoded_url
+        print(url)


unnecessary print statement.

aman051197 · 2024-02-28T13:01:09Z

imagekitio/url.py

+        last_slash_pos = url.rfind('/')
+        question_mark_pos = url.find('?', last_slash_pos)
+        path = url[last_slash_pos + 1:question_mark_pos] if question_mark_pos != -1 else url[last_slash_pos + 1:]
+        encoded_path = Url.encode_string_if_required(path)
+        encoded_url = url[:last_slash_pos + 1] + encoded_path + url[question_mark_pos:] if question_mark_pos != -1 else url[:last_slash_pos + 1] + encoded_path
+        url = encoded_url


This logic is wrong. Please refer this document on how to generate signed Url: https://docs.imagekit.io/features/security/signed-urls#generating-signed-urls

This will generate wrong signature for any asset not in root i.e. if url is
https://ik.imagekit.io/test_url_endpoint/folder,test/four-penguins-with-é.webp?q=rt it will only encode four-penguins-with-é.webp and not /folder,test/four-penguins-with-é.webp.

have updated the logic with relevant test cases

aman051197 · 2024-03-01T05:41:01Z

imagekitio/url.py

        return Default.CHAIN_TRANSFORM_DELIMITER.value.join(parsed_transforms)
+
+    @staticmethod
+    def custom_encodeURIComponent(url_str):


Please rename it to "encodeURI" because the functionality is similar to "encodeURI" in javascript and not "encodeURIComponent".

aman051197 · 2024-03-01T06:52:13Z

imagekitio/url.py

+        encoded_url = f"{parsed_url.scheme}://{parsed_url.netloc}"
+        encoded_url +=quote(parsed_url.path, safe='~@#$&()*!+=:;,?/\'')
+        if(parsed_url.query):
+            encoded_url = encoded_url+"?"+quote(unquote(parsed_url.query), safe='~@#$&()*!+=:;?/\'')


Please provide a comment explaining the reasoning behind quote(unquote(parsed_url.query), safe='~@#$&()*!+=:;?/\'') for future reference and also mention the reference/link used for the logic of this custom "encodeURI" function.

aman051197 · 2024-03-01T06:55:49Z

imagekitio/url.py

+    def custom_encodeURIComponent(url_str):
+        parsed_url = urlparse(url_str)
+        encoded_url = f"{parsed_url.scheme}://{parsed_url.netloc}"
+        encoded_url +=quote(parsed_url.path, safe='~@#$&()*!+=:;,?/\'')


This is encoding imagekitId as well. We need to keep the logic exactly same as given here: https://docs.imagekit.io/features/security/signed-urls#pseudo-code-for-signed-url-generation, i.e. use url_endpoint to extract the string to encode from url in get_signature function above.

aman051197 · 2024-03-01T07:00:30Z

imagekitio/url.py


    @staticmethod
    def get_signature(private_key, url, url_endpoint, expiry_timestamp: int) -> str:
+        url = Url.encode_string_if_required(url)


Wouldn't it be better to call Url.encode_string_if_required(url) this down the line when on the replaced_url which we actually used for signature generation? It would save the unnecessary logic in custom_encodeURIComponent of replacing url_endpoint from url, thereby simplifying the code.

aman051197 · 2024-03-01T08:08:24Z

imagekitio/url.py

+            # This function ensures that characters in the query are properly encoded. While some characters are already encoded, others require encoding for correct processing.
+            encoded_url = quote(url_str.split('?')[0], safe='~@#$&()*!+=:;,?/\'')+"?"+quote(unquote(url_str.split('?')[1]), safe='~@#$&()*!+=:;?/\'')
+        else:
+            encoded_url = quote(url_str.split('?')[0], safe='~@#$&()*!+=:;,?/\'')


Why we are splitting on ? here in else condition?
Replace url_str.split('?')[0] with url_str.
Also make '~@#$&()*!+=:;?/\'' a separate variable and use that for better code readability.

ANKUR DWIVEDI added 3 commits February 26, 2024 15:24

diactric fix

128e022

diactric fix

f391379

diactric fix

86c8dc4

ANKUR DWIVEDI added 2 commits February 26, 2024 16:11

diactric fix

601e59d

diactric fix

9f0e968

aman051197 reviewed Feb 28, 2024

View reviewed changes

aman051197 mentioned this pull request Feb 28, 2024

added encoding logic imagekit-developer/imagekit-nodejs#92

Merged

aman051197 reviewed Feb 28, 2024

View reviewed changes

improve test case

d05c7f6

aman051197 reviewed Mar 1, 2024

View reviewed changes

improve test case

82d8e94

aman051197 reviewed Mar 1, 2024

View reviewed changes

ANKUR DWIVEDI added 5 commits March 5, 2024 02:30

code readibility

e88c32f

update test cases

ec8f633

query encoding logic

e2ce24a

query encoding logic

a914762

query encoding logic

ae6dcb2

aman051197 approved these changes Mar 12, 2024

View reviewed changes

ahnv approved these changes Mar 13, 2024

View reviewed changes

imagekitio merged commit 694c7b9 into master Apr 2, 2024

Comments

Conversation

ankur-dwivedi commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aman051197 Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aman051197 Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ankur-dwivedi commented Feb 26, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading

aman051197 Mar 1, 2024 •

edited

Loading

aman051197 Mar 1, 2024 •

edited

Loading