[JENKINS-56931] handle absolute paths correctly when unzipping

[aviadatsnyk]

When checking that an unzipped file does not break out of the target directory - this handle relative paths correctly, where the previous implementation might not. This should prevent a bug that might have been introduced in #3402. For example: if the target directory is ./the-dir and the unzipping happens in /a/b/c an archive with a file path /a/b/c/the-dir which should be legal, will throw an exception.

I don't know if this is even possible under jenkins, but it's a bug nonetheless.

See JENKINS-56931.

Proposed changelog entries

• internal: fix a "not-yet-existing" bug introduced in [JENKINS-32778] - Prevent extracting archived plugins outside of target path #3402

Submitter checklist

• JIRA issue is well described
• Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
  * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
• Appropriate autotests or explanation to why this change has no tests

Desired reviewers

[aviadatsnyk]

I'm not sure which tests are failing - couldn't find any on the link, but a bunch of linux tests were skipped.

[oleg-nenashev]

Test failure is unrelated, we have an infra issue after the weekly release: ERROR: No artifacts found that match the file pattern "**/*-rc15725.2d1a7693b618/*-rc15725.2d1a7693b618*".

[oleg-nenashev]

@aviadatsnyk Could you please create a JIRA ticket for it. If 2.120 gets elected as LTS, we may need t backport it

[daniel-beck]

@aviadatsnyk Don't you mean absolute paths?

[aviadatsnyk]

@daniel-beck according to StackOverflow and getCanonicalPath docs I think we need canonical paths here, to avoid edge cases like /my/ok/path/to/zip and /my/also/../ok/path. That said, I haven't done much java programming lately, so might be missing something here.

[aviadatsnyk]

@oleg-nenashev I'd be happy to open a JIRA, could you please point me to an example similar ticket or some guidlines? I wouldn't like to mess things up in JIRA, and I noticed y'all are very organized with it.

[daniel-beck]

@aviadatsnyk Was more of a comment about the PR description. The paths you're concerned about here are absolute paths in the zip file that happen to end up within the destination dir -- or I don't understand what this does.

[aviadatsnyk]

@daniel-beck you're right. fixed the PR title and the commit message.

[oleg-nenashev]

I completely missed updates in this PR, sorry. After some reviews, I believe this is a reasonable code quality improvement which we could merge. I will create a JIRA issue and update the changelog

CC @Wadeck @jeffret-b @jvz who might be interested to double-check

[jvz]

Makes sense to me.

[jeffret-b]

Looks good. It's good to get caught up on ones like this and get them merged in.

[Wadeck]

Thank you for the contribution @aviadatsnyk.

I would strongly appreciate a test demonstrating the things that are fixed with this PR.

I don't know if this is even possible under jenkins, but it's a bug nonetheless.

My point here is that a bug that is just hypothetical but not happening in reality, is not really a bug, right? If you can at least try to write a test to demonstrate what is fixed, would be nice.

If you can't make a "regular" test working there, perhaps you can try to just call the private method using reflection or marking the method as internal (no access modifier).

[batmat]

Test failures look like #3962, retriggering anew to see the PR build status updated at least.

[aviadatsnyk]

@Wadeck - this can totally happen in reality. All you need is a JAR for a plugin that is malicious, something like https://github.com/snyk/zip-slip-vulnerability/tree/master/archives

Testing this will probably be not trivial, since we'll need to look at the file system to see the difference.

[batmat]

Given the number of approvals, this seems ready for merge.

I would also have appreciated an additional test, but going to consider the majority of reviewers, many from the security team, as sufficient I think.

[Wadeck]

since we'll need to look at the file system to see the difference

If it's the only thing that is not trivial, I think we can collaborate with you to create that test.

[batmat]

Failure seems related to infra, which was partly improved by #3962, retriggering given this was previously building fine the days before without any new change (and I don't believe changes on master touched this area).

[Wadeck]

After looking at the code, the bug is not reachable from regular code. You need to provide a relative File but the method is always used with FilePath that are swallowing the ./ stuff. So from my PoV, this corrects a potential future bug, reducing pain for future developers!

I took the opportunity to add tests to cover this case but also the initial one you wrote.

Test name	Result before 3402	After 3402 before 3425	after 3425
zipAbsolutePathHandledCorrectly_unix	❌	✔️	✔️
zipAbsolutePathHandledCorrectly_win	❌	✔️	✔️
zipRelativePathHandledCorrectly	✔️	✔️	✔️
zipRelativePathHandledCorrectly_oneUp	❌	✔️	✔️
zipTarget_regular	✔️	✔️	✔️
zipTarget_relative	✔️	❌ (this bug)	✔️

[aviadatsnyk]

thank you @Wadeck !

[batmat]

\o/ Thanks @Wadeck for the test addition. Thanks a lot @aviadatsnyk for the fix!

I guess we can now really say this is ready for merge. I'll merge this later today or tomorrow, if nobody objects or does it in the meantime.

Thanks everyone!