From 73289ac9757c192c952ccc542ba2ee7922461b50 Mon Sep 17 00:00:00 2001 From: Jonathan Yoder Date: Tue, 22 Sep 2026 08:55:16 -0400 Subject: [PATCH] Decode Python string escapes in PEP 508 marker literals (#19401) Marker quoted strings now decode Python escapes (\n, \x41, A, \U0001F600, octal, etc.) in one pass, matching upstream's process_python_str/ast.literal_eval. Previously Token.Unquoted() returned the raw backslash sequence, so a requires_dist marker comparing against an escaped value compared against the wrong string. Validation is also tightened: truncated \u, truncated \U, and non-octal digits after a backslash (\8, \9) are now rejected, not silently accepted. Decoding happens once, in decodeQuotedStringContents (marker.go), at Unquoted()'s only production call site, so there is one source of truth for a literal's value; Unquoted() itself stays a raw, tokenizer-level accessor. This module's marker package is imported by PPM's src/pyresolve/target.go (marker.EnvironmentFromTarget) and evaluated during dependency resolution (PPM #20799, `get pypi --file-in --resolve-dependencies`), so this fixes a real marker-evaluation defect once a PPM release picks up a module version past the current v0.10.0 pin (PPM currently pins v0.9.0 via MVS, so this does not reach PPM immediately). Co-Authored-By: Claude Sonnet 5 --- internal/pep508/marker.go | 189 ++++++++++++++++++++++++------ internal/pep508/marker_test.go | 150 ++++++++++++++++++++++++ internal/pep508/tokenizer.go | 16 +-- internal/pep508/tokenizer_test.go | 10 ++ 4 files changed, 318 insertions(+), 47 deletions(-) diff --git a/internal/pep508/marker.go b/internal/pep508/marker.go index 21bc056..142c88e 100644 --- a/internal/pep508/marker.go +++ b/internal/pep508/marker.go @@ -4,6 +4,7 @@ package pep508 import ( "fmt" + "strconv" "strings" "github.com/posit-dev/go-python-packaging/extras" @@ -130,8 +131,8 @@ func (l Literal) String() string { return quoteLiteral(l.Value) } // // Note this is about what we emit, not what we accept: upstream runs the token // through ast.literal_eval, so Python string escapes ARE valid on input (see -// Token.Unquoted and validateQuotedStringContents). Do not use this comment as -// grounds for removing the escape validation. +// Token.Unquoted and decodeQuotedStringContents). Do not use this comment as +// grounds for removing the escape decoding/validation. func quoteLiteral(v string) string { if strings.Contains(v, `"`) { return "'" + v + "'" @@ -318,12 +319,8 @@ func parseMarkerVar(t *Tokenizer) (Operand, error) { } if t.check(QuotedString) { tok := t.read() - val := tok.Unquoted() - // Upstream calls ast.literal_eval on the token, which rejects - // malformed escape sequences. We validate only the two specific - // cases upstream asserts in tests: a trailing unpaired backslash - // (the closing quote is "escaped"), and a truncated \x escape. - if err := validateQuotedStringContents(val); err != nil { + val, err := decodeQuotedStringContents(tok.Unquoted()) + if err != nil { return nil, t.NewSyntaxErrorAt(err.Error(), tok.Pos, tok.Pos+len(tok.Text)) } return Literal{Value: val}, nil @@ -354,44 +351,158 @@ func parseMarkerOp(t *Tokenizer) (CompareOp, error) { return "", t.NewSyntaxError("Expected marker operator, one of <=, <, !=, ==, >=, >, ~=, ===, in, not in") } -// validateQuotedStringContents checks for the two specific malformed escape -// cases that upstream's ast.literal_eval rejects: a trailing unpaired -// backslash, and a truncated \x escape (not followed by two hex digits). -// Returning nil means the string is valid (or contains other escape sequences -// we do not validate). A non-nil error's message is "Invalid quoted string", -// matching upstream. -// It walks the string ONCE, left to right, consuming each escape sequence as a -// unit. A single pass is required, not two independent scans: an escaped -// backslash consumes BOTH of its bytes, so `\\x` is a complete pair followed by -// a literal "x" -- not a truncated \x escape. Scanning for `\x` separately from -// the trailing-backslash check has no way to know which backslashes were -// already consumed, and falsely rejects `"C:\\xyz"`, `"a\\x"`, and `"\\x"` -- -// all of which upstream accepts. -func validateQuotedStringContents(s string) error { - for i := 0; i < len(s); i++ { - if s[i] != '\\' { +// decodeQuotedStringContents decodes s - a QuotedString token's contents, +// already stripped of its surrounding quote characters by Token.Unquoted - +// as a Python string literal, mirroring upstream's process_python_str +// (_parser.py), which is essentially ast.literal_eval. Decoding and +// validation happen together: a malformed escape is a validation failure +// with the message "Invalid quoted string" (matching upstream), and every +// other backslash is replaced by the value it escapes. +// +// It walks s ONCE, left to right, consuming each escape sequence as a single +// unit and advancing its cursor past whichever it just consumed. A single +// pass is required, not a validate-then-decode split: an escaped backslash +// ("\\") consumes BOTH of its source bytes as one unit, so "C:\\x41" is a +// "\\" pair (decoding to one literal backslash) followed by the three +// literal characters "x41" -- not a truncated "\x" hex escape. A second, +// independent scan for "\x" has no way to know that the preceding backslash +// was already consumed as half of a "\\" pair, and would misdecode this case +// (this is the exact bug an earlier two-pass implementation had; see the +// "doubled_backslash_before_x" cases in TestDecodeQuotedStringContents_Accepts). +// +// Recognized escapes, matching Python's string-literal grammar +// (https://docs.python.org/3/reference/lexical_analysis.html#string-and-bytes-literals): +// \\, \', \", the single-character escapes \a \b \f \n \r \t \v, \xHH +// (exactly 2 hex digits), \uHHHH (exactly 4 hex digits), \UHHHHHHHH (exactly +// 8 hex digits, rejected if the value exceeds U+10FFFF), and \OOO octal +// escapes (1-3 octal digits, consumed greedily). \x/\u/\U/octal all decode to +// the Unicode code point with that numeric value (not a raw byte) - e.g. +// "\xFF" decodes to U+00FF, encoded as the two UTF-8 bytes C3 BF, matching +// CPython. Any other backslash - one at end-of-string, one followed by a +// digit outside 0-7, or a truncated hex/unicode escape - is a validation +// failure. +// +// Verified by hand against packaging 26.2's _parser.py +// (process_python_str/parse_marker_var) and _tokenizer.py (QUOTED_STRING), +// not against packaging's test suite directly (no repo access at +// implementation time). +// +// Two gaps, neither exercised by this issue's boundary table: \N{...} named +// escapes are rejected here (upstream's ast.literal_eval accepts them, but +// supporting them needs the Unicode name database, which is out of scope for +// a marker string); and a decoded lone surrogate (U+D800-U+DFFF) from \u/\U, +// which Python permits in a str but which Go's utf8/strings.Builder cannot +// represent - strings.Builder.WriteRune substitutes U+FFFD instead. +func decodeQuotedStringContents(s string) (string, error) { + var b strings.Builder + for i := 0; i < len(s); { + c := s[i] + if c != '\\' { + b.WriteByte(c) + i++ continue } - // A backslash at the very end is unpaired: the escape is unterminated. - // This is upstream's `"C:\"` case. if i+1 >= len(s) { - return &SyntaxError{Msg: "Invalid quoted string"} + // A backslash at the very end is unpaired: the escape is + // unterminated. This is upstream's `"C:\"` case. + return "", errInvalidQuotedString() } - if s[i+1] == 'x' { - // \x requires exactly two hex digits. This is upstream's `"\x"` - // (and `"\x4"`, `"\xZZ"`) case. - if i+3 >= len(s) || !isHexDigit(s[i+2]) || !isHexDigit(s[i+3]) { - return &SyntaxError{Msg: "Invalid quoted string"} + next := s[i+1] + switch { + case next == '\\' || next == '\'' || next == '"': + b.WriteByte(next) + i += 2 + case next == 'a': + b.WriteByte('\a') + i += 2 + case next == 'b': + b.WriteByte('\b') + i += 2 + case next == 'f': + b.WriteByte('\f') + i += 2 + case next == 'n': + b.WriteByte('\n') + i += 2 + case next == 'r': + b.WriteByte('\r') + i += 2 + case next == 't': + b.WriteByte('\t') + i += 2 + case next == 'v': + b.WriteByte('\v') + i += 2 + case next == 'x': + v, err := decodeFixedHexEscape(s, i+2, 2) + if err != nil { + return "", err } - i += 3 // consume \xNN - continue + b.WriteRune(rune(v)) + i += 4 + case next == 'u': + v, err := decodeFixedHexEscape(s, i+2, 4) + if err != nil { + return "", err + } + b.WriteRune(rune(v)) + i += 6 + case next == 'U': + v, err := decodeFixedHexEscape(s, i+2, 8) + if err != nil { + return "", err + } + if v > 0x10FFFF { + return "", errInvalidQuotedString() + } + b.WriteRune(rune(v)) + i += 10 + case next >= '0' && next <= '7': + v, n := decodeOctalEscape(s, i+1) + b.WriteRune(rune(v)) + i += 1 + n + default: + return "", errInvalidQuotedString() } - // Any other escape (including \\, \n, \t, \', \") consumes exactly two - // bytes. We deliberately do not validate \u/\U/octal -- see the package - // note on why full Python escape semantics are out of scope. - i++ } - return nil + return b.String(), nil +} + +// decodeFixedHexEscape parses exactly width hex digits at s[start:start+width] +// and returns their numeric value, for the \x (width 2), \u (width 4), and \U +// (width 8) escapes. An error means the escape is truncated: fewer than +// width hex digits are available before the string ends. +func decodeFixedHexEscape(s string, start, width int) (uint64, error) { + if start+width > len(s) { + return 0, errInvalidQuotedString() + } + for i := 0; i < width; i++ { + if !isHexDigit(s[start+i]) { + return 0, errInvalidQuotedString() + } + } + // The preceding loop already confirmed width hex digits, so this cannot + // fail to parse or overflow (width is at most 8, well within uint64). + v, _ := strconv.ParseUint(s[start:start+width], 16, 64) + return v, nil +} + +// decodeOctalEscape parses 1-3 octal digits starting at s[start] (which the +// caller has already confirmed is an octal digit), stopping at the first +// non-octal-digit byte or after 3 digits - Python's octal escape is greedy +// and variable-width, unlike \x/\u/\U's fixed width. It returns the escape's +// numeric value and how many digits (1-3) it consumed. +func decodeOctalEscape(s string, start int) (value uint64, n int) { + end := start + for end < len(s) && end < start+3 && s[end] >= '0' && s[end] <= '7' { + end++ + } + v, _ := strconv.ParseUint(s[start:end], 8, 64) + return v, end - start +} + +func errInvalidQuotedString() error { + return &SyntaxError{Msg: "Invalid quoted string"} } func isHexDigit(c byte) bool { diff --git a/internal/pep508/marker_test.go b/internal/pep508/marker_test.go index 18b0fa9..6c60f70 100644 --- a/internal/pep508/marker_test.go +++ b/internal/pep508/marker_test.go @@ -222,3 +222,153 @@ func TestLiteral_String_UsesSingleQuoteWhenValueContainsDoubleQuote(t *testing.T lit := Literal{Value: `has "quote"`} assert.Equal(t, `'has "quote"'`, lit.String()) } + +// --- Escape decoding (#19401) --- +// +// Boundary cases below were verified by hand against packaging 26.2's +// process_python_str (_parser.py) and QUOTED_STRING (_tokenizer.py) - via +// ast.literal_eval, which process_python_str essentially is - since this +// task was scoped without direct access to packaging's own test suite +// (tests/test_markers.py / test_tokenizer.py). No pinned SHA is cited for +// this specific escape-handling table for that reason; contrast +// grammar_conformance_test.go, whose requirement-parsing ports ARE pinned to +// a verified SHA. + +func TestDecodeQuotedStringContents_Accepts(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"doubled_backslash_then_U", `C:\\U`, `C:\U`}, + {"newline", `\n`, "\n"}, + {"tab", `\t`, "\t"}, + {"carriage_return", `\r`, "\r"}, + {"nul_octal", `\0`, "\x00"}, + {"escaped_single_quote", `\'`, `'`}, + {"escaped_double_quote", `\"`, `"`}, + {"alert", `\a`, "\a"}, + {"backspace", `\b`, "\b"}, + {"form_feed", `\f`, "\f"}, + {"vertical_tab", `\v`, "\v"}, + {"hex_escape", `\x41`, "A"}, + {"hex_escape_high_byte", `\xFF`, "\u00FF"}, + {"unicode_escape", `\u0041`, "A"}, + {"unicode_escape_wide", `\U0001F600`, "\U0001F600"}, + {"octal_two_digit", `\101`, "A"}, // 0o101 == 65 == 'A' + {"octal_three_digit_wide", `\401`, "\u0101"}, // 0o401 == 257 + {"octal_single_digit", `\7`, "\a"}, // 0o7 == 7 == BEL + {"octal_greedy_stops_at_non_octal", `\1a`, "\x01a"}, + // Regression guard: an escaped backslash ("\\") consumes BOTH its + // source bytes as one unit, so the "x41"/"x" that follows is + // literal characters, not a fresh \x escape. A validator that scans + // for "\x" independently of consumed "\\" pairs misreads this as a + // truncated \x escape and wrongly rejects it - the regression + // #18640's review caught; see decodeQuotedStringContents's doc + // comment in marker.go for why a single pass avoids it. + {"doubled_backslash_before_x_regression", `C:\\x41`, `C:\x41`}, + {"doubled_backslash_before_x_short", `\\x`, `\x`}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, err := decodeQuotedStringContents(c.in) + require.NoError(t, err, "input %q should decode", c.in) + assert.Equal(t, c.want, got, "input %q", c.in) + }) + } +} + +func TestDecodeQuotedStringContents_Rejects(t *testing.T) { + cases := []struct { + name string + in string + }{ + {"trailing_unpaired_backslash", `C:\`}, + {"truncated_hex_empty", `\x`}, + {"truncated_hex_one_digit", `\x4`}, + {"truncated_hex_bad_digit", `\xZZ`}, + {"truncated_unicode_empty", `\u`}, + {"truncated_unicode_short", `\u123`}, + {"truncated_unicode_wide_empty", `\U`}, + {"truncated_unicode_wide_short", `\U1234567`}, + {"unicode_wide_out_of_range", `\U00110000`}, + {"invalid_octal_digit_8", `\8`}, + {"invalid_octal_digit_9", `\9`}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + _, err := decodeQuotedStringContents(c.in) + require.Error(t, err, "input %q should be rejected", c.in) + assert.Contains(t, err.Error(), "Invalid quoted string") + }) + } +} + +// TestDecodeQuotedStringContents_RoundTripIsIdempotent checks that decoding +// is a pure, deterministic function: re-encoding a decoded value back into +// an escaped literal body and decoding it again reproduces the same value, +// and decoding the same input twice never differs. +func TestDecodeQuotedStringContents_RoundTripIsIdempotent(t *testing.T) { + cases := []string{ + `C:\\U`, + `\n\t\r`, + `\x41\u0041\U0001F600`, + `\101\7`, + `C:\\x41`, + `it's "fine"`, + `plain text, no escapes`, + } + for _, in := range cases { + t.Run(in, func(t *testing.T) { + decoded1, err := decodeQuotedStringContents(in) + require.NoError(t, err) + + reencoded := escapeBackslashesAndQuotes(decoded1) + decoded2, err := decodeQuotedStringContents(reencoded) + require.NoError(t, err) + assert.Equal(t, decoded1, decoded2, "decoding must be stable across a round trip") + + decodedAgain, err := decodeQuotedStringContents(in) + require.NoError(t, err) + assert.Equal(t, decoded1, decodedAgain, "decoding the same input twice must agree") + }) + } +} + +// escapeBackslashesAndQuotes re-encodes a decoded string value into a +// Python-style escaped literal body, for +// TestDecodeQuotedStringContents_RoundTripIsIdempotent. It only needs to +// handle what decodeQuotedStringContents can itself produce (a bare +// backslash or quote character), since every other decoded rune requires no +// escape to round-trip. +func escapeBackslashesAndQuotes(s string) string { + out := "" + for _, r := range s { + switch r { + case '\\': + out += `\\` + case '\'': + out += `\'` + case '"': + out += `\"` + default: + out += string(r) + } + } + return out +} + +// --- Escape decoding, via the public marker-parsing surface --- + +func TestParseMarker_QuotedStringLiteralDecodesEscapes(t *testing.T) { + expr := parseFullMarkerString(t, `os_name == "line\nbreak"`) + cmp, ok := expr.(*CompareExpr) + require.True(t, ok) + assert.Equal(t, Literal{Value: "line\nbreak"}, cmp.Rhs) +} + +func TestParseMarker_QuotedStringRejectsMalformedUnicodeEscape(t *testing.T) { + tok := NewTokenizer(`os_name == "\u12"`) + _, err := ParseMarker(tok) + assert.Error(t, err) +} diff --git a/internal/pep508/tokenizer.go b/internal/pep508/tokenizer.go index 6f02b99..eeae072 100644 --- a/internal/pep508/tokenizer.go +++ b/internal/pep508/tokenizer.go @@ -45,8 +45,8 @@ const ( // ('[^']*' / "[^"]*"), so a double-quoted string may embed unescaped // single quotes and vice versa, and a backslash tokenizes fine. // Upstream then validates the token via ast.literal_eval, i.e. Python - // string escapes ARE meaningful on input; see validateQuotedStringContents - // in marker.go for the subset we validate. + // string escapes ARE meaningful on input; see decodeQuotedStringContents + // in marker.go, which decodes and validates them. QuotedString // End matches only at the end of the source (zero-width). End @@ -142,12 +142,12 @@ type Token struct { } // Unquoted returns a QuotedString token's value with its surrounding quote -// characters stripped. No escape processing is performed: while -// pypa/packaging calls ast.literal_eval on the token to decode Python -// string-literal escapes (\n, \\, \x41, etc.), we deliberately do not -// implement full escape decoding here (out of scope). We validate only the -// two specific malformed cases upstream asserts in tests: a trailing unpaired -// backslash and a truncated \x escape. +// characters stripped. No escape processing is performed here - Unquoted +// stays a raw, tokenizer-level accessor, matching packaging's Token.text[1:-1] +// slice. Python string-literal escape decoding (\n, \\, \x41, etc.), which +// pypa/packaging performs via ast.literal_eval, happens one call away at +// Unquoted's only production call site: see decodeQuotedStringContents in +// marker.go. func (t Token) Unquoted() string { if len(t.Text) < 2 { return t.Text diff --git a/internal/pep508/tokenizer_test.go b/internal/pep508/tokenizer_test.go index 98190f8..663ec12 100644 --- a/internal/pep508/tokenizer_test.go +++ b/internal/pep508/tokenizer_test.go @@ -81,6 +81,16 @@ func TestCheck_QuotedString_Unterminated(t *testing.T) { assert.False(t, tok.check(QuotedString)) } +func TestUnquoted_DoesNotDecodeEscapes(t *testing.T) { + // Unquoted stays a raw, tokenizer-level accessor: escape decoding + // happens one call away, at Unquoted's only production call site (see + // decodeQuotedStringContents in marker.go, and Unquoted's doc comment). + tok := NewTokenizer(`"a\nb"`) + require.True(t, tok.check(QuotedString)) + got := tok.read() + assert.Equal(t, `a\nb`, got.Unquoted()) +} + // --- WS handling --- func TestConsume_WS_SkipsWhitespace(t *testing.T) {