diff --git a/CHANGELOG.md b/CHANGELOG.md index 378edc0..42b0b40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,49 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Reject raw control characters anywhere in the input, matching libyaml's + own reader-level behavior. Found by directly comparing `PureYAML.parse()` + against Yams on a 221MB real-world OpenAPI/compose-file corpus (150 + files): a genuinely public file with corrupted, mojibake-looking encoding + contains a raw C0 control character, which Yams's underlying libyaml + rejects unconditionally at the reader level ("control characters are not + allowed"), but `PureYAML.parse()` silently accepted and built a value tree + from. `PureYAML` already had a printable-character predicate matching + libyaml's `c-printable` set (`CharacterClass.isPrintable`), but it was + only ever applied narrowly to anchor names + (`LibYAML.Scanner.Anchor.swift`), never to the input generally; the + `Validation.PreflightScanner`'s separate `invalidControlCharacterColumn` + check exists but is opt-in via `validate()`, not part of what `parse()` + itself runs. + `Reader` now records the index of the first non-printable scalar in + whatever it has buffered (`init`/`append(_:)`, covering both batch and + streaming input), and `fetchNextToken()` throws once actual scanning has + reached that position (gated on `reader.index >= badIndex`, not just "one + exists"), so a real syntax error earlier in the document is not hidden + behind a control character occurring later, matching the order libyaml's + own byte-by-byte reader would surface them in. New `ParseError` case + `.invalidControlCharacter(line:column:)`, translated from the scanner + error the same way `.tabIndentation` already is. + Verified with 5 new tests (a bare control character rejected at the exact + position; a tab in scalar content, and CR/LF, both still legal, since + `isPrintable` already correctly includes them; a control character 5000 + lines into a document still caught; an earlier real syntax error not + masked by a later control character) plus a full re-run of the official + yaml-test-suite conformance (still 333/333 parse-level, 208/208 + value-level of comparable cases, unchanged) and the whole-suite streaming + differential (still clean), confirming this closes a real gap without + regressing anything already passing. Negligible throughput impact + measured on the existing 9.5MB benchmark fixture (within normal run-to-run + noise). + Disclosed limitation: the check is applied in `fetchNextToken()`, which + `scanStep()`'s plain-scalar resume path (`resumePlainScalar`, the + streaming API) does not route through, so a token containing a control + character can be returned to a streaming caller one token before this + fires. `scanAll()`/`PureYAML.parse()`, the primary target here (matching + what the corpus comparison actually calls), always calls + `fetchNextToken()` again before returning, so it is caught before a batch + parse completes. + - Add a public `PureYAML.Streaming` namespace (issue #4), exposing the internal `LibYAML.Scanner.scanStep()` proven below for the first time as a callable public API rather than a module-internal implementation detail. diff --git a/Sources/Parsing/LibYAML.Reader.swift b/Sources/Parsing/LibYAML.Reader.swift index b1de466..bdd423a 100644 --- a/Sources/Parsing/LibYAML.Reader.swift +++ b/Sources/Parsing/LibYAML.Reader.swift @@ -22,9 +22,21 @@ extension PureYAML.Parsing.LibYAML { /// unreachable when `isFinal` is true. private(set) var ranDry = false + /// The index of the first non-`c-printable` scalar found in `scalars` + /// (libyaml's reader-level check, `yaml_parser_update_buffer`'s printable + /// validation), or nil if none has been found yet. Set once, by whichever + /// of `init`/`append(_:)` first buffers the offending scalar; never + /// cleared. `fetchNextToken()` checks this once per token and throws, + /// mirroring libyaml rejecting raw control characters (anything outside + /// `c-printable`, i.e. not a plain tab/CR/LF and not in the printable + /// ranges `CharacterClass.isPrintable` implements) wherever they occur in + /// the stream, not only inside specific constructs like anchor names. + private(set) var invalidControlCharacterIndex: Int? + init(_ string: String = "", isFinal: Bool = true) { scalars = Array(string.unicodeScalars) self.isFinal = isFinal + recordFirstInvalidControlCharacter(in: 0 ..< scalars.count) } /// `PEEK_TOKEN` / `CHECK_AT`: the scalar `offset` ahead, or nil past the @@ -49,8 +61,10 @@ extension PureYAML.Parsing.LibYAML { /// reader position instead of restoring a snapshot (`PlainScalarSuspended`) /// has no other point where a stale `true` would ever become false again. mutating func append(_ chunk: String) { + let previousCount = scalars.count scalars.append(contentsOf: chunk.unicodeScalars) ranDry = false + recordFirstInvalidControlCharacter(in: previousCount ..< scalars.count) } /// Declare that no further chunks are coming: a `peek` past the buffered @@ -90,5 +104,49 @@ extension PureYAML.Parsing.LibYAML { line += 1 column = 0 } + + /// The 0-based line/column of `scalars[index]`, computed by counting line + /// breaks from the start of the buffer. Not the hot `mark` path (that + /// tracks `line`/`column` incrementally as scanning consumes characters); + /// used only once, lazily, when `fetchNextToken()` actually needs to report + /// `invalidControlCharacterIndex` as a `Mark`, since that index can be + /// anywhere in the buffer, not necessarily at the reader's current + /// scanning position. + func mark(atIndex target: Int) -> PureYAML.Parsing.Mark { + var line = 0 + var column = 0 + var cursor = 0 + while cursor < target, cursor < scalars.count { + if scalars[cursor] == "\r", cursor + 1 < scalars.count, scalars[cursor + 1] == "\n" { + line += 1 + column = 0 + cursor += 2 + } else if CharacterClass.isBreak(scalars[cursor]) { + line += 1 + column = 0 + cursor += 1 + } else { + column += 1 + cursor += 1 + } + } + return PureYAML.Parsing.Mark(line: line, column: column, index: target) + } + + /// Scans `range` (a slice of `scalars` just buffered by `init`/`append(_:)`) + /// for the first scalar outside `CharacterClass.isPrintable`, recording its + /// index if one is found and none was already recorded. Only the first + /// occurrence overall matters (matching libyaml, which reports the first + /// invalid byte and stops), so once `invalidControlCharacterIndex` is set + /// this is a no-op. + private mutating func recordFirstInvalidControlCharacter(in range: Range) { + guard invalidControlCharacterIndex == nil else { + return + } + for index in range where !CharacterClass.isPrintable(scalars[index]) { + invalidControlCharacterIndex = index + return + } + } } } diff --git a/Sources/Parsing/LibYAML.Scanner.Fetch.swift b/Sources/Parsing/LibYAML.Scanner.Fetch.swift index b9426c5..175ad15 100644 --- a/Sources/Parsing/LibYAML.Scanner.Fetch.swift +++ b/Sources/Parsing/LibYAML.Scanner.Fetch.swift @@ -23,6 +23,25 @@ extension PureYAML.Parsing.LibYAML.Scanner { fetchStreamStart() return } + // Gated on `reader.index >= badIndex`, not just "is one set", so a real + // syntax error earlier in the document (which normal scanning would reach + // and throw first) is not hidden behind a control character that only + // occurs later: this only fires once the reader has genuinely scanned up + // to the offending position, matching the order libyaml's own byte-by-byte + // reader would hit them in. `resumePlainScalar`'s own suspend/resume path + // (`scanStep()`, the streaming API) does not route through + // `fetchNextToken()` and so is not covered by this check; a token + // containing a control character can be returned to a streaming caller one + // token before this fires. `scanAll()`/`PureYAML.parse()`, the primary + // target, always calls `fetchNextToken()` again before returning, so this + // still surfaces before a batch parse completes. + if let badIndex = reader.invalidControlCharacterIndex, reader.index >= badIndex { + throw ScannerError( + context: "while scanning for the next token", + problem: "control characters are not allowed", + mark: reader.mark(atIndex: badIndex), + ) + } try scanToNextToken() try checkFlowContinuationIndent() diff --git a/Sources/Parsing/ParseError.LibYAML.swift b/Sources/Parsing/ParseError.LibYAML.swift index 24cb8c1..3678a98 100644 --- a/Sources/Parsing/ParseError.LibYAML.swift +++ b/Sources/Parsing/ParseError.LibYAML.swift @@ -8,6 +8,8 @@ extension PureYAML.Parsing.ParseError { let context = error.context ?? "" if error.problem.hasPrefix("found a tab") { self = .tabIndentation(line: line) + } else if error.problem == "control characters are not allowed" { + self = .invalidControlCharacter(line: line, column: column) } else if context == "while scanning a quoted scalar", error.problem.hasSuffix("end of stream") { self = .unterminatedQuotedString(line: line) } else if context.hasPrefix("while scanning a tag") || context.hasSuffix("TAG directive") { diff --git a/Sources/Parsing/ParseError.swift b/Sources/Parsing/ParseError.swift index 3f5e8c5..4d95610 100644 --- a/Sources/Parsing/ParseError.swift +++ b/Sources/Parsing/ParseError.swift @@ -3,6 +3,7 @@ public extension PureYAML.Parsing { enum ParseError: Swift.Error, Equatable, Sendable, CustomStringConvertible { case emptyDocument case tabIndentation(line: Int) + case invalidControlCharacter(line: Int, column: Int) case unexpectedIndentation(line: Int) case mixedCollectionStyles(line: Int) case expectedMappingKey(line: Int) @@ -30,6 +31,8 @@ public extension PureYAML.Parsing { "document is empty" case let .tabIndentation(line): "tabs are not allowed for indentation at line \(line)" + case let .invalidControlCharacter(line, column): + "invalid control character at line \(line), column \(column)" case let .unexpectedIndentation(line): "unexpected indentation at line \(line)" case let .mixedCollectionStyles(line): @@ -79,6 +82,8 @@ public extension PureYAML.Parsing { "emptyDocument" case .tabIndentation: "tabIndentation" + case .invalidControlCharacter: + "invalidControlCharacter" case .unexpectedIndentation: "unexpectedIndentation" case .mixedCollectionStyles: @@ -147,6 +152,7 @@ public extension PureYAML.Parsing { let .unexpectedEvent(_, _, line, _), let .unexpectedToken(_, _, line, _), let .nestingTooDeep(line, _), + let .invalidControlCharacter(line, _), let .malformed(_, line, _): line } @@ -176,6 +182,7 @@ public extension PureYAML.Parsing { let .unexpectedEvent(_, _, _, column), let .unexpectedToken(_, _, _, column), let .nestingTooDeep(_, column), + let .invalidControlCharacter(_, column), let .malformed(_, _, column): column } diff --git a/Tests/InvalidControlCharacterTests.swift b/Tests/InvalidControlCharacterTests.swift new file mode 100644 index 0000000..40fd90d --- /dev/null +++ b/Tests/InvalidControlCharacterTests.swift @@ -0,0 +1,55 @@ +@testable import PureYAML +import Testing + +/// The reader-level control-character check (`Reader.recordFirstInvalidControlCharacter`, +/// `fetchNextToken()`'s check in `LibYAML.Scanner.Fetch.swift`) added to close a real gap +/// found comparing PureYAML against Yams on a real-world OpenAPI corpus: Yams's underlying +/// libyaml rejects raw control characters (`c-printable` violations) anywhere in the input, +/// while `PureYAML.parse()` previously accepted them silently (the only existing check, +/// `PreflightScanner.invalidControlCharacterColumn`, is opt-in via `validate()`, not part of +/// the core scanner `parse()` uses). +@Suite("Invalid control character rejection") +struct InvalidControlCharacterTests { + @Test("A raw control character in scalar content is rejected with the correct position") + func test_rawControlCharacterRejected() { + expectParseError( + "key: value\u{01}with control\n", + .invalidControlCharacter(line: 1, column: 11), + ) + } + + @Test("A tab in scalar content, not indentation, is still legal") + func test_tabInContentStillAccepted() throws { + let value = try PureYAML.parse("key: value\twith tab\n") + let mapping = requireMapping(value) + #expect(mapping?["key"] == .string("value\twith tab")) + } + + @Test("CR and LF are still legal") + func test_crAndLFStillAccepted() throws { + let value = try PureYAML.parse("key: |\n line one\n line two\n") + let mapping = requireMapping(value) + #expect(mapping?["key"] == .string("line one\nline two\n")) + } + + @Test("A control character deep inside a large document is still caught") + func test_controlCharacterFarIntoDocumentIsCaught() { + let padding = String(repeating: "line: value\n", count: 5000) + expectParseError( + padding + "bad: value\u{02}here\n", + .invalidControlCharacter(line: 5001, column: 11), + ) + } + + @Test("An earlier real syntax error is not hidden behind a later control character") + func test_earlierSyntaxErrorNotHiddenByLaterControlCharacter() { + do { + _ = try PureYAML.parse("key: \"unterminated\nbad: value\u{02}here\n") + recordIssue("expected parse to throw") + } catch let error as PureYAML.Parsing.ParseError { + #expect(error.diagnosticCode != "invalidControlCharacter") + } catch { + recordIssue("expected a ParseError, got \(error)") + } + } +}