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

Allow "_" for sanitize Report Name #36

Merged
merged 4 commits into from Oct 2, 2018
Merged

Allow "_" for sanitize Report Name #36

merged 4 commits into from Oct 2, 2018

Conversation

@Lunik
Copy link
Contributor

Lunik commented Aug 8, 2018

No description provided.

@rbywater

This comment has been minimized.

Copy link
Member

rbywater commented Aug 12, 2018

Thanks for the pull request. I haven't had a chance to take an in-depth look but one worry I've got with this change is that it might break people who have previously had underscores in the report name which were getting replace with the hex value as part of their builds.

I'll take a closer look when I get a chance but is this something that you've tested to see if it still is happy?

@Lunik

This comment has been minimized.

Copy link
Contributor Author

Lunik commented Aug 12, 2018

I'm aware of your concern. Actually in my case it's the complet opposite.
My file was named with a space witch was translated as an underscore that was fine. But now it's the hex value. Even by replacing it by a proper underscore now give the hex value...

To put some context, I'm working with Squash TM and it search for a file report named with the following uri /test_list/. As your plugin don't allow underscore anymore, /test_list/ cannot exist in any way.

@mrooney

This comment has been minimized.

Copy link
Member

mrooney commented Aug 13, 2018

Yes, we should definitely make sure that this won't break existing URLs, as bookmarking these is a very common use case. If the URLs were previously broken and this fixes them, but it has been many months since, then everyone has probably fixed them and we'd break them again :P

@Lunik

This comment has been minimized.

Copy link
Contributor Author

Lunik commented Aug 16, 2018

It will break.
With the current version, if the report is named "foo_bar" the url will be "foo_5fbar".
So if _ is allowed the url will be "foo_bar"

Am I right ?

@rbywater

This comment has been minimized.

Copy link
Member

rbywater commented Aug 19, 2018

@rbywater

This comment has been minimized.

Copy link
Member

rbywater commented Aug 19, 2018

@rbywater

This comment has been minimized.

Copy link
Member

rbywater commented Aug 23, 2018

Sorry @Lunik - missed your thumbs up as I'd only been looking at the emails 😄 Are you happy/comfortable with making those changes or do you need help with that?

@abloemert

This comment has been minimized.

Copy link

abloemert commented Aug 23, 2018

I think it is quiet safe to change this, even without the checkbox.
It is currently not possible to create a report name like example_name, because it will be changed to example_5freport. However if people changed their bookmarks to example_5freport after the 1.16 release and they don't want to change it again because of this change, they can just change the name to example_5freport and the reports will keep the same name.

@Lunik

This comment has been minimized.

Copy link
Contributor Author

Lunik commented Aug 23, 2018

Hi @rbywater, I think the checkbox option is the best idea for everyone. People who have already corrected the issue will not be affected and it will resolve my issue in the same time.
So yeah, I can't disapprove 👍

@abloemert

This comment has been minimized.

Copy link

abloemert commented Sep 24, 2018

Hi @rbywater, do you have any updates about this? In my opinion it's safe this way, even without checkboxes.

@rbywater

This comment has been minimized.

Copy link
Member

rbywater commented Sep 24, 2018

@abloemert No update from my end as was waiting on @Lunik to provide the checkbox option - however now I think about it I wonder if there's been a bit of miscommunication and it was thought that I was going to add that in.

@Lunik if you can let me know whether you were thinking that I was going to do the checkbox option or whether its something that you are working on? 😄

@rbywater

This comment has been minimized.

Copy link
Member

rbywater commented Sep 25, 2018

I got a bit of spare time so have pushed the changes that should mean it will be an option to enable or disable the escaping of underscores. It will default to true for new reports and existing reports from a previous version.

@daniel-beck Given your involvement with the escaping issue, just wanted to quickly pass this PR past you to make sure that I'm not re-opening up any issues that you can see 😄

@Lunik

This comment has been minimized.

Copy link
Contributor Author

Lunik commented Sep 25, 2018

@abloemert No update from my end as was waiting on @Lunik to provide the checkbox option - however now I think about it I wonder if there's been a bit of miscommunication and it was thought that I was going to add that in.

@Lunik if you can let me know whether you were thinking that I was going to do the checkbox option or whether its something that you are working on? 😄

I was not working on it, sorry for the misunderstanding 😉

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Sep 25, 2018

Changing this option for a given job results in previous reports not being found or references being broken (unsure which), which seems undesirable.

TBH this doesn't seem worth the effort in general: A very minor improvement in URL/directory readability (and there are plenty of other safe characters here that get escaped, especially once we look to CJK) comes with the overhead of an additional option and the above "temporal" compatibility concerns.

That said, as I'm currently not using this, so no opinion. No security concerns either.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Sep 25, 2018

Seems to me a much better solution would be to allow underscores but then automatically support existing report names, as I've done when I wrote the security fix.

@rbywater

This comment has been minimized.

Copy link
Member

rbywater commented Sep 25, 2018

I was not working on it, sorry for the misunderstanding 😉

No worries - if you can have a poke around with my updates to make sure they work with your use-case that would be great.

@rbywater

This comment has been minimized.

Copy link
Member

rbywater commented Sep 25, 2018

Thanks @daniel-beck - quite understand your points. Unfortunately it appears that the '_' situation is quite a common one that has come up from a number of people with some of the tools that unfortunately expect things a certain way. Therefore I'm inclined to give those people an option.

I did look into doing the supporting of existing report names but the chain of checking it would end up doing made my head hurt - I think the defaulting of escaping '_' will mean that for the general population there will be no change but people who really don't like the escaping can turn it off. Yes it will break the existing reports but then currently changing the report name breaks previous reports anyway so I don't think it's too much worse for those who care about turning off escaping.

As an aside I'm working on a branch of code that will hopefully eventually solve most of these issues by not using report name as the storage name but that's still a WIP and not ready yet.

All that said, definitely good though that there are no security concerns as would hate to reopen old wounds.

@@ -115,14 +117,31 @@ public String getReportTitles() {
* @since 1.4
*/
@DataBoundConstructor
public HtmlPublisherTarget(String reportName, String reportDir, String reportFiles, String reportTitles, boolean keepAll, boolean alwaysLinkToLastBuild, boolean allowMissing) {
public HtmlPublisherTarget(String reportName, String reportDir, String reportFiles, String reportTitles, boolean keepAll, boolean alwaysLinkToLastBuild, boolean allowMissing, boolean escapeUnderscores) {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Sep 26, 2018

Member

Should be added in a @DataboundSetter.

This comment has been minimized.

Copy link
@rbywater

rbywater Sep 26, 2018

Member

Thanks - didn't really know much about @DataboundSetter and how it fit into things but now changed to use and a great thing to know for future changes.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Sep 26, 2018

Member

The benefit is that for use in pipeline (unsure if relevant) those are considered optional.

https://jenkins.io/doc/developer/plugin-development/pipeline-integration/#constructor-vs-setters

I think configuration-as-code works similarly, but unsure. @ndeloof might know.

@rbywater rbywater merged commit cc35f9c into jenkinsci:master Oct 2, 2018
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
@rbywater

This comment has been minimized.

Copy link
Member

rbywater commented Oct 2, 2018

@Lunik / @abloemert This has now been release as 1.17. Thanks for your contributions and feedback.

@Lunik

This comment has been minimized.

Copy link
Contributor Author

Lunik commented Oct 3, 2018

Thanks for the patch 👍

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Jan 10, 2020

Another side effect of this change is that there can be file names colliding with sanitized file names, as the set of characters resulting from sanitization (_20 or whatever) would now be part of a regular file name that is no further transformed; previously foo_20bar would have become foo_5F20bar. With this option, foo_20bar and foo bar will override each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.