Skip to content

[2.x] Escape single-quotes from SOQLConnection#107

Open
caseydwyer wants to merge 4 commits intoroblesterjr04:masterfrom
caseydwyer:bug/escape-single-quote-strings
Open

[2.x] Escape single-quotes from SOQLConnection#107
caseydwyer wants to merge 4 commits intoroblesterjr04:masterfrom
caseydwyer:bug/escape-single-quote-strings

Conversation

@caseydwyer
Copy link

@roblesterjr04 revisiting this bit from #95 tonight, and I'm pretty sure #96 doesn't actually solve the underlying problem.

I've continued to get SOQL errors related to unescaped quotes. As far as I can tell, the toSql method inside of SOQLBuilder.php never actually gets called, at least not on SELECT queries. Eg, you can chuck a dd('won't ever show'); into the top of that method, hit a page or endpoint that calls a select query, and it will go through without dumping. This might be anecdotal, but at least on simple tests I ran, it's never getting called.

I think what's happening with that SOQLBuilder class, is that it's intending to override the toSql method from its parent...but SOQLBuilder extends the Eloquent Builder, not the Query Builder, and that toSql method exists on the latter. It therefore never gets called on the former, which means the SOQLBuilder's override never gets called either. This is handled via the $passthru property from the Eloquent Builder, which (via the toBase() method) hands off calls like toSql down to the Query Builder.

Definitely wouldn't put money on this, but I suspect you could delete that entire toSql method from SOQLBuilder, and nothing would break, as I just don't think it gets called anywhere (except maybe in SOQLBatch).

Will defer to you on any changes to that class, though—this PR doesn't touch it.


So TL;DR - this PR essentially takes the patch in #96, and relocates it to the SOQLConnection->prepareBindings() method with an is_string check, where it actually gets called & applied.

I would definitely give this all a thorough review, though, as I think there might be some security considerations with how ' and similar characters are able to be passed in? Dunno, that's not my forte, so I defer to your expertise. 🙂 That said, let me know if there's anything you'd like to see modified with this PR; happy to help where I can.

::of and stringable unnecessary w/ is_string check
@caseydwyer
Copy link
Author

Hey @roblesterjr04! 👋 Sorry to bother, just wanted to see if you had a chance to check this out. Any thoughts/feedback?

@roblesterjr04
Copy link
Owner

Hi Casey, i just haven't had time to review it in depth. Have you run the unit tests on your machine?

@danielpetrica
Copy link
Contributor

danielpetrica commented Jun 7, 2024

I've meet a problem which I think can be solved by this. I need to query some data and filter using this value Sant'Elena.

I overcome it with the following code

Str::replace("'", "\'", urldecode($request->input_field)));

Copy link
Contributor

@danielpetrica danielpetrica left a comment

Choose a reason for hiding this comment

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

There's a missing parentesis on line 158

Copy link
Contributor

@danielpetrica danielpetrica left a comment

Choose a reason for hiding this comment

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

Looks good to me

@caseydwyer
Copy link
Author

@roblesterjr04 mannn, I apologize for the insane-o delay here! Had this on my radar in March of last year, and it just completely slipped through the cracks. That's my fault, sorry for the wait. Added a test (here, in #107) for escaping single-quotes in strings and confirmed that all other tests are passing...though you might need to also pull in/merge #122 for things to click. Let me know if there's anything else I can help with here, and I'll respond in a timely manner. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants