Skip to content

UnixPath and WindowsPath#14267

Merged
ibeckermayer merged 53 commits intomasterfrom
isaiah/clean-path-strings
Aug 6, 2022
Merged

UnixPath and WindowsPath#14267
ibeckermayer merged 53 commits intomasterfrom
isaiah/clean-path-strings

Conversation

@ibeckermayer
Copy link
Copy Markdown
Contributor

Adds a UnixPath and WindowsPath struct for denoting the types of path Strings we expect to hold in our various RDP and TDP structs. Neither type implements any strong validation, since paths (nearly) always originate from the RDP server which we must assume is working correctly. The assumptions made about the types of WindowsPaths we'll receive are based on our experience with RDP so far, since the precise details are not specified in the spec documents.

Before going with this option, I looked into std::path::Path and the path_slash crate (which relies on std::path::Path). I was unable to find any standardized means for handling Windows style paths while running rust on Unix machines, so I opted for the approach taken here.

@espadolini
Copy link
Copy Markdown
Contributor

it's not clear from the utf16string docs whether they can handle unpaired surrogates

utf16string::WStr has an infallible, lossless conversion to String, so the answer to that question is no, you'll get a utf16string::Utf16Error when trying to create a WStr.

How does the javascript side of things fare with non-unicode paths? If there's nothing sane we can do on that side either, we could just throw some IO error when attempting to write a file with a broken path in the shared directory.

@ibeckermayer
Copy link
Copy Markdown
Contributor Author

How does the javascript side of things fare with non-unicode paths? If there's nothing sane we can do on that side either, we could just throw some IO error when attempting to write a file with a broken path in the shared directory.

Apparently JS doesn't fare well with UTF-16 generally. Right now, presuming a utf16string::Utf16Error is returned from from_unicode, a path with invalid UTF-16 will just end the session with an error.

I think that's good enough for now as long as we document it. I've created an issue to explore this in more detail later: #15126

@ibeckermayer ibeckermayer requested a review from xacrimon August 2, 2022 17:41
@ibeckermayer
Copy link
Copy Markdown
Contributor Author

Apparently JS doesn't fare well with UTF-16 generally.

Oops, I meant to link this Stack Overflow answer. Looking at it again, though, its from 2011, and some of the problem may be ameliorated by now. Nevertheless, I say its low priority and we continue as planned, and I will come back to that issue later on.

Comment thread lib/srv/desktop/rdp/rdpclient/src/rdpdr/path.rs Outdated
Comment thread lib/srv/desktop/rdp/rdpclient/src/rdpdr/path.rs Outdated
Comment thread lib/srv/desktop/rdp/rdpclient/src/rdpdr/path.rs Outdated
Comment thread lib/srv/desktop/rdp/rdpclient/src/rdpdr/path.rs
@ibeckermayer ibeckermayer requested a review from espadolini August 3, 2022 20:11
@ibeckermayer
Copy link
Copy Markdown
Contributor Author

Waiting until #13630 is merged and then will merge this direct into master

@ibeckermayer ibeckermayer changed the base branch from windows-desktop-directory-sharing to master August 4, 2022 22:45
@ibeckermayer ibeckermayer enabled auto-merge (squash) August 6, 2022 03:16
@ibeckermayer ibeckermayer merged commit 241c3af into master Aug 6, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 6, 2022

@ibeckermayer See the table below for backport results.

Branch Result
branch/v10 Failed
branch/v9 Failed

ibeckermayer pushed a commit that referenced this pull request Aug 23, 2022
ibeckermayer pushed a commit that referenced this pull request Aug 23, 2022
ibeckermayer pushed a commit that referenced this pull request Sep 7, 2022
…ring) (#15770)

* Windows Desktop Directory Sharing (#13630)

* `IRP_MJ_CREATE` (#12665)

* `IRP_MJ_QUERY_INFORMATION` (#12717)

* `IRP_MJ_CLOSE` (#12729)

* Refactor rdpdr client (#12750)

* Adding logic for `FILE_SUPERSEDE` (#12829)

* Improve `process_irp_create` (#12830)

* adds return statements that got lost in a merge

* `IRP_MJ_DIRECTORY_CONTROL` (#12870)

* `FileFullDirectoryInformation` (#12908)

* Improve `ClientDriveQueryDirectoryResponse.encode()` (#12912)

* `IRP_MJ_QUERY_VOLUME_INFORMATION` (#13071)

* Fix Shared Directory Request handling when feature is disabled (#13439)

* IRP_MJ_READ, IRP_MJ_WRITE, and IRP_MJ_SET_INFORMATION (#13995)

* Adds constants for sizing calculations (#14051)

Co-authored-by: Łukasz Kozłowski <lukasz.kozlowski@goteleport.com>
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

* `UnixPath` and `WindowsPath` (#14267)

* `SharedDirectoryMoveRequest` and `SharedDirectoryMoveResponse` (#14959)

* `SharedDirectoryCreateResponse` update (#15289)

* Fix `process_irp_set_information` (#15364)

* Sanitize Rust Debug Logs (#15743)

* updates rdp-rs ref to include licensing changes

* Updates rdp-rs ref and fixes Cargo

Co-authored-by: Łukasz Kozłowski <lukasz.kozlowski@goteleport.com>
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
@zmb3 zmb3 deleted the isaiah/clean-path-strings branch September 9, 2022 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants