Skip to content

SONARPY-358 RSPEC-4423 weak protocol version#279

Merged
benzonico merged 1 commit intomasterfrom
SONARPY-358
Aug 30, 2019
Merged

SONARPY-358 RSPEC-4423 weak protocol version#279
benzonico merged 1 commit intomasterfrom
SONARPY-358

Conversation

@benzonico
Copy link
Copy Markdown
Contributor

No description provided.

private static boolean isWeakProtocol(PyNameTree pyNameTree, @Nullable Symbol symbol) {
Predicate<String> matchWeakProtocol;
if (symbol == null) {
matchWeakProtocol = s -> s.substring(s.lastIndexOf('.') + 1).equals(pyNameTree.name());
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 don't get why we need that. I think we have a problem because, in the unit test, this method returns true only when we go through this branch. The other branch, where we use the symbol, doesn't seem to raise issues. Maybe it's related to https://github.com/SonarSource/sonar-python/blob/master/python-squid/src/main/java/org/sonar/python/semantic/SymbolTableBuilderVisitor.java#L329

Maybe we should either:

  • create a ticket to fix the problem later
  • or fix the problem in the symbol table right away

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.

Problem is deeper : this is completely linked to semantic: this is about solving dotted names : in order to get fully qualified name of the constant we need to figure out fqn of its parent and right now the SymbolTableVisitor does not deal with this kind of constructs (if i'm correct, it only deals with the first part of a dotted name).
As I did not want to address semantic in the implementation of this new rule and as I managed to make it work I believe this is ok.
Not sure which kind of ticket I should create for such case to be honest.

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.

Let me jump in the discussion. Just to clarify, currently the semantic has some limitations and inconsistencies regarding symbol resolution:

from foo import A
from foo.A import fn

A.fn() # Here "getSymbol()" will retrieve the correct symbol for "A.fn" PythonGrammar.ATTRIBUTE_REF (corresponding to PyQualifiedExpressionTree), but not for "fn" PythonGrammar.ATOM node

fn() # Here "getSymbol()" will retrieve the correct symbol for "fn" PythonGrammar.ATOM node

I agree we shouldn't fix the semantics right now, I can create a ticket for 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.

@andrea-guarino-sonarsource be my guest and please link it here !

@@ -0,0 +1,34 @@
{
"title": "Weak SSL\/TLS protocols should not be used",
"type": "VULNERABILITY",
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.

Shouldn't it be a hotspot?

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.

This is a vulnerability as per PM specification, summoning @pierre-loup-tristant-sonarsource for confirmation.

Choose a reason for hiding this comment

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

It is a vulnerability for Python as it also is for Java and PHP.

@benzonico benzonico merged commit 82b64d7 into master Aug 30, 2019
@benzonico benzonico deleted the SONARPY-358 branch August 30, 2019 14:07
hashicorp-vault-sonar-prod Bot pushed a commit that referenced this pull request May 26, 2025
…) prevents short-circuiting - rewrite as a generator (#279)

GitOrigin-RevId: b06eb8e0ee3fe61706c21c4842419dbb655436f2
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.

4 participants