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

Remove URL sanitizer as it is malfunctioning and unclear where needed… #683

Merged
merged 1 commit into from
Jan 21, 2016

Conversation

stefan-kolb
Copy link
Member

Also see #667.

@JabRef/developers If someone knows a use case where this is needed and why we can try to reimplement it properly. But this way it really makes no sense IMHO.

@stefan-kolb stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 19, 2016
@oscargus
Copy link
Contributor

So then either URLDecoder.decode or URI.toASCIIstring converts # to %23? Of course not impossible, but seems like one has used the incorrect methods in that case.

I think the main things it does is replacing space with %20 in the URLs and removing any \url{...} commands. There will probably be users that will run into problems, but maybe one can add a URL cleanup that does the corresponding thing?

I think an alternative solution would just be to do a replaceAll("%23","#") at the end.

@stefan-kolb
Copy link
Member Author

The problem is that is really hard to correctly encode a URL if we are not aware of its structure. Regardless of what methods we use.
So if we are not completely sure we need it, we should probably not do this.

@simonharrer
Copy link
Contributor

👍 sounds reasonable.

@simonharrer
Copy link
Contributor

@koppor what do you think about this? Especially is it possible that users have a url={\url{http://google.com}} in their bibfile? And what about escaping symbols for latex like %or #?

@koppor
Copy link
Member

koppor commented Jan 21, 2016

Unsure. The method comment states following:

make sure an URL is "portable", in that it doesn't contain bad characters that break the open command in some OSes. A call to this method will also remove \url{} enclosings.

That reads perfectly fine.

OK, I also read the following

// FIXME: everything below is really flawed atm

Maybe, we should cleanup the URL fields and not do the cleanup at the place of opening. Although I see the issue of malicious bib files which could have an URL executing malicious code. However, that risk is so low that I would not count on it.

Meaning: 👍 and open a new issue for CleanUp.

stefan-kolb added a commit that referenced this pull request Jan 21, 2016
Remove URL sanitizer as it is malfunctioning and unclear where needed…
@stefan-kolb stefan-kolb merged commit f2adf04 into master Jan 21, 2016
@stefan-kolb stefan-kolb deleted the rm-url-sanitizer branch January 21, 2016 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants