New code examples for pyscg-0036 as part of issue #1059#1078
New code examples for pyscg-0036 as part of issue #1059#1078
Conversation
Signed-off-by: s19110 <hubertdan24@gmail.com>
myteron
left a comment
myteron
left a comment
There was a problem hiding this comment.
compliant code has inline comment "Non-compliant"
gatekeeping would be nice in the compliant02.py
wording in explaining -1
| @@ -3,18 +3,25 @@ | |||
| """ Non-compliant Code Example """ | |||
There was a problem hiding this comment.
this is the compliant version
| """ Non-compliant Code Example """ | |
| """ Compliant Code Example """ |
| index_start = full_string.find(sub_string) | ||
|
|
||
| if index_start >= 0: | ||
| index_end = index_start + len(sub_string) | ||
| return (full_string[:index_start] | ||
| + "\"" | ||
| + full_string[index_start:index_end] | ||
| + "\"" | ||
| + full_string[index_end:]) | ||
| else: | ||
| print(f"There is no '{sub_string}' in '{full_string}'") | ||
| # Nothing to wrap, return unchanged string | ||
| return full_string |
There was a problem hiding this comment.
Gatekeeping is preferred over if/else.
| index_start = full_string.find(sub_string) | |
| if index_start >= 0: | |
| index_end = index_start + len(sub_string) | |
| return (full_string[:index_start] | |
| + "\"" | |
| + full_string[index_start:index_end] | |
| + "\"" | |
| + full_string[index_end:]) | |
| else: | |
| print(f"There is no '{sub_string}' in '{full_string}'") | |
| # Nothing to wrap, return unchanged string | |
| return full_string | |
| index_start = full_string.find(sub_string) | |
| if index_start < 0: | |
| # Nothing to wrap, return unchanged string | |
| return full_string | |
| index_end = index_start + len(sub_string) | |
| return (full_string[:index_start] | |
| + "\"" | |
| + full_string[index_start:index_end] | |
| + "\"" | |
| + full_string[index_end:]) |
| @@ -97,25 +101,32 @@ | |||
| """ Non-compliant Code Example """ | |||
There was a problem hiding this comment.
| """ Non-compliant Code Example """ | |
| """ Compliant Code Example """ |
| if index_start >= 0: | ||
| index_end = index_start + len(sub_string) | ||
| return (full_string[:index_start] | ||
| + "\"" | ||
| + full_string[index_start:index_end] | ||
| + "\"" | ||
| + full_string[index_end:]) | ||
| else: | ||
| print(f"There is no '{sub_string}' in '{full_string}'") | ||
| # Nothing to wrap, return unchanged string | ||
| return full_string |
There was a problem hiding this comment.
gatekeep
| if index_start >= 0: | |
| index_end = index_start + len(sub_string) | |
| return (full_string[:index_start] | |
| + "\"" | |
| + full_string[index_start:index_end] | |
| + "\"" | |
| + full_string[index_end:]) | |
| else: | |
| print(f"There is no '{sub_string}' in '{full_string}'") | |
| # Nothing to wrap, return unchanged string | |
| return full_string | |
| if index_start < 0: | |
| # Nothing to wrap, return unchanged string | |
| return full_string | |
| index_end = index_start + len(sub_string) | |
| return (full_string[:index_start] | |
| + "\"" | |
| + full_string[index_start:index_end] | |
| + "\"" | |
| + full_string[index_end:]) |
|
|
||
| ## Compliant Solution - Invalid value handling | ||
|
|
||
| Since `str.find()` indicates the fact that the sub-string couldn't be found with a negative index, a simple `if` check is enough to tackle the issue from the previous code example. |
There was a problem hiding this comment.
our wording is a bit vague how about:
| The `str.find()` method returning `-1` when it can't find the string requires to check for `-1` before using the return value as an index. |
BartKaras1128
left a comment
BartKaras1128
left a comment
There was a problem hiding this comment.
Few suggestions. Kinda nitpicking most of them to be honest, but have a look and see if you agree!
| return (full_string[:index_start] | ||
| + "\"" | ||
| + full_string[index_start:index_end] | ||
| + "\"" | ||
| + full_string[index_end:]) |
There was a problem hiding this comment.
Seems to be one space too many here
| return (full_string[:index_start] | |
| + "\"" | |
| + full_string[index_start:index_end] | |
| + "\"" | |
| + full_string[index_end:]) | |
| return (full_string[:index_start] | |
| + "\"" | |
| + full_string[index_start:index_end] | |
| + "\"" | |
| + full_string[index_end:]) |
| ##################### | ||
| # exploiting above code example | ||
| ##################### | ||
| my_string = "Secure Python coding" |
There was a problem hiding this comment.
Nitpicking here, but since we're revisiting this, should we conform to "UPPER_CASE" naming style.
Constant name "my_string" doesn't conform to UPPER_CASE naming stylePylintC0103:invalid-name
| my_string = "Secure Python coding" | |
| MY_STRING = "Secure Python coding" |
| find_in_string(my_string, "Python") | ||
| find_in_string(my_string, "I'm evil") | ||
| print(wrap_in_quotes(my_string, "Secure")) | ||
| print(wrap_in_quotes(my_string, "I'm evil")) No newline at end of file |
There was a problem hiding this comment.
Removed a newline, may as well have one in, again most linters just suggest this.
| print(wrap_in_quotes(my_string, "I'm evil")) | |
| print(wrap_in_quotes(my_string, "I'm evil")) | |
| find_in_string(my_string, "Python") | ||
| find_in_string(my_string, "I'm evil") | ||
| print(wrap_in_quotes(my_string, "Secure")) | ||
| print(wrap_in_quotes(my_string, "I'm evil")) No newline at end of file |
There was a problem hiding this comment.
Maybe just suggestion to have a newline at the end, most linters like this, lights up the IDE less!
| print(wrap_in_quotes(my_string, "I'm evil")) | |
| print(wrap_in_quotes(my_string, "I'm evil")) | |
| ##################### | ||
| # exploiting above code example | ||
| ##################### | ||
| my_string = "Secure Python coding" |
There was a problem hiding this comment.
Nitpicking here, but since we're revisiting this, should we conform to "UPPER_CASE" naming style.
Constant name "my_string" doesn't conform to UPPER_CASE naming stylePylintC0103:invalid-name
| my_string = "Secure Python coding" | |
| MY_STRING = "Secure Python coding" |
| ##################### | ||
| # exploiting above code example | ||
| ##################### | ||
| my_string = "Secure Python coding" |
There was a problem hiding this comment.
Same suggestion here as in the code example.
| my_string = "Secure Python coding" | |
| MY_STRING = "Secure Python coding" |
| ##################### | ||
| # exploiting above code example | ||
| ##################### | ||
| my_string = "Secure Python coding" |
There was a problem hiding this comment.
Same suggestion here as in the code example:
| my_string = "Secure Python coding" | |
| MY_STRING = "Secure Python coding" |
3 participants
This change aims to update the code examples from pyscg-0036 to be better alligned with CWE-252.
The second set of code examples was changed to edit the string using the index instead of just printing it. This way, we can more tangibly show negative consequences of ignoring return values that indicate errors or missing information, making it more similar to example 2 from https://cwe.mitre.org/data/definitions/252.html