Skip to content

Add test coverage for actix-web, poem, and http-types cookie secure attribute #20749

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Nov 11, 2025
+3,336 −559
Merged

Add test coverage for actix-web, poem, and http-types cookie secure attribute #20749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2497d8c
Initial plan
Copilot Nov 3, 2025
8f02ab1
Add test cases for actix-web, poem, and http-types cookie secure attr…
Copilot Nov 3, 2025
ee3d57e
Update test cases with correct APIs and run CodeQL test
Copilot Nov 3, 2025
7fe4877
Rust: Update test annotations.
geoffw0 Nov 4, 2025
55cf375
Rust: Add the cookies to jars, indicating that they're ready for use.
geoffw0 Nov 4, 2025
e5933d0
Rust: Add test cases with cookie builders.
geoffw0 Nov 4, 2025
21274d3
Rust: Add tests of poem CookieConfig.
geoffw0 Nov 4, 2025
7383e4f
Rust: Test for another edge cases supported by two of the libraries.
geoffw0 Nov 4, 2025
5fed5a2
Rust: It turns out Poem defaults 'secure' to true.
geoffw0 Nov 4, 2025
ff06181
Rust: We actually want barriers on set_secure(false) as well as set_s…
geoffw0 Nov 4, 2025
99a3692
Rust: Model poem cookie methods.
geoffw0 Nov 5, 2025
ad24b74
Rust: Fix for Poem cookies defaulting secure.
geoffw0 Nov 5, 2025
6e35cb9
Rust: Change note.
geoffw0 Nov 5, 2025
e780187
Rust: Add the model file (missed on previous commits).
geoffw0 Nov 5, 2025
1e7acc5
Merge branch 'main' into copilot/add-secure-cookie-test-cases
geoffw0 Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions rust/ql/lib/change-notes/2025-11-05-poem.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Added models for cookie methods in the `poem` crate.
22 changes: 22 additions & 0 deletions rust/ql/lib/codeql/rust/frameworks/poem.model.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
extensions:
- addsTo:
pack: codeql/rust-all
extensible: sinkModel
data:
- ["<poem::web::cookie::CookieJar>::add", "Argument[0]", "cookie-use", "manual"]
- ["<poem::web::cookie::SignedCookieJar>::add", "Argument[0]", "cookie-use", "manual"]
- ["<poem::web::cookie::PrivateCookieJar>::add", "Argument[0]", "cookie-use", "manual"]
- ["<poem::session::server_session::ServerSession>::new", "Argument[0]", "cookie-use", "manual"]
- addsTo:
pack: codeql/rust-all
extensible: summaryModel
data:
- ["<poem::web::cookie::Cookie>::set_secure", "Argument[self].OptionalBarrier[cookie-secure-arg0]", "Argument[self]", "taint", "manual"]
- ["<poem::session::cookie_config::CookieConfig>::secure", "Argument[self].OptionalBarrier[cookie-secure-arg0]", "ReturnValue", "taint", "manual"]
- ["<poem::session::cookie_config::CookieConfig>::partitioned", "Argument[self].OptionalBarrier[cookie-partitioned-arg0]", "ReturnValue", "taint", "manual"]
- ["<poem::session::cookie_config::CookieConfig>::name", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["<poem::session::cookie_config::CookieConfig>::path", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["<poem::session::cookie_config::CookieConfig>::domain", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["<poem::session::cookie_config::CookieConfig>::http_only", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["<poem::session::cookie_config::CookieConfig>::same_site", "Argument[self]", "ReturnValue", "taint", "manual"]
- ["<poem::session::cookie_config::CookieConfig>::max_age", "Argument[self]", "ReturnValue", "taint", "manual"]
6 changes: 3 additions & 3 deletions rust/ql/src/queries/security/CWE-614/InsecureCookie.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ module InsecureCookieConfig implements DataFlow::ConfigSig {
node instanceof Sink
}

predicate isBarrier(DataFlow::Node node) {
// setting the 'secure' attribute to true
cookieSetNode(node, "secure", true)
predicate isBarrierIn(DataFlow::Node node) {
// setting the 'secure' attribute
cookieSetNode(node, "secure", _)
Copy link
Contributor

@paldepind paldepind Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change because setting a cookie's security to false is a sink, and hence we want to make false a barrier as well, as otherwise two sources would flow to a sink?

What is the isBarrier -> isBarrierIn change for?

Copy link
Contributor

@geoffw0 geoffw0 Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's about preventing excessive flows in a case such as this:

set_secure(false); // source
set_secure(false); // source
sink();

Making line 2 a barrier prevents flow from line 1 to 3, leaving us with just the shorter path from line 2 to 3. It's a fairly common pattern to make sources in-barriers, e.g. you'll see this in all of the rust/cleartext-* queries.

The reason I've changed it to an in barrier is simple - if I didn't, then flow from every source would be blocked immediately by that source itself being a barrier. As an in-barrier, the situation becomes something like this:

|barrier| set_secure(false);
|barrier| set_secure(false); -> flow ->
sink();

or
node instanceof Barrier
}
Expand Down
Loading