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.

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

Remove frame-ancestors CSP to allow embedding via iframe #5170

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nisargjhaveri
Copy link
Contributor

Fix #5169.

While the https://profiler.firefox.com/from-post-message/ endpoint is designed with iframes in mind, the actual deployment disallows embedding profiler in an iframe.

I'm trying to use the profiler to analyse Android CPU profiles in an VSCode extension. And this ability would help a lot.

Proposing this change as I think this was the original intention.

Copy link

codecov bot commented Oct 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.62%. Comparing base (8764e83) to head (6ba2029).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5170   +/-   ##
=======================================
  Coverage   88.62%   88.62%           
=======================================
  Files         308      308           
  Lines       28059    28059           
  Branches     7598     7598           
=======================================
  Hits        24867    24867           
  Misses       2978     2978           
  Partials      214      214           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@nisargjhaveri
Copy link
Contributor Author

@canova, @julienw, @gregtatum I see you're involved in some similar conversations. Any suggestions or ideas on this? Somewhat related to #5148 as well.

@nisargjhaveri
Copy link
Contributor Author

Here is an example on how I intend to use it. Any suggestions are welcome if there is a better way to do this instead of embedding an iframe.

https://github.com/nisargjhaveri/vscode-android-debug/blob/f6d81aa8e277dfe4b28f79077daac2890937c98f/src/profile-viewer/profileCustomEditor.ts#L91-L150

@julienw
Copy link
Contributor

julienw commented Oct 24, 2024

I believe most people I know are using this feature using window.open, but that said it's not a bad idea.

It would be good to limit it to /from-post-message though, I think it should be possible to do it in _headers. Can you please try that?

My only request when running in an iframe, is being able to open the app in https://profiler.firefox.com/uploaded-recordings/ so that the user could see all uploaded profiles and possibly delete them.

Also please note that we very recently changed the messages due to a problem we discovered, you can check this out in #5148.

@julienw
Copy link
Contributor

julienw commented Oct 24, 2024

Also, I don't know vscode addon programming, but I'm surprised that an iframe loaded from vscode is restricted by the CSP.

@mstange
Copy link
Contributor

mstange commented Oct 24, 2024

It would be good to limit it to /from-post-message though, I think it should be possible to do it in _headers. Can you please try that?

I think our previous service worker used the cached response for / for everything that doesn't match a cached file path, but I'm not sure if that's still the case. If it is, I don't think having different headers for different paths would work.

@julienw
Copy link
Contributor

julienw commented Oct 24, 2024

Wondering, would it be possible for an extension to open a top level navigation page instead of opening in an iframe?

It would be good to limit it to /from-post-message though, I think it should be possible to do it in _headers. Can you please try that?

I think our previous service worker used the cached response for / for everything that doesn't match a cached file path, but I'm not sure if that's still the case. If it is, I don't think having different headers for different paths would work.

Ah that's a good point!

@nisargjhaveri
Copy link
Contributor Author

Wondering, would it be possible for an extension to open a top level navigation page instead of opening in an iframe?

No. VS Code API expects a html string to be shown in a Webview. So we need to embed in an iframe inside that. VS Code's webview itself is also an iframe. So even if it provides API to use url instead of html string, it would still be an iframe and would respect CSP.

I have the same issue, going through the service worker and some other code, I understood that this is essentially a single page app? I'm not sure how defining CSP for sub path would work. Though, I have a very limited understanding and haven't tried yet.

@nisargjhaveri
Copy link
Contributor Author

My only request when running in an iframe, is being able to open the app in https://profiler.firefox.com/uploaded-recordings/ so that the user could see all uploaded profiles and possibly delete them.

About this. I was going to create another issue just now! I was wondering if we should have a way to disable profile uploads. While the feature is super useful, in this context it might not make much sense since the paradigm here is essentially opening a profile trace file inside an editor. I wouldn't expect additional ways to upload specific kind of files from inside VS Code. (Also not sure how we think about uploading non-Firefox profiles to this service, but that is separate)

@nisargjhaveri
Copy link
Contributor Author

@julienw, @mstange any more thoughts on this? I don't think it would be possible to limit only to one endpoint easily.

@julienw julienw self-requested a review November 14, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow embedding the profiler with iframe?
3 participants