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
Protect telemetry properties route #15111
Conversation
Codecov ReportBase: 59.87% // Head: 59.90% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #15111 +/- ##
==========================================
+ Coverage 59.87% 59.90% +0.02%
==========================================
Files 1348 1348
Lines 32810 32812 +2
Branches 6260 6260
==========================================
+ Hits 19646 19655 +9
+ Misses 11308 11302 -6
+ Partials 1856 1855 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
if (uuid && currentToken) { | ||
const { | ||
data: { data: properties }, | ||
} = await axios.get(`${strapi.backendURL}/admin/telemetry-properties`); | ||
} = await axios.get(`${strapi.backendURL}/admin/telemetry-properties`, { | ||
headers: { | ||
Authorization: `Bearer ${currentToken}`, | ||
Accept: 'application/json', | ||
'Content-Type': 'application/json', | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simotae14 Where are we with the new fetch hook? This would be a prime example to use it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He's on holiday but it's ready to merge, so maybe we should just merge it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great - I'll do it. @christiancp100 Would be up to refactoring this part a bit using #14759?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, actually I did it with the useFetchClient hook in the first place a couple of weeks ago, but as it wasn't getting merged I re-implemented it without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 🌻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Issue related to this fix encountered. Does not impact any visible functionality, however may not be a desired outcome. nvm: 18.13.0 Whenever loading up the admin interface, specifically in develop, The important piece of information is that the user is not yet logged in, which is causing the request to fail...as they have no auth token. Steps to repeat:
Let me know if this is a problem, or the intended outcome. |
I fixed it by deleting the .cache, build & node_modules folders and running yarn install -> yarn develop |
What does it do?
Makes admin/telemetry-properties route private just for admin users and modifies the frontend so that it doesn't break because of this backend protection.
Why is it needed?
To avoid potential security gaps exposing sensitive data of the strapi instance.
How to test it?
Try to go to admin/telemetry-properties and check that it gives a 401 error if a valid Bearer token is not provided. Check also that in the admin interface, the admin/telemetry-properties route is not being called when the frontend doesn't have a valid token.
Related issue(s)/PR(s)
Needs to be merged before: #14900