diactric fix #63
diactric fix #63
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
imagekitio/url.py
Outdated
| 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) |
imagekitio/url.py
Outdated
| 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 |
There was a problem hiding this comment.
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-é.webp?q=rt it will only encode four-penguins-with-é.webp and not /folder,test/four-penguins-with-é.webp.
There was a problem hiding this comment.
have updated the logic with relevant test cases
imagekitio/url.py
Outdated
| return Default.CHAIN_TRANSFORM_DELIMITER.value.join(parsed_transforms) | ||
|
|
||
| @staticmethod | ||
| def custom_encodeURIComponent(url_str): |
There was a problem hiding this comment.
Please rename it to "encodeURI" because the functionality is similar to "encodeURI" in javascript and not "encodeURIComponent".
imagekitio/url.py
Outdated
| 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='~@#$&()*!+=:;?/\'') |
There was a problem hiding this comment.
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.
imagekitio/url.py
Outdated
| 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='~@#$&()*!+=:;,?/\'') |
There was a problem hiding this comment.
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.
imagekitio/url.py
Outdated
|
|
||
| @staticmethod | ||
| def get_signature(private_key, url, url_endpoint, expiry_timestamp: int) -> str: | ||
| url = Url.encode_string_if_required(url) |
There was a problem hiding this comment.
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.
imagekitio/url.py
Outdated
| # 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='~@#$&()*!+=:;,?/\'') |
There was a problem hiding this comment.
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.
https://www.loom.com/share/925021aeec754a9db3c66ab6ad6f0d62?sid=d5198d96-790c-4505-b804-0a5fcb004a6b