Skip to content

Start of fix for #21859, allow users to open files with newlines in the filenames.

Jesse Hall requested to merge jessehall3/gitlab-ce:21859-newline_in_filename into master

What does this MR do?

This is meant serve as a beginning of a fix for issue #21859 (closed).

Currently, users receive a 500 error when attempting to view files with newline characters in their filenames.

This MR resolves the 500 error, users can view files with newline characters in their filenames.

Additionally, when viewing such files, new issues come into play. The "find files" search bar does not accept newlines. The input field for editing a filename does not accept newlines. Further decisions need to be made regarding these new issues. This MR is meant to offer a solution to the 500 error, but does not resolve the issue entirely, further work will be needed.

Details

As discussed in #21859 (closed), when it comes to rendering the views of these files, the problem comes not from retrieving the files themselves, but in rendering their templates, see this error.

In the error message, there are two important things to note:

No route matches {:action=>"show", :controller=>"projects/blob", :id=>"master/Literature Study/1_Core/Enhancing Reuse of Constraint Solutions to Improve\nSymbolic Execution.pd
missing required keys: [:id])

This is confusing to say the least, because looking at the routing configuration for that controller and action, we see that the constraints should permit everything, including strings with newline characters: L68 - 83 in config/routes/repository.rb

scope constraints: { id: /.+/ } do
    scope controller: :blob do
      get '/new/*id', action: :new, as: :new_blob
      post '/create/*id', action: :create, as: :create_blob
      get '/edit/*id', action: :edit, as: :edit_blob
      put '/update/*id', action: :update, as: :update_blob
      post '/preview/*id', action: :preview, as: :preview_blob

      scope path: '/blob/*id', as: :blob do
        get :diff
        get '/', action: :show
        delete '/', action: :destroy
        post '/', action: :create
        put '/', action: :update
      end
    end

Focusing on the constraint /.+/, we realize what the issue actually IS with matching paths with a newline character -- in most flavors of regex, the dot will match everything EXCEPT the newline character, see this explanation in Regex Buddy.

Knowing this, it would seem logical to simply add a multi-line option, /.+/m, to enable matching for newlines. However, Rails does not permit the multi-line option for routing constraints, see L287 in actionpack/lib/action_dispatch/routing/mapper.rb.

The one catch is that we can match newlines by way of an exclusionary rule. In this MR, I used the following:
/[^\0]+/

This will match one or more of everything but the Null character. Excluding the Null character makes sense as I'm assuming it's prohibited at some layer in the application already due to it being a security risk.

That's it?

Yes, simply by changing /.+/ to /[^\0]+/ in one spot, I was immediately able to view and edit these types of files without any further changes.

New Issues

My hope is to be clear that new usability issues arise by enabling the viewing of these files. Two immediate ones I ran into dealt with editing file names and searching for files. When editing files, the user is presented with an input field, as opposed to a text area, so newlines are ignored. Because of this, if a user simply clicks the filename field and then clicks "commit", the filename will be changed (newlines will be eliminated) without any notification to the user. Regarding the "find file" search field, it will also not permit newlines -- though the search function will treat newlines, in existing filenames, as whitespaces. I am sure there are others issues that I failed to notice on my own.

Does this MR meet the acceptance criteria?

Conformity

Performance and Testing

What risks does this change pose? How might it affect the quality/performance of the product?

As mentioned above, there are a number of new issues that arise by making this change, further discussion is needed.

What additional test coverage or changes to tests will be needed?

Yes, additional testing will be needed. With this MR, I added a few routing related tests to validate that paths with newlines will be both recognized by, and constructed with, the relevant routing rules. More thorough testing is necessary.

Will it require cross-browser testing?

Not with this MR, but if changes are ultimately made to the UI, then yes.

Closes #21859 (closed)

Edited by Nick Thomas

Merge request reports