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

Fix invalid json escaping #224

Merged
merged 3 commits into from
Jul 16, 2021

Conversation

gbip
Copy link
Contributor

@gbip gbip commented Jul 12, 2021

This fixes #223.

@saosebastiao Could you report if this fixes your issue ?

@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #224 (d3b0a09) into master (aaae520) will decrease coverage by 0.06%.
The diff coverage is 66.66%.

❗ Current head d3b0a09 differs from pull request most recent head e78333e. Consider uploading reports for the commit e78333e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
- Coverage   49.02%   48.96%   -0.07%     
==========================================
  Files          14       14              
  Lines        1638     1636       -2     
==========================================
- Hits          803      801       -2     
  Misses        835      835              
Impacted Files Coverage Δ
src/function_source.rs 89.79% <66.66%> (-0.41%) ⬇️
src/utils.rs 66.66% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaae520...e78333e. Read the comment docs.

@saosebastiao
Copy link

Yes, this works correctly for our specific function source.

It might be good to modify the public.function_source function in the test suite to test for json parseability.

IF (query_params->>'sample_param')::varchar IS NULL THEN
    RAISE EXCEPTION 'the `sample_param` json parameter does not exist in `query_params`';
END

Then a test case could be written that feeds it '{"sample_param":"hello world"}'::json. If the query_params is an actual parseable json object, it will not throw an exception.

@gbip
Copy link
Contributor Author

gbip commented Jul 13, 2021

I agree that adding a test case would be a good idea, however I have more urgent tasks right now.

@gbip
Copy link
Contributor Author

gbip commented Jul 15, 2021

Yes, this works correctly for our specific function source.

It might be good to modify the public.function_source function in the test suite to test for json parseability.

IF (query_params->>'sample_param')::varchar IS NULL THEN
    RAISE EXCEPTION 'the `sample_param` json parameter does not exist in `query_params`';
END

Then a test case could be written that feeds it '{"sample_param":"hello world"}'::json. If the query_params is an actual parseable json object, it will not throw an exception.

I have been investigating a way for us to have some kind of unit tests.
However I am lacking sql skills and I could not find a way to write a function that tests if sample_param is not null.
If you happen to have written I can try to integrate it into the test suite, otherwise my PR will be ready for review.

@stepankuzmin
Copy link
Collaborator

Hey @gbip,

Thanks a lot for the PR! Could you please allow edits from maintainers, so I can add tests for this case?

@gbip
Copy link
Contributor Author

gbip commented Jul 16, 2021

It is on :)
2021-07-16-094331_271x264_scrot

@stepankuzmin stepankuzmin merged commit 4994273 into maplibre:master Jul 16, 2021
@stepankuzmin
Copy link
Collaborator

Awesome, merged! Thank you, @gbip

@frodrigo
Copy link
Contributor

Since this fix a security issue, I suggest making a new release.

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.

Recent pull request broke parameter passing via json
4 participants