Skip to content
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

fix multiple sharp signs in URIs #356

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Trim
Copy link
Member

@Trim Trim commented Jan 7, 2023

Home site validation used the URL regexp, but didn't anchor it. Because
of this it was possible to add text before or after the URL and have it
considered valid. For instance, this was considered a valid homesite
value: "lorem ipsum https://example.org/#foo#bar dolor sit amet"

It is possible to fix it by anchoring the regexp, but IMO it is wiser to
use the same URL validator used at other places in the site. Hence this
commit replaces this validation with the `http_url` validator used for
news links and bookmarks.

https://linuxfr.org/suivi/impossible-de-mettre-un-lien-vers-un-salon-matrix-dans-les-liens-d-une-depeche#comment-1911550
@nud
Copy link
Contributor

nud commented Jan 8, 2023

Beware of the invisible merge conflict with #355: the change won't cause a conflict but it won't work due to renaming the validator.

rename HttpUrlValidator as UriValidator
This is used to revert the workaround applied on fragment part of the
URI to accept multiple sharp sign in the URI.

This decoding of sharp sign in the fragment part allows to keep visual match
with the user input.
So the bookmark model is the only leader the validation of the link.

Thus the protocol validation is moved to the model too.
@Trim Trim force-pushed the fix-uri-fragement-validation branch from 0f0164c to 51b2fd9 Compare January 28, 2023 22:36
@Trim
Copy link
Member Author

Trim commented Jan 28, 2023

Beware of the invisible merge conflict with #355: the change won't cause a conflict but it won't work due to renaming the validator.

Indeed, I've just merged #355 in the current PR so @Oumph will just need to merge this one as the conflict is resolved.

I've just added commits to add an after_validation method too, to revert the workaround with the sharp sign before saving data inside the database. So user input will match with the data saved and visually the user will not be confused with the %23 encoding.

@nud
Copy link
Contributor

nud commented Mar 6, 2024

@Trim given that #355 was trivial and that this one doesn't show any sign of being merged, wouldn't it be easier to at least fix the existing bug by merging #355 first? Or would you merge this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants