Refactor some Str2html code #29397
Merged
Refactor some Str2html code #29397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wxiaoguang
added
the
type/refactoring
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/L
wxiaoguang
force-pushed
the
refactor-str2html
branch
4 times, most recently
from
70d39fc
to
d69dbbe
Compare
wxiaoguang
force-pushed
the
refactor-str2html
branch
from
d69dbbe
to
18c5653
Compare
|
Ideally we should ensure that Go's contextual auto-escaping works wherever possible to avoid needing such helpers. Are the limitations of this auto-escaper well-known? |
What I am doing is this. You can see we don't need "Safe" or "Escape" in templates anymore. "limitations of this auto-escaper": there are many problems: it has limitations, that's why there were a lot of "printf" "Escape" "Safe" and "QueryEscape" in code. Some could be improved by our framework code, some can't be avoided. |
in my opinion no ... it's to error prone |
lunny
approved these changes
Feb 29, 2024
GiteaBot
added
lgtm/need 1
6543
reviewed
Feb 29, 2024
6543
approved these changes
Mar 1, 2024
GiteaBot
added
lgtm/done
zjjhot
added a commit
to zjjhot/gitea
that referenced
this pull request
Mar 1, 2024
* giteaofficial/main: Set pre-step status to `skipped` if job is skipped (go-gitea#29489) Use a predictiable fork URL to allow forking repositories without providing a repo ID (go-gitea#29519) Adding back missing options to app.example.ini (go-gitea#29511) Refactor the "attachments" sub-template data key to RenderedContent (go-gitea#29517) Rename Str2html to SanitizeHTML and clarify its behavior (go-gitea#29516) Add admin API route for managing user's badges (go-gitea#23106) Refactor some Str2html code (go-gitea#29397) Move migration functions to services layer (go-gitea#29497)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR touches the most interesting part of the "template refactoring".
Before, "Tr", "Escape" and "Safe" are quite straightforward, they just escapes and casts.
But "Str2html" is special: it sanitizes the input and casts the "safe" result to HTML type.
During the refactoring, I can see that there are many legacy problems, for example:
project.RenderedContent = project.Descriptionin web/org/projects.gor.Note = rendered contentin web/repo/release.go{{Str2html .PackageDescriptor.Metadata.ReleaseNotes}}in package/content/nuget.tmpl, I guess the name Str2html misleads developers to use it to "render string to html", but it only sanitizes .... well, if ReleaseNotes really contains HTML, then this is not a problem, but I haven't figured out it yet.There is still a question: should we encourage to use "Str2html" in templates in the future? Actually, variables like "RenderedContent" are generated by "markup.Render", it has already been sanitized by the renderer. Although double-sanitizing doesn't cause side-effect, I don't know whether people would to keep using it in templates (for a sense of security) or just only use it when it is necessary (for making code to have clearly defined behaviors)