diff --git a/.github/.OwlBot.lock.yaml b/.github/.OwlBot.lock.yaml index 24943e116..0bb507a61 100644 --- a/.github/.OwlBot.lock.yaml +++ b/.github/.OwlBot.lock.yaml @@ -1,4 +1,4 @@ -# Copyright 2024 Google LLC +# Copyright 2025 Google LLC # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -13,5 +13,5 @@ # limitations under the License. docker: image: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest - digest: sha256:609822e3c09b7a1bd90b99655904609f162cc15acb4704f1edf778284c36f429 -# created: 2024-10-01T19:34:30.797530443Z + digest: sha256:bdf89cdfb5b791d382184a7a769862b15c38e94e7d82b268c58d40d8952720f2 +# created: 2025-10-03T19:51:38.870830821Z diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 1a639c73d..93dd20eeb 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,7 +1,30 @@ -Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: -- [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/nodejs-storage/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea +> Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: + +## Description + +> Please provide a detailed description for the change. +> As much as possible, please try to keep changes separate by purpose. For example, try not to make a one-line bug fix in a feature request, or add an irrelevant README change to a bug fix. + +## Impact + +> What's the impact of this change? + +## Testing + +> Have you added unit and integration tests if necessary? +> Were any tests changed? Are any breaking changes necessary? + +## Additional Information + +> Any additional details that we should be aware of? + +## Checklist + +- [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/nodejs-storage/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass -- [ ] Code coverage does not decrease (if any source code was changed) -- [ ] Appropriate docs were updated (if necessary) +- [ ] Code coverage does not decrease +- [ ] Appropriate docs were updated +- [ ] Appropriate comments were added, particularly in complex areas or places that require background +- [ ] No new warnings or issues will be generated from this change -Fixes # 🦕 +Fixes #issue_number_goes_here 🦕 diff --git a/.github/release-trigger.yml b/.github/release-trigger.yml index d4ca94189..1e3188f69 100644 --- a/.github/release-trigger.yml +++ b/.github/release-trigger.yml @@ -1 +1,2 @@ enabled: true +multiScmName: nodejs-storage \ No newline at end of file diff --git a/.github/scripts/close-invalid-link.cjs b/.github/scripts/close-invalid-link.cjs index d7a3688e7..fdb514881 100644 --- a/.github/scripts/close-invalid-link.cjs +++ b/.github/scripts/close-invalid-link.cjs @@ -12,21 +12,26 @@ // See the License for the specific language governing permissions and // limitations under the License. +const fs = require('fs'); +const yaml = require('js-yaml'); +const path = require('path'); +const TEMPLATE_FILE_PATH = path.resolve(__dirname, '../ISSUE_TEMPLATE/bug_report.yml') + async function closeIssue(github, owner, repo, number) { await github.rest.issues.createComment({ owner: owner, repo: repo, issue_number: number, - body: 'Issue was opened with an invalid reproduction link. Please make sure the repository is a valid, publicly-accessible github repository, and make sure the url is complete (example: https://github.com/googleapis/google-cloud-node)' + body: "Issue was opened with an invalid reproduction link. Please make sure the repository is a valid, publicly-accessible github repository, and make sure the url is complete (example: https://github.com/googleapis/google-cloud-node)" }); await github.rest.issues.update({ owner: owner, repo: repo, issue_number: number, - state: 'closed' + state: "closed" }); } -module.exports = async ({github, context}) => { +module.exports = async ({ github, context }) => { const owner = context.repo.owner; const repo = context.repo.repo; const number = context.issue.number; @@ -37,20 +42,32 @@ module.exports = async ({github, context}) => { issue_number: number, }); - const isBugTemplate = issue.data.body.includes('Link to the code that reproduces this issue'); + const yamlData = fs.readFileSync(TEMPLATE_FILE_PATH, 'utf8'); + const obj = yaml.load(yamlData); + const linkMatchingText = (obj.body.find(x => {return x.type === 'input' && x.validations.required === true && x.attributes.label.includes('link')})).attributes.label; + const isBugTemplate = issue.data.body.includes(linkMatchingText); if (isBugTemplate) { console.log(`Issue ${number} is a bug template`) try { - const link = issue.data.body.split('\n')[18].match(/(https?:\/\/(gist\.)?github.com\/.*)/)[0]; - console.log(`Issue ${number} contains this link: ${link}`) - const isValidLink = (await fetch(link)).ok; - console.log(`Issue ${number} has a ${isValidLink ? 'valid' : 'invalid'} link`) - if (!isValidLink) { - await closeIssue(github, owner, repo, number); - } + const text = issue.data.body; + const match = text.indexOf(linkMatchingText); + if (match !== -1) { + const nextLineIndex = text.indexOf('http', match); + if (nextLineIndex == -1) { + await closeIssue(github, owner, repo, number); + return; + } + const link = text.substring(nextLineIndex, text.indexOf('\n', nextLineIndex)); + console.log(`Issue ${number} contains this link: ${link}`); + const isValidLink = (await fetch(link)).ok; + console.log(`Issue ${number} has a ${isValidLink ? "valid" : "invalid"} link`) + if (!isValidLink) { + await closeIssue(github, owner, repo, number); + } + } } catch (err) { await closeIssue(github, owner, repo, number); } } -}; +}; \ No newline at end of file diff --git a/.github/scripts/close-unresponsive.cjs b/.github/scripts/close-unresponsive.cjs index 142dc1265..6f81b508f 100644 --- a/.github/scripts/close-unresponsive.cjs +++ b/.github/scripts/close-unresponsive.cjs @@ -1,4 +1,4 @@ -// Copyright 2024 Google LLC +/// Copyright 2024 Google LLC // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -13,57 +13,57 @@ // limitations under the License. function labeledEvent(data) { - return data.event === 'labeled' && data.label.name === 'needs more info'; - } - - const numberOfDaysLimit = 15; - const close_message = `This has been closed since a request for information has \ - not been answered for ${numberOfDaysLimit} days. It can be reopened when the \ - requested information is provided.`; - - module.exports = async ({github, context}) => { - const owner = context.repo.owner; - const repo = context.repo.repo; - - const issues = await github.rest.issues.listForRepo({ - owner: owner, - repo: repo, - labels: 'needs more info', - }); - const numbers = issues.data.map((e) => e.number); - - for (const number of numbers) { - const events = await github.paginate( - github.rest.issues.listEventsForTimeline, - { - owner: owner, - repo: repo, - issue_number: number, - }, - (response) => response.data.filter(labeledEvent) - ); - - const latest_response_label = events[events.length - 1]; - - const created_at = new Date(latest_response_label.created_at); - const now = new Date(); - const diff = now - created_at; - const diffDays = diff / (1000 * 60 * 60 * 24); - - if (diffDays > numberOfDaysLimit) { - await github.rest.issues.update({ - owner: owner, - repo: repo, - issue_number: number, - state: 'closed', - }); - - await github.rest.issues.createComment({ - owner: owner, - repo: repo, - issue_number: number, - body: close_message, - }); - } + return data.event === "labeled" && data.label.name === "needs more info"; +} + +const numberOfDaysLimit = 15; +const close_message = `This has been closed since a request for information has \ +not been answered for ${numberOfDaysLimit} days. It can be reopened when the \ +requested information is provided.`; + +module.exports = async ({ github, context }) => { + const owner = context.repo.owner; + const repo = context.repo.repo; + + const issues = await github.rest.issues.listForRepo({ + owner: owner, + repo: repo, + labels: "needs more info", + }); + const numbers = issues.data.map((e) => e.number); + + for (const number of numbers) { + const events = await github.paginate( + github.rest.issues.listEventsForTimeline, + { + owner: owner, + repo: repo, + issue_number: number, + }, + (response) => response.data.filter(labeledEvent) + ); + + const latest_response_label = events[events.length - 1]; + + const created_at = new Date(latest_response_label.created_at); + const now = new Date(); + const diff = now - created_at; + const diffDays = diff / (1000 * 60 * 60 * 24); + + if (diffDays > numberOfDaysLimit) { + await github.rest.issues.update({ + owner: owner, + repo: repo, + issue_number: number, + state: "closed", + }); + + await github.rest.issues.createComment({ + owner: owner, + repo: repo, + issue_number: number, + body: close_message, + }); } - }; + } +}; \ No newline at end of file diff --git a/.github/scripts/fixtures/invalidIssueBody.txt b/.github/scripts/fixtures/invalidIssueBody.txt new file mode 100644 index 000000000..504bd6690 --- /dev/null +++ b/.github/scripts/fixtures/invalidIssueBody.txt @@ -0,0 +1,50 @@ +### Please make sure you have searched for information in the following guides. + +- [X] Search the issues already opened: https://github.com/GoogleCloudPlatform/google-cloud-node/issues +- [X] Search StackOverflow: http://stackoverflow.com/questions/tagged/google-cloud-platform+node.js +- [X] Check our Troubleshooting guide: https://googlecloudplatform.github.io/google-cloud-node/#/docs/guides/troubleshooting +- [X] Check our FAQ: https://googlecloudplatform.github.io/google-cloud-node/#/docs/guides/faq +- [X] Check our libraries HOW-TO: https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md +- [X] Check out our authentication guide: https://github.com/googleapis/google-auth-library-nodejs +- [X] Check out handwritten samples for many of our APIs: https://github.com/GoogleCloudPlatform/nodejs-docs-samples + +### A screenshot that you have tested with "Try this API". + + +N/A + +### Link to the code that reproduces this issue. A link to a **public** Github Repository or gist with a minimal reproduction. + +not-a-link + +### A step-by-step description of how to reproduce the issue, based on the linked reproduction. + + +Change MY_PROJECT to your project name, add credentials if needed and run. + +### A clear and concise description of what the bug is, and what you expected to happen. + +The application crashes with the following exception (which there is no way to catch). It should just emit error, and allow graceful handling. +TypeError [ERR_INVALID_ARG_TYPE]: The "chunk" argument must be of type string or an instance of Buffer or Uint8Array. Received an instance of Object + at _write (node:internal/streams/writable:474:13) + at Writable.write (node:internal/streams/writable:502:10) + at Duplexify._write (/project/node_modules/duplexify/index.js:212:22) + at doWrite (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:390:139) + at writeOrBuffer (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:381:5) + at Writable.write (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:302:11) + at Pumpify. (/project/node_modules/@google-cloud/speech/build/src/helpers.js:79:27) + at Object.onceWrapper (node:events:633:26) + at Pumpify.emit (node:events:518:28) + at obj. [as _write] (/project/node_modules/stubs/index.js:28:22) + at doWrite (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:390:139) + at writeOrBuffer (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:381:5) + at Writable.write (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:302:11) + at PassThrough.ondata (node:internal/streams/readable:1007:22) + at PassThrough.emit (node:events:518:28) + at addChunk (node:internal/streams/readable:559:12) { + code: 'ERR_INVALID_ARG_TYPE' + + +### A clear and concise description WHY you expect this behavior, i.e., was it a recent change, there is documentation that points to this behavior, etc. ** + +No library should crash an application this way. \ No newline at end of file diff --git a/.github/scripts/fixtures/validIssueBody.txt b/.github/scripts/fixtures/validIssueBody.txt new file mode 100644 index 000000000..6e0ace338 --- /dev/null +++ b/.github/scripts/fixtures/validIssueBody.txt @@ -0,0 +1,50 @@ +### Please make sure you have searched for information in the following guides. + +- [X] Search the issues already opened: https://github.com/GoogleCloudPlatform/google-cloud-node/issues +- [X] Search StackOverflow: http://stackoverflow.com/questions/tagged/google-cloud-platform+node.js +- [X] Check our Troubleshooting guide: https://googlecloudplatform.github.io/google-cloud-node/#/docs/guides/troubleshooting +- [X] Check our FAQ: https://googlecloudplatform.github.io/google-cloud-node/#/docs/guides/faq +- [X] Check our libraries HOW-TO: https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md +- [X] Check out our authentication guide: https://github.com/googleapis/google-auth-library-nodejs +- [X] Check out handwritten samples for many of our APIs: https://github.com/GoogleCloudPlatform/nodejs-docs-samples + +### A screenshot that you have tested with "Try this API". + + +N/A + +### Link to the code that reproduces this issue. A link to a **public** Github Repository or gist with a minimal reproduction. + +https://gist.github.com/orgads/13cbf44c91923da27d8772b5f10489c9 + +### A step-by-step description of how to reproduce the issue, based on the linked reproduction. + + +Change MY_PROJECT to your project name, add credentials if needed and run. + +### A clear and concise description of what the bug is, and what you expected to happen. + +The application crashes with the following exception (which there is no way to catch). It should just emit error, and allow graceful handling. +TypeError [ERR_INVALID_ARG_TYPE]: The "chunk" argument must be of type string or an instance of Buffer or Uint8Array. Received an instance of Object + at _write (node:internal/streams/writable:474:13) + at Writable.write (node:internal/streams/writable:502:10) + at Duplexify._write (/project/node_modules/duplexify/index.js:212:22) + at doWrite (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:390:139) + at writeOrBuffer (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:381:5) + at Writable.write (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:302:11) + at Pumpify. (/project/node_modules/@google-cloud/speech/build/src/helpers.js:79:27) + at Object.onceWrapper (node:events:633:26) + at Pumpify.emit (node:events:518:28) + at obj. [as _write] (/project/node_modules/stubs/index.js:28:22) + at doWrite (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:390:139) + at writeOrBuffer (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:381:5) + at Writable.write (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:302:11) + at PassThrough.ondata (node:internal/streams/readable:1007:22) + at PassThrough.emit (node:events:518:28) + at addChunk (node:internal/streams/readable:559:12) { + code: 'ERR_INVALID_ARG_TYPE' + + +### A clear and concise description WHY you expect this behavior, i.e., was it a recent change, there is documentation that points to this behavior, etc. ** + +No library should crash an application this way. \ No newline at end of file diff --git a/.github/scripts/fixtures/validIssueBodyDifferentLinkLocation.txt b/.github/scripts/fixtures/validIssueBodyDifferentLinkLocation.txt new file mode 100644 index 000000000..984a420e3 --- /dev/null +++ b/.github/scripts/fixtures/validIssueBodyDifferentLinkLocation.txt @@ -0,0 +1,50 @@ +### Please make sure you have searched for information in the following guides. + +- [X] Search the issues already opened: https://github.com/GoogleCloudPlatform/google-cloud-node/issues +- [X] Search StackOverflow: http://stackoverflow.com/questions/tagged/google-cloud-platform+node.js +- [X] Check our Troubleshooting guide: https://googlecloudplatform.github.io/google-cloud-node/#/docs/guides/troubleshooting +- [X] Check our FAQ: https://googlecloudplatform.github.io/google-cloud-node/#/docs/guides/faq +- [X] Check our libraries HOW-TO: https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md +- [X] Check out our authentication guide: https://github.com/googleapis/google-auth-library-nodejs +- [X] Check out handwritten samples for many of our APIs: https://github.com/GoogleCloudPlatform/nodejs-docs-samples + +### A screenshot that you have tested with "Try this API". + + +N/A + +### A step-by-step description of how to reproduce the issue, based on the linked reproduction. + + +Change MY_PROJECT to your project name, add credentials if needed and run. + +### A clear and concise description of what the bug is, and what you expected to happen. + +The application crashes with the following exception (which there is no way to catch). It should just emit error, and allow graceful handling. +TypeError [ERR_INVALID_ARG_TYPE]: The "chunk" argument must be of type string or an instance of Buffer or Uint8Array. Received an instance of Object + at _write (node:internal/streams/writable:474:13) + at Writable.write (node:internal/streams/writable:502:10) + at Duplexify._write (/project/node_modules/duplexify/index.js:212:22) + at doWrite (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:390:139) + at writeOrBuffer (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:381:5) + at Writable.write (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:302:11) + at Pumpify. (/project/node_modules/@google-cloud/speech/build/src/helpers.js:79:27) + at Object.onceWrapper (node:events:633:26) + at Pumpify.emit (node:events:518:28) + at obj. [as _write] (/project/node_modules/stubs/index.js:28:22) + at doWrite (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:390:139) + at writeOrBuffer (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:381:5) + at Writable.write (/project/node_modules/duplexify/node_modules/readable-stream/lib/_stream_writable.js:302:11) + at PassThrough.ondata (node:internal/streams/readable:1007:22) + at PassThrough.emit (node:events:518:28) + at addChunk (node:internal/streams/readable:559:12) { + code: 'ERR_INVALID_ARG_TYPE' + +### Link to the code that reproduces this issue. A link to a **public** Github Repository with a minimal reproduction. + + +https://gist.github.com/orgads/13cbf44c91923da27d8772b5f10489c9 + +### A clear and concise description WHY you expect this behavior, i.e., was it a recent change, there is documentation that points to this behavior, etc. ** + +No library should crash an application this way. \ No newline at end of file diff --git a/.github/scripts/remove-response-label.cjs b/.github/scripts/remove-response-label.cjs index 887cf349e..4a784ddf7 100644 --- a/.github/scripts/remove-response-label.cjs +++ b/.github/scripts/remove-response-label.cjs @@ -13,21 +13,21 @@ // limitations under the License. module.exports = async ({ github, context }) => { - const commenter = context.actor; - const issue = await github.rest.issues.get({ + const commenter = context.actor; + const issue = await github.rest.issues.get({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + }); + const author = issue.data.user.login; + const labels = issue.data.labels.map((e) => e.name); + + if (author === commenter && labels.includes("needs more info")) { + await github.rest.issues.removeLabel({ owner: context.repo.owner, repo: context.repo.repo, issue_number: context.issue.number, + name: "needs more info", }); - const author = issue.data.user.login; - const labels = issue.data.labels.map((e) => e.name); - - if (author === commenter && labels.includes('needs more info')) { - await github.rest.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - name: 'needs more info', - }); - } - }; + } +}; \ No newline at end of file diff --git a/.github/scripts/tests/close-invalid-link.test.cjs b/.github/scripts/tests/close-invalid-link.test.cjs new file mode 100644 index 000000000..f63ee89c8 --- /dev/null +++ b/.github/scripts/tests/close-invalid-link.test.cjs @@ -0,0 +1,86 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +'use strict'; + +const { describe, it } = require('mocha'); +const closeInvalidLink = require('../close-invalid-link.cjs'); +const fs = require('fs'); +const sinon = require('sinon'); + +describe('close issues with invalid links', () => { + let octokitStub; + let issuesStub; + + beforeEach(() => { + issuesStub = { + get: sinon.stub(), + createComment: sinon.stub(), + update: sinon.stub(), + }; + octokitStub = { + rest: { + issues: issuesStub, + }, + }; + }); + + afterEach(() => { + sinon.restore(); + }); + + it('does not do anything if it is not a bug', async () => { + const context = { repo: { owner: 'testOrg', repo: 'testRepo' }, issue: { number: 1 } }; + issuesStub.get.resolves({ data: { body: "I'm having a problem with this." } }); + + await closeInvalidLink({ github: octokitStub, context }); + + sinon.assert.calledOnce(issuesStub.get); + sinon.assert.notCalled(issuesStub.createComment); + sinon.assert.notCalled(issuesStub.update); + }); + + it('does not do anything if it is a bug with an appropriate link', async () => { + const context = { repo: { owner: 'testOrg', repo: 'testRepo' }, issue: { number: 1 } }; + issuesStub.get.resolves({ data: { body: fs.readFileSync('./fixtures/validIssueBody.txt', 'utf-8') } }); + + await closeInvalidLink({ github: octokitStub, context }); + + sinon.assert.calledOnce(issuesStub.get); + sinon.assert.notCalled(issuesStub.createComment); + sinon.assert.notCalled(issuesStub.update); + }); + + it('does not do anything if it is a bug with an appropriate link and the template changes', async () => { + const context = { repo: { owner: 'testOrg', repo: 'testRepo' }, issue: { number: 1 } }; + issuesStub.get.resolves({ data: { body: fs.readFileSync('./fixtures/validIssueBodyDifferentLinkLocation.txt', 'utf-8') } }); + + await closeInvalidLink({ github: octokitStub, context }); + + sinon.assert.calledOnce(issuesStub.get); + sinon.assert.notCalled(issuesStub.createComment); + sinon.assert.notCalled(issuesStub.update); + }); + + it('closes the issue if the link is invalid', async () => { + const context = { repo: { owner: 'testOrg', repo: 'testRepo' }, issue: { number: 1 } }; + issuesStub.get.resolves({ data: { body: fs.readFileSync('./fixtures/invalidIssueBody.txt', 'utf-8') } }); + + await closeInvalidLink({ github: octokitStub, context }); + + sinon.assert.calledOnce(issuesStub.get); + sinon.assert.calledOnce(issuesStub.createComment); + sinon.assert.calledOnce(issuesStub.update); + }); +}); \ No newline at end of file diff --git a/.github/scripts/tests/close-or-remove-response-label.test.cjs b/.github/scripts/tests/close-or-remove-response-label.test.cjs new file mode 100644 index 000000000..fb092c536 --- /dev/null +++ b/.github/scripts/tests/close-or-remove-response-label.test.cjs @@ -0,0 +1,109 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +'use strict'; + +const { describe, it, beforeEach, afterEach } = require('mocha'); +const removeResponseLabel = require('../remove-response-label.cjs'); +const closeUnresponsive = require('../close-unresponsive.cjs'); +const sinon = require('sinon'); + +function getISODateDaysAgo(days) { + const today = new Date(); + const daysAgo = new Date(today.setDate(today.getDate() - days)); + return daysAgo.toISOString(); +} + +describe('close issues or remove needs more info labels', () => { + let octokitStub; + let issuesStub; + let paginateStub; + + beforeEach(() => { + issuesStub = { + listForRepo: sinon.stub(), + update: sinon.stub(), + createComment: sinon.stub(), + get: sinon.stub(), + removeLabel: sinon.stub(), + }; + paginateStub = sinon.stub(); + octokitStub = { + rest: { + issues: issuesStub, + }, + paginate: paginateStub, + }; + }); + + afterEach(() => { + sinon.restore(); + }); + + it('closes the issue if the OP has not responded within the allotted time and there is a needs-more-info label', async () => { + const context = { owner: 'testOrg', repo: 'testRepo' }; + const issuesInRepo = [{ user: { login: 'OP' }, labels: [{ name: 'needs more info' }] }]; + const eventsInIssue = [{ event: 'labeled', label: { name: 'needs more info' }, created_at: getISODateDaysAgo(16) }]; + + issuesStub.listForRepo.resolves({ data: issuesInRepo }); + paginateStub.resolves(eventsInIssue); + + await closeUnresponsive({ github: octokitStub, context }); + + sinon.assert.calledOnce(issuesStub.listForRepo); + sinon.assert.calledOnce(paginateStub); + sinon.assert.calledOnce(issuesStub.update); + sinon.assert.calledOnce(issuesStub.createComment); + }); + + it('does nothing if not enough time has passed and there is a needs-more-info label', async () => { + const context = { owner: 'testOrg', repo: 'testRepo' }; + const issuesInRepo = [{ user: { login: 'OP' }, labels: [{ name: 'needs more info' }] }]; + const eventsInIssue = [{ event: 'labeled', label: { name: 'needs more info' }, created_at: getISODateDaysAgo(14) }]; + + issuesStub.listForRepo.resolves({ data: issuesInRepo }); + paginateStub.resolves(eventsInIssue); + + await closeUnresponsive({ github: octokitStub, context }); + + sinon.assert.calledOnce(issuesStub.listForRepo); + sinon.assert.calledOnce(paginateStub); + sinon.assert.notCalled(issuesStub.update); + sinon.assert.notCalled(issuesStub.createComment); + }); + + it('removes the label if OP responded', async () => { + const context = { actor: 'OP', repo: { owner: 'testOrg', repo: 'testRepo' }, issue: { number: 1 } }; + const issueContext = { user: {login: 'OP'}, labels: [{ name: 'needs more info' }] }; + + issuesStub.get.resolves({ data: issueContext }); + + await removeResponseLabel({ github: octokitStub, context }); + + sinon.assert.calledOnce(issuesStub.get); + sinon.assert.calledOnce(issuesStub.removeLabel); + }); + + it('does not remove the label if author responded', async () => { + const context = { actor: 'repo-maintainer', repo: { owner: 'testOrg', repo: 'testRepo' }, issue: { number: 1 } }; + const issueContext = { user: {login: 'OP'}, labels: [{ name: 'needs more info' }] }; + + issuesStub.get.resolves({ data: issueContext }); + + await removeResponseLabel({ github: octokitStub, context }); + + sinon.assert.calledOnce(issuesStub.get); + sinon.assert.notCalled(issuesStub.removeLabel); + }); +}); \ No newline at end of file diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 18ec4115b..f547520d5 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -9,10 +9,10 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - node: [14, 16, 18, 20] + node: [14, 16, 18] steps: - - uses: actions/checkout@v3 - - uses: actions/setup-node@v3 + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 with: node-version: ${{ matrix.node }} - run: node --version @@ -29,10 +29,10 @@ jobs: windows: runs-on: windows-latest steps: - - uses: actions/checkout@v3 - - uses: actions/setup-node@v3 + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 with: - node-version: 14 + node-version: 18 - run: npm install --engine-strict - run: npm test env: @@ -40,9 +40,9 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - uses: actions/setup-node@v3 + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 with: - node-version: 14 + node-version: 18 - run: npm install - run: npm run lint diff --git a/.github/workflows/issues-no-repro.yaml b/.github/workflows/issues-no-repro.yaml index 442a46bcc..531054022 100644 --- a/.github/workflows/issues-no-repro.yaml +++ b/.github/workflows/issues-no-repro.yaml @@ -10,7 +10,12 @@ jobs: issues: write pull-requests: write steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 + - uses: actions/setup-node@v4 + with: + node-version: 18 + - run: npm install + working-directory: ./.github/scripts - uses: actions/github-script@v7 with: script: | diff --git a/.github/workflows/response.yaml b/.github/workflows/response.yaml index 6ed37326f..e81a3603a 100644 --- a/.github/workflows/response.yaml +++ b/.github/workflows/response.yaml @@ -13,7 +13,7 @@ jobs: issues: write pull-requests: write steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 - uses: actions/github-script@v7 with: script: | @@ -27,7 +27,7 @@ jobs: issues: write pull-requests: write steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 - uses: actions/github-script@v7 with: script: | diff --git a/.kokoro/common.cfg b/.kokoro/common.cfg index 4ee7584a3..713afb2ec 100644 --- a/.kokoro/common.cfg +++ b/.kokoro/common.cfg @@ -16,7 +16,7 @@ build_file: "nodejs-storage/.kokoro/trampoline_v2.sh" # Configure the docker image for kokoro-trampoline. env_vars: { key: "TRAMPOLINE_IMAGE" - value: "gcr.io/cloud-devrel-kokoro-resources/node:14-user" + value: "gcr.io/cloud-devrel-kokoro-resources/node:18-user" } env_vars: { key: "TRAMPOLINE_BUILD_FILE" diff --git a/.kokoro/continuous/node18/common.cfg b/.kokoro/continuous/node18/common.cfg new file mode 100644 index 000000000..713afb2ec --- /dev/null +++ b/.kokoro/continuous/node18/common.cfg @@ -0,0 +1,24 @@ +# Format: //devtools/kokoro/config/proto/build.proto + +# Build logs will be here +action { + define_artifacts { + regex: "**/*sponge_log.xml" + } +} + +# Download trampoline resources. +gfile_resources: "/bigstore/cloud-devrel-kokoro-resources/trampoline" + +# Use the trampoline script to run in docker. +build_file: "nodejs-storage/.kokoro/trampoline_v2.sh" + +# Configure the docker image for kokoro-trampoline. +env_vars: { + key: "TRAMPOLINE_IMAGE" + value: "gcr.io/cloud-devrel-kokoro-resources/node:18-user" +} +env_vars: { + key: "TRAMPOLINE_BUILD_FILE" + value: "github/nodejs-storage/.kokoro/test.sh" +} diff --git a/.kokoro/continuous/node18/lint.cfg b/.kokoro/continuous/node18/lint.cfg new file mode 100644 index 000000000..72c4ad241 --- /dev/null +++ b/.kokoro/continuous/node18/lint.cfg @@ -0,0 +1,4 @@ +env_vars: { + key: "TRAMPOLINE_BUILD_FILE" + value: "github/nodejs-storage/.kokoro/lint.sh" +} diff --git a/.kokoro/continuous/node18/samples-test.cfg b/.kokoro/continuous/node18/samples-test.cfg new file mode 100644 index 000000000..2137cefc0 --- /dev/null +++ b/.kokoro/continuous/node18/samples-test.cfg @@ -0,0 +1,12 @@ +# Download resources for system tests (service account key, etc.) +gfile_resources: "/bigstore/cloud-devrel-kokoro-resources/google-cloud-nodejs" + +env_vars: { + key: "TRAMPOLINE_BUILD_FILE" + value: "github/nodejs-storage/.kokoro/samples-test.sh" +} + +env_vars: { + key: "SECRET_MANAGER_KEYS" + value: "long-door-651-kokoro-system-test-service-account" +} \ No newline at end of file diff --git a/.kokoro/continuous/node18/system-test.cfg b/.kokoro/continuous/node18/system-test.cfg new file mode 100644 index 000000000..9ba9ad1c7 --- /dev/null +++ b/.kokoro/continuous/node18/system-test.cfg @@ -0,0 +1,12 @@ +# Download resources for system tests (service account key, etc.) +gfile_resources: "/bigstore/cloud-devrel-kokoro-resources/google-cloud-nodejs" + +env_vars: { + key: "TRAMPOLINE_BUILD_FILE" + value: "github/nodejs-storage/.kokoro/system-test.sh" +} + +env_vars: { + key: "SECRET_MANAGER_KEYS" + value: "long-door-651-kokoro-system-test-service-account" +} \ No newline at end of file diff --git a/.kokoro/continuous/node18/test.cfg b/.kokoro/continuous/node18/test.cfg new file mode 100644 index 000000000..e69de29bb diff --git a/.kokoro/presubmit/node18/common.cfg b/.kokoro/presubmit/node18/common.cfg new file mode 100644 index 000000000..713afb2ec --- /dev/null +++ b/.kokoro/presubmit/node18/common.cfg @@ -0,0 +1,24 @@ +# Format: //devtools/kokoro/config/proto/build.proto + +# Build logs will be here +action { + define_artifacts { + regex: "**/*sponge_log.xml" + } +} + +# Download trampoline resources. +gfile_resources: "/bigstore/cloud-devrel-kokoro-resources/trampoline" + +# Use the trampoline script to run in docker. +build_file: "nodejs-storage/.kokoro/trampoline_v2.sh" + +# Configure the docker image for kokoro-trampoline. +env_vars: { + key: "TRAMPOLINE_IMAGE" + value: "gcr.io/cloud-devrel-kokoro-resources/node:18-user" +} +env_vars: { + key: "TRAMPOLINE_BUILD_FILE" + value: "github/nodejs-storage/.kokoro/test.sh" +} diff --git a/.kokoro/presubmit/node18/samples-test.cfg b/.kokoro/presubmit/node18/samples-test.cfg new file mode 100644 index 000000000..2137cefc0 --- /dev/null +++ b/.kokoro/presubmit/node18/samples-test.cfg @@ -0,0 +1,12 @@ +# Download resources for system tests (service account key, etc.) +gfile_resources: "/bigstore/cloud-devrel-kokoro-resources/google-cloud-nodejs" + +env_vars: { + key: "TRAMPOLINE_BUILD_FILE" + value: "github/nodejs-storage/.kokoro/samples-test.sh" +} + +env_vars: { + key: "SECRET_MANAGER_KEYS" + value: "long-door-651-kokoro-system-test-service-account" +} \ No newline at end of file diff --git a/.kokoro/presubmit/node18/system-test.cfg b/.kokoro/presubmit/node18/system-test.cfg new file mode 100644 index 000000000..9ba9ad1c7 --- /dev/null +++ b/.kokoro/presubmit/node18/system-test.cfg @@ -0,0 +1,12 @@ +# Download resources for system tests (service account key, etc.) +gfile_resources: "/bigstore/cloud-devrel-kokoro-resources/google-cloud-nodejs" + +env_vars: { + key: "TRAMPOLINE_BUILD_FILE" + value: "github/nodejs-storage/.kokoro/system-test.sh" +} + +env_vars: { + key: "SECRET_MANAGER_KEYS" + value: "long-door-651-kokoro-system-test-service-account" +} \ No newline at end of file diff --git a/.kokoro/presubmit/node18/test.cfg b/.kokoro/presubmit/node18/test.cfg new file mode 100644 index 000000000..e69de29bb diff --git a/.kokoro/release/docs-devsite.cfg b/.kokoro/release/docs-devsite.cfg index eea22f239..c98f038de 100644 --- a/.kokoro/release/docs-devsite.cfg +++ b/.kokoro/release/docs-devsite.cfg @@ -11,7 +11,7 @@ before_action { # doc publications use a Python image. env_vars: { key: "TRAMPOLINE_IMAGE" - value: "gcr.io/cloud-devrel-kokoro-resources/node:14-user" + value: "gcr.io/cloud-devrel-kokoro-resources/node:18-user" } # Download trampoline resources. diff --git a/.kokoro/release/docs.cfg b/.kokoro/release/docs.cfg index 25c377eef..71c80e6b0 100644 --- a/.kokoro/release/docs.cfg +++ b/.kokoro/release/docs.cfg @@ -11,7 +11,7 @@ before_action { # doc publications use a Python image. env_vars: { key: "TRAMPOLINE_IMAGE" - value: "gcr.io/cloud-devrel-kokoro-resources/node:14-user" + value: "gcr.io/cloud-devrel-kokoro-resources/node:18-user" } # Download trampoline resources. diff --git a/.kokoro/release/docs.sh b/.kokoro/release/docs.sh index 1d8f3f490..e9079a605 100755 --- a/.kokoro/release/docs.sh +++ b/.kokoro/release/docs.sh @@ -16,7 +16,7 @@ set -eo pipefail -# build jsdocs (Python is installed on the Node 10 docker image). +# build jsdocs (Python is installed on the Node 18 docker image). if [[ -z "$CREDENTIALS" ]]; then # if CREDENTIALS are explicitly set, assume we're testing locally # and don't set NPM_CONFIG_PREFIX. diff --git a/.kokoro/samples-test.sh b/.kokoro/samples-test.sh index 8c5d108cb..d195fbf7c 100755 --- a/.kokoro/samples-test.sh +++ b/.kokoro/samples-test.sh @@ -18,6 +18,10 @@ set -eo pipefail export NPM_CONFIG_PREFIX=${HOME}/.npm-global +# Ensure the npm global directory is writable, otherwise rebuild `npm` +mkdir -p $NPM_CONFIG_PREFIX +npm config -g ls || npm i -g npm@`npm --version` + # Setup service account credentials. export GOOGLE_APPLICATION_CREDENTIALS=${KOKORO_GFILE_DIR}/secret_manager/long-door-651-kokoro-system-test-service-account export GCLOUD_PROJECT=long-door-651 @@ -56,7 +60,7 @@ fi # codecov combines coverage across integration and unit tests. Include # the logic below for any environment you wish to collect coverage for: -COVERAGE_NODE=14 +COVERAGE_NODE=18 if npx check-node-version@3.3.0 --silent --node $COVERAGE_NODE; then NYC_BIN=./node_modules/nyc/bin/nyc.js if [ -f "$NYC_BIN" ]; then diff --git a/.kokoro/test.bat b/.kokoro/test.bat index 0bb124052..caf825656 100644 --- a/.kokoro/test.bat +++ b/.kokoro/test.bat @@ -21,7 +21,7 @@ cd .. @rem we upgrade Node.js in the image: SET PATH=%PATH%;/cygdrive/c/Program Files/nodejs/npm -call nvm use v14.17.3 +call nvm use 18 call which node call npm install || goto :error diff --git a/.kokoro/test.sh b/.kokoro/test.sh index 862d478d3..0d9f6392a 100755 --- a/.kokoro/test.sh +++ b/.kokoro/test.sh @@ -39,7 +39,7 @@ npm test # codecov combines coverage across integration and unit tests. Include # the logic below for any environment you wish to collect coverage for: -COVERAGE_NODE=14 +COVERAGE_NODE=18 if npx check-node-version@3.3.0 --silent --node $COVERAGE_NODE; then NYC_BIN=./node_modules/nyc/bin/nyc.js if [ -f "$NYC_BIN" ]; then diff --git a/.kokoro/trampoline_v2.sh b/.kokoro/trampoline_v2.sh index 4d0311212..5d6cfcca5 100755 --- a/.kokoro/trampoline_v2.sh +++ b/.kokoro/trampoline_v2.sh @@ -44,7 +44,7 @@ # the project root. # # Here is an example for running this script. -# TRAMPOLINE_IMAGE=gcr.io/cloud-devrel-kokoro-resources/node:10-user \ +# TRAMPOLINE_IMAGE=gcr.io/cloud-devrel-kokoro-resources/node:18-user \ # TRAMPOLINE_BUILD_FILE=.kokoro/system-test.sh \ # .kokoro/trampoline_v2.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 97e95c736..391562a0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ [1]: https://www.npmjs.com/package/@google-cloud/storage?activeTab=versions +## [7.17.2](https://github.com/googleapis/nodejs-storage/compare/v7.17.1...v7.17.2) (2025-10-06) + + +### Bug Fixes + +* Common Service: should retry a request failed ([#2652](https://github.com/googleapis/nodejs-storage/issues/2652)) ([b38b5d2](https://github.com/googleapis/nodejs-storage/commit/b38b5d221f2cb72658c1eb4a726315ab395a542c)) +* Implement path containment to prevent traversal attacks ([#2654](https://github.com/googleapis/nodejs-storage/issues/2654)) ([08d7abf](https://github.com/googleapis/nodejs-storage/commit/08d7abf32dd365b24ce34c66174be06c30bfce8f)) + ## [7.17.1](https://github.com/googleapis/nodejs-storage/compare/v7.17.0...v7.17.1) (2025-08-27) diff --git a/README.md b/README.md index 7e7979a40..81047c5e9 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ # [Google Cloud Storage: Node.js Client](https://github.com/googleapis/nodejs-storage) [![release level](https://img.shields.io/badge/release%20level-stable-brightgreen.svg?style=flat)](https://cloud.google.com/terms/launch-stages) -[![npm version](https://img.shields.io/npm/v/@google-cloud/storage.svg)](https://www.npmjs.org/package/@google-cloud/storage) +[![npm version](https://img.shields.io/npm/v/@google-cloud/storage.svg)](https://www.npmjs.com/package/@google-cloud/storage) diff --git a/owlbot.py b/owlbot.py index 3df451978..0607a2f0c 100644 --- a/owlbot.py +++ b/owlbot.py @@ -30,7 +30,8 @@ '.kokoro/continuous/node14/system-test.cfg', '.kokoro/presubmit/node14/system-test.cfg', '.kokoro/release/publish.cfg', - '.kokoro/system-test.sh' + '.kokoro/system-test.sh', + '.kokoro/samples-test.sh', ]) # Create .config directory under $HOME to get around permissions issues diff --git a/package.json b/package.json index 23434caf0..568f927a6 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@google-cloud/storage", "description": "Cloud Storage Client Library for Node.js", - "version": "7.17.1", + "version": "7.17.2", "license": "Apache-2.0", "author": "Google Inc.", "engines": { diff --git a/renovate.json b/renovate.json index c5c702cf4..f39fd3232 100644 --- a/renovate.json +++ b/renovate.json @@ -15,6 +15,10 @@ { "extends": "packages:linters", "groupName": "linters" + }, + { + "matchManagers": ["github-actions"], + "enabled": false } ], "ignoreDeps": ["typescript"] diff --git a/samples/package.json b/samples/package.json index 6b1c10d2e..26266855f 100644 --- a/samples/package.json +++ b/samples/package.json @@ -17,7 +17,7 @@ }, "dependencies": { "@google-cloud/pubsub": "^4.0.0", - "@google-cloud/storage": "^7.17.1", + "@google-cloud/storage": "^7.17.2", "node-fetch": "^2.6.7", "uuid": "^8.0.0", "yargs": "^16.0.0" diff --git a/samples/system-test/transfer-manager.test.js b/samples/system-test/transfer-manager.test.js index f6180bed3..9f51c3598 100644 --- a/samples/system-test/transfer-manager.test.js +++ b/samples/system-test/transfer-manager.test.js @@ -56,26 +56,37 @@ describe('transfer manager', () => { ); }); - it('should download mulitple files', async () => { + it('should download multiple files', async () => { + // Remove absolute path marker to prepare for joining/validation. + const expectedFirstFilePath = firstFilePath.startsWith('/') + ? firstFilePath.slice(1) + : firstFilePath; + const expectedSecondFilePath = secondFilePath.startsWith('/') + ? secondFilePath.slice(1) + : secondFilePath; const output = execSync( - `node downloadManyFilesWithTransferManager.js ${bucketName} ${firstFilePath} ${secondFilePath}` + `node downloadManyFilesWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${expectedSecondFilePath}` ); assert.match( output, new RegExp( - `gs://${bucketName}/${firstFilePath} downloaded to ${firstFilePath}.\ngs://${bucketName}/${secondFilePath} downloaded to ${secondFilePath}.` + `gs://${bucketName}/${expectedFirstFilePath} downloaded to ${expectedFirstFilePath}.\ngs://${bucketName}/${expectedSecondFilePath} downloaded to ${expectedSecondFilePath}.` ) ); }); it('should download a file utilizing chunked download', async () => { + // Remove absolute path marker to prepare for joining/validation. + const expectedFirstFilePath = firstFilePath.startsWith('/') + ? firstFilePath.slice(1) + : firstFilePath; const output = execSync( - `node downloadFileInChunksWithTransferManager.js ${bucketName} ${firstFilePath} ${downloadFilePath} ${chunkSize}` + `node downloadFileInChunksWithTransferManager.js ${bucketName} ${expectedFirstFilePath} ${downloadFilePath} ${chunkSize}` ); assert.match( output, new RegExp( - `gs://${bucketName}/${firstFilePath} downloaded to ${downloadFilePath}.` + `gs://${bucketName}/${expectedFirstFilePath} downloaded to ${downloadFilePath}.` ) ); }); diff --git a/src/file.ts b/src/file.ts index 0d18458bb..2aa4ec7e1 100644 --- a/src/file.ts +++ b/src/file.ts @@ -545,6 +545,9 @@ export enum FileExceptionMessages { To be sure the content is the same, you should try uploading the file again.`, MD5_RESUMED_UPLOAD = 'MD5 cannot be used with a continued resumable upload as MD5 cannot be extended from an existing value', MISSING_RESUME_CRC32C_FINAL_UPLOAD = 'The CRC32C is missing for the final portion of a resumed upload, which is required for validation. Please provide `resumeCRC32C` if validation is required, or disable `validation`.', + ABSOLUTE_FILE_NAME = 'Object name is an absolute path. Security block to prevent arbitrary file writes.', + TRAVERSAL_OUTSIDE_BASE = 'Path traversal detected. Security block to prevent writing outside the base directory.', + TRAVERSAL_OUTSIDE_BASE_DESTINATION = "The provided destination path is unsafe and attempts to traverse outside the application's base directory (current working directory).", } /** diff --git a/src/transfer-manager.ts b/src/transfer-manager.ts index dd4e41eeb..db7dd4fc6 100644 --- a/src/transfer-manager.ts +++ b/src/transfer-manager.ts @@ -497,9 +497,16 @@ export class TransferManager { [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.UPLOAD_MANY, }; - passThroughOptionsCopy.destination = options.customDestinationBuilder - ? options.customDestinationBuilder(filePath, options) - : filePath.split(path.sep).join(path.posix.sep); + if (options.customDestinationBuilder) { + passThroughOptionsCopy.destination = options.customDestinationBuilder( + filePath, + options + ); + } else { + let segments = filePath.split(path.sep); + segments = segments.filter(s => s !== ''); + passThroughOptionsCopy.destination = path.posix.join(...segments); + } if (options.prefix) { passThroughOptionsCopy.destination = path.posix.join( ...options.prefix.split(path.sep), @@ -587,27 +594,61 @@ export class TransferManager { }); } + const baseDir = this._resolveAndValidateBaseDir(options); + const stripRegexString = options.stripPrefix ? `^${options.stripPrefix}` : EMPTY_REGEX; const regex = new RegExp(stripRegexString, 'g'); + const createdDirectories = new Set(); for (const file of files) { + let name = file.name; + + // Apply stripPrefix first if requested + if (options.stripPrefix) { + name = name.replace(regex, ''); + } + + // This ensures the full intended relative path is validated. + if (options.prefix) { + name = path.join(options.prefix, name); + } + + // Reject absolute paths and traversal sequences + if (path.isAbsolute(name)) { + const absolutePathError = new RequestError( + FileExceptionMessages.ABSOLUTE_FILE_NAME + ); + throw absolutePathError; + } + + // Resolve the final path and perform the containment check + let finalPath = path.resolve(baseDir, name); + const normalizedBaseDir = baseDir.endsWith(path.sep) + ? baseDir + : baseDir + path.sep; + if (finalPath !== baseDir && !finalPath.startsWith(normalizedBaseDir)) { + const traversalError = new RequestError( + FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE + ); + throw traversalError; + } + + if (file.name.endsWith('/') && !finalPath.endsWith(path.sep)) { + finalPath = finalPath + path.sep; + } + const passThroughOptionsCopy = { ...options.passthroughOptions, + destination: finalPath, [GCCL_GCS_CMD_KEY]: GCCL_GCS_CMD_FEATURE.DOWNLOAD_MANY, }; - if (options.prefix || passThroughOptionsCopy.destination) { - passThroughOptionsCopy.destination = path.join( - options.prefix || '', - passThroughOptionsCopy.destination || '', - file.name - ); - } - if (options.stripPrefix) { - passThroughOptionsCopy.destination = file.name.replace(regex, ''); - } + const destinationDir = finalPath.endsWith(path.sep) + ? finalPath + : path.dirname(finalPath); + if ( options.skipIfExists && existsSync(passThroughOptionsCopy.destination || '') @@ -618,8 +659,12 @@ export class TransferManager { promises.push( limit(async () => { const destination = passThroughOptionsCopy.destination; + if (!createdDirectories.has(destinationDir)) { + // If not, create it and add it to the set for tracking + await fsp.mkdir(destinationDir, {recursive: true}); + createdDirectories.add(destinationDir); + } if (destination && destination.endsWith(path.sep)) { - await fsp.mkdir(destination, {recursive: true}); return Promise.resolve([ Buffer.alloc(0), ]) as Promise; @@ -867,4 +912,34 @@ export class TransferManager { : yield fullPath; } } + + /** + * Resolves the absolute base directory for downloads and validates it against + * the current working directory (CWD) to prevent path traversal outside the base destination. + * @param options The download options, potentially containing passthroughOptions.destination. + * @returns The absolute, validated base directory path (baseDir). + */ + private _resolveAndValidateBaseDir( + options: DownloadManyFilesOptions + ): string { + const cwd = process.cwd(); + + // Resolve baseDir, defaulting to CWD if no destination is provided + const baseDir = path.resolve( + options.passthroughOptions?.destination ?? cwd + ); + + // Check for path traversal: baseDir must be equal to or contained within cwd. + const relativeBaseDir = path.relative(cwd, baseDir); + + // The condition checks for traversal ('..') or cross-drive traversal (absolute path on Windows) + if (relativeBaseDir.startsWith('..') || path.isAbsolute(relativeBaseDir)) { + const traversalError = new RequestError( + FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE_DESTINATION + ); + throw traversalError; + } + + return baseDir; + } } diff --git a/system-test/common.ts b/system-test/common.ts index 0288143c6..17bc59b61 100644 --- a/system-test/common.ts +++ b/system-test/common.ts @@ -57,7 +57,10 @@ describe('Common', () => { }); it('should retry a request', function (done) { - this.timeout(60 * 1000); + // We've increased the timeout to accommodate the retry backoff strategy. + // The test's retry attempts and the delay between them can exceed the default timeout, + // causing a false negative (test failure due to timeout instead of a logic error). + this.timeout(90 * 1000); let numRequestAttempts = 0; diff --git a/test/transfer-manager.ts b/test/transfer-manager.ts index 2582782fa..2ab91ec1f 100644 --- a/test/transfer-manager.ts +++ b/test/transfer-manager.ts @@ -41,6 +41,7 @@ import fs from 'fs'; import {promises as fsp, Stats} from 'fs'; import * as sinon from 'sinon'; +import {FileExceptionMessages, RequestError} from '../src/file.js'; describe('Transfer Manager', () => { const BUCKET_NAME = 'test-bucket'; @@ -218,7 +219,10 @@ describe('Transfer Manager', () => { it('sets the destination correctly when provided a prefix', async () => { const prefix = 'test-prefix'; const filename = 'first.txt'; - const expectedDestination = path.normalize(`${prefix}/${filename}`); + const expectedDestination = path.resolve( + process.cwd(), + path.join(prefix, filename) + ); const file = new File(bucket, filename); sandbox.stub(file, 'download').callsFake(options => { @@ -233,7 +237,7 @@ describe('Transfer Manager', () => { it('sets the destination correctly when provided a strip prefix', async () => { const stripPrefix = 'should-be-removed/'; const filename = 'should-be-removed/first.txt'; - const expectedDestination = 'first.txt'; + const expectedDestination = path.resolve(process.cwd(), 'first.txt'); const file = new File(bucket, filename); sandbox.stub(file, 'download').callsFake(options => { @@ -263,8 +267,10 @@ describe('Transfer Manager', () => { destination: 'test-destination', }; const filename = 'first.txt'; - const expectedDestination = path.normalize( - `${passthroughOptions.destination}/${filename}` + const expectedDestination = path.resolve( + process.cwd(), + passthroughOptions.destination, + filename ); const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { @@ -280,6 +286,57 @@ describe('Transfer Manager', () => { await transferManager.downloadManyFiles([file], {passthroughOptions}); }); + it('should throws an error for absolute file names', async () => { + const expectedErr = new RequestError( + FileExceptionMessages.ABSOLUTE_FILE_NAME + ); + const maliciousFilename = '/etc/passwd'; + const file = new File(bucket, maliciousFilename); + + await assert.rejects( + transferManager.downloadManyFiles([file]), + expectedErr + ); + }); + + it('should throw an error for path traversal in destination', async () => { + const expectedErr = new RequestError( + FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE_DESTINATION + ); + const passthroughOptions = { + destination: '../traversal-destination', + }; + const file = new File(bucket, 'first.txt'); + await assert.rejects( + transferManager.downloadManyFiles([file], {passthroughOptions}), + expectedErr + ); + }); + + it('should throw an error for path traversal in file name', async () => { + const expectedErr = new RequestError( + FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE + ); + const file = new File(bucket, '../traversal-filename.txt'); + await assert.rejects( + transferManager.downloadManyFiles([file]), + expectedErr + ); + }); + + it('should throw an error for path traversal using prefix', async () => { + const expectedErr = new RequestError( + FileExceptionMessages.TRAVERSAL_OUTSIDE_BASE + ); + const file = new File(bucket, 'first.txt'); + await assert.rejects( + transferManager.downloadManyFiles([file], { + prefix: '../traversal-prefix', + }), + expectedErr + ); + }); + it('does not download files that already exist locally when skipIfExists is true', async () => { const firstFile = new File(bucket, 'first.txt'); sandbox.stub(firstFile, 'download').callsFake(options => { @@ -301,14 +358,16 @@ describe('Transfer Manager', () => { await transferManager.downloadManyFiles(files, options); }); - it('does not set the destination when prefix, strip prefix and passthroughOptions.destination are not provided', async () => { + it('sets the destination to CWD when prefix, strip prefix and passthroughOptions.destination are not provided', async () => { const options = {}; const filename = 'first.txt'; + const expectedDestination = path.resolve(process.cwd(), filename); + const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { optionsOrCb(null, Buffer.alloc(0)); } else if (optionsOrCb) { - assert.strictEqual(optionsOrCb.destination, undefined); + assert.strictEqual(optionsOrCb.destination, expectedDestination); } return Promise.resolve([Buffer.alloc(0)]) as Promise; }; @@ -321,10 +380,16 @@ describe('Transfer Manager', () => { it('should recursively create directory and write file contents if destination path is nested', async () => { const prefix = 'text-prefix'; const folder = 'nestedFolder/'; - const file = 'first.txt'; - const filesOrFolder = [folder, path.join(folder, file)]; - const expectedFilePath = path.join(prefix, folder, file); - const expectedDir = path.join(prefix, folder); + const filename = 'first.txt'; + const filesOrFolder = [folder, path.join(folder, filename)]; + const dirNameWithPrefix = path.join(prefix, folder); + const normalizedDir = path.resolve(process.cwd(), dirNameWithPrefix); + const expectedDir = normalizedDir + path.sep; + const expectedFilePath = path.resolve( + process.cwd(), + path.join(prefix, folder, filename) + ); + const mkdirSpy = sandbox.spy(fsp, 'mkdir'); const download = (optionsOrCb?: DownloadOptions | DownloadCallback) => { if (typeof optionsOrCb === 'function') { @@ -335,16 +400,21 @@ describe('Transfer Manager', () => { return Promise.resolve([Buffer.alloc(0)]) as Promise; }; - sandbox.stub(bucket, 'file').callsFake(filename => { - const file = new File(bucket, filename); - file.download = download; + sandbox.stub(bucket, 'file').callsFake(objectName => { + const file = new File(bucket, objectName); + if (objectName === path.join(folder, filename)) { + file.download = download; + } else { + file.download = () => + Promise.resolve([Buffer.alloc(0)]) as Promise; + } return file; }); await transferManager.downloadManyFiles(filesOrFolder, { prefix: prefix, }); assert.strictEqual( - mkdirSpy.calledOnceWith(expectedDir, { + mkdirSpy.calledWith(expectedDir, { recursive: true, }), true