Skip to content

SONARPY-489 Rule S2092: Creating cookies without the "secure" flag is security-sensitive#548

Merged
andrea-guarino-sonarsource merged 7 commits into
masterfrom
SONARPY-489
Feb 4, 2020
Merged

SONARPY-489 Rule S2092: Creating cookies without the "secure" flag is security-sensitive#548
andrea-guarino-sonarsource merged 7 commits into
masterfrom
SONARPY-489

Conversation

@andrea-guarino-sonarsource
Copy link
Copy Markdown
Contributor

No description provided.

@andrea-guarino-sonarsource andrea-guarino-sonarsource changed the title tmp SOANRPY-489 Rule S2053: Hashes should include an unpredictable salt Feb 3, 2020
@andrea-guarino-sonarsource andrea-guarino-sonarsource changed the title SOANRPY-489 Rule S2053: Hashes should include an unpredictable salt SONARPY-489 Rule S2053: Hashes should include an unpredictable salt Feb 3, 2020
@andrea-guarino-sonarsource andrea-guarino-sonarsource changed the title SONARPY-489 Rule S2053: Hashes should include an unpredictable salt SONARPY-489 Rule S2092: Creating cookies without the "secure" flag is security-sensitive Feb 3, 2020
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understood correctly, we won't cover this case at first, so I guess it might be worth updating the rule description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation seems to assume the secure flag will always be passed as a keyword argument, with a risk of FP if it's passed as a positional argument. I haven't checked for django but I believe it's possible to use positional arguments with flask at least.
Would you consider accounting for positional arguments as well to avoid those FP (maybe by doing something similar to PredictableSaltCheck) ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good point, I'll check for positional argument

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to rename this variable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, thanks! :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought: do you think it would actually be feasible to raise on cases like:
cookie['c1'] = 'value' # FN
by checking whether the cookie symbol has another assignment usage with the same c1 subscript as well as a secure one?
Maybe it's not worth doing as I believe this is not the usual way to set cookies and it might be more complex to do than it looks, but I prefer mentioning it, feel free to disregard this!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point. However I have the feeling that the value doesn't justify the extra complexity. I suggest to create a ticket to keep track of this FN.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

@guillaume-dequenne guillaume-dequenne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment about Optional#orElse, otherwise lgtm

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use Optional#orElseGet instead of orElse ? As the latter is always evaluated no matter whether the former is present or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! thanks!

@andrea-guarino-sonarsource andrea-guarino-sonarsource merged commit 8cfd979 into master Feb 4, 2020
@andrea-guarino-sonarsource andrea-guarino-sonarsource deleted the SONARPY-489 branch February 4, 2020 09:08
hashicorp-vault-sonar-prod Bot pushed a commit that referenced this pull request Oct 3, 2025
GitOrigin-RevId: 6f869848853e50be065ae5cb257c74c3691001d0
hashicorp-vault-sonar-prod Bot pushed a commit that referenced this pull request Oct 15, 2025
GitOrigin-RevId: 6f869848853e50be065ae5cb257c74c3691001d0
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.

3 participants