Skip to content
Snippets Groups Projects

Improve file handling of special characters in WebIDE

Merged Kieran Andrews requested to merge hiddentiger/gitlab-ce:feature/webide_escaping into master

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?

#54376 (closed)

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.

Edited by Paul Slaughter

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Kieran Andrews marked as a Work In Progress

    marked as a Work In Progress

  • Kieran Andrews changed the description

    changed the description

  • Kieran Andrews added 1 commit

    added 1 commit

    Compare with previous version

  • Kieran Andrews added 1 commit

    added 1 commit

    Compare with previous version

  • mentioned in issue #54376 (closed)

  • Kieran Andrews added 1 commit

    added 1 commit

    • a0f16f5b - ensure argument is not modified

    Compare with previous version

  • Author Contributor

    @pslaughter Are you able to help out as a merge request coach?

    Edited by Kieran Andrews
  • Kieran Andrews changed the description

    changed the description

  • Kieran Andrews marked the checklist item Conforms to the code review guidelines as incomplete

    marked the checklist item Conforms to the code review guidelines as incomplete

  • Kieran Andrews marked the checklist item Conforms to the code review guidelines as completed

    marked the checklist item Conforms to the code review guidelines as completed

  • Kieran Andrews marked the checklist item Conforms to the code review guidelines as incomplete

    marked the checklist item Conforms to the code review guidelines as incomplete

  • Kieran Andrews marked the checklist item Conforms to the code review guidelines as completed

    marked the checklist item Conforms to the code review guidelines as completed

  • Kieran Andrews marked the checklist item Tested in all supported browsers as completed

    marked the checklist item Tested in all supported browsers as completed

  • Kieran Andrews marked the checklist item Tested in all supported browsers as incomplete

    marked the checklist item Tested in all supported browsers as incomplete

  • Thanks for this contribution @hiddentiger! I also really appreciate your comments :muscle:

    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 in file_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, this url 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 :smile:

  • Kieran Andrews added 1 commit

    added 1 commit

    • e69b4320 - 25727 Improve special character support in webide

    Compare with previous version

  • Kieran Andrews 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

    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

  • Kieran Andrews added 12 commits

    added 12 commits

    Compare with previous version

  • Kieran Andrews added 517 commits

    added 517 commits

    Compare with previous version

  • Author Contributor

    @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
  • Author Contributor

    @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 Andrews
  • Kieran Andrews added 1 commit

    added 1 commit

    • 38d36c46 - Resolve merge conflict by applying patch to new file

    Compare with previous version

  • Kieran Andrews added 1 commit

    added 1 commit

    Compare with previous version

  • 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! :bow:

    /cc @sbigelow @andr3

  • Author Contributor

    Thanks @pslaughter for the update. Now that I know what is happening I will continue working on this.

  • Author Contributor

    @pslaughter to set expectations and to help with planning, I am looking to work on this early next week.

  • Kieran Andrews added 1 commit

    added 1 commit

    Compare with previous version

  • Author Contributor

    @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:

    fix_ide_lib_files.patch

    What happened?

    • It looks like .replace('%2F', '/') in JavaScript only replaces the first occurrence of the string... Sorry about that! :face_palm:. 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. :thumbsup:

  • Kieran Andrews added 842 commits

    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...

    Compare with previous version

  • Kieran Andrews added 1 commit

    added 1 commit

    • 8b947530 - Ensure escape will replace all occurrences

    Compare with previous version

  • Kieran Andrews added 1 commit

    added 1 commit

    Compare with previous version

  • Kieran Andrews added 219 commits

    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.

    Compare with previous version

  • Please register or sign in to reply
    Loading