From ebbcc17f243337581536bd1f34dc7d6b5712ebb9 Mon Sep 17 00:00:00 2001 From: Kieran Andrews Date: Wed, 3 Apr 2019 11:38:46 +0000 Subject: [PATCH] Merge branch 'feature/webide_escaping' of gitlab.com:hiddentiger/gitlab-ce into feature/webide_escaping --- app/assets/javascripts/ide/lib/files.js | 6 ++++-- app/helpers/blob_helper.rb | 6 +++++- .../unreleased/feature-webide_escaping.yml | 5 +++++ spec/frontend/ide/lib/files_spec.js | 17 +++++++++-------- spec/helpers/blob_helper_spec.rb | 13 +++++++++++++ 5 files changed, 36 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/feature-webide_escaping.yml diff --git a/app/assets/javascripts/ide/lib/files.js b/app/assets/javascripts/ide/lib/files.js index 5dfba8fe531..df100f753d7 100644 --- a/app/assets/javascripts/ide/lib/files.js +++ b/app/assets/javascripts/ide/lib/files.js @@ -1,6 +1,8 @@ import { viewerInformationForPath } from '~/vue_shared/components/content_viewer/lib/viewer_utils'; import { decorateData, sortTree } from '../stores/utils'; +export const escapeFileUrl = fileUrl => encodeURIComponent(fileUrl).replace(/%2F/g, '/'); + export const splitParent = path => { const idx = path.lastIndexOf('/'); @@ -45,7 +47,7 @@ export const decorateFiles = ({ id: path, name, path, - url: `/${projectId}/tree/${branchId}/-/${path}/`, + url: `/${projectId}/tree/${branchId}/-/${escapeFileUrl(path)}/`, type: 'tree', parentTreeUrl: parentFolder ? parentFolder.url : `/${projectId}/tree/${branchId}/`, tempFile, @@ -81,7 +83,7 @@ export const decorateFiles = ({ id: path, name, path, - url: `/${projectId}/blob/${branchId}/-/${path}`, + url: `/${projectId}/blob/${branchId}/-/${escapeFileUrl(path)}`, type: 'blob', parentTreeUrl: fileFolder ? fileFolder.url : `/${projectId}/blob/${branchId}`, tempFile, diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 3e1bb9af5cc..d6fff1c36da 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -19,10 +19,14 @@ module BlobHelper def ide_edit_path(project = @project, ref = @ref, path = @path, options = {}) segments = [ide_path, 'project', project.full_path, 'edit', ref] - segments.concat(['-', path]) if path.present? + segments.concat(['-', encode_ide_path(path)]) if path.present? File.join(segments) end + def encode_ide_path(path) + url_encode(path).gsub('%2F', '/') + end + def edit_blob_button(project = @project, ref = @ref, path = @path, options = {}) return unless blob = readable_blob(options, path, project, ref) diff --git a/changelogs/unreleased/feature-webide_escaping.yml b/changelogs/unreleased/feature-webide_escaping.yml new file mode 100644 index 00000000000..88fa1bd948e --- /dev/null +++ b/changelogs/unreleased/feature-webide_escaping.yml @@ -0,0 +1,5 @@ +--- +title: Fixed bug with hashes in urls in WebIDE +merge_request: 54376 +author: Kieran Andrews +type: fixed diff --git a/spec/frontend/ide/lib/files_spec.js b/spec/frontend/ide/lib/files_spec.js index fe791aa2b74..aa1fa0373db 100644 --- a/spec/frontend/ide/lib/files_spec.js +++ b/spec/frontend/ide/lib/files_spec.js @@ -1,5 +1,5 @@ import { viewerInformationForPath } from '~/vue_shared/components/content_viewer/lib/viewer_utils'; -import { decorateFiles, splitParent } from '~/ide/lib/files'; +import { decorateFiles, splitParent, escapeFileUrl } from '~/ide/lib/files'; import { decorateData } from '~/ide/stores/utils'; const TEST_BRANCH_ID = 'lorem-ipsum'; @@ -20,7 +20,7 @@ const createEntries = paths => { id: path, name, path, - url: createUrl(`/${TEST_PROJECT_ID}/${type}/${TEST_BRANCH_ID}/-/${path}`), + url: createUrl(`/${TEST_PROJECT_ID}/${type}/${TEST_BRANCH_ID}/-/${escapeFileUrl(path)}`), type, previewMode: viewerInformationForPath(path), parentPath: parent, @@ -28,7 +28,7 @@ const createEntries = paths => { ? parentEntry.url : createUrl(`/${TEST_PROJECT_ID}/${type}/${TEST_BRANCH_ID}`), }), - tree: children.map(childName => jasmine.objectContaining({ name: childName })), + tree: children.map(childName => expect.objectContaining({ name: childName })), }; return acc; @@ -36,10 +36,10 @@ const createEntries = paths => { const entries = paths.reduce(createEntry, {}); - // Wrap entries in jasmine.objectContaining. + // Wrap entries in expect.objectContaining. // We couldn't do this earlier because we still need to select properties from parent entries. return Object.keys(entries).reduce((acc, key) => { - acc[key] = jasmine.objectContaining(entries[key]); + acc[key] = expect.objectContaining(entries[key]); return acc; }, {}); @@ -47,13 +47,14 @@ const createEntries = paths => { describe('IDE lib decorate files', () => { it('creates entries and treeList', () => { - const data = ['app/assets/apples/foo.js', 'app/bugs.js', 'README.md']; + const data = ['app/assets/apples/foo.js', 'app/bugs.js', 'app/#weird#file?.txt', 'README.md']; const expectedEntries = createEntries([ - { path: 'app', type: 'tree', children: ['assets', 'bugs.js'] }, + { path: 'app', type: 'tree', children: ['assets', '#weird#file?.txt', 'bugs.js'] }, { path: 'app/assets', type: 'tree', children: ['apples'] }, { path: 'app/assets/apples', type: 'tree', children: ['foo.js'] }, { path: 'app/assets/apples/foo.js', type: 'blob', children: [] }, { path: 'app/bugs.js', type: 'blob', children: [] }, + { path: 'app/#weird#file?.txt', type: 'blob', children: [] }, { path: 'README.md', type: 'blob', children: [] }, ]); @@ -64,7 +65,7 @@ describe('IDE lib decorate files', () => { }); // Here we test the keys and then each key/value individually because `expect(entries).toEqual(expectedEntries)` - // was taking a very long time for some reason. Probably due to large objects and nested `jasmine.objectContaining`. + // was taking a very long time for some reason. Probably due to large objects and nested `expect.objectContaining`. const entryKeys = Object.keys(entries); expect(entryKeys).toEqual(Object.keys(expectedEntries)); diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb index 2bc3933809f..6808ed86c9a 100644 --- a/spec/helpers/blob_helper_spec.rb +++ b/spec/helpers/blob_helper_spec.rb @@ -230,5 +230,18 @@ describe BlobHelper do expect(helper.ide_edit_path(project, "master", "")).to eq("/gitlab/-/ide/project/#{project.namespace.path}/#{project.path}/edit/master") end + + it 'escapes special characters' do + Rails.application.routes.default_url_options[:script_name] = nil + + expect(helper.ide_edit_path(project, "testing/#hashes", "readme.md#test")).to eq("/-/ide/project/#{project.namespace.path}/#{project.path}/edit/testing/#hashes/-/readme.md%23test") + expect(helper.ide_edit_path(project, "testing/#hashes", "src#/readme.md#test")).to eq("/-/ide/project/#{project.namespace.path}/#{project.path}/edit/testing/#hashes/-/src%23/readme.md%23test") + end + + it 'does not escape "/" character' do + Rails.application.routes.default_url_options[:script_name] = nil + + expect(helper.ide_edit_path(project, "testing/slashes", "readme.md/")).to eq("/-/ide/project/#{project.namespace.path}/#{project.path}/edit/testing/slashes/-/readme.md/") + end end end