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
Go: Add Rs Cors Support #14873
base: main
Are you sure you want to change the base?
Go: Add Rs Cors Support #14873
Conversation
| /** | ||
| * Provides abstract class for modeling the Go CORS handler model origin write. | ||
| */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /** | |
| * Provides abstract class for modeling the Go CORS handler model origin write. | |
| */ | |
| /** | |
| * A Go CORS handler model origin write. | |
| */ |
| /** | ||
| * Provides abstract class for modeling the Go CORS handler model allow all origins write. | ||
| */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
| /** | ||
| * Provides abstract class for modeling the Go CORS handler model allow credentials write. | ||
| */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abstract classes should not be in RsCors.qll. I'd either put them in go/ql/lib/semmle/go/Concepts.qll or in a new file in go/ql/lib/semmle/go/concepts/ which you then import in go/ql/lib/semmle/go/Concepts.qll.
| /** | ||
| * Provides abstract class for modeling the Go CORS handler model origin write. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /** | |
| * Provides abstract class for modeling the Go CORS handler model origin write. | |
| */ | |
| /** | |
| * A Go CORS handler model origin write. | |
| */ |
|
Actually, since these abstract classes are only used by this one query, they shouldn't go in concepts (which is more meant for abstract classes that are used by multiple queries, I think). And I also didn't notice that this query is still in experimental. That makes it difficult to know where to put them. Maybe where they are is good enough for now. Do you think the query should be promoted? |
| } | ||
|
|
||
| /** | ||
| * A variable of type Options that holds the headers to be set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually correct: if v is a.b.c then this is the Variable a and the thing that has type Options is a.b.c. I didn't notice this when you originally modeled GinCors. Can I ask why you are doing it this way? Is there a particular case where just using Variable or just using SsaWithFields doesn't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at it more carefully, I see that, because of the way that they are modeled, RsOptions and GinConfig are confined to be local variables defined in functions. Is that what you intended? Might you ever want to reason about one that is defined at package scope?
Add RsCors support to CodeQL CorsMisconfiguration.ql