Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
64 changes: 56 additions & 8 deletions crates/monty/src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1249,10 +1249,13 @@ impl<'a> Parser<'a> {
fn parse_identifier(&mut self, ast: AstExpr) -> Result<Identifier, ParseError> {
match ast {
AstExpr::Name(ast::ExprName { id, range, .. }) => Ok(self.identifier(&id, range)),
other => Err(ParseError::syntax(
format!("Expected name, got {other:?}"),
self.convert_range(other.range()),
)),
other => {
let kind = describe_expr_kind(&other);
Err(ParseError::syntax(
format!("Expected name, got {kind}"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
let kind = describe_expr_kind(&other);
Err(ParseError::syntax(
format!("Expected name, got {kind}"),
Err(ParseError::syntax(
format!("Expected name, got {}", describe_expr_kind(&other);),

self.convert_range(other.range()),
))
}
}
}

Expand Down Expand Up @@ -1350,10 +1353,13 @@ impl<'a> Parser<'a> {
}
Ok(UnpackTarget::Tuple { targets, position })
}
other => Err(ParseError::syntax(
format!("invalid unpacking target: {other:?}"),
self.convert_range(other.range()),
)),
other => {
let kind = describe_expr_kind(&other);
Err(ParseError::syntax(
format!("invalid unpacking target: {kind}"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
let kind = describe_expr_kind(&other);
Err(ParseError::syntax(
format!("invalid unpacking target: {kind}"),
Err(ParseError::syntax(
format!("invalid unpacking target: {}", describe_expr_kind(&other)),

self.convert_range(other.range()),
))
}
}
}

Expand Down Expand Up @@ -1611,6 +1617,48 @@ fn convert_conversion_flag(flag: RuffConversionFlag) -> ConversionFlag {
}
}

/// Short human-readable name for an `AstExpr` variant, for use in
/// user-facing parse errors. Avoids the Rust `Debug` formatting of the
/// node, which would leak internal field names, ranges, and struct
/// layout of `ruff_python_ast` into the error message.
fn describe_expr_kind(expr: &AstExpr) -> &'static str {
match expr {
AstExpr::Name(_) => "name",
AstExpr::Starred(_) => "starred expression",
AstExpr::Attribute(_) => "attribute",
AstExpr::Subscript(_) => "subscript",
AstExpr::Call(_) => "function call",
AstExpr::Tuple(_) => "tuple",
AstExpr::List(_) => "list",
AstExpr::Set(_) => "set",
AstExpr::Dict(_) => "dict",
AstExpr::NumberLiteral(_) => "number literal",
AstExpr::StringLiteral(_) => "string literal",
AstExpr::BytesLiteral(_) => "bytes literal",
AstExpr::BooleanLiteral(_) => "boolean literal",
AstExpr::NoneLiteral(_) => "None",
AstExpr::EllipsisLiteral(_) => "...",
AstExpr::FString(_) => "f-string",
AstExpr::TString(_) => "t-string",
AstExpr::Lambda(_) => "lambda",
AstExpr::If(_) => "conditional expression",
AstExpr::BoolOp(_) => "boolean expression",
AstExpr::BinOp(_) => "binary expression",
AstExpr::UnaryOp(_) => "unary expression",
AstExpr::Compare(_) => "comparison",
AstExpr::Named(_) => "named expression",
AstExpr::Yield(_) => "yield expression",
AstExpr::YieldFrom(_) => "yield from expression",
AstExpr::Await(_) => "await expression",
AstExpr::ListComp(_) => "list comprehension",
AstExpr::SetComp(_) => "set comprehension",
AstExpr::DictComp(_) => "dict comprehension",
AstExpr::Generator(_) => "generator expression",
AstExpr::Slice(_) => "slice",
AstExpr::IpyEscapeCommand(_) => "IPython escape command",
}
}

/// Source code location for a parsed node, stored as raw byte offsets.
///
/// `CodeRange` is written by the parser for every AST node and must therefore
Expand Down
79 changes: 79 additions & 0 deletions crates/monty/tests/parse_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -450,3 +450,82 @@ fn long_source_line_does_not_overflow_column() {
let result = run.run_no_limits(vec![]);
assert!(result.is_ok(), "long line should run: {result:?}");
}

// === Parse error messages must not leak ruff_python_ast Debug formatting ===
//
// These guard the previously observed info-disclosure where compile-time
// parse errors returned the Rust `Debug` formatting of the offending AST
// node verbatim in the HTTP response body (`ExprStarred { node_index:
// NodeIndex(None), range: ..., ... }`). The messages should now be short
// human prose that does not expose ruff's internal struct layout.

/// Asserts an error message is present, contains the expected prose,
/// and does not leak any telltale `ruff_python_ast` Debug tokens.
fn assert_no_debug_leak(msg: Option<&str>, expected_substr: &str) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would remove this and instead use insta to check the full error text.

let msg = msg.expect("error should have a message");
assert!(
msg.contains(expected_substr),
"expected message to contain {expected_substr:?}, got: {msg:?}"
);
for tok in [
"ExprStarred",
"ExprName",
"ExprAttribute",
"ExprSubscript",
"ExprNumberLiteral",
"NodeIndex",
"node_index",
"ctx: Store",
"ctx: Load",
] {
assert!(
!msg.contains(tok),
"parse error must not leak ruff AST Debug token {tok:?}, got: {msg:?}"
);
}
}

#[test]
fn starred_assignment_target_name_has_clean_message() {
// `*a = [1, 2]`: Ruff parses the LHS as a bare starred target, which
// Monty rejects at `parse_identifier`. The message must not embed the
// Debug format of the `ExprStarred` node.
let result = MontyRun::new("*a = [1, 2]".to_owned(), "test.py", vec![]);
let exc = result.expect_err("expected parse error");
assert_eq!(exc.exc_type(), ExcType::SyntaxError);
assert_no_debug_leak(exc.message(), "starred expression");
}

#[test]
fn starred_attribute_target_has_clean_message() {
// `*x.y = 1`: starred target wrapping an attribute. Same rejection
// path, different inner node shape. Message must not leak
// `ExprAttribute` / `Identifier` Debug.
let result = MontyRun::new("*x.y = 1".to_owned(), "test.py", vec![]);
let exc = result.expect_err("expected parse error");
assert_eq!(exc.exc_type(), ExcType::SyntaxError);
assert_no_debug_leak(exc.message(), "starred expression");
}

#[test]
fn starred_subscript_target_has_clean_message() {
// `*x[0] = 1`: starred target wrapping a subscript. Must not leak
// `ExprSubscript` / `ExprNumberLiteral` Debug.
let result = MontyRun::new("*x[0] = 1".to_owned(), "test.py", vec![]);
let exc = result.expect_err("expected parse error");
assert_eq!(exc.exc_type(), ExcType::SyntaxError);
assert_no_debug_leak(exc.message(), "starred expression");
}

#[test]
fn for_loop_attribute_target_has_clean_message() {
// `for x.y in [1]: pass`: attribute as a for-loop target. CPython
// accepts this; Monty currently rejects at `parse_unpack_target_impl`
// with a generic "invalid unpacking target" error. That rejection of
// valid Python is a separate issue; this test locks only that the
// error message does not leak `ExprAttribute` Debug.
let result = MontyRun::new("for x.y in [1]: pass".to_owned(), "test.py", vec![]);
let exc = result.expect_err("expected parse error");
assert_eq!(exc.exc_type(), ExcType::SyntaxError);
assert_no_debug_leak(exc.message(), "attribute");
}
Loading