make blog/instance description a SafeString #223

trinity-1686a · 2018-09-14T13:15:03Z

igalic commented

2018-09-14 13:15:03 +00:00

(Migrated from github.com)

long_description & short_description's documentation say they can be
Markdown, but they are String, not SafeString.

This led to escaped strings being printed in the editor
https://github.com/Plume-org/Plume/issues/220

long_description & short_description's documentation say they can be Markdown, but they are String, not SafeString. This led to escaped strings being printed in the editor https://github.com/Plume-org/Plume/issues/220

👍 2 ❤️ 1

igalic commented

2018-09-14 13:15:28 +00:00

(Migrated from github.com)

n.b.: This is a work-in-progress and does not solve the problem yet

n.b.: This is a work-in-progress and does ***not*** solve the problem yet

pwoolcoc commented

2018-09-14 13:44:02 +00:00

(Migrated from github.com)

For the FromFormValue problem, you should be able to delegate the parsing to the String::from_form_value method. I haven't compiled this but it should go something like:

// safe_string.rs
use rocket::request::FromFormValue;
use rocket::http::RawStr;

impl<'v> FromFormValue<'v> for SafeString {
    type Error = &'v RawStr;

    fn from_form_value(form_value: &'v RawStr) -> Result<SafeString, &'v RawStr> {
        let val = String::from_form_value(form_value)?;
        Ok(SafeString {
          value: val,
        })
    }
}

For the `FromFormValue` problem, you should be able to delegate the parsing to the `String::from_form_value` method. I haven't compiled this but it should go something like: ``` // safe_string.rs use rocket::request::FromFormValue; use rocket::http::RawStr; impl<'v> FromFormValue<'v> for SafeString { type Error = &'v RawStr; fn from_form_value(form_value: &'v RawStr) -> Result<SafeString, &'v RawStr> { let val = String::from_form_value(form_value)?; Ok(SafeString { value: val, }) } } ```

👍 1

pwoolcoc (Migrated from github.com) reviewed 2018-09-14 13:56:17 +00:00

plume-models/src/blogs.rs

pwoolcoc (Migrated from github.com) commented

2018-09-14 13:56:17 +00:00

you can change &<String>::new here to just "", since SafeString::new() takes a &str. It will save an allocation, at least, especially since currently the allocation just gets dropped and another is made in the ammonia::clean function.

you can change `&<String>::new` here to just `""`, since `SafeString::new()` takes a `&str`. It will save an allocation, at least, especially since currently the allocation just gets dropped and another is made in the `ammonia::clean` function.

pwoolcoc (Migrated from github.com) reviewed 2018-09-14 13:56:36 +00:00

plume-models/src/users.rs

pwoolcoc (Migrated from github.com) commented

2018-09-14 13:56:36 +00:00

Same comment here re: String::new

Same comment here re: `String::new`

pwoolcoc (Migrated from github.com) reviewed 2018-09-14 13:56:57 +00:00

src/setup.rs

pwoolcoc (Migrated from github.com) commented

2018-09-14 13:56:56 +00:00

and here as well

igalic commented

2018-09-14 16:33:35 +00:00

(Migrated from github.com)

@pwoolcoc thank you for your review!
i've pushed one commit which addresses your concerns wrt allocation, and one which adds your imp FromFormValue for SafeString… now… to make use of it ;)

(by which i mean, "now to find out how to make use of it")

@pwoolcoc thank you for your review! i've pushed one commit which addresses your concerns wrt allocation, and one which adds your `imp FromFormValue for SafeString`… now… to make use of it ;) (by which i mean, "now to find out how to make use of it")

elegaanz commented

2018-09-14 16:52:29 +00:00

(Migrated from github.com)

@igalic Thanks for working on this. Rocket will use your FromFormValue directly if you replace the String fields of the form struct (InstanceSettingsForm in src/routes/instance.rs) by SafeString.

@igalic Thanks for working on this. Rocket will use your `FromFormValue` directly if you replace the `String` fields of the form struct (`InstanceSettingsForm` in `src/routes/instance.rs`) by `SafeString`.

trinity-1686a requested changes 2018-09-14 17:29:30 +00:00

plume-models/src/safe_string.rs

trinity-1686a commented

2018-09-14 17:29:18 +00:00

SafeStrings are cleaned at creation, you must either call SafeString::new(&str) or ammonia::clean(&str) (which is used internally by new) when creating such struct, not built it directly from an unstrusted String.

`SafeString`s are cleaned at creation, you must either call `SafeString::new(&str)` or `ammonia::clean(&str)` (which is used internally by new) when creating such struct, not built it directly from an unstrusted String.

igalic (Migrated from github.com) reviewed 2018-09-14 17:54:48 +00:00

igalic (Migrated from github.com) left a comment

big confused about @pwoolcoc's code here ;)

plume-models/src/safe_string.rs

					
				@ -104,0 +106,4 @@

				use rocket::http::RawStr;

				impl<'v> FromFormValue<'v> for SafeString {

				    type Error = &'v RawStr;

igalic (Migrated from github.com) commented

2018-09-14 17:54:28 +00:00

can someone help me understand what 'v means?

can someone help me understand what `'v` means?

igalic (Migrated from github.com) reviewed 2018-09-14 17:55:40 +00:00

plume-models/src/safe_string.rs

igalic (Migrated from github.com) commented

2018-09-14 17:55:40 +00:00

this should be fixed in d62c72d

this should be fixed in d62c72d

trinity-1686a reviewed 2018-09-14 18:00:25 +00:00

plume-models/src/safe_string.rs

					
				@ -104,0 +106,4 @@

				use rocket::http::RawStr;

				impl<'v> FromFormValue<'v> for SafeString {

				    type Error = &'v RawStr;

trinity-1686a commented

2018-09-14 18:00:25 +00:00

'v is a lifetime, because you got a reference to a RawStr, not a RawStr directly. This helps Rust figure out how long a struct containing pointers (references) can live, and when some parts of it might got dropped and it's therefore no longer safe to access a struct. You can read more about it in rust's book, which probably explain better than me 😉

`'v` is a lifetime, because you got a reference to a `RawStr`, not a `RawStr` directly. This helps Rust figure out how long a struct containing pointers (references) can live, and when some parts of it might got dropped and it's therefore no longer safe to access a struct. You can read more about it in [rust's book](https://doc.rust-lang.org/1.9.0/book/lifetimes.html), which probably explain better than me :wink:

trinity-1686a approved these changes 2018-09-14 18:00:59 +00:00

igalic (Migrated from github.com) reviewed 2018-09-14 18:31:28 +00:00

plume-models/src/safe_string.rs

					
				@ -104,0 +106,4 @@

				use rocket::http::RawStr;

				impl<'v> FromFormValue<'v> for SafeString {

				    type Error = &'v RawStr;

igalic (Migrated from github.com) commented

2018-09-14 18:31:27 +00:00

okay, thanks! i was just not clear on the notation

igalic commented

2018-09-14 18:32:35 +00:00

(Migrated from github.com)

okay! so with the last commit (06718a5c8a) i've now done everything everyone on here has so helpfully suggested

which means that it now… still doesn't work 😅

okay! so with the last commit (https://github.com/Plume-org/Plume/pull/223/commits/06718a5c8a0af6c9facd10debfadae100fd25aa2) i've now done everything everyone on here has so helpfully suggested which means that it now… still doesn't work 😅

trinity-1686a commented

2018-09-14 19:08:49 +00:00

I think you did all that was needed on rust side. Now you just have to edit templates to add | safe whenever one of the vars you changed to SafeString is used as done here for user's summary d355379e01/templates/users/header.html.tera (L41)
This should be in templates/instance/admin.html.tera I think.

I think you did all that was needed on rust side. Now you just have to edit templates to add `| safe` whenever one of the vars you changed to SafeString is used as done here for user's summary https://github.com/Plume-org/Plume/blob/d355379e01006938ef87d230b456509a9731294c/templates/users/header.html.tera#L41 This should be in `templates/instance/admin.html.tera` I think.

igalic commented

2018-09-14 19:10:27 +00:00

(Migrated from github.com)

but that's not done with content:
d355379e01/templates/posts/new.html.tera (L31)

but that's not done with content: https://github.com/Plume-org/Plume/blob/d355379e01006938ef87d230b456509a9731294c/templates/posts/new.html.tera#L31

igalic commented

2018-09-14 19:25:07 +00:00

(Migrated from github.com)

@fdb-hiroshima i tried | safe anyway, and it did nothing!

@fdb-hiroshima i tried `| safe` anyway, and it did nothing!

trinity-1686a commented

2018-09-14 19:37:02 +00:00

The exact line you need is <textarea id="short_description" name="short_description">{{ form.short_description | default(value=instance.short_description | safe) }}</textarea>, with the | safe inside default's brackets. The difference with post's content is instance.short_description is read by default, not passed directly (same goes with long_description). I'm sorry I was not clear enough in my previous comment

The exact line you need is `<textarea id="short_description" name="short_description">{{ form.short_description | default(value=instance.short_description | safe) }}</textarea>`, with the `| safe` inside `default`'s brackets. The difference with post's content is `instance.short_description` is read by `default`, not passed directly (same goes with `long_description`). I'm sorry I was not clear enough in my previous comment

👍 2 🎉 2

elegaanz (Migrated from github.com) approved these changes 2018-09-14 19:56:04 +00:00

Sign in to join this conversation.

No reviewers

elegaanz

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Plume/Plume#223

Rows
Columns