Added parentheses for permissions policy parser according issue #195
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hey, When I tried this PR with default configuration it does not work correctly and throws an error in the browser console Tested with following config: permissionsPolicy: {
'camera': ['()'],
'display-capture': ['()'],
'fullscreen': ['()'],
'geolocation': ['()'],
'microphone': ['()'],
}It generates following headers: Permissions-Policy:
camera=(()), display-capture=(()), fullscreen=(()), geolocation=(()), microphone=(())
|
|
By default |
|
@nekotoriy I meant that I was using the default config. With changes from your PR, the the module is not working correctly in terms of permissions policy. It may work for your case, but it does not work with the default config or when the user will pass just a single I think you would have to add some condition to the parser to only add additional parentheses when value passes is an array with multiple elements. |
|
But in this case, passing |
|
Hey @nekotoriy sorry for no response but I was on holidays. In this case we have two options then:
From my perspective, the better would be the option two as it wont break the functionality for the current users. However, the option one might be better for the future so we could plan it for the 1.0.0 version where we could ship breaking changes. Let me know which option would work better from your side and we can think about it :) |
|
Hello @Baroshem! At the moment I don't have an urgent need to fix this bug, so we can schedule break changes for version 1.0.0 following the first point. |
|
@nekotoriy Ok, so I will add this fix to the plan for 1.0.0 and we will work on it in the upcoming weeks :) |
|
Alright, thank you! |
|
Hey @nekotoriy I have changed the target branch to 1.0.0-rc.1 as I want to include this functionality in the upcoming 1.0.0-rc.1 release. I have tested it and it seems to be working as expected :) |
| Object.entries(value) | ||
| .map( | ||
| ([directive, sources]) => | ||
| (sources as string[])?.length && |
There was a problem hiding this comment.
issue: here you can safely delete the checking for length as we will allow for empty arrays that will be translated to ()
| @@ -41,7 +41,15 @@ const headerValueMappers = { | |||
| }) | |||
| .filter(Boolean).join('; ') | |||
| }, | |||
| permissionsPolicy: (value: PermissionsPolicyValue) => Object.entries(value).map(([directive, sources]) => (sources as string[])?.length && `${directive}=${(sources as string[]).join(' ')}`).filter(Boolean).join(', ') | |||
| permissionsPolicy: (value: PermissionsPolicyValue) => | |||
There was a problem hiding this comment.
issue: please change defaultConfig.ts value for permissionsPolicy from the existing one to:
permissionsPolicy: {
camera: [],
'display-capture': [],
fullscreen: [],
geolocation: [],
microphone: []
}
This with the addition of the next comment would allow to have both () and your functionality :)
|
Hey @nekotoriy Do you need any help with the implementation. I can do this if you dont have time (I will still mention you in the release notes as the contributor so no worries about that :) ) |
|
@nekotoriy Are you planning to add the changes I mentioned? If you dont have time, just let me know. I will add the changes and include you in the release notes :) |
|
Closing in favor of #212 |
Fixes: #194