Skip to content

Conversation

bethanyj28
Copy link
Contributor

@bethanyj28 bethanyj28 commented Feb 22, 2024

Updating our unzipping logic to use unzip.Parse and handle the entry event for greater control of the unzip process.

@bethanyj28 bethanyj28 marked this pull request as ready for review February 23, 2024 13:52
@bethanyj28 bethanyj28 requested review from a team as code owners February 23, 2024 13:52
new stream.Transform({
objectMode: true,
transform: async (entry, _, callback) => {
const fullPath = path.normalize(path.join(directory, entry.path))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const fullPath = path.resolve(directory, entry.path)

will path.resolve() be an better option, assume entry.path should be a relative path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me! I think download-artifact ensures that the path is absolute, but it doesn't look like toolkit does any validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, doing that throws off the check below for the directory path 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind if I changed it back? On second thought, it might be better to keep this generic and not assume path for anyone utilizing this package. For download-artifact, we have the absolute path: https://github.com/actions/download-artifact/blob/348754975ef0295bfa2c111cba996120cfdf8a5d/src/download-artifact.ts#L38

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought directory is absolute path, looks like it can be relative...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directory is absolute when it comes from download-artifact, but isn't guaranteed absolute from other sources.

objectMode: true,
transform: async (entry, _, callback) => {
const fullPath = path.normalize(path.join(directory, entry.path))
if (!directory.endsWith(path.sep)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is important for the comparison below. Otherwise it will fail with, for example, "/tmp/dest/file".startsWith("../dest/")

Copy link

@gdamiaN538 gdamiaN538 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[gh]

@gdamiaN538
Copy link

gdamiaN538 commented Mar 1, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants