Reason for use of RawStr in peach-web? #14

New Issue

notplants · 2021-11-04T11:41:20Z

notplants commented

2021-11-04 11:41:20 +00:00

While working on the upgrade to rocket 0.5 I encountered the error:

 pub fn network_add_ssid(ssid: &RawStr, flash: Option<FlashMessage>) -> Template {
    |                               ^^^^^^^ the trait `FromFormField<'_>` is not implemented for `&RawStr`

Presumably this error is from a change in the rocket implementation of RawStr.

@glyph I wanted to confirm - is there something special about the ssid string that you need to manually decode it instead of making this of the type ssid: &str?

While working on the upgrade to rocket 0.5 I encountered the error: ``` pub fn network_add_ssid(ssid: &RawStr, flash: Option<FlashMessage>) -> Template { | ^^^^^^^ the trait `FromFormField<'_>` is not implemented for `&RawStr` ``` Presumably this error is from a change in the rocket implementation of RawStr. @glyph I wanted to confirm - is there something special about the ssid string that you need to manually decode it instead of making this of the type `ssid: &str`?

glyph commented

2021-11-04 12:11:51 +00:00

@notplants

Hmm...that's a good question. I can't recall exactly why &RawStr is used for the ssid (I think there are three functions with that parameter pattern). If I had to guess, I'd say I probably used &RawStr to express that the input was unvalidated (this might have been a recommendation from the Rocket guide at the time) and needed to be URL-decoded (percent decode).

It's strange to me that they haven't implemented FromFormField for &RawStr in the latest release.

It should be OK to accept the ssid as &str and then perform the percent decode step.

Side-note: I don't think the SSID input could be abused for a code injection attack but it might still be a good idea to think-through the context and sanitize the input if required.

@notplants Hmm...that's a good question. I can't recall exactly why `&RawStr` is used for the `ssid` (I think there are three functions with that parameter pattern). If I had to guess, I'd say I probably used `&RawStr` to express that the input was unvalidated (this might have been a recommendation from the Rocket guide at the time) and needed to be URL-decoded (percent decode). It's strange to me that they haven't implemented `FromFormField` for `&RawStr` in the latest release. It should be OK to accept the `ssid` as `&str` and then perform the percent decode step. Side-note: I don't think the SSID input could be abused for a code injection attack but it might still be a good idea to think-through the context and sanitize the input if required.

notplants commented

2021-11-04 12:12:33 +00:00

my initial guess is that rocket0.4 didn't handle utf-8 in query strings well?

In 0.5 documentatation they explicitly mention that they allow utf8-strings in query parameters (and all forms) now. https://rocket.rs/v0.5-rc/guide/requests/#static-parameters

Ps. the documentation for rocket 0.5 is greatly improved and more comprehensive than the documentation for rocket 0.4. Its great.

my initial guess is that rocket0.4 didn't handle utf-8 in query strings well? In 0.5 documentatation they explicitly mention that they allow utf8-strings in query parameters (and all forms) now. https://rocket.rs/v0.5-rc/guide/requests/#static-parameters Ps. the documentation for rocket 0.5 is greatly improved and more comprehensive than the documentation for rocket 0.4. Its great.

notplants commented

2021-11-04 12:14:33 +00:00

(I wrote above comment at the same time as your comment and hadn't read it yet, btw)

notplants commented

2021-11-04 12:19:44 +00:00

@glyph from what I'm reading in the documentation, I think in rocket 0.5 its not the intention to use RawStr as a type for a route parameter, but rather to put validation and parsing into FromFormField (https://docs.rs/rocket/0.5.0-rc.1/rocket/form/trait.FromFormField.html) which you implement or derive for your types (for any types which they haven't already implemented it for),
which would explain why they haven't implemented the function for RawStr.

@glyph from what I'm reading in the documentation, *I think* in rocket 0.5 its not the intention to use RawStr as a type for a route parameter, but rather to put validation and parsing into FromFormField (https://docs.rs/rocket/0.5.0-rc.1/rocket/form/trait.FromFormField.html) which you implement or derive for your types (for any types which they haven't already implemented it for), which would explain why they haven't implemented the function for RawStr.

👍 1

glyph commented

2021-11-04 12:24:45 +00:00

Ah yes, that seems to be the case. I see in FromForm provided implementations that &str is automatically percent-decoded. That's the way to go then 👍

Ps. the documentation for rocket 0.5 is greatly improved and more comprehensive than the documentation for rocket 0.4. Its great.

I'm stoked to hear that! I always thought the 0.4 docs were pretty thorough so I'm impressed to know that they've made them even better :) Rocket is an exemplary project.

Ah yes, that seems to be the case. I see in [`FromForm` provided implementations](https://docs.rs/rocket/0.5.0-rc.1/rocket/form/trait.FromForm.html#provided-implementations) that `&str` is automatically percent-decoded. That's the way to go then 👍 > Ps. the documentation for rocket 0.5 is greatly improved and more comprehensive than the documentation for rocket 0.4. Its great. I'm stoked to hear that! I always thought the 0.4 docs were pretty thorough so I'm impressed to know that they've made them even better :) Rocket is an exemplary project.

notplants commented

2021-11-04 12:25:29 +00:00

So for now I will change these to type &str, and we can test it out a bit, and confirm its all still working.

I think we should be safe from code injection attacks, as long as we don't pass strings from form and query parameters directly into system calls (Command::new) or sql commands... and even system calls I think are ok as long as we pass the input as a single argument via Command.arg("somevariablehere") which I think won't allow "somevariablehere" to be split into new commands we weren't intending to execute.

So for now I will change these to type &str, and we can test it out a bit, and confirm its all still working. I think we should be safe from code injection attacks, as long as we don't pass strings from form and query parameters directly into system calls (Command::new) or sql commands... and even system calls I think are ok as long as we pass the input as a single argument via Command.arg("somevariablehere") which I think won't allow "somevariablehere" to be split into new commands we weren't intending to execute.

👍 1

notplants commented

2021-11-04 12:28:43 +00:00

Also hopefully these are behind authenticated routes, so if users are code-injecting themselves maybe thats their own perogative.

(more just said this because it made me giggle... still a good idea ot not allow code injections...)

Also hopefully these are behind authenticated routes, so if users are code-injecting themselves maybe thats their own perogative. (more just said this because it made me giggle... still a good idea ot not allow code injections...)

😆 1

glyph commented

2021-11-05 11:23:00 +00:00

Addressed in PR#15.

Addressed in [PR#15](https://git.coopcloud.tech/PeachCloud/peach-workspace/pulls/15).

glyph closed this issue

2021-11-05 11:23:00 +00:00

Sign in to join this conversation.