Improve file handling of special characters in WebIDE
What does this MR do?
This fixes #54376 (closed) by escaping the hash characters in the Web IDE.
If a hash (#) was added to a file name it would crash the Web IDE.
What are the relevant issue numbers?
Does this MR meet the acceptance criteria?
Since this is my first MR, I may need help in crafting it. I have put my comments in italics where I think things are at.
-
Changelog entry added, if necessary after reading the guide, this didnt look necessary -
Documentation created/updated via this MR no change in functionality -
Documentation reviewed by technical writer or follow-up review issue created N/A -
Tests added for this feature/bug Should I add tests for this bug? If so I can. -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer
Merge request reports
Activity
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
added 1 commit
- d1faa1e8 - #54376 (closed) Dont use as it's deprecated.
- Resolved by Paul Slaughter
mentioned in issue #54376 (closed)
added Community contribution label
assigned to @pslaughter
added 1st contribution label
marked the checklist item Conforms to the code review guidelines as incomplete
marked the checklist item Conforms to the code review guidelines as completed
marked the checklist item Conforms to the code review guidelines as incomplete
marked the checklist item Conforms to the code review guidelines as completed
marked the checklist item Tested in all supported browsers as completed
marked the checklist item Tested in all supported browsers as incomplete
Thanks for this contribution @hiddentiger! I also really appreciate your comments
I'll be taking a look at this today and get this back to you shortly.
@hiddentiger, this is an interesting problem. Thanks for going after it! I did a bit of investigation on the frontend and I think I discovered the root issue...
Whenever a file is loaded or clicked we use it's
url
property to fetch it's data. Here's an example infile_row.vue
component. Coincidentally, I was actually just working on improving our file decorator, so I knew where this was set. As you'd expect, thisurl
property is never escaped.So how should we escape this URL?
As you mentioned in an earlier comment we can't simply use
encodeURIComponent()
because we lose the/
. WDYT of encoding and then "decoding" only the/
? Something like:const escapeFileUrl = fileUrl => encodeURIComponent(fileUrl).replace('%2F', '/');
Here's a patch to your branch that fixes the FE:
25727_ide_special_char_support.patch
Could you apply this patch locally and let me know if it works for you?
If you'd like to keep these changes in this MR, you should update the title and description to reflect the new scope. Please let me know if you have any questions
assigned to @hiddentiger
added 1 commit
- e69b4320 - 25727 Improve special character support in webide
changed title from WIP: #54376 (closed) Escape hash in link generation and in frontend to WIP: #54376 (closed) Improve file handling of special characters in WebIDE
added 12 commits
-
e69b4320...4b0036b8 - 10 commits from branch
gitlab-org:master
- 433f9d23 - #54376 (closed) encode url and ensure slashes are kept
- d3d07526 - Merge branch 'master' into feature/webide_escaping
-
e69b4320...4b0036b8 - 10 commits from branch
added 517 commits
-
d3d07526...8a59c9fd - 516 commits from branch
gitlab-org:master
- fd53fc68 - Merge branch 'master' of https://gitlab.com/gitlab-org/gitlab-ce into feature/webide_escaping
-
d3d07526...8a59c9fd - 516 commits from branch
@pslaughter I have applied your patch and I am still getting the issue.
The web request to retrieve the content is still dropping the hashes.
I have also identified another issue, the web ide tree view also generates the wrong url which means that navigating to this file does not work either.
Edited by Kieran Andrews@pslaughter The file
app/assets/javascripts/ide/stores/workers/files_decorator_worker.js
was moved so I applied the patch to the new location.I am still having the issue and it looks like someone from the frontend team was assigned to the main issue as well.
Edited by Kieran Andrewsadded 1 commit
- 38d36c46 - Resolve merge conflict by applying patch to new file
- Resolved by Kieran Andrews
- Resolved by Kieran Andrews
- Resolved by Kieran Andrews
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
Thanks @hiddentiger for working on these updates!
I am still having the issue...
I tested on this branch locally and it works for me. Is it still not working on your end?
...it looks like someone from the frontend team was assigned to the main issue as well.
This looks like a scheduling miscommunication. If you think you have time for this, I'd love to work with you to get this through the finish line (April 7th). Otherwise, we'll have someone on our team follow the "coach will finish" process since we want this resolved by our next release.
Thanks again for taking the initiative on this!
Thanks @pslaughter for the update. Now that I know what is happening I will continue working on this.
@pslaughter to set expectations and to help with planning, I am looking to work on this early next week.
@pslaughter Can you help with this test failure? https://gitlab.com/hiddentiger/gitlab-ce/-/jobs/181371606
I am not familiar enough with the testing framework to know what is going wrong. It looks like the "sample" element should not be there.
Here's a patch that should fix the test failure:
What happened?
- It looks like
.replace('%2F', '/')
in JavaScript only replaces the first occurrence of the string... Sorry about that! . The patch changes this to.replace(/%2F/g, '/')
. - An unexpected trailing slash was left in the line:
url: `/${projectId}/blob/${branchId}/-/${escapeFileUrl(path)}/`,
Thanks @hiddentiger! Let me know if you need any help resolving the other discussions.
- It looks like
- Resolved by Kieran Andrews
@hiddentiger, could you generate a CHANGELOG entry for this merge request and credit yourself. Thanks!
added 842 commits
-
448121be...535bd574 - 840 commits from branch
gitlab-org:master
- 8b1858ab - Merge branch 'master' into feature/webide_escaping
- 2e2027d2 - Merge branch 'feature/webide_escaping' of gitlab.com:hiddentiger/gitlab-ce...
-
448121be...535bd574 - 840 commits from branch
- Resolved by Kieran Andrews
added 219 commits
-
9ffec0c0...e08c693b - 217 commits from branch
gitlab-org:master
- b33eb46c - Merge remote-tracking branch 'origin/master' into feature/webide_escaping
- ebcf94d7 - Fix changelog format.
-
9ffec0c0...e08c693b - 217 commits from branch