Open feldrim opened 5 years ago
This will take some time to review due to the holidays. @healingbrew check when you have some time.
The regex seems to return C
for C:\Windows\System32\cmd.exe
This is your regex:
"cmd" = cmd
"garbled cmd garbled" = garbled
"cmd.exe" = cmd
"garbled cmd.exe garbled" = garbled
"C:\cmd.exe" = C
"garbled C:\cmd.exe garbled" = garbled
"C:\Windows\system32\cmd.exe" = C
"garbled C:\Windows\system32\cmd.exe garbled" = garbled
"\\server\cmd.exe" = \
"garbled \\server\cmd.exe garbled" = garbled
""warden dot dll.exe"" = "warden
"garbled "warden dot dll.exe" garbled" = garbled
This is the current implementation
"cmd" = NULL
"garbled cmd garbled" = NULL
"cmd.exe" = NULL
"garbled cmd.exe garbled" = NULL
"C:\cmd.exe" = C:\cmd.exe
"garbled C:\cmd.exe garbled" = C:\cmd.exe
"C:\Windows\system32\cmd.exe" = C:\Windows\system32\cmd.exe
"garbled C:\Windows\system32\cmd.exe garbled" = C:\Windows\system32\cmd.exe
"\\server\cmd.exe" = \\server\cmd.exe
"garbled \\server\cmd.exe garbled" = \\server\cmd.exe
""warden dot dll.exe"" = NULL
"garbled "warden dot dll.exe" garbled" = NULL
This is our fault for not writing the tests, but this PR cannot be merged until this is fixed.
The entire regex is hugely inefficient and complicated.
It can be trimmed down to
((([\w])?[:%\\\/]|").*(\.[\w:]+|"|%))|([\w\d.\\:]*(\s|$))
(add path-safe characters like _-()&
)
But that yields some quirks as well
"cmd" = cmd
"garbled cmd garbled" = garbled
"cmd.exe" = cmd.exe
"garbled cmd.exe garbled" = garbled
"C:\cmd.exe" = C:\cmd.exe
"garbled C:\cmd.exe garbled" = garbled
"C:\Windows\system32\cmd.exe" = C:\Windows\system32\cmd.exe
"garbled C:\Windows\system32\cmd.exe garbled" = garbled
"\\server\cmd.exe" = \\server\cmd.exe
"garbled \\server\cmd.exe garbled" = garbled
""warden dot dll.exe"" = "warden dot dll.exe"
"garbled "warden dot dll.exe" garbled" = garbled
So you can either split it up into two regexes (((([\w])?[:%\\\/]|").*(\.[\w:]+|"|%))
and ([\w\d.\\:]*(\s|$))
as a fallback which is easier to maintain I guess) or figure prioritization out.
Oh, thanks. I was totally focused on the "cmd" on tutorial. The cases are very detailed and I'll work on unit tests with them. Thank you.
I actually couldn't get the format except garbled in "garbled "warden dot dll.exe" garbled"
. We want commands in quotations? Then what's the issue with .exe
. Can you please explain the format and expected result?
Out of those examples, the expected outcome would be
"cmd" = cmd
"garbled cmd garbled" = garbled
"cmd.exe" = cmd.exe
"garbled cmd.exe garbled" = cmd.exe
"C:\cmd.exe" = C:\cmd.exe
"garbled C:\cmd.exe garbled" = C:\cmd.exe
"C:\Windows\system32\cmd.exe" = C:\Windows\system32\cmd.exe
"garbled C:\Windows\system32\cmd.exe garbled" = C:\Windows\system32\cmd.exe
"\\server\cmd.exe" = \\server\cmd.exe
"garbled \\server\cmd.exe garbled" = \\server\cmd.exe
""warden dot dll.exe"" = warden dot dll.exe
"garbled "warden dot dll.exe" garbled" = warden dot dll.exe
As of now, the tests pass but the part in quotation marks are accepted with any text inside. I believe it should be something like "command command filename.exe" or if it's specific to warden, "warden command filename.exe". Yet, it's incomplete although all tests pass.
@healingbrew Hi. I have a question. Let's say I typed something like cd "c:\Users\Some User\Desktop\Some Path Containing Spaces"
. Then the implementation would only care the quoted string. It might be incorrect by design.
Although it's the default behavior, the regex pattern added with commit https://github.com/RainwayApp/warden/commit/4948bd6a31df4ad6c528426302b9ee4e20e9ddce does not accept commands from environment paths, as I have mentioned on issue https://github.com/RainwayApp/warden/issues/6 . So I changed the pattern to accept commands like cmd. Tests are working but they are not ideal solutions, just workarounds. It needs some refactoring yet still capable of esting the condition.