Play challenge unzip.go needs clarification [RESOLVED]

I wrote an entire recursive filepath sanitiser without any libraries XDDD

basically, the problem is that the description does not specify how the vulnerability should be handled. I wrote something that preserved legitimate file paths and extracted the file. However, all you needed to do was raise an error.

The issue is that the autotest is handled out of order. the first test should be checking that an error is raised, the second test should test that the file was not uploaded to a vulnerable directory.

When the vuln directory check is done first, the autotest fails with “malicious file found in ”. This made me figure out a way to upload the file to a legit directory first, and then realising you’re supposed to prevent the upload entirely.

Hey @johnzt2020 firstly great effort and well done in smashing many challenge in a short time.

Unzip is a community contributed challenge and I had another look both at its security tests and your test outputs. It does look the order of test is as you described. Let’s see in the security test output below:

First is the check if the program return error should the vulnerability is about get exploited. This is clear in the output as what test expect. And second check is to verify the filesystem.

Please let me know if I may miss something here.

BTW, for path traversal check there is a better approach than doing it recursively. Which you don’t need to install any external libs and can use Go’s existing official libs.

1 Like

Hmm, I guess what I found unclear was:

  • the challenge was unclear whether or not the filepath should be sanitised + uploaded or discarded.
  • there were no security autotests included
  1. Valid point. Do you have any suggestions how to make the error output more clear? e.g. expected error and rejecting the malicious file, got nothing is this better?
  2. I guess you mean the security tests in the forked repo. As you build up your skill and progress with challenges, security tests meant to be less verbose and removed from the forked repo. This is to simulate a real world scenario where we need to find and fix vulns on our own. I am also aware some tests can be opinionated, so with the feedback from players like yourself, we make them more clear.

Thanks again for raising the issues.

1 Like

Hi, thanks for the quick replies. I’ve edited the post to mark as resolved.