fix(mql): terminate parse loop on quoted value as final token

[Lutherwaves]

Summary

mql.NewParser(...).Parse() loops forever on any query where a quoted value is the final token, e.g. k:"v" or name:"alice". This is a denial-of-service hazard for any code that parses untrusted MQL input on a request path.

Fixes #221.

Root cause

In mql/parser.go, parseValueFromPos advances the scanner past a quoted string with:

for p.s.Pos().Offset-1 < endIdx+1 {
    p.next()
}

When the closing quote is the last character of the input, endIdx+1 == len(input). The scanner is already at EOF, so next() no longer advances Offset (it caps at the input length). The condition len(input)-1 < len(input) stays true forever, and the loop never terminates.

Non-final quoted values happen to work only because later tokens push Offset past the threshold, letting the loop exit.

Fix

Guard the loop on forward progress — break as soon as next() fails to advance the scanner offset (i.e. EOF):

for p.s.Pos().Offset-1 < endIdx+1 {
    prev := p.s.Pos().Offset
    p.next()
    if p.s.Pos().Offset == prev {
        break // scanner reached EOF, no further progress possible
    }
}

Compatibility — not a breaking change

This is a strict bug fix; no exported API, signature, or type changes.

• The new break is unreachable for any input that already terminated. It fires only when next() fails to advance the scanner offset, which text/scanner does only at EOF. Any previously-terminating input satisfied the original exit condition (Offset-1 < endIdx+1 going false) before reaching EOF, so the added branch is never taken and the returned value and final scanner position are identical.
• The only inputs whose behavior changes are the ones that previously hung (quoted value as the final token). They never returned, so no caller could depend on the old behavior — they now parse to the correct TermExpr.
• Adjacent paths are untouched. An unclosed quote (k:"unclosed) never enters this block (endIdx == len(input)) and still falls through to the unquoted-value path unchanged.

A differential battery over quoted/unquoted/dotted-key/compound/unclosed-quote/wildcard inputs produced identical results to the pre-fix parser on every input that didn't hang.

Test

Adds TestParseQuotedValueAsFinalToken in a new mql/parser_test.go. Each case parses in a goroutine and asserts termination within a timeout, so a future regression fails the suite instead of hanging it. It also asserts the parsed key/value are correct. The test hangs (times out) against the old code and passes against the fix.

$ go test ./mql/ -run TestParseQuotedValueAsFinalToken
ok  	github.com/tink3rlabs/magic/mql

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed a potential unresponsive/hanging behavior when parsing quoted values at end-of-input.
  • Quoted values that are the final token—including cases with trailing spaces—now parse reliably.

• Tests

  • Added automated coverage to ensure quoted values used as the final query token parse successfully without timing out, including inputs with trailing spaces.

[coderabbitai]

Warning

Review limit reached

@Lutherwaves, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 56 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c939b929-f0fc-4d49-add4-0556ae871310

📥 Commits

Reviewing files that changed from the base of the PR and between 45deaa2 and e53ad2b.

📒 Files selected for processing (2)

• mql/parser.go
• mql/parser_test.go

📝 Walkthrough

Walkthrough

The quoted-value scanner loop now detects EOF without progress, preventing an infinite loop when a quoted value is the final token. A regression test covers multiple final-token inputs and validates the parsed expression.

Changes

Quoted Value EOF Handling

Layer / File(s)	Summary
Guard quoted parsing at EOF mql/parser.go, mql/parser_test.go	The parser breaks when scanner offsets stop advancing, while tests verify termination and parsed keys and values for quoted final tokens.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: deanefrati

Poem

A bunny found a scanner stuck,
Its final quote had run out of luck.
A progress check made parsing hop,
Tests now ensure the loop will stop.
“No endless spins!” the rabbit cheered.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The parser fix and regression test satisfy #221, but the linked #39 SNS dependency bump is not addressed.	Add the aws-sdk-go-v2/service/sns 1.34.5 bump for #39, or remove that issue from the PR scope if it is unrelated.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately summarizes the parser-loop fix.
Out of Scope Changes check	✅ Passed	No out-of-scope code changes are visible; the diff stays within the parser bug fix and regression test.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}