Switch to proxy URL to handle API requests to demo, #PG-5029#876
Switch to proxy URL to handle API requests to demo, #PG-5029#876
Conversation
app/templates/api-swagger.twig
Outdated
| window.onload = function () { | ||
| let pluginSpec = {{ pluginSpecJson|raw }}; | ||
| let pluginSpecJson = {{ pluginSpecJson|raw }}; | ||
| let proxyBaseUrl = window.location.origin + '/demo-proxy'; |
There was a problem hiding this comment.
| let proxyBaseUrl = window.location.origin + '/demo-proxy'; | |
| let proxyBaseUrl = '/demo-proxy'; |
There was a problem hiding this comment.
Made that change, it also collapses the server field down to this
I changed it to 'demo' rather than 'demo-proxy', proxy felt too technical for a user facing page.
|
|
||
| $proxiedResponse = DemoProxy::get($targetUrl, $request->getHeaderLine('Authorization')); | ||
|
|
||
| $response->getBody()->write($proxiedResponse['body']); |
There was a problem hiding this comment.
we should add a try/catch here
There was a problem hiding this comment.
Done, I also realised we return errors with a 200 OK? At the moment it doesn't break anything in Swagger, technically we could catch in the proxy and throw something else
app/routes/page.php
Outdated
| }); | ||
|
|
||
| $app->get('/demo-proxy/{path:.*}', function (Request $request, Response $response, $args) { | ||
| $path = ltrim($args['path'] ?? '', '/'); |
There was a problem hiding this comment.
main concern is that it now becomes a proxy to query demo.matomo.cloud without any CORS error
We can add a check that requestURL path should be index.php and Module should be API along with CSRF token.
There was a problem hiding this comment.
Have added some URL validation so only the API can be queried.
Demo is a public instance, are we exposing anything that isn't already available with this proxy? For the CSRF token, we don't have a session on developer docs, so not sure where we can store that
AltamashShaikh
left a comment
AltamashShaikh
left a comment
There was a problem hiding this comment.
Is this correct ?
AltamashShaikh
left a comment
AltamashShaikh
left a comment
There was a problem hiding this comment.
left few comments
@AltamashShaikh That's the new proxy endpoint that swagger will make requests against, do you think it should still show demo.matomo.cloud? I might check if I can change the text but not the URL |
AltamashShaikh
left a comment
AltamashShaikh
left a comment
There was a problem hiding this comment.
@lachiebol Good to add some tests with the help of AI
2 participants
Description
Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.
Checklist
Review