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

UI: Fix addon URL escaping in manager #19375

Merged
merged 7 commits into from
Oct 11, 2022

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Oct 6, 2022

The URL injected into the manager for addons were not escaped properly causing them to 404 on some hosting platforms.

I added escaping for these URLs.

@ndelangen ndelangen self-assigned this Oct 6, 2022
@ndelangen ndelangen added the bug label Oct 6, 2022
@ndelangen
Copy link
Member Author

ndelangen commented Oct 6, 2022

Hmm I'm having second thoughts on this remapping to different files thing...
If there are split bundles that might break the references..

Perhaps all I need to do is escape the URL? @tmeasday you said that "the files are there"..
Could you send me a zip file of that broken storybook as it was deployed?

Maybe I can make it work by tweaking the html a bit.

@ndelangen
Copy link
Member Author

OK, I think if found a much better solution!

@ndelangen ndelangen requested review from tmeasday and removed request for shilman October 7, 2022 11:35
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM apart from typo in function name

code/lib/builder-manager/src/utils/files.ts Outdated Show resolved Hide resolved
@shilman shilman changed the title Norbert/sb-735-bug-report-addons-are-not-loading-in UI: Fix addon URL escaping in manager Oct 8, 2022
@ndelangen ndelangen merged commit 4af32ed into next Oct 11, 2022
@ndelangen ndelangen deleted the norbert/sb-735-bug-report-addons-are-not-loading-in branch October 11, 2022 13:33
@IanVS
Copy link
Member

IanVS commented Oct 19, 2022

@ndelangen I think this change might have broken Windows support. See #19534. Or maybe not, but something in alpha.37 caused it to break, and this looks suspicious. Maybe by encoding backslashes, we're not able to convert it to a posix path later on using slash().

@mjcarsjens
Copy link

mjcarsjens commented Oct 20, 2022

slash() is definitely not built to handle URI encoded backslash characters. Would there be any possible issues with just splitting on both / and \\ and then joining with / in sanitizePath()? This could theoretically break usage of \ in UNIX file paths, but I don't really see a use-case for this (even disregarding the fact that this would be a terrible idea wrt cross-platform usage) .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants