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

Validate required inputs are set before invoking an action #1070

Open
NorseGaud opened this issue Jun 1, 2020 · 32 comments
Open

Validate required inputs are set before invoking an action #1070

NorseGaud opened this issue Jun 1, 2020 · 32 comments
Labels
bug Something isn't working Runner Bug Bug fix scope to the runner

Comments

@NorseGaud
Copy link

Describe the bug

I have the following action.yml:

name: 'Anka Prepare VM and execute inside'
description: 'Prepare an Anka VM and execute commands inside'
author: Veertu
branding:
  icon: 'anchor'
  color: 'blue'
inputs:
  anka-template:
    description: 'name or UUID of Anka Template'
    required: true

My index.js looks like:

const core = require('@actions/core');
const io = require('@actions/io');
const prepare = require('./prepare');
const execute = require('./execute');

// most @actions toolkit packages have async methods
async function run() {
  try { 
    const ankaVirtCLIPath = await io.which('anka', true)
    console.log(`Anka Virtualization CLI found at: ${ankaVirtCLIPath}`)
    const ankaTemplate = core.getInput('anka-template');
    const ankaTag = core.getInput('anka-tag');
    const ankaCommands = core.getInput('commands');
    const hostPreCommands = core.getInput('host-pre-commands');
    const hostPostCommands = core.getInput('host-post-commands');
    console.log("=========================" + "\n" + `${hostPreCommands}` + "\n" + "=================================")
    console.log("=========================" + "\n" + `${hostPostCommands}` + "\n" + "=================================")
    console.log("=========================" + "\n" + `${ankaCommands}` + "\n" + "=================================")
    if (hostPreCommands) {
      await execute.nodeCommands(hostPreCommands)
    }
    // Prepare VM
    await prepare.ankaRegistryPull(ankaTemplate,ankaTag)
    // Run commands inside VM
    // Cleanup
    if (hostPostCommands) {
      await execute.nodeCommands(hostPostCommands)
    }
  } catch (error) {
    core.setFailed(error);
  }
}

run()

and finally, the important part of my .github/workflows/test.yml:

    - name: basic commands
      id: basic
      uses: ./
      with:
        commands: |
          env
          ls -laht ./
          ls -laht ../
          pwd
          echo "HERE" && \
          echo "THERE HERE WHERE"

Yet, there is no failure for missing anka-template... It fails for a reason inside of the ankaRegistryPull function which is well after it tries to load the input.

https://help.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#inputs seems to indicate that it shouldn't allow the action to run if it's missing that input.

What am I missing?

@NorseGaud
Copy link
Author

NorseGaud commented Jun 1, 2020

More info from my testing:

    const ankaTemplate = core.getInput('anka-template');
    console.log(`${typeof(ankaTemplate)}`)
    console.log(`TEMPLATE: ${ankaTemplate}`)
    if (typeof(ankaTemplate) === 'undefined') { 
      throw new Error('anka-template is not set!'); 
    }

Screen Shot 2020-06-01 at 3 46 47 PM

if I use .length ===0 I can get it to fail... But again, shouldn't this be handled by required: true?

@thboop
Copy link
Collaborator

thboop commented Jun 25, 2020

We do provide the ability to set an input as required in core.getInput(), which will fail if an input is not set.

The action.yaml is mainly for the benefit of the the runner/action author, we don't really use it in the toolkit. We do some validation on inputs in the runner already for unexpected inputs, we could also fail the step if the required inputs are not set.

cc @TingluoHuang , @ericsciple , I'd love to hear your thoughts as well.

@aggenebbisj
Copy link

aggenebbisj commented Jan 14, 2021

I just ran into the same issue. Have to say it's quite unexpected to mark something as required and then finding out it blows up silently. Especially since toolkit is the official GitHub toolkit for actions.

we could also fail the step if the required inputs are not set.

I think this would make a lot of sense :-)

@thboop
Copy link
Collaborator

thboop commented Apr 30, 2021

This would improve the experience of developing actions, but would also break users that have erroneously set required in their action.yaml.

That being said, this isn't really a toolkit issue, the runner controls this. I'm going to transfer this issue to that repository.

@thboop thboop changed the title inputs required doesn't seem to be working Validate required inputs are set before invoking an action Apr 30, 2021
@thboop thboop transferred this issue from actions/toolkit Apr 30, 2021
@NorseGaud
Copy link
Author

bump

@anukul
Copy link

anukul commented Aug 12, 2021

So what's the point of required? 🤔

@gjadi-mej
Copy link

Bump, I was also surprised by this behavior (https://github.saobby.my.eu.orgmunity/t/inputs-required-not-enforced-with-no-default/206745/3).

@gajus
Copy link

gajus commented Dec 8, 2021

Does anyone have a one-liner workaround to enforce required parameters?

@anukul
Copy link

anukul commented Dec 8, 2021

@gajus This might help:

: "${INPUT_APP:?input \"app\" not set or empty}"

@gajus
Copy link

gajus commented Dec 8, 2021

Thanks @anukul , I was more wondering if there a way to write generic check, e.g. using JavaScript actions. However, it seems that it is impossible to get parent inputs.

@gajus
Copy link

gajus commented Dec 8, 2021

@anukul Looks like your suggestion doesn't work either. At least in composite actions, env does not include INPUT_.

#665

@gajus
Copy link

gajus commented Dec 8, 2021

Something like this works:

- run: |
    [[ "${{ inputs.docker_image_name }}" ]] || { echo "docker_image_name input is empty" ; exit 1; }
    [[ "${{ inputs.doppler_token }}" ]] || { echo "doppler_token input is empty" ; exit 1; }

@nikola-jokic nikola-jokic added bug Something isn't working Runner Bug Bug fix scope to the runner labels Mar 18, 2022
rossjrw added a commit to rossjrw/pr-preview-action that referenced this issue Apr 17, 2022
The Actions runner does not verify that inputs marked as 'required' are
actually given: actions/runner#1070
@ViacheslavKudinov
Copy link

Any updates with validating required inputs?

@balaji-srin
Copy link

Also having the same issue. Would be nice to get this fixed.

@Rarisma
Copy link

Rarisma commented Aug 16, 2022

Would help as Im facing the same issue

@Eshnek
Copy link

Eshnek commented Oct 6, 2022

+1, had to turn debug logging on to realize this was my issue

@alexquitiaquez
Copy link

Hi, any update about it?

@kpet
Copy link

kpet commented Jun 3, 2023

Just ran into this as well. It'd be nice to get a clean failure of the using step when one or more required inputs are not provided. An error message should be printed and state which required input(s) were not provided. It'd be nice to have all the required but missing inputs printed in the message to avoid back and forth (hit the error, add one, hit the error again, ...). Thanks!

@hytromo
Copy link

hytromo commented Jun 21, 2023

Run into this as well, spent quite some time debugging an issue that would have been solved in 1' if github had let me know that a required input was missing 😬

@christianbuerk0
Copy link

Run into this as well

@jonelleamio
Copy link

jonelleamio commented Aug 25, 2023

Something like this works:

- run: |
    [[ "${{ inputs.docker_image_name }}" ]] || { echo "docker_image_name input is empty" ; exit 1; }
    [[ "${{ inputs.doppler_token }}" ]] || { echo "doppler_token input is empty" ; exit 1; }

It's been a couple of years, is it still the best good answer ? or did they fixed it ?

@mwestphal
Copy link

Still not fixed.

@GoodMirek
Copy link

GoodMirek commented Nov 30, 2023

I ran into this issue too.
It looks as such a basic thing which competing CI systems have for many years.
Here, it is open for three years and still not solved.

I can imagine it cannot be just changed, as it would break too many things. But it could be an opt-in feature.

@GoodMirek
Copy link

So what's the point of required? 🤔

I have the same question. required currently seems completely useless in Github Actions.

@erikahansen
Copy link

Is this resolved now? I just got this:

Invalid workflow file: .github/workflows/development.yml#L10
The workflow is not valid. .github/workflows/development.yml (Line: 10, Col: 11): Input x is required, but not provided while calling.

@ChristopherHX
Copy link
Contributor

Not fixed for actions.

Is this resolved now? I just got this:

This was never a problem for reusable workflows (your example), these are a newer concept.

I agree GitHub Actions (CI/CD Product name) vs. actions (those are the steps of a job < this issue is about that part) vs. (reusable) Workflows (those are files having an on key and one or more jobs) might be an easy to mix buzzwords in GitHub.

@alexkuc
Copy link

alexkuc commented Jan 23, 2024

🤔 Maybe something got changed but when I changed input to required: true, the workflow_dispatch form made the field mandatory making it impossible to submit the form w/o supplying a value (HTML5 validation)

@Bloodsucker
Copy link

Bloodsucker commented Jan 23, 2024

the workflow_dispatch form

The issue people are describing doesn't affect the dispatch form that you're referring to in your comment. The people are complaining about the input required setting being ignored when calling a job from another job. The expectation was for the job to fail if not all required parameters are set, however it just continues like nothing.

@kkroening
Copy link

kkroening commented Feb 25, 2024

Does required: true for composite action inputs do anything? It doesn't seem like it - the action happily chugs along without a peep when the input is unspecified, and then something somewhere down the line ends up receiving a blank string, wreaking havoc without an obvious cause. I guess required: true is just a no-op, unless I'm missing something or not understanding the purpose.

Maybe it's mentioned somewhere in the fine print above that this is indeed a no-op. Might be worth calling it out in the inputs.<input_id>.required API reference or something though.

@ianlewis
Copy link

Does required: true for composite action inputs do anything?

It doesn't do anything except to provide an intention to users. It's basically just documentation. Actions authors have to do all the input validation themselves currently.

@ramonpetgrave64
Copy link

It seems like input types are also not enforced. I had assumed types number and boolean would keep my workflows safe from script injection. But the only way is to use them as environment variables.

@ianlewis
Copy link

It seems like input types are also not enforced. I had assumed types number and boolean would keep my workflows safe from script injection. But the only way is to use them as environment variables.

Inputs are provided to actions as environment variables so they are only strings. The actions.yml metadata format for inputs does not even had a type field. Types for inputs are only valid on reusable workflows.

ramonpetgrave64 added a commit to slsa-framework/slsa-verifier that referenced this issue May 22, 2024
changing the update-dist workflow to use the `pr_number` input as an env
variable to avoid [script
injection](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#good-practices-for-mitigating-script-injection-attacks).

Our workflows are only invokable by our trusted maintainers so we should
be okay. This is just an extra hardening measure.

Open issue
actions/runner#1070 (comment)

## Testing

I confirmed the issue by invoking the workflow with `650 && echo SCRIPT
INJECTION`, and it did also do the extra `echo` command.
-
https://github.com/slsa-framework/slsa-verifier/actions/runs/9101350247/job/25018333703#step:3:36

after invoking the workflow again with this PR's version, the problem is
mitigated.
-
https://github.com/slsa-framework/slsa-verifier/actions/runs/9101495332/job/25018812710#step:3:8
-
https://github.com/slsa-framework/slsa-verifier/actions/runs/9101516757/job/25018888519#step:3:7

Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
ramonpetgrave64 added a commit to ramonpetgrave64/slsa-verifier that referenced this issue Jun 10, 2024
changing the update-dist workflow to use the `pr_number` input as an env
variable to avoid [script
injection](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#good-practices-for-mitigating-script-injection-attacks).

Our workflows are only invokable by our trusted maintainers so we should
be okay. This is just an extra hardening measure.

Open issue
actions/runner#1070 (comment)

## Testing

I confirmed the issue by invoking the workflow with `650 && echo SCRIPT
INJECTION`, and it did also do the extra `echo` command.
-
https://github.com/slsa-framework/slsa-verifier/actions/runs/9101350247/job/25018333703#step:3:36

after invoking the workflow again with this PR's version, the problem is
mitigated.
-
https://github.com/slsa-framework/slsa-verifier/actions/runs/9101495332/job/25018812710#step:3:8
-
https://github.com/slsa-framework/slsa-verifier/actions/runs/9101516757/job/25018888519#step:3:7

Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
dbertram added a commit to ynab/slack-post-message-action that referenced this issue Nov 14, 2024
dbertram added a commit to ynab/slack-post-message-action that referenced this issue Nov 14, 2024
dbertram added a commit to ynab/slack-post-message-action that referenced this issue Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Runner Bug Bug fix scope to the runner
Projects
None yet
Development

No branches or pull requests