Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Oct 11, 2024

Description

The dashboard supports OTLP HTTP and CORS to get telemetry from browser apps. CORS allowed origins are automatically calculated based on resource endpoints.

However, there are some scenarios where automatic CORS endpoints don't work:

  • When browsing to the internal, proxy endpoint for a resource.
  • When a custom domain is added to the host file and pointed to localhost, browsing to the custom domain.

These origins aren't automatically added and so sending telemetry fails.

The fix in this PR is to add manual dashboard CORS configuration to the app host. When the dashboard resource is configured, the app host manual configuration is simply forwarded to the dashboard. If there is no dashboard configuration then the existing logic is used.

Fixes #6249

Fixes customer bug with existing feature. Intended for 9.0.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@JamesNK JamesNK added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Oct 11, 2024
@davidfowl
Copy link
Member

This is a feature....

@JamesNK
Copy link
Member Author

JamesNK commented Oct 11, 2024

This is a feature....

Because it is a new config setting? IMO it's a bug fix for scenarios where browser telemetry feature doesn't work.

If it doesn't meet the bar then it can be moved to post 9.0.

@JamesNK JamesNK added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Oct 15, 2024
@JamesNK JamesNK force-pushed the jamesnk/add-apphost-cors-config-override branch from fbf7f87 to 2e64ef9 Compare October 22, 2024 00:49
@JamesNK
Copy link
Member Author

JamesNK commented Oct 25, 2024

@mitchdenny Can you take a look? The changes are all in the app host.

@mitchdenny
Copy link
Member

Implementations wise I have no problem with this. The only thing that gives me pause is whether it might be better to have some kind of mechanism to add to the computed origins list rather than completely replacing it.

That said, I think that given this only impacts the OTLP endpoint means its probably not much of a security concern. After all most telemetry endpoints are fairly permissive anyway and this is still just locally bound.

We should probably mention this in the threat model?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 29, 2024

The only thing that gives me pause is whether it might be better to have some kind of mechanism to add to the computed origins list rather than completely replacing it.

I think it is simplest to understand if it is a replace. That means setting DOTNET_DASHBOARD_CORS_ALLOWED_ORIGINS on the host is equivilent to setting DOTNET_DASHBOARD_CORS_ALLOWED_ORIGINS on the dashboard.

If it turns out we want a third option we could look at that as a different setting.

We should probably mention this in the threat model?

Sure

@JamesNK JamesNK merged commit e516d82 into main Oct 29, 2024
9 checks passed
@JamesNK JamesNK deleted the jamesnk/add-apphost-cors-config-override branch October 29, 2024 07:57
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AppHost Dashboard CORS Configuration
4 participants