Go: Promote non-httponly cookie query, and add insecure cookie query #20762

joefarebrother · 2025-11-05T11:01:45Z

Promotes go/cookie-httponly-not-set from experimental, and adds go/cookie-secure-not-set query.

go/ql/lib/semmle/go/concepts/HTTP.qll

go/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.ql

go/ql/src/Security/CWE-614/CookieWithoutSecure.ql

github-actions · 2025-11-10T09:32:18Z

QHelp previews:

go/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.qhelp

Cookie 'HttpOnly' attribute is not set to true

Cookies without the HttpOnly flag set are accessible to client-side scripts such as JavaScript running in the same origin. In case of a Cross-Site Scripting (XSS) vulnerability, the cookie can be stolen by a malicious script. If a sensitive cookie does not need to be accessed directly by client-side JS, the HttpOnly flag should be set.

Recommendation

Set the HttpOnly flag to true for authentication cookies to ensure they are not accessible to client-side scripts.

Example

In the following example, in the case marked BAD, the HttpOnly flag is not set, so the default value of false is used. In the case marked GOOD, the HttpOnly flag is set to true.

package main

import (
	"net/http"
)

func handlerBad(w http.ResponseWriter, r *http.Request) {
	c := http.Cookie{
		Name:  "session",
		Value: "secret",
	}
	http.SetCookie(w, &c) // BAD: The HttpOnly flag is set to false by default.
}

func handlerGood(w http.ResponseWriter, r *http.Request) {
	c := http.Cookie{
		Name:     "session",
		Value:    "secret",
		HttpOnly: true,
	}
	http.SetCookie(w, &c) // GOOD: The HttpOnly flag is set to true.
}

References

MDN: Set-Cookie Header.
PortSwigger: Cookie without HttpOnly flag set
Common Weakness Enumeration: CWE-1004.

go/ql/src/Security/CWE-614/CookieWithoutSecure.qhelp

Cookie 'Secure' attribute is not set to true

Cookies without the Secure flag set may be transmitted using HTTP instead of HTTPS. This leaves them vulnerable to being read by a third party attacker. If a sensitive cookie such as a session key is intercepted this way, it would allow the attacker to perform actions on a user's behalf.

Recommendation

Set the Secure flag to true to ensure cookies are only transmitted over secure HTTPS connections.

Example

In the following example, in the case marked BAD, the Secure flag is set to false by default. In the case marked GOOD, the Secure flag is set to true.

package main

import (
	"net/http"
)

func handlerBad(w http.ResponseWriter, r *http.Request) {
	c := http.Cookie{
		Name:  "session",
		Value: "secret",
	}
	http.SetCookie(w, &c) // BAD: The Secure flag is set to false by default.
}

func handlerGood(w http.ResponseWriter, r *http.Request) {
	c := http.Cookie{
		Name:   "session",
		Value:  "secret",
		Secure: true,
	}
	http.SetCookie(w, &c) // GOOD: The Secure flag is set to true.
}

References

MDN: Set-Cookie Header.
Detectify: Cookie lack Secure flag.
PortSwigger: TLS cookie without secure flag set.
Common Weakness Enumeration: CWE-614.

Copilot

Pull Request Overview

This PR promotes an experimental query for detecting cookies without the HttpOnly flag and introduces a new query for detecting cookies without the Secure flag. The changes include:

Migration of the CookieWithoutHttpOnly query from experimental to the main security query pack
Introduction of a new CookieWithoutSecure query
New shared library code (SecureCookies.qll) for modeling HTTP cookie security attributes
Updates to framework models for net/http and gin-gonic/gin

Reviewed Changes

Copilot reviewed 31 out of 42 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
go/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.ql	New query for detecting cookies without HttpOnly flag
go/ql/src/Security/CWE-614/CookieWithoutSecure.ql	New query for detecting cookies without Secure flag
go/ql/lib/semmle/go/security/SecureCookies.qll	Shared library for cookie security analysis
go/ql/lib/semmle/go/concepts/HTTP.qll	Added `CookieWrite` and `CookieOptions` concepts
go/ql/lib/semmle/go/frameworks/NetHttp.qll	Added cookie write models for net/http
go/ql/lib/semmle/go/frameworks/Gin.qll	New framework models for gin-gonic/gin
go/ql/src/change-notes/2025-11-10-inseucre-cookie.md	Change notes documenting the updates
go/ql/src/experimental/CWE-1004/*	Removed experimental query files
go/ql/test/query-tests/Security/CWE-614/*	Test files for CookieWithoutSecure
go/ql/test/query-tests/Security/CWE-1004/*	Test files for CookieWithoutHttpOnly

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

go/ql/src/change-notes/2025-11-10-insecure-cookie.md

go/ql/src/Security/CWE-614/CookieWithoutSecure.ql

go/ql/lib/semmle/go/concepts/HTTP.qll

owen-mc

I love the new concepts. I think they make the structure quite clean, and allow us to add other libraries easily in future.

I've only done a partial review so far. It will be easier to continue once these points have been addressed.

I note that you've removed modelling and tests for github.com/gorilla/sessions. Was that deliberate? It seems like a popular enough library, and it's only a bit of extra modelling and some tests that have already been written.

owen-mc · 2025-11-11T13:37:06Z

go/ql/lib/semmle/go/concepts/HTTP.qll

    predicate guardedBy(DataFlow::Node check) { super.guardedBy(check) }
  }
+
+  /** Provides a class for modeling HTTP response cookie writes. */


Suggested change

/** Provides a class for modeling HTTP response cookie writes. */

/** Provides a class for modeling new HTTP response cookie write APIs. */

owen-mc · 2025-11-11T13:37:53Z

go/ql/lib/semmle/go/concepts/HTTP.qll

+    DataFlow::Node getHttpOnly() { result = super.getHttpOnly() }
+  }
+
+  /** Provides a class for modeling the options of an HTTP cookie. */


Suggested change

/** Provides a class for modeling the options of an HTTP cookie. */

/** Provides a class for modeling new APIs for the options of an HTTP cookie. */

owen-mc · 2025-11-11T13:43:05Z

go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll

+    Write w;
+    Field f;


Rather than storing these as fields, they should be introduced in the characteristic predicate using exists, since they aren't referenced by any of the member predicates.

owen-mc · 2025-11-11T13:44:09Z

go/ql/lib/semmle/go/concepts/HTTP.qll

+    /** Gets the name of the cookie represented. */
+    DataFlow::Node getName() { result = super.getName() }
+
+    /** Gets the value of the cookie represented. */
+    DataFlow::Node getValue() { result = super.getValue() }
+
+    /** Gets the `Secure` attribute of the cookie represented. */
+    DataFlow::Node getSecure() { result = super.getSecure() }
+
+    /** Gets the `HttpOnly` attribute of the cookie represented. */
+    DataFlow::Node getHttpOnly() { result = super.getHttpOnly() }


Have I understood correctly that these won't always be defined? If so, add ", if any" to the end of the QLDocs for them. (Same in the Range class too.)

owen-mc · 2025-11-11T13:45:17Z

go/ql/lib/semmle/go/concepts/HTTP.qll

+  module CookieOptions {
+    /**
+     * An HTTP Cookie object.


The class name doesn't really match the QLDoc, and I think they're both wrong. It isn't a cookie object, or the options of a cookie. The only class extending it is NetHttp::CookieFieldWrite. I think it is a node which sets an option of a cookie, so maybe CookieOptionWrite?

owen-mc · 2025-11-11T13:46:55Z

go/ql/lib/semmle/go/security/SecureCookies.qll

+import semmle.go.dataflow.DataFlow
+
+/**
+ * Holds if the expression or its value has a sensitive name


Suggested change

* Holds if the expression or its value has a sensitive name

* Holds if the expression or its value has a sensitive name.

owen-mc · 2025-11-11T13:57:39Z

go/ql/lib/semmle/go/security/SecureCookies.qll

+/**
+ * Holds if the expression or its value has a sensitive name
+ */
+private predicate isSensitiveExpr(Expr expr, string val) {


The predicate name is confusing because in go/ql/lib/semmle/go/security/SensitiveActions.qll there is a class SensitiveExpr. A clearer name would be something like isSensitiveCookieName? But actually I think there is a better approach, which avoids having to come up with a clear name for it: make it an additional predicate in SensitiveCookieNameConfig with the signature isSource(Expr expr, string val) and then isSource(DataFlow::Node source) would just be isSource(source.asExpr(), _).

owen-mc · 2025-11-11T14:27:53Z

go/ql/lib/semmle/go/security/SecureCookies.qll

+private module SensitiveCookieNameConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { isSensitiveExpr(source.asExpr(), _) }
+
+  predicate isSink(DataFlow::Node sink) { exists(Http::CookieWrite cw | sink = cw.getName()) }


I think this would be cleaner with an additional predicate isSink(DataFlow::Node sink, Http::CookieWrite cw) { sink = cw.getName() } and then isSink(DataFlow::Node sink) { isSink(sink, _) }. Then you can use the additional predicate in isSensitiveCookie

owen-mc · 2025-11-11T14:33:09Z

go/ql/src/Security/CWE-614/CookieWithoutSecure.ql

+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @security-severity 5.0


I think this be lowered to 4.0, as was recently done for java.

owen-mc · 2025-11-11T14:36:31Z

go/ql/lib/semmle/go/security/SecureCookies.qll

It's confusing having predicates for two different queries in this one file (which is then only named after one of the predicates). Unless there is a good reason to keep them together, please put them in two separate files, named CookieWithoutSecure.qll and CookieWithoutHttpOnly.qll, so it's clear which query they relate to. I think there isn't any shared code - there probably was before you put things into concepts/HTTP.qll.

github-actions bot added the Go label Nov 5, 2025

github-advanced-security bot found potential problems Nov 5, 2025

View reviewed changes

github-actions bot added the documentation label Nov 9, 2025

joefarebrother force-pushed the go-insecure-cookie branch 2 times, most recently from d00f670 to 2805a60 Compare November 10, 2025 09:31

joefarebrother added 9 commits November 10, 2025 09:48

Implement cookie write concepts and httponly query

79f1818

Fixes, add secure query

5c1db90

Add modeling for gin

9ce5975

Add tests

533b633

Fixes and doc updates

c7a8712

Add qhelp & fix tests

87bf748

Remove experimental query

c4e9b10

Update integration tests

1c96aff

Add change note

999eff1

joefarebrother force-pushed the go-insecure-cookie branch from 6f26b83 to 999eff1 Compare November 10, 2025 09:50

joefarebrother added 2 commits November 10, 2025 10:18

Update integration test

4c18577

Update formatting

837aba8

joefarebrother marked this pull request as ready for review November 10, 2025 10:35

joefarebrother requested a review from a team as a code owner November 10, 2025 10:35

Copilot AI review requested due to automatic review settings November 10, 2025 10:35

Copilot AI reviewed Nov 10, 2025

View reviewed changes

Fix typos

a078a4e

owen-mc requested changes Nov 11, 2025

View reviewed changes

	/** Provides a class for modeling HTTP response cookie writes. */
	/** Provides a class for modeling new HTTP response cookie write APIs. */

	/** Provides a class for modeling the options of an HTTP cookie. */
	/** Provides a class for modeling new APIs for the options of an HTTP cookie. */

	* Holds if the expression or its value has a sensitive name
	* Holds if the expression or its value has a sensitive name.

Go: Promote non-httponly cookie query, and add insecure cookie query #20762

Are you sure you want to change the base?

Go: Promote non-httponly cookie query, and add insecure cookie query #20762

Conversation

joefarebrother commented Nov 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Cookie 'HttpOnly' attribute is not set to true

Recommendation

Example

References

Cookie 'Secure' attribute is not set to true

Recommendation

Example

References

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owen-mc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Nov 10, 2025 •

edited

Loading