Class `SQLParser` misparses C-style escape sequences?

[hartwork]

Hi!

This is related to #223. While skip-reading through related code, it came to my attention that the code at…

agent/apps/dbagent/src/lib/targetdb/unsafe-explain.ts

Lines 122 to 152 in 397bf8c

	private parseSingleQuotedString(): void {
	this.pos++; // Skip opening quote
	while (this.pos < this.len) {
	const char = this.input[this.pos];
	if (char === "'") {
	this.pos++;
	return;
	} else if (char === '\\') {
	this.pos += 2; // Skip escaped character
	} else {
	this.pos++;
	}
	}
	throw new Error('Unterminated single-quoted string');
	}
	private parseDoubleQuotedString(): void {
	this.pos++; // Skip opening quote
	while (this.pos < this.len) {
	const char = this.input[this.pos];
	if (char === '"') {
	this.pos++;
	return;
	} else if (char === '\\') {
	this.pos += 2; // Skip escaped character
	} else {
	this.pos++;
	}
	}
	throw new Error('Unterminated double-quoted string');
	}

…seems to mishandle numeric escape sequences of than 2 characters:

I have not checked for the consequences of this issue.

Best, Sebastian

[tsg]

hi @hartwork , I have been looking at this but because the escape sequences can only appear in string literals, and because they can't be used to terminate the string, I think there's now way to trick the goal of this function, which is just to identify where the query boundaries are.

I was looking for adding support for escape sequences in any case for clarity, but because I couldn't come up with a relevant test case, I'm thinking it's maybe better to not complicate the code.

[hartwork]

@tsg what about sequence \7"? That would terminate the string to the eyes of PostgreSQL but have parseDoubleQuotedString continue parsing, I think? PS: Personally I don't think there is anything between no parsing and full feature parsing here.

[tsg]

what about sequence \7"?

Aha, in that case, both would terminate the string (only the 7 is skipped), but I think something like \'' would be interpreted differently (terminate string in the Postgres, but not in our parser), because \ blindly skips the next char in the current code.

Since actually escape sequences are only allowed in special literals like Estring`` and U&"string", I think it's much easier to just forbid those, erroring on the side of caution. In any normal usage those would be parametrized anyway. The remaining regular literal strings don't allow escape sequences beyond the `''` and are pretty simple to parse. I'll push a commit for this. Thanks for thinking this through with me!