Skip to content

Switch to proxy URL to handle API requests to demo, #PG-5029#876

Open
lachiebol wants to merge 2 commits intolivefrom
PG-5029-fix-cors
Open

Switch to proxy URL to handle API requests to demo, #PG-5029#876
lachiebol wants to merge 2 commits intolivefrom
PG-5029-fix-cors

Conversation

@lachiebol
Copy link
Copy Markdown
Contributor

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

  • [✔] I have understood, reviewed, and tested all AI outputs before use
  • [✔] All AI instructions respect security, IP, and privacy rules

Review

window.onload = function () {
let pluginSpec = {{ pluginSpecJson|raw }};
let pluginSpecJson = {{ pluginSpecJson|raw }};
let proxyBaseUrl = window.location.origin + '/demo-proxy';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
let proxyBaseUrl = window.location.origin + '/demo-proxy';
let proxyBaseUrl = '/demo-proxy';


$proxiedResponse = DemoProxy::get($targetUrl, $request->getHeaderLine('Authorization'));

$response->getBody()->write($proxiedResponse['body']);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should add a try/catch here

});

$app->get('/demo-proxy/{path:.*}', function (Request $request, Response $response, $args) {
$path = ltrim($args['path'] ?? '', '/');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@AltamashShaikh AltamashShaikh left a comment

Choose a reason for hiding this comment

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

Is this correct ?

Image

Copy link
Copy Markdown
Contributor

@AltamashShaikh AltamashShaikh left a comment

Choose a reason for hiding this comment

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

left few comments

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

Development

Successfully merging this pull request may close these issues.

2 participants