Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escaping a string to pass to flash() causes a PHP warning #231

Open
AurelioDeRosa opened this issue Sep 3, 2014 · 10 comments
Open

Escaping a string to pass to flash() causes a PHP warning #231

AurelioDeRosa opened this issue Sep 3, 2014 · 10 comments
Assignees
Labels
Milestone

Comments

@AurelioDeRosa
Copy link
Contributor

I'm using Klein 2.0.2 and in a function I have:

$service->flash('my message' . $service->escape($request->data), 'info');

While trying the robustness of the code, I set $request->data to <script> which is converted into %3Cscript%3E and this is the message I receive:

Warning: vsprintf(): Too few arguments in XXX\vendor\klein\klein\Klein\ServiceProvider.php on line 239

I've noted that even without calling $service->escape() the issue is still there because the parameter has already been escaped.

Hope this helps.

@Rican7 Rican7 self-assigned this Sep 3, 2014
@Rican7 Rican7 added the Bug label Sep 3, 2014
@Rican7 Rican7 added this to the 2.1 milestone Sep 3, 2014
@Rican7
Copy link
Member

Rican7 commented Oct 24, 2014

Interesting. The error you're getting is referring to the markdown() method in the ServiceProvider, as you can see here: https://github.com/chriso/klein.php/blob/6e1f228ce82333b437402dafad049713eb3eeac6/Klein/ServiceProvider.php#L239

I don't see where you are calling markdown() in your example.

@AurelioDeRosa
Copy link
Contributor Author

Indeed I don't call it, and yes...this is really interesting.

@Rican7
Copy link
Member

Rican7 commented Oct 24, 2014

Ah ha!
I see why the error is happening. If you pass a string to markdown() it calls vsprintf(). Since the printf() family of functions use %x (where x is a modifier) as notation for string replacements, it makes sense that its causing an error when you pass a string containing % characters.

I've fixed this locally on a branch of mine by escaping the % characters before passing them in.

You must be calling markdown() somewhere in your chain. There's no other way that you'd get that error or line-trace.

@AurelioDeRosa
Copy link
Contributor Author

I'm sorry but I really don't use that method. Anyway, if you want you can create a release with this fix so that I can update my vendor folder, and then I'll inform you of the result.

@Rican7
Copy link
Member

Rican7 commented Oct 24, 2014

Hmm, the problem is that escaping the % signs means it'll no longer allow for template string parsing.
Its a regression.

I won't be able to release this without a more clever fix.

@Rican7
Copy link
Member

Rican7 commented Oct 24, 2014

@AurelioDeRosa
Copy link
Contributor Author

I see. Do whatever is best for the project, I've solved this issue as shown below:

$service->flash('my message' . $service->escape(strip_tags(urldecode($request->data))), 'info');

@Rican7
Copy link
Member

Rican7 commented Oct 24, 2014

There you go!
👍

Now if only we could figure out how you're ever even reaching line 239 in the markdown() method...

@Rican7 Rican7 removed this from the 2.1 milestone Oct 24, 2014
@nbish11
Copy link

nbish11 commented Oct 26, 2014

@Rican7 I could be wrong, but isn't the flash() method calling the markdown() method behind the scenes?

@Rican7
Copy link
Member

Rican7 commented Oct 26, 2014

@nbish11 ohhhh yea. :P
Well that's strange. Damn.

@Rican7 Rican7 added this to the 2.2 milestone Nov 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants