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.

Sign up for GitHub

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

Jump to bottom

Modify rule S4507: Add support for Flask-GraphQL #3428

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

Modify rule S4507: Add support for Flask-GraphQL #3428

wants to merge 7 commits into from

Conversation

daniel-teuchert-sonarsource
Copy link
Contributor

Review

A dedicated reviewer checked the rule description successfully for:

  • logical errors and incorrect information
  • information gaps and missing content
  • text style and tone
  • PR summary and labels follow the guidelines

pierre-loup-tristant-sonarsource
pierre-loup-tristant-sonarsource approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. Some minor comments regarding the consistency of code examples.

rules/S4507/python/how-to-fix-it/flask-graphql.adoc Outdated
[source,python]
----
from flask import Flask
from flask_graphql import GraphQLView
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from flask_graphql import GraphQLView
from graphql_server.flask import GraphQLView

I suggest using a different import since flask_graphql is now part of graphql_server (see README)

In general, try to be consistent with the code example we already have in S6786

Copy link
Contributor

Choose a reason for hiding this comment

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

@daniel-teuchert-sonarsource not sure you saw this one.

pierre-loup-tristant-sonarsource
pierre-loup-tristant-sonarsource requested changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

I just notice the asciidoc check is failing with the following errors:

Rule python:S4507 has a "Code examples" subsection with an unallowed name: "Sensitive Code Example"
Rule python:S4507 has an unconventional header "How to fix it in Django"

@daniel-teuchert-sonarsource
Copy link
Contributor Author

I missed that hotspots do not make use of the "How to fix it in" sections.
I adjusted the format.

pierre-loup-tristant-sonarsource
pierre-loup-tristant-sonarsource requested changes Nov 13, 2023
View reviewed changes
rules/S4507/python/rule.adoc Outdated
@@ -4,8 +4,6 @@ include::../ask-yourself.adoc[]

include::../recommended.adoc[]

== Sensitive Code Example
Copy link
Contributor

Choose a reason for hiding this comment

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

This section title should not be removed.

pierre-loup-tristant-sonarsource
pierre-loup-tristant-sonarsource approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM.
Before merging make sure you read the comment I made earlier this morning #3428 (comment)

@daniel-teuchert-sonarsource
Copy link
Contributor Author

LGTM. Before merging make sure you read the comment I made earlier this morning #3428 (comment)

Thanks! I overlooked this. I will make this adjustment. The PR will be merged once the implementation ticket is done.

@sonarqube-next SonarQube-Next
Copy link

Quality Gate passed Quality Gate passed for 'rspec-frontend'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No Coverage information No data about Coverage
No Duplication information No data about Duplication

See analysis details on SonarQube

@sonarqube-next SonarQube-Next
Copy link

Quality Gate passed Quality Gate passed for 'rspec-tools'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No Coverage information No data about Coverage
No Duplication information No data about Duplication

See analysis details on SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierre-loup-tristant-sonarsource pierre-loup-tristant-sonarsource pierre-loup-tristant-sonarsource approved these changes

Assignees
No one assigned
Labels
appsec python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daniel-teuchert-sonarsource @pierre-loup-tristant-sonarsource