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

Exclude hugo specific safe* keyword from squirrelly analysis #118

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

heurtematte
Copy link

@heurtematte heurtematte commented Apr 8, 2024

The squirrelly_template regex produces false positives on Hugo projects code-based.

The regex excludes keywords: safeURL/safeHTML/safeJS/ ... https://gohugo.io/functions/safe/

…y true positives

Signed-off-by: sebastien.heurtematte <sebastien.heurtematte@eclipse-foundation.org>
@ajinabraham
Copy link
Owner

Ain't safeHTML/safeJS also allow to pass unescaped variables to the template for Hugo?

@heurtematte
Copy link
Author

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.

@ajinabraham
Copy link
Owner

This is very much applicable to Hugo templates as well as suggested here: https://gohugo.io/about/security/#web-application-security

@heurtematte
Copy link
Author

heurtematte commented Apr 10, 2024

I can understand your point but as mentioned in the link: It may be worth adding that Hugo is a static site generator with no concept of dynamic user input.

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
If no, clarify the message of this rule by not targeting a specific framework like squirrelly but as a general rule

WDYT?

@ajinabraham
Copy link
Owner

ajinabraham commented Apr 11, 2024

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 <sebastien.heurtematte@eclipse-foundation.org>
@@ -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.*}}'
Copy link
Owner

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.

https://regex101.com/r/a3QdmS/1

Copy link
Author

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.

Copy link
Owner

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.

Copy link
Author

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]*?}}

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.

None yet

2 participants