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

[Python] : Cors misconfiguration #16989

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ahmed-farid-dev
Copy link
Contributor

No description provided.

Copy link
Contributor

@joefarebrother joefarebrother left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
Just a few comments:

/**
* Holds if `header` sets `Access-Control-Allow-Credentials` to `true`.
*/
private predicate setsAllowCredentials(HeaderDeclaration header) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The HeaderDeclaration concept has recently been removed from the experimental library, with its functionality and modelling being provided by the Http::Server::ResponseHeaderWrite concept from the main library (semmle.python.Concepts) instead. Uses here will need to be replaced with that concept in order for this query to compile on main.


override predicate isSink(DataFlow::Node sink) {
exists(Compare cmp, Expr left, Expr right, Cmpop cmpop |
cmpop.getSymbol() = ["==", "in", "is not", "!="] and
Copy link
Contributor

Choose a reason for hiding this comment

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

Is is also intended to be a possible comparison operator here?
Or if in is intended to be checked, should not in also be checked?

/**
* This taintflow2 configuration checks if there is a flow from source node towards CorsProbableCheckAccess methods.
*/
class CorsSourceReachesCheckConfig extends TaintTracking2::Configuration {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a more complex (and I'd say optional) change, but this second configuration could be expressed as a BarrierGaurd sanitizer instead.

@@ -0,0 +1 @@
| test.py:7:55:7:64 | ControlFlowNode for rfs_header | test.py:5:18:5:24 | ControlFlowNode for request | test.py:7:55:7:64 | ControlFlowNode for rfs_header | CORS header is being set using user controlled value $@. | test.py:5:18:5:24 | ControlFlowNode for request | user-provided value |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the test.qlref file missing here?

Cross-Origin request.
</p>

<sample src="test.py" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This example file appears to be missing.
(if the test.py file from the test directory is intended to be used as the example here, it should be copied into this directory).

Additionally, the closing </example> tag appears to be missing.

@sidshank sidshank added the awaiting-response The CodeQL team is awaiting further input or clarification from the original reporter of this issue. label Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-response The CodeQL team is awaiting further input or clarification from the original reporter of this issue. documentation external-contribution Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants