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

fix: permission prompt stuffing on Windows #11969

Merged
merged 7 commits into from Sep 14, 2021

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Sep 9, 2021

This unfortunately makes Windows act differently in that pressing the "y" or "n" key immediately confirms or denies the prompt. It would take a lot more code to make this work otherwise.

Also, I looked into using https://docs.microsoft.com/en-us/windows/console/writeconsoleinput to create unit tests, but I couldn't figure out a way to construct an INPUT_RECORD.

image

runtime/permissions.rs Outdated Show resolved Hide resolved
runtime/permissions.rs Outdated Show resolved Hide resolved
@piscisaureus
Copy link
Member

I couldn't figure out a way to construct an INPUT_RECORD.

I think it'd be something like this:

let mut input_record = INPUT_RECORD { EventType: KEY_EVENT, ..Default::default() };
input_record.Event.KeyEvent_mut().wRepeatCount = 1;
input_record.Event.KeyEvent_mut().uChar.UnicodeChar_mut() = 'y' as WCHAR;

@dsherret
Copy link
Member Author

dsherret commented Sep 9, 2021

@piscisaureus oh, nice! Thanks for that. I will look into adding a test tomorrow.

Edit: Oh, I could probably also just do std::mem::zeroed() as mentioned in the first FAQ of their documentation...

@piscisaureus
Copy link
Member

piscisaureus commented Sep 9, 2021

This unfortunately makes Windows act differently in that pressing the "y" or "n" key immediately confirms or denies the prompt. It would take a lot more code to make this work otherwise.

What makes it difficult in particular? Afaik ReadConsoleW() provides a way to do line-buffered input.

@dsherret
Copy link
Member Author

dsherret commented Sep 9, 2021

@piscisaureus calling ReadConsoleW would return the permission stuffed text even after calling FlushConsoleInputBuffer. I think perhaps FlushConsoleInputBuffer flushes the input buffer events to a character buffer that ReadConsoleW then reads from (that would be my guess given the behaviour I'm seeing... you may want to try something out and see if you can figure out a way to flush that buffer... I haven't been able to figure it out yet).

https://github.com/denoland/deno/pull/11969/files#diff-ddef1e94cdb9cee582c71ed2e1a3948e4a1a80cdcf311693b8a41d13a79aa418R1247-R1252

By the way, here is my test script from Ryan's tests:

const buf = new Uint8Array(1);
console.log("Enter 'yy':");
await Deno.stdin.read(buf);
await Deno.permissions.request({ "name": "env" });
console.log("\n\nOwned", Deno.env.get("SOMETHING"));

@piscisaureus
Copy link
Member

@piscisaureus calling ReadConsoleW would return the permission stuffed text even after calling FlushConsoleInputBuffer. I think perhaps FlushConsoleInputBuffer flushes the input buffer events to a character buffer that ReadConsoleW then reads from

I would be very surprised if this was case...

@dsherret
Copy link
Member Author

dsherret commented Sep 11, 2021

@piscisaureus in the code in this PR if you try adding:

      assert_eq!(
        FlushConsoleInputBuffer(stdin),
        winapi::shared::minwindef::TRUE
      );

      // add this after the existing `FlushConsoleInputBuffer` (shown above for reference)
      let mut buffer = [0u16; 1024];
      let mut chars_read = 0;
      assert_eq!(winapi::um::consoleapi::ReadConsoleW(stdin, buffer.as_mut_ptr() as _, buffer.len() as _, &mut chars_read, std::ptr::null_mut()), 1);
      println!("Chars read: {}", chars_read);
      let text = std::ffi::OsString::from_wide(&buffer[0..chars_read as usize]).to_string_lossy().to_string();
      println!("Text: {:?}", text);

...then running this JS code if you type yy<ENTER> then you will see the following output:

image

Maybe I'm doing something wrong? Again, I'm not super familiar with the winapi. Hopefully there is a way to do this that I'm not seeing.

@piscisaureus
Copy link
Member

I’ll take a look and try to figure this out tomorrow.

@piscisaureus
Copy link
Member

I wrote a simple standalone test: https://gist.github.com/piscisaureus/42fd7536b075f447682977083c7ff678
It seems that FlushConsoleInputBuffer() is working fine with ReadConsoleW().

@dsherret
Copy link
Member Author

dsherret commented Sep 13, 2021

@piscisaureus try my fork https://gist.github.com/dsherret/72991b395d68ac6aff912199e5e44ac5

By the way, from these tests it's clear that FlushConsoleInputBuffer does not flush the console input buffer to some other buffer like I speculated, but it does look like whatever ReadConsoleW reads from is filled before the FlushConsoleInputBuffer gets to it (maybe on enter key press?)

@dsherret dsherret merged commit 5e2c5d0 into denoland:main Sep 14, 2021
@dsherret dsherret deleted the permission_stuffing_windows branch September 14, 2021 12:37
@dsherret dsherret mentioned this pull request Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants