-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Exclude hugo specific safe* keyword from squirrelly analysis #118
base: master
Are you sure you want to change the base?
Conversation
…y true positives Signed-off-by: sebastien.heurtematte <[email protected]>
Ain't |
This is a bit different in this context. Hugo is a static site generator with no concept of dynamic user input. While Squirrelly or any other templating framework allows the creation of forms. |
This is very much applicable to Hugo templates as well as suggested here: https://gohugo.io/about/security/#web-application-security |
I can understand your point but as mentioned in the link: So my question in this situation, do we trust the author of the code? If yes, The PR is still relevant, or If you prefer maybe creating a dedicated rule for hugo with a different severity: e.g: warning WDYT? |
The intention of these rules is to catch accidental mistakes (Trust, but verify). It is a common to consider something as "safe" and end up passing untrusted user data which can be abused for code injection. In this specific case , it's alright to create a separate rule for Hugo with a WARNING severity while ignoring the same for Squirrelly templates. |
Signed-off-by: sebastien.heurtematte <[email protected]>
@@ -69,13 +69,25 @@ | |||
message: The Squirrelly.js template has an unescaped variable. Untrusted user input | |||
passed to this variable results in Cross Site Scripting (XSS) | |||
type: Regex | |||
pattern: '{{.+\|.*safe.*}}' | |||
pattern: '{{(?!.*(?:safeURL|safeHTML|safeCSS|safeJS|safeJSStr|safeHTMLAttr)).*\|.*safe.*}}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a problem with this regex, it will skip variable names with safeURL/.. in them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have excluded all Hugo-related keywords from this rule and used those keywords in the specific Hugo rule. Otherwise I'll still have error on hugo SAFE keyword instead of the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to move the Hugo specific keywords after .*\|
so that it ignores something like {{ foo | safeURL }}
but still catch{{ safeURL | safe }}
for this Squirrelly.js rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
I propose a new way this regex could work.
WDYT of this: {{[\s\S]*?\|\s*\bsafe\b[\s\S]*?}}
The
squirrelly_template
regex produces false positives on Hugo projects code-based.The regex excludes keywords: safeURL/safeHTML/safeJS/ ... https://gohugo.io/functions/safe/