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

Proposal: Add flag to scan string literals #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VoidingWarranties
Copy link

@VoidingWarranties VoidingWarranties commented Apr 17, 2017

You can't run glypcheck on itself without this flag :P

There are many cases (user messages written in other languages, i18n, etc) where you wouldn't want to scan string literals. However, use this flag with caution — as mentioned in the README, import statements and URLs use string literals.

Also, what do you think about checking if the string literal is part of an import statement? I didn't bother to implement this as you should still use caution with URLs.

String literals are scanned by default.

I'll write tests after the PR #1 is merged, and if you think this flag is a good idea.

String literals are scanned by default.
@lukechampine
Copy link
Member

I think this is a good flag to have, but I'd want it to be disabled by default (or perhaps make it -ignore-strings). The rationale being that most projects don't contain any non-ASCII characters, so they should be suspicious of everything by default. And as you mentioned, import statements (and any other URL in source code!) are one of the most dangerous attack vectors. We probably want to check import statements even if -ignore-strings is set, but that requires some more complex parsing.

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.

2 participants