-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Java: Promote Sensitive Cookie without HttpOnly query from experimental #20572
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?
Java: Promote Sensitive Cookie without HttpOnly query from experimental #20572
Conversation
…how the implementation works, remove filter for test code (prefer to filter in code scanning ui than in query logic)
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 promotes the Java query java/sensitive-cookie-not-httponly
from experimental status to production, moving it from the experimental to the main security query suite. The query detects sensitive cookies that are created without the HttpOnly flag set, which can expose them to client-side script access.
Key Changes
- Removed the "experimental" tag from the query metadata
- Moved test files from the experimental directory to the main query-tests directory
- Updated query structure and replaced deprecated predicates with the current select statement format
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql |
Promoted query from experimental, removed deprecated predicate, updated class/module names |
java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.qhelp |
Updated documentation with clearer explanation and additional MDN reference |
java/ql/test/query-tests/security/CWE-1004/* |
New test files moved from experimental directory with updated annotations |
java/ql/test/experimental/query-tests/security/CWE-1004/* |
Removed experimental test files |
* (e.g. `session` or `token`) to a `ServletResponse.addHeader(...)` or `.addCookie(...)` | ||
* method that does not set the `httpOnly` flag. Subsidiary configurations | ||
* `MatchesHttpOnlyConfiguration` and `SetHttpOnlyInCookieConfiguration` are used to establish | ||
* `MatchesHttpOnlyToRawHeaderConfiguration` and `SetHttpOnlyInCookieConfiguration` are used to establish |
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 comment references MatchesHttpOnlyToRawHeaderConfiguration
and SetHttpOnlyInCookieConfiguration
, but the actual module names in the code are MatchesHttpOnlyToRawHeaderConfig
and SetHttpOnlyOrRemovesCookieToAddCookieConfig
. The documentation should match the actual implementation.
* `MatchesHttpOnlyToRawHeaderConfiguration` and `SetHttpOnlyInCookieConfiguration` are used to establish | |
* `MatchesHttpOnlyToRawHeaderConfig` and `SetHttpOnlyOrRemovesCookieToAddCookieConfig` are used to establish |
Copilot uses AI. Check for mistakes.
/** | ||
* A taint configuration tracking flow from a sensitive cookie without the `HttpOnly` flag | ||
* set to its HTTP response. | ||
* Tracks string literals containing sensitive names (`SensitiveNameExpr`), to an `addCookie` call (as a `Cookie` object) |
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 comment references SensitiveNameExpr
but the actual class name is SensitiveCookieNameExpr
. The documentation should use the correct class name.
* Tracks string literals containing sensitive names (`SensitiveNameExpr`), to an `addCookie` call (as a `Cookie` object) | |
* Tracks string literals containing sensitive names (`SensitiveCookieNameExpr`), to an `addCookie` call (as a `Cookie` object) |
Copilot uses AI. Check for mistakes.
* A taint configuration tracking flow from a sensitive cookie without the `HttpOnly` flag | ||
* set to its HTTP response. | ||
* Tracks string literals containing sensitive names (`SensitiveNameExpr`), to an `addCookie` call (as a `Cookie` object) | ||
* or an `addHeader` call (as a string) (`CookieResponseWithoutHttpOnly`). |
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 comment references CookieResponseWithoutHttpOnly
but the actual class name is CookieResponseWithoutHttpOnlySink
. The documentation should use the correct class name.
* or an `addHeader` call (as a string) (`CookieResponseWithoutHttpOnly`). | |
* or an `addHeader` call (as a string) (`CookieResponseWithoutHttpOnlySink`). |
Copilot uses AI. Check for mistakes.
QHelp previews: java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.qhelpSensitive cookies without the HttpOnly response header setCookies without the RecommendationUse the ExampleThe following example shows two ways of generating sensitive cookies. In the 'BAD' cases, the class SensitiveCookieNotHttpOnly {
// GOOD - Create a sensitive cookie with the `HttpOnly` flag set.
public void addCookie(String jwt_token, HttpServletRequest request, HttpServletResponse response) {
Cookie jwtCookie =new Cookie("jwt_token", jwt_token);
jwtCookie.setPath("/");
jwtCookie.setMaxAge(3600*24*7);
jwtCookie.setHttpOnly(true);
response.addCookie(jwtCookie);
}
// BAD - Create a sensitive cookie without the `HttpOnly` flag set.
public void addCookie2(String jwt_token, String userId, HttpServletRequest request, HttpServletResponse response) {
Cookie jwtCookie =new Cookie("jwt_token", jwt_token);
jwtCookie.setPath("/");
jwtCookie.setMaxAge(3600*24*7);
response.addCookie(jwtCookie);
}
// GOOD - Set a sensitive cookie header with the `HttpOnly` flag set.
public void addCookie3(String authId, HttpServletRequest request, HttpServletResponse response) {
response.addHeader("Set-Cookie", "token=" +authId + ";HttpOnly;Secure");
}
// BAD - Set a sensitive cookie header without the `HttpOnly` flag set.
public void addCookie4(String authId, HttpServletRequest request, HttpServletResponse response) {
response.addHeader("Set-Cookie", "token=" +authId + ";Secure");
}
// GOOD - Set a sensitive cookie header using the class `javax.ws.rs.core.Cookie` with the `HttpOnly` flag set through string concatenation.
public void addCookie5(String accessKey, HttpServletRequest request, HttpServletResponse response) {
response.setHeader("Set-Cookie", new NewCookie("session-access-key", accessKey, "/", null, null, 0, true) + ";HttpOnly");
}
// BAD - Set a sensitive cookie header using the class `javax.ws.rs.core.Cookie` without the `HttpOnly` flag set.
public void addCookie6(String accessKey, HttpServletRequest request, HttpServletResponse response) {
response.setHeader("Set-Cookie", new NewCookie("session-access-key", accessKey, "/", null, null, 0, true).toString());
}
// GOOD - Set a sensitive cookie header using the class `javax.ws.rs.core.Cookie` with the `HttpOnly` flag set through the constructor.
public void addCookie7(String accessKey, HttpServletRequest request, HttpServletResponse response) {
NewCookie accessKeyCookie = new NewCookie("session-access-key", accessKey, "/", null, null, 0, true, true);
response.setHeader("Set-Cookie", accessKeyCookie.toString());
}
} References
|
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.
And the suite inclusion test expectations need to be updated.
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.
It's a little bit weird that for the last way of making a cookie we only have the good example and not the bad one. I mean the same thing but with false
as the last argument. Is there a reason not to include that? I guess just that you wouldn't use that constructor, you'd use the one with one less argument. I guess there's no need to change it.
Promotes
java/sensitive-cookie-not-httponly
query from experimental.