Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sanity check for cml-publish with filesystem paths #427

Merged
merged 4 commits into from
Mar 8, 2021

Conversation

0x2b3bfa0
Copy link
Member

Resolves #308 and probably also closes #401 by throwing a meaningful error for paths that don't exist, aren't files or can't be read.

Closes #308 and probably also closes #401
Co-authored-by: Restyled.io <[email protected]>
src/utils.js Outdated Show resolved Hide resolved
@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented Mar 4, 2021

Let me check this. But the issue is that in the command line there is no difference between a buffer and a path. I mean you can do:

  • cml-publish path
  • pipe a buffer

@0x2b3bfa0
Copy link
Member Author

@DavidGOrtega, both of my proposed solutions consider the buffer/path dichotomy, taking care only of the latter.

@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented Mar 6, 2021

Nice! Can we add a small test with a non existent file? Also with a a buffer would be awesome.
If not maybe opening a ticket would be also very nice!

* Also removes an stray leading dot on the introduced error message
@0x2b3bfa0 0x2b3bfa0 force-pushed the publish-check-existance branch from c2cb17a to 303b773 Compare March 6, 2021 12:00
@0x2b3bfa0 0x2b3bfa0 marked this pull request as ready for review March 6, 2021 12:02
@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Mar 6, 2021

I finally chose to call stat before calling mime_type so the former provides the safeguard and the meaningful message. If you want to offer a custom but more broad error message, we can revert to the previous solution:

if (path) {
  try {
    await fs.promises.access(path, fs.constants.F_OK | fs.constants.R_OK);
  } catch (err) {
    throw new Error(`Path ${path} does not exist or is not a readable file.`);
  }
}

Otherwise, please review the latest changes and approve the pull request if applicable.

@0x2b3bfa0 0x2b3bfa0 requested a review from DavidGOrtega March 6, 2021 12:06
@0x2b3bfa0 0x2b3bfa0 changed the base branch from master to release/v0.3.1 March 6, 2021 12:17
Copy link
Contributor

@DavidGOrtega DavidGOrtega left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@0x2b3bfa0 0x2b3bfa0 merged commit 95c6d38 into release/v0.3.1 Mar 8, 2021
@0x2b3bfa0 0x2b3bfa0 deleted the publish-check-existance branch March 8, 2021 16:04
@0x2b3bfa0 0x2b3bfa0 added this to the 0.3.1 milestone Mar 8, 2021
DavidGOrtega added a commit that referenced this pull request Mar 18, 2021
…#431)

* [create-pull-request] automated change (#425)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update dependacron pull request step (#426)

This commit updates the major version for the peter-evans/create-pull-request GitHub Action step.

Required in order to avoid a fatal error caused by the  hard deprecation of the `set-env` and `add-path` standard output commands after CVE-2020-15228.

* Add sanity check for cml-publish with filesystem paths (#427)

* Add sanity check for cml-publish files

Closes #308 and probably also closes #401

* Restyled by prettier (#428)

Co-authored-by: Restyled.io <[email protected]>

* Add tests for cml-publish file errors

* Also removes an stray leading dot on the introduced error message

* Embrace native file error messages for cml-publish

Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <[email protected]>

* Add destroy timeout feature (#419)

* [create-pull-request] automated change (#441)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Helio Machado <[email protected]>

* Add support for singleton runners (#422)

* Add support for singleton runners

The --reuse-existing flag will cause cml-runner
to look up for already existing runners with the
specified --labels and skip creating a new one
if that's the case.

* Deprecate --name with a warning

* Apply pre-review suggestions and fixes

* Comment out deprecation warning
* Remove unnecessary comparison
* Rename reuse-existing to reuse
* Fix misconception about empty array truthiness
* Simplify reuse by name logic
* Add missing awaits
* Give precedence to name deduplication over label deduplication
* Remove commented code

* Startup script (#445)

* Startup script

* lint

* snapshots

* Add debugging message for #443 (#448)

This commit doesn't completely fix #443, but closes it until we can reproduce again the error.

* Bitbucket-1000-handling

* bump version

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: DavidGOrtega <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants