No reviewers
No Label
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Plume/Plume#327
Loading…
Reference in New Issue
There is no content yet.
Delete Branch 'ructe'
Deleting a branch is permanent. It CANNOT be undone. Continue?
All the template are now compiled at compile-time with the
ructecrate.I preferred to use it instead of askama because it allows more complex Rust expressions, where askama only supports a small subset of expressions and doesn't allow them everywhere (for instance,
{{ macro!() | filter }}would result in a parsing error).The diff is quite huge, but there is normally no changes in functionality.
Fixes #161 and unblocks #110 and #273
It also updates Rocket to 0.4 RC 1, and Rocket i18n to the latest commits (that I should push btw, I completely forgot 😅).
I knew it would land to the pr x)
Yes, I just remembered I forgot it. 😁
Whoa that's a lot of work. I didn't realize there would be that much changes in here.
This is good work, however I think this might have introduced some XSSs, so I'll have to put my veto for the time being. I'll check if I'm right when the compilation end on my side.
I think we lost translation here, and I don't see it added neither on
requires_logincalls, nor in the template or the routeAs the return type is supposed to be html, maybe it should be something else (SafeString or Html), but we should make sure all user controlled parameters are properly escaped. Otherwise there could be an xss in the
altandtitlefields the user control, or insrca remote server could sendyou could use
!self.display_name.is_empty()(at least it's what clippy would tell you)@ -337,3 +335,3 @@msgid_plural "{{ count }} articles in this blog"msgid_plural "{0} articles in this blog"msgstr[0] ""msgstr[1] ""I love how it changed from a very strange and long message to a very normal and short one
@ -29,3 +30,3 @@user_id: user.id,value: random_hex(),scopes: query.scopes,scopes: query.scopes.clone(),what is the
..in a query part for? I only know its use in the path partIt would feel more at it's place in
src/template_utils.rsI thinkugh I did the same change on
search, we will have some merge issues for literally nothing 😢 (and also in a lot of other places, but for good reasons)@ -0,0 +44,4 @@pub fn avatar(conn: &Connection, user: &User, size: Size, pad: bool, catalog: &Catalog) -> Html<String> {let name = escape(&user.name(conn)).to_string();Html(format!(I haven't tested yet, but big warning on fields like
title, one could maybe use a name containing html (or js) and get it unescaped in his pageis it normal for
{0}to be translated by{0}{1}{2}?You are right, I forgot to call it from the routes (I preferred to change this behavior to avoid having to depend on rocket_i18n in plume-common, and to have an extra
Catalogparameter).@ -0,0 +12,4 @@<p>@article.subtitle</p></main><p class="author">@Html(i18n!(ctx.1, "By {0}"; format!(there is an xss injection there, because {0} is not properly escaped. Example paylod: display name =
"><script>alert("xss");</script> <div title="@ -0,0 +44,4 @@pub fn avatar(conn: &Connection, user: &User, size: Size, pad: bool, catalog: &Catalog) -> Html<String> {let name = escape(&user.name(conn)).to_string();Html(format!(I was right on this one
"><script>alert("xss");</script> <div title="as a display name trigger it@ -29,3 +30,3 @@user_id: user.id,value: random_hex(),scopes: query.scopes,scopes: query.scopes.clone(),It is the new way to handle queries in Rocket 0.4. The new syntax is:
/?<x>is a single query argument (?x=foo)&(/?<x>&<y>will match?x=foo&y=bar)..and each of its field will be a query argument (I think it was the default behavior in 0.3)No, it is a translation I forgot to update I think.
@ -0,0 +12,4 @@<p>@article.subtitle</p></main><p class="author">@Html(i18n!(ctx.1, "By {0}"; format!(Yes, I guess we could use
.to_htmlfrom Ructe to escape it https://github.com/kaj/ructe/blob/master/src/template_utils.rs#L24The same payload is also working, however as this is only used on private pages, this has less impact (one can only harm themselves)
@ -0,0 +12,4 @@<p>@article.subtitle</p></main><p class="author">@Html(i18n!(ctx.1, "By {0}"; format!(then it should be called only on L18, as to not break the link we build
@ -0,0 +12,4 @@<p>@article.subtitle</p></main><p class="author">@Html(i18n!(ctx.1, "By {0}"; format!(Would doing that break anything?
@ -0,0 +12,4 @@<p>@article.subtitle</p></main><p class="author">@Html(i18n!(ctx.1, "By {0}"; format!(it would prevent an xss, but I think a simple
"as user name could break the layout (you can test it by yourself)@ -0,0 +12,4 @@<p>@article.subtitle</p></main><p class="author">@Html(i18n!(ctx.1, "By {0}"; format!(to_htmlseems to be only to include content in the html tree, not inside tags@ -29,3 +30,3 @@user_id: user.id,value: random_hex(),scopes: query.scopes,scopes: query.scopes.clone(),how ok, I didn't know, and I prefer it a lot to the old way
Codecov Report
89.9% <ø> (-1.4%)0% <0%> (ø)0% <0%> (ø)0% <0%> (ø)0% <0%> (ø)0% <0%> (ø)0% <0%> (ø)9.81% <0%> (+0.07%)0% <0%> (ø)90.75% <0%> (ø)Continue to review full report at Codecov.
I totally understand your thought, it will be more logical to do so
and again x)
The attribute to use is action, not href (this issue was introduced on Tera)
I haven't finished yet, but I will post it now so you can start working on it if you have time
this is not supposed to be committed, you
git added to much after checking off from search (same is true for other files of the directory)there is an xss reflection here (someone can inject script, but only to themselves, or at least I think). An example payload is trying to log in as
"><script>alert("xss")</script><a href="with an invalid password@ -0,0 +53,4 @@></div>"#,size = size.as_str(),padded = if pad { "padded" } else { "" },url = user.avatar_url(conn),avatar url for remote user is currently unverified, a rogue server could maybe try to inject something through there
@ -0,0 +62,4 @@let mut res = String::from(r#"<div class="tabs">"#);for (url, title, selected) in links {res.push_str(r#"<a href=""#);res.push_str(url);in case of a user name (as in the @ they gave at signup) like
"><script>alert("xss")</script><a href=", the url can contain dangerous content. However I think user name should not contains any characters from the set"<>&, in which case there is no injection possible. So I'd argue even if the xss appear here, the real fix will be to validate user name at registrationThe author.name is not interpreted
@ -0,0 +30,4 @@<p>@i18n!(ctx.1, "No posts to see here yet.")</p>}@if is_author {<a href="/~/@fqn/new/" class="button inline-block">@i18n!(ctx.1, "New article")</a>I'm not sure why yet, but having a user&display name of
" content=shift the button to the left@ -0,0 +4,4 @@@(ctx: BaseContext, error_message: &str, error: Content)@:base_template(ctx, error_message, {}, {}, {@:error()I don't see the definition of this macro/function, could you tell me where it is?
Wouldn't it be better to have one "long" string to translate, in which we inject
format!("</p><em>{}</em><p>", n_users)? I can imagine in some languages it's more accurate to say something where "people" goes first and "home to" secondYou can do without
format(not sure if it's better though)this button should not be here if MediaCategory != Image
(was true for Tera too though)
same about translating with a single string
@ -0,0 +5,4 @@<div class="card">@if article.cover_id.is_some() {<div class="cover" style="background-image: url('@Html(article.cover_url(ctx.0).unwrap_or_default())')"></div>a rogue server could have sent us an invalid url containing dangerous content. Not sure if it's really an attack vector yet, but this might very well be
@ -0,0 +104,4 @@<p aria-label="@i18n!(ctx.1, "One like", "{0} likes", &n_likes)" title="@i18n!(ctx.1, "One like", "{0} likes", n_likes)">@n_likes</p><a href="/login?m=Login%20to%20like" class="action">@icon!("heart") @i18n!(ctx.1, "Add yours")</a>Message won't get translated
@ -0,0 +111,4 @@<p aria-label="@i18n!(ctx.1, "One boost", "{0} boost", &n_reshares)" title="@i18n!(ctx.1, "One boost", "{0} boosts", n_reshares)">@n_reshares</p><a href="/login?m=Login%20to%20boost" class="action">@icon!("repeat") @i18n!(ctx.1, "Boost")</a>Message won't get translated
I have to say I prefer partial templates included in other templates over functions returning an
Html<String>, because in the latter case ructe will consider all as trusted, whereas with partial template it will try to escape as much as it can@ -0,0 +9,4 @@@:header(ctx, &user, follows, is_remote, remote_url)@tabs(&[(&format!("/@/{}", user.get_fqn(ctx.0)), i18n!(ctx.1, "Articles"), true),user fqn can't be assumed to be trusted yet, one can inject content by this way
@ -0,0 +8,4 @@@:header(ctx, &user, follows, is_remote, remote_url)@tabs(&[(&format!("/@/{}", user.get_fqn(ctx.0)), i18n!(ctx.1, "Articles"), false),same here, user fqn can't be trusted yet
@ -0,0 +62,4 @@let mut res = String::from(r#"<div class="tabs">"#);for (url, title, selected) in links {res.push_str(r#"<a href=""#);res.push_str(url);fixed by
9714bafded@ -0,0 +30,4 @@<p>@i18n!(ctx.1, "No posts to see here yet.")</p>}@if is_author {<a href="/~/@fqn/new/" class="button inline-block">@i18n!(ctx.1, "New article")</a>this was only on user name (fqn), fixed by
9714bafdedthen@ -0,0 +9,4 @@@:header(ctx, &user, follows, is_remote, remote_url)@tabs(&[(&format!("/@/{}", user.get_fqn(ctx.0)), i18n!(ctx.1, "Articles"), true),fixed by
9714bafded@ -0,0 +8,4 @@@:header(ctx, &user, follows, is_remote, remote_url)@tabs(&[(&format!("/@/{}", user.get_fqn(ctx.0)), i18n!(ctx.1, "Articles"), false),fixed by
9714bafded@ -0,0 +104,4 @@<p aria-label="@i18n!(ctx.1, "One like", "{0} likes", &n_likes)" title="@i18n!(ctx.1, "One like", "{0} likes", n_likes)">@n_likes</p><a href="/login?m=Login%20to%20like" class="action">@icon!("heart") @i18n!(ctx.1, "Add yours")</a>actually as there are route for it, and has_liked and has_reshared are both set to false by default, the whole
@if ctx.2.is_some() {} else {}could be turned into a single branch. That way there is less code and it work better. The only issue is there would be no csrf token (no session cookie so rocket_csrf don't issue a token), so the user would be faced with an invalid csrf page. I should probably make rocket_csrf accept request with no cookie, not only response. Anyway it make no sens to do a csrf attack on a client having no session. And this would also remove most exceptions we set in mainEdit: I've added the functionality to latest rocket_csrf/master
use more raw strings!
maybe use
so you don't have to keep escaping the inner
"@ -0,0 +5,4 @@<div class="card">@if article.cover_id.is_some() {<div class="cover" style="background-image: url('@Html(article.cover_url(ctx.0).unwrap_or_default())')"></div>Fixed by
ed71d24fe9@ -0,0 +53,4 @@></div>"#,size = size.as_str(),padded = if pad { "padded" } else { "" },url = user.avatar_url(conn),fixed by
ed71d24fe9@igalic because I always do this mistake, warning on the end of your string, the correct syntax is
r#"some content"#@ -0,0 +4,4 @@@(ctx: BaseContext, error_message: &str, error: Content)@:base_template(ctx, error_message, {}, {}, {@:error()It comes from the
errorparameter of this template. TheContenttype is a special type used by Ructe to pass template fragments to functions. Then, to inherit a template, you can call it (as we are doing with@:base_templatehere) and give it fragments of templates to be included as arguments between{}.@ -0,0 +4,4 @@@(ctx: BaseContext, error_message: &str, error: Content)@:base_template(ctx, error_message, {}, {}, {@:error()How ok I through it was a function call when it's more of a "insert the content of this var here". It's the same syntax as in
templates/base.rs.htmlwhere it didn't bother me x)I think this is easier for translators to add new languages in here than it is to edit main.rs. Maybe include_i18! should read this file instead of getting languages from a static parameter (actually it could accept both), what do you think @BaptisteGelez ?
Ideally, I think include_i18n! should read all the .mo files in
translationsif no language list is provided. This way new languages would be loaded automatically, without needing to add them anywhere.this would be perfect, however I'm not sure this can be done with macros. Still I'll try to do it
It seems like it's not possible to
lswith normal macros, however it exist crates doing more or less what we want here (we could also use it to embed wasm files later on) and thereI think using any of those isn't in the range of this pr anyway, a macro is a thing, a dependency is another
you forgot to add it to include_i18n
Reviewers
70af57c6e1.Step 1:
From your project repository, check out a new branch and test the changes.Step 2:
Merge the changes and update on Forgejo.