fix(windows): don't send key twice

[sigmaSd]

see crossterm-rs/crossterm#797 (comment)

windows release event needs to be explicitly ignored

I don't have windows to test , but I saw this issue filed against crossterm many times , so I'm assuming bandwhich have the same problem

crossterm-rs/crossterm#778 would improve things

note: I don't think this will affect tests, maybe we should start disabling the flaky ones

[sigmaSd]

windows ci fails with

Run Invoke-WebRequest -Uri "$env:NPCAP_OEM_URL" -OutFile "$env:TEMP/npcap-oem.exe"
Invoke-WebRequest: D:\a\_temp\0adafdb5-37c4-4718-b5ce-7b33119eb186.ps1:2
Line |
   2 |  Invoke-WebRequest -Uri "$env:NPCAP_OEM_URL" -OutFile "$env:TEMP/npcap …
     |  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Invalid URI: The hostname could not be parsed.

Error: Process completed with exit code 1.

Can prs not access the url variable ?

[cyqsimon]

Hmm. It seems like it's a permission thingy relating to (dis)allowing secrets access on PRs from external repositories. See https://securitylab.github.com/research/github-actions-preventing-pwn-requests/.

It is possible to allow PRs access to secrets by changing the workflow trigger to pull_request_target. However that introduces the possibility of an attacker exfiltrating secrets with a malicious PR.

Granted PRs from first-time contributors require explicit approval to run workflows, but there's nothing preventing a more cunning attacker from bypassing this by submitting an innocuous PR before the actual attack.

We can change it so that all workflow needs approval, but obviously that's hardly an ideal solution. You got any ideas?

[cyqsimon]

It can be argued that since the only use for secrets in this repository is npcap's OEM installer download link, the consequences of a pwn is not as significant as that of, let's say, a private key. Considering the consequences for the attacker (likely losing their account), it is unlikely that anyone would find this a worthy target.

Obviously this is a questionable attitude towards security in general. Plus, doing this would mean that we will no longer be able to use Github secrets for something more sensitive if such a need ever arises (e.g. a deploy key). So I don't like this option either.

[sigmaSd]

I didn't find any info about this, but this pr seems to use windows-2019-npcap os in the runner https://github.com/OpenCyphal/yukon/pull/400/files#diff-c34469acb6dfef04f199d518e466db578ba43f7be784fd981d08ae69457ffdcdR14

[sigmaSd]

I see its a self hosted runner.

[cyqsimon]

For reference, this is how rust-pcap dealt with this:

https://github.com/rust-pcap/pcap/blob/8e3c8ba23f5ec94bf90b8fef98c4239fd9a9a86b/.github/workflows/ci-steps.yml#L57-L62

Not great, not terrible™️. If we cannot come up with something better, I suppose this is what we have to go for.

[cyqsimon]

Also for the record, the issue addressed by this PR seems to impact MacOS too, but only occasionally so.

Examples:

• https://github.com/imsnif/bandwhich/actions/runs/5991693782/job/16250310401
• https://github.com/imsnif/bandwhich/actions/runs/5986016747/job/16238701326
• https://github.com/imsnif/bandwhich/actions/runs/5991693782/job/16250310401

[sigmaSd]

I think thats reasonable, but in theory the situation can be better, since many (if not all) the tests don't actually use npcap

Maybe we can have some cfgs to not build it and stub its function when targetting tests

[sigmaSd]

Also for the record, the issue addressed by this PR seems to impact MacOS too, but only occasionally so.

This PR doesn't address the test flakiness issue

This PR adress a problem that occurs on windows, if you press a key it triggers twice because release event gets sent,

I don't have windows to test so it would be great if you actually test that this is indeed a problem and this pr fixes it

I'm just sending this pr without even triggering the bug first because I saw this issue filed many times against crossterm repo

[cyqsimon]

Maybe we can have some cfgs to not build it and stub its function when targetting tests

Hmm. Good idea. I'll give it a shot in a separate PR.

[cyqsimon]

Also for the record, the issue addressed by this PR seems to impact MacOS too, but only occasionally so.

This PR doesn't address the test flakiness issue

This PR adress a problem that occurs on windows, if you press a key it triggers twice because release event gets sent,

I don't have windows to test so it would be great if you actually test that this is indeed a problem and this pr fixes it

I'm just sending this pr without even triggering the bug first because I saw this issue filed many times against crossterm repo

I see. Sorry about that. So what do you think about these strange transient failures on MacOS?

[sigmaSd]

Unfortunately I don't have an explanation now

My idea is if we can't actual fix it

• find which tests are flaky (hopefully its a small subset and hopefully its not all of them xD)
• disable them or add something like #[flaky] which will run the test for maybe 3 times before saying it failed

[sigmaSd]

Or maybe switch to next-test which does support this https://nexte.st/book/retries.html?highlight=flaky#retries-and-flaky-tests

[cyqsimon]

I think thats reasonable, but in theory the situation can be better, since many (if not all) the tests don't actually use npcap

Maybe we can have some cfgs to not build it and stub its function when targetting tests

So I tried your idea, but as a wise man once said, trying is the first step towards failure. You tried your best and failed miserably. The lesson is, never try.

Jokes aside, it seems like the entire test binary will fail if stuff from pnet is imported anywhere, which makes gating some tests behind a feature flag much more difficult than I thought it was going to be. Plus, I don't think there actually are many tests (if any at all) not relating to packet capture.

Anyways, my conclusion is that it's way too much effort for too little gain. I'll just disable Windows tests for PRs and call it a day.

[cyqsimon]

Okay, if you rebase now I think the tests should pass.

[sigmaSd]

I tried to debug the flaky tests today , but that went nowhere, hopefully you find better ideas