Closed kevin-lindsay-1 closed 3 years ago
Ya sounds like a good feature. If you wanna send a PR let me know and I’ll merge it 😊
@mafintosh whenever you're ready
MERGED! :)
@mafintosh - I'm not familiar with the details here - but I've read enough security bug reports that this raises a yellow flag as being a potential point of exploitation. At the very least It opens the door that if there is a vulnerability in the way that a particular file is processed then it would make exploiting it easier. As an example, let's say that a malicious file were uploaded that caused a fork bomb in the attempt to extract that file it would be a bad situation. For me it goes back to the maxim of never trust user input. In any case, I simply raise it as a point for consideration. I trust your judgment about whether the merged change is in fact helpful/desirable or not. Peace - Anthony
@arborrow I agree with the security thoughts, but I have a real-life, non-malicious use case for this that is caused by tooling (which appears to be a tarring tool on Windows). In express
, for example, you can turn off a fair amount of security features. You probably shouldn't in a lot of use cases, but you can.
In my particular use case, there's only one source generating these tarballs, and I fully trust said source. If I were working on a project with more than one source, or I don't fully trust them, then I would simply not enable the feature.
Thanks @kevin-lindsay-1, I suspected there was an actual usage case that made good sense. I completely trust Mathias and the project contributors to do what is best but at least wanted to raise the question.
@arborrow Yeah ideally whatever tool on Windows generating these tarballs wouldn't have deviated from the spec, but the tarballs themselves are otherwise legitimate. I would have not prefered to do any work :)
I would much prefer that windows-land have tools that tar things correctly, but when common tools that don't conform perfectly to the spec pollute the ecosystem by being adopted widespread, unless you figure out what's generating the issue, patch it, and then force windows users to update, there's very little choice but to accommodate reality.
@mafintosh I'm using this feature now and I'm good to close this issue, feel free to close if you decide to weigh in.
It's opt-in, so no concern on my end.
Would be nice if
headers.js
could be updated to not throw an error (only if enabled) on archives that were not packed via theustar
orgnu
variants.I've received tars from people who appear to be running on Windows, and it appears that the program they were using doesn't include those in the header.
If I comment out the error being thrown, it appears to extract just fine, so it would be nice if the
extract
function could be updated with an option likeallowUnknownFormat
/attemptUnknownFormat
, which would be disabled by default.