Full URI-encoding of URLs returned by kiwix-serve #890
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Before this change `InternalServer::build_redirect()` only URI-encoded the article path, ignoring the book name and/or the root location components of the URL. - In order to be able to test this fix, corner_cases.zim was renamed to contain a couple of special URL symbols in its filename. The `create_corner_cases_zim_file` script was updated accordingly.
Testing of this functionality revealed that the query part containing + symbols (as replacement for spaces in the parameter values) isn't forwarded properly as the + symbols are URI-encoded (this is a bug on the part of the `RequestContext::get_query()` the result of which already contains URI-encoded +'s).
This change doesn't make much sense on its own - the real goal is to prepare some ground for easier implementation of URI-encoding of the root location.
This silly optimization in fact helps to avoid a somewhat more serious waste of CPU cycles that would otherwise result in the next commit.
veloman-yunkan
force-pushed
the
url_encoding_of_redirects
branch
from
711d635
to
06d7cf8
Compare
Codecov ReportBase: 71.88% // Head: 71.95% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #890 +/- ##
==========================================
+ Coverage 71.88% 71.95% +0.07%
==========================================
Files 54 54
Lines 3752 3748 -4
Branches 2100 2100
==========================================
Hits 2697 2697
+ Misses 1053 1049 -4
Partials 2 2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
mgautierfr
reviewed
Feb 8, 2023
Comment on lines
+16
to
+18
| # Assuming that tests are NOT run under Windows, above symbols can be included | ||
| # in testing if the file is renamed while copying to the build directory (see | ||
| # test/meson.build), though that would make maintenance slightly more confusing. |
There was a problem hiding this comment.
We will compile libkiwix on windows and it would be a good thing to test it also.
One thing will could do is rename the file only on linux and don't rename it on windows.
(And adapt the code accordingly). But in this case, that would make maintenance more than slightly more confusing.
There was a problem hiding this comment.
Should we do anything about it now/in this PR?
There was a problem hiding this comment.
No, we can leave it at it is.
Now the root location is URI-encoded too. In order to properly test this change the root location in the tests was changed from "/ROOT" to "/ROOT#?" (or "/ROOT%23%3F" in URI-encoded form), which is why this commit is so big.
veloman-yunkan
force-pushed
the
url_encoding_of_redirects
branch
from
06d7cf8
to
c2fffac
Compare
The alleged bug seems rather an issue with httplib which seems to URI-encode any + present in query parameters.
veloman-yunkan
force-pushed
the
url_encoding_of_redirects
branch
from
d087746
to
51206f4
Compare
mgautierfr
approved these changes
Feb 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tries to ensure that the full path component of URLs returned by kiwix-serve is properly URI-encoded. In particular, all of the following constituents of the path component encoded:
Fixes #441