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.

Sign up for GitHub

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

Jump to bottom

Data proxy: Fix encoded characters in URL path should be proxied as encoded #30597

Merged
merged 10 commits into from
Mar 17, 2021
Merged

Data proxy: Fix encoded characters in URL path should be proxied as encoded #30597

merged 10 commits into from
Mar 17, 2021

Conversation

marefr
Copy link
Contributor

@marefr marefr commented Jan 25, 2021 •
edited
Loading

What this PR does / why we need it:
Fix encoded characters in URL path should be proxied encoded in data proxy.

Which issue(s) this PR fixes:
Fixes #26870
Fixes #31438

Special notes for your reviewer:
Is it viable to replace c.Params("*") with a replace of the raw URL path?

@marefr marefr requested a review from a team as a code owner January 25, 2021 12:42
@marefr marefr requested review from aknuds1, jessabe, zoltanbedi and wbrowne and removed request for a team and jessabe January 25, 2021 12:42
aknuds1
aknuds1 reviewed Jan 25, 2021
View reviewed changes
pkg/api/dataproxy.go Outdated
var proxyPathRegexp = regexp.MustCompile("^\\/api\\/datasources\\/proxy\\/[\\d]+\\/?")

func getProxyPath(originalRawPath string) string {
return proxyPathRegexp.ReplaceAllString(originalRawPath, "")
Copy link
Contributor

@aknuds1 aknuds1 Jan 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can you just move the call to ensureProxyPathTrailingSlash here, is there any need to require the caller to take care of this? Benefit would be more of the code covered by the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@domasx2 pointed out that ensureProxyPathTrailingSlash is no longer needed. Please have a look again

pkg/api/dataproxy_test.go Outdated Show resolved Hide resolved
@zoltanbedi
Copy link
Member

It is still not working but thank you very much for picking this up.
In here https://github.com/grafana/grafana/blob/master/pkg/api/pluginproxy/ds_proxy.go#L193
proxyPath is still good with url encoded

In here https://github.com/grafana/grafana/blob/master/pkg/api/pluginproxy/ds_proxy.go#L101
proxy.ctx.Req.URL.Path is decoded

domasx2
domasx2 reviewed Feb 24, 2021
View reviewed changes
pkg/api/dataproxy.go Outdated Show resolved Hide resolved
@domasx2
Copy link
Contributor

domasx2 commented Feb 24, 2021

I tested this a bit, and seems that now the slash gets escape twice 😞

Eg if you set req.URL.Path = "/api/prom/rules/default/%2Ftest" you get req.URL.String() == "https://prometheus-us-central1.grafana.net/api/prom/rules/default/%252Ftest"

@marefr
Copy link
Contributor Author

marefr commented Mar 16, 2021

I'll have a look and see if I can resolve these problems again

@marefr marefr requested a review from domasx2 March 16, 2021 13:32
@marefr
Copy link
Contributor Author

marefr commented Mar 16, 2021

@zoltanbedi @domasx2 can you please help out testing this out and/or provide some way to easily verify my changes. I tried to use the docker/blocks/slow_proxy, but with my changes and trying to include escaped slash doesn't arrive at the slow proxy 😕

@marefr
Copy link
Contributor Author

marefr commented Mar 16, 2021

I think I might be able to verify using the jaeger docker block...

@marefr
Copy link
Contributor Author

marefr commented Mar 16, 2021

@zoltanbedi @domasx2 Using jaeger with a service named grafana/service I can list the operations so it seems to work as expected now:
image
image

zoltanbedi
zoltanbedi approved these changes Mar 16, 2021
View reviewed changes
Copy link
Member

@zoltanbedi zoltanbedi left a comment

Choose a reason for hiding this comment

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

Thank you @marefr lgtm.

marefr and others added 2 commits March 16, 2021 15:40
@marefr
Fix lint issue
9d19a19
@marefr @aknuds1
Update pkg/api/dataproxy_test.go
508a2c9 
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
@domasx2
Copy link
Contributor

domasx2 commented Mar 16, 2021

Works for me as well, thanks @marefr 👍

@marefr marefr requested a review from aknuds1 March 16, 2021 15:14
@marefr marefr added this to the 7.5.0-beta2 milestone Mar 16, 2021
@marefr marefr changed the title Data proxy: Make sure encoded values are proxied Data proxy: Fix encoded characters in URL path should be proxied encoded Mar 16, 2021
@marefr marefr added the old backport v7.5.x Mark PR for automatic backport to v7.5.x label Mar 16, 2021
@marefr marefr merged commit c0edf88 into master Mar 17, 2021
@marefr marefr deleted the 26870_fix branch March 17, 2021 11:17
@marefr marefr changed the title Data proxy: Fix encoded characters in URL path should be proxied encoded Data proxy: Fix encoded characters in URL path should be proxied as encoded Mar 17, 2021
@grafanabot grafanabot mentioned this pull request Mar 17, 2021
Merged
grafanabot pushed a commit that referenced this pull request Mar 17, 2021
@marefr @grafanabot
Data proxy: Fix encoded characters in URL path should be proxied enco…
45c9229 
…ded (#30597)

Fix encoded characters in URL path should be proxied as encoded in the data proxy.

Fixes #26870
Fixes #31438

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
(cherry picked from commit c0edf88)
marefr added a commit that referenced this pull request Mar 17, 2021
@grafanabot @marefr
Data proxy: Fix encoded characters in URL path should be proxied enco…
3c8daef 
…ded (#30597) (#32060)

Fix encoded characters in URL path should be proxied as encoded in the data proxy.

Fixes #26870
Fixes #31438

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
(cherry picked from commit c0edf88)

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
ryantxu pushed a commit that referenced this pull request Mar 17, 2021
@marefr @aknuds1 @ryantxu
Data proxy: Fix encoded characters in URL path should be proxied enco…
dc7d817 
…ded (#30597)

Fix encoded characters in URL path should be proxied as encoded in the data proxy.

Fixes #26870 
Fixes #31438

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
ryantxu pushed a commit to dejapong/grafana that referenced this pull request Mar 30, 2021
@marefr @aknuds1 @ryantxu
Data proxy: Fix encoded characters in URL path should be proxied enco…
6f8ac3f 
…ded (grafana#30597)

Fix encoded characters in URL path should be proxied as encoded in the data proxy.

Fixes grafana#26870 
Fixes grafana#31438

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
@kaydelaney kaydelaney mentioned this pull request Sep 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zoltanbedi zoltanbedi zoltanbedi approved these changes

@domasx2 domasx2 domasx2 approved these changes

@wbrowne wbrowne Awaiting requested review from wbrowne

@aknuds1 aknuds1 Awaiting requested review from aknuds1

Assignees

@marefr marefr

Labels
add to changelog area/datasource/proxy old backport v7.5.x Mark PR for automatic backport to v7.5.x type/bug
Projects
None yet
Milestone
7.5.0-beta2
4 participants
@marefr @zoltanbedi @domasx2 @aknuds1