JS: Move cors-misconfiguration query from experimental to Security #20146

Napalys · 2025-07-31T09:01:40Z

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.

github-actions · 2025-07-31T09:02:32Z

QHelp previews:

Copilot

Pull Request Overview

This PR moves the CORS misconfiguration query from experimental to the main Security suite, making it part of the default security analysis.

Relocates the CORS permissive configuration query and supporting files from experimental/Security/CWE-942/ to Security/CWE-942/
Updates library imports to use the standard location and removes deprecated functionality
Adds Apollo Server modeling through external model files instead of custom QL code

Reviewed Changes

Copilot reviewed 16 out of 20 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`javascript/ql/src/Security/CWE-942/CorsPermissiveConfiguration.ql`	Updates query imports and improves naming from "overly CORS configuration" to "Permissive CORS configuration"
`javascript/ql/src/Security/CWE-942/CorsPermissiveConfiguration.qhelp`	Adds comprehensive help documentation with improved formatting and clearer explanations
`javascript/ql/lib/semmle/javascript/security/CorsPermissiveConfigurationQuery.qll`	Removes deprecated Configuration class and flow label classes
`javascript/ql/lib/semmle/javascript/security/CorsPermissiveConfigurationCustomizations.qll`	Updates imports to use standard Cors framework and replaces custom Apollo modeling with model-based approach
`javascript/ql/lib/ext/apollo-server.model.yml`	Adds Apollo Server type models and sink models to replace custom QL implementations
`javascript/ql/test/query-tests/Security/CWE-942/CorsPermissiveConfiguration.qlref`	Creates new test reference pointing to the moved query location
Multiple test expectation files	Updates expected query suite contents to include the new security query

Comments suppressed due to low confidence (1)

javascript/ql/src/Security/CWE-942/CorsPermissiveConfiguration.ql:2

The query name should be 'CORS misconfiguration' to match the title mentioned in the change notes and maintain consistency with existing naming conventions.

 * @name Permissive CORS configuration

asgerf · 2025-08-13T11:10:39Z

The query is a near duplicate of the existing js/cors-misconfiguration-for-credentials query. It should be merged into one query, or renamed and described in a way that makes it clear why there are two queries.

…rCredentials

asgerf

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.

asgerf · 2025-08-21T09:17:11Z

...lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll

+
+  /** An overly permissive value for `origin` (Apollo) */
+  class TrueNullValue extends Source {
+    TrueNullValue() { this.mayHaveBooleanValue(true) or this.asExpr() instanceof NullLiteral }


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.

asgerf · 2025-08-21T09:19:26Z

...lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll

@@ -75,4 +76,58 @@ module CorsMisconfigurationForCredentials {
      this.asExpr().mayHaveStringValue("null")
    }
  }
+
+  /** An overly permissive value for `origin` (Apollo) */


Suggested change

/** An overly permissive value for `origin` (Apollo) */

/** An overly permissive value for `origin` */

Not specific to Apollo in any way.

asgerf · 2025-08-21T09:19:50Z

...lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll

+    TrueNullValue() { this.mayHaveBooleanValue(true) or this.asExpr() instanceof NullLiteral }
+  }
+
+  /** An overly permissive value for `origin` (Express) */


Suggested change

/** An overly permissive value for `origin` (Express) */

/** An overly permissive value for `origin`. */

The * value is not specific to Express in any way. The CORS-related headers are checked by the browser.

Maybe just merge all these classes.

asgerf · 2025-08-21T09:28:15Z

...lib/semmle/javascript/security/dataflow/CorsMisconfigurationForCredentialsCustomizations.qll

+  /**
+   * The value of cors origin when initializing the application.
+   */
+  class ExpressCors extends Sink, DataFlow::ValueNode {
+    ExpressCors() {
+      exists(CorsConfiguration config | this = config.getCorsConfiguration().getOrigin())
+    }
+
+    override Http::HeaderDefinition getCredentialsHeader() { none() }
+  }
+
+  /**
+   * An express route setup configured with the `cors` package.
+   */
+  class CorsConfiguration extends DataFlow::MethodCallNode {
+    Cors::Cors corsConfig;
+
+    CorsConfiguration() {
+      exists(Express::RouteSetup setup | this = setup |
+        if setup.isUseCall()
+        then corsConfig = setup.getArgument(0)
+        else corsConfig = setup.getArgument(any(int i | i > 0))
+      )
+    }
+
+    /** Gets the expression that configures `cors` on this route setup. */
+    Cors::Cors getCorsConfiguration() { result = corsConfig }
+  }


These classes taken together don't end up doing much useful work.

We are effectively treating cors;Argument[0].Member[origin] as a sink (which is fine), but these classes ensure that we only actually use the sink in these two specific situations:

The cors middleware is used in an express app (why not other frameworks?)

It is set up with the first argument to a use call, or a non-first argument to another kind of route setup. I don't understand the rationale behind this requirement.

I would just replace the sink with API::moduleImport("cors").getArgument(0).getMember("origin").asSink() and use that as the sink (or add a sink kind for it and use MaD). And then remove Cors.qll as I think it's not used anywhere else. Too many wrappers that just end up adding unhelpful restrictions.

Napalys added 5 commits July 30, 2025 10:22

Move CORS misconfiguration query from experimental to Security

358617f

Updated suite expectations

92daa7d

Added inline test expectations for cors permissive config

95743d7

Added missing doc strings

84ffbbe

Moved apollo modeling to MaD

fd4233e

github-actions bot added JS documentation labels Jul 31, 2025

Napalys added 3 commits July 31, 2025 11:08

Removed deprecations from cors as it was moved out experimental

2baca58

Updated qhelp for cors permissive configuration

791a7e2

Added change note

021aa13

Napalys marked this pull request as ready for review July 31, 2025 11:15

Napalys requested a review from a team as a code owner July 31, 2025 11:15

Copilot AI review requested due to automatic review settings July 31, 2025 11:15

Copilot AI reviewed Jul 31, 2025

View reviewed changes

Napalys added 4 commits August 19, 2025 09:32

Merged test cases from CorsPermissiveConfig to CorsMisconfigurationFo…

3a4a57f

…rCredentials

Merged CorsPermissiveConfig to CorsMisconfigurationForCredentials

d758433

Removed now redundant js/cors-misconfiguration

c0f78b2

Updated change note to reflect changes

1c33a0a

Napalys force-pushed the js/move-cors-query-from-experimental branch from 227071b to 1c33a0a Compare August 19, 2025 08:06

asgerf reviewed Aug 21, 2025

View reviewed changes

Replace complex wrapper classes with MaD

83cb788

Napalys force-pushed the js/move-cors-query-from-experimental branch from e043692 to 83cb788 Compare August 21, 2025 11:12

Napalys added 2 commits August 22, 2025 07:33

Merge CORS source classes

6e2432e

Added cors-misconfiguration as valid sink kind

58f1371

Napalys force-pushed the js/move-cors-query-from-experimental branch from c360425 to 58f1371 Compare August 22, 2025 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JS: Move cors-misconfiguration query from experimental to Security #20146

JS: Move cors-misconfiguration query from experimental to Security #20146

Napalys commented Jul 31, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jul 31, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

asgerf commented Aug 13, 2025

Uh oh!

asgerf left a comment

Uh oh!

asgerf Aug 21, 2025

Uh oh!

asgerf Aug 21, 2025

Uh oh!

asgerf Aug 21, 2025

Uh oh!

asgerf Aug 21, 2025

Uh oh!

Uh oh!

	/** An overly permissive value for `origin` (Apollo) */
	/** An overly permissive value for `origin` */

JS: Move cors-misconfiguration query from experimental to Security #20146

Are you sure you want to change the base?

JS: Move cors-misconfiguration query from experimental to Security #20146

Conversation

Napalys commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

asgerf commented Aug 13, 2025

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

asgerf Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

asgerf Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

asgerf Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

asgerf Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Napalys commented Jul 31, 2025 •

edited

Loading

github-actions bot commented Jul 31, 2025 •

edited

Loading