-
Notifications
You must be signed in to change notification settings - Fork 1.8k
JS: Move cors-misconfiguration query from experimental to Security #20146
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
base: main
Are you sure you want to change the base?
JS: Move cors-misconfiguration query from experimental to Security #20146
Conversation
QHelp previews: javascript/ql/src/Security/CWE-942/CorsPermissiveConfiguration.qhelpPermissive CORS configurationA server can use CORS (Cross-Origin Resource Sharing) to relax the restrictions imposed by the Same-Origin Policy, allowing controlled, secure cross-origin requests when necessary. A server with an overly permissive CORS configuration may inadvertently expose sensitive data or enable CSRF attacks, which allow attackers to trick users into performing unwanted operations on websites they're authenticated to. RecommendationWhen the When the If the ExampleIn the following example, import { ApolloServer } from 'apollo-server';
var https = require('https'),
url = require('url');
var server = https.createServer(function () { });
server.on('request', function (req, res) {
// BAD: origin is too permissive
const server_1 = new ApolloServer({
cors: { origin: true }
});
let user_origin = url.parse(req.url, true).query.origin;
// BAD: CORS is controlled by user
const server_2 = new ApolloServer({
cors: { origin: user_origin }
});
}); To fix these issues, import { ApolloServer } from 'apollo-server';
var https = require('https'),
url = require('url');
var server = https.createServer(function () { });
server.on('request', function (req, res) {
// GOOD: origin is restrictive
const server_1 = new ApolloServer({
cors: { origin: false }
});
let user_origin = url.parse(req.url, true).query.origin;
// GOOD: user data is properly sanitized
const server_2 = new ApolloServer({
cors: { origin: (user_origin === "https://allowed1.com" || user_origin === "https://allowed2.com") ? user_origin : false }
});
}); References
|
The query is a near duplicate of the existing |
227071b
to
1c33a0a
Compare
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.
Actually I'm not sure merging the queries was actually the right call. There are real use-cases for using *
as the origin, especially when not allowing credential transfer, and AFAIU the new query just flags *
origin as bad without further context.
The original name, permissive configuration, was more apt as it didn't imply mis-configuration, it just detected permissive configurations.
|
||
/** An overly permissive value for `origin` (Apollo) */ | ||
class TrueNullValue extends Source { | ||
TrueNullValue() { this.mayHaveBooleanValue(true) or this.asExpr() instanceof NullLiteral } |
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 null case is already covered by the NullToStringValue
class right above here.
Also, while you're at it, could you change mayHaveStringValue()
to use getStringValue()
. Since we're defining a data flow source, we should only use the actual string literal as the source, not its local aliases. Same for mayHaveBooleanValue
.
...lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll
Outdated
Show resolved
Hide resolved
...lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll
Outdated
Show resolved
Hide resolved
...lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll
Outdated
Show resolved
Hide resolved
c360425
to
58f1371
Compare
4540fc2
to
d3d608f
Compare
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.
Pull Request Overview
This PR moves the CORS misconfiguration query from experimental to the Security suite and merges it into the existing CORS-related functionality. The main purpose is to promote the experimental cors-misconfiguration
query to become part of the default security suite.
- Moved the
cors-misconfiguration
query from experimental toSecurity/CWE-942/CorsPermissiveConfiguration
- Added the new
cors-misconfiguration
sink kind to the ModelValidation framework - Updated test files with additional test cases and inline expectations
Reviewed Changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
shared/mad/codeql/mad/ModelValidation.qll |
Added cors-misconfiguration to the list of valid sink kinds |
javascript/ql/test/query-tests/Security/CWE-942/express-test.js |
Added test cases and inline expectation annotations for CORS configuration scenarios |
javascript/ql/test/query-tests/Security/CWE-942/apollo-test.js |
Added inline expectation annotations for Apollo server CORS test cases |
javascript/ql/test/query-tests/Security/CWE-942/CorsPermissiveConfiguration.qlref |
New query reference file pointing to the Security suite query |
javascript/ql/test/query-tests/Security/CWE-942/CorsPermissiveConfiguration.expected |
Updated expected test results with new alert patterns |
javascript/ql/src/change-notes/2025-07-31-cors-move-out-of-experimental.md |
Change note documenting the query promotion |
javascript/ql/src/Security/CWE-942/CorsPermissiveConfiguration.ql |
New main query file for CORS permissive configuration detection |
javascript/ql/src/Security/CWE-942/CorsPermissiveConfiguration.qhelp |
Documentation for the CORS permissive configuration query |
javascript/ql/lib/semmle/javascript/security/CorsPermissiveConfigurationQuery.qll |
Core query logic and dataflow configuration |
javascript/ql/lib/semmle/javascript/security/CorsPermissiveConfigurationCustomizations.qll |
Sources, sinks, and sanitizers for CORS misconfiguration detection |
javascript/ql/lib/ext/cors.model.yml |
Model for CORS library sink configuration |
javascript/ql/lib/ext/apollo-server.model.yml |
Added CORS sink model for Apollo server and additional type mappings |
javascript/ql/integration-tests/query-suite/*.expected |
Updated query suite expectations to include the new Security query |
Moved cors-misconfiguration query outside experimental and merged it into existing
js/cors-misconfiguration-for-credentials
DCA: https://github.com/github/codeql-dca-main/blob/data/Napalys/PR-20146-0-javascript/reports/alert-comparison.md looks good to me, we got 2 new alerts due to
*
being used.