Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add csp headers for embedded apps#18374

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
code-asher wants to merge2 commits intomain
base:main
Choose a base branch
Loading
fromasher/frame-src

Conversation

code-asher
Copy link
Member

I modified the proxy host cache we already had and were using for websocket csp headers to also include the wildcard app host, then used those for frame-src policies.

I did not add frame-ancestors, since if I understand correctly, those would go on the app, and this middleware does not come into play there. Maybe we will want to add it on workspace apps like we do with cors, if we find apps are setting it tonone or something.

Closescoder/internal#684

Comment on lines 1550 to 1555
proxies := []*proxyhealth.ProxyHost{
{
Host: api.AccessURL.Host,
AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.String(), api.AppHostname),
},
}
Copy link
MemberAuthor

@code-ashercode-asherJun 14, 2025
edited
Loading

Choose a reason for hiding this comment

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

I tacked on the primary here, but we could instead add the primary as part of theWorkspaceProxyHostsFn function.

The current method here also includes the primary for AGPL, so if we do that we would also need to make some changes for the AGPL version (assuming wildcard apps and tasks even work with AGPL).

Copy link
Member

Choose a reason for hiding this comment

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

api.AppHostname can be"". We should probably handle empty string, and not add it to the csp header.

Copy link
MemberAuthor

@code-ashercode-asherJun 16, 2025
edited
Loading

Choose a reason for hiding this comment

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

ConvertAppHostForCSP uses the first argument if the second is"", so we end up adding the base host instead (my thinking here was that means we are using path-based apps, so the root domain is the app host).

Copy link
MemberAuthor

@code-ashercode-asherJun 16, 2025
edited
Loading

Choose a reason for hiding this comment

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

Actually I think my reasoning is incorrect because you can use both wildcard and path-based at the same time, right? Hmm...that means we always want to add the access URL? I guessself would cover that one already, but can you use path-based for the regions? Maybe I need to add both there.

Copy link
MemberAuthor

@code-ashercode-asherJun 16, 2025
edited
Loading

Choose a reason for hiding this comment

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

Also I need to use.Host instead of.String() since I think CSP does not like the protocol being in there, could have sworn I fixed that already.

Edit: wait no what am I talking about, apparently my brain is fried today, seems to work either way.

@code-ashercode-asher requested a review fromEmyrkJune 14, 2025 02:46
Copy link
Member

@EmyrkEmyrk left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Have you done any manual QA@code-asher ?

@code-asher
Copy link
MemberAuthor

code-asher commentedJun 16, 2025
edited
Loading

Overall LGTM. Have you done any manual QA@code-asher ?

I checked that the primary's headers get added toworkspace apps misspoke, I mean the dashboard but I have not tested multiple regions (no idea how to set that up) and I have not actually tested an iframe because for the life of me I cannot get the tasks stuff to work locally (also I would need a subdomain to properly test).

Definitely open to ideas on how I can more fully test. Maybe I need to make a proper deployment somewhere.

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

@EmyrkEmyrkEmyrk left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@code-ashercode-asher

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tasks: allow embedding workspace apps in iframes
2 participants
@code-asher@Emyrk

[8]ページ先頭

©2009-2025 Movatter.jp