-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Conversation
…ibute Co-authored-by: geoffw0 <40627776+geoffw0@users.noreply.github.com>
Co-authored-by: geoffw0 <40627776+geoffw0@users.noreply.github.com>
|
Looks good at a glance. I will have a closer look tomorrow. |
|
I've pushed some updates to the tests. The |
…ecure(true), to prevent excessive flow paths.
|
Added new models, fixed a path duplication issue. Ready for review. Note that the PR title doesn't represent this new work - we are now improving models (and logic) as well as tests. |
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 adds support for detecting insecure cookies in three additional Rust web frameworks: actix-web, poem, and http-types. The changes extend the existing CWE-614 query to handle cookie creation and configuration patterns in these frameworks.
Key Changes:
- Extended test coverage to include
actix-web,poem, andhttp-typescookie handling - Modified the query's barrier logic to use
isBarrierIninstead ofisBarrier - Added new dependencies to the test suite for the three frameworks
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/ql/test/query-tests/security/CWE-614/options.yml | Added dependencies for actix-web, poem, and http-types |
| rust/ql/test/query-tests/security/CWE-614/main.rs | Added test functions for the three new frameworks |
| rust/ql/test/query-tests/security/CWE-614/InsecureCookie.expected | Updated expected query results with new test cases |
| rust/ql/test/query-tests/security/CWE-614/CookieSet.expected | Updated expected cookie set tracking results |
| rust/ql/test/query-tests/security/CWE-614/Cargo.lock | Added transitive dependencies for the new crates |
| rust/ql/src/queries/security/CWE-614/InsecureCookie.ql | Changed barrier from isBarrier to isBarrierIn |
| rust/ql/lib/change-notes/2025-11-05-poem.md | Added change note for poem support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
DCA LGTM. This is ready for approval. |
paldepind
left a comment
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.
Looks good! My only comment is a question.
| cookieSetNode(node, "secure", true) | ||
| predicate isBarrierIn(DataFlow::Node node) { | ||
| // setting the 'secure' attribute | ||
| cookieSetNode(node, "secure", _) |
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.
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?
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.
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();
Expands CWE-614 test coverage to include three additional Rust web frameworks that handle HTTP cookies.
Changes
Added dependencies to
options.yml:actix-web = { version = "4", features = ["cookies"] }poem = { version = "3", features = ["cookie"] }http-types = { version = "2", features = ["cookies"] }Added test functions in
main.rs:test_actix_web()- Usesactix_web::cookie::Cookie::new()+set_secure()test_poem()- Usespoem::web::cookie::Cookie::new_with_str()+set_secure()test_http_types()- Useshttp_types::Cookie::new()+set_secure()Each test validates three cases: secure explicitly false, secure explicitly true, and secure left as default.
Updated test expectations:
Regenerated
CookieSet.expected,InsecureCookie.expected, andCargo.lockviacodeql test run . --learn.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
docs.rscurl -s REDACTED(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.