[ANE-2955] Fix vendored archive uploads with absolute paths#1713
Conversation
safeSeparators kept the root "/" as a leading element of its output, and the System.FilePath.Posix.(</>) at the call site treats an absolute RHS as the result. That made compressFile try to write the tarball at the filesystem root and fail with EACCES on non-root users. This is the default code path for archive-upload runs from the meta-fossa Yocto layer, which always emits absolute paths in vendored-dependencies. Filter "/" components out before joining so the result stays relative.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR fixes a crash in vendored dependency archive uploads when using absolute file paths. The 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
csasarak
left a comment
There was a problem hiding this comment.
Can you address my comments re: windows? Aside from that nothing is blocking.
| safeSeparators "foo/bar" `shouldBe` "foo_bar" | ||
| it "leaves bare filenames untouched" $ | ||
| safeSeparators "foo" `shouldBe` "foo" | ||
| it "drops the root component for absolute paths" $ | ||
| safeSeparators "/foo/bar/baz" `shouldBe` "foo_bar_baz" |
There was a problem hiding this comment.
I strongly recommend making these paths in an abstract way. Something like:
do currDir <- getCurrentDirectory -- Should make an absolute path
safeSeparators `shouldSatisfy` (not . any (== pathSeparator))The reason is that our path finding libraries will use the OS native path format. I think if you constructed the paths more abstractly, this test would fail on our Windows build and probably should.
| -- intended output directory. | ||
| safeSeparators :: FilePath -> FilePath | ||
| safeSeparators = intercalate "_" . splitDirectories | ||
| safeSeparators = intercalate "_" . filter (/= "/") . splitDirectories |
There was a problem hiding this comment.
Would it be sufficient to just remove leading /? Not requiring a change, just trying to make sure I understand the problem.
You may also want to consider if this code will work on Windows. https://hackage-content.haskell.org/package/filepath-1.5.5.0/docs/System-FilePath.html#g:1
Use platform-agnostic `System.FilePath.dropDrive` and `splitDirectories` so the function handles Windows drive letters and UNC roots, not just Posix `/`. Test now seeds from `getCurrentDir` to exercise OS-native paths on each CI platform.
|
Sorry to hijack this, I was the person who reported this. Currently the script is putting all the data into a directory under This is - under linux at least - backed up by my RAM. So the question is:
|
|
Hi @hellow554. You should be able to modify the path of temp directory we use like so: TMPDIR=/some/path fossa analyze ...We use If this doesn't work for you, I'll ask that you file a new support ticket as this is a separate issue than the one fixed in this PR. |
Overview
fossa analyzewith archive uploads crashes when anyvendored-dependencies[].pathis absolute. It fails withwithBinaryFile: permission deniedwhile trying to write the tarball at the filesystem root. The meta-fossa Yocto layer always emits absolute paths, so every archive-upload run from it hits this.The temp filename builder kept a leading
/for absolute inputs and that escaped the intended output directory. Fix strips the root component so the tarball stays where it should.Acceptance criteria
Archive-upload
fossa analyzesucceeds when a vendored deppathis absolute, including for the meta-fossa Yocto integration.Testing plan
Stage a vendored source dir at any absolute path:
Create a project dir with a
fossa-deps.jsonpointing at it:Run analyze, forcing archive upload so this code path is exercised regardless of org default:
Against
master, the run dies withwithBinaryFile: permission deniedon a path starting with/_tmp_vendor_foo.... With this branch, the run succeeds and the vendored dep shows up in the project on app.fossa.com.Risks
Behaviour change is scoped to absolute-path inputs; relative paths produce the same filename as before.
Metrics
None.
References
Checklist
docs/.docs/README.msand gave consideration to how discoverable or not my documentation is.Changelog.md. If this PR did not mark a release, I added my changes into an## Unreleasedsection at the top..fossa.ymlorfossa-deps.{json.yml}, I updateddocs/references/files/*.schema.jsonAND I have updated example files used byfossa initcommand. You may also need to update these if you have added/removed new dependency type (e.g.pip) or analysis target type (e.g.poetry).docs/references/subcommands/<subcommand>.md.No docs/schema/subcommand changes needed. Changelog entry under a new
## 3.17.7section since this is intended to ship as the next release.