JS: Actually extract .html.erb files. #12190
Conversation
| /** Determine the {@link FileType} for a given file based on its extension only. */ | ||
| public static FileType forFileExtension(File f) { | ||
| String lcExt = StringUtil.lc(FileUtil.extension(f)); | ||
| String lcExt = StringUtil.lc(FileUtil.extension(f)); // TODO: Here, it doesn't recognize .html.erb files |
There was a problem hiding this comment.
Is it helpful to keep this comment in? Unless you plan on changing something in the future, I'd suggest dropping TODO:
There was a problem hiding this comment.
Oops, I forgot to remove that 😅
| } | ||
| // for ERB files we are only interrested in `.html.erb` files | ||
| if (FileUtil.extension(f).equalsIgnoreCase(".erb")) { | ||
| if (!f.getName().endsWith(".html.erb")) { |
There was a problem hiding this comment.
You could simplify this with
if(f.getName().endsWith(".html.erb"))
return true;This code doesn't handle upper case - is this important?
There was a problem hiding this comment.
Your suggestion wouldn't work, because it would let through all .erb files (the super.contains(f, lcExt, config); call returns true for any .erb file).
This code doesn't handle upper case - is this important?
I don't think so, but the existing code handles upper case, so I wanted to keep that.
I'll keep the code as is.
There was a problem hiding this comment.
I don't think handling upper case is important, but I feel we should test things consistently. Having equalsIgnoreCase in combination with a case sensitive endsWith feels wrong.
There was a problem hiding this comment.
Agreed on the logic - didn't see that .erb would have been matched in the following lines.
You could just shove in f.getName().toLowerCase().endsWith(".html.erb")
There was a problem hiding this comment.
Agree that we should do f.getName().toLowerCase() in the second branch - keeps it more obvious and also handles upper case extensions.
CVE-2022-3704: TP
The file-extension check only works on the last part of the extension, so I had to change my implementation.
We already have a filter for which files to extract, so I just put
.erbas an extension we support, but then use the filter to ensure it's only.html.erbfiles that are considered.I didn't test the previous PR for this well enough.
Evaluation on nightly was very unevenful.
Evaluation on a bunch of rails projects was very eventful.
Lots of new results (in
.html.erbfiles), slightly worse performance (from extracting more files).But it looks good to me.