Skip to content

Commit 9ae4c14

Browse files
paldepindpaldepind
committed
Rust: Address PR feedback
1 parent 411d1fa commit 9ae4c14

File tree

10 files changed

+54
-38
lines changed

10 files changed

+54
-38
lines changed

‎rust/ql/lib/codeql/rust/security/XssExtensions.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ module Xss {
5454
private class HeuristicHtmlEncodingBarrier extends Barrier {
5555
HeuristicHtmlEncodingBarrier() {
5656
exists(Call fc |
57-
fc.getStaticTarget().(Function).getName().getText().regexpMatch(".*(escape|encode).*") and
57+
fc.getStaticTarget().getName().getText().regexpMatch(".*(escape|encode).*") and
5858
fc.getArgument(_) = this.asExpr()
5959
)
6060
}

‎rust/ql/src/change-notes/2025-11-24-xss-query.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
---
22
category: newQuery
33
---
4-
* Added a new query `rust/xss`, to detect XSS security vulnerabilities.
4+
* Added a new a query `rust/xss`, to detect cross-site scripting security vulnerabilities.

‎rust/ql/src/queries/security/CWE-079/XSS.qhelp‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,20 +10,22 @@ scripting vulnerability.</p>
1010
</overview>
1111

1212
<recommendation>
13-
<p>To guard against cross-site scripting, consider encoding/escaping the unstrusted
13+
<p>To guard against cross-site scripting, consider encoding/escaping the untrusted
1414
input before including it in the HTML.</p>
1515
</recommendation>
1616

1717
<example>
1818

19-
<p>The following example shows a simple web handler that writes a path of the
20-
URL parameter directly to an HTML response, leaving the website vulnerable to
21-
cross-site scripting:</p>
19+
<p>The following example shows a simple web handler that writes a URL path parameter
20+
directly to an HTML response, leaving the website vulnerable to cross-site
21+
scripting:</p>
2222

2323
<sample src="XSSBad.rs" />
2424

2525
<p>To fix this vulnerability, the user input should be HTML-encoded before being
26-
included in the response:</p>
26+
included in the response. In the following example <code>encode_text</code> from
27+
the <a href="https://docs.rs/html-escape/latest/html_escape/index.html">html_escape</a>
28+
crate is used:</p>
2729

2830
<sample src="XSSGood.rs" />
2931

@@ -36,7 +38,7 @@ included in the response:</p>
3638
(Cross Site Scripting) Prevention Cheat Sheet</a>.
3739
</li>
3840
<li>
39-
Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
41+
Wikipedia: <a href="https://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
4042
</li>
4143
<li>
4244
OWASP:

‎rust/ql/src/queries/security/CWE-079/XSSGood.rs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
use actix_web::{web, HttpResponse, Result};
2-
use askama::Template;
32

4-
// GOOD: Manual HTML encoding using an `html_escape` function
3+
// GOOD: Manual HTML encoding using an `html_escape::encode_text` function
54
async fn safe_handler_with_encoding(path: web::Path<String>) -> impl Responder {
65
let user_input = path.into_inner();
7-
let escaped_input = html_escape(&user_input);
8-
6+
let escaped_input = html_escape::encode_text(&user_input);
97
let html = format!(
108
r#"
119
<!DOCTYPE html>

‎rust/ql/src/queries/summary/Stats.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ private import codeql.rust.security.TaintedPathExtensions
3131
private import codeql.rust.security.UncontrolledAllocationSizeExtensions
3232
private import codeql.rust.security.UseOfHttpExtensions
3333
private import codeql.rust.security.WeakSensitiveDataHashingExtensions
34+
private import codeql.rust.security.XssExtensions
3435

3536
/**
3637
* Gets a count of the total number of lines of code in the database.

‎rust/ql/test/query-tests/security/CWE-079/actix/Cargo.lock‎

Lines changed: 16 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎rust/ql/test/query-tests/security/CWE-079/actix/main.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use actix_web::{
55
};
66

77
// The "bad" example from the qldoc
8-
#[get("/bad/{a}")] // $ Source
8+
#[get("/bad/{a}")] // $ Source=a
99
async fn vulnerable_handler(path: web::Path<String>) -> impl Responder {
1010
let user_input = path.into_inner();
1111

@@ -22,7 +22,7 @@ async fn vulnerable_handler(path: web::Path<String>) -> impl Responder {
2222
user_input
2323
);
2424

25-
Html::new(html) // $ Alert[rust/xss]
25+
Html::new(html) // $ Alert[rust/xss]=a
2626
}
2727

2828
fn html_escape(s: &str) -> String {
@@ -42,7 +42,7 @@ fn html_escape(s: &str) -> String {
4242
// The "good" example from the qldoc
4343
async fn safe_handler_with_encoding(path: web::Path<String>) -> impl Responder {
4444
let user_input = path.into_inner();
45-
let escaped_input = html_escape(&user_input);
45+
let escaped_input = html_escape::encode_text(&user_input);
4646

4747
let html = format!(
4848
r#"

‎rust/ql/test/query-tests/security/CWE-079/actix/options.yml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
qltest_use_nightly: true
22
qltest_dependencies:
3-
- actix-web = { version = "4.12.0" }
3+
- actix-web = { version = "4.12.0" }
4+
- html-escape = { version = "0.2.13" }

‎rust/ql/test/query-tests/security/CWE-079/warp/XSS.expected‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,27 @@
11
#select
2-
| main.rs:12:13:12:29 | ...::html | main.rs:9:10:9:12 | map | main.rs:12:13:12:29 | ...::html | Cross-site scripting vulnerability due to a $@. | main.rs:9:10:9:12 | map | user-provided value |
2+
| main.rs:10:13:10:29 | ...::html | main.rs:7:10:7:12 | map | main.rs:10:13:10:29 | ...::html | Cross-site scripting vulnerability due to a $@. | main.rs:7:10:7:12 | map | user-provided value |
33
edges
4-
| main.rs:9:10:9:12 | map | main.rs:9:15:9:26 | ...: String | provenance | Src:MaD:2 |
5-
| main.rs:9:15:9:26 | ...: String | main.rs:11:32:11:56 | MacroExpr | provenance | |
6-
| main.rs:11:17:11:20 | body | main.rs:12:31:12:34 | body | provenance | |
7-
| main.rs:11:32:11:56 | ...::format(...) | main.rs:11:32:11:56 | { ... } | provenance | |
8-
| main.rs:11:32:11:56 | ...::must_use(...) | main.rs:11:17:11:20 | body | provenance | |
9-
| main.rs:11:32:11:56 | MacroExpr | main.rs:11:32:11:56 | ...::format(...) | provenance | MaD:3 |
10-
| main.rs:11:32:11:56 | { ... } | main.rs:11:32:11:56 | ...::must_use(...) | provenance | MaD:4 |
11-
| main.rs:12:31:12:34 | body | main.rs:12:13:12:29 | ...::html | provenance | MaD:1 Sink:MaD:1 |
4+
| main.rs:7:10:7:12 | map | main.rs:7:15:7:26 | ...: String | provenance | Src:MaD:2 |
5+
| main.rs:7:15:7:26 | ...: String | main.rs:9:32:9:56 | MacroExpr | provenance | |
6+
| main.rs:9:17:9:20 | body | main.rs:10:31:10:34 | body | provenance | |
7+
| main.rs:9:32:9:56 | ...::format(...) | main.rs:9:32:9:56 | { ... } | provenance | |
8+
| main.rs:9:32:9:56 | ...::must_use(...) | main.rs:9:17:9:20 | body | provenance | |
9+
| main.rs:9:32:9:56 | MacroExpr | main.rs:9:32:9:56 | ...::format(...) | provenance | MaD:3 |
10+
| main.rs:9:32:9:56 | { ... } | main.rs:9:32:9:56 | ...::must_use(...) | provenance | MaD:4 |
11+
| main.rs:10:31:10:34 | body | main.rs:10:13:10:29 | ...::html | provenance | MaD:1 Sink:MaD:1 |
1212
models
1313
| 1 | Sink: warp::reply::html; Argument[0]; html-injection |
1414
| 2 | Source: <_ as warp::filter::Filter>::map; Argument[0].Parameter[0..7]; remote |
1515
| 3 | Summary: alloc::fmt::format; Argument[0]; ReturnValue; taint |
1616
| 4 | Summary: core::hint::must_use; Argument[0]; ReturnValue; value |
1717
nodes
18-
| main.rs:9:10:9:12 | map | semmle.label | map |
19-
| main.rs:9:15:9:26 | ...: String | semmle.label | ...: String |
20-
| main.rs:11:17:11:20 | body | semmle.label | body |
21-
| main.rs:11:32:11:56 | ...::format(...) | semmle.label | ...::format(...) |
22-
| main.rs:11:32:11:56 | ...::must_use(...) | semmle.label | ...::must_use(...) |
23-
| main.rs:11:32:11:56 | MacroExpr | semmle.label | MacroExpr |
24-
| main.rs:11:32:11:56 | { ... } | semmle.label | { ... } |
25-
| main.rs:12:13:12:29 | ...::html | semmle.label | ...::html |
26-
| main.rs:12:31:12:34 | body | semmle.label | body |
18+
| main.rs:7:10:7:12 | map | semmle.label | map |
19+
| main.rs:7:15:7:26 | ...: String | semmle.label | ...: String |
20+
| main.rs:9:17:9:20 | body | semmle.label | body |
21+
| main.rs:9:32:9:56 | ...::format(...) | semmle.label | ...::format(...) |
22+
| main.rs:9:32:9:56 | ...::must_use(...) | semmle.label | ...::must_use(...) |
23+
| main.rs:9:32:9:56 | MacroExpr | semmle.label | MacroExpr |
24+
| main.rs:9:32:9:56 | { ... } | semmle.label | { ... } |
25+
| main.rs:10:13:10:29 | ...::html | semmle.label | ...::html |
26+
| main.rs:10:31:10:34 | body | semmle.label | body |
2727
subpaths

‎rust/ql/test/query-tests/security/CWE-079/warp/main.rs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,13 @@
1-
//! Tests for XSS
2-
//!
31
use warp::Filter;
42

53
#[tokio::main]
64
pub async fn main() {
75
let hello = warp::path("greet")
86
.and(warp::path::param())
9-
.map(|name: String| { // $ Source
7+
.map(|name: String| { // $ Source=name
108
// Vulnerable to XSS because it directly includes user input in the response
119
let body = format!("<h1>Hello, {name}!</h1>");
12-
warp::reply::html(body) // $ Alert[rust/xss]
10+
warp::reply::html(body) // $ Alert[rust/xss]=name
1311
});
1412

1513
// Start the web server on port 3000

0 commit comments

Comments
 (0)