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

Desktop: Fixes 7434: Encode file path uri in addPluginAssets #7449

Merged
merged 3 commits into from
Dec 18, 2022

Conversation

Wartijn
Copy link
Contributor

@Wartijn Wartijn commented Dec 13, 2022

Encoded all off the special characters, except for /, of the file paths in addPluginAssets so they don't' get a special treatment (e.g. removing everything after a # when loading the stylesheet, like happened in #7434)

I'm not sure how to add tests for this, but I've tested this manually in Linux by changing the id of the current profile so it contains these characters.

@laurent22
Copy link
Owner

I don't think this fix is right. script.src already encodes the data so if it doesn't encode it properly it's because the input is wrong. Most likely those Windows paths should be converted to forward slash beforehand

@Wartijn
Copy link
Contributor Author

Wartijn commented Dec 14, 2022

It's not just on Windows though. If I add a # to the path by changing the profile id to foo#bar nothing after the # will be read in Linux.

image

I've tested it some more, and it seems that only ? and # cause errors, because everything after those characters will be ignored. That seems to make sense, since you won't need to read anything after that when loading a file as stylesheet or script from a url.

@@ -343,6 +347,7 @@
setPercentScroll(event.options.percent);
}

console.log(event.options)
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove

@laurent22
Copy link
Owner

Yes I guess you're right, currently it's handling # and ? in a special way, but in this particular case it shouldn't since it's a local path

@laurent22 laurent22 merged commit 1025222 into laurent22:dev Dec 18, 2022
@laurent22
Copy link
Owner

It's all good now, thanks @Wartijn!

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.

2 participants