Skip to content

Conversation

@farmaazon
Copy link
Contributor

@farmaazon farmaazon commented Apr 16, 2025

Pull Request Description

Part of the fix for #12800

It is visible in network tab (in this case we don't put additional /)
image

However, as seen in this image, cloud still doesn't handle it: @PabloBuchu is working on that. Nevertheless, this won't hurt and may be merged.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • [ ] Unit tests have been written where possible.
  • [ ] If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@github-actions
Copy link

github-actions bot commented Apr 16, 2025

🧪 Storybook is successfully deployed!

📊 Dashboard:

// Here we always display docs from `src/Main.enso` module
const appliedUrl = new URL(path, 'file:///src')
if (appliedUrl.protocol !== 'file:') {
return Promise.resolve(Ok({ url: path }))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to allow any non-file protocol here? I think users will not need anything other than https/http, and it seems safer not to hotlink images over arbitrary protocols from user-provided content.

Copy link
Contributor

Choose a reason for hiding this comment

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

We used to use enso:// protocol for fetching internal files from the project. Under the hood we check that the requested path is in a subfolder of the project (for security reasons)

I think we can use the same approach here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@kazcw kazcw Apr 16, 2025

Choose a reason for hiding this comment

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

I just copied what is in https://github.com/enso-org/enso/blob/develop/app/gui/src/project-view/components/DocumentationEditor/images.ts#L68

Ok, we should change it there. It would be nice if we could share one implementation of this URL classification/normalization logic, that should probably have a protocol whitelist.

@farmaazon farmaazon added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 16, 2025
@farmaazon farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Apr 17, 2025
@mergify mergify bot merged commit 30e68ac into develop Apr 17, 2025
62 of 64 checks passed
@mergify mergify bot deleted the wip/farmaazon/fix-paths-in-dashboard-doc-editor branch April 17, 2025 11:30
farmaazon added a commit that referenced this pull request Apr 17, 2025
Part of the fix for #12800

It is visible in network tab (in this case we don't put additional `/`)
![image](https://github.com/user-attachments/assets/78a2722f-3708-4a05-a2d8-407e13514ea4)

However, as seen in this image, cloud still doesn't handle it: @PabloBuchu is working on that. Nevertheless, this won't hurt and may be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

-gui CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge g-dashboard

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants