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

make blog/instance description a SafeString #223

Merged
merged 6 commits into from
Sep 14, 2018
Merged

make blog/instance description a SafeString #223

merged 6 commits into from
Sep 14, 2018

Conversation

igalic
Copy link
Contributor

@igalic igalic commented Sep 14, 2018

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
#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
#220
@igalic
Copy link
Contributor Author

igalic commented Sep 14, 2018

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

@pwoolcoc
Copy link

pwoolcoc commented Sep 14, 2018

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,
        })
    }
}

@@ -142,8 +143,8 @@ impl Blog {
name: inst.clone(),
local: false,
// We don't really care about all the following for remote instances
long_description: String::new(),
short_description: String::new(),
long_description: SafeString::new(&<String>::new()),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -205,8 +205,8 @@ impl User {
public_domain: inst.clone(),
local: false,
// We don't really care about all the following for remote instances
long_description: String::new(),
short_description: String::new(),
long_description: SafeString::new(&<String>::new()),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here re: String::new

src/setup.rs Outdated
@@ -152,8 +153,8 @@ fn quick_setup(conn: DbConn) {
public_domain: domain,
name: name,
local: true,
long_description: String::new(),
short_description: String::new(),
long_description: SafeString::new(&<String>::new()),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here as well

follow review from @pwoolcoc, and do not use

    SafeString::new(&<String>::new())

since this makes an allocation which will then just be thrown away.
Instead, we pass ""
@igalic
Copy link
Contributor Author

igalic commented Sep 14, 2018

@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
Copy link
Member

@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.

plume-models/src/safe_string.rs Outdated Show resolved Hide resolved
thanks to @fdb-hiroshima for this review!
Copy link
Contributor Author

@igalic igalic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

big confused about @pwoolcoc's code here ;)

plume-models/src/safe_string.rs Show resolved Hide resolved
@igalic
Copy link
Contributor Author

igalic commented Sep 14, 2018

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

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

@trinity-1686a
Copy link
Contributor

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

{{ user.summary | safe }}

This should be in templates/instance/admin.html.tera I think.

@igalic
Copy link
Contributor Author

igalic commented Sep 14, 2018

but that's not done with content:

<textarea id="content" name="content" rows="20">{{ form.content | default(value="") }}</textarea>

@igalic
Copy link
Contributor Author

igalic commented Sep 14, 2018

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

@trinity-1686a
Copy link
Contributor

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

thanks again to @fdb-hiroshima for pointing me in the right direction!
@elegaanz elegaanz merged commit eb24ba1 into Plume-org:master Sep 14, 2018
@igalic igalic deleted the fix/safe-string branch September 14, 2018 19:59
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.

4 participants