Fix checking validity of tmate-server-xxx parameters #105

vadz · 2022-02-11T01:13:43Z

Don't throw an exception if they match the expected form but rather if
they do not match it.

Also add colon to the list of allowed characters as it must be used in
the fingerprints (which have the form of "SHA256:xxxxxx").

Sorry if I'm missing something obvious here, but it seems that the check can't work as currently written: as soon as I add

        with:
          tmate-server-host: shell.example.com

to my work workflow, I get

Error: Invalid value for 'tmate-server-host': 'shell.example.com'

and looking at the code it seems impossible to avoid this.

I believe that the intention was to negate this check, but even then it's still not quite right because we also need to allow colons here. But, again, perhaps I'm misunderstanding something here because I just don't see how could this have ever worked as written.

dscho

I wonder how that ever worked... I assumed that the original contributor had tested it.

Now that I look at the code again, I cannot fail to notice how little sense it makes to validate all four inputs against the same regex. The port number, for example, should only contain digits, and start with a non-zero one.

So I would like to suggest moving the regex to a parameter that is passed to the getValidatedInput() function. And the different inputs should then have their own, different regexes.

Also, we will want to prevent an equally face-palm-triggering bug as a missing ! in the future. The way to do that is of course to add a regression test.

Here is an example how to mock getInput(), and here is code demonstrating how to verify that the tmate command is correct (you will need to figure out what execShellCommand is the tmate one).

For good measure, there should also be a test that verifies that invalid input triggers an error and not call tmate (or execShellCommand() at all, more likely).

Would you mind making those changes?

vadz · 2022-02-11T14:22:57Z

Now that I look at the code again, I cannot fail to notice how little sense it makes to validate all four inputs against the same regex. The port number, for example, should only contain digits, and start with a non-zero one.

I agree with this, but I wanted to keep my changes as small as possible, so I decided to not change this.

So I would like to suggest moving the regex to a parameter that is passed to the getValidatedInput() function. And the different inputs should then have their own, different regexes.

This looks like a good idea, but as long as we're discussing this anyhow, let me ask another question: what is the purpose of this validation in the first place? The parameters come from the workflow file and the action code itself also comes (albeit indirectly) from the same workflow file, so there doesn't seem to be any security concerns here (you would be just attacking yourself, basically, by using malicious parameters). What's the worst that is going to happen if you pass an invalid value for any of those? Wouldn't we just get an error from tmate anyhow?

I.e. do we really need the validation at all? And if we do, maybe we should just do core.warning(...) instead of throwing an error if it fails?

Also, we will want to prevent an equally face-palm-triggering bug as a missing ! in the future. The way to do that is of course to add a regression test.
[...]
Would you mind making those changes?

I'm not a JS/TS developer at all, so I might have some trouble with/it might take me some time to do this, but I'll try to do it, I agree that having tests would be good. But I'd like to see what do you think about the questions above first. TIA!

dscho · 2022-02-11T17:24:46Z

Now that I look at the code again, I cannot fail to notice how little sense it makes to validate all four inputs against the same regex. The port number, for example, should only contain digits, and start with a non-zero one.

I agree with this, but I wanted to keep my changes as small as possible, so I decided to not change this.

Please do change it.

So I would like to suggest moving the regex to a parameter that is passed to the getValidatedInput() function. And the different inputs should then have their own, different regexes.

This looks like a good idea, but as long as we're discussing this anyhow, let me ask another question: what is the purpose of this validation in the first place?

Usage mistakes are very easy to make. These validations help identify and rectify the mistakes. There is not really any security concern, but there is the concern that unhelpful error messages are such a pain.

I'm not a JS/TS developer at all

Hah, neither am I. But the existing code is easy enough to imitate, no?

FWIW if you have trouble figuring out how to run the tests, simply call npm install followed by npm run test.

vadz · 2022-02-11T17:30:06Z

This looks like a good idea, but as long as we're discussing this anyhow, let me ask another question: what is the purpose of this validation in the first place?

Usage mistakes are very easy to make. These validations help identify and rectify the mistakes. There is not really any security concern, but there is the concern that unhelpful error messages are such a pain.

Right, but AFAICS core.warning() would work nicely in this case, as it should create an annotation with the problem. OTOH it would still allow the action to work if the validating regex is too strict for whatever reason.

Are you sure you prefer to keep the exception (I promise not to ask again)?

dscho · 2022-02-11T17:45:49Z

Right, but AFAICS core.warning() would work nicely in this case, as it should create an annotation with the problem.

I would go for a core.error(), actually.

Are you sure you prefer to keep the exception (I promise not to ask again)?

That's the easiest way to exit with error, even easier than core.error() and then ensuring that we return without proceeding. So actually, yes, I would keep the exception: I prefer simpler solutions to complicated ones.

Use appropriate regexes for each parameter instead of using the same, wrong, regex for all of them and fix the direction of test. Add unit tests to verify that the validation works as expected.

vadz · 2022-02-12T21:04:30Z

I've finally implemented this (it took much longer than I thought due to my inexperience with JS and Jest) and it seems to work as expected. Please have a look at the new version.

dscho

Looks good!

dscho · 2022-02-13T07:37:59Z

lib/index.js

+      const value = getValidatedInput(opt, options[opt]);
+      if (value !== undefined) {
+        setDefaultCommand = `${setDefaultCommand} set-option -g ${opt} "${value}" \\;`;
+      }


Strictly speaking, this is a change in behavior (while the code is much cleaner now, thank you!): previously, we required all those settings, or none. Now we accept a subset.

dscho · 2022-02-13T07:40:23Z

src/index.test.js

+    __esModule: true,
+    ...originalModule,
+    execShellCommand: jest.fn(() => 'mocked execShellCommand'),
+  };


Can't we mock the execShellCommand() function more elegantly?

dscho · 2022-02-13T07:45:58Z

@mxschmitt I can't release this right now, as I am typing this on a phone... if you have some time, would you mind? Otherwise I will do it as soon as I can.

dscho · 2022-02-13T12:02:53Z

Okay, I'm on it.

dscho · 2022-02-13T12:07:02Z

Done: https://github.com/mxschmitt/action-tmate/releases/tag/v3.10

clinssen · 2022-02-13T13:18:40Z

Hi, this seems to break the action when no tmate-server-host is specified (and it should thus fall back to the default).

What I'm getting instead is Error: Invalid value for 'tmate-server-host': ''.

dscho · 2022-02-13T15:54:11Z

Hi, this seems to break the action when no tmate-server-host is specified (and it should thus fall back to the default).

What I'm getting instead is Error: Invalid value for 'tmate-server-host': ''.

Sorry for the breakage! Next version incoming...

vadz · 2022-02-13T16:09:11Z

Oops, sorry about this, I'm on the phone now but should be back shortly and can test/fix/submit a new PR a bit later today if you'd like.

dscho · 2022-02-13T16:47:29Z

@mxschmitt fixed it in #109

vadz requested review from dscho and mxschmitt as code owners February 11, 2022 01:13

dscho requested changes Feb 11, 2022

View reviewed changes

larsks mentioned this pull request Feb 11, 2022

Not possible to use a custom tmate host #106

Closed

Fix validation of tmake-server-xxx options

dc9bdec

Use appropriate regexes for each parameter instead of using the same, wrong, regex for all of them and fix the direction of test. Add unit tests to verify that the validation works as expected.

vadz force-pushed the fix-option-validity-check branch from ec8935a to dc9bdec Compare February 12, 2022 21:02

dscho approved these changes Feb 13, 2022

View reviewed changes

dscho merged commit bf9f18e into mxschmitt:master Feb 13, 2022

odanado mentioned this pull request Feb 13, 2022

Error: Invalid value for 'tmate-server-host': '' #108

Closed

vadz deleted the fix-option-validity-check branch February 13, 2022 17:06

vadz mentioned this pull request Feb 13, 2022

Some tests improvements #110

Merged

Uh oh!

Fix checking validity of tmate-server-xxx parameters #105

Fix checking validity of tmate-server-xxx parameters #105

Uh oh!

Conversation

vadz commented Feb 11, 2022

Uh oh!

dscho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadz commented Feb 11, 2022

Uh oh!

dscho commented Feb 11, 2022

Uh oh!

vadz commented Feb 11, 2022

Uh oh!

dscho commented Feb 11, 2022

Uh oh!

vadz commented Feb 12, 2022

Uh oh!

dscho left a comment

Choose a reason for hiding this comment

Uh oh!

dscho Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

dscho Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

dscho commented Feb 13, 2022

Uh oh!

dscho commented Feb 13, 2022

Uh oh!

dscho commented Feb 13, 2022

Uh oh!

clinssen commented Feb 13, 2022

Uh oh!

dscho commented Feb 13, 2022

Uh oh!

vadz commented Feb 13, 2022

Uh oh!

dscho commented Feb 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dscho left a comment •

edited

Loading