Fix URLs for needles in subdirectories (POO #58959)

[AdamWill]

As discussed in the POO, this was broken by PR #2410 commit
36aa974 - it assumes you can always find a needle simply by
sticking the needle filename on the end of a needledir call,
but you can't, needles are allowed to be in subdirectories of
needledir. This should hopefully fix that without breaking
the custom run case by using the whole of the JSON file path -
we just figure out the subdirectory component from it. This works
for me in the 'needle is in a subdirectory of the normal needle
dir' case, but I didn't test it in the custom run case.

Signed-off-by: Adam Williamson awilliam@redhat.com

[AdamWill]

I'm not sure what the right way to test this would be. I don't know exactly where in the existing tests we can exercise it...

[AdamWill]

That test failure doesn't look like it has anything to do with this code...

[Martchus]

Thanks for taking care of the problem. This is indeed something I've overlooked.

I don't know exactly where in the existing tests we can exercise it...

The test cases for this route are hidden within a UI test: https://github.com/os-autoinst/openQA/blob/master/t/ui/07-file.t#L64

So it would be good to extend this subtest. The case that somebody tries to sneak out of the directory using ../ should likely be covered as well.

[okurz]

@AdamWill with the current tests failing in https://circleci.com/gh/os-autoinst/openQA/4088?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link and https://circleci.com/gh/os-autoinst/openQA/4084?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link we should not merge this. If you can manage to fix the tests I would merge to help you get a fix sooner rather than later.

[perlpunk]

fullstack is failing also in master, that's a known problem we're working on.
unstable is, well, unstable, so maybe retrigger a build?

[AdamWill]

I'm planning to add tests and consider the 'directory escape' case as @Martchus suggested anyway, so not expecting this to be merged yet. Will work on it today or tomorrow.

[AdamWill]

Looking at this again, I believe reverse directory traversal protection is already in place: that's what the block commented "make sure the directory of the file parameter is a real subdir of testcasedir" does, if you look at what the is_in_tests function it calls does. So I think I only need to add tests.

[AdamWill]

so I poked this a bit more today. It turns out to be...a little messy.

First off, I think is_in_tests is supposed to protect against people trying to break out of the share dir this way, but it doesn't totally work, because rel2abs does not resolve ... So I changed it to using abs_path (from Cwd), which does resolve those. I added a test for this breakout scenario - below one I found that already exists (for the 'just pass an absolute path that's not under the share dir' variant) in 12-needle-edit.t.

Second, looking at the tests it becomes clear that some other stuff is broken for needles in subdirectories. You can't get the image and JSON for a needle that's in a subdirectory via needles/(needle_id)/json and needles/(needle_id)/image - if I add tests for those they fail, and if I test in Real Life on the Fedora instances it fails too. It's also obviously just icky that you can't find a needle in a subdirectory except by passing this jsonfile arg - you can't do browse to needles/(distri)/subdir/needle.png and find the needle, we don't handle it. I added comments about both of these issues to the tests.

I don't really want to rewrite the whole of this stuff right now, I probably don't understand all the ins and outs. This at least fixes the observed problem on Fedora for now.

[AdamWill]

Given that this is icky, I'm willing to consider the possibility that maybe we should just stop allowing needles in subdirectories. Splitting the Fedora needles up into subdirectories wasn't my idea and I'm really not convinced it's useful. We could just merge all the Fedora needles back into a single directory again, it shouldn't be difficult (there are no name conflicts because we already figured out that openQA is confused by two needles in different subdirectories with the same file name).

[AdamWill]

well damnit, we can't use abs_path because symlinks. sigh. let's try this instead.

[codecov]

Codecov Report

Merging #2456 into master will increase coverage by <.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #2456      +/-   ##
==========================================
+ Coverage   87.16%   87.16%   +<.01%     
==========================================
  Files         169      169              
  Lines       11092    11103      +11     
==========================================
+ Hits         9668     9678      +10     
- Misses       1424     1425       +1

Impacted Files	Coverage Δ
lib/OpenQA/WebAPI/Controller/File.pm	91.34% <91.66%> (-0.06%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07be6b0...19556ef. Read the comment docs.

[okurz]

Looks all good to me now :)